Refactor contextual search's panel host and scene layer

Refactor contextual search's panel host and scene layer

This change moves ownership of an OverlayPanel's scene layer to
the panel itself. The concept of UpdatableSceneLayer has been
introduced as a child class of SceneLayer.

BUG=521773
Committed: https://crrev.com/a8c387bd6d5f094afc7166103f9e0869474b9b2d
Cr-Commit-Position: refs/heads/master@{#357126}

[mdjones, 2015-10-01 00:39:26]

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

[mdjones, 2015-10-01 00:39:27]

This is more of an intermediate change for having a layout manage multiple
panels. PTAL

[mdjones, 2015-10-02 15:36:58]

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

[pedro (no code reviews), 2015-10-15 22:25:17]

https://codereview.chromium.org/1374603004/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java
(right):

https://codereview.chromium.org/1374603004/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:136:
public UpdatableSceneLayer getUpdatableSceneLayer() {
getSceneLayer

https://codereview.chromium.org/1374603004/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:140:
/**
add updateSceneLayer

https://codereview.chromium.org/1374603004/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:143:
public void setUpdatableSceneLayer(UpdatableSceneLayer sceneLayer) {
There should not be such method exposed.

https://codereview.chromium.org/1374603004/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:463:
public void updateTopControlsState(int current, boolean animate) {
Is this coming from the manager? If so, shouldn't we remove the code from there?

https://codereview.chromium.org/1374603004/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/ContextualSearchSupportedLayout.java
(left):

https://codereview.chromium.org/1374603004/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/ContextualSearchSupportedLayout.java:29:
public abstract class ContextualSearchSupportedLayout extends Layout {
Put a TODO here to rename this class

https://codereview.chromium.org/1374603004/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/ContextualSearchSupportedLayout.java
(right):

https://codereview.chromium.org/1374603004/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/ContextualSearchSupportedLayout.java:150:
mSearchPanel.getUpdatableSceneLayer().update(contentViewCore, resourceManager);
The Panel now owns the Content so it feels weird getting the Content from it,
then passing to the SceneLayer that the Panel itself owns.

I think we should instead call mSearchPanel.updateSceneLayer(resourceManager),
and let the Panel combine their components to do the job.

https://codereview.chromium.org/1374603004/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/UpdatableSceneLayer.java
(right):

https://codereview.chromium.org/1374603004/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/UpdatableSceneLayer.java:14:
*/
I think we don't need this anymore

[mdjones, 2015-10-19 18:38:09]

https://codereview.chromium.org/1374603004/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java
(right):

https://codereview.chromium.org/1374603004/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:136:
public UpdatableSceneLayer getUpdatableSceneLayer() {
On 2015/10/15 22:25:17, pedrosimonetti wrote:
> getSceneLayer

Done.

https://codereview.chromium.org/1374603004/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:140:
/**
On 2015/10/15 22:25:17, pedrosimonetti wrote:
> add updateSceneLayer

Done.

https://codereview.chromium.org/1374603004/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:143:
public void setUpdatableSceneLayer(UpdatableSceneLayer sceneLayer) {
On 2015/10/15 22:25:17, pedrosimonetti wrote:
> There should not be such method exposed.

Removed

https://codereview.chromium.org/1374603004/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:463:
public void updateTopControlsState(int current, boolean animate) {
On 2015/10/15 22:25:17, pedrosimonetti wrote:
> Is this coming from the manager? If so, shouldn't we remove the code from
there?

Done.

https://codereview.chromium.org/1374603004/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/ContextualSearchSupportedLayout.java
(left):

https://codereview.chromium.org/1374603004/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/ContextualSearchSupportedLayout.java:29:
public abstract class ContextualSearchSupportedLayout extends Layout {
On 2015/10/15 22:25:17, pedrosimonetti wrote:
> Put a TODO here to rename this class

Done.

https://codereview.chromium.org/1374603004/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/ContextualSearchSupportedLayout.java
(right):

https://codereview.chromium.org/1374603004/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/ContextualSearchSupportedLayout.java:150:
mSearchPanel.getUpdatableSceneLayer().update(contentViewCore, resourceManager);
On 2015/10/15 22:25:17, pedrosimonetti wrote:
> The Panel now owns the Content so it feels weird getting the Content from it,
> then passing to the SceneLayer that the Panel itself owns.
> 
> I think we should instead call mSearchPanel.updateSceneLayer(resourceManager),
> and let the Panel combine their components to do the job.

Done.

https://codereview.chromium.org/1374603004/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/UpdatableSceneLayer.java
(right):

https://codereview.chromium.org/1374603004/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/UpdatableSceneLayer.java:14:
*/
On 2015/10/15 22:25:17, pedrosimonetti wrote:
> I think we don't need this anymore

Done.

[mdjones, 2015-10-21 16:11:52]

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

[mdjones, 2015-10-21 16:11:52]

+dtrainor PTAL

[David Trainor- moved to gerrit, 2015-10-22 01:20:52]

lgtm but wait for pedro!

https://chromiumcodereview.appspot.com/1374603004/diff/120001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/ContextualSearchSupportedLayout.java
(right):

https://chromiumcodereview.appspot.com/1374603004/diff/120001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/ContextualSearchSupportedLayout.java:145:
if (mSearchPanel.getSceneLayer() == null) {
Does updateSceneLayer check internally?

[mdjones, 2015-10-22 15:33:29]

https://chromiumcodereview.appspot.com/1374603004/diff/120001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/ContextualSearchSupportedLayout.java
(right):

https://chromiumcodereview.appspot.com/1374603004/diff/120001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/ContextualSearchSupportedLayout.java:145:
if (mSearchPanel.getSceneLayer() == null) {
On 2015/10/22 01:20:52, David Trainor wrote:
> Does updateSceneLayer check internally?

Indeed it does.

[pedro (no code reviews), 2015-10-28 19:14:07]

lgtm

Change is looking good to me, but let's coordinate the submission of this change
with Theresa.

https://codereview.chromium.org/1374603004/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java
(right):

https://codereview.chromium.org/1374603004/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:83:
private OverlayPanelHost mOverlayPanelHost;
Nit: Move this below the OverlayPanelContent declaration below, so that
OverlayPanelContent is declared right after OverlayPanelContentFactory.

https://codereview.chromium.org/1374603004/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:276:
super.onAnimationFinished();
Nit: Add blank line after super call.

https://codereview.chromium.org/1374603004/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java
(right):

https://codereview.chromium.org/1374603004/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:327:
private boolean shouldHideContextualSearchLayout() {
Shouldn't you remove this method from this class since you moved to the
OverlayPanel?

[mdjones, 2015-10-29 00:45:36]

https://codereview.chromium.org/1374603004/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java
(right):

https://codereview.chromium.org/1374603004/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:83:
private OverlayPanelHost mOverlayPanelHost;
On 2015/10/28 19:14:07, pedrosimonetti wrote:
> Nit: Move this below the OverlayPanelContent declaration below, so that
> OverlayPanelContent is declared right after OverlayPanelContentFactory.

Done.

https://codereview.chromium.org/1374603004/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:276:
super.onAnimationFinished();
On 2015/10/28 19:14:07, pedrosimonetti wrote:
> Nit: Add blank line after super call.

Done.

https://codereview.chromium.org/1374603004/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java
(right):

https://codereview.chromium.org/1374603004/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:327:
private boolean shouldHideContextualSearchLayout() {
On 2015/10/28 19:14:07, pedrosimonetti wrote:
> Shouldn't you remove this method from this class since you moved to the
> OverlayPanel?

Done.

[mdjones, 2015-10-30 16:09:22]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2015-10-30 16:09:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, pedrosimonetti@chromium.org
Link to the patchset: https://codereview.chromium.org/1374603004/#ps180001
(title: "rebase")

[commit-bot: I haz the power, 2015-10-30 16:09:39]

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

[commit-bot: I haz the power, 2015-10-30 16:56:20]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-10-30 16:57:07]

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