[prev in list] [next in list] [prev in thread] [next in thread] 

List:       webkit-reviews
Subject:    [webkit-reviews] review denied: [Bug 87920] [chromium] Software compositor initialization and base c
From:       bugzilla-daemon () webkit ! org
Date:       2012-05-31 22:40:28
Message-ID: 20120531224028.C30D9131B6FB () lists ! macosforge ! org
[Download RAW message or body]

James Robinson <jamesr@chromium.org> has denied Alexandre Elias
<aelias@chromium.org>'s request for review:
Bug 87920: [chromium] Software compositor initialization and base classes
https://bugs.webkit.org/show_bug.cgi?id=87920

Attachment 144978: Patch
https://bugs.webkit.org/attachment.cgi?id=144978&action=review

------- Additional Comments from James Robinson <jamesr@chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=144978&action=review


This does end up being pretty nice, I think it'll definitely work out.	Some
things need addressing of the more nitpicky variety.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:252
> +						PassRefPtr<CCGraphicsContext>
context,

Why does LRC need a CCGraphicsContext? It seems to me like it'd make more sense
for whoever constructs a LRC to have already extracted the GraphicsContext3D
and pass that in, since they've clearly already decided to use the
GC3D-specific CCRenderer implementation.  I don't think we should need any
changes in the implementation of any LayerRendererChromium functions due to
CCGraphicsContext

>> Source/WebCore/platform/graphics/chromium/ManagedTexture.cpp:126
>> +	    // ASSERT(false);
> 
> How come the assert is commented out here, and in a number of other places?
> 
> Also, ASSERT(false) is generally done via ASSERT_NOT_REACHED() I think, or
assert on opposite thing as in the if, before doing the if()return.

What Dana says is truth. Also, normally we just ASSERT() our invariants and
keep going, we don't add a release mode branch for it if we expect it to be a
"can never happen" scenario. If we want to make sure we explicitly crash
instead of potentially doing weird things we add CRASH() instead of just
return, but that's rare

> Source/WebCore/platform/graphics/chromium/cc/CCGraphicsContext.h:32
> +#if USE(ACCELERATED_COMPOSITING)

I'd not bother with this guard - we're in the chromium compositor directory, so
obviously we have accelerated_compositing enabled. We can't even compile the
chromium port these days without USE(ACCELERATED_COMPOSITING) and I don't think
we will ever want to, so having the guard is just line noise

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:552
> +    if (!context3d) {
> +	   ASSERT(false);
> +	   return false;

same comments here RE assert()s
_______________________________________________
webkit-reviews mailing list
webkit-reviews@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-reviews
[prev in list] [next in list] [prev in thread] [next in thread]