cc: Chromify Layer and LayerImpl classes.

cc: Chromify Layer and LayerImpl classes.

Style-only change. Bring the Layer and LayerImpl classes
into the Chromium style.

R=enne,piman,jamesr
TBR=joth

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

[danakj, 2013-03-11 23:02:42]

There are some simple accessors on Layer that need to be CapitalLetters because
they are not simple on LayerImpl and templates want them to match. Probably
vice-versa also.

[danakj, 2013-03-11 23:03:11]

+piman for content/ ui/ and webkit/plugins/

[danakj, 2013-03-11 23:11:29]

Patch set 4 actually managed to fully upload. I think I'll just make any further
changes locally and dcommit at the end, if that's acceptable.

[piman, 2013-03-11 23:54:46]

On 2013/03/11 23:03:11, danakj wrote:
> +piman for content/ ui/ and webkit/plugins/

LGTM for those.

[enne (OOO), 2013-03-12 02:40:19]

lgtm

THANK YOU.  This is a heroic effort.

https://codereview.chromium.org/12774006/diff/20017/cc/layer.cc
File cc/layer.cc (right):

https://codereview.chromium.org/12774006/diff/20017/cc/layer.cc#newcode21
cc/layer.cc:21: static int s_next_layer_id = 1;
I thought this was weird, but the style guide really has no guidance on this
name.

https://codereview.chromium.org/12774006/diff/20017/cc/layer.h
File cc/layer.h (right):

https://codereview.chromium.org/12774006/diff/20017/cc/layer.h#newcode477
cc/layer.h:477: };
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/12774006/diff/20017/cc/layer_impl.cc
File cc/layer_impl.cc (right):

https://codereview.chromium.org/12774006/diff/20017/cc/layer_impl.cc#newcode592
cc/layer_impl.cc:592: return;
Also needs braces to match if clause.

https://codereview.chromium.org/12774006/diff/20017/cc/layer_impl.cc#newcode613
cc/layer_impl.cc:613: return;
Braces.

https://codereview.chromium.org/12774006/diff/20017/cc/layer_impl.h
File cc/layer_impl.h (right):

https://codereview.chromium.org/12774006/diff/20017/cc/layer_impl.h#newcode422
cc/layer_impl.h:422: // mask_layer_ can be temporarily stolen during tree sync,
we need this ID to confirm newly assigned layer is still the previous one
80 col

https://codereview.chromium.org/12774006/diff/20017/cc/layer_impl.h#newcode452
cc/layer_impl.h:452: // For layers that do not own a surface this flag acts as
layer_property_changed_.
80 col.

https://codereview.chromium.org/12774006/diff/20017/cc/layer_impl.h#newcode470
cc/layer_impl.h:470: // This is true if the layer should be fixed to the closest
ancestor container.
80 col.

[danakj, 2013-03-12 02:47:16]

https://codereview.chromium.org/12774006/diff/20017/cc/layer.cc
File cc/layer.cc (right):

https://codereview.chromium.org/12774006/diff/20017/cc/layer.cc#newcode21
cc/layer.cc:21: static int s_next_layer_id = 1;
On 2013/03/12 02:40:19, enne wrote:
> I thought this was weird, but the style guide really has no guidance on this
> name.

Yeh.. not sure.

https://codereview.chromium.org/12774006/diff/20017/cc/layer.h
File cc/layer.h (right):

https://codereview.chromium.org/12774006/diff/20017/cc/layer.h#newcode477
cc/layer.h:477: };
On 2013/03/12 02:40:19, enne wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/12774006/diff/20017/cc/layer_impl.cc
File cc/layer_impl.cc (right):

https://codereview.chromium.org/12774006/diff/20017/cc/layer_impl.cc#newcode592
cc/layer_impl.cc:592: return;
On 2013/03/12 02:40:19, enne wrote:
> Also needs braces to match if clause.

Done.

https://codereview.chromium.org/12774006/diff/20017/cc/layer_impl.cc#newcode613
cc/layer_impl.cc:613: return;
On 2013/03/12 02:40:19, enne wrote:
> Braces.

Done.

https://codereview.chromium.org/12774006/diff/20017/cc/layer_impl.h
File cc/layer_impl.h (right):

https://codereview.chromium.org/12774006/diff/20017/cc/layer_impl.h#newcode422
cc/layer_impl.h:422: // mask_layer_ can be temporarily stolen during tree sync,
we need this ID to confirm newly assigned layer is still the previous one
On 2013/03/12 02:40:19, enne wrote:
> 80 col

Done.

https://codereview.chromium.org/12774006/diff/20017/cc/layer_impl.h#newcode452
cc/layer_impl.h:452: // For layers that do not own a surface this flag acts as
layer_property_changed_.
On 2013/03/12 02:40:19, enne wrote:
> 80 col.

Done.

https://codereview.chromium.org/12774006/diff/20017/cc/layer_impl.h#newcode470
cc/layer_impl.h:470: // This is true if the layer should be fixed to the closest
ancestor container.
On 2013/03/12 02:40:19, enne wrote:
> 80 col.

Done.

[commit-bot: I haz the power, 2013-03-12 03:02:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/12774006/23037

[commit-bot: I haz the power, 2013-03-12 03:20:51]

Failed to trigger a try job on linux_clang
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-03-12 03:25:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/12774006/22049

[commit-bot: I haz the power, 2013-03-12 03:54:15]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-03-12 04:00:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/12774006/22077

[commit-bot: I haz the power, 2013-03-12 04:28:48]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-03-12 04:43:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/12774006/4124

[commit-bot: I haz the power, 2013-03-12 04:44:32]

Presubmit check for 12774006-4124 failed and returned exit status 1.

INFO:root:Found 150 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/cc/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/ui/compositor/PRESUBMIT.py

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    webkit/plugins/npapi/webplugin_impl.cc
    android_webview/browser/browser_view_renderer_impl.cc

Presubmit checks took 9.0s to calculate.

[danakj, 2013-03-12 04:51:12]

TBR=piman for the iosurfacelayer renames in webkit/plugins/

[commit-bot: I haz the power, 2013-03-12 04:52:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/12774006/4124

[commit-bot: I haz the power, 2013-03-12 05:25:21]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[jamesr, 2013-03-12 06:23:25]

lgtm for webkit/

[commit-bot: I haz the power, 2013-03-12 07:09:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/12774006/43002

[commit-bot: I haz the power, 2013-03-12 07:11:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/12774006/43002

[commit-bot: I haz the power, 2013-03-12 07:13:01]

Presubmit check for 12774006-43002 failed and returned exit status 1.

INFO:root:Found 150 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/cc/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/ui/compositor/PRESUBMIT.py

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    android_webview/browser/browser_view_renderer_impl.cc

Presubmit checks took 8.9s to calculate.

[danakj, 2013-03-12 07:14:24]

TBR=joth for android_webview/browser/browser_view_renderer_impl.cc

[commit-bot: I haz the power, 2013-03-12 07:15:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/12774006/43002

[commit-bot: I haz the power, 2013-03-12 10:17:38]

Change committed as 187555

[joth, 2013-03-12 16:00:55]

lgtm

[Tom Hudson, 2013-03-12 17:12:32]

On 2013/03/12 02:40:19, enne wrote:
> THANK YOU.  This is a heroic effort.

This is good work to be doing, but could we make future changes less heroic?
Chrome for Android broke, and figuring out where and how is proving pretty
difficult with 6000+ lines changed.

[enne (OOO), 2013-03-12 17:26:42]

Chrome for Android broke how? The presubmit build and tests ran fine.

[danakj, 2013-03-12 17:30:49]

On Tue, Mar 12, 2013 at 10:12 AM, <tomhudson@chromium.org> wrote:

> On 2013/03/12 02:40:19, enne wrote:
>
>> THANK YOU.  This is a heroic effort.
>>
>
> This is good work to be doing, but could we make future changes less
> heroic?
> Chrome for Android broke, and figuring out where and how is proving pretty
> difficult with 6000+ lines changed.
>

Sorry, we couldn't change one class without changing the other due to
templates. This should be the worst of them. I'll be doing LayerTreeHost
today, and from there it's mostly trivial for embedders I'd think.


>
>
https://chromiumcodereview.**appspot.com/12774006/<https://chromiumcodereview...
>

[Tom Hudson, 2013-03-12 17:31:39]

On 2013/03/12 17:26:42, enne wrote:
> Chrome for Android broke how? The presubmit build and tests ran fine.

The (non-public) Android UI for Chrome has several subclasses of cc::Layer and
cc::LayerImpl, and when I did an analogous renaming there we started seeing
failures like:

F/chromium( 7210): [FATAL:tree_synchronizer.cc(153)] Check failed: layer->id()
== layerImpl->id() (26 vs. 363)

[piman, 2013-03-12 17:37:38]

On Tue, Mar 12, 2013 at 10:31 AM, <tomhudson@chromium.org> wrote:

> On 2013/03/12 17:26:42, enne wrote:
>
>> Chrome for Android broke how? The presubmit build and tests ran fine.
>>
>
> The (non-public) Android UI for Chrome has several subclasses of cc::Layer
> and
> cc::LayerImpl


That's rather problematic. It's certainly not the intended use of cc/ and
violates layering, let alone makes it very brittle to API changes.
What can we do to upstream those classes ?

Antoine


> , and when I did an analogous renaming there we started seeing
> failures like:
>
> F/chromium( 7210): [FATAL:tree_synchronizer.cc(**153)] Check failed:
> layer->id()
> == layerImpl->id() (26 vs. 363)
>
>
>
https://chromiumcodereview.**appspot.com/12774006/<https://chromiumcodereview...
>

[jamesr, 2013-03-12 17:37:38]

On 2013/03/12 17:31:39, Tom Hudson wrote:
> On 2013/03/12 17:26:42, enne wrote:
> > Chrome for Android broke how? The presubmit build and tests ran fine.
> 
> The (non-public) Android UI for Chrome has several subclasses of cc::Layer and
> cc::LayerImpl

Wow, that's an astoundingly *terrible* idea.  Who on earth thought that
subclassing another component's classes without even telling them was OK?

[Tom Hudson, 2013-03-12 17:53:10]

On 2013/03/12 17:31:39, Tom Hudson wrote:
> ...when I did an analogous renaming there...

Predictably enough, incomplete renamings fail, and clang (and clholgat and
tedchoc) are all better than I am at spotting missed OVERRIDEs and the like.

Android's immediate problem seems to be fixed.