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

[bugzilla-daemon at webkit.org]

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


Darin Fisher (:fishd, Google) <fishd at chromium.org> changed:

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




--- Comment #15 from Darin Fisher (:fishd, Google) <fishd at chromium.org>  2011-10-05 21:26:49 PST ---
(From update of attachment 109899)
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?

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