Toolbar is a root layer for the UI compositor

Toolbar is a root layer for the UI compositor

This change converts the toolbar layer into its own SceneOverlay.
The SceneOverlay is created and owned by the layout manager and
is added to all layouts before any of the other SceneOverlays.
Doing this places the layer in the content tree.

BUG=612085, 614268
Committed: https://crrev.com/0c9e11ddad202a35e5b99b2871fe8cc1e5acbc40
Cr-Commit-Position: refs/heads/master@{#396209}

[mdjones, 2016-05-20 17:07:38]

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

[mdjones, 2016-05-20 17:07:39]

PTAL

[Bernhard Bauer, 2016-05-23 08:17:52]

Description was changed from

==========
Toolbar is a root layer for the UI compositor

This change converts the toolbar layer into its own SceneLayer. This
SceneLayer is created by the CompositorView and is passed to the
various layouts of Chrome as a root layer. This allows all of the
layouts to have access to the toolbar layer while also allowing it
to be in the tree during modification (i.e. SceneOverlays).

BUG=612085
==========

to

==========
Toolbar is a root layer for the UI compositor

This change converts the toolbar layer into its own SceneLayer. This
SceneLayer is created by the CompositorView and is passed to the
various layouts of Chrome as a root layer. This allows all of the
layouts to have access to the toolbar layer while also allowing it
to be in the tree during modification (i.e. SceneOverlays).

BUG=612085
==========

[Theresa, 2016-05-24 01:05:16]

lgtm

https://codereview.chromium.org/1988023010/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java
(right):

https://codereview.chromium.org/1988023010/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java:1149:
* @param contentViewport   A viewport in which to display content.
nit: this is missing the @param viewport. Do you mind adding it here and in the
other places?

https://codereview.chromium.org/1988023010/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ToolbarSceneLayer.java
(right):

https://codereview.chromium.org/1988023010/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ToolbarSceneLayer.java:38:
mIsTablet = DeviceFormFactor.isTablet(context);
nit: let's not cache this here. DeviceFormFactor already caches it, and
recalculates it when needed.

[mdjones, 2016-05-24 02:30:16]

https://codereview.chromium.org/1988023010/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java
(right):

https://codereview.chromium.org/1988023010/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java:1149:
* @param contentViewport   A viewport in which to display content.
On 2016/05/24 01:05:16, Theresa Wellington wrote:
> nit: this is missing the @param viewport. Do you mind adding it here and in
the
> other places?

Done.

https://codereview.chromium.org/1988023010/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ToolbarSceneLayer.java
(right):

https://codereview.chromium.org/1988023010/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ToolbarSceneLayer.java:38:
mIsTablet = DeviceFormFactor.isTablet(context);
On 2016/05/24 01:05:16, Theresa Wellington wrote:
> nit: let's not cache this here. DeviceFormFactor already caches it, and
> recalculates it when needed.

Done.

[mdjones, 2016-05-24 02:31:08]

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

[mdjones, 2016-05-24 02:31:08]

+changwan for compositor, ptal

[Changwan Ryu, 2016-05-24 08:10:22]

Thanks for this refactoring. This was a long-standing technical debt I wanted to
pay off one day. This was not possible a long time ago due to its tie with the
top controls, but I guess it's not the case any more. :)

https://codereview.chromium.org/1988023010/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java
(right):

https://codereview.chromium.org/1988023010/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java:1173:
content = root;
Instead of introducing the new concept 'root', is it possible to just make sure
that mSceneOverlays.get(0) is ToolbarSceneLayer?

I guess LayoutManagerDocument#addGlobalSceneOverlay() could be one such
candidate.

[mdjones, 2016-05-24 15:47:48]

https://codereview.chromium.org/1988023010/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java
(right):

https://codereview.chromium.org/1988023010/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java:1173:
content = root;
On 2016/05/24 08:10:22, Changwan Ryu wrote:
> Instead of introducing the new concept 'root', is it possible to just make
sure
> that mSceneOverlays.get(0) is ToolbarSceneLayer?
> 
> I guess LayoutManagerDocument#addGlobalSceneOverlay() could be one such
> candidate.

I agree! This was actually the original idea. The problem was that the
SceneOverlay interface is huge and it would only be used for the method
"getUpdatedSceneOverlayTree" so we decided against it. We thought it might makes
sense to wait until we split up the interface. However, if you think it's a good
idea now, I'd be happy to implement it.

[mdjones, 2016-05-24 17:34:54]

Decided to try SceneOverlay impl, ptal.

[mdjones, 2016-05-24 18:29:55]

Description was changed from

==========
Toolbar is a root layer for the UI compositor

This change converts the toolbar layer into its own SceneLayer. This
SceneLayer is created by the CompositorView and is passed to the
various layouts of Chrome as a root layer. This allows all of the
layouts to have access to the toolbar layer while also allowing it
to be in the tree during modification (i.e. SceneOverlays).

BUG=612085
==========

to

==========
Toolbar is a root layer for the UI compositor

This change converts the toolbar layer into its own SceneOverlay.
The SceneOverlay is created and owned by the layout manager and 
is added to all layouts before any of the other SceneOverlays.
Doing this places the layer in the content tree.

BUG=612085, 614268
==========

[Changwan Ryu, 2016-05-25 06:49:35]

https://codereview.chromium.org/1988023010/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerDocument.java
(right):

https://codereview.chromium.org/1988023010/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerDocument.java:219:
protected void addGlobalSceneOverlay(SceneOverlay helper) {
could you add a note to make sure that overriding function should call
super.addGlobalSceneOverlay(helper) before adding their own?

https://codereview.chromium.org/1988023010/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerDocument.java:228:
addGlobalSceneOverlay(mToolbarOverlay);
Hmm... The only concern I have is that in
LayoutManagerChromeTablet#addAllSceneOverlays() we will add
mTabStripLayoutHelperManager before mToolbarOverlay.

In most cases it should be fine as tabstrip area and toolbar area does not
overlap much. But please test a bit to make sure that they are in the correct
order in all cases.

https://codereview.chromium.org/1988023010/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ToolbarSceneLayer.java
(right):

https://codereview.chromium.org/1988023010/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ToolbarSceneLayer.java:146:
update(mRenderHost.getTopControlsBackgroundColor(),
mRenderHost.getTopControlsUrlBarAlpha(),
Hmm... Unfortunately, there are two entry points for updating visual assets. I'm
ok with the current implementation, but is there any difference between updating
it here vs updating it in updateOverlay()?

https://codereview.chromium.org/1988023010/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ToolbarSceneLayer.java:194:
public void tabTitleChanged(int tabId, String title) {}
At least for tab related functions, SceneOverlay's subclasses should implement
them only when they elect to do so. How about giving them default empty
implementations?

[mdjones, 2016-05-25 15:52:13]

https://codereview.chromium.org/1988023010/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerDocument.java
(right):

https://codereview.chromium.org/1988023010/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerDocument.java:219:
protected void addGlobalSceneOverlay(SceneOverlay helper) {
On 2016/05/25 06:49:35, Changwan Ryu wrote:
> could you add a note to make sure that overriding function should call
> super.addGlobalSceneOverlay(helper) before adding their own?

The order of addition shouldn't be important here. This method's goal is to call
addSceneOverlay on any layout this class is responsible for and won't actually
affect the order of SceneOverlay addition (since each layout has its own list).
However, order is very important in addAllSceneOverlays, I added a comment
there.

https://codereview.chromium.org/1988023010/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerDocument.java:228:
addGlobalSceneOverlay(mToolbarOverlay);
On 2016/05/25 06:49:35, Changwan Ryu wrote:
> Hmm... The only concern I have is that in
> LayoutManagerChromeTablet#addAllSceneOverlays() we will add
> mTabStripLayoutHelperManager before mToolbarOverlay.
> 
> In most cases it should be fine as tabstrip area and toolbar area does not
> overlap much. But please test a bit to make sure that they are in the correct
> order in all cases.

I was careful to have the tab strip be the top-most layer when this was
originally added; LayoutManagerChromeTablet calls super only after adding the
tab strip making it currently always on top. I did add a comment mentioning the
importance of order and calling super.

https://codereview.chromium.org/1988023010/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ToolbarSceneLayer.java
(right):

https://codereview.chromium.org/1988023010/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ToolbarSceneLayer.java:146:
update(mRenderHost.getTopControlsBackgroundColor(),
mRenderHost.getTopControlsUrlBarAlpha(),
On 2016/05/25 06:49:35, Changwan Ryu wrote:
> Hmm... Unfortunately, there are two entry points for updating visual assets.
I'm
> ok with the current implementation, but is there any difference between
updating
> it here vs updating it in updateOverlay()?

I think the original idea was to have this method update any resources and the
other drive animations since it provides current and delta time. It probably
makes sense to merge these methods into one; I can address that as a follow-up.

https://codereview.chromium.org/1988023010/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ToolbarSceneLayer.java:194:
public void tabTitleChanged(int tabId, String title) {}
On 2016/05/25 06:49:35, Changwan Ryu wrote:
> At least for tab related functions, SceneOverlay's subclasses should implement
> them only when they elect to do so. How about giving them default empty
> implementations?
I would have to make a base class that implements SceneOverlay and extends
OverlayPanelLayer unfortunately (this class already extends SceneOverlayLayer
and can't have another). Otherwise I agree having an EmptySceneOverlay would be
great.

[Changwan Ryu, 2016-05-26 01:43:00]

compositor/ (both Java and C++) lgtm

[mdjones, 2016-05-26 15:52:20]

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

[mdjones, 2016-05-26 15:52:21]

+dfalcantara jni registrar please

[gone, 2016-05-26 15:58:27]

jni_registar.cc lgtm

[mdjones, 2016-05-26 16:05:06]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2016-05-26 16:05:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
twellington@chromium.org
Link to the patchset: https://codereview.chromium.org/1988023010/#ps80001
(title: "add comments")

[commit-bot: I haz the power, 2016-05-26 16:05:32]

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

[commit-bot: I haz the power, 2016-05-26 17:16:37]

Description was changed from

==========
Toolbar is a root layer for the UI compositor

This change converts the toolbar layer into its own SceneOverlay.
The SceneOverlay is created and owned by the layout manager and 
is added to all layouts before any of the other SceneOverlays.
Doing this places the layer in the content tree.

BUG=612085, 614268
==========

to

==========
Toolbar is a root layer for the UI compositor

This change converts the toolbar layer into its own SceneOverlay.
The SceneOverlay is created and owned by the layout manager and 
is added to all layouts before any of the other SceneOverlays.
Doing this places the layer in the content tree.

BUG=612085, 614268
==========

[commit-bot: I haz the power, 2016-05-26 17:16:38]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-26 17:18:14]

Description was changed from

==========
Toolbar is a root layer for the UI compositor

This change converts the toolbar layer into its own SceneOverlay.
The SceneOverlay is created and owned by the layout manager and 
is added to all layouts before any of the other SceneOverlays.
Doing this places the layer in the content tree.

BUG=612085, 614268
==========

to

==========
Toolbar is a root layer for the UI compositor

This change converts the toolbar layer into its own SceneOverlay.
The SceneOverlay is created and owned by the layout manager and
is added to all layouts before any of the other SceneOverlays.
Doing this places the layer in the content tree.

BUG=612085, 614268

Committed: https://crrev.com/0c9e11ddad202a35e5b99b2871fe8cc1e5acbc40
Cr-Commit-Position: refs/heads/master@{#396209}
==========

[commit-bot: I haz the power, 2016-05-26 17:18:15]

Patchset 5 (id:??) landed as
https://crrev.com/0c9e11ddad202a35e5b99b2871fe8cc1e5acbc40
Cr-Commit-Position: refs/heads/master@{#396209}