Implement OverlayPanels as SceneOverlays

Implement OverlayPanels as SceneOverlays

Use the SceneOverlay infrastructure to draw on top of Chrome send events
to OverlayPanels rather than using layout switching.

BUG=584340
Committed: https://crrev.com/5a5b9fbe34f23087353c763ec5420deb075194d9
Cr-Commit-Position: refs/heads/master@{#393851}

[pedro (no code reviews), 2016-02-19 01:38:10]

Description was changed from

==========
Implement OverlayPanels as SceneOverlays

BUG=
==========

to

==========
Implement OverlayPanels as SceneOverlays

BUG=
==========

[mdjones, 2016-03-13 19:51:40]

Description was changed from

==========
Implement OverlayPanels as SceneOverlays

BUG=
==========

to

==========
Implement OverlayPanels as SceneOverlays

Use the SceneOverlay infrastructure to draw on top of Chrome send events
to OverlayPanels rather than using layout switching. 

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

[mdjones, 2016-04-20 22:21:35]

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

[mdjones, 2016-04-20 22:21:36]

ptal

[Theresa, 2016-04-21 21:22:39]

Happy to see this change! I'll go through and do a more through review later
tonight.

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

https://codereview.chromium.org/1716653002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:287:
* Set focus on the panel's content and reomve it from the base page.
s/reomve/remove

I think this could also be expanded, e.g.
"If the panel is open and doesn't have focus, set focus on the panel's content
and remove it from the base page. If the panel is not open and has focus, set
focus on the base page."

https://codereview.chromium.org/1716653002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:300:
if (!mHasFocus && isPanelOpened() && panelContentView != null) {
I think this would read a little better if the this block was combined w/ the if
above. maybe:

if (!mHasFocus && isPanelOpened()) {
    // Remove focus from the base page.
    baseContentView.preserveSelectionOnNextLossOfFocus();
    baseContentView.onDetachedFromWindow();

    // Set focus on the overlay content.
    if (panelContentView != null) {
        mHasFocus = true;
        panelContentView.getContainerView().requestFocus();
        panelContentView.onAttachedToWindow();
    }
} else if (mHasFocus && !isPanelOpened() && baseContentView != null) {
   // Give focus to the base page.
   ....
}

https://codereview.chromium.org/1716653002/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/1716653002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:146:
}
Not knowing the history of this code particularly well, can mSceneLayer no
longer null when this is called (where as it could in the old
updateSceneLayer())?

https://codereview.chromium.org/1716653002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:158:

Left over method? It doesn't look like this is used. If it is necessary, is it
actually focusing the base page?

https://codereview.chromium.org/1716653002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerDocument.java
(left):

https://codereview.chromium.org/1716653002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerDocument.java:60:
implements ContextualSearchTapHandler, OverlayPanelContentViewDelegate {
Happy to see some of the CS specific stuff disappearing from this class!

https://codereview.chromium.org/1716653002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/StaticLayout.java
(left):

https://codereview.chromium.org/1716653002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/StaticLayout.java:34:
public class StaticLayout extends ContextualSearchSupportedLayout {
Yes!!

https://codereview.chromium.org/1716653002/diff/140001/chrome/browser/android...
File chrome/browser/android/compositor/scene_layer/reader_mode_scene_layer.cc
(right):

https://codereview.chromium.org/1716653002/diff/140001/chrome/browser/android...
chrome/browser/android/compositor/scene_layer/reader_mode_scene_layer.cc:146:
content_container_->SetPosition(gfx::PointF(0.0f, 0.0f));
Does it make sense for this code (and some of the other duplicated code) to be
shared in an OverlaySceneLayer base class? Right not the duplicate code is
pretty limited, so I also think it's okay to keep the # of classes smaller.

[pedro (no code reviews), 2016-04-22 02:11:05]

Thanks for your huge effort in making this a reality!

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

https://codereview.chromium.org/1716653002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:296:
baseContentView.onDetachedFromWindow();
Please confirm with someone more familiar with the ContentView/ContentViewCore
if this is okay. It seems weird that we would call onDetachedFromWindow without
actually detaching it.

It seems that what you really want here is to set/unset the focus on the base
page's ContentView. The ContentView already has the requestFocus() method (from
Android's View class) and it seems that trying to call that instead might be
better here.

https://codereview.chromium.org/1716653002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:300:
if (!mHasFocus && isPanelOpened() && panelContentView != null) {
On 2016/04/21 21:22:38, Theresa Wellington wrote:
> I think this would read a little better if the this block was combined w/ the
if
> above. maybe:
> 
> if (!mHasFocus && isPanelOpened()) {
>     // Remove focus from the base page.
>     baseContentView.preserveSelectionOnNextLossOfFocus();
>     baseContentView.onDetachedFromWindow();
> 
>     // Set focus on the overlay content.
>     if (panelContentView != null) {
>         mHasFocus = true;
>         panelContentView.getContainerView().requestFocus();
>         panelContentView.onAttachedToWindow();
>     }
> } else if (mHasFocus && !isPanelOpened() && baseContentView != null) {
>    // Give focus to the base page.
>    ....
> }

I think it would be even better if there was separate methods focusBaseContent()
and unfocusBaseContent(), then we wouldn't have to add lots of comments
explaining the complexity of the method since each one would have a clear and
specific purpose.

https://codereview.chromium.org/1716653002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:303:
panelContentView.onAttachedToWindow();
Same comment about the onDetachedFromWindow() applies here.

https://codereview.chromium.org/1716653002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:823:
public void tabTitleChanged(int tabId, String title) {}
It feels really weird that a SceneOverlay needs all of these Tab methods. It
looks like a bunch of things that only StripLayoutHelperManager() cares about
was included in the SceneOverlay interface, which is sad.

https://codereview.chromium.org/1716653002/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/1716653002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:145:
updateContentFocus();
I think we should only call the code to focus/unfocus once when first
showing/hiding the panel layer, and not calling on every frame update.

Also a method named get<Something> should not have side effects.

At least, the only side effect I would expect to happen here is updating the
layer, since it's called getUpdatedSceneOverlayTree(). But other than that, I
would not expect any other side effects.

https://codereview.chromium.org/1716653002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:269:
if (mSceneLayer != null) mSceneLayer.hideTree();
Can't this be done by the OverlayPanel class?

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

https://codereview.chromium.org/1716653002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/readermode/ReaderModePanel.java:68:
OverlayPanelContentViewDelegate contentViewDelegate) {
Contextual Search uses a similar mechanism to inform the system about its
ContentViewCore. Shouldn't this belong to the OverlayPanelClass instead?

This is orthogonal to this change, so please put a TODO to address this later.

https://codereview.chromium.org/1716653002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/readermode/ReaderModePanel.java:138:
updateTopControlsState();
As stated in ContextualSearchPanel, a method named get<Something> should not
have side effects, which makes me think this is probably not the right place to
call this.

https://codereview.chromium.org/1716653002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/readermode/ReaderModePanel.java:141:
updateContentFocus();
Same comment I made in ContextualSearchPanel. I don't think we should call this
on every frame update.

https://codereview.chromium.org/1716653002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/readermode/ReaderModePanel.java:283:
if (mSceneLayer != null) mSceneLayer.hideTree();
Can't this be done by the OverlayPanel class?

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

https://codereview.chromium.org/1716653002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java:1086:
public boolean isTabCreatingIntercepted() {
Looks like this method is identical to the handlesTabCreating() above. Why do we
need this method?

https://codereview.chromium.org/1716653002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/StaticLayout.java
(left):

https://codereview.chromium.org/1716653002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/StaticLayout.java:243:
// TODO(pedrosimonetti): Coordinate w/ dtrainor@ to improve integration with
TreeProvider.
:)

https://codereview.chromium.org/1716653002/diff/140001/chrome/browser/android...
File chrome/browser/android/compositor/scene_layer/reader_mode_scene_layer.cc
(right):

https://codereview.chromium.org/1716653002/diff/140001/chrome/browser/android...
chrome/browser/android/compositor/scene_layer/reader_mode_scene_layer.cc:146:
content_container_->SetPosition(gfx::PointF(0.0f, 0.0f));
On 2016/04/21 21:22:39, Theresa Wellington wrote:
> Does it make sense for this code (and some of the other duplicated code) to be
> shared in an OverlaySceneLayer base class? Right not the duplicate code is
> pretty limited, so I also think it's okay to keep the # of classes smaller. 

I also think moving duplicate code to a shared class is a good idea.

[pedro (no code reviews), 2016-04-22 06:30:30]

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

https://codereview.chromium.org/1716653002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:296:
baseContentView.onDetachedFromWindow();
On 2016/04/22 02:11:04, pedrosimonetti wrote:
> Please confirm with someone more familiar with the ContentView/ContentViewCore
> if this is okay. It seems weird that we would call onDetachedFromWindow
without
> actually detaching it.
> 
> It seems that what you really want here is to set/unset the focus on the base
> page's ContentView. The ContentView already has the requestFocus() method
(from
> Android's View class) and it seems that trying to call that instead might be
> better here.

Just to be clear. Once preserveSelectionOnNextLossOfFocus() is called, all we
should do is to unfocus the base content view (by focusing the panel's content
view), and then ContentViewCore should handle preserving the selection and other
things (hiding Context Menu, etc.). If this is not happening, it is a bug in the
ContentViewCore code.

[mdjones, 2016-04-22 17:03:42]

Addressed most comments, will discuss others offline.

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

https://codereview.chromium.org/1716653002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:287:
* Set focus on the panel's content and reomve it from the base page.
On 2016/04/21 21:22:38, Theresa Wellington wrote:
> s/reomve/remove
> 
> I think this could also be expanded, e.g.
> "If the panel is open and doesn't have focus, set focus on the panel's content
> and remove it from the base page. If the panel is not open and has focus, set
> focus on the base page."

Done.

https://codereview.chromium.org/1716653002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:300:
if (!mHasFocus && isPanelOpened() && panelContentView != null) {
On 2016/04/21 21:22:38, Theresa Wellington wrote:
> I think this would read a little better if the this block was combined w/ the
if
> above. maybe:
> 
> if (!mHasFocus && isPanelOpened()) {
>     // Remove focus from the base page.
>     baseContentView.preserveSelectionOnNextLossOfFocus();
>     baseContentView.onDetachedFromWindow();
> 
>     // Set focus on the overlay content.
>     if (panelContentView != null) {
>         mHasFocus = true;
>         panelContentView.getContainerView().requestFocus();
>         panelContentView.onAttachedToWindow();
>     }
> } else if (mHasFocus && !isPanelOpened() && baseContentView != null) {
>    // Give focus to the base page.
>    ....
> }

Done.

https://codereview.chromium.org/1716653002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:823:
public void tabTitleChanged(int tabId, String title) {}
On 2016/04/22 02:11:04, pedrosimonetti wrote:
> It feels really weird that a SceneOverlay needs all of these Tab methods. It
> looks like a bunch of things that only StripLayoutHelperManager() cares about
> was included in the SceneOverlay interface, which is sad.

This interface was originally built with tab related additions in mind. It may
be possible to separate the functionality in the future.

https://codereview.chromium.org/1716653002/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/1716653002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:145:
updateContentFocus();
On 2016/04/22 02:11:05, pedrosimonetti wrote:
> I think we should only call the code to focus/unfocus once when first
> showing/hiding the panel layer, and not calling on every frame update.
> 
> Also a method named get<Something> should not have side effects.
> 
> At least, the only side effect I would expect to happen here is updating the
> layer, since it's called getUpdatedSceneOverlayTree(). But other than that, I
> would not expect any other side effects.

The contentviewcore may not be created by the time this is called. Unlike in the
past, this method is now called even when the panel is not showing. The panel
also needs to give/take focus when it goes from peek/open/peek without
destroying the CVC. I can re-architect to have events like
onContentViewCoreVisible but I'd rather save that for a separate cl.

https://codereview.chromium.org/1716653002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:146:
}
On 2016/04/21 21:22:38, Theresa Wellington wrote:
> Not knowing the history of this code particularly well, can mSceneLayer no
> longer null when this is called (where as it could in the old
> updateSceneLayer())?

mSceneLayer is built in the constructor. The creator method can be overridden,
but it is only supposed to be for testing.

https://codereview.chromium.org/1716653002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:158:

On 2016/04/21 21:22:38, Theresa Wellington wrote:
> Left over method? It doesn't look like this is used. If it is necessary, is it
> actually focusing the base page?

Definitely a method I cut the code from and forgot to remove the shell.

https://codereview.chromium.org/1716653002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:269:
if (mSceneLayer != null) mSceneLayer.hideTree();
On 2016/04/22 02:11:05, pedrosimonetti wrote:
> Can't this be done by the OverlayPanel class?

OverlayPanel currently doesn't have a concept of SceneLayer. Further, hideTree
is not a shared method of SceneLayer so we would need a base class for java
SceneLayers. I can do this, but as I mentioned earlier, I think it might be
worth having a follow-up CL.

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

https://codereview.chromium.org/1716653002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/readermode/ReaderModePanel.java:68:
OverlayPanelContentViewDelegate contentViewDelegate) {
On 2016/04/22 02:11:05, pedrosimonetti wrote:
> Contextual Search uses a similar mechanism to inform the system about its
> ContentViewCore. Shouldn't this belong to the OverlayPanelClass instead?
> 
> This is orthogonal to this change, so please put a TODO to address this later.

Yes, however, ContextualSearchPanel does not own its OverlayContentDelegate (the
thing that needs this object). It might even be better to move Reader Mode's
delegate out of this class to the manager.

https://codereview.chromium.org/1716653002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/readermode/ReaderModePanel.java:138:
updateTopControlsState();
On 2016/04/22 02:11:05, pedrosimonetti wrote:
> As stated in ContextualSearchPanel, a method named get<Something> should not
> have side effects, which makes me think this is probably not the right place
to
> call this.

Done.

https://codereview.chromium.org/1716653002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/readermode/ReaderModePanel.java:141:
updateContentFocus();
On 2016/04/22 02:11:05, pedrosimonetti wrote:
> Same comment I made in ContextualSearchPanel. I don't think we should call
this
> on every frame update.

See earlier comment.

https://codereview.chromium.org/1716653002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/readermode/ReaderModePanel.java:283:
if (mSceneLayer != null) mSceneLayer.hideTree();
On 2016/04/22 02:11:05, pedrosimonetti wrote:
> Can't this be done by the OverlayPanel class?

See above comment.

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

https://codereview.chromium.org/1716653002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java:1086:
public boolean isTabCreatingIntercepted() {
On 2016/04/22 02:11:05, pedrosimonetti wrote:
> Looks like this method is identical to the handlesTabCreating() above. Why do
we
> need this method? 

Another hold-over. This was originally created in this CL; I instead created a
separate  CL https://codereview.chromium.org/1738803004 that already merged. The
name was changed and I forgot to remove this.

https://codereview.chromium.org/1716653002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerDocument.java
(left):

https://codereview.chromium.org/1716653002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerDocument.java:60:
implements ContextualSearchTapHandler, OverlayPanelContentViewDelegate {
On 2016/04/21 21:22:39, Theresa Wellington wrote:
> Happy to see some of the CS specific stuff disappearing from this class!

\o/

https://codereview.chromium.org/1716653002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/StaticLayout.java
(left):

https://codereview.chromium.org/1716653002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/StaticLayout.java:34:
public class StaticLayout extends ContextualSearchSupportedLayout {
On 2016/04/21 21:22:39, Theresa Wellington wrote:
> Yes!!

This was honestly the most satisfying line of this whole change.

https://codereview.chromium.org/1716653002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/StaticLayout.java:243:
// TODO(pedrosimonetti): Coordinate w/ dtrainor@ to improve integration with
TreeProvider.
On 2016/04/22 02:11:05, pedrosimonetti wrote:
> :)

:D

https://codereview.chromium.org/1716653002/diff/140001/chrome/browser/android...
File chrome/browser/android/compositor/scene_layer/reader_mode_scene_layer.cc
(right):

https://codereview.chromium.org/1716653002/diff/140001/chrome/browser/android...
chrome/browser/android/compositor/scene_layer/reader_mode_scene_layer.cc:146:
content_container_->SetPosition(gfx::PointF(0.0f, 0.0f));
On 2016/04/21 21:22:39, Theresa Wellington wrote:
> Does it make sense for this code (and some of the other duplicated code) to be
> shared in an OverlaySceneLayer base class? Right not the duplicate code is
> pretty limited, so I also think it's okay to keep the # of classes smaller. 

This is something I'd like to address in a follow-up CL along with some of the
java changes I mentioned for pedro.

[Theresa, 2016-04-25 18:30:03]

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

https://codereview.chromium.org/1716653002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:299:
baseContentView.onDetachedFromWindow();
As discussed offline, maybe it makes sense to pull the contents of
ContentViewCore#onDetachedFromWindow() and onAttachedToWindow() out into new
methods that can be called from those methods and from here.

https://codereview.chromium.org/1716653002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:785:
// TODO(mdjones): Add views for accessibility.
Currently Contextual Search is just disabled when accessibility is on. Is the
same true for reader mode? (Just curious)

https://codereview.chromium.org/1716653002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java
(left):

https://codereview.chromium.org/1716653002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:333:
return mManagementDelegate != null &&
!mManagementDelegate.isRunningInCompatibilityMode();
Is contextual search still disabled on svelte devices with this line removed? 

Pedro - are we still disabled on svelte and when TalkBack is turned on?

https://codereview.chromium.org/1716653002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/readermode/ReaderModePanel.java
(right):

https://codereview.chromium.org/1716653002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/readermode/ReaderModePanel.java:138:
// updateTopControlsState();
Delete this line instead of commenting it out

https://codereview.chromium.org/1716653002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/eventfilter/ContextualSearchEventFilter.java
(left):

https://codereview.chromium.org/1716653002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/eventfilter/ContextualSearchEventFilter.java:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
Just to confirm, the event filter stuff was moved to OverlayPanelEventFilter in
a separate CL?

[pedro (no code reviews), 2016-04-25 18:46:13]

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

https://codereview.chromium.org/1716653002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:299:
baseContentView.onDetachedFromWindow();
On 2016/04/25 18:30:02, Theresa Wellington wrote:
> As discussed offline, maybe it makes sense to pull the contents of
> ContentViewCore#onDetachedFromWindow() and onAttachedToWindow() out into new
> methods that can be called from those methods and from here.

Could you elaborate what was discussed? Were ContentViewCore experts consulted?
What did they say about calling the detach/attach methods?

I wish I was included in those discussions since I brought up the concern about
calling those methods.

https://codereview.chromium.org/1716653002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java
(left):

https://codereview.chromium.org/1716653002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:333:
return mManagementDelegate != null &&
!mManagementDelegate.isRunningInCompatibilityMode();
On 2016/04/25 18:30:02, Theresa Wellington wrote:
> Is contextual search still disabled on svelte devices with this line removed? 
> 
> Pedro - are we still disabled on svelte and when TalkBack is turned on?

Good point. Don't know if CS will still be disabled after this change.

AFAIK it is disabled for svelte, regardless of when TalkBack is on. Also
disabled when TalkBack is on, regardless of whether it's a svelte device.

[Theresa, 2016-04-25 19:23:45]

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

https://codereview.chromium.org/1716653002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:299:
baseContentView.onDetachedFromWindow();
On 2016/04/25 18:46:13, pedrosimonetti wrote:
> On 2016/04/25 18:30:02, Theresa Wellington wrote:
> > As discussed offline, maybe it makes sense to pull the contents of
> > ContentViewCore#onDetachedFromWindow() and onAttachedToWindow() out into new
> > methods that can be called from those methods and from here.
> 
> Could you elaborate what was discussed? Were ContentViewCore experts
consulted?
> What did they say about calling the detach/attach methods?
> 
> I wish I was included in those discussions since I brought up the concern
about
> calling those methods.

It was just Matt and I discussing by his desk. If the same logic needs to run
when this method is called as when the CVC is attached/detached from the window,
then pulling it out into a separate method that can be called from both places
seems like a good option. That was the extent of the conversation.

[mdjones, 2016-04-25 20:17:19]

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

https://codereview.chromium.org/1716653002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:299:
baseContentView.onDetachedFromWindow();
On 2016/04/25 19:23:45, Theresa Wellington wrote:
> On 2016/04/25 18:46:13, pedrosimonetti wrote:
> > On 2016/04/25 18:30:02, Theresa Wellington wrote:
> > > As discussed offline, maybe it makes sense to pull the contents of
> > > ContentViewCore#onDetachedFromWindow() and onAttachedToWindow() out into
new
> > > methods that can be called from those methods and from here.
> > 
> > Could you elaborate what was discussed? Were ContentViewCore experts
> consulted?
> > What did they say about calling the detach/attach methods?
> > 
> > I wish I was included in those discussions since I brought up the concern
> about
> > calling those methods.
> 
> It was just Matt and I discussing by his desk. If the same logic needs to run
> when this method is called as when the CVC is attached/detached from the
window,
> then pulling it out into a separate method that can be called from both places
> seems like a good option. That was the extent of the conversation.

I haven't looked into this yet.

https://codereview.chromium.org/1716653002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:785:
// TODO(mdjones): Add views for accessibility.
On 2016/04/25 18:30:02, Theresa Wellington wrote:
> Currently Contextual Search is just disabled when accessibility is on. Is the
> same true for reader mode? (Just curious)

Yes, we should be disabled when accessibility is on.

https://codereview.chromium.org/1716653002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java
(left):

https://codereview.chromium.org/1716653002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:333:
return mManagementDelegate != null &&
!mManagementDelegate.isRunningInCompatibilityMode();
On 2016/04/25 18:46:13, pedrosimonetti wrote:
> On 2016/04/25 18:30:02, Theresa Wellington wrote:
> > Is contextual search still disabled on svelte devices with this line
removed? 
> > 
> > Pedro - are we still disabled on svelte and when TalkBack is turned on?
> 
> Good point. Don't know if CS will still be disabled after this change.
> 
> AFAIK it is disabled for svelte, regardless of when TalkBack is on. Also
> disabled when TalkBack is on, regardless of whether it's a svelte device.

This is handled by onInterceptPanelOpening(...) in this file. This was an
example of duplicate logic that we can remove now; its only purpose was to tell
the layout manager if the ContextualSearchLayout should show.

https://codereview.chromium.org/1716653002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/readermode/ReaderModePanel.java
(right):

https://codereview.chromium.org/1716653002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/readermode/ReaderModePanel.java:138:
// updateTopControlsState();
On 2016/04/25 18:30:02, Theresa Wellington wrote:
> Delete this line instead of commenting it out

Whoops.

https://codereview.chromium.org/1716653002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/eventfilter/ContextualSearchEventFilter.java
(left):

https://codereview.chromium.org/1716653002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/eventfilter/ContextualSearchEventFilter.java:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
On 2016/04/25 18:30:02, Theresa Wellington wrote:
> Just to confirm, the event filter stuff was moved to OverlayPanelEventFilter
in
> a separate CL?

Correct, the tests that support this will land after this.
(https://codereview.chromium.org/1808653006/)

[mdjones, 2016-04-25 20:17:46]

Addressed all but content focus issue, still looking into it.

[mdjones, 2016-04-25 21:10:12]

Pulled out some of the text selection code from ContentViewCore. I'll confirm
this is ok with a few other people.

[Theresa, 2016-04-25 21:16:28]

lgtm

[pedro (no code reviews), 2016-04-26 09:45:12]

Took another look. My main general comments are:

1) Always keep layers in the hierarchy?

We usually only keep things in the hierarchy when needed, and remove them as
soon as they are no longer needed.

Now we are: a) always keeping the overlay layers in the hierarchy, and; b)
delegating the responsibility of removing the inner layers to the OverlayPanel's
subclasses.

[a] is bad because even when both ContextualSearch and ReaderMode are not
displayed we'll still have 6 unused layers in hierarchy from what I can tell.
For each panel there will be the scene layer with two child layers (content and
panel layers).

And [b] makes the situation more fragile.

I think that adding/removing layers from the hierarchy should be handled by the
OverlayPanel class. The class already know exactly when it starts and finishes
showing.

We should also only create ContextualSearch/ReaderMode layers when necessary.


2) Focus

I think we should try using View.clearFocus() to solve the problem of
ContentViewCore not updating its internal states.

I don't like how we're constantly trying to update the focus on every frame.
Specially because it happens inside the getUpdatedSceneOverlayTree() method,
that should have no other side effects than updating the scene layer.

I think we should instead:

Inside OverlayPanel#notifyBarTouched() call private method unfocusBaseContent()
which will call preserveSelectionOnNextLossOfFocus() and clearFocus().

Inside OverlayPanel#onAnimationFinished() call focusBaseContent()

This way, it is much clearer when and how the focus is changing, and the
getUpdatedSceneOverlayTree() only updates the layer as a side effect.


3) onInterceptBarSwipe()

It seems this not being called anymore. This should be fixed.


4) Changes in ContextualSearchPanel/ReaderModePanel and
ContextualSearchSceneLayer/ReaderModeSceneLayer are almost identical, and are
handling a very important aspect of the OverlayPanel. It would be nice having
the OverlayPanel and a new OverlayPanelSceneLayer classes handle that.

https://codereview.chromium.org/1716653002/diff/220001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java
(left):

https://codereview.chromium.org/1716653002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
See my main message before reading comments in code.

https://codereview.chromium.org/1716653002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:467:
if (mOverlayPanelHost != null) {
Here you could call focusBaseContent()

https://codereview.chromium.org/1716653002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:478:
private boolean shouldHideOverlayPanelLayout() {
Instead, you could rename this method to shouldRestoreBaseContentFocus()

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

https://codereview.chromium.org/1716653002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:252:
getOverlayPanelContent().showContent();
You could call unfocusBaseContent() here.

https://codereview.chromium.org/1716653002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:290:
protected void updateContentFocus() {
As suggested before, can we split this method into focusBaseContent(),
unfocusBaseContent() methods?

It would be easier to reason about them if each one had a more specific purpose.

https://codereview.chromium.org/1716653002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:302:
panelContentView.getContainerView().requestFocus();
As we discussed offline, you might be able to achieve what you want by calling
View.clearFocus() method here instead of setting the focus on the panel's
content.

https://codereview.chromium.org/1716653002/diff/220001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerDocument.java
(left):

https://codereview.chromium.org/1716653002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerDocument.java:381:
if (panel != null && !panel.onInterceptBarSwipe()) {
Is this not being calling onInterceptBarSwipe() anymore? If not, it should be.

[mdjones, 2016-04-26 20:54:10]

On 2016/04/26 09:45:12, pedrosimonetti wrote:
> Took another look. My main general comments are:
> 
> 1) Always keep layers in the hierarchy?
> 
> We usually only keep things in the hierarchy when needed, and remove them as
> soon as they are no longer needed.

This is a restriction imposed by the infrastructure that can be fixed in a
follow-up. Either way, the layer is made invisible and is not updated when a
panel is not showing.

> 
> Now we are: a) always keeping the overlay layers in the hierarchy, and; b)
> delegating the responsibility of removing the inner layers to the
OverlayPanel's
> subclasses.

As we previously discussed, the subclassing can be addressed in a follow-up cl.

> 
> [a] is bad because even when both ContextualSearch and ReaderMode are not
> displayed we'll still have 6 unused layers in hierarchy from what I can tell.
> For each panel there will be the scene layer with two child layers (content
and
> panel layers).
> 
> And [b] makes the situation more fragile.

Each panel introduces a single new parent layer to the content tree. I'm not
sure how having those layers in the tree makes this more fragile.

> 
> I think that adding/removing layers from the hierarchy should be handled by
the
> OverlayPanel class. The class already know exactly when it starts and finishes
> showing.

If we decide to do this, it can be handled in the same CL that adds support for
it in SceneOverlay.

> 
> We should also only create ContextualSearch/ReaderMode layers when necessary.

Unless the feature is disabled we only ever create a single instance of each of
the layers. This concern is secondary.

> 
> 
> 2) Focus
> 
> I think we should try using View.clearFocus() to solve the problem of
> ContentViewCore not updating its internal states.
> 
> I don't like how we're constantly trying to update the focus on every frame.
> Specially because it happens inside the getUpdatedSceneOverlayTree() method,
> that should have no other side effects than updating the scene layer.
> 
> I think we should instead:
> 
> Inside OverlayPanel#notifyBarTouched() call private method
unfocusBaseContent()
> which will call preserveSelectionOnNextLossOfFocus() and clearFocus().
> 
> Inside OverlayPanel#onAnimationFinished() call focusBaseContent()
> 
> This way, it is much clearer when and how the focus is changing, and the
> getUpdatedSceneOverlayTree() only updates the layer as a side effect.

For the most part this is done now. See comments in the code.

> 
> 
> 3) onInterceptBarSwipe()
> 
> It seems this not being called anymore. This should be fixed.

Done.

> 
> 
> 4) Changes in ContextualSearchPanel/ReaderModePanel and
> ContextualSearchSceneLayer/ReaderModeSceneLayer are almost identical, and are
> handling a very important aspect of the OverlayPanel. It would be nice having
> the OverlayPanel and a new OverlayPanelSceneLayer classes handle that.

As discussed in #1, this would be a separate cl.

> 
>
https://codereview.chromium.org/1716653002/diff/220001/chrome/android/java/sr...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java
> (left):
> 
>
https://codereview.chromium.org/1716653002/diff/220001/chrome/android/java/sr...
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:1:
> // Copyright 2015 The Chromium Authors. All rights reserved.
> See my main message before reading comments in code.
> 
>
https://codereview.chromium.org/1716653002/diff/220001/chrome/android/java/sr...
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:467:
> if (mOverlayPanelHost != null) {
> Here you could call focusBaseContent()
> 
>
https://codereview.chromium.org/1716653002/diff/220001/chrome/android/java/sr...
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:478:
> private boolean shouldHideOverlayPanelLayout() {
> Instead, you could rename this method to shouldRestoreBaseContentFocus()
> 
>
https://codereview.chromium.org/1716653002/diff/220001/chrome/android/java/sr...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java
> (right):
> 
>
https://codereview.chromium.org/1716653002/diff/220001/chrome/android/java/sr...
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:252:
> getOverlayPanelContent().showContent();
> You could call unfocusBaseContent() here.
> 
>
https://codereview.chromium.org/1716653002/diff/220001/chrome/android/java/sr...
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:290:
> protected void updateContentFocus() {
> As suggested before, can we split this method into focusBaseContent(),
> unfocusBaseContent() methods?
> 
> It would be easier to reason about them if each one had a more specific
purpose.
> 
>
https://codereview.chromium.org/1716653002/diff/220001/chrome/android/java/sr...
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:302:
> panelContentView.getContainerView().requestFocus();
> As we discussed offline, you might be able to achieve what you want by calling
> View.clearFocus() method here instead of setting the focus on the panel's
> content.
> 
>
https://codereview.chromium.org/1716653002/diff/220001/chrome/android/java/sr...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerDocument.java
> (left):
> 
>
https://codereview.chromium.org/1716653002/diff/220001/chrome/android/java/sr...
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerDocument.java:381:
> if (panel != null && !panel.onInterceptBarSwipe()) {
> Is this not being calling onInterceptBarSwipe() anymore? If not, it should be.

[mdjones, 2016-04-26 20:54:21]

https://codereview.chromium.org/1716653002/diff/220001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java
(left):

https://codereview.chromium.org/1716653002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:467:
if (mOverlayPanelHost != null) {
On 2016/04/26 09:45:11, pedrosimonetti wrote:
> Here you could call focusBaseContent()

What if the ContentViewCore has not been created yet? The panel's content will
never get focus. onContentViewCoreCreated seems like the solution here, but I
think we would need to support multiple delegates on the OverlayPanelContent,
especially if we want this to be the OverlayPanel's responsibility.

I added this but kept a call to focusPanelContent in the getUpdatedSceneLayer
function.

https://codereview.chromium.org/1716653002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:478:
private boolean shouldHideOverlayPanelLayout() {
On 2016/04/26 09:45:11, pedrosimonetti wrote:
> Instead, you could rename this method to shouldRestoreBaseContentFocus()

I just included this logic in the above function.

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

https://codereview.chromium.org/1716653002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:252:
getOverlayPanelContent().showContent();
On 2016/04/26 09:45:11, pedrosimonetti wrote:
> You could call unfocusBaseContent() here.

Possibly no ContentViewCore at this point.

https://codereview.chromium.org/1716653002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:290:
protected void updateContentFocus() {
On 2016/04/26 09:45:11, pedrosimonetti wrote:
> As suggested before, can we split this method into focusBaseContent(),
> unfocusBaseContent() methods?
> 
> It would be easier to reason about them if each one had a more specific
purpose.

I personally find it easier to manage all the focus changing logic in a single
method considering the signals we currently have access to. This also requires
extra logic in more places to work. Updated my code despite these concerns.

https://codereview.chromium.org/1716653002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:302:
panelContentView.getContainerView().requestFocus();
On 2016/04/26 09:45:11, pedrosimonetti wrote:
> As we discussed offline, you might be able to achieve what you want by calling
> View.clearFocus() method here instead of setting the focus on the panel's
> content.

The base content view is being refocused almost immediately after I clear focus.
It looks like fixing this would require changes in ContentViewCore.

https://codereview.chromium.org/1716653002/diff/220001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerDocument.java
(left):

https://codereview.chromium.org/1716653002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerDocument.java:381:
if (panel != null && !panel.onInterceptBarSwipe()) {
On 2016/04/26 09:45:11, pedrosimonetti wrote:
> Is this not being calling onInterceptBarSwipe() anymore? If not, it should be.

Done.

[mdjones, 2016-05-03 20:14:44]

Created follow-up change to address extra layers in the tree:
https://codereview.chromium.org/1936133003/. Are there any other major concerns
with this change?

[pedro (no code reviews), 2016-05-04 18:34:18]

On 2016/05/03 20:14:44, mdjones wrote:
> Created follow-up change to address extra layers in the tree:
> https://codereview.chromium.org/1936133003/. Are there any other major
concerns
> with this change?

That change is addressing a concern that I expressed while reviewing this change
and would make more sense if it were included in this change.

Or at the very least, I should have been included as a reviewer, or added to the
cc list.

Can you merge those changes into this CL?

[mdjones, 2016-05-04 20:58:39]

On 2016/05/04 18:34:18, pedrosimonetti wrote:
> On 2016/05/03 20:14:44, mdjones wrote:
> > Created follow-up change to address extra layers in the tree:
> > https://codereview.chromium.org/1936133003/. Are there any other major
> concerns
> > with this change?
> 
> That change is addressing a concern that I expressed while reviewing this
change
> and would make more sense if it were included in this change.
> 
> Or at the very least, I should have been included as a reviewer, or added to
the
> cc list.
> 
> Can you merge those changes into this CL?

I'd prefer not to since it stands as a separate API change and this change is
already huge. It will be landing almost immediately after this change (as that
one depends on this).

[mdjones, 2016-05-04 21:02:22]

On 2016/05/04 20:58:39, mdjones wrote:
> On 2016/05/04 18:34:18, pedrosimonetti wrote:
> > On 2016/05/03 20:14:44, mdjones wrote:
> > > Created follow-up change to address extra layers in the tree:
> > > https://codereview.chromium.org/1936133003/. Are there any other major
> > concerns
> > > with this change?
> > 
> > That change is addressing a concern that I expressed while reviewing this
> change
> > and would make more sense if it were included in this change.
> > 
> > Or at the very least, I should have been included as a reviewer, or added to
> the
> > cc list.
> > 
> > Can you merge those changes into this CL?
> 
> I'd prefer not to since it stands as a separate API change and this change is
> already huge. It will be landing almost immediately after this change (as that
> one depends on this).

Also worth noting is that the change is less about removing layers and more
about altering how the tree is rebuilt (when it eventually is).

[pedro (no code reviews), 2016-05-04 22:58:56]

On 2016/05/04 21:02:22, mdjones wrote:
> On 2016/05/04 20:58:39, mdjones wrote:
> > On 2016/05/04 18:34:18, pedrosimonetti wrote:
> > > On 2016/05/03 20:14:44, mdjones wrote:
> > > > Created follow-up change to address extra layers in the tree:
> > > > https://codereview.chromium.org/1936133003/. Are there any other major
> > > concerns
> > > > with this change?
> > > 
> > > That change is addressing a concern that I expressed while reviewing this
> > change
> > > and would make more sense if it were included in this change.
> > > 
> > > Or at the very least, I should have been included as a reviewer, or added
to
> > the
> > > cc list.
> > > 
> > > Can you merge those changes into this CL?
> > 
> > I'd prefer not to since it stands as a separate API change and this change
is
> > already huge. It will be landing almost immediately after this change (as
that
> > one depends on this).
> 
> Also worth noting is that the change is less about removing layers and more
> about altering how the tree is rebuilt (when it eventually is).

I completely disagree. That change is all about preventing the problem caused by
this change. If this change wasn't adding a bunch of layers even when we don't
need them, then we wouldn't have to worry about how the tree is rebuilt.

Yes, this change is big, but I don't think merging a 22 line change here would
make things more difficult to review. Specially when it's fixing a problem
caused by this change. Having the problem and the fix together makes much more
sense, specially because changes might get merged, reverted, and so on, so
having them separate could complicate things because nobody will know that they
are actually related.

[pedro (no code reviews), 2016-05-04 22:59:34]

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

https://codereview.chromium.org/1716653002/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:119:
private boolean mHasFocus;
We don't control the focus, we simply request it. But calling requestFocus()
doesn't guarantee that it will be focused, so we shouldn't assume that.

Instead it should be called mHasRequestedFocus.

https://codereview.chromium.org/1716653002/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:293:
protected void clearPanelFocus() {
As I said before, I think we should model this in terms of the *base page*
focus.

We only need to deal with the focus because we wan't to hide the selection
controls (pins, context menu, etc). And the way it should work is simply calling
preserveSelectionOnNextLossOfFocus() then making the ContentView loose focus. If
this is not happening is a bug and should be fixed.

We shouldn't even care if the *panel* ContentView doesn't exist. Even if there's
no panel ContentView, this should still work, as long as we unfocus (then later
focus) the *base* page.

I still think this code would be clearer if this method was called
focusBaseContent() and the one below unfocusBaseContent().

https://codereview.chromium.org/1716653002/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:296:
ContentViewCore panelContentView = getContentViewCore();
Why are storing panelContentView here?

https://codereview.chromium.org/1716653002/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:303:
baseContentView.updateTextSelectionUI(true);
This method is called clearPanelFocus() but it is not doing that. This, in my
opinion, is yet another sign that this is not quite right.

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

https://codereview.chromium.org/1716653002/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:143:
if (isPanelOpened()) {
As I said before, this method should not have any side effect other than
updating the scene layer.

This is called getUpdatedSceneOverlayTree() and not
getUpdatedSceneOverlayTreeButBeforeFocusPanelContentIfPanelIsOpenedAndNotFocusedAlready()

https://codereview.chromium.org/1716653002/diff/260001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/readermode/ReaderModePanel.java
(right):

https://codereview.chromium.org/1716653002/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/readermode/ReaderModePanel.java:136:
if (isPanelOpened()) {
As I said before, this method should not have any side effect other than
updating the scene layer.

This is called getUpdatedSceneOverlayTree() and not
getUpdatedSceneOverlayTreeButBeforeFocusPanelContentIfPanelIsOpenedAndNotFocusedAlready()

https://codereview.chromium.org/1716653002/diff/260001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ContextualSearchSceneLayer.java
(right):

https://codereview.chromium.org/1716653002/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ContextualSearchSceneLayer.java:51:
nativeCreateContextualSearchLayer(mNativePtr, resourceManager);
Can we only create the layer when it becomes visible?

https://codereview.chromium.org/1716653002/diff/260001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ReaderModeSceneLayer.java
(right):

https://codereview.chromium.org/1716653002/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ReaderModeSceneLayer.java:45:
nativeCreateReaderModeLayer(mNativePtr, resourceManager);
Can we only create the layer when it becomes visible?

https://codereview.chromium.org/1716653002/diff/260001/chrome/browser/android...
File
chrome/browser/android/compositor/scene_layer/contextual_search_scene_layer.cc
(right):

https://codereview.chromium.org/1716653002/diff/260001/chrome/browser/android...
chrome/browser/android/compositor/scene_layer/contextual_search_scene_layer.cc:30:
void ContextualSearchSceneLayer::CreateContextualSearchLayer(
TODO to move duplicated code to a super class?

https://codereview.chromium.org/1716653002/diff/260001/chrome/browser/android...
File chrome/browser/android/compositor/scene_layer/reader_mode_scene_layer.cc
(right):

https://codereview.chromium.org/1716653002/diff/260001/chrome/browser/android...
chrome/browser/android/compositor/scene_layer/reader_mode_scene_layer.cc:40: //
The Reader Mode layer is initially invisible.
TODO to move duplicated code to a super class?

https://codereview.chromium.org/1716653002/diff/260001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/1716653002/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1522:
public void updateTextSelectionUI(boolean focused) {
Have you talked to aelias@ about this issue, who now owns the selection code?

The original owner, jdduke@ was pretty clear that he didn't want to expose a
method to control the visibility of the selection UI. That's why we came up with
the preserveSelectionOnNextLossOfFocus() API.

This is a workaround to overcome the fact that
preserveSelectionOnNextLossOfFocus() API is not be doing what it was suppose to
and is no better than calling onAttachedToWindow()/onDetachedFromWindow() as you
were doing before.

There should not be a way for consumers to hide/show the selection controls at
will.

If you can't fix this now I'd prefer calling
onAttachedToWindow()/onDetachedFromWindow() as you were doing before. At least
this way we won't be exposing a way for other to control the visibility of the
selection UI.

[mdjones, 2016-05-05 00:46:21]

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

https://codereview.chromium.org/1716653002/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:119:
private boolean mHasFocus;
On 2016/05/04 22:59:33, pedrosimonetti wrote:
> We don't control the focus, we simply request it. But calling requestFocus()
> doesn't guarantee that it will be focused, so we shouldn't assume that.
> 
> Instead it should be called mHasRequestedFocus.

Done.

https://codereview.chromium.org/1716653002/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:293:
protected void clearPanelFocus() {
On 2016/05/04 22:59:34, pedrosimonetti wrote:
> As I said before, I think we should model this in terms of the *base page*
> focus.
> 
> We only need to deal with the focus because we wan't to hide the selection
> controls (pins, context menu, etc). And the way it should work is simply
calling
> preserveSelectionOnNextLossOfFocus() then making the ContentView loose focus.
If
> this is not happening is a bug and should be fixed.
> 
> We shouldn't even care if the *panel* ContentView doesn't exist. Even if
there's
> no panel ContentView, this should still work, as long as we unfocus (then
later
> focus) the *base* page.
> 
> I still think this code would be clearer if this method was called
> focusBaseContent() and the one below unfocusBaseContent().

So, just to be clear, we don't care if the panel's content ever gains focus?

Either way, I'll still argue that functionality of the OverlayPanel should be
with respect to itself rather than something else.

https://codereview.chromium.org/1716653002/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:296:
ContentViewCore panelContentView = getContentViewCore();
On 2016/05/04 22:59:33, pedrosimonetti wrote:
> Why are storing panelContentView here?

Not sure. Removed.

https://codereview.chromium.org/1716653002/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:303:
baseContentView.updateTextSelectionUI(true);
On 2016/05/04 22:59:33, pedrosimonetti wrote:
> This method is called clearPanelFocus() but it is not doing that. This, in my
> opinion, is yet another sign that this is not quite right.

Fixed.

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

https://codereview.chromium.org/1716653002/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:143:
if (isPanelOpened()) {
On 2016/05/04 22:59:34, pedrosimonetti wrote:
> As I said before, this method should not have any side effect other than
> updating the scene layer.
> 
> This is called getUpdatedSceneOverlayTree() and not
>
getUpdatedSceneOverlayTreeButBeforeFocusPanelContentIfPanelIsOpenedAndNotFocusedAlready()

I'm pretty sure it was called that at one point. Moved call elsewhere.

https://codereview.chromium.org/1716653002/diff/260001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/readermode/ReaderModePanel.java
(right):

https://codereview.chromium.org/1716653002/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/readermode/ReaderModePanel.java:136:
if (isPanelOpened()) {
On 2016/05/04 22:59:34, pedrosimonetti wrote:
> As I said before, this method should not have any side effect other than
> updating the scene layer.
> 
> This is called getUpdatedSceneOverlayTree() and not
>
getUpdatedSceneOverlayTreeButBeforeFocusPanelContentIfPanelIsOpenedAndNotFocusedAlready()

Moved.

https://codereview.chromium.org/1716653002/diff/260001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ContextualSearchSceneLayer.java
(right):

https://codereview.chromium.org/1716653002/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ContextualSearchSceneLayer.java:51:
nativeCreateContextualSearchLayer(mNativePtr, resourceManager);
On 2016/05/04 22:59:34, pedrosimonetti wrote:
> Can we only create the layer when it becomes visible?

With the follow-up change, that will be the case.

https://codereview.chromium.org/1716653002/diff/260001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ReaderModeSceneLayer.java
(right):

https://codereview.chromium.org/1716653002/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ReaderModeSceneLayer.java:45:
nativeCreateReaderModeLayer(mNativePtr, resourceManager);
On 2016/05/04 22:59:34, pedrosimonetti wrote:
> Can we only create the layer when it becomes visible?

Same as above, follow-up will fix this.

https://codereview.chromium.org/1716653002/diff/260001/chrome/browser/android...
File
chrome/browser/android/compositor/scene_layer/contextual_search_scene_layer.cc
(right):

https://codereview.chromium.org/1716653002/diff/260001/chrome/browser/android...
chrome/browser/android/compositor/scene_layer/contextual_search_scene_layer.cc:30:
void ContextualSearchSceneLayer::CreateContextualSearchLayer(
On 2016/05/04 22:59:34, pedrosimonetti wrote:
> TODO to move duplicated code to a super class?

Done.

https://codereview.chromium.org/1716653002/diff/260001/chrome/browser/android...
File chrome/browser/android/compositor/scene_layer/reader_mode_scene_layer.cc
(right):

https://codereview.chromium.org/1716653002/diff/260001/chrome/browser/android...
chrome/browser/android/compositor/scene_layer/reader_mode_scene_layer.cc:40: //
The Reader Mode layer is initially invisible.
On 2016/05/04 22:59:34, pedrosimonetti wrote:
> TODO to move duplicated code to a super class?

Done.

https://codereview.chromium.org/1716653002/diff/260001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/1716653002/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1522:
public void updateTextSelectionUI(boolean focused) {
On 2016/05/04 22:59:34, pedrosimonetti wrote:
> Have you talked to aelias@ about this issue, who now owns the selection code?
> 
> The original owner, jdduke@ was pretty clear that he didn't want to expose a
> method to control the visibility of the selection UI. That's why we came up
with
> the preserveSelectionOnNextLossOfFocus() API.
> 
> This is a workaround to overcome the fact that
> preserveSelectionOnNextLossOfFocus() API is not be doing what it was suppose
to
> and is no better than calling onAttachedToWindow()/onDetachedFromWindow() as
you
> were doing before.
> 
> There should not be a way for consumers to hide/show the selection controls at
> will.
> 
> If you can't fix this now I'd prefer calling
> onAttachedToWindow()/onDetachedFromWindow() as you were doing before. At least
> this way we won't be exposing a way for other to control the visibility of the
> selection UI.
> 

I'll chat some more with the content people. I have talked to a couple of people
in the UI office who would rather see this separated.

[mdjones, 2016-05-05 01:18:53]

On 2016/05/04 22:58:56, pedrosimonetti wrote:
> On 2016/05/04 21:02:22, mdjones wrote:
> > On 2016/05/04 20:58:39, mdjones wrote:
> > > On 2016/05/04 18:34:18, pedrosimonetti wrote:
> > > > On 2016/05/03 20:14:44, mdjones wrote:
> > > > > Created follow-up change to address extra layers in the tree:
> > > > > https://codereview.chromium.org/1936133003/. Are there any other major
> > > > concerns
> > > > > with this change?
> > > > 
> > > > That change is addressing a concern that I expressed while reviewing
this
> > > change
> > > > and would make more sense if it were included in this change.
> > > > 
> > > > Or at the very least, I should have been included as a reviewer, or
added
> to
> > > the
> > > > cc list.
> > > > 
> > > > Can you merge those changes into this CL?
> > > 
> > > I'd prefer not to since it stands as a separate API change and this change
> is
> > > already huge. It will be landing almost immediately after this change (as
> that
> > > one depends on this).
> > 
> > Also worth noting is that the change is less about removing layers and more
> > about altering how the tree is rebuilt (when it eventually is).
> 
> I completely disagree. That change is all about preventing the problem caused
by
> this change. If this change wasn't adding a bunch of layers even when we don't
> need them, then we wouldn't have to worry about how the tree is rebuilt.
> 
> Yes, this change is big, but I don't think merging a 22 line change here would
> make things more difficult to review. Specially when it's fixing a problem
> caused by this change. Having the problem and the fix together makes much more
> sense, specially because changes might get merged, reverted, and so on, so
> having them separate could complicate things because nobody will know that
they
> are actually related.

To address your concerns:

- Hidden layers have extremely little effect on the overall tree (I talked
offline to vmpstr@), so I don't think this is an extreme problem.
- We aren't merging this change anywhere (I don't see why we would).
- Dependencies are tracked by this site (see the bottom of each patch) so people
will know they are related.

This change has the explicit goal of implementing OverlayPanels as
SceneOverlays, I think we should try to stick to that and address other concerns
in follow-ups.

[pedro (no code reviews), 2016-05-05 01:40:52]

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

https://codereview.chromium.org/1716653002/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:293:
protected void clearPanelFocus() {
On 2016/05/05 00:46:20, mdjones wrote:
> On 2016/05/04 22:59:34, pedrosimonetti wrote:
> > As I said before, I think we should model this in terms of the *base page*
> > focus.
> > 
> > We only need to deal with the focus because we wan't to hide the selection
> > controls (pins, context menu, etc). And the way it should work is simply
> calling
> > preserveSelectionOnNextLossOfFocus() then making the ContentView loose
focus.
> If
> > this is not happening is a bug and should be fixed.
> > 
> > We shouldn't even care if the *panel* ContentView doesn't exist. Even if
> there's
> > no panel ContentView, this should still work, as long as we unfocus (then
> later
> > focus) the *base* page.
> > 
> > I still think this code would be clearer if this method was called
> > focusBaseContent() and the one below unfocusBaseContent().
> 
> So, just to be clear, we don't care if the panel's content ever gains focus?
> 
> Either way, I'll still argue that functionality of the OverlayPanel should be
> with respect to itself rather than something else.

Yes, we don't care whether the panel's content gets focused.

We only care about focusing/unfocusing the base page's content in order to
hide/show selection controls.

https://codereview.chromium.org/1716653002/diff/260001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ContextualSearchSceneLayer.java
(right):

https://codereview.chromium.org/1716653002/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ContextualSearchSceneLayer.java:51:
nativeCreateContextualSearchLayer(mNativePtr, resourceManager);
On 2016/05/05 00:46:20, mdjones wrote:
> On 2016/05/04 22:59:34, pedrosimonetti wrote:
> > Can we only create the layer when it becomes visible?
> 
> With the follow-up change, that will be the case.

The follow-up change will prevent this update() method from being called.

What I'm asking is, even if this method gets called, why would we create the
layer if the panel is not visible?

In other words, can't we just move the if block to right after the early return
below?

https://codereview.chromium.org/1716653002/diff/260001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/1716653002/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1522:
public void updateTextSelectionUI(boolean focused) {
On 2016/05/05 00:46:21, mdjones wrote:
> On 2016/05/04 22:59:34, pedrosimonetti wrote:
> > Have you talked to aelias@ about this issue, who now owns the selection
code?
> > 
> > The original owner, jdduke@ was pretty clear that he didn't want to expose a
> > method to control the visibility of the selection UI. That's why we came up
> with
> > the preserveSelectionOnNextLossOfFocus() API.
> > 
> > This is a workaround to overcome the fact that
> > preserveSelectionOnNextLossOfFocus() API is not be doing what it was suppose
> to
> > and is no better than calling onAttachedToWindow()/onDetachedFromWindow() as
> you
> > were doing before.
> > 
> > There should not be a way for consumers to hide/show the selection controls
at
> > will.
> > 
> > If you can't fix this now I'd prefer calling
> > onAttachedToWindow()/onDetachedFromWindow() as you were doing before. At
least
> > this way we won't be exposing a way for other to control the visibility of
the
> > selection UI.
> > 
> 
> I'll chat some more with the content people. I have talked to a couple of
people
> in the UI office who would rather see this separated.

I'm surprised that we're okay with allowing other to control when to show/hide
the selection controls.

This means that a bug in our code could potentially permanently hide the
controls for a particular tab, which would completely break the user selection
flow.

[mdjones, 2016-05-05 17:48:34]

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

https://codereview.chromium.org/1716653002/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:293:
protected void clearPanelFocus() {
On 2016/05/05 01:40:52, pedrosimonetti wrote:
> On 2016/05/05 00:46:20, mdjones wrote:
> > On 2016/05/04 22:59:34, pedrosimonetti wrote:
> > > As I said before, I think we should model this in terms of the *base page*
> > > focus.
> > > 
> > > We only need to deal with the focus because we wan't to hide the selection
> > > controls (pins, context menu, etc). And the way it should work is simply
> > calling
> > > preserveSelectionOnNextLossOfFocus() then making the ContentView loose
> focus.
> > If
> > > this is not happening is a bug and should be fixed.
> > > 
> > > We shouldn't even care if the *panel* ContentView doesn't exist. Even if
> > there's
> > > no panel ContentView, this should still work, as long as we unfocus (then
> > later
> > > focus) the *base* page.
> > > 
> > > I still think this code would be clearer if this method was called
> > > focusBaseContent() and the one below unfocusBaseContent().
> > 
> > So, just to be clear, we don't care if the panel's content ever gains focus?
> > 
> > Either way, I'll still argue that functionality of the OverlayPanel should
be
> > with respect to itself rather than something else.
> 
> Yes, we don't care whether the panel's content gets focused.
> 
> We only care about focusing/unfocusing the base page's content in order to
> hide/show selection controls.

Not according to crbug.com/531743. It actually seems like it will be fairly
important (especially for things like keyboard input).

I renamed the methods and removed the logic for focusing the panel content.

https://codereview.chromium.org/1716653002/diff/260001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ContextualSearchSceneLayer.java
(right):

https://codereview.chromium.org/1716653002/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ContextualSearchSceneLayer.java:51:
nativeCreateContextualSearchLayer(mNativePtr, resourceManager);
On 2016/05/05 01:40:52, pedrosimonetti wrote:
> On 2016/05/05 00:46:20, mdjones wrote:
> > On 2016/05/04 22:59:34, pedrosimonetti wrote:
> > > Can we only create the layer when it becomes visible?
> > 
> > With the follow-up change, that will be the case.
> 
> The follow-up change will prevent this update() method from being called.
> 
> What I'm asking is, even if this method gets called, why would we create the
> layer if the panel is not visible?
> 
> In other words, can't we just move the if block to right after the early
return
> below?

Unfortunately, no. The scene layer needs to exist in order for the content tree
to be added to it (otherwise we just get a white screen).

https://codereview.chromium.org/1716653002/diff/260001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/1716653002/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1522:
public void updateTextSelectionUI(boolean focused) {
On 2016/05/05 01:40:52, pedrosimonetti wrote:
> On 2016/05/05 00:46:21, mdjones wrote:
> > On 2016/05/04 22:59:34, pedrosimonetti wrote:
> > > Have you talked to aelias@ about this issue, who now owns the selection
> code?
> > > 
> > > The original owner, jdduke@ was pretty clear that he didn't want to expose
a
> > > method to control the visibility of the selection UI. That's why we came
up
> > with
> > > the preserveSelectionOnNextLossOfFocus() API.
> > > 
> > > This is a workaround to overcome the fact that
> > > preserveSelectionOnNextLossOfFocus() API is not be doing what it was
suppose
> > to
> > > and is no better than calling onAttachedToWindow()/onDetachedFromWindow()
as
> > you
> > > were doing before.
> > > 
> > > There should not be a way for consumers to hide/show the selection
controls
> at
> > > will.
> > > 
> > > If you can't fix this now I'd prefer calling
> > > onAttachedToWindow()/onDetachedFromWindow() as you were doing before. At
> least
> > > this way we won't be exposing a way for other to control the visibility of
> the
> > > selection UI.
> > > 
> > 
> > I'll chat some more with the content people. I have talked to a couple of
> people
> > in the UI office who would rather see this separated.
> 
> I'm surprised that we're okay with allowing other to control when to show/hide
> the selection controls.
> 
> This means that a bug in our code could potentially permanently hide the
> controls for a particular tab, which would completely break the user selection
> flow.

That's true, but in a similar manner a bug in the overlay panel could hide part
or all of the content on the page, breaking the browser experience (which has
actually happened before).

This decision will be up to the content folks when I add them to the review.

[pedro (no code reviews), 2016-05-05 19:12:16]

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

https://codereview.chromium.org/1716653002/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:293:
protected void clearPanelFocus() {
On 2016/05/05 17:48:34, mdjones wrote:
> On 2016/05/05 01:40:52, pedrosimonetti wrote:
> > On 2016/05/05 00:46:20, mdjones wrote:
> > > On 2016/05/04 22:59:34, pedrosimonetti wrote:
> > > > As I said before, I think we should model this in terms of the *base
page*
> > > > focus.
> > > > 
> > > > We only need to deal with the focus because we wan't to hide the
selection
> > > > controls (pins, context menu, etc). And the way it should work is simply
> > > calling
> > > > preserveSelectionOnNextLossOfFocus() then making the ContentView loose
> > focus.
> > > If
> > > > this is not happening is a bug and should be fixed.
> > > > 
> > > > We shouldn't even care if the *panel* ContentView doesn't exist. Even if
> > > there's
> > > > no panel ContentView, this should still work, as long as we unfocus
(then
> > > later
> > > > focus) the *base* page.
> > > > 
> > > > I still think this code would be clearer if this method was called
> > > > focusBaseContent() and the one below unfocusBaseContent().
> > > 
> > > So, just to be clear, we don't care if the panel's content ever gains
focus?
> > > 
> > > Either way, I'll still argue that functionality of the OverlayPanel should
> be
> > > with respect to itself rather than something else.
> > 
> > Yes, we don't care whether the panel's content gets focused.
> > 
> > We only care about focusing/unfocusing the base page's content in order to
> > hide/show selection controls.
> 
> Not according to crbug.com/531743. It actually seems like it will be fairly
> important (especially for things like keyboard input).
> 
> I renamed the methods and removed the logic for focusing the panel content.

That bug has nothing to do with what we're discussing here. That bug refers to
supporting common tab features like context menus in the panel. In the future,
we might need to focus the panel's content in order to support editing input
fields for example, but there are strong indications that calling requestFocus()
or clearFocus() is not doing what we expect.

Currently, we don't focus the panel's content, and we only care about
unfocusing/focusing the *base* content for the reasons I already explained.

As I said before, conceptually, we would like to focus the panel's content,
since it will be one being manipulated by the user, and therefore it would make
sense focusing it. But, this does not seem to be happening. Calling
requestFocus() on the panel's content does not cause the base content to loose
focus. Not sure exactly why this is the case, but it seems that the system has a
special way to handle the content focus (for example, assuming that the content
will always be focused when the content is unfocusable, which is confusing, to
say the least).

What appears to be happening here is that calling requestFocus() or clearFocus()
is having no effect, or at least, not the expected effect. And now why are
bypassing this problem by forcing the UI to show/hide manually.

https://codereview.chromium.org/1716653002/diff/260001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ContextualSearchSceneLayer.java
(right):

https://codereview.chromium.org/1716653002/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ContextualSearchSceneLayer.java:51:
nativeCreateContextualSearchLayer(mNativePtr, resourceManager);
On 2016/05/05 17:48:34, mdjones wrote:
> On 2016/05/05 01:40:52, pedrosimonetti wrote:
> > On 2016/05/05 00:46:20, mdjones wrote:
> > > On 2016/05/04 22:59:34, pedrosimonetti wrote:
> > > > Can we only create the layer when it becomes visible?
> > > 
> > > With the follow-up change, that will be the case.
> > 
> > The follow-up change will prevent this update() method from being called.
> > 
> > What I'm asking is, even if this method gets called, why would we create the
> > layer if the panel is not visible?
> > 
> > In other words, can't we just move the if block to right after the early
> return
> > below?
> 
> Unfortunately, no. The scene layer needs to exist in order for the content
tree
> to be added to it (otherwise we just get a white screen). 

I'm asking to not create the *ContextualSearch* layer and you're saying that
can't do that because you need the *Scene* layer?

But, you need the *Scene* layer just because you're adding it to the hierarchy
even when you don't need it?

Anyway, it should still be possible to not create the *ContextualSearch* layer,
even when we are unnecessarily adding *Scene* layer to the hierarchy because the
Content tree will be added to the Content layer, not the *ContextualSearch*
layer.

https://codereview.chromium.org/1716653002/diff/260001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/1716653002/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1522:
public void updateTextSelectionUI(boolean focused) {
On 2016/05/05 17:48:34, mdjones wrote:
> On 2016/05/05 01:40:52, pedrosimonetti wrote:
> > On 2016/05/05 00:46:21, mdjones wrote:
> > > On 2016/05/04 22:59:34, pedrosimonetti wrote:
> > > > Have you talked to aelias@ about this issue, who now owns the selection
> > code?
> > > > 
> > > > The original owner, jdduke@ was pretty clear that he didn't want to
expose
> a
> > > > method to control the visibility of the selection UI. That's why we came
> up
> > > with
> > > > the preserveSelectionOnNextLossOfFocus() API.
> > > > 
> > > > This is a workaround to overcome the fact that
> > > > preserveSelectionOnNextLossOfFocus() API is not be doing what it was
> suppose
> > > to
> > > > and is no better than calling
onAttachedToWindow()/onDetachedFromWindow()
> as
> > > you
> > > > were doing before.
> > > > 
> > > > There should not be a way for consumers to hide/show the selection
> controls
> > at
> > > > will.
> > > > 
> > > > If you can't fix this now I'd prefer calling
> > > > onAttachedToWindow()/onDetachedFromWindow() as you were doing before. At
> > least
> > > > this way we won't be exposing a way for other to control the visibility
of
> > the
> > > > selection UI.
> > > > 
> > > 
> > > I'll chat some more with the content people. I have talked to a couple of
> > people
> > > in the UI office who would rather see this separated.
> > 
> > I'm surprised that we're okay with allowing other to control when to
show/hide
> > the selection controls.
> > 
> > This means that a bug in our code could potentially permanently hide the
> > controls for a particular tab, which would completely break the user
selection
> > flow.
> 
> That's true, but in a similar manner a bug in the overlay panel could hide
part
> or all of the content on the page, breaking the browser experience (which has
> actually happened before).
> 
> This decision will be up to the content folks when I add them to the review.

But you seem to have missed the point. The selection controls are an integral
part of the ContentViewCore and because of that it should be the only one
responsible to handling selection controls, in order to prevent others from
breaking them.

Yes, a bug in ContextualSearch could make the content of the page disappear.
That is exactly why I criticized the current approach of delegating
adding/removing layers to the subclasses, instead of making the OverlayPanel
system handle it (via a OverlayPanelSceneLayer).

Of course bugs can happen at any part of the code, therefore anything can break.
But, when a class/concept delegates the control of one of its primary functions
to external classes/concepts to handle, it's exposing itself to greater risks of
breakages.

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

https://codereview.chromium.org/1716653002/diff/300001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:119:
private boolean mDidClearBasePageFocus;
As I said in my previous comment, we don't control the focus, we simply request
it. But calling requestFocus() doesn't guarantee that it will be focused, so we
shouldn't assume that.

Similarly, calling clearFocus() doesn't guarantee that it will loose focus, so
we shouldn't assume it was cleared.

Instead it should be called mHasRequestedFocus.

https://codereview.chromium.org/1716653002/diff/300001/chrome/browser/android...
File
chrome/browser/android/compositor/scene_layer/contextual_search_scene_layer.cc
(right):

https://codereview.chromium.org/1716653002/diff/300001/chrome/browser/android...
chrome/browser/android/compositor/scene_layer/contextual_search_scene_layer.cc:42:
layer()->AddChild(content_container_);
This line does not belong in here. This method should just create the
ContextualSearch layer, as the name implies.

Adding the content container layer to the scene layer has nothing to do with the
creation of the ContextualSearch layer, and therefore should be moved to
somewhere else, possibly this class' constructor.

[mdjones, 2016-05-05 20:46:02]

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

https://codereview.chromium.org/1716653002/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:293:
protected void clearPanelFocus() {
On 2016/05/05 19:12:15, pedrosimonetti wrote:
> On 2016/05/05 17:48:34, mdjones wrote:
> > On 2016/05/05 01:40:52, pedrosimonetti wrote:
> > > On 2016/05/05 00:46:20, mdjones wrote:
> > > > On 2016/05/04 22:59:34, pedrosimonetti wrote:
> > > > > As I said before, I think we should model this in terms of the *base
> page*
> > > > > focus.
> > > > > 
> > > > > We only need to deal with the focus because we wan't to hide the
> selection
> > > > > controls (pins, context menu, etc). And the way it should work is
simply
> > > > calling
> > > > > preserveSelectionOnNextLossOfFocus() then making the ContentView loose
> > > focus.
> > > > If
> > > > > this is not happening is a bug and should be fixed.
> > > > > 
> > > > > We shouldn't even care if the *panel* ContentView doesn't exist. Even
if
> > > > there's
> > > > > no panel ContentView, this should still work, as long as we unfocus
> (then
> > > > later
> > > > > focus) the *base* page.
> > > > > 
> > > > > I still think this code would be clearer if this method was called
> > > > > focusBaseContent() and the one below unfocusBaseContent().
> > > > 
> > > > So, just to be clear, we don't care if the panel's content ever gains
> focus?
> > > > 
> > > > Either way, I'll still argue that functionality of the OverlayPanel
should
> > be
> > > > with respect to itself rather than something else.
> > > 
> > > Yes, we don't care whether the panel's content gets focused.
> > > 
> > > We only care about focusing/unfocusing the base page's content in order to
> > > hide/show selection controls.
> > 
> > Not according to crbug.com/531743. It actually seems like it will be fairly
> > important (especially for things like keyboard input).
> > 
> > I renamed the methods and removed the logic for focusing the panel content.
> 
> That bug has nothing to do with what we're discussing here. That bug refers to
> supporting common tab features like context menus in the panel. In the future,
> we might need to focus the panel's content in order to support editing input
> fields for example, but there are strong indications that calling
requestFocus()
> or clearFocus() is not doing what we expect.
> 
> Currently, we don't focus the panel's content, and we only care about
> unfocusing/focusing the *base* content for the reasons I already explained.
> 
> As I said before, conceptually, we would like to focus the panel's content,
> since it will be one being manipulated by the user, and therefore it would
make
> sense focusing it. But, this does not seem to be happening. Calling
> requestFocus() on the panel's content does not cause the base content to loose
> focus. Not sure exactly why this is the case, but it seems that the system has
a
> special way to handle the content focus (for example, assuming that the
content
> will always be focused when the content is unfocusable, which is confusing, to
> say the least).
> 
> What appears to be happening here is that calling requestFocus() or
clearFocus()
> is having no effect, or at least, not the expected effect. And now why are
> bypassing this problem by forcing the UI to show/hide manually.

Acknowledged.

https://codereview.chromium.org/1716653002/diff/260001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ContextualSearchSceneLayer.java
(right):

https://codereview.chromium.org/1716653002/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ContextualSearchSceneLayer.java:51:
nativeCreateContextualSearchLayer(mNativePtr, resourceManager);
On 2016/05/05 19:12:15, pedrosimonetti wrote:
> On 2016/05/05 17:48:34, mdjones wrote:
> > On 2016/05/05 01:40:52, pedrosimonetti wrote:
> > > On 2016/05/05 00:46:20, mdjones wrote:
> > > > On 2016/05/04 22:59:34, pedrosimonetti wrote:
> > > > > Can we only create the layer when it becomes visible?
> > > > 
> > > > With the follow-up change, that will be the case.
> > > 
> > > The follow-up change will prevent this update() method from being called.
> > > 
> > > What I'm asking is, even if this method gets called, why would we create
the
> > > layer if the panel is not visible?
> > > 
> > > In other words, can't we just move the if block to right after the early
> > return
> > > below?
> > 
> > Unfortunately, no. The scene layer needs to exist in order for the content
> tree
> > to be added to it (otherwise we just get a white screen). 
> 
> I'm asking to not create the *ContextualSearch* layer and you're saying that
> can't do that because you need the *Scene* layer?
> 
> But, you need the *Scene* layer just because you're adding it to the hierarchy
> even when you don't need it?
> 
> Anyway, it should still be possible to not create the *ContextualSearch*
layer,
> even when we are unnecessarily adding *Scene* layer to the hierarchy because
the
> Content tree will be added to the Content layer, not the *ContextualSearch*
> layer.

Yes, because at this level, the two are indistinguishable. Again, I'm using the
current infrastructure to build this, I address this in follow-up changes. But
just to be clear, we only ever create one of these and having hidden layers has
virtually no affect on rendering speed.

https://codereview.chromium.org/1716653002/diff/260001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/1716653002/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1522:
public void updateTextSelectionUI(boolean focused) {
On 2016/05/05 19:12:16, pedrosimonetti wrote:
> On 2016/05/05 17:48:34, mdjones wrote:
> > On 2016/05/05 01:40:52, pedrosimonetti wrote:
> > > On 2016/05/05 00:46:21, mdjones wrote:
> > > > On 2016/05/04 22:59:34, pedrosimonetti wrote:
> > > > > Have you talked to aelias@ about this issue, who now owns the
selection
> > > code?
> > > > > 
> > > > > The original owner, jdduke@ was pretty clear that he didn't want to
> expose
> > a
> > > > > method to control the visibility of the selection UI. That's why we
came
> > up
> > > > with
> > > > > the preserveSelectionOnNextLossOfFocus() API.
> > > > > 
> > > > > This is a workaround to overcome the fact that
> > > > > preserveSelectionOnNextLossOfFocus() API is not be doing what it was
> > suppose
> > > > to
> > > > > and is no better than calling
> onAttachedToWindow()/onDetachedFromWindow()
> > as
> > > > you
> > > > > were doing before.
> > > > > 
> > > > > There should not be a way for consumers to hide/show the selection
> > controls
> > > at
> > > > > will.
> > > > > 
> > > > > If you can't fix this now I'd prefer calling
> > > > > onAttachedToWindow()/onDetachedFromWindow() as you were doing before.
At
> > > least
> > > > > this way we won't be exposing a way for other to control the
visibility
> of
> > > the
> > > > > selection UI.
> > > > > 
> > > > 
> > > > I'll chat some more with the content people. I have talked to a couple
of
> > > people
> > > > in the UI office who would rather see this separated.
> > > 
> > > I'm surprised that we're okay with allowing other to control when to
> show/hide
> > > the selection controls.
> > > 
> > > This means that a bug in our code could potentially permanently hide the
> > > controls for a particular tab, which would completely break the user
> selection
> > > flow.
> > 
> > That's true, but in a similar manner a bug in the overlay panel could hide
> part
> > or all of the content on the page, breaking the browser experience (which
has
> > actually happened before).
> > 
> > This decision will be up to the content folks when I add them to the review.
> 
> But you seem to have missed the point. The selection controls are an integral
> part of the ContentViewCore and because of that it should be the only one
> responsible to handling selection controls, in order to prevent others from
> breaking them.
> 
> Yes, a bug in ContextualSearch could make the content of the page disappear.
> That is exactly why I criticized the current approach of delegating
> adding/removing layers to the subclasses, instead of making the OverlayPanel
> system handle it (via a OverlayPanelSceneLayer).
> 
> Of course bugs can happen at any part of the code, therefore anything can
break.
> But, when a class/concept delegates the control of one of its primary
functions
> to external classes/concepts to handle, it's exposing itself to greater risks
of
> breakages.

Acknowledged.

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

https://codereview.chromium.org/1716653002/diff/300001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:119:
private boolean mDidClearBasePageFocus;
On 2016/05/05 19:12:16, pedrosimonetti wrote:
> As I said in my previous comment, we don't control the focus, we simply
request
> it. But calling requestFocus() doesn't guarantee that it will be focused, so
we
> shouldn't assume that.
> 
> Similarly, calling clearFocus() doesn't guarantee that it will loose focus, so
> we shouldn't assume it was cleared.
> 
> Instead it should be called mHasRequestedFocus.

That name doesn't suit what it is actually doing. Since we never request focus,
the var shouldn't indicate that. We do, however, clear the base page focus (or
attempt to).

[mdjones, 2016-05-05 20:56:00]

https://codereview.chromium.org/1716653002/diff/300001/chrome/browser/android...
File
chrome/browser/android/compositor/scene_layer/contextual_search_scene_layer.cc
(right):

https://codereview.chromium.org/1716653002/diff/300001/chrome/browser/android...
chrome/browser/android/compositor/scene_layer/contextual_search_scene_layer.cc:42:
layer()->AddChild(content_container_);
On 2016/05/05 19:12:16, pedrosimonetti wrote:
> This line does not belong in here. This method should just create the
> ContextualSearch layer, as the name implies.
> 
> Adding the content container layer to the scene layer has nothing to do with
the
> creation of the ContextualSearch layer, and therefore should be moved to
> somewhere else, possibly this class' constructor.

Done.

[pedro (no code reviews), 2016-05-05 21:53:55]

lgtm

As we discussed offline, please make the suggested change to not create
ContextualSearchLayer and ReaderModeLayer when they are not visible.

The remaining question is what to do with the focus issue and/or whether to
expose an API that allow others to control when the selection controls are
visible, so let's ask people familiar with the ContentViewCore for guidance.

[mdjones, 2016-05-12 17:30:02]

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

[mdjones, 2016-05-12 17:30:04]

+dtrainor PTAL

[David Trainor- moved to gerrit, 2016-05-13 16:14:34]

mostly nits and a few suggestions, but lgtm.

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

https://codereview.chromium.org/1716653002/diff/410001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:118:
/** If the base page text controlls have been cleared. */
controlls -> controls

https://codereview.chromium.org/1716653002/diff/410001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:293:
protected void focusBasePage() {
I know we talked about this, but now that I see it I'm not sure I like this
class mucking with the top level focus of the ContentViewCore :(.  It's just an
OverlayPanel it shouldn't really have any concept of the currently shown Tab or
what it's doing.  I recognize this is a hard problem that's a small component of
this CL, so I'd be fine with you adding this as long as we had a bug/follow up
to at least investigate whether or not we can find a cleaner way to handle focus
and selection through CompositorViewHolder or some other owner.

https://codereview.chromium.org/1716653002/diff/410001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:314:
if (mDidClearTextControls || !isPanelOpened()) return;
Do we have to check if baseContentView is null or not?

https://codereview.chromium.org/1716653002/diff/410001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ContextualSearchSceneLayer.java
(right):

https://codereview.chromium.org/1716653002/diff/410001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ContextualSearchSceneLayer.java:161:
nativeSetContentTree(mNativePtr, contentTree);
Should we check mIsInitialized?

https://codereview.chromium.org/1716653002/diff/410001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ContextualSearchSceneLayer.java:168:
nativeHideTree(mNativePtr);
Same here

https://codereview.chromium.org/1716653002/diff/410001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ReaderModeSceneLayer.java
(right):

https://codereview.chromium.org/1716653002/diff/410001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ReaderModeSceneLayer.java:80:
nativeSetContentTree(mNativePtr, contentTree);
Should we check mIsInitialized for this and below?

https://codereview.chromium.org/1716653002/diff/410001/chrome/browser/android...
File
chrome/browser/android/compositor/scene_layer/contextual_search_scene_layer.cc
(right):

https://codereview.chromium.org/1716653002/diff/410001/chrome/browser/android...
chrome/browser/android/compositor/scene_layer/contextual_search_scene_layer.cc:112:
if (base_page_brightness < 1.f)
I would just add {} since it's a wrapped line below.

https://codereview.chromium.org/1716653002/diff/410001/chrome/browser/android...
File chrome/browser/android/compositor/scene_layer/reader_mode_scene_layer.cc
(right):

https://codereview.chromium.org/1716653002/diff/410001/chrome/browser/android...
chrome/browser/android/compositor/scene_layer/reader_mode_scene_layer.cc:94: if
(base_page_brightness < 1.f)
Same here.  Maybe check the styleguide first?

[pedro (no code reviews), 2016-05-13 17:29:43]

Just to be clear, did you fix the problem that the selection is not becoming
gray when the panel gets opened?

If you did not, please file a bug so we don't loose track of it. This is a
regression and should be fixed before it hits a stable build.

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

On 2016/05/13 17:29:43, pedrosimonetti wrote:
> Just to be clear, did you fix the problem that the selection is not becoming
> gray when the panel gets opened?
> 
> If you did not, please file a bug so we don't loose track of it. This is a
> regression and should be fixed before it hits a stable build.

https://crbug.com/611815

[mdjones, 2016-05-13 20:30:07]

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

https://codereview.chromium.org/1716653002/diff/410001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:118:
/** If the base page text controlls have been cleared. */
On 2016/05/13 16:14:34, David Trainor (OOO until 5-9) wrote:
> controlls -> controls

Done.

https://codereview.chromium.org/1716653002/diff/410001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:293:
protected void focusBasePage() {
On 2016/05/13 16:14:34, David Trainor (OOO until 5-9) wrote:
> I know we talked about this, but now that I see it I'm not sure I like this
> class mucking with the top level focus of the ContentViewCore :(.  It's just
an
> OverlayPanel it shouldn't really have any concept of the currently shown Tab
or
> what it's doing.  I recognize this is a hard problem that's a small component
of
> this CL, so I'd be fine with you adding this as long as we had a bug/follow up
> to at least investigate whether or not we can find a cleaner way to handle
focus
> and selection through CompositorViewHolder or some other owner.

Changed this to only modify the visibility of the text controls. Will address
focusing in a different CL (crbug.com/611850).

https://codereview.chromium.org/1716653002/diff/410001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:314:
if (mDidClearTextControls || !isPanelOpened()) return;
On 2016/05/13 16:14:34, David Trainor (OOO until 5-9) wrote:
> Do we have to check if baseContentView is null or not?

The assumption is that if the panel showed up, a base ContentViewCore is there.
Adding check just in case.

https://codereview.chromium.org/1716653002/diff/410001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ContextualSearchSceneLayer.java
(right):

https://codereview.chromium.org/1716653002/diff/410001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ContextualSearchSceneLayer.java:161:
nativeSetContentTree(mNativePtr, contentTree);
On 2016/05/13 16:14:34, David Trainor (OOO until 5-9) wrote:
> Should we check mIsInitialized?

This is still needed because the content layer needs to be put somewhere in the
tree. If this step is skipped, we will just get a white screen (even when the
panel isn't showing). https://codereview.chromium.org/1936133003/ adds support
for this.

https://codereview.chromium.org/1716653002/diff/410001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ContextualSearchSceneLayer.java:168:
nativeHideTree(mNativePtr);
On 2016/05/13 16:14:34, David Trainor (OOO until 5-9) wrote:
> Same here

This technically wouldn't crash, but it does save a jni call. Done.

https://codereview.chromium.org/1716653002/diff/410001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ReaderModeSceneLayer.java
(right):

https://codereview.chromium.org/1716653002/diff/410001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ReaderModeSceneLayer.java:80:
nativeSetContentTree(mNativePtr, contentTree);
On 2016/05/13 16:14:34, David Trainor (OOO until 5-9) wrote:
> Should we check mIsInitialized for this and below?

Done.

https://codereview.chromium.org/1716653002/diff/410001/chrome/browser/android...
File
chrome/browser/android/compositor/scene_layer/contextual_search_scene_layer.cc
(right):

https://codereview.chromium.org/1716653002/diff/410001/chrome/browser/android...
chrome/browser/android/compositor/scene_layer/contextual_search_scene_layer.cc:112:
if (base_page_brightness < 1.f)
On 2016/05/13 16:14:34, David Trainor (OOO until 5-9) wrote:
> I would just add {} since it's a wrapped line below.

Done.

https://codereview.chromium.org/1716653002/diff/410001/chrome/browser/android...
File chrome/browser/android/compositor/scene_layer/reader_mode_scene_layer.cc
(right):

https://codereview.chromium.org/1716653002/diff/410001/chrome/browser/android...
chrome/browser/android/compositor/scene_layer/reader_mode_scene_layer.cc:94: if
(base_page_brightness < 1.f)
On 2016/05/13 16:14:34, David Trainor (OOO until 5-9) wrote:
> Same here.  Maybe check the styleguide first?

Checked, you are correct.

[mdjones, 2016-05-14 00:11:19]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2016-05-14 00:11:19]

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

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

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

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

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

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

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)

[mdjones, 2016-05-14 00:48:22]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2016-05-14 00:48:22]

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

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

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

[mdjones, 2016-05-14 01:55:00]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2016-05-14 01:55:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
twellington@chromium.org, dtrainor@chromium.org, pedrosimonetti@chromium.org
Link to the patchset: https://codereview.chromium.org/1716653002/#ps470001
(title: "visible for testing added")

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

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

[commit-bot: I haz the power, 2016-05-14 04:09:37]

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

[commit-bot: I haz the power, 2016-05-14 04:09:38]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[mdjones, 2016-05-16 15:46:55]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2016-05-16 15:46:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
twellington@chromium.org, dtrainor@chromium.org, pedrosimonetti@chromium.org
Link to the patchset: https://codereview.chromium.org/1716653002/#ps490001
(title: "Prevent native call in test")

[commit-bot: I haz the power, 2016-05-16 15:47:08]

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

[Donn Denman, 2016-05-16 17:04:22]

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

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

Description was changed from

==========
Implement OverlayPanels as SceneOverlays

Use the SceneOverlay infrastructure to draw on top of Chrome send events
to OverlayPanels rather than using layout switching. 

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

to

==========
Implement OverlayPanels as SceneOverlays

Use the SceneOverlay infrastructure to draw on top of Chrome send events
to OverlayPanels rather than using layout switching. 

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

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

Committed patchset #26 (id:490001)

[commit-bot: I haz the power, 2016-05-16 17:06:53]

Description was changed from

==========
Implement OverlayPanels as SceneOverlays

Use the SceneOverlay infrastructure to draw on top of Chrome send events
to OverlayPanels rather than using layout switching. 

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

to

==========
Implement OverlayPanels as SceneOverlays

Use the SceneOverlay infrastructure to draw on top of Chrome send events
to OverlayPanels rather than using layout switching.

BUG=584340

Committed: https://crrev.com/5a5b9fbe34f23087353c763ec5420deb075194d9
Cr-Commit-Position: refs/heads/master@{#393851}
==========

[commit-bot: I haz the power, 2016-05-16 17:06:55]

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