Bug 69107 – Webkit API for compositor

• Component: New Bugs
• Status: RESOLVED FIXED

[Antoine Labour]

Webkit API for compositor

[Antoine Labour]

Created attachment 109216 [details]
Patch

[Antoine Labour]

I exposed most of LayerChromium, ContentLayerChromium and CCLayerTreeHost that would be needed to build UI, modeled on WebNode (e.g. WebLayer is both a smart pointer and directly exposes the API).

Currently this API can only be called on the WebKit thread, until I rework a couple of things in https://bugs.webkit.org/show_bug.cgi?id=69048

[James Robinson]

Comment on attachment 109216 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109216&action=review

FYI Darin and I sketched up the input stuff in more detail and as a result https://bugs.webkit.org/show_bug.cgi?id=69117 changes the threading around a bit. I hope this doesn't complicate your design too much, I think it's actually a bit simpler to reason about with the thread managed externally to WebKit.

> Source/WebKit/chromium/public/WebContentLayer.h:48
> +    WebContentLayer(const WebContentLayer& l) : WebLayer(l) { }

webkit generally doesn't like 1-character variable names. in addition, in the font that the webkit review tool uses the glyphs for '1' (one) and 'l' (ell) are pixel-for-pixel identical :'(. can you call this 'layer' instead or something like that?

[Antoine Labour]

Created attachment 109368 [details]
Patch

[Antoine Labour]

(In reply to comment #3)
> (From update of attachment 109216 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109216&action=review
> 
> FYI Darin and I sketched up the input stuff in more detail and as a result https://bugs.webkit.org/show_bug.cgi?id=69117 changes the threading around a bit. I hope this doesn't complicate your design too much, I think it's actually a bit simpler to reason about with the thread managed externally to WebKit.

I removed the notion of thread for now. It wasn't working yet anyway. I'll follow up in a later CL.

> 
> > Source/WebKit/chromium/public/WebContentLayer.h:48
> > +    WebContentLayer(const WebContentLayer& l) : WebLayer(l) { }
> 
> webkit generally doesn't like 1-character variable names. in addition, in the font that the webkit review tool uses the glyphs for '1' (one) and 'l' (ell) are pixel-for-pixel identical :'(. can you call this 'layer' instead or something like that?

Done.

[James Robinson]

Comment on attachment 109368 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109368&action=review

> Source/WebKit/chromium/public/WebContentLayer.h:37
> +namespace WebCore { class ContentLayerChromium; }

you shouldn't leak any WebCore things in the header. does this just need to be in a #if WEBKIT_IMPLEMENTATION guard?

> Source/WebKit/chromium/public/WebContentLayer.h:49
> +    ~WebContentLayer() { }

virtual

> Source/WebKit/chromium/public/WebLayer.h:37
> +namespace WebCore { class LayerChromium; }

shouldn't this be inside a WEBKIT_IMPLEMENTATION guard? we shouldn't leak webcore types through this header

> Source/WebKit/chromium/public/WebLayer.h:50
> +    ~WebLayer() { reset(); }

if you wanna let people destroy WebLayers via a WebLayer*, virtual on this please

> Source/WebKit/chromium/public/WebLayerDelegate.h:16
> + *     * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following disclaimer
> + * in the documentation and/or other materials provided with the
> + * distribution.
> + *     * Neither the name of Google Inc. nor the names of its
> + * contributors may be used to endorse or promote products derived from
> + * this software without specific prior written permission.

here and other files: we use a 2-clause license header for new code

> Source/WebKit/chromium/src/WebContentLayer.cpp:5
> + * copyright (c) 2011 google inc. all rights reserved.
> + *
> + * redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are

somehow bizarrely this license header became totally lowercase

[James Robinson]

Comment on attachment 109368 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109368&action=review

> Source/WebKit/chromium/public/WebLayerTreeHost.h:61
> +class WebLayerTreeHost {

fyi, we're sort of thinking that CCLayerTreeHost is not quite the right level of abstraction to expose in chromium as the API to WebViewImpl - CCLayerTreeHost's public interface exposes way too many internal details and even the name "layertreehost" is about its implementation, not about what it really is about.

i haven't talked about with with Nat yet but I was thinking of adding a CCView pure virtual interface that exposes roughly what you've exposed here. you can't call the WK api WebView (since that's taken) but maybe it could be something more view-oriented?

[Darin Fisher (:fishd, Google)]

(In reply to comment #6)
> (From update of attachment 109368 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109368&action=review
> 
> > Source/WebKit/chromium/public/WebContentLayer.h:37
> > +namespace WebCore { class ContentLayerChromium; }
> 
> you shouldn't leak any WebCore things in the header. does this just need to be in a #if WEBKIT_IMPLEMENTATION guard?

Actually, we are not so strict about that these days.  This doesn't really hurt anything.  What would hurt is if someone could actually use this type for something in Chromium.  So long as we don't expose any functions that operate on these types, it should be fine.

The extra WEBKIT_IMPLEMENTATION guard is totally fine, but perhaps unnecessary.

[Antoine Labour]

Created attachment 109719 [details]
Patch

[Antoine Labour]

(In reply to comment #6)
> (From update of attachment 109368 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109368&action=review
> 
> > Source/WebKit/chromium/public/WebContentLayer.h:37
> > +namespace WebCore { class ContentLayerChromium; }
> 
> you shouldn't leak any WebCore things in the header. does this just need to be in a #if WEBKIT_IMPLEMENTATION guard?
> 
> > Source/WebKit/chromium/public/WebContentLayer.h:49
> > +    ~WebContentLayer() { }
> 
> virtual

Done.

> 
> > Source/WebKit/chromium/public/WebLayer.h:37
> > +namespace WebCore { class LayerChromium; }
> 
> shouldn't this be inside a WEBKIT_IMPLEMENTATION guard? we shouldn't leak webcore types through this header

So, it is needed so that the header compiles outside of WebKit. The type needs to be forward-declared so that m_private can be declared, even outside of WebKit.

> 
> > Source/WebKit/chromium/public/WebLayer.h:50
> > +    ~WebLayer() { reset(); }
> 
> if you wanna let people destroy WebLayers via a WebLayer*, virtual on this please

Done.

> 
> > Source/WebKit/chromium/public/WebLayerDelegate.h:16
> > + *     * Redistributions of source code must retain the above copyright
> > + * notice, this list of conditions and the following disclaimer.
> > + *     * Redistributions in binary form must reproduce the above
> > + * copyright notice, this list of conditions and the following disclaimer
> > + * in the documentation and/or other materials provided with the
> > + * distribution.
> > + *     * Neither the name of Google Inc. nor the names of its
> > + * contributors may be used to endorse or promote products derived from
> > + * this software without specific prior written permission.
> 
> here and other files: we use a 2-clause license header for new code

Done.

> 
> > Source/WebKit/chromium/src/WebContentLayer.cpp:5
> > + * copyright (c) 2011 google inc. all rights reserved.
> > + *
> > + * redistribution and use in source and binary forms, with or without
> > + * modification, are permitted provided that the following conditions are
> 
> somehow bizarrely this license header became totally lowercase

Done (typo 'u' in vim).

I also renamed WebLayerTreeHost* to WebLayerTreeView*

[Darin Fisher (:fishd, Google)]

Comment on attachment 109719 [details]
Patch

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?

[Antoine Labour]

Created attachment 109899 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Antoine Labour]

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

[Darin Fisher (:fishd, Google)]

Comment on attachment 109899 [details]
Patch

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?

[Antoine Labour]

Created attachment 110034 [details]
Patch

[Antoine Labour]

(In reply to comment #15)
> (From update of attachment 109899 [details])
> 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"

Done.

> 
> > Source/WebKit/chromium/public/WebContentLayer.h:53
> > +    WEBKIT_EXPORT bool drawsContent();
> 
> nit: make this function const?  on WebLayer, similar getters are const methods.

Done.

> 
> > Source/WebKit/chromium/public/WebContentLayer.h:62
> > +    WEBKIT_EXPORT WebFloatRect dirtyRect();
> 
> nit: make this function const?

Done.

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

Done.

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

Done. (invalidateRect/invalidate/invalidRect)

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

Done. I mentioned that it's that way in the single thread case. In the out-of-thread compositing case, it's not.
> 
> > Source/WebKit/chromium/public/WebLayerTreeViewClient.h:40
> > +    // Applies a scroll delta to the root layer. This is trigerred by events
> 
> nit: trigerred

Done.

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

Done.

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

Well, in the SkBitmap case, it's coming from skia/include/core, which is a dependent setting exported from skia/skia.gyp.
However, SkMatrix44 is in skia/include/utils, which is not. I explicitly added it to WebKit.gyp. If we want to, we could add it to skia/skia.gyp and then clean this after it rolls (but that'd add a useless include path to ~8000 .cc files in chrome and WebCore, which I don't think is desirable).

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

I axed that class, passing SkMatrix44 to WebLayer directly. Simpler, and fewer copies to get the data through.

[Antoine Labour]

Created attachment 110039 [details]
Patch

[WebKit Review Bot]

Comment on attachment 110039 [details]
Patch

Clearing flags on attachment: 110039

Committed r96896: <http://trac.webkit.org/changeset/96896>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Antoine Labour]

Created attachment 110089 [details]
Patch

[Antoine Labour]

New patch up, should fix issues.

[Darin Fisher (:fishd, Google)]

Comment on attachment 110089 [details]
Patch

Ugh, that warning :-P

[WebKit Review Bot]

Comment on attachment 110089 [details]
Patch

Clearing flags on attachment: 110089

Committed r96909: <http://trac.webkit.org/changeset/96909>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Antoine Labour]

Created attachment 110185 [details]
Patch

[Antoine Labour]

Gah, hopefully they're all fixed now.

[WebKit Review Bot]

Comment on attachment 110185 [details]
Patch

Clearing flags on attachment: 110185

Committed r96990: <http://trac.webkit.org/changeset/96990>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.