[Webkit-unassigned] [Bug 69107] Webkit API for compositor

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Oct 6 15:08:42 PDT 2011


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





--- Comment #17 from Antoine Labour <piman at chromium.org>  2011-10-06 15:08:42 PST ---
(In reply to comment #15)
> (From update of attachment 109899 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109899&action=review
> 
> Very close... sorry, I was ready to R+ this, but then I found more things.  Nothing at all major.
> 
> > Source/WebKit/chromium/public/WebContentLayer.h:51
> > +    // Sets whether the layer draws it content when compositing.
> 
> nit: "draws it" -> "draws its"

Done.

> 
> > Source/WebKit/chromium/public/WebContentLayer.h:53
> > +    WEBKIT_EXPORT bool drawsContent();
> 
> nit: make this function const?  on WebLayer, similar getters are const methods.

Done.

> 
> > Source/WebKit/chromium/public/WebContentLayer.h:62
> > +    WEBKIT_EXPORT WebFloatRect dirtyRect();
> 
> nit: make this function const?

Done.

> 
> i'd probably put a new line separating setNeedsDisplay from dirtyRect since they
> are not a setter/getter pair.

Done.

> 
> nit: it might be nice to converge on fewer terms here and to reuse existing terms.
> e.g., instead of setNeedsDisplay, how about invalidateRect?  see WebWidgetClient.
> instead of dirtyRect, how about invalidationRect or invalidationBounds?

Done. (invalidateRect/invalidate/invalidRect)

> 
> > Source/WebKit/chromium/public/WebLayerTreeView.h:75
> > +    // later time. Before the compositing pass happens, WebContentLayers will be
> 
> Can you more strongly say "Before composite() returns, WebContentsLayers will be..."?

Done. I mentioned that it's that way in the single thread case. In the out-of-thread compositing case, it's not.
> 
> > Source/WebKit/chromium/public/WebLayerTreeViewClient.h:40
> > +    // Applies a scroll delta to the root layer. This is trigerred by events
> 
> nit: trigerred

Done.

> 
> > Source/WebKit/chromium/public/WebLayerTreeViewClient.h:50
> > +    virtual void didRecreateGraphicsContext(bool success) = 0;
> 
> in the comments you say "rebinding", but in the method name you use the term "recreate"
> are those supposed to mean the same thing?  should they perhaps use the same word?

Done.

> 
> > Source/WebKit/chromium/public/WebTransformationMatrix.h:32
> > +#include "third_party/skia/include/utils/SkMatrix44.h"
> 
> you should be able to just #include <SkMatrix44.h> here.  see WebImage.h for reference.

Well, in the SkBitmap case, it's coming from skia/include/core, which is a dependent setting exported from skia/skia.gyp.
However, SkMatrix44 is in skia/include/utils, which is not. I explicitly added it to WebKit.gyp. If we want to, we could add it to skia/skia.gyp and then clean this after it rolls (but that'd add a useless include path to ~8000 .cc files in chrome and WebCore, which I don't think is desirable).

> 
> > Source/WebKit/chromium/public/WebTransformationMatrix.h:51
> > +#if WEBKIT_IMPLEMENTATION
> 
> nit: we usually make the WEBKIT_IMPLEMENTATION section be the very last bit
> of the "public:" section.
> 
> > Source/WebKit/chromium/public/WebTransformationMatrix.h:69
> > +    double m_data[4][4];
> 
> had you considered just implementing this class as a SkMatrix44?  maybe it should
> even just extend from SkMatrix44?

I axed that class, passing SkMatrix44 to WebLayer directly. Simpler, and fewer copies to get the data through.

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



More information about the webkit-unassigned mailing list