[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