[webkit-reviews] review denied: [Bug 67750] Create a delegate class to help cleanly isolate the chromium compositor API : [Attachment 107132] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Antoine Labour
<piman at chromium.org>'s request for review:
Bug 67750: Create a delegate class to help cleanly isolate the chromium
compositor API
https://bugs.webkit.org/show_bug.cgi?id=67750

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

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


This change looks really good - R-'ing for some naming nitpicks and because of
some unsafe code in GraphicsLayerChromium (the old code was safe for the
specific case of video layers, but still skeevy).

I also think that having the LayerChromium back pointer be called 'm_owner'
doesn't really make much sense with the new type.  We use m_delegate elsewhere
in WebKit.  It might also make sense to call it CCLayerClient and have the
pointer be m_client but I leave it to up to you.

> Source/WebCore/ChangeLog:12
> +	   No new tests. (OOPS!)

There's an SVN presubmit hook that rejects checkins with this line.  You should
remove this line and instead say that these changes don't change behavior and
the code is covered by tests in compositing/

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:704
> +bool GraphicsLayerChromium::paintingGoesToWindow() const
> +{
> +    RenderLayerBacking* backing =
static_cast<RenderLayerBacking*>(client());
> +    return !backing || backing->paintingGoesToWindow();
> +}

this downcast isn't actually safe, we do create GraphicsLayerChromium instances
with clients that are not RenderLayerBackings - for instance, for overflow
controls - and we may do more of this in the future. I'm pretty sure you can
kill this completely - see the comments for VideoLayerChromium.

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:62
> +    virtual bool getDrawsContent() const = 0;
> +    virtual bool getPreserves3D() const = 0;

in WebKit style, simple getters do not have the 'get' prefix - so these should
be drawsContent() and preserves3D()

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:63
> +    virtual bool paintingGoesToWindow() const = 0;

I think you can (and should) remove this

> Source/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:82
> +    if (!m_contentsDirty || !m_owner || m_owner->paintingGoesToWindow())

the paintingGoesToWindow() check is old and I'm pretty sure it was never
necessary. can you verify our tests pass without this and remove it?