[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 bugzilla-daemon at webkit.org
Mon Sep 12 21:01:31 PDT 2011

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

Attachment 107132: Patch

------- 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 =
> +    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?

More information about the webkit-reviews mailing list