Plumb CompositingReason from Blink to compositor.

Plumb CompositingReason from Blink to compositor.

This is the blink-side patch.
(1) adds a few more compositing reasons that represent some of the internal hierarchy in RenderLayerBacking.
(2) does the plumbing

BUG=240946
R=enne@chromium.org, jamesr@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=151039

[shawnsingh, 2013-05-15 00:57:09]

Working prototype.  Still need to add the #if guards mechanism for the actual
patch for landing.

[jamesr, 2013-05-17 00:10:20]

https://codereview.chromium.org/15181006/diff/1/Source/Platform/chromium/publ...
File Source/Platform/chromium/public/WebCompositingReasons.h (right):

https://codereview.chromium.org/15181006/diff/1/Source/Platform/chromium/publ...
Source/Platform/chromium/public/WebCompositingReasons.h:13: * THIS SOFTWARE IS
PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS'' AND ANY
wrong copyright header.  Look for an email from darin@ to the internal list for
the right one to use

https://codereview.chromium.org/15181006/diff/1/Source/Platform/chromium/publ...
Source/Platform/chromium/public/WebCompositingReasons.h:29: #include
"core/rendering/RenderLayer.h"
we shouldn't do this

https://codereview.chromium.org/15181006/diff/1/Source/Platform/chromium/publ...
Source/Platform/chromium/public/WebCompositingReasons.h:74: // FIXME: Change
this enum to a list of 64-bit constants so that we can
An enum is not a signed type, so I'm not sure what this comment is trying to say

https://codereview.chromium.org/15181006/diff/1/Source/Platform/chromium/publ...
Source/Platform/chromium/public/WebCompositingReasons.h:84:
COMPILE_ASSERT(static_cast<unsigned>(WebKit::CompositingReasonUnknown) ==
static_cast<unsigned>(WebCore::CompositingReasonNone),
WebCompositingReasons_enum_does_not_match_webcore);
no - put these in AssertMatchingEnums

https://codereview.chromium.org/15181006/diff/1/Source/core/rendering/RenderL...
File Source/core/rendering/RenderLayerBacking.cpp (right):

https://codereview.chromium.org/15181006/diff/1/Source/core/rendering/RenderL...
Source/core/rendering/RenderLayerBacking.cpp:53: #include
"core/rendering/RenderLayer.h" // FIXME - only needed for CompositingReasons,
see if we can move that to a better home.
RenderLayerBacking reaches into RenderLayers all over the place, I think this
#include is fine. I'd remove the FIXME

[jamesr, 2013-05-17 00:17:54]

What do you intend to do with the reasons on the chromium side?

[shawnsingh, 2013-05-17 02:35:16]

On 2013/05/17 00:17:54, jamesr wrote:
> What do you intend to do with the reasons on the chromium side?

(1) debugging aid
(2) web developer insight via the frame viewer feature

Thanks for review, will revisit this tomorrow (friday) if possible.

[nduca, 2013-05-18 00:44:14]

how're we doing here, do we have consensus on this being useful?

[shawnsingh, 2013-05-20 20:38:06]

On 2013/05/18 00:44:14, nduca wrote:
> how're we doing here, do we have consensus on this being useful?

PTAL.  This version is actually intended for review/landing.  Thanks in advance!

[shawnsingh, 2013-05-21 20:11:54]

Enne, can you PTAL?  This is the blink-side of the compositing reasons.  (You
had already glanced at the chromium side).

Thanks in advance.

[enne (OOO), 2013-05-21 20:19:28]

https://codereview.chromium.org/15181006/diff/7001/Source/core/rendering/Rend...
File Source/core/rendering/RenderLayerBacking.cpp (right):

https://codereview.chromium.org/15181006/diff/7001/Source/core/rendering/Rend...
Source/core/rendering/RenderLayerBacking.cpp:355:
m_ancestorClippingLayer->setCompositingReasons(CompositingReasonLayerForClip);
What about just setting these reasons when you create the layers, rather than
every updateCompositingReasons call?

[shawnsingh, 2013-05-21 20:36:21]

On 2013/05/21 20:19:28, enne wrote:
>
https://codereview.chromium.org/15181006/diff/7001/Source/core/rendering/Rend...
> File Source/core/rendering/RenderLayerBacking.cpp (right):
> 
>
https://codereview.chromium.org/15181006/diff/7001/Source/core/rendering/Rend...
> Source/core/rendering/RenderLayerBacking.cpp:355:
> m_ancestorClippingLayer->setCompositingReasons(CompositingReasonLayerForClip);
> What about just setting these reasons when you create the layers, rather than
> every updateCompositingReasons call?

The compositing reasons may change as things get updated, even if the actual
layers are not re-allocated anew.

For example:  a layer with overlap + opacity animation --> once the animation
stops, it still has overlap.  but the reasons have changed.  And the layers
would not have been re-allocated just for this, but the compositing reason
should ideally reflect why it is composited for that instant.

[enne (OOO), 2013-05-21 23:00:03]

Maybe that's true only for RenderLayerBacking::m_graphicsLayer.  However, the
foreground layer will always be composited for
CompositingReasonLayerForForeground, right?

[shawnsingh, 2013-05-22 00:45:06]

On 2013/05/21 23:00:03, enne wrote:
> Maybe that's true only for RenderLayerBacking::m_graphicsLayer.  However, the
> foreground layer will always be composited for
> CompositingReasonLayerForForeground, right?

Done. PTAL, thanks!

[enne (OOO), 2013-05-22 00:53:47]

https://codereview.chromium.org/15181006/diff/18001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerBacking.cpp (right):

https://codereview.chromium.org/15181006/diff/18001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerBacking.cpp:864: m_childContainmentLayer =
createGraphicsLayer("Child clipping Layer", CompositingReasonNone);
CompositingReasonLayerForClip?

https://codereview.chromium.org/15181006/diff/18001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerBacking.cpp:1012: m_backgroundLayer =
createGraphicsLayer(layerName, CompositingReasonNone);
Is there a better reason than None here?

https://codereview.chromium.org/15181006/diff/18001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerBacking.cpp:1024: m_contentsContainmentLayer =
createGraphicsLayer(layerName, CompositingReasonNone);
CompositingReasonLayerForClip?

https://codereview.chromium.org/15181006/diff/18001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerBacking.cpp:1048: m_maskLayer =
createGraphicsLayer("Mask", CompositingReasonNone);
Is there a better reason than None here?

[shawnsingh, 2013-05-22 02:13:22]

New patch uploaded, with all RenderLayerBacking layers have real reasons.  PTAL.

For the sake of keeping only 32 reasons, I've lumped together (a) animations +
transitions, (b) background layer + the container needed when background exists,
and (c) the various clipping layers.  I've also currently ignored
RenderLayerCompositor special layers, since they're in flux right now with NCCH
changes.

I would like to abandon the 32-bit enum and use 64-bit constants instead,
instead of lumping reasons down.  The fine granularity will be very useful - for
our own debugging as well as for web developer's insight into the compositor's
brain.

Are you OK with that in a follow-up patch?  Or I could do that in this patch. 
Either way, probably should be done before plumbing the chromium side, when it
becomes much more painful to change both sides simultaneously.

[enne (OOO), 2013-05-22 16:27:06]

lgtm

[commit-bot: I haz the power, 2013-05-22 19:53:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shawnsingh@chromium.org/15181006/31001

[shawnsingh, 2013-05-22 21:44:00]

On 2013/05/22 19:53:49, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>
https://chromium-status.appspot.com/cq/shawnsingh%40chromium.org/15181006/31001

jamesr@ can you please give OWNERS review for Source/Platform and
public/platform, to make the presubmit happy?  Thanks in advance.

[jamesr, 2013-05-22 21:47:34]

https://codereview.chromium.org/15181006/diff/31001/Source/Platform/chromium/...
File Source/Platform/chromium/public/WebCompositingReasons.h (right):

https://codereview.chromium.org/15181006/diff/31001/Source/Platform/chromium/...
Source/Platform/chromium/public/WebCompositingReasons.h:1: #include
"../../../../public/platform/WebCompositingReasons.h"
no need to add a forwarding header for a brand new header.  just have all of the
#includes point to the actual .h

https://codereview.chromium.org/15181006/diff/31001/public/platform/WebCompos...
File public/platform/WebCompositingReasons.h (right):

https://codereview.chromium.org/15181006/diff/31001/public/platform/WebCompos...
public/platform/WebCompositingReasons.h:35: #define USE_WEB_COMPOSITING_REASONS
1
hmm, not sure what purpose this serves. chromium code can't #include this header
until it exists, at which point it'll be in use.

https://codereview.chromium.org/15181006/diff/31001/public/platform/WebLayer.h
File public/platform/WebLayer.h (right):

https://codereview.chromium.org/15181006/diff/31001/public/platform/WebLayer....
public/platform/WebLayer.h:145: // FIXME: non-pure until the chromium-side
implements this.x
typo "this.x"

https://codereview.chromium.org/15181006/diff/31001/public/platform/WebLayer....
public/platform/WebLayer.h:146: virtual void
setCompositingReasons(WebCompositingReasons) { };
no ;

[commit-bot: I haz the power, 2013-05-22 23:01:28]

Retried try job too often on blink_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

[shawnsingh, 2013-05-22 23:34:35]

Hi, abarth - jamesr mentions that you are currently doing some include path
migration.  Please advise how you would like to handle my new header,
WebCompositingReasons.h   You can see how it's included in
AssertMachingEnums.cpp.

I'm blocked on this, hopefully you have a chance to reply soon.  Thanks!

[nduca, 2013-05-23 15:30:42]

Shawn, looks like the moves have happened, maybe you can just update the patch
to work with the new world order.

[abarth-chromium, 2013-05-23 17:34:31]

You're probably going to need a forwarding header in
Source/Platform/chromium/public like the rest of the public/platform headers
until https://code.google.com/p/chromium/issues/detail?id=242829 is fixed.  It's
currently blocked on an iOS build issue.  (Stuart tells me it will be resolved
in a day or two.)

[jamesr, 2013-05-23 20:15:08]

Bummer. A forwarding header is better than #include <../>, though

[shawnsingh, 2013-05-23 20:38:33]

On 2013/05/23 20:15:08, jamesr wrote:
> Bummer. A forwarding header is better than #include <../>, though

jamesr@  PTAL, latest patch addresses your feedback but keeps the forwarding
header for now.

[jamesr, 2013-05-23 21:45:37]

lgtm

it's really silly, but I think you'll need a copyright header on the forwarding
header to keep all our scripts/etc happy.

[shawnsingh, 2013-05-23 22:49:58]

On 2013/05/23 21:45:37, jamesr wrote:
> lgtm
> 
> it's really silly, but I think you'll need a copyright header on the
forwarding
> header to keep all our scripts/etc happy.

No other headers there have the copyright;  I have just been following that
pattern.  I'm going to leave the copyright header omitted. 

If you still disagree, please let me know in the next few hours (still need a
few try bot runs to make sure it can land)

[shawnsingh, 2013-05-24 02:04:22]

Committed patchset #6 manually as r151039 (presubmit successful).