Add layer name into frame viewer

Add layer name into frame viewer

Layer name is useful for web developer to identify different layers. Layer name is pulled from blink. There is a blink side patch provides debugName() API in GraphicsLayer.

BUG=269258
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=218007

[qiankun, 2013-06-17 08:54:51]

Please help to review this patch. Thanks in advance!

[enne (OOO), 2013-06-17 17:25:38]

https://codereview.chromium.org/16903005/diff/1/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/16903005/diff/1/cc/layers/layer_impl.cc#newco...
cc/layers/layer_impl.cc:1145: state->SetString("layer_name", debug_name());
Debug names are only set in debug mode.  Won't this leave traces saying that
layer_name is the empty string in release builds?

[qiankun, 2013-06-18 10:56:54]

On 2013/06/17 17:25:38, enne wrote:
> https://codereview.chromium.org/16903005/diff/1/cc/layers/layer_impl.cc
> File cc/layers/layer_impl.cc (right):
> 
>
https://codereview.chromium.org/16903005/diff/1/cc/layers/layer_impl.cc#newco...
> cc/layers/layer_impl.cc:1145: state->SetString("layer_name", debug_name());
> Debug names are only set in debug mode.  Won't this leave traces saying that
> layer_name is the empty string in release builds?

Yes, it's empty string in release version. What about only recording layer name
in debug build? 
Layer name provides obvious hints for differentiating layers. It's useful when
debugging.

[enne (OOO), 2013-06-18 16:45:56]

https://codereview.chromium.org/16903005/diff/5001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/16903005/diff/5001/cc/layers/layer_impl.cc#ne...
cc/layers/layer_impl.cc:1146: state->SetString("layer_name", debug_name());
On second thought, how about if (!debug_name().empty()) without any #ifdefs? Not
all layers have names even in debug builds either.

[nduca, 2013-06-18 17:19:06]

I wonder, can we switch the impedence of names? E.g. have a GraphicsLayerClient
getName() method? They're ifdef debug because they're push, but if we made it an
accessor, then blink/renderer could generate the string only on demand. This
would make the layer stuff much more useful, hence me trying to figure out how
to do this "right" rather than hackily...

[enne (OOO), 2013-06-18 17:34:54]

On 2013/06/18 17:19:06, nduca wrote:
> I wonder, can we switch the impedence of names? E.g. have a
GraphicsLayerClient
> getName() method? They're ifdef debug because they're push, but if we made it
an
> accessor, then blink/renderer could generate the string only on demand. This
> would make the layer stuff much more useful, hence me trying to figure out how
> to do this "right" rather than hackily...

Hmm.  That's an intriguing thought.  I think it's possible, but it'd take some
plumbing.

For layers that get composited, you'd have to do something like cc::Layer =>
WebLayer => GraphicsLayer => GraphicsLayerClient [is-a RenderLayerBacking, most
of the time] => m_owningLayer [is-a RenderLayer] => renderer() =>
RenderObject::renderName.  You'd need to add a WebLayerClient with some name()
function to get to GraphicsLayer from WebLayer.  For layers that get created as
a result of compositing (e.g. clip layers, etc) either in RenderLayerBacking or
in GraphicsLayer, you'd have to short-circuit that walk along the way.

[qiankun, 2013-06-19 03:17:16]

On 2013/06/18 16:45:56, enne wrote:
> https://codereview.chromium.org/16903005/diff/5001/cc/layers/layer_impl.cc
> File cc/layers/layer_impl.cc (right):
> 
>
https://codereview.chromium.org/16903005/diff/5001/cc/layers/layer_impl.cc#ne...
> cc/layers/layer_impl.cc:1146: state->SetString("layer_name", debug_name());
> On second thought, how about if (!debug_name().empty()) without any #ifdefs?
Not
> all layers have names even in debug builds either.

debug_name() may be "Layer for " other than totally empty string "", because
updateNames()
can be called from GraphicsLayer::setupContentsLayer() besides
GraphicsLayer::setName().

How about both checking debug build and if (!debug_name().empty())?

[nduca, 2013-06-19 05:20:27]

could you try making names an accessor going in the other direction per #5 and
#6? I think it may be the more production-worthy approach...

[qiankun, 2013-06-19 06:26:27]

On 2013/06/19 05:20:27, nduca wrote:
> could you try making names an accessor going in the other direction per #5 and
> #6? I think it may be the more production-worthy approach...

Sure, I will do. I am trying to understand the method in #5 and #6. Do you mean:
1) define a new function in cc::Layer, like getName() to get name on demand.
2) cc::Layer::getName() will call similar method WebLayerImpl::getName()
3) WebLayerImpl delegates the call to WebContentLayerImpl
4) WebContentLayerImpl has a member WebKit::WebContentLayerClient* client_ which
can used finally to get the name from GraphicsLayer.
5) then following: GraphicsLayer => GraphicsLayerClient [is-a
RenderLayerBacking, most of the time] => m_owningLayer [is-a RenderLayer] =>
renderer() => RenderObject::renderName.

[nduca, 2013-06-19 18:32:47]

Yep, something like that!

[qiankun, 2013-07-02 06:11:52]

On 2013/06/19 18:32:47, nduca wrote:
> Yep, something like that!

PTAL. Get debug name on demand. Blink side patch is at:
https://codereview.chromium.org/18473002.
This is a prototype and not completely finished.  Could you give some
suggestions?
(1) The start point of get debug name is at GraphicsLayer other than
RenderObject, because it's hard to differentiate GraphicsLayers created in
RenderLayerCompositing [is-a GraphicsLayerClient] and RenderLayerBacking [is-a
GraphicsLayerClient]. I can add a layer type to differentiate these
GraphicsLayers and get debug name from RenderObject for the GraphicsLayer which
represents the web contents. But layer name can be seen as another type of layer
type. 
(2) Some WebLayers such as contentsLayer and m_linkHighlight in GraphicsLayer
still need push their names to cc::Layer. It's hard for them to get name on
demand.
(3) For layers that get created as a result of compositing (e.g. clip layers,
etc), set their name when creating and get their name on demand in compositor.
(4) some #ifdef should be removed to avoid empty name.

[nduca, 2013-07-03 11:26:27]

This is enne and caseq territory.

[nduca, 2013-07-03 11:27:31]

(though, #3, just fix those by subtyping the dynamic graphics layers they
create?)

[enne (OOO), 2013-07-03 17:40:43]

https://codereview.chromium.org/16903005/diff/16001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/16903005/diff/16001/cc/layers/layer.cc#newcod...
cc/layers/layer.cc:678: layer->SetDebugName(DebugName());
How about only pulling the name when tracing is active?  See
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/pictu...
for an example.

https://codereview.chromium.org/16903005/diff/16001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/16903005/diff/16001/cc/layers/layer_impl.cc#n...
cc/layers/layer_impl.cc:1080: #ifndef NDEBUG
Can you remove the #ifdefs?

https://codereview.chromium.org/16903005/diff/16001/webkit/renderer/composito...
File webkit/renderer/compositor_bindings/web_content_layer_impl.cc (right):

https://codereview.chromium.org/16903005/diff/16001/webkit/renderer/composito...
webkit/renderer/compositor_bindings/web_content_layer_impl.cc:70: return
UTF16ToASCII(client_->debugName());
Why is this conversion needed?

https://codereview.chromium.org/16903005/diff/16001/webkit/renderer/composito...
File webkit/renderer/compositor_bindings/web_content_layer_impl.h (right):

https://codereview.chromium.org/16903005/diff/16001/webkit/renderer/composito...
webkit/renderer/compositor_bindings/web_content_layer_impl.h:35: std::string
DebugName();
Is there a reason this is on WebContentLayer and not on WebLayer?

[qiankun, 2013-07-04 04:34:04]

Updated the patch. PTAL.

https://codereview.chromium.org/16903005/diff/16001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/16903005/diff/16001/cc/layers/layer.cc#newcod...
cc/layers/layer.cc:678: layer->SetDebugName(DebugName());
On 2013/07/03 17:40:43, enne wrote:
> How about only pulling the name when tracing is active?  See
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/pictu...
> for an example.

OK.

https://codereview.chromium.org/16903005/diff/16001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/16903005/diff/16001/cc/layers/layer_impl.cc#n...
cc/layers/layer_impl.cc:1080: #ifndef NDEBUG
On 2013/07/03 17:40:43, enne wrote:
> Can you remove the #ifdefs?

OK.

https://codereview.chromium.org/16903005/diff/16001/webkit/renderer/composito...
File webkit/renderer/compositor_bindings/web_content_layer_impl.cc (right):

https://codereview.chromium.org/16903005/diff/16001/webkit/renderer/composito...
webkit/renderer/compositor_bindings/web_content_layer_impl.cc:70: return
UTF16ToASCII(client_->debugName());
On 2013/07/03 17:40:43, enne wrote:
> Why is this conversion needed?

It's used for converting WebKit::WebString to std::string. I refer the
implementation in WebLayerImpl::setDebugName().

https://codereview.chromium.org/16903005/diff/16001/webkit/renderer/composito...
File webkit/renderer/compositor_bindings/web_content_layer_impl.h (right):

https://codereview.chromium.org/16903005/diff/16001/webkit/renderer/composito...
webkit/renderer/compositor_bindings/web_content_layer_impl.h:35: std::string
DebugName();
On 2013/07/03 17:40:43, enne wrote:
> Is there a reason this is on WebContentLayer and not on WebLayer?

That's due to WebContentLayer is passed as ContentLayerClient when creating
cc::ContentLayer, while WebLayer doesn't have a client.

[enne (OOO), 2013-07-09 20:15:31]

https://codereview.chromium.org/16903005/diff/16001/webkit/renderer/composito...
File webkit/renderer/compositor_bindings/web_content_layer_impl.h (right):

https://codereview.chromium.org/16903005/diff/16001/webkit/renderer/composito...
webkit/renderer/compositor_bindings/web_content_layer_impl.h:35: std::string
DebugName();
On 2013/07/04 04:34:04, qiankun wrote:
> On 2013/07/03 17:40:43, enne wrote:
> > Is there a reason this is on WebContentLayer and not on WebLayer?
> 
> That's due to WebContentLayer is passed as ContentLayerClient when creating
> cc::ContentLayer, while WebLayer doesn't have a client. 

I don't think WebLayer not having a client is a reason to put this on
WebContentLayerClient.

That said, non-ContentLayers don't really get interesting names:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
(ironically, m_layer ends up being a cc::ContentLayer, m_contentsLayer ends up
being some other non-Content type of layer).

I'd reluctantly be ok with the simplification to only have names for
WebContentLayers, but it seems like this could be more general.

https://codereview.chromium.org/16903005/diff/27001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/16903005/diff/27001/cc/layers/layer.cc#newcod...
cc/layers/layer.cc:681:
TRACE_EVENT_CATEGORY_GROUP_ENABLED(TRACE_DISABLED_BY_DEFAULT("cc.debug"),
Please remove the disabled by default here.

Also, as it turns out, this is calling into Blink code from the compositor
thread while the main thread is blocked.  I think this would be safer to update
the name on the layer during the Update call, which does happen on the main
thread.

https://codereview.chromium.org/16903005/diff/27001/cc/layers/layer.cc#newcod...
cc/layers/layer.cc:686: layer->SetDebugName(debug_name_);
What about clearing the name if tracing is off?

https://codereview.chromium.org/16903005/diff/27001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/16903005/diff/27001/cc/layers/layer_impl.cc#n...
cc/layers/layer_impl.cc:357: void LayerImpl::PushPropertiesTo(LayerImpl* layer)
{
I suspect you need to push the name here too.

[qiankun, 2013-07-10 04:53:07]

PTAL. Thanks.

https://codereview.chromium.org/16903005/diff/16001/webkit/renderer/composito...
File webkit/renderer/compositor_bindings/web_content_layer_impl.h (right):

https://codereview.chromium.org/16903005/diff/16001/webkit/renderer/composito...
webkit/renderer/compositor_bindings/web_content_layer_impl.h:35: std::string
DebugName();
On 2013/07/09 20:15:32, enne wrote:
> On 2013/07/04 04:34:04, qiankun wrote:
> > On 2013/07/03 17:40:43, enne wrote:
> > > Is there a reason this is on WebContentLayer and not on WebLayer?
> > 
> > That's due to WebContentLayer is passed as ContentLayerClient when creating
> > cc::ContentLayer, while WebLayer doesn't have a client. 
> 
> I don't think WebLayer not having a client is a reason to put this on
> WebContentLayerClient.
> 
It seems part of WebLayer's behavior is a bridge between WebContentLayer and
cc::Layer. I wonder why WebContentLayer is required, maybe we can implement
WebContentLayer's actions in WebLayer. I keep their current implementations. 

> That said, non-ContentLayers don't really get interesting names:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> (ironically, m_layer ends up being a cc::ContentLayer, m_contentsLayer ends up
> being some other non-Content type of layer).
> 
> I'd reluctantly be ok with the simplification to only have names for
> WebContentLayers, but it seems like this could be more general.

https://codereview.chromium.org/16903005/diff/27001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/16903005/diff/27001/cc/layers/layer.cc#newcod...
cc/layers/layer.cc:681:
TRACE_EVENT_CATEGORY_GROUP_ENABLED(TRACE_DISABLED_BY_DEFAULT("cc.debug"),
On 2013/07/09 20:15:32, enne wrote:
> Please remove the disabled by default here.
> 
Done.

> Also, as it turns out, this is calling into Blink code from the compositor
> thread while the main thread is blocked.  I think this would be safer to
update
> the name on the layer during the Update call, which does happen on the main
> thread.

Pull name then call SetDebugName() in ContentLayer::Update()? If it's correct, I
can do this.

https://codereview.chromium.org/16903005/diff/27001/cc/layers/layer.cc#newcod...
cc/layers/layer.cc:686: layer->SetDebugName(debug_name_);
On 2013/07/09 20:15:32, enne wrote:
> What about clearing the name if tracing is off?

Ok.

https://codereview.chromium.org/16903005/diff/27001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/16903005/diff/27001/cc/layers/layer_impl.cc#n...
cc/layers/layer_impl.cc:357: void LayerImpl::PushPropertiesTo(LayerImpl* layer)
{
On 2013/07/09 20:15:32, enne wrote:
> I suspect you need to push the name here too.

Isn't the name pushed here: "layer->SetDebugName(debug_name_);"

[enne (OOO), 2013-07-10 16:48:41]

https://codereview.chromium.org/16903005/diff/16001/webkit/renderer/composito...
File webkit/renderer/compositor_bindings/web_content_layer_impl.h (right):

https://codereview.chromium.org/16903005/diff/16001/webkit/renderer/composito...
webkit/renderer/compositor_bindings/web_content_layer_impl.h:35: std::string
DebugName();
On 2013/07/10 04:53:07, qiankun wrote:
> On 2013/07/09 20:15:32, enne wrote:
> > On 2013/07/04 04:34:04, qiankun wrote:
> > > On 2013/07/03 17:40:43, enne wrote:
> > > > Is there a reason this is on WebContentLayer and not on WebLayer?
> > > 
> > > That's due to WebContentLayer is passed as ContentLayerClient when
creating
> > > cc::ContentLayer, while WebLayer doesn't have a client. 
> > 
> > I don't think WebLayer not having a client is a reason to put this on
> > WebContentLayerClient.
> > 
> It seems part of WebLayer's behavior is a bridge between WebContentLayer and
> cc::Layer. I wonder why WebContentLayer is required, maybe we can implement
> WebContentLayer's actions in WebLayer. I keep their current implementations. 
> 
> > That said, non-ContentLayers don't really get interesting names:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > (ironically, m_layer ends up being a cc::ContentLayer, m_contentsLayer ends
up
> > being some other non-Content type of layer).
> > 
> > I'd reluctantly be ok with the simplification to only have names for
> > WebContentLayers, but it seems like this could be more general.
> 

WebContentLayer is a logical extension of WebLayer.  It does things that general
WebLayers (and other derived Layer classes) don't need.

https://codereview.chromium.org/16903005/diff/27001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/16903005/diff/27001/cc/layers/layer.cc#newcod...
cc/layers/layer.cc:681:
TRACE_EVENT_CATEGORY_GROUP_ENABLED(TRACE_DISABLED_BY_DEFAULT("cc.debug"),
On 2013/07/10 04:53:07, qiankun wrote:

> Pull name then call SetDebugName() in ContentLayer::Update()? If it's correct,
I
> can do this.

Yeah, that's exactly it.  Rather than pulling the name in PushPropertiesTo, do
it in Update.

https://codereview.chromium.org/16903005/diff/27001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/16903005/diff/27001/cc/layers/layer_impl.cc#n...
cc/layers/layer_impl.cc:357: void LayerImpl::PushPropertiesTo(LayerImpl* layer)
{
On 2013/07/10 04:53:07, qiankun wrote:
> On 2013/07/09 20:15:32, enne wrote:
> > I suspect you need to push the name here too.
> 
> Isn't the name pushed here: "layer->SetDebugName(debug_name_);"

Oh, quite right! I missed that entirely.

[qiankun, 2013-07-11 10:04:42]

https://codereview.chromium.org/16903005/diff/27001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/16903005/diff/27001/cc/layers/layer.cc#newcod...
cc/layers/layer.cc:681:
TRACE_EVENT_CATEGORY_GROUP_ENABLED(TRACE_DISABLED_BY_DEFAULT("cc.debug"),
On 2013/07/10 16:48:42, enne wrote:
> On 2013/07/10 04:53:07, qiankun wrote:
> 
> > Pull name then call SetDebugName() in ContentLayer::Update()? If it's
correct,
> I
> > can do this.
> 
> Yeah, that's exactly it.  Rather than pulling the name in PushPropertiesTo, do
> it in Update.
Done.

[enne (OOO), 2013-07-11 16:53:42]

lgtm

[enne (OOO), 2013-07-18 17:50:23]

https://codereview.chromium.org/16903005/diff/40001/webkit/renderer/composito...
File webkit/renderer/compositor_bindings/web_content_layer_impl.cc (right):

https://codereview.chromium.org/16903005/diff/40001/webkit/renderer/composito...
webkit/renderer/compositor_bindings/web_content_layer_impl.cc:70: return
UTF16ToASCII(client_->debugName());
This will assert if the name is not ascii:
https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin...

Surely there is a safer way to convert to std::string.

[jamesr, 2013-07-18 18:03:42]

https://codereview.chromium.org/16903005/diff/40001/webkit/renderer/composito...
File webkit/renderer/compositor_bindings/web_content_layer_impl.cc (right):

https://codereview.chromium.org/16903005/diff/40001/webkit/renderer/composito...
webkit/renderer/compositor_bindings/web_content_layer_impl.cc:70: return
UTF16ToASCII(client_->debugName());
On 2013/07/18 17:50:23, enne wrote:
> This will assert if the name is not ascii:
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin...
> 
> Surely there is a safer way to convert to std::string.

You probably want to call utf8() on the string and DCHECK() that it's ASCII (see
base/strings/string_util.h)

[qiankun, 2013-07-19 05:03:14]

On 2013/07/18 17:50:23, enne wrote:
>
https://codereview.chromium.org/16903005/diff/40001/webkit/renderer/composito...
> File webkit/renderer/compositor_bindings/web_content_layer_impl.cc (right):
> 
>
https://codereview.chromium.org/16903005/diff/40001/webkit/renderer/composito...
> webkit/renderer/compositor_bindings/web_content_layer_impl.cc:70: return
> UTF16ToASCII(client_->debugName());
> This will assert if the name is not ascii:
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin...
> 
> Surely there is a safer way to convert to std::string.

Suppose the name should be ascii, the assertion is correct behavior if it's not
ascii. What's the safer way do you think?

[qiankun, 2013-07-19 05:13:16]

On 2013/07/18 18:03:42, jamesr wrote:
>
https://codereview.chromium.org/16903005/diff/40001/webkit/renderer/composito...
> File webkit/renderer/compositor_bindings/web_content_layer_impl.cc (right):
> 
>
https://codereview.chromium.org/16903005/diff/40001/webkit/renderer/composito...
> webkit/renderer/compositor_bindings/web_content_layer_impl.cc:70: return
> UTF16ToASCII(client_->debugName());
> On 2013/07/18 17:50:23, enne wrote:
> > This will assert if the name is not ascii:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin...
> > 
> > Surely there is a safer way to convert to std::string.
> 
> You probably want to call utf8() on the string and DCHECK() that it's ASCII
(see
> base/strings/string_util.h)

No utf8() function in the mentioned file. I didn't understand clearly here.

[qiankun, 2013-07-19 08:06:37]

On 2013/07/19 05:13:16, qiankun wrote:
> On 2013/07/18 18:03:42, jamesr wrote:
> >
>
https://codereview.chromium.org/16903005/diff/40001/webkit/renderer/composito...
> > File webkit/renderer/compositor_bindings/web_content_layer_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/16903005/diff/40001/webkit/renderer/composito...
> > webkit/renderer/compositor_bindings/web_content_layer_impl.cc:70: return
> > UTF16ToASCII(client_->debugName());
> > On 2013/07/18 17:50:23, enne wrote:
> > > This will assert if the name is not ascii:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin...
> > > 
> > > Surely there is a safer way to convert to std::string.
> > 
> > You probably want to call utf8() on the string and DCHECK() that it's ASCII
> (see
> > base/strings/string_util.h)
> 
> No utf8() function in the mentioned file. I didn't understand clearly here.

@jamesr,
Do you mean: 
std::string name = client_->debugName().utf8();
DCHECK(IsStringASCII(name));
return name;

[jamesr, 2013-07-19 16:51:41]

Yup! If the string is ASCII this will actually just be one string copy and
no Unicode conversions. If we decide to support non- ASCII names then it's
just a matter of removing that dcheck.

[enne (OOO), 2013-07-19 17:02:06]

On 2013/07/19 16:51:41, jamesr wrote:
> Yup! If the string is ASCII this will actually just be one string copy and
> no Unicode conversions. If we decide to support non- ASCII names then it's
> just a matter of removing that dcheck.

I mentioned this because I hit a DCHECK yesterday on the existing UTF16ToASCII
conversion in WebLayerImpl, so was thinking that maybe we should be robust to
that.

[qiankun, 2013-07-26 08:44:03]

On 2013/07/19 17:02:06, enne wrote:
> On 2013/07/19 16:51:41, jamesr wrote:
> > Yup! If the string is ASCII this will actually just be one string copy and
> > no Unicode conversions. If we decide to support non- ASCII names then it's
> > just a matter of removing that dcheck.
> 
> I mentioned this because I hit a DCHECK yesterday on the existing UTF16ToASCII
> conversion in WebLayerImpl, so was thinking that maybe we should be robust to
> that.

Fix this as jamesr suggested. 

I updated the patch to add a WebLayerClient for pulling names.

[enne (OOO), 2013-08-01 22:12:59]

https://codereview.chromium.org/16903005/diff/58001/cc/layers/content_layer.h
File cc/layers/content_layer.h (right):

https://codereview.chromium.org/16903005/diff/58001/cc/layers/content_layer.h...
cc/layers/content_layer.h:8: #include <string>
This include can go away now.

https://codereview.chromium.org/16903005/diff/58001/cc/layers/content_layer_c...
File cc/layers/content_layer_client.h (right):

https://codereview.chromium.org/16903005/diff/58001/cc/layers/content_layer_c...
cc/layers/content_layer_client.h:8: #include <string>
As can this?

https://codereview.chromium.org/16903005/diff/58001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/16903005/diff/58001/cc/layers/layer.cc#newcod...
cc/layers/layer.cc:847: bool is_tracing;
Could you do this during Layer::PushPropertiesTo instead of Layer::Update? Not
every layer gets updated.  This would also remove the need to make sure that
every derived layer called Layer::Update.

https://codereview.chromium.org/16903005/diff/58001/webkit/renderer/composito...
File webkit/renderer/compositor_bindings/web_content_layer_impl.h (right):

https://codereview.chromium.org/16903005/diff/58001/webkit/renderer/composito...
webkit/renderer/compositor_bindings/web_content_layer_impl.h:8: #include
<string>
Is this include necessary?

[qiankun, 2013-08-02 17:50:26]

PTAL. Thanks:)

https://codereview.chromium.org/16903005/diff/58001/cc/layers/content_layer.h
File cc/layers/content_layer.h (right):

https://codereview.chromium.org/16903005/diff/58001/cc/layers/content_layer.h...
cc/layers/content_layer.h:8: #include <string>
On 2013/08/01 22:12:59, enne wrote:
> This include can go away now.

Done.

https://codereview.chromium.org/16903005/diff/58001/cc/layers/content_layer_c...
File cc/layers/content_layer_client.h (right):

https://codereview.chromium.org/16903005/diff/58001/cc/layers/content_layer_c...
cc/layers/content_layer_client.h:8: #include <string>
On 2013/08/01 22:12:59, enne wrote:
> As can this?

Done.

https://codereview.chromium.org/16903005/diff/58001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/16903005/diff/58001/cc/layers/layer.cc#newcod...
cc/layers/layer.cc:847: bool is_tracing;
On 2013/08/01 22:12:59, enne wrote:
> Could you do this during Layer::PushPropertiesTo instead of Layer::Update? Not
> every layer gets updated.  This would also remove the need to make sure that
> every derived layer called Layer::Update.

Done.

https://codereview.chromium.org/16903005/diff/58001/webkit/renderer/composito...
File webkit/renderer/compositor_bindings/web_content_layer_impl.h (right):

https://codereview.chromium.org/16903005/diff/58001/webkit/renderer/composito...
webkit/renderer/compositor_bindings/web_content_layer_impl.h:8: #include
<string>
On 2013/08/01 22:12:59, enne wrote:
> Is this include necessary?

Done.

[enne (OOO), 2013-08-02 17:58:17]

https://codereview.chromium.org/16903005/diff/69001/cc/layers/heads_up_displa...
File cc/layers/heads_up_display_layer.cc (right):

https://codereview.chromium.org/16903005/diff/69001/cc/layers/heads_up_displa...
cc/layers/heads_up_display_layer.cc:60: TRACE_EVENT_CATEGORY_GROUP_ENABLED("cc",
&is_tracing);
No need for the trace event check here.  The caller can do that.

https://codereview.chromium.org/16903005/diff/69001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/16903005/diff/69001/cc/layers/layer.cc#newcod...
cc/layers/layer.cc:740: TRACE_EVENT_CATEGORY_GROUP_ENABLED("cc", &is_tracing);
I think "cc" should instead be TRACE_DISABLED_BY_DEFAULT("cc.debug").  It seems
like layer names are only useful when dumping frames to the frame viewer.

https://codereview.chromium.org/16903005/diff/69001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/16903005/diff/69001/cc/layers/layer.h#newcode296
cc/layers/layer.h:296: void SetDebugName(const std::string& debug_name);
Can you remove SetDebugName?

https://codereview.chromium.org/16903005/diff/69001/webkit/renderer/composito...
File webkit/renderer/compositor_bindings/web_layer_impl.h (right):

https://codereview.chromium.org/16903005/diff/69001/webkit/renderer/composito...
webkit/renderer/compositor_bindings/web_layer_impl.h:127: virtual std::string
DebugName() OVERRIDE;
Please don't use OVERRIDE here even though it is an override.  This is a
cross-repo OVERRIDE, so it makes it changes harder in the future.

[qiankun, 2013-08-05 06:54:14]

Thanks for review:) 
Updated the patch, PTAL.

https://codereview.chromium.org/16903005/diff/69001/cc/layers/heads_up_displa...
File cc/layers/heads_up_display_layer.cc (right):

https://codereview.chromium.org/16903005/diff/69001/cc/layers/heads_up_displa...
cc/layers/heads_up_display_layer.cc:60: TRACE_EVENT_CATEGORY_GROUP_ENABLED("cc",
&is_tracing);
On 2013/08/02 17:58:17, enne wrote:
> No need for the trace event check here.  The caller can do that.

Done.

https://codereview.chromium.org/16903005/diff/69001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/16903005/diff/69001/cc/layers/layer.cc#newcod...
cc/layers/layer.cc:740: TRACE_EVENT_CATEGORY_GROUP_ENABLED("cc", &is_tracing);
On 2013/08/02 17:58:17, enne wrote:
> I think "cc" should instead be TRACE_DISABLED_BY_DEFAULT("cc.debug").  It
seems
> like layer names are only useful when dumping frames to the frame viewer.

Done.

https://codereview.chromium.org/16903005/diff/69001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/16903005/diff/69001/cc/layers/layer.h#newcode296
cc/layers/layer.h:296: void SetDebugName(const std::string& debug_name);
On 2013/08/02 17:58:17, enne wrote:
> Can you remove SetDebugName?

Done.

https://codereview.chromium.org/16903005/diff/69001/webkit/renderer/composito...
File webkit/renderer/compositor_bindings/web_layer_impl.h (right):

https://codereview.chromium.org/16903005/diff/69001/webkit/renderer/composito...
webkit/renderer/compositor_bindings/web_layer_impl.h:127: virtual std::string
DebugName() OVERRIDE;
On 2013/08/02 17:58:17, enne wrote:
> Please don't use OVERRIDE here even though it is an override.  This is a
> cross-repo OVERRIDE, so it makes it changes harder in the future.

Done.

[enne (OOO), 2013-08-05 16:54:29]

https://codereview.chromium.org/16903005/diff/77001/cc/layers/layer_client.h
File cc/layers/layer_client.h (right):

https://codereview.chromium.org/16903005/diff/77001/cc/layers/layer_client.h#...
cc/layers/layer_client.h:16: virtual std::string DebugName() = 0;
style nit: 2 space indent.

[enne (OOO), 2013-08-06 16:31:55]

lgtm

[qiankun, 2013-08-14 10:32:32]

Add back "OVERRIDE" for virtual std::string DebugName() OVERRIDE; in
web_layer_impl.h. It's not a cross-repo OVERRIDE referring virtual void
PaintContents() in web_content_layer_impl.h.

[commit-bot: I haz the power, 2013-08-16 10:30:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qiankun.miao@intel.com/16903005/89001

[commit-bot: I haz the power, 2013-08-16 13:04:27]

Change committed as 218007