[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





--- Comment #9 from Antoine Labour <piman at chromium.org>  2011-09-13 11:35:38 PST ---
(In reply to comment #7)
> (From update of attachment 107132 [details])
> 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.

Done (m_delegate).

> 
> > 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/

Done.

> 
> > 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.

Done. I removed it and the compositing layout tests still pass.

> 
> > 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()

Done. It may get a little confusing because in GraphicsLayerChromium, the CCLayerDelegate overrides hide the GraphicsLayers's versions, but semantically it's consistent.

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