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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Oct 5 13:00:14 PDT 2011


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 109719: Patch
https://bugs.webkit.org/attachment.cgi?id=109719&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=109719&action=review


> Source/WebKit/chromium/public/WebContentLayer.h:41
> +    static WebContentLayer create(WebLayerDelegate*,
WebContentLayerDelegate*);

nit: new line after static function block

Perhaps WebContentLayerDelegate should extend from WebLayerDelegate?
Notice how WebViewClient extends from WebWidgetClient and WebView
extends from WebWidget.

This requires virtual inheritance, but the upside is that the code
becomes a bit cleaner.

> Source/WebKit/chromium/public/WebContentLayerDelegate.h:34
> +class WebContentLayerDelegate {

nit: we use the suffix Client instead of Delegate.

can we rename to WebContentLayerClient and WebLayerClient?

> Source/WebKit/chromium/public/WebContentLayerDelegate.h:36
> +    virtual bool drawsContent() const = 0;

is there some relationship between drawsContent and paintContents?  "draw"
and "paint" sound similar.  should we use the same term?  or is there some
subtle distinction you are trying to make here?

if drawsContent is a predicate to decide if paintContents should be called,
then how about renaming drawsContent to canPaintContents?

> Source/WebKit/chromium/public/WebLayer.h:42
> +    static WebLayer create(WebLayerDelegate*);

nit: new line after static function block

> Source/WebKit/chromium/public/WebLayerDelegate.h:33
> +    virtual void notifyNeedsCommit() = 0;

what does this function mean?  can you document it a bit?
what is the implementer supposed to do in response to this
notification?

> Source/WebKit/chromium/public/WebLayerTreeView.h:42
> +struct WebCompositorSettings {

nit: one header file per class/struct.

> Source/WebKit/chromium/public/WebLayerTreeView.h:44
> +	       : acceleratePainting(false)

nit: indentation

> Source/WebKit/chromium/public/WebLayerTreeView.h:58
> +    static WebLayerTreeView create(WebLayerTreeViewDelegate*, const
WebLayer& root, const WebCompositorSettings&);

nit: add a new line after this static function

perhaps it would make more sense for WebCompositorSettings to be named
WebLayerTreeView::Settings?
or, if WebLayerTreeView is the top-level thing, then how does it differ from
WebCompositor?	does
a WebCompositor own a WebLayerTreeView, or should they be merged together into
one thing?

> Source/WebKit/chromium/public/WebLayerTreeView.h:71
> +    WEBKIT_EXPORT void composite();

perhaps you should document the side-effects of composite().  is it in the
context of this function that WebLayerClient methods will get invoked?

> Source/WebKit/chromium/public/WebLayerTreeView.h:72
> +    WEBKIT_EXPORT void setViewport(const WebSize& viewportSize);

nit: setViewportSize

> Source/WebKit/chromium/public/WebLayerTreeViewDelegate.h:34
> +class WebLayerTreeViewDelegate {

nit: *Client

> Source/WebKit/chromium/public/WebLayerTreeViewDelegate.h:40
> +    virtual void scheduleComposite() = 0;

it would be good to document the processing model.  for example, see
the comments in WebWidgetClient.h for the scheduleComposite method there.

> Source/WebKit/chromium/public/WebTransformationMatrix.h:32
> +#include <ui/gfx/transform.h>

includes like this create a circular dependency between WebKit and Chromium.
we should make sure that we are happy never moving <ui/gfx/transform.h>.
at least let Ben Goodger know about this.  i suspect this is probably worth
while to do, just as it was worth while to add the dependency on
<ui/gfx/rect.h>

> Source/WebKit/chromium/public/WebTransformationMatrix.h:52
> +#if WEBKIT_IMPLEMENTATION

some of the functions in here seem like they would possibly
benefit from not being inlined.

> Source/WebKit/chromium/src/WebContentLayerImpl.cpp:75
> +void WebContentLayerImpl::notifySyncRequired()

looks like there is a terminology change here.	from notifySyncRequired
to notifyNeedsCommit.  should we try to change one to match the other?


More information about the webkit-reviews mailing list