Bug 86269 – [chromium] Convert GraphicsLayerChromium to use WebLayer types

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Robinson]

[chromium] Convert GraphicsLayerChromium to use WebLayer types

[James Robinson]

Created attachment 141521 [details]
Patch

[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 141521 [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/WebContentLayer.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 13 files


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

[Adam Barth]

Comment on attachment 141521 [details]
Patch

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

> Source/WebCore/ChangeLog:6
> +        This converts GraphicsLayerChromium over to use WebLayer and WebContentLayer. The conversion is not completely

completely -> complete

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:-353
> -    bool m_pageScaleDirty;

Is this unrelated cleanup?  It doesn't seem to interact with the rest of the patch.

[Adam Barth]

Comment on attachment 141521 [details]
Patch

rs=me.  I looked through the patch and it looks reasonable.  Let me know if you'd prefer a review from a Chromium Compositing expert.

[James Robinson]

Thanks Adam.  I'd like Enne to take a look as well before landing.

[Adrienne Walker]

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

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:106
> +        m_layer.clearClient();
> +        m_layer.unwrap<LayerChromium>()->clearRenderSurface();
>          // Primary layer may have at one point been m_layer. Be sure to reset
>          // the delegate, just in case.
> -        m_layer->setLayerAnimationDelegate(0);
> +        m_layer.unwrap<LayerChromium>()->setLayerAnimationDelegate(0);

In terms of a smaller API, do we just need a LayerChromium::willBeDestroyed() that does all of these things? This is all just to clear out circular references, yeah?

[James Robinson]

That's correct. Let me try willBeDestroyed() and see how it goes.

[James Robinson]

I've added a WebLayer::willBeDestroyed() and it works, but it feels a bit strange.  I think the issue here is that WebLayer is a value-type that wraps a WebPrivatePtr<> (aka RefPtr), so when a given WebLayer instance is destroyed it's not clear whether that is supposed to be the "last" WebLayer or not.  WebLayer::willBeDestroyed() is the caller saying basically "I promise I'm the last guy who cares about this layer, go ahead and shut down now".  We can't just rely on the d'tor of the WebPrivatePtr<> object because the willBeDestroyed() call breaks reference cycles.

I'm not entirely sure what to do here.  I'll put the patch with willBeDestroyed() up so you can see it, but something feels wrong.  I think my preference would be to make WebLayer have a WebPrivateOwnPtr<> and be non-copyable and make the users deal with how to copy/etc the WebLayer themselves.  That way ~WebLayer() would always mean that everyone who is interested in the WebLayer is gone and it would be free to break cycles on the impl.  That might make code using WebLayer a bit more awkward.

Another fix would be to get rid of the reference cycles in the impl (LayerChromium <-> RenderSurfaceChromium).  Last I looked that seemed non-trivial but I could take another crack.

[James Robinson]

Created attachment 141534 [details]
with willBeDestroyed()

[WebKit Review Bot]

Attachment 141534 [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/WebContentLayer.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 13 files


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

[Dana Jansens]

(In reply to comment #9)
> Another fix would be to get rid of the reference cycles in the impl (LayerChromium <-> RenderSurfaceChromium).  Last I looked that seemed non-trivial but I could take another crack.

I think one way to do it would be to make the renderSurfaceLayerList hold RenderSurfaces directly instead of owning layers. Then the list could own the surfaces and the layers could have raw pointers? Might be the layer iterator is about the only thing that would take effort to be changed.

[Adrienne Walker]

(In reply to comment #9)
> I've added a WebLayer::willBeDestroyed() and it works, but it feels a bit strange.  I think the issue here is that WebLayer is a value-type that wraps a WebPrivatePtr<> (aka RefPtr), so when a given WebLayer instance is destroyed it's not clear whether that is supposed to be the "last" WebLayer or not.  WebLayer::willBeDestroyed() is the caller saying basically "I promise I'm the last guy who cares about this layer, go ahead and shut down now".  We can't just rely on the d'tor of the WebPrivatePtr<> object because the willBeDestroyed() call breaks reference cycles.

Ok, that's a great point--willBeDestroyed is an unfortunate name given that the GraphicsLayer is getting destroyed, but it's not guaranteed that the LayerChromiums are.  Thanks for putting it up; I'm inclined to go with your patch #1 and solve this problem elsewhere.

> I'm not entirely sure what to do here.  I'll put the patch with willBeDestroyed() up so you can see it, but something feels wrong.  I think my preference would be to make WebLayer have a WebPrivateOwnPtr<> and be non-copyable and make the users deal with how to copy/etc the WebLayer themselves.  That way ~WebLayer() would always mean that everyone who is interested in the WebLayer is gone and it would be free to break cycles on the impl.  That might make code using WebLayer a bit more awkward.

I'm intrigued by this idea, but am not convinced yet.  If WebLayer had a WebPrivateOwnPtr, then the entire LayerChromium tree would be owned by external classes rather than having some internal ownership over its children.  That sounds a little dodgy.

[James Robinson]

(In reply to comment #13)
> > I'm not entirely sure what to do here.  I'll put the patch with willBeDestroyed() up so you can see it, but something feels wrong.  I think my preference would be to make WebLayer have a WebPrivateOwnPtr<> and be non-copyable and make the users deal with how to copy/etc the WebLayer themselves.  That way ~WebLayer() would always mean that everyone who is interested in the WebLayer is gone and it would be free to break cycles on the impl.  That might make code using WebLayer a bit more awkward.
> 
> I'm intrigued by this idea, but am not convinced yet.  If WebLayer had a WebPrivateOwnPtr, then the entire LayerChromium tree would be owned by external classes rather than having some internal ownership over its children.  That sounds a little dodgy.

Sorry, I was not clear here.  I intended that WebLayer would have a WebPrivateOwnPtr<T> where T is something like WebLayerImpl (sorry for the suffix collision) and WebLayerImpl would have a RefPtr<LayerChromium>.  Then the internals of the LayerChromium tree would all be refcounted and manage their own lifetimesl, but all of the outside world's links in would be via single-ownership pointers.

[Antoine Labour]

Comment on attachment 141534 [details]
with willBeDestroyed()

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

> Source/Platform/chromium/public/WebLayer.h:121
> +    WEBKIT_EXPORT void setContentsScale(float);

Drive-by: I think we should make sure these are exposed at the right place. I agree that e.g. setPreserves3Dand setBackgroundColor belong here, but setBackgroundColor or setContentsScale look like they should belong in WebContentsLayer.
I have no idea what setIsDrawable means (and how it relates to WebContentLayer::setDrawsContent)

> Source/WebKit/chromium/src/WebLayer.cpp:102
> +    m_private->clearRenderSurface();

This doesn't outright bother me, since it's conservative anyway (we'll recreate the RS if we need to).

> Source/WebKit/chromium/src/WebLayer.cpp:103
> +    m_private->setLayerAnimationDelegate(0);

We could make that an explicit separate call, so that it's clear that whoever created the layer and set the delegate can also reset it. It /seems/ safe, but I'm not very knowledgeable about the code.

[Antoine Labour]

(In reply to comment #9)
> I've added a WebLayer::willBeDestroyed() and it works, but it feels a bit strange.  I think the issue here is that WebLayer is a value-type that wraps a WebPrivatePtr<> (aka RefPtr), so when a given WebLayer instance is destroyed it's not clear whether that is supposed to be the "last" WebLayer or not.  WebLayer::willBeDestroyed() is the caller saying basically "I promise I'm the last guy who cares about this layer, go ahead and shut down now".  We can't just rely on the d'tor of the WebPrivatePtr<> object because the willBeDestroyed() call breaks reference cycles.
> 
> I'm not entirely sure what to do here.  I'll put the patch with willBeDestroyed() up so you can see it, but something feels wrong.

I'm not outraged by it. See my comments there, I think if we're clear about the semantics we're ok.
To break the cycles, we can also decide to do that when we remove it from the tree.

>  I think my preference would be to make WebLayer have a WebPrivateOwnPtr<> and be non-copyable and make the users deal with how to copy/etc the WebLayer themselves.  That way ~WebLayer() would always mean that everyone who is interested in the WebLayer is gone and it would be free to break cycles on the impl.  That might make code using WebLayer a bit more awkward.

One of the problem is calls that return a WebLayer.

> 
> Another fix would be to get rid of the reference cycles in the impl (LayerChromium <-> RenderSurfaceChromium).  Last I looked that seemed non-trivial but I could take another crack.

That'd be even better of course :)

[WebKit Review Bot]

Comment on attachment 141534 [details]
with willBeDestroyed()

Attachment 141534 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12666687

[James Robinson]

(In reply to comment #15)
> (From update of attachment 141534 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141534&action=review
> 
> > Source/Platform/chromium/public/WebLayer.h:121
> > +    WEBKIT_EXPORT void setContentsScale(float);
> 
> Drive-by: I think we should make sure these are exposed at the right place. I agree that e.g. setPreserves3Dand setBackgroundColor belong here, but setBackgroundColor or setContentsScale look like they should belong in WebContentsLayer.
> I have no idea what setIsDrawable means (and how it relates to WebContentLayer::setDrawsContent)
> 

I've moved setDoubleSided() and setContentsScale() down to WebContentLayer and added a comment for each.  setIsDrawable() is the same thing as WebContentLayer::setDrawsContent(), so I've moved that function up to WebLayer and updated callers to refer to setDrawsContent().

As for the shutdown issue for now I've decided to punt and just unwrap to call clearRenderSurface()/setLayerAnimationDelegate().  We'll need to add WebLayer APIs for animation soon, when that happens it'll make sense to have an explicit call to clear the animation delegate.  I think we just need to fix the render surface RefPtr cycle anyway without an API change.

[James Robinson]

Created attachment 141819 [details]
Patch

[WebKit Review Bot]

Attachment 141819 [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/WebContentLayer.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 13 files


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

[James Robinson]

Created attachment 141820 [details]
diff to previous patch

[Adrienne Walker]

Comment on attachment 141819 [details]
Patch

R=me.  I'm still good with punting the refcycle issue and landing this patch.  Thanks for the diff.  :)

[WebKit Review Bot]

Comment on attachment 141819 [details]
Patch

Rejecting attachment 141819 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/12718463

[James Robinson]

Committed r117469: <http://trac.webkit.org/changeset/117469>