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

[bugzilla-daemon at webkit.org]

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





--- Comment #14 from Antoine Labour <piman at chromium.org>  2011-10-05 19:01:04 PST ---
(In reply to comment #11)
> (From update of attachment 109719 [details])
> 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

Done.

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

As mentioned in our discussion, it can easily make sense to have a single WebLayerClient for a whole tree, and one WebContentLayerClient for each WebContentLayer. Having inheritance there makes it harder. I've kept them separate for now.

I'm actually hoping at some point to be able to remove the need for a WebLayerClient (and passing the "needs commit" information through the compositor / WebLayerTreeView instead of through the layer tree), but that needs significant refactoring in the LayerChromium and friends, so that's for another CL.

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

Done.

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

There is a difference, and it's not what I would call subtle. paintContents is to update the contents of the layer. drawsContent is to indicate whether or not the layer should be drawn when the compositing pass occurs.
That being said, I removed paintContents from the visible client API and made it a settable value on the WebContentLayer - to be more consistent with the rest. I want to push a similar change lower in the stack but it's another significant refactoring, so that's also for another CL.

In any case, I added some doc/comments.

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

Done.

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

I renamed as notifyNeedsCompositing, and added some doc.

> 
> > Source/WebKit/chromium/public/WebLayerTreeView.h:42
> > +struct WebCompositorSettings {
> 
> nit: one header file per class/struct.

Moved as WebLayerTreeView::Settings

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

Done.

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

Done.

> 
> perhaps it would make more sense for WebCompositorSettings to be named WebLayerTreeView::Settings?
Done.

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

As discussed, WebCompositor maps to the compositor-thread API, whereas WebLayerTreeView maps to the main thread API, so keeping them separate is desirable.

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

Done.

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

Done.

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

Done.

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

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

I switched it to use SkMatrix44 so we don't depend on Chrome types.

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

Done.

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

I renamed notifyNeedsCommit to notifyNeedsCompositing. I think that indicates properly the intent.
The internal API could be renamed, but I believe that trickles all the way to WebCore, so that's for another CL.

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