[Contextual Search] Remove layers when no longer needed.

[Contextual Search] Remove layers when no longer needed.

Now that there can be multiple Overlays Panels being used,
a few bugs are happening due to the Panel's cc::Layer not
being properly removed.

This CL fixes the problem by removing the layer when a
OverlayPanel shows up when another one is being displayed.

Also, this CL removed the OverlayPanel layer from the
StaticLayout whenever that layout gets hidden (see
dettachViews method).

BUG=539847
Committed: https://crrev.com/a53e46eff945b879f6e72108073d3d70d47d6b0e
Cr-Commit-Position: refs/heads/master@{#363092}

[pedro (no code reviews), 2015-12-02 01:08:24]

Description was changed from

==========
[Contextual Search] Remove layers when no longer needed.

Now that there can be multiple Overlays Panels being used,
a few bugs are happening due to the Panel's cc::Layer not
being properly removed.

This CL fixes the problem by removing the layer when a
OverlayPanel shows up when another one is being displayed.

Also, this CL removed the OverlayPanel layer from the
StaticLayout whenever that layout gets hidden (see
dettachViews method).

BUG=539847
==========

to

==========
[Contextual Search] Remove layers when no longer needed.

Now that there can be multiple Overlays Panels being used,
a few bugs are happening due to the Panel's cc::Layer not
being properly removed.

This CL fixes the problem by removing the layer when a
OverlayPanel shows up when another one is being displayed.

Also, this CL removed the OverlayPanel layer from the
StaticLayout whenever that layout gets hidden (see
dettachViews method).

BUG=539847
==========

[pedro (no code reviews), 2015-12-02 01:08:25]

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

[pedro (no code reviews), 2015-12-02 01:10:12]

Guys, please take a look at this change. This fixes a M48 Release Blocker.

[David Trainor- moved to gerrit, 2015-12-02 16:34:13]

https://chromiumcodereview.appspot.com/1489213002/diff/1/chrome/android/java/...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerDocument.java
(right):

https://chromiumcodereview.appspot.com/1489213002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerDocument.java:81:
protected final OverlayPanelManager mOverlayPanelManager;
Eh might as well add a javadoc now.

https://chromiumcodereview.appspot.com/1489213002/diff/1/chrome/android/java/...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/StaticLayout.java
(right):

https://chromiumcodereview.appspot.com/1489213002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/StaticLayout.java:258:
mSceneLayer.setContentSceneLayer(null);
Should we do this during hide or is this the only good hook for it?  IIRC this
was meant to attach/detach Android views to the view hierarchy (although
overriding it doesn't seem too dangerous for this particular scenario).

https://chromiumcodereview.appspot.com/1489213002/diff/1/chrome/browser/andro...
File chrome/browser/android/compositor/scene_layer/static_tab_scene_layer.cc
(right):

https://chromiumcodereview.appspot.com/1489213002/diff/1/chrome/browser/andro...
chrome/browser/android/compositor/scene_layer/static_tab_scene_layer.cc:130: //
If |content_scene_layer| is null, or different from the previously added
Would this be easier if we just pulled out the actually new ContentSceneLayer
here and used that?

scoped_refptr<Layer> layer = content_scene_layer ? content_scene_layer->layer()
: nullptr;

if (content_scene_layer_ && content_scene_layer_ != layer) {
  // Remove meh.
}

if (layer) {
  content_scene_layer_ = layer;
  // Add meh.
}

Also, do we want to do this each time we call SetContentSceneLayer?  What if
it's already set?  Even if the layers are equal it looks like we still call
AddChild below, which will remove it and readd it IIUC.

[Donn Denman, 2015-12-02 17:53:49]

LGTM with my limited knowledge of scene layers.

https://codereview.chromium.org/1489213002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerChrome.java
(right):

https://codereview.chromium.org/1489213002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerChrome.java:370:
// Check if a layout is showing that should hide the contextual search bar.
Update this comment to match code generalization.

[pedro (no code reviews), 2015-12-02 23:56:15]

Thanks guys. Please take another look.

https://codereview.chromium.org/1489213002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerChrome.java
(right):

https://codereview.chromium.org/1489213002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerChrome.java:370:
// Check if a layout is showing that should hide the contextual search bar.
On 2015/12/02 17:53:49, Donn Denman wrote:
> Update this comment to match code generalization. 

Thanks for noticing this.

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

https://codereview.chromium.org/1489213002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerDocument.java:81:
protected final OverlayPanelManager mOverlayPanelManager;
On 2015/12/02 16:34:13, David Trainor wrote:
> Eh might as well add a javadoc now.

Done.

https://codereview.chromium.org/1489213002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/StaticLayout.java
(right):

https://codereview.chromium.org/1489213002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/StaticLayout.java:258:
mSceneLayer.setContentSceneLayer(null);
On 2015/12/02 16:34:13, David Trainor wrote:
> Should we do this during hide or is this the only good hook for it?  IIRC this
> was meant to attach/detach Android views to the view hierarchy (although
> overriding it doesn't seem too dangerous for this particular scenario).

As we discussed offline, we need to remove the content scene layer every time
the Static Layout is hidden, but I couldn't find a better hook for it, given
that startHiding()/doneHiding() is not called when transitioning .

I'm including a TODO to revisit this later. For now, I think it's safer to keep
this code in here, since changing the system to dispatch
startHiding()/doneHiding() might have unexpected side effects.

https://codereview.chromium.org/1489213002/diff/1/chrome/browser/android/comp...
File chrome/browser/android/compositor/scene_layer/static_tab_scene_layer.cc
(right):

https://codereview.chromium.org/1489213002/diff/1/chrome/browser/android/comp...
chrome/browser/android/compositor/scene_layer/static_tab_scene_layer.cc:130: //
If |content_scene_layer| is null, or different from the previously added
On 2015/12/02 16:34:13, David Trainor wrote:
> Would this be easier if we just pulled out the actually new ContentSceneLayer
> here and used that?
> 
> scoped_refptr<Layer> layer = content_scene_layer ?
content_scene_layer->layer()
> : nullptr;
> 
> if (content_scene_layer_ && content_scene_layer_ != layer) {
>   // Remove meh.
> }
> 
> if (layer) {
>   content_scene_layer_ = layer;
>   // Add meh.
> }
> 
> Also, do we want to do this each time we call SetContentSceneLayer?  What if
> it's already set?  Even if the layers are equal it looks like we still call
> AddChild below, which will remove it and readd it IIUC.

As we've discussed offline, the problem is that the content layer is being
re-added to hierarchy on every frame, so if we don't do the same, the content
scene layer will be occluded by the content layer.

For now, since we need to merge this in M48, I'll keep re-adding on every frame.
I put a TODO to revisit this later. We could force the content layer to be added
always at the first position (and only add it once), and then we could add the
content scene layer only once as well, and it will be guaranteed that it will be
placed after the content layer. But I'd rather do that later so we have more
time to properly test the new approach.

[mdjones, 2015-12-03 00:13:38]

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

[mdjones, 2015-12-03 00:13:38]

cc::Layer has a "RemoveFromParent" method. Would it be possible to have a native
method on the ContextualSearchLayer that could be called to remove itself using
this? It looks like we have access to the root layer in that class. This way it
might be possible to avoid modifying static*.

[pedro (no code reviews), 2015-12-03 00:27:27]

On 2015/12/03 00:13:38, mdjones wrote:
> cc::Layer has a "RemoveFromParent" method. Would it be possible to have a
native
> method on the ContextualSearchLayer that could be called to remove itself
using
> this? It looks like we have access to the root layer in that class. This way
it
> might be possible to avoid modifying static*.

The problem is that we need to do that only when the StaticLayout
is about to be hidden, and there's no way to know that from
the ContextualSearchLayer class.

[mdjones, 2015-12-03 00:39:56]

On 2015/12/03 00:27:27, pedrosimonetti wrote:
> On 2015/12/03 00:13:38, mdjones wrote:
> > cc::Layer has a "RemoveFromParent" method. Would it be possible to have a
> native
> > method on the ContextualSearchLayer that could be called to remove itself
> using
> > this? It looks like we have access to the root layer in that class. This way
> it
> > might be possible to avoid modifying static*.
> 
> The problem is that we need to do that only when the StaticLayout
> is about to be hidden, and there's no way to know that from
> the ContextualSearchLayer class.

What would stop us from doing this whenever "closePanel" is called? The panel is
closed any time you switch tabs right?

[pedro (no code reviews), 2015-12-03 01:34:37]

On 2015/12/03 00:39:56, mdjones wrote:
> On 2015/12/03 00:27:27, pedrosimonetti wrote:
> > On 2015/12/03 00:13:38, mdjones wrote:
> > > cc::Layer has a "RemoveFromParent" method. Would it be possible to have a
> > native
> > > method on the ContextualSearchLayer that could be called to remove itself
> > using
> > > this? It looks like we have access to the root layer in that class. This
way
> > it
> > > might be possible to avoid modifying static*.
> > 
> > The problem is that we need to do that only when the StaticLayout
> > is about to be hidden, and there's no way to know that from
> > the ContextualSearchLayer class.
> 
> What would stop us from doing this whenever "closePanel" is called? The panel
is
> closed any time you switch tabs right?

I'm not sure this would cover all the cases. When the bar is peeking, and then
you swipe it up, we're transitioning from StaticLayout to
ContextualSearchLayout.
We want to remove the layer from the (StaticLayout) tree in this case as well,
which does not involve closing the Panel.

[mdjones, 2015-12-03 01:54:37]

On 2015/12/03 01:34:37, pedrosimonetti wrote:
> On 2015/12/03 00:39:56, mdjones wrote:
> > On 2015/12/03 00:27:27, pedrosimonetti wrote:
> > > On 2015/12/03 00:13:38, mdjones wrote:
> > > > cc::Layer has a "RemoveFromParent" method. Would it be possible to have
a
> > > native
> > > > method on the ContextualSearchLayer that could be called to remove
itself
> > > using
> > > > this? It looks like we have access to the root layer in that class. This
> way
> > > it
> > > > might be possible to avoid modifying static*.
> > > 
> > > The problem is that we need to do that only when the StaticLayout
> > > is about to be hidden, and there's no way to know that from
> > > the ContextualSearchLayer class.
> > 
> > What would stop us from doing this whenever "closePanel" is called? The
panel
> is
> > closed any time you switch tabs right?
> 
> I'm not sure this would cover all the cases. When the bar is peeking, and then
> you swipe it up, we're transitioning from StaticLayout to
> ContextualSearchLayout.
> We want to remove the layer from the (StaticLayout) tree in this case as well,
> which does not involve closing the Panel.

Since we use the same layer for both static and contextual search layouts, the
parent is updated when it is moved from one layout to the other (automatically).
In either case, the parent of ContextualSearchLayer will always belong to the
correct layout (if it is being shown at all). So when RemoveFromParent is
finally called the parent should either be valid or null.

If the concern is that "closePanel" is not being called when tabs are switched,
I would consider that a separate issue.

[David Trainor- moved to gerrit, 2015-12-03 03:19:59]

lgtm with nit.

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

https://codereview.chromium.org/1489213002/diff/20001/chrome/browser/android/...
chrome/browser/android/compositor/scene_layer/static_tab_scene_layer.cc:144: if
(layer && layer.get()) {
You can just check "if (layer)" I think.

[pedro (no code reviews), 2015-12-03 19:53:19]

Thanks everyone!

I did some more testing and realized some of the changes I made were not
necessary, including overriding detachViews().

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

https://codereview.chromium.org/1489213002/diff/20001/chrome/browser/android/...
chrome/browser/android/compositor/scene_layer/static_tab_scene_layer.cc:144: if
(layer && layer.get()) {
On 2015/12/03 03:19:59, David Trainor wrote:
> You can just check "if (layer)" I think.

Done.

[pedro (no code reviews), 2015-12-03 19:53:24]

The CQ bit was checked by pedrosimonetti@chromium.org

[pedro (no code reviews), 2015-12-03 19:53:24]

The patchset sent to the CQ was uploaded after l-g-t-m from donnd@chromium.org,
dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/1489213002/#ps40001
(title: "cleanup")

[commit-bot: I haz the power, 2015-12-03 20:03:54]

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

[commit-bot: I haz the power, 2015-12-03 22:07:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-03 22:07:38]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[pedro (no code reviews), 2015-12-03 22:38:59]

The CQ bit was checked by pedrosimonetti@chromium.org

[pedro (no code reviews), 2015-12-03 22:38:59]

The patchset sent to the CQ was uploaded after l-g-t-m from donnd@chromium.org,
dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/1489213002/#ps60001
(title: "Fixing broken tests")

[commit-bot: I haz the power, 2015-12-03 22:39:20]

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

[commit-bot: I haz the power, 2015-12-04 00:44:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-04 00:44:01]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  android_compile_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  cast_shell_android on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  cast_shell_linux on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_android_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)

[pedro (no code reviews), 2015-12-04 00:46:56]

The CQ bit was checked by pedrosimonetti@chromium.org

[commit-bot: I haz the power, 2015-12-04 00:48:52]

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

[commit-bot: I haz the power, 2015-12-04 02:47:23]

Description was changed from

==========
[Contextual Search] Remove layers when no longer needed.

Now that there can be multiple Overlays Panels being used,
a few bugs are happening due to the Panel's cc::Layer not
being properly removed.

This CL fixes the problem by removing the layer when a
OverlayPanel shows up when another one is being displayed.

Also, this CL removed the OverlayPanel layer from the
StaticLayout whenever that layout gets hidden (see
dettachViews method).

BUG=539847
==========

to

==========
[Contextual Search] Remove layers when no longer needed.

Now that there can be multiple Overlays Panels being used,
a few bugs are happening due to the Panel's cc::Layer not
being properly removed.

This CL fixes the problem by removing the layer when a
OverlayPanel shows up when another one is being displayed.

Also, this CL removed the OverlayPanel layer from the
StaticLayout whenever that layout gets hidden (see
dettachViews method).

BUG=539847
==========

[commit-bot: I haz the power, 2015-12-04 02:47:24]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-12-04 02:48:31]

Description was changed from

==========
[Contextual Search] Remove layers when no longer needed.

Now that there can be multiple Overlays Panels being used,
a few bugs are happening due to the Panel's cc::Layer not
being properly removed.

This CL fixes the problem by removing the layer when a
OverlayPanel shows up when another one is being displayed.

Also, this CL removed the OverlayPanel layer from the
StaticLayout whenever that layout gets hidden (see
dettachViews method).

BUG=539847
==========

to

==========
[Contextual Search] Remove layers when no longer needed.

Now that there can be multiple Overlays Panels being used,
a few bugs are happening due to the Panel's cc::Layer not
being properly removed.

This CL fixes the problem by removing the layer when a
OverlayPanel shows up when another one is being displayed.

Also, this CL removed the OverlayPanel layer from the
StaticLayout whenever that layout gets hidden (see
dettachViews method).

BUG=539847

Committed: https://crrev.com/a53e46eff945b879f6e72108073d3d70d47d6b0e
Cr-Commit-Position: refs/heads/master@{#363092}
==========

[commit-bot: I haz the power, 2015-12-04 02:48:32]

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