Bug 110505 – Expose a list of all reasons that qualify a RenderLayer to be composited

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Antoine Quint]

Expose a list of all reasons that qualify a RenderLayer to be compositedd

[Antoine Quint]

Created attachment 189582 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/13266329>

[Eric Seidel (no email)]

Sounds very useful to the inspector. :)

[WebKit Review Bot]

Comment on attachment 189582 [details]
Patch

Attachment 189582 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16697265

[Antoine Quint]

(In reply to comment #3)
> Sounds very useful to the inspector. :)

That's what I'm thinking too, and that's why I'm adding it.

[Dean Jackson]

Comment on attachment 189582 [details]
Patch

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

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1779
> +    if (inCompositingMode() && layer->isRootLayer())
> +        reason |= CompositingReasonRoot;

As far as I can tell (sorry, I'm terrible at reading diffs), this is the only case where you'd have more than one reason for compositing. See below.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:-1782
> +    if (reason & CompositingReasonRoot)
>          return "root";
> -

So what happens if you have this reason and another one. You'll never report "root" because you exit early above.

This makes me wonder why you're using a bitmask over just a regular enum?

[Antoine Quint]

(In reply to comment #6)
> (From update of attachment 189582 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189582&action=review
> 
> > Source/WebCore/rendering/RenderLayerCompositor.cpp:1779
> > +    if (inCompositingMode() && layer->isRootLayer())
> > +        reason |= CompositingReasonRoot;
> 
> As far as I can tell (sorry, I'm terrible at reading diffs), this is the only case where you'd have more than one reason for compositing. See below.

You're looking at the wrong method, this is only related to logging. There can definitely be several reasons as reported in the new version of reasonsForCompositing(). For instance, you can have a <video> element with a 3D transform applied.

> > Source/WebCore/rendering/RenderLayerCompositor.cpp:-1782
> > +    if (reason & CompositingReasonRoot)
> >          return "root";
> > -
> 
> So what happens if you have this reason and another one. You'll never report "root" because you exit early above.
> 
> This makes me wonder why you're using a bitmask over just a regular enum?

The bit mask will be used for the Web Inspector to be able to several reasons a RenderLayer was composited. The logReasonsForCompsiting() method replaces the old reasonsForCompositing() and its behavior which only reports the first matching reason.

[Dean Jackson]

Comment on attachment 189582 [details]
Patch

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

>>> Source/WebCore/rendering/RenderLayerCompositor.cpp:1779
>>> +        reason |= CompositingReasonRoot;
>> 
>> As far as I can tell (sorry, I'm terrible at reading diffs), this is the only case where you'd have more than one reason for compositing. See below.
> 
> You're looking at the wrong method, this is only related to logging. There can definitely be several reasons as reported in the new version of reasonsForCompositing(). For instance, you can have a <video> element with a 3D transform applied.

Yep - I screwed up.

>>> Source/WebCore/rendering/RenderLayerCompositor.cpp:-1782
>>> -
>> 
>> So what happens if you have this reason and another one. You'll never report "root" because you exit early above.
>> 
>> This makes me wonder why you're using a bitmask over just a regular enum?
> 
> The bit mask will be used for the Web Inspector to be able to several reasons a RenderLayer was composited. The logReasonsForCompsiting() method replaces the old reasonsForCompositing() and its behavior which only reports the first matching reason.

So I still think you should fix this method to return multiple reasons. If reason has both CompositingReasonVideo and CompositingReason3DTransform then the string should have "3d transform + video" or something.

[Simon Fraser (smfr)]

CA has no support for non-separable modes, BTW.

[Simon Fraser (smfr)]

(In reply to comment #9)
> CA has no support for non-separable modes, BTW.
Sorry, wrong bug.

What I want to say here is that I'd rather not have the expense of running all the "requires compositing for" in order to compute those flags (tho if we only do it when the inspector needs it, that would be OK).

[Antoine Quint]

(In reply to comment #10)
> (In reply to comment #9)
> > CA has no support for non-separable modes, BTW.
> Sorry, wrong bug.
> 
> What I want to say here is that I'd rather not have the expense of running all the "requires compositing for" in order to compute those flags (tho if we only do it when the inspector needs it, that would be OK).

This would only be for the inspector and logging scenarios. In the case of the inspector, I intend on exposing a per-layer call into WebCore to gather this information as needed from the front-ned, so it would only be done as necessary and not upfront for all layers pushed to the front-end.

[Simon Fraser (smfr)]

Comment on attachment 189582 [details]
Patch

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

>>>> Source/WebCore/rendering/RenderLayerCompositor.cpp:-1782
>>>> -
>>> 
>>> So what happens if you have this reason and another one. You'll never report "root" because you exit early above.
>>> 
>>> This makes me wonder why you're using a bitmask over just a regular enum?
>> 
>> The bit mask will be used for the Web Inspector to be able to several reasons a RenderLayer was composited. The logReasonsForCompsiting() method replaces the old reasonsForCompositing() and its behavior which only reports the first matching reason.
> 
> So I still think you should fix this method to return multiple reasons. If reason has both CompositingReasonVideo and CompositingReason3DTransform then the string should have "3d transform + video" or something.

It's fine for this logging to not change.

> Source/WebCore/rendering/RenderLayerCompositor.h:85
> +typedef unsigned CompositingReason;

CompositingReasons (plural), since this is a bitmask.

> Source/WebCore/rendering/RenderLayerCompositor.h:269
> +    CompositingReason reasonForCompositing(const RenderLayer*);

This should be const.

[Antoine Quint]

http://trac.webkit.org/changeset/143691

[WebKit Review Bot]

Re-opened since this is blocked by bug 110559

[Antoine Quint]

Created attachment 189769 [details]
Patch

[WebKit Review Bot]

Comment on attachment 189769 [details]
Patch

Attachment 189769 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16716012

[Antoine Quint]

Created attachment 189781 [details]
Patch

[WebKit Review Bot]

Comment on attachment 189781 [details]
Patch

Rejecting attachment 189781 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 189781, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:

fatal: read error: Connection reset by peer
Died at Tools/Scripts/update-webkit line 151.

Failed to run "['Tools/Scripts/update-webkit', '--chromium', '--force-update']" exit_code: 2

Updating OpenSource
fatal: read error: Connection reset by peer
Died at Tools/Scripts/update-webkit line 151.

Failed to run "['Tools/Scripts/update-webkit', '--chromium', '--force-update']" exit_code: 2
Updating OpenSource
fatal: read error: Connection reset by peer
Died at Tools/Scripts/update-webkit line 151.

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

[WebKit Review Bot]

Comment on attachment 189781 [details]
Patch

Clearing flags on attachment: 189781

Committed r143787: <http://trac.webkit.org/changeset/143787>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.