Bug 94174 – [chromium] Change WebLayer from a concrete type to a pure virtual interface

• Component: New Bugs
• Status: RESOLVED FIXED

[James Robinson]

[chromium] Change WebLayer from a concrete type to a pure virtual interface

[James Robinson]

Created attachment 158693 [details]
Patch

[James Robinson]

This patch isn't quite ready for full review, but it's close and I would like some feedback on the Web*Layer API.  The goal of the changes is to make WebLayer and friends be pure virtual interfaces and provide a factory function off of WebKit's platform support (not in this patch) so that WebKit.dll can link without unresolved symbols and an external library can provide the WebLayer implementations.  As a secondary goal, I'd like to clean up some of the API warts resulting from the ref-counted nature of WebLayer leaking into the API - things like clearClient(), clearRenderSurface(), etc - by making WebLayer have clear ownership by a single API consumer.

WebLayer exposes a large-ish number of entries points for common layer manipulation - setting up geometry, tree structure, and various properties.  The compositor supports a number of specific types of layers - content-backed, texture-backed, image-backed, IOSurface-backed - which without this patch are expressed as subclasses of WebLayer.  The class hierarchy maps pretty directly to the underlying classes (WebLayer -> LayerChromium, WebContentLayer -> ContentLayerChromium) with a few things that don't match up directly.

The difficulty with making Web*Layer pure virtual and preserving this hierarchy is that it creates a diamond inheritance structure: WebContentLayer inherits from WebLayer (at the public API layer), WebLayerImpl inherits from WebLayerImpl, WebContentLayerImpl inherits from WebContentLayer, but WebContentLayerImpl has to satisfy the WebLayer interface as well.  Simply inheriting from WebLayerImpl doesn't quite work - you have to use virtual inheritance from WebLayer -> WebLayerImpl and WebLayer -> WebContentLayer to get the compiler to generate the thunks from WebContentLayerImpl up to WebLayerImpl to satisfy the WebLayer contract.  However, using virtual inheritance makes it impossible to downcast from a WebLayer to a WebLayerImpl statically.  This makes it really hard to implement APIs like WebLayer::addChild(WebLayer* child) since the implementation has to be able to "see" the implementation of "child".  Without being able to downcast, I think I would have to add some knowledge of implemenation types to the public WebLayer interface.  Another alternative is to avoid virtual inheritance completely and satisfy the WebLayer API explicitly in WebContentLayerImpl - effectively writing out the thunks by hand that the compiler would generate.  This is gross since WebLayer is pretty large and we have a fair number of layer types.

To avoid all of this, I've broken up the Web*Layer type system such that WebLayer is a standalone type and the more specific layer subclasses are all of the form:

class WebContentLayer {
public:

   // The WebContentLayer has ownership of the value returned, not the caller.
   virtual WebLayer* layer() = 0;

   // content layer specific calls
};

then the caller has to be aware of the split and go to layer() when they want to manipulate common properties.  This also means that downcasting from a WebLayer to anything else is not possible - callers have to retain their handles to specific types if they want them.  In practice, this doesn't appear to be an issue.

The biggest downside of this is that it makes the API fairly stuttery.  For instance if you have a WebScrollbarLayer called m_scrollbarLayer and you want to adjust its position, you have to do:

m_scrollbarLayer->layer()->setPosition(foo)

I can't think of any good way to get around this.  One possibility is to take the "Layer" suffix off of the classes that aren't WebLayer since they no longer satisfy have an "is-a" relationship with WebLayer they have a "has-a" relationship.

Another more minor issue related to removing the refcounted-ness is that ownership is a bit odd for APIs that return a WebLayer* - for instance parent(). What I've done is just constructed a new wrapper WebLayer instance and declared that the caller has to take ownership of it.  I haven't fixed up all the callers and this seems really leak-prone, so I want to get rid of it.  I think the answer here is to remove the tree introspection APIs from WebLayer.  Whoever is building up the WebLayer tree must have some object model that they are using to populate the list (GraphicsLayerChromiums in webkit, ui::Layers in aura), so they can use that for walking around the tree.

[James Robinson]

Created attachment 158931 [details]
Patch

[James Robinson]

There's one remaining issue with this patch related to ScrollingCoordinatorChromium (I have a hack to avoid it in WebViewImpl, but I'd rather find a better way).  The problem there is that when navigating across pages the old GraphicsLayer tree is torn down, but the ScrollingCoordinator receives notifications about scrolling updates before it is notified that the scrollable layer pointer it was using now points to nowhere.  Previously, since ScrollingCoordinatorChromium held ref to a layer, these notifications would go to a soon-to-be-dead layer and we would simply have slightly stale state (and some extra memory use) until ScrollingCoordinator was notified of the new scrollable layer.

I can fix this in two ways:
1.) Reorder the cross-platform calls so we always get notified of the new scrollable layer before we get any property updates.  I'll look at doing this first.
2.) Buffer all property updates within ScrollingCoordinatorChromium until we know our view of the layer tree is up to date, then push to the layers. I'm wary of this since it means that we would have to add yet another buffer and synchronization mechanism that is mostly redundant with our layer commit.  I'm not excited about doing this, but it would move us closer to how other ScrollingCoordinator implementations work and thus be more robust to changes from other ports.

Otherwise, I think this patch is good to go.  It depends on https://bugs.webkit.org/show_bug.cgi?id=94229 and a ui/compositor/ patch on the chromium side to avoid breaking builds (will upload that shortly).

[James Robinson]

Created attachment 158935 [details]
rebased to ToT

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Attachment 158935 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1
Source/WebKit/chromium/src/WebViewImpl.cpp:187:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 51 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[James Robinson]

Created attachment 158957 [details]
Patch

[James Robinson]

This works around the out-of-order ScrollingCoordinator calls by grabbing the scroll layer off of the Page->Frame->....->RenderLayerCompositor in the property setters instead of relying on the setScrollLayer call.  A bit ugly, but we were doing it already anyway.

[Adrienne Walker]

Comment on attachment 158957 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.h:171
> +    OwnPtr<WebKit::WebContentLayer> m_contentLayer;
> +    WebKit::WebLayer* m_layer;
> +    OwnPtr<WebKit::WebLayer> m_transformLayer;
> +    OwnPtr<WebKit::WebImageLayer> m_imageLayer;
> +    WebKit::WebLayer* m_contentsLayer;

m_contentsLayer *and* m_contentLayer? ಠ_ಠ

[James Robinson]

Created attachment 159236 [details]
rebased and addresses feedback

[James Robinson]

Updated to just use m_layer. I wasn't sure if you meant to set review? or not - http://codereview.chromium.org/10832355/ needs to land before this anyway.

[WebKit Review Bot]

Attachment 159236 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1
Source/WebKit/chromium/src/WebLayerImpl.cpp:32:  cc includes should be "CCFoo.h" instead of "cc/CCFoo.h".  [build/include] [4]
Total errors found: 1 in 51 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[James Robinson]

(In reply to comment #13)
> Attachment 159236 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1
> Source/WebKit/chromium/src/WebLayerImpl.cpp:32:  cc includes should be "CCFoo.h" instead of "cc/CCFoo.h".  [build/include] [4]
> Total errors found: 1 in 51 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

Oh the irony!  Will fix :P

[James Robinson]

Created attachment 159241 [details]
Patch

[Adrienne Walker]

Comment on attachment 159241 [details]
Patch

R=me.

[James Robinson]

Committed r126076: <http://trac.webkit.org/changeset/126076>

[Antoine Labour]

FYI, this looks like it broke accelerated compositing on aura (all tabs appear blank).

[Antoine Labour]

See comments in http://codereview.chromium.org/10832355/ , that's where the bug seems to be.

[WebKit Review Bot]

Re-opened since this is blocked by 94614

[Kenneth Russell]

So that there is complete history somewhere: this change caused a crash during compositor shutdown in Chromium Aura builds. This was blocking WebKit rolls, and the thought this morning was that the crash (downstream in Chromium code) had not been fixed, so I rolled this out in https://bugs.webkit.org/show_bug.cgi?id=94614 . However, it appears that the crash had actually already been fixed, in https://chromiumcodereview.appspot.com/10827435/ , so the rollout will be reverted.

Example failures:

http://build.chromium.org/p/chromium/buildstatus?builder=Linux%20%28aura%29&number=4341
http://build.chromium.org/p/chromium.chromiumos/buildstatus?builder=ChromiumOS%20%28x86%29&number=7763

[Antoine Labour]

(In reply to comment #21)
> So that there is complete history somewhere: this change caused a crash during compositor shutdown in Chromium Aura builds. This was blocking WebKit rolls, and the thought this morning was that the crash (downstream in Chromium code) had not been fixed, so I rolled this out in https://bugs.webkit.org/show_bug.cgi?id=94614 . However, it appears that the crash had actually already been fixed, in https://chromiumcodereview.appspot.com/10827435/ , so the rollout will be reverted.
> 
> Example failures:
> 
> http://build.chromium.org/p/chromium/buildstatus?builder=Linux%20%28aura%29&number=4341
> http://build.chromium.org/p/chromium.chromiumos/buildstatus?builder=ChromiumOS%20%28x86%29&number=7763

Mmh, those crashes are not familiar. The issues I was seeing in the CL above were not at shutdown but related to accelerated compositing. Are we positive they are fixed? These crashes look related to the ownership semantics changes, where we may need to be more precise about destruction order.

[James Robinson]

Sounds like something else.  I'll prep a re-re-re-revert and try to repro locally.

[Kenneth Russell]

Yes, excellent point. piman, your fix landed at http://src.chromium.org/viewvc/chrome?view=rev&revision=152530 , but epoger's attempted (and failed) WebKit roll landed at http://src.chromium.org/viewvc/chrome?view=rev&revision=152561 . So there is still a problem with the combination of the upstream and downstream code associated with this change.

jamesr's indicated he'll build Aura, test and debug before re-landing.

[James Robinson]

There's a shutdown issue in ~Compositor. It's easy enough to patch downstream but it's possible that we could be more robust.

[James Robinson]

https://bugs.webkit.org/show_bug.cgi?id=94631 fixes the issues on the tests that the aura bots failed on.  I'll hold off on relanding this patch until we get a successful WebKit roll in since it's pretty backed up and I don't want to cause more risk for everything else depending on the roll.  Sorry about all of the problems, I really should have run these aura tests myself.

[James Robinson]

Created attachment 160051 [details]
Patch

[James Robinson]

Rebased with LinkHighlighter updated and regenerated changelogs

[Adrienne Walker]

Comment on attachment 160051 [details]
Patch

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

R=me, assuming you smoke test aura before landing.  LinkHighlight changes and GraphicsLayerChromium changes all look fine to me.

> Source/Platform/ChangeLog:12
> +invoke
> +        special cleanup methods before shutdown.

This is a weird word wrap.

[James Robinson]

Committed r126378: <http://trac.webkit.org/changeset/126378>