OverlayPanel Manager

Add OverlayPanel Manager

This change adds a manager for OverlayPanel objects. Since
only one panel can be shown on the screen at a time, this
class will decide which should show when multiple are
attempting to.

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

[mdjones, 2015-10-26 22:04:22]

Description was changed from

==========
OverlayPanel Manager

WIP

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

to

==========
Add OverlayPanel Manager

This change adds a manager for OverlayPanel objects. Since
only one panel can be shown on the screen at a time, this
class will decide which should show when multiple are attempting to.

BUG=521773
==========

[mdjones, 2015-10-26 22:04:48]

Description was changed from

==========
Add OverlayPanel Manager

This change adds a manager for OverlayPanel objects. Since
only one panel can be shown on the screen at a time, this
class will decide which should show when multiple are attempting to.

BUG=521773
==========

to

==========
Add OverlayPanel Manager

This change adds a manager for OverlayPanel objects. Since
only one panel can be shown on the screen at a time, this
class will decide which should show when multiple are 
attempting to.

BUG=521773
==========

[mdjones, 2015-10-28 15:52:36]

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

[mdjones, 2015-10-28 15:52:37]

Some locations that use panelManager.getActivePanel() do no have a null check
around them because the code should never reach those points if null. PTAL

[pedro (no code reviews), 2015-11-03 19:26:43]

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

https://codereview.chromium.org/1417793003/diff/150001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:59:
REVIVE_SUPPRESSED;
The name REVIVE_SUPPRESSED is kinda weird. Also, I think we need two new
reasons: SUPPRESS (reason for closing) and UNSUPPRESS (reason for reopening).

https://codereview.chromium.org/1417793003/diff/150001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:108:
if (panelManager != null) {
I would think we would register the panel once it's fully constructed. So, I'd
move this below to the end of the constructor.

https://codereview.chromium.org/1417793003/diff/150001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:108:
if (panelManager != null) {
We shouldn't check if the panelManager exists. We should assume it's always
there and let is crash if null if passed to the constructor. Without the manager
the panel will not work (no event will be dispatched, therefore the Panel won't
be interactive), so it doesn't make sense trying to support that case.

https://codereview.chromium.org/1417793003/diff/150001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:124:
if (mPanelManager != null) {
No need to check if manager is null. Also, I would think we would notify the
manager after the panel is completely closed/destroyed because the notification
might trigger another panel to be displayed.

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

https://codereview.chromium.org/1417793003/diff/170001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:59:
REVIVE_SUPPRESSED;
See comments on previous patch.

https://codereview.chromium.org/1417793003/diff/170001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelManager.java
(right):

https://codereview.chromium.org/1417793003/diff/170001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelManager.java:60:
public void requestPanelShow(OverlayPanel panel, StateChangeReason reason) {
I find the logic of requestPanelShow() and notifyPanelClosed() very confusing.
As we discussed offline, I'll try to come up a simple change to make this easier
to follow. Meanwhile, I'll gonna post my remaining comments.

https://codereview.chromium.org/1417793003/diff/170001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelManager.java:69:
mActivePanel.closePanel(reason, false);
I don't like that calling closePanel() will affect the code in different ways
depending who's calling it.

https://codereview.chromium.org/1417793003/diff/170001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelManager.java:69:
mActivePanel.closePanel(reason, false);
The use of canBeSuppresed is confusing, because we're suppressing (calling
closePanel) without respecting it.

https://codereview.chromium.org/1417793003/diff/170001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelManager.java:166:
mPanelSet.add(panel);
Check that Panel is not not null?

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

https://codereview.chromium.org/1417793003/diff/170001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/ContextualSearchSupportedLayout.java:135:
boolean panelAnimating = panel != null && panel.onUpdateAnimation(time,
jumpToEnd);
I think the mPanelManager should be responsible for dealing with Panels, for the
most part. I think we should include a onUpdateAnimation() method in the manager
and let it update the active panel.

https://codereview.chromium.org/1417793003/diff/170001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/ContextualSearchSupportedLayout.java:143:
return panel.getSceneLayer();
Similarly, I think we should delegate that to mPanelManager.getSceneLayer()

https://codereview.chromium.org/1417793003/diff/170001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/ContextualSearchSupportedLayout.java:153:
if (panel == null || !panel.isShowing()) return;
Similarly I think we should have:

if (!mPanelManager.shouldUpdateSceneLayer()) return;

https://codereview.chromium.org/1417793003/diff/170001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/ContextualSearchSupportedLayout.java:155:
panel.updateSceneLayer(resourceManager);
Similarly, I think we should delegate that to mPanelManager.updateSceneLayer()

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

https://codereview.chromium.org/1417793003/diff/170001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerDocument.java:318:
if (mContextualSearchDelegate == null) return;
Remove mContextualSearchDelegate reference from here (see comment below).

https://codereview.chromium.org/1417793003/diff/170001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerDocument.java:327:
if (!mContextualSearchDelegate.isRunningInCompatibilityMode()) {
This condition should be:

if (mOverlayPanelManager.supportsContextualSearchLayout()) {
  ...
}

That is, the manager should control that, and the Panel should also have a
method with the same name, returning true by default, which CS Panel will
override to return !mCSManagerDelegate.isRunningInCompatibilityMode()

https://codereview.chromium.org/1417793003/diff/170001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerDocument.java:331:
mOverlayPanelManager.getActivePanel().handleClick(time, x, y);
Add a mOverlayPanelManager.handleClick(time, x, y) and let it forward to the
active panel (if not null).

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

https://codereview.chromium.org/1417793003/diff/170001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/StaticLayout.java:307:
if (panel != null && panel.isShowing()) {
Use mPanelManager.shouldUpdateSceneLayer() (that I suggested creating in another
comment).

https://codereview.chromium.org/1417793003/diff/170001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java
(right):

https://codereview.chromium.org/1417793003/diff/170001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:521:
mSearchPanel.requestPanelShow(stateChangeReason);
This should be a few lines below (see peekPanel() call).

[mdjones, 2015-11-03 23:04:11]

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

[mdjones, 2015-11-03 23:04:12]

+dtrainor PTAL

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

https://codereview.chromium.org/1417793003/diff/150001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:59:
REVIVE_SUPPRESSED;
On 2015/11/03 19:26:43, pedrosimonetti wrote:
> The name REVIVE_SUPPRESSED is kinda weird. Also, I think we need two new
> reasons: SUPPRESS (reason for closing) and UNSUPPRESS (reason for reopening).

Done.

https://codereview.chromium.org/1417793003/diff/150001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:108:
if (panelManager != null) {
On 2015/11/03 19:26:43, pedrosimonetti wrote:
> We shouldn't check if the panelManager exists. We should assume it's always
> there and let is crash if null if passed to the constructor. Without the
manager
> the panel will not work (no event will be dispatched, therefore the Panel
won't
> be interactive), so it doesn't make sense trying to support that case.

Done.

https://codereview.chromium.org/1417793003/diff/150001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:108:
if (panelManager != null) {
On 2015/11/03 19:26:43, pedrosimonetti wrote:
> I would think we would register the panel once it's fully constructed. So, I'd
> move this below to the end of the constructor.

Done.

https://codereview.chromium.org/1417793003/diff/150001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:124:
if (mPanelManager != null) {
On 2015/11/03 19:26:43, pedrosimonetti wrote:
> No need to check if manager is null. Also, I would think we would notify the
> manager after the panel is completely closed/destroyed because the
notification
> might trigger another panel to be displayed.

Done.

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

https://codereview.chromium.org/1417793003/diff/170001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:59:
REVIVE_SUPPRESSED;
On 2015/11/03 19:26:43, pedrosimonetti wrote:
> See comments on previous patch.

Acknowledged.

https://codereview.chromium.org/1417793003/diff/170001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelManager.java
(right):

https://codereview.chromium.org/1417793003/diff/170001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelManager.java:60:
public void requestPanelShow(OverlayPanel panel, StateChangeReason reason) {
On 2015/11/03 19:26:43, pedrosimonetti wrote:
> I find the logic of requestPanelShow() and notifyPanelClosed() very confusing.
> As we discussed offline, I'll try to come up a simple change to make this
easier
> to follow. Meanwhile, I'll gonna post my remaining comments.

This will be made more clear with closing being managed by this class.

https://codereview.chromium.org/1417793003/diff/170001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelManager.java:69:
mActivePanel.closePanel(reason, false);
On 2015/11/03 19:26:43, pedrosimonetti wrote:
> I don't like that calling closePanel() will affect the code in different ways
> depending who's calling it.

This instance is the only case where we do something slightly different. It will
only stay this way until close is completely managed by this class and we have a
more complete API for animation.

https://codereview.chromium.org/1417793003/diff/170001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelManager.java:166:
mPanelSet.add(panel);
On 2015/11/03 19:26:43, pedrosimonetti wrote:
> Check that Panel is not not null?

The only object adding panels is the OverlayPanel object itself (in the
constructor) so this should never be null.

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

https://codereview.chromium.org/1417793003/diff/170001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/ContextualSearchSupportedLayout.java:135:
boolean panelAnimating = panel != null && panel.onUpdateAnimation(time,
jumpToEnd);
On 2015/11/03 19:26:43, pedrosimonetti wrote:
> I think the mPanelManager should be responsible for dealing with Panels, for
the
> most part. I think we should include a onUpdateAnimation() method in the
manager
> and let it update the active panel.

As per our discussion, I don't think delegating these methods to the manager
adds much of a benefit and starts to get into a situation like we had with the
ContextualSearchPanelDelegate.

https://codereview.chromium.org/1417793003/diff/170001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/ContextualSearchSupportedLayout.java:143:
return panel.getSceneLayer();
On 2015/11/03 19:26:43, pedrosimonetti wrote:
> Similarly, I think we should delegate that to mPanelManager.getSceneLayer()

See above.

https://codereview.chromium.org/1417793003/diff/170001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/ContextualSearchSupportedLayout.java:153:
if (panel == null || !panel.isShowing()) return;
On 2015/11/03 19:26:43, pedrosimonetti wrote:
> Similarly I think we should have:
> 
> if (!mPanelManager.shouldUpdateSceneLayer()) return;

See above.

https://codereview.chromium.org/1417793003/diff/170001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/ContextualSearchSupportedLayout.java:155:
panel.updateSceneLayer(resourceManager);
On 2015/11/03 19:26:43, pedrosimonetti wrote:
> Similarly, I think we should delegate that to mPanelManager.updateSceneLayer()

See above.

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

https://codereview.chromium.org/1417793003/diff/170001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerDocument.java:318:
if (mContextualSearchDelegate == null) return;
On 2015/11/03 19:26:43, pedrosimonetti wrote:
> Remove mContextualSearchDelegate reference from here (see comment below).

This is a different CL:
https://codereview.chromium.org/1432613002/

https://codereview.chromium.org/1417793003/diff/170001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerDocument.java:327:
if (!mContextualSearchDelegate.isRunningInCompatibilityMode()) {
On 2015/11/03 19:26:43, pedrosimonetti wrote:
> This condition should be:
> 
> if (mOverlayPanelManager.supportsContextualSearchLayout()) {
>   ...
> }
> 
> That is, the manager should control that, and the Panel should also have a
> method with the same name, returning true by default, which CS Panel will
> override to return !mCSManagerDelegate.isRunningInCompatibilityMode()

Different CL:
https://codereview.chromium.org/1432613002/

https://codereview.chromium.org/1417793003/diff/170001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerDocument.java:331:
mOverlayPanelManager.getActivePanel().handleClick(time, x, y);
On 2015/11/03 19:26:43, pedrosimonetti wrote:
> Add a mOverlayPanelManager.handleClick(time, x, y) and let it forward to the
> active panel (if not null).

See comment in ContextualSearchSupportedLayout.java

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

https://codereview.chromium.org/1417793003/diff/170001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/StaticLayout.java:307:
if (panel != null && panel.isShowing()) {
On 2015/11/03 19:26:43, pedrosimonetti wrote:
> Use mPanelManager.shouldUpdateSceneLayer() (that I suggested creating in
another
> comment).

See comments in ContextualSearchSupportedLayout.java

https://codereview.chromium.org/1417793003/diff/170001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java
(right):

https://codereview.chromium.org/1417793003/diff/170001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:521:
mSearchPanel.requestPanelShow(stateChangeReason);
On 2015/11/03 19:26:43, pedrosimonetti wrote:
> This should be a few lines below (see peekPanel() call).

This was the result of a bad rebase. Fixed.

[David Trainor- moved to gerrit, 2015-11-09 23:56:47]

nits!  lgtm once you fix the nits and that if block!

https://chromiumcodereview.appspot.com/1417793003/diff/190001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelManager.java
(right):

https://chromiumcodereview.appspot.com/1417793003/diff/190001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelManager.java:33:
private Set<OverlayPanel> mPanelSet;
final?

https://chromiumcodereview.appspot.com/1417793003/diff/190001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelManager.java:61:
if (panel != null && panel != mActivePanel) {
Flip this around and just return early?

https://chromiumcodereview.appspot.com/1417793003/diff/190001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelManager.java:63:
if (mActivePanel != null
This if and the if/else below are confusing me :'(.  Can we clean these up into
one if/else if/else block or something?

https://chromiumcodereview.appspot.com/1417793003/diff/190001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelManager.java:104:
if (mSuppressedPanel != null) {
What if the panel was neither active nor suppressed at the time of closing?

https://chromiumcodereview.appspot.com/1417793003/diff/190001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelManager.java:123:
for (OverlayPanel p : mPanelSet) {
Should we clear our mActivePanel and mSuppressedPanel references?  And drop all
panels from mPanelSet?

https://chromiumcodereview.appspot.com/1417793003/diff/190001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java
(right):

https://chromiumcodereview.appspot.com/1417793003/diff/190001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:73:
public ContextualSearchPanel(Context context, LayoutUpdateHost updateHost,
Fix javadoc

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

https://chromiumcodereview.appspot.com/1417793003/diff/190001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/ContextualSearchSupportedLayout.java:54:
public ContextualSearchSupportedLayout(Context context, LayoutUpdateHost
updateHost,
javadoc!  same with other added variables for other methods.

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

https://chromiumcodereview.appspot.com/1417793003/diff/190001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/StaticLayout.java:73:
EventFilter eventFilter, OverlayPanelManager panelManager) {
jd!

[pedro (no code reviews), 2015-11-10 01:14:21]

https://codereview.chromium.org/1417793003/diff/190001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelManager.java
(right):

https://codereview.chromium.org/1417793003/diff/190001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelManager.java:63:
if (mActivePanel != null
On 2015/11/09 23:56:47, David Trainor wrote:
> This if and the if/else below are confusing me :'(.  Can we clean these up
into
> one if/else if/else block or something?

I agree that this is confusing. I have a suggestion to make it clearer.

Add suppress() and unsuppress() methods to the Panel, and respective
canBeSuppressed() and canBeUnsuppressed() methods.

The suppress() method would close the Panel without calling notifyPanelClosed().
Part of the confusion is that notifyPanelClosed() is being called both when the
user closed the Panel (and when this happens we need to restore/unsuppress the
Panel that was hidden), but also when the PanelManager suppresses a Panel (by
calling closePanel()).

This would make the logic of what is suppressed and was is active much simpler.

I also vote for rewriting the logic because the code is checking similar things
twice (there's an active Panel, new Panel has higher priority, can be
suppressed) in slightly different ways, which makes this code hard to follow.

[mdjones, 2015-11-10 01:30:06]

https://codereview.chromium.org/1417793003/diff/190001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelManager.java
(right):

https://codereview.chromium.org/1417793003/diff/190001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelManager.java:33:
private Set<OverlayPanel> mPanelSet;
On 2015/11/09 23:56:47, David Trainor wrote:
> final?

Done.

https://codereview.chromium.org/1417793003/diff/190001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelManager.java:61:
if (panel != null && panel != mActivePanel) {
On 2015/11/09 23:56:47, David Trainor wrote:
> Flip this around and just return early?

Done.

https://codereview.chromium.org/1417793003/diff/190001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelManager.java:63:
if (mActivePanel != null
On 2015/11/09 23:56:47, David Trainor wrote:
> This if and the if/else below are confusing me :'(.  Can we clean these up
into
> one if/else if/else block or something?

I reworked this into a single if/else chain. I think it's better than this
version.

https://codereview.chromium.org/1417793003/diff/190001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelManager.java:104:
if (mSuppressedPanel != null) {
On 2015/11/09 23:56:47, David Trainor wrote:
> What if the panel was neither active nor suppressed at the time of closing?

This should never be the case, but if it did happen, there is no state that
needs to be changed up here. The panel will have closed and this notification
function will have no effect.

https://codereview.chromium.org/1417793003/diff/190001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelManager.java:123:
for (OverlayPanel p : mPanelSet) {
On 2015/11/09 23:56:47, David Trainor wrote:
> Should we clear our mActivePanel and mSuppressedPanel references?  And drop
all
> panels from mPanelSet?

Done.

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

https://codereview.chromium.org/1417793003/diff/190001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:73:
public ContextualSearchPanel(Context context, LayoutUpdateHost updateHost,
On 2015/11/09 23:56:47, David Trainor wrote:
> Fix javadoc

Done.

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

https://codereview.chromium.org/1417793003/diff/190001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/ContextualSearchSupportedLayout.java:54:
public ContextualSearchSupportedLayout(Context context, LayoutUpdateHost
updateHost,
On 2015/11/09 23:56:47, David Trainor wrote:
> javadoc!  same with other added variables for other methods.

Done.

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

https://codereview.chromium.org/1417793003/diff/190001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/StaticLayout.java:73:
EventFilter eventFilter, OverlayPanelManager panelManager) {
On 2015/11/09 23:56:47, David Trainor wrote:
> jd!

Done.

[mdjones, 2015-11-10 01:33:11]

https://codereview.chromium.org/1417793003/diff/190001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelManager.java
(right):

https://codereview.chromium.org/1417793003/diff/190001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelManager.java:63:
if (mActivePanel != null
On 2015/11/10 01:14:21, pedrosimonetti wrote:
> On 2015/11/09 23:56:47, David Trainor wrote:
> > This if and the if/else below are confusing me :'(.  Can we clean these up
> into
> > one if/else if/else block or something?
> 
> I agree that this is confusing. I have a suggestion to make it clearer.
> 
> Add suppress() and unsuppress() methods to the Panel, and respective
> canBeSuppressed() and canBeUnsuppressed() methods.
> 
> The suppress() method would close the Panel without calling
notifyPanelClosed().
> Part of the confusion is that notifyPanelClosed() is being called both when
the
> user closed the Panel (and when this happens we need to restore/unsuppress the
> Panel that was hidden), but also when the PanelManager suppresses a Panel (by
> calling closePanel()).
> 
> This would make the logic of what is suppressed and was is active much
simpler.
> 
> I also vote for rewriting the logic because the code is checking similar
things
> twice (there's an active Panel, new Panel has higher priority, can be
> suppressed) in slightly different ways, which makes this code hard to follow. 

PTAL at the new version.

[pedro (no code reviews), 2015-11-10 02:48:34]

https://codereview.chromium.org/1417793003/diff/210001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelManager.java
(right):

https://codereview.chromium.org/1417793003/diff/210001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelManager.java:76:
OverlayPanel toBeSuppressed = mActivePanel;
I think that this is better, but still confusing. If you don't want to add a
suppress() you could still make the code clearer by differentiating when a close
happens due to a suppression or when it happens due to a user interaction. For
that, you simply need to pass the right reason to closePanel(), and then passing
the reason to notifyPanelClosed().

Ideally the code inside this IF block would simply be:

mActivePanel.closePanel(SUPPRESS, true);
mPendingPanel = panel;
mPendingReason = reason;

See comment below in notifyPanelClosed().

https://codereview.chromium.org/1417793003/diff/210001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelManager.java:77:
mActivePanel.closePanel(reason, false);
Shouldn't the suppression be animated? Why are we passing false here?

https://codereview.chromium.org/1417793003/diff/210001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelManager.java:77:
mActivePanel.closePanel(reason, false);
We are passing the show reason of one panel as the closing reason of another.
The reason when suppressing a Panel should be StateChangeReason.SUPPRESS.

https://codereview.chromium.org/1417793003/diff/210001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelManager.java:97:
public void notifyPanelClosed(OverlayPanel panel) {
If you pass the reason to notifyPanelClosed() then you can separate things much
clearly, like:

if (reason == SUPPRESS) {
  mSuppressedPanel = mActivePanel;
  if (mPendingPanel != null) {
    mActivePanel = mPendingPanel;
    mActivePanel.peekPanel(mPendingReason);
    mPendingPanel = null;
    mPendingReason = null;
  }
} else {
  assert panel == mActivePanel;
  mActivePanel = null;
  if (mSuppressedPanel != null) {
    mActivePanel = mSuppressedPanel;
    mActivePanel.peekPanel(UNSUPPRESS);
    mSuppressedPanel = null;
  }
}

[mdjones, 2015-11-10 04:24:45]

https://codereview.chromium.org/1417793003/diff/210001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelManager.java
(right):

https://codereview.chromium.org/1417793003/diff/210001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelManager.java:76:
OverlayPanel toBeSuppressed = mActivePanel;
On 2015/11/10 02:48:34, pedrosimonetti wrote:
> I think that this is better, but still confusing. If you don't want to add a
> suppress() you could still make the code clearer by differentiating when a
close
> happens due to a suppression or when it happens due to a user interaction. For
> that, you simply need to pass the right reason to closePanel(), and then
passing
> the reason to notifyPanelClosed().
> 
> Ideally the code inside this IF block would simply be:
> 
> mActivePanel.closePanel(SUPPRESS, true);
> mPendingPanel = panel;
> mPendingReason = reason;
> 
> See comment below in notifyPanelClosed().

Done.

https://codereview.chromium.org/1417793003/diff/210001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelManager.java:77:
mActivePanel.closePanel(reason, false);
On 2015/11/10 02:48:34, pedrosimonetti wrote:
> We are passing the show reason of one panel as the closing reason of another.
> The reason when suppressing a Panel should be StateChangeReason.SUPPRESS.

Done.

https://codereview.chromium.org/1417793003/diff/210001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelManager.java:77:
mActivePanel.closePanel(reason, false);
On 2015/11/10 02:48:34, pedrosimonetti wrote:
> Shouldn't the suppression be animated? Why are we passing false here?

We discussed offline that we wanted better animation infrastructure before this
was implemented; it is possible to get into some weird states while waiting for
animation to finish. This implementation also mimics the current reader/cs
behavior.

https://codereview.chromium.org/1417793003/diff/210001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelManager.java:97:
public void notifyPanelClosed(OverlayPanel panel) {
On 2015/11/10 02:48:34, pedrosimonetti wrote:
> If you pass the reason to notifyPanelClosed() then you can separate things
much
> clearly, like:
> 
> if (reason == SUPPRESS) {
>   mSuppressedPanel = mActivePanel;
>   if (mPendingPanel != null) {
>     mActivePanel = mPendingPanel;
>     mActivePanel.peekPanel(mPendingReason);
>     mPendingPanel = null;
>     mPendingReason = null;
>   }
> } else {
>   assert panel == mActivePanel;
>   mActivePanel = null;
>   if (mSuppressedPanel != null) {
>     mActivePanel = mSuppressedPanel;
>     mActivePanel.peekPanel(UNSUPPRESS);
>     mSuppressedPanel = null;
>   }
> }

Done.

[pedro (no code reviews), 2015-11-10 22:28:58]

lgtm

Thanks for all the changes. I have a single comment, but otherwise this change
looks good to me.

https://codereview.chromium.org/1417793003/diff/230001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelManager.java
(right):

https://codereview.chromium.org/1417793003/diff/230001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelManager.java:107:
if (mActivePanel == panel && reason == StateChangeReason.SUPPRESS) {
Thanks for the changes. Would it be possible to change this logic to have only
one outer if-else-block? Like:

if (reason == SUPPRESSED) {
  // do everything related to suppressed
} else {
  // do everything related to normal closing
}

It would be much easier to reason about the code if it was structured like that.

[mdjones, 2015-11-10 23:19:07]

https://codereview.chromium.org/1417793003/diff/230001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelManager.java
(right):

https://codereview.chromium.org/1417793003/diff/230001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelManager.java:107:
if (mActivePanel == panel && reason == StateChangeReason.SUPPRESS) {
On 2015/11/10 22:28:57, pedrosimonetti wrote:
> Thanks for the changes. Would it be possible to change this logic to have only
> one outer if-else-block? Like:
> 
> if (reason == SUPPRESSED) {
>   // do everything related to suppressed
> } else {
>   // do everything related to normal closing
> }
> 
> It would be much easier to reason about the code if it was structured like
that.

Done.

[mdjones, 2015-11-10 23:19:42]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2015-11-10 23:19:42]

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

[commit-bot: I haz the power, 2015-11-10 23:20:35]

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

[commit-bot: I haz the power, 2015-11-11 00:02:35]

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

[commit-bot: I haz the power, 2015-11-11 00:02:36]

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

[mdjones, 2015-11-11 00:22:50]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2015-11-11 00:22:50]

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

[commit-bot: I haz the power, 2015-11-11 00:25:03]

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

[commit-bot: I haz the power, 2015-11-11 01:01:13]

Committed patchset #15 (id:270001)

[commit-bot: I haz the power, 2015-11-11 01:01:50]

Patchset 15 (id:??) landed as
https://crrev.com/076bca4a15979b44c4d5410638492fb40ca2cadc
Cr-Commit-Position: refs/heads/master@{#358988}