Remove blink::WebLayerClient and WebGraphicsLayerDebugInfo.

Remove blink::WebLayerClient and WebGraphicsLayerDebugInfo.

It is now legal for Blink to provide an implementation of cc::LayerClient.

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/69698f992d3c6b2badd01f718c71df7a52b099fb
Cr-Commit-Position: refs/heads/master@{#360855}

[jbroman, 2015-10-22 15:42:54]

Description was changed from

==========
Remove blink::WebLayerClient.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Remove blink::WebLayerClient and WebGraphicsLayerDebugInfo.

It is now legal for Blink to provide an implementation of cc::LayerClient.

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[jbroman, 2015-10-22 20:07:38]

Patchset #7 (id:120001) has been deleted

[jbroman, 2015-10-23 15:37:52]

jbroman@chromium.org changed reviewers:
+ chrishtr@chromium.org, danakj@chromium.org, enne@chromium.org,
pdr@chromium.org

[jbroman, 2015-10-23 15:37:53]

Apologies for the large reviewer list, but it seemed appropriate given this CL
may form precedent.

To summarize:
- cc::LayerClient::TakeDebugInfo gains a cc::Layer* argument so that clients for
multiple layers, like GraphicsLayer, can tell which cc::Layer is being asked
about
- GraphicsLayer implements cc::LayerClient and uses
base::trace_event::TracedValue to provide its traced data
- WebLayer supports setting the cc::LayerClient, and getting the cc::Layer back
(used to check which cc::Layer is being asked about) to support the previous
- WebLayerClient and WebGraphicsLayerDebugInfo go away
- blink_platform gains a public dependency on both base and cc
- gn's public_deps automatically forwards the dynamic link dependency to
webcore, but I don't know if this can be done with gyp; to fix a Win component
dbg link failure, I've made webcore_shared link to base (since merely including
base/memory/ref_counted.h causes a dependency)

Removes about 140 lines of plumbing and hopefully paves the way for future
blink-cc integration. Thoughts?

[chrishtr, 2015-10-23 17:23:13]

https://codereview.chromium.org/1419913002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/GraphicsLayerDebugInfo.cpp
(right):

https://codereview.chromium.org/1419913002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/GraphicsLayerDebugInfo.cpp:50:
tracedValue->BeginDictionary();
Why these changes?

[jbroman, 2015-10-23 17:33:35]

https://codereview.chromium.org/1419913002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/GraphicsLayerDebugInfo.cpp
(right):

https://codereview.chromium.org/1419913002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/GraphicsLayerDebugInfo.cpp:50:
tracedValue->BeginDictionary();
On 2015/10/23 at 17:23:13, chrishtr wrote:
> Why these changes?

Need to produce a base::trace_event::ConvertableToTraceValue to implement
cc::LayerClient, rather than a WebGraphicsLayerDebugInfo.
base::trace_event::TracedValue provides a convenient way of doing so, and also
removes the separate clone operation that was previously required on
GraphicsLayerDebugInfo.

[chrishtr, 2015-10-23 17:34:54]

https://codereview.chromium.org/1419913002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/GraphicsLayerDebugInfo.cpp
(right):

https://codereview.chromium.org/1419913002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/GraphicsLayerDebugInfo.cpp:50:
tracedValue->BeginDictionary();
On 2015/10/23 at 17:33:35, jbroman wrote:
> On 2015/10/23 at 17:23:13, chrishtr wrote:
> > Why these changes?
> 
> Need to produce a base::trace_event::ConvertableToTraceValue to implement
cc::LayerClient, rather than a WebGraphicsLayerDebugInfo.
base::trace_event::TracedValue provides a convenient way of doing so, and also
removes the separate clone operation that was previously required on
GraphicsLayerDebugInfo.

Is base::trace_event now allowed in Blink? I think that was decided but checking
to be sure.

[jbroman, 2015-10-23 17:47:13]

To be clear, it's not urgent that this land, but this seems like a small and
reasonably self-contained way to test the waters for this kind of change.

https://codereview.chromium.org/1419913002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/GraphicsLayerDebugInfo.cpp
(right):

https://codereview.chromium.org/1419913002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/GraphicsLayerDebugInfo.cpp:50:
tracedValue->BeginDictionary();
On 2015/10/23 at 17:34:54, chrishtr wrote:
> On 2015/10/23 at 17:33:35, jbroman wrote:
> > On 2015/10/23 at 17:23:13, chrishtr wrote:
> > > Why these changes?
> > 
> > Need to produce a base::trace_event::ConvertableToTraceValue to implement
cc::LayerClient, rather than a WebGraphicsLayerDebugInfo.
base::trace_event::TracedValue provides a convenient way of doing so, and also
removes the separate clone operation that was previously required on
GraphicsLayerDebugInfo.
> 
> Is base::trace_event now allowed in Blink? I think that was decided but
checking to be sure.

My understanding is "Don't use base's TRACE_EVENT instead of Blink's, but using
base types to implement cc interfaces is now OK inside Source/platform/."

I think Elliott is working on a more long-term plan (but maybe I should add him
to this CL, too), but my reading of
https://groups.google.com/a/chromium.org/d/msg/blink-dev/kjoJSffteFc/ez3yMs8-...
suggests that people are generally OK with using base to talk to cc.

[danakj, 2015-10-23 17:53:39]

LGTM

https://codereview.chromium.org/1419913002/diff/140001/cc/blink/web_layer_imp...
File cc/blink/web_layer_impl.cc (right):

https://codereview.chromium.org/1419913002/diff/140001/cc/blink/web_layer_imp...
cc/blink/web_layer_impl.cc:488: const cc::Layer* WebLayerImpl::ccLayer() const {
part of me wonders if this should return void* to avoid things randomly skipping
thru the WebLayer while it exists, but then that might be a good transition path
actually to move things over a few CLs. maybe make it void* until you are
actively removing WebLayer?

https://codereview.chromium.org/1419913002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/DEPS (right):

https://codereview.chromium.org/1419913002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/DEPS:2: "+cc",
also -cc/blink

[jbroman, 2015-10-23 17:59:00]

https://codereview.chromium.org/1419913002/diff/140001/cc/blink/web_layer_imp...
File cc/blink/web_layer_impl.cc (right):

https://codereview.chromium.org/1419913002/diff/140001/cc/blink/web_layer_imp...
cc/blink/web_layer_impl.cc:488: const cc::Layer* WebLayerImpl::ccLayer() const {
On 2015/10/23 at 17:53:39, danakj wrote:
> part of me wonders if this should return void* to avoid things randomly
skipping thru the WebLayer while it exists, but then that might be a good
transition path actually to move things over a few CLs. maybe make it void*
until you are actively removing WebLayer?

If people (including the whole of you) would like me to change this, I can.
Given it's no longer illegal for Blink to know about cc::Layer's existence, I
didn't see a reason to obscure the type, but I don't really have a reason to
make the type visible other than clarity.

https://codereview.chromium.org/1419913002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/DEPS (right):

https://codereview.chromium.org/1419913002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/DEPS:2: "+cc",
On 2015/10/23 at 17:53:39, danakj wrote:
> also -cc/blink

Done.

[chrishtr, 2015-10-23 18:00:11]

lgtm

[danakj, 2015-10-23 18:01:04]

https://codereview.chromium.org/1419913002/diff/140001/cc/blink/web_layer_imp...
File cc/blink/web_layer_impl.cc (right):

https://codereview.chromium.org/1419913002/diff/140001/cc/blink/web_layer_imp...
cc/blink/web_layer_impl.cc:488: const cc::Layer* WebLayerImpl::ccLayer() const {
On 2015/10/23 17:58:59, jbroman wrote:
> On 2015/10/23 at 17:53:39, danakj wrote:
> > part of me wonders if this should return void* to avoid things randomly
> skipping thru the WebLayer while it exists, but then that might be a good
> transition path actually to move things over a few CLs. maybe make it void*
> until you are actively removing WebLayer?
> 
> If people (including the whole of you) would like me to change this, I can.
> Given it's no longer illegal for Blink to know about cc::Layer's existence, I
> didn't see a reason to obscure the type, but I don't really have a reason to
> make the type visible other than clarity.

It'll just be weird if the WebLayer API still exists but new cc::Layer apis
start just bypassing it, we'll get into an inconsistent code-spaghetti land is
my concern. This only matters if WebLayer is going to stick around for a long
time. (Hopefully not?)

[esprehn, 2015-10-23 19:17:40]

esprehn@chromium.org changed reviewers:
+ esprehn@chromium.org

[esprehn, 2015-10-23 19:17:41]

Lets hold off on this for now, please be patient. We need better enforcement of
what can be used where and a written policy which I'm working on. In particular
using base or cc types from inside core/ should fail to compile.

WebLayer will need to continue to exist as long as blink is making compositing
decisions inside core/, there's currently no plan to use cc or base inside core.

https://codereview.chromium.org/1419913002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp (right):

https://codereview.chromium.org/1419913002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp:1174:
tracedValue->SetString("layer_name", std::string(layerName.data(),
layerName.length()));
fyi: this is multiple copies of the string, once in utf8(), then here when you
convert to std::string

https://codereview.chromium.org/1419913002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/GraphicsLayerDebugInfo.h
(right):

https://codereview.chromium.org/1419913002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/GraphicsLayerDebugInfo.h:53:
scoped_refptr<base::trace_event::TracedValue> asTracedValue() const;
we need to come up with a way to enforce that base isn't used outside platform.

[esprehn, 2015-10-23 19:20:01]

fyi: this patch is totally reasonable from the current layering idea and I'll
probably use it as an example in the docs, we just need automated rules instead
of me being on all the patches before opening the flood gates. Can you wait
until next Friday? :)

[jbroman, 2015-10-23 19:26:24]

I'm not sure exactly what the path for WebLayer will be. Most of our compositing
logic in core uses GraphicsLayer rather than WebLayer, but that isn't
universally true.

Maybe we could migrate those to GraphicsLayer, or maybe they could get a
different WebLayer wrapper in Source/platform/ that handled whatever reason they
have for using WebLayer instead of GraphicsLayer.

Waiting until next Friday is no big deal from my perspective; this isn't
blocking anything aside from the fact that I want to have precedent before I
start trying to do more important things through a Blink->cc dependency. :)

https://codereview.chromium.org/1419913002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp (right):

https://codereview.chromium.org/1419913002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp:1174:
tracedValue->SetString("layer_name", std::string(layerName.data(),
layerName.length()));
On 2015/10/23 at 19:17:41, esprehn wrote:
> fyi: this is multiple copies of the string, once in utf8(), then here when you
convert to std::string

I know; maybe I should have left a TODO. There are a number of ways this could
be solved. My personal favourite is implementing WTF::CString::operator
StringPiece, and then making TracedValue::SetString take a StringPiece (since
all it does is forward to something that does).

The first copy isn't always avoidable because String can be 16-bit, and I
couldn't think of an unobtrusive way to avoid that one when possible.

https://codereview.chromium.org/1419913002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/GraphicsLayerDebugInfo.h
(right):

https://codereview.chromium.org/1419913002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/GraphicsLayerDebugInfo.h:53:
scoped_refptr<base::trace_event::TracedValue> asTracedValue() const;
On 2015/10/23 at 19:17:41, esprehn wrote:
> we need to come up with a way to enforce that base isn't used outside
platform.

For some reasonable definition of "use", I hope. Unfortunately "can end up
transitively including a header" is pretty hard because you need class
definitions to be able to do a lot of entirely reasonable things, like implement
an interface.

[enne (OOO), 2015-10-23 21:03:32]

This is great.  lgtm in general.

[jbroman, 2015-11-13 00:49:33]

On 2015/10/23 at 19:20:01, esprehn wrote:
> fyi: this patch is totally reasonable from the current layering idea and I'll
probably use it as an example in the docs, we just need automated rules instead
of me being on all the patches before opening the flood gates. Can you wait
until next Friday? :)

Dunno about automated rules, but should I rebase and land this now that you've
given your Onion Soup talk, or should I keep waiting? :)

[danakj, 2015-11-16 21:47:16]

https://codereview.chromium.org/1419913002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/DEPS (right):

https://codereview.chromium.org/1419913002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/DEPS:2: "+cc",
This seems to cover the automating part.

[chrishtr, 2015-11-17 00:21:49]

https://codereview.chromium.org/1419913002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/GraphicsLayerDebugInfo.h
(right):

https://codereview.chromium.org/1419913002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/GraphicsLayerDebugInfo.h:53:
scoped_refptr<base::trace_event::TracedValue> asTracedValue() const;
Elliott communicated it offline I think, but for completeness: I just also asked
him about the patch, and it's ok to commit once a presumbit rule is added that
disallows the string "scoped_refptr" in Blink.

[chrishtr, 2015-11-17 00:24:11]

https://codereview.chromium.org/1419913002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/GraphicsLayerDebugInfo.h
(right):

https://codereview.chromium.org/1419913002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/GraphicsLayerDebugInfo.h:53:
scoped_refptr<base::trace_event::TracedValue> asTracedValue() const;
On 2015/11/17 at 00:21:49, chrishtr wrote:
> Elliott communicated it offline I think, but for completeness: I just also
asked him about the patch, and it's ok to commit once a presumbit rule is added
that disallows the string "scoped_refptr" in Blink.

s/Blink/core/

[jbroman, 2015-11-20 16:51:15]

OK, so a PRESUBMIT rule is now in place, in addition to DEPS:
  https://codereview.chromium.org/1453743002

And base::trace_event::TracedValue now accepts StringPiece:
  https://codereview.chromium.org/1451943002

Which StringUTF8Adaptor can now produce:
  https://codereview.chromium.org/1458513002

And now this CL uses those to remove the extra copies.
Now, compositing and paint invalidation reasons only do one
copy into the TracedValue (directly from the const char*),
and layer name is the same if it is ASCII (in this case the
WTF::String's Latin-1 buffer is used directly); otherwise
one more pass is needed to do UTF-8 encoding.

esprehn, I think this addresses your concerns?

[esprehn, 2015-11-20 17:16:52]

Lgtm!

[jbroman, 2015-11-20 17:38:26]

The CQ bit was checked by jbroman@chromium.org

[jbroman, 2015-11-20 17:38:26]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
chrishtr@chromium.org, enne@chromium.org
Link to the patchset: https://codereview.chromium.org/1419913002/#ps200001
(title: "merge with origin/master, use base::StringPiece to avoid copies, and
obey `gn format`")

[commit-bot: I haz the power, 2015-11-20 17:38:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1419913002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1419913002/200001

[commit-bot: I haz the power, 2015-11-20 17:44:06]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2015-11-20 17:45:11]

Patchset 10 (id:??) landed as
https://crrev.com/69698f992d3c6b2badd01f718c71df7a52b099fb
Cr-Commit-Position: refs/heads/master@{#360855}

[David Yen, 2015-11-20 19:51:58]

A revert of this CL (patchset #10 id:200001) has been created in
https://codereview.chromium.org/1461423002/ by dyen@chromium.org.

The reason for reverting is: Looks to be causing bot failures for
ContextLost.WebGLContextLostInHiddenTab.