Webkit API for compositor
Created attachment 109216 [details] Patch
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
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?
Created attachment 109368 [details] Patch
(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.
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
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?
(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.
Created attachment 109719 [details] Patch
(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*
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?
Created attachment 109899 [details] Patch
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
(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.
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?
Created attachment 110034 [details] Patch
(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.
Created attachment 110039 [details] Patch
Comment on attachment 110039 [details] Patch Clearing flags on attachment: 110039 Committed r96896: <http://trac.webkit.org/changeset/96896>
All reviewed patches have been landed. Closing bug.
Created attachment 110089 [details] Patch
New patch up, should fix issues.
Comment on attachment 110089 [details] Patch Ugh, that warning :-P
Comment on attachment 110089 [details] Patch Clearing flags on attachment: 110089 Committed r96909: <http://trac.webkit.org/changeset/96909>
Created attachment 110185 [details] Patch
Gah, hopefully they're all fixed now.
Comment on attachment 110185 [details] Patch Clearing flags on attachment: 110185 Committed r96990: <http://trac.webkit.org/changeset/96990>