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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Oct 5 21:26:49 PDT 2011


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

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.

More information about the webkit-unassigned mailing list