[webkit-reviews] review denied: [Bug 69107] Webkit API for compositor : [Attachment 109899] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Antoine Labour
<piman at chromium.org>'s request for review:
Bug 69107: Webkit API for compositor
https://bugs.webkit.org/show_bug.cgi?id=69107

Attachment 109899: Patch
https://bugs.webkit.org/attachment.cgi?id=109899&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
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"

> Source/WebKit/chromium/public/WebContentLayer.h:53
> +    WEBKIT_EXPORT bool drawsContent();

nit: make this function const?	on WebLayer, similar getters are const methods.


> Source/WebKit/chromium/public/WebContentLayer.h:62
> +    WEBKIT_EXPORT WebFloatRect dirtyRect();

nit: make this function const?

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

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?

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

> Source/WebKit/chromium/public/WebLayerTreeViewClient.h:40
> +    // Applies a scroll delta to the root layer. This is trigerred by events


nit: trigerred

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

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

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