[Webkit-unassigned] [Bug 67750] Create a delegate class to help cleanly isolate the chromium compositor API

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=67750


James Robinson <jamesr at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #107132|review?                     |review-
               Flag|                            |




--- Comment #7 from James Robinson <jamesr at chromium.org>  2011-09-12 21:01:32 PST ---
(From update of attachment 107132)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.