StaticTabSceneLayer owns ToolbarLayer

StaticTabSceneLayer owns ToolbarLayer

Since StaticLayout is the only layout that needs the toolbar layout,
its ownership has been moved to StaticLayout's SceneLayer. This puts
the ToolbarLayer in the content tree for SceneOverlays.

BUG=584340
Committed: https://crrev.com/94e6d2cbf7bf2691014401bc11be703dc45f43b0
Cr-Commit-Position: refs/heads/master@{#393696}

[mdjones, 2016-02-23 00:54:50]

mdjones@chromium.org changed reviewers:
+ dtrainor@chromium.org

[mdjones, 2016-02-23 00:54:51]

PTAL. What I might do is have many partial reviews like this one and have a
final huge CL that combines them all. That way, I won't temporarily break
anything.

The toolbar could probably be its own scene layer, I chose to piggyback on
StaticTabSceneLayer since it now owns it.

[David Trainor- moved to gerrit, 2016-02-24 17:06:10]

https://codereview.chromium.org/1706293005/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/StaticTabSceneLayer.java
(right):

https://codereview.chromium.org/1706293005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/StaticTabSceneLayer.java:33:
private Context mContext;
final?

https://codereview.chromium.org/1706293005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/StaticTabSceneLayer.java:38:
private boolean mIsTablet;
final?

https://codereview.chromium.org/1706293005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/StaticTabSceneLayer.java:87:
public void updateToolbarLayer(float dpToPx, int topControlsBackgroundColor,
I'm assuming most of this logic is a copy from
CompositorView#updateToolbarLayer?

https://codereview.chromium.org/1706293005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/StaticTabSceneLayer.java:89:
ResourceManager resourceManager, Layout layout) {
It seems like there are 4 things we need Layout for:

1. forceHideTopControlsAndroidView
2. GetTopControlsOffset
3. GetToolbarBrightness
4. GetSizingFlags

It would be better to just pipe these in this method and remove the methods
above that aren't called anywhere else (the first three I bet?).

https://codereview.chromium.org/1706293005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/StaticTabSceneLayer.java:100:
if (fullscreenManager == null) return;
Should we check this before dereferencing it on line 92?

https://codereview.chromium.org/1706293005/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java
(right):

https://codereview.chromium.org/1706293005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java:382:
return (ControlContainer) mControlContainer;
Do we make this assumption anywhere else in this file?

https://codereview.chromium.org/1706293005/diff/40001/chrome/browser/android/...
File chrome/browser/android/compositor/scene_layer/static_tab_scene_layer.cc
(right):

https://codereview.chromium.org/1706293005/diff/40001/chrome/browser/android/...
chrome/browser/android/compositor/scene_layer/static_tab_scene_layer.cc:148:
layer_->AddChild(layer);
Can't we just insert this layer at the beginning so we don't have to readd the
toolbar layer to move it to the end?

[mdjones, 2016-02-25 21:38:01]

https://codereview.chromium.org/1706293005/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/StaticTabSceneLayer.java
(right):

https://codereview.chromium.org/1706293005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/StaticTabSceneLayer.java:33:
private Context mContext;
On 2016/02/24 17:06:10, David Trainor wrote:
> final?

Done.

https://codereview.chromium.org/1706293005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/StaticTabSceneLayer.java:38:
private boolean mIsTablet;
On 2016/02/24 17:06:10, David Trainor wrote:
> final?

Done.

https://codereview.chromium.org/1706293005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/StaticTabSceneLayer.java:87:
public void updateToolbarLayer(float dpToPx, int topControlsBackgroundColor,
On 2016/02/24 17:06:10, David Trainor wrote:
> I'm assuming most of this logic is a copy from
> CompositorView#updateToolbarLayer?

That is correct. Some things have been removed since they aren't relevant in
this context.

https://codereview.chromium.org/1706293005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/StaticTabSceneLayer.java:89:
ResourceManager resourceManager, Layout layout) {
On 2016/02/24 17:06:10, David Trainor wrote:
> It seems like there are 4 things we need Layout for:
> 
> 1. forceHideTopControlsAndroidView
> 2. GetTopControlsOffset
> 3. GetToolbarBrightness
> 4. GetSizingFlags
> 
> It would be better to just pipe these in this method and remove the methods
> above that aren't called anywhere else (the first three I bet?).

1 and 4 need to stick around, I removed 2 and 3 and their usages in general.

https://codereview.chromium.org/1706293005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/StaticTabSceneLayer.java:100:
if (fullscreenManager == null) return;
On 2016/02/24 17:06:10, David Trainor wrote:
> Should we check this before dereferencing it on line 92?

Yes

https://codereview.chromium.org/1706293005/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java
(right):

https://codereview.chromium.org/1706293005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java:382:
return (ControlContainer) mControlContainer;
On 2016/02/24 17:06:10, David Trainor wrote:
> Do we make this assumption anywhere else in this file?

Changed to store a ControlContainer with new interface method "getView".

https://codereview.chromium.org/1706293005/diff/40001/chrome/browser/android/...
File chrome/browser/android/compositor/scene_layer/static_tab_scene_layer.cc
(right):

https://codereview.chromium.org/1706293005/diff/40001/chrome/browser/android/...
chrome/browser/android/compositor/scene_layer/static_tab_scene_layer.cc:148:
layer_->AddChild(layer);
On 2016/02/24 17:06:10, David Trainor wrote:
> Can't we just insert this layer at the beginning so we don't have to readd the
> toolbar layer to move it to the end?

Removed this whole function as it actually doesn't make sense in the new
architecture.

The layer is added in the correct place when it is created.

[David Trainor- moved to gerrit, 2016-04-15 15:59:43]

lgtm!

[mdjones, 2016-05-13 22:18:17]

The CQ bit was checked by mdjones@chromium.org

[commit-bot: I haz the power, 2016-05-13 22:18:50]

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

[mdjones, 2016-05-13 23:09:55]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2016-05-13 23:09:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/1706293005/#ps160001
(title: "fix findbugs issue")

[commit-bot: I haz the power, 2016-05-13 23:10:36]

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

[commit-bot: I haz the power, 2016-05-14 00:09:32]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-05-14 00:11:04]

Description was changed from

==========
StaticTabSceneLayer owns ToolbarLayer

Since StaticLayout is the only layout that needs the toolbar layout,
its ownership has been moved to StaticLayout's SceneLayer. This puts
the ToolbarLayer in the content tree for SceneOverlays.

BUG=584340
==========

to

==========
StaticTabSceneLayer owns ToolbarLayer

Since StaticLayout is the only layout that needs the toolbar layout,
its ownership has been moved to StaticLayout's SceneLayer. This puts
the ToolbarLayer in the content tree for SceneOverlays.

BUG=584340

Committed: https://crrev.com/94e6d2cbf7bf2691014401bc11be703dc45f43b0
Cr-Commit-Position: refs/heads/master@{#393696}
==========

[commit-bot: I haz the power, 2016-05-14 00:11:05]

Patchset 9 (id:??) landed as
https://crrev.com/94e6d2cbf7bf2691014401bc11be703dc45f43b0
Cr-Commit-Position: refs/heads/master@{#393696}