[webkit-reviews] review denied: [Bug 87920] [chromium] Software compositor initialization and base classes : [Attachment 144978] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Alexandre Elias
<aelias at 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 at 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