Separate ContextualSearchPanel from OverlayPanel

Separate ContextualSearchPanel from OverlayPanel

This change separates the two classes into a functional base and
an implementor.

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

[mdjones, 2015-09-30 23:17:18]

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

[mdjones, 2015-09-30 23:17:18]

The ContextualSearchPanel overrides a lot of methods and adding another
"delegate" did not seem appropriate. PTAL

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

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

[pedro (no code reviews), 2015-10-07 00:39:11]

Hey Matt. This is just my first round of comment.

Will take a look now at your most recent patch.

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

https://codereview.chromium.org/1372963005/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:5:
package org.chromium.chrome.browser.compositor.bottombar;
This should live in org.chromium.chrome.browser.compositor.overlaypanel

https://codereview.chromium.org/1372963005/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:254:
public boolean isCoordinateInsideOverlayContentView(float x, float y) {
No method in this class should have Overlay or ContentView in its name, except
for getContentViewCore(), as far as I can tell.

The "overlay" is a quality of the panel, not the content. And the fact that the
content is a "ContentViewCore" is a implementation detail that the panel should
not know and care about.

I suggest renaming methods in this class so that they refer to the content as
simply "Content".

For example, this class should be named to isCoordinateContent()

https://codereview.chromium.org/1372963005/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:262:
public float getOverlayContentViewOffsetX() {
Similarly, I think we could simply things and omit all "Offset" mentions in the
method names of this class.

This method, then, would be named getContentX()

https://codereview.chromium.org/1372963005/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:269:
public float getOverlayContentViewOffsetY() {
getContentY()

https://codereview.chromium.org/1372963005/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:284:
public void onTouchOverlayContentViewAck() {
I think it would sound better if this was name acknowledgeContentTouched()

https://codereview.chromium.org/1372963005/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:295:
public float getOverlayContentViewVerticalScroll() {
getContentVerticalScroll()

https://codereview.chromium.org/1372963005/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelAnimation.java
(right):

https://codereview.chromium.org/1372963005/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelAnimation.java:429:
PrefServiceBridge.getInstance().setContextualSearchState(true);
I think the CSManager should still control whether the feature is enabled, not
the panel. I have a CL to address a regression introduced recently, and this CL
changes code related to the preference state. We should coordinate because that
CL needs to be merged into M47 so we'll need to incorporate those changes here
as well.

https://codereview.chromium.org/1372963005/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java
(right):

https://codereview.chromium.org/1372963005/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:238:
protected boolean isPromoAvailable() {
Nit: Move this to the "Opt Out Promo View" section below, because this section
is intended for abstract methods.

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

https://codereview.chromium.org/1372963005/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/eventfilter/ContextualSearchEventFilter.java:14:
import
org.chromium.chrome.browser.contextualsearch.ContextualSearchManagementDelegate;
Nice being able to remove dependencies!

https://codereview.chromium.org/1372963005/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchStaticEventFilter.java
(right):

https://codereview.chromium.org/1372963005/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchStaticEventFilter.java:72:
&& mSearchPanel.isCoordinateInsideOverlayBar(event.getX() * mPxToDp,
Nit: Let's call the Bar, a ToolBar, instead of SearchBar (seems too specific to
Contextual Search).

OverlayBar seems weird. The Bar is not an overlay.

[mdjones, 2015-10-07 16:55:08]

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

[mdjones, 2015-10-07 16:55:09]

+dtrainor PTAL

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

https://codereview.chromium.org/1372963005/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:5:
package org.chromium.chrome.browser.compositor.bottombar;
On 2015/10/07 00:39:10, pedrosimonetti wrote:
> This should live in org.chromium.chrome.browser.compositor.overlaypanel

I agree, I still have to move this and anything with Overlay* to a different
package. I was going to do all of that in a CL where I don't also have to change
code.

https://codereview.chromium.org/1372963005/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:254:
public boolean isCoordinateInsideOverlayContentView(float x, float y) {
On 2015/10/07 00:39:10, pedrosimonetti wrote:
> No method in this class should have Overlay or ContentView in its name, except
> for getContentViewCore(), as far as I can tell.
> 
> The "overlay" is a quality of the panel, not the content. And the fact that
the
> content is a "ContentViewCore" is a implementation detail that the panel
should
> not know and care about.
> 
> I suggest renaming methods in this class so that they refer to the content as
> simply "Content".
> 
> For example, this class should be named to isCoordinateContent()

Done.

https://codereview.chromium.org/1372963005/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:262:
public float getOverlayContentViewOffsetX() {
On 2015/10/07 00:39:10, pedrosimonetti wrote:
> Similarly, I think we could simply things and omit all "Offset" mentions in
the
> method names of this class.
> 
> This method, then, would be named getContentX()

Done.

https://codereview.chromium.org/1372963005/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:269:
public float getOverlayContentViewOffsetY() {
On 2015/10/07 00:39:10, pedrosimonetti wrote:
> getContentY()

Done.

https://codereview.chromium.org/1372963005/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:284:
public void onTouchOverlayContentViewAck() {
On 2015/10/07 00:39:10, pedrosimonetti wrote:
> I think it would sound better if this was name acknowledgeContentTouched()

Done.

https://codereview.chromium.org/1372963005/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:295:
public float getOverlayContentViewVerticalScroll() {
On 2015/10/07 00:39:10, pedrosimonetti wrote:
> getContentVerticalScroll()

Done.

https://codereview.chromium.org/1372963005/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelAnimation.java
(right):

https://codereview.chromium.org/1372963005/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelAnimation.java:429:
PrefServiceBridge.getInstance().setContextualSearchState(true);
On 2015/10/07 00:39:10, pedrosimonetti wrote:
> I think the CSManager should still control whether the feature is enabled, not
> the panel. I have a CL to address a regression introduced recently, and this
CL
> changes code related to the preference state. We should coordinate because
that
> CL needs to be merged into M47 so we'll need to incorporate those changes here
> as well. 

Once you land the patch, I can figure out something better to do here. The goal
of this was to remove the low-level dependency on the manager. Since we are also
trying to remove promos from the base panel, I thought this could be a temporary
patch.

https://codereview.chromium.org/1372963005/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java
(right):

https://codereview.chromium.org/1372963005/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:238:
protected boolean isPromoAvailable() {
On 2015/10/07 00:39:11, pedrosimonetti wrote:
> Nit: Move this to the "Opt Out Promo View" section below, because this section
> is intended for abstract methods.

Done.

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

https://codereview.chromium.org/1372963005/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/eventfilter/ContextualSearchEventFilter.java:14:
import
org.chromium.chrome.browser.contextualsearch.ContextualSearchManagementDelegate;
On 2015/10/07 00:39:11, pedrosimonetti wrote:
> Nice being able to remove dependencies!

Yeah, this felt pretty good. :)

https://codereview.chromium.org/1372963005/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchStaticEventFilter.java
(right):

https://codereview.chromium.org/1372963005/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchStaticEventFilter.java:72:
&& mSearchPanel.isCoordinateInsideOverlayBar(event.getX() * mPxToDp,
On 2015/10/07 00:39:11, pedrosimonetti wrote:
> Nit: Let's call the Bar, a ToolBar, instead of SearchBar (seems too specific
to
> Contextual Search).
> 
> OverlayBar seems weird. The Bar is not an overlay.

I've changed it to just "Bar" for now. I'm not sure toolbar is the best option
as it has no tools.

[pedro (no code reviews), 2015-10-08 21:23:51]

Hey Matt. Here are some more comments.

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

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:74:
private boolean mHasContentViewBeenTouched;
Nit: mHasContentViewBeenTouched --> mHasContentBeenTouched

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:105:
public boolean isCustomTab() {
The overlay panel should not care about custom tabs. This is a Contextual Search
requirement.

I would vote for deleting the ContextualSearchFeature class, and moving the
implementation of the methods of that class (currently there are only two
methods in there) to the OverlayPanel class, making them return default values
(which will be overridden by ContextualSearchPanel).

This way we can move the isCustomTab() to the ContextualSearchPanel class.

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:183:
} else if (isCoordinateInsideBar(x, y)) {
Add a condition to trigger handleBarClick():

} else if (isCoordinateInsideBar(x, y) && shouldHandleBarClick()) {

// Method should not be public
protected boolean shouldHandleBarClick();

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:197:
if (!handleCompatibilityModeClick()) {
Please note that when in compatibility mode, the panel will never expand, so
it's safe to assume the panel is always peeking (given that we will only receive
click events if the panel is visible).

Therefore, it's safe to put the shouldHandleBarClick() outside this method,
since there's no need to check if the bar is peeking.

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:212:
public boolean handleCompatibilityModeClick() {
This class should not worry about compatibility mode. Instead we should use a
protected shouldHandleBarClick() that can be overridden by
ContextualSearchPanel.

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:349:
// NOTE(pedrosimonetti): exposing superclass method to the interface.
Nit: Remove obsolete comment.

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:366:
public boolean didTouchOverlayContentView() {
Nit: didTouchOverlayContentView --> didTouchContent

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:373:
public boolean isContentViewShowing() {
Nit: isContentViewShowing --> isContentShowing

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:414:
public boolean didLoadAnyUrl() {
Similar to the comment I made in another CL, this should be called
didStartLoadingUrl()

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java
(right):

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:55:
private boolean mDidLoadAnyUrl;
Similar to the comment I made in another CL, this should be called
mDidStartLoadingUrl

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:303:
public boolean didLoadAnyUrl() {
Similar to the comment I made in another CL, this should be called
didStartLoadingUrl()

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

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:76:
setSupportsExpandedState(isFullscreenSizePanel());
See comment on ContextualSearchPanelAnimation.

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:88:
overlayPanelContent.setContentViewClient(new ContentViewClient() {
I think it would be better to have a new OverlayPanelContent constructor that
accepts a ContentViewClient (the default constructor would use a null client).
This way we don't have to expose the setContentViewClient().

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:426:
public boolean shouldAnimatePanelCloseOnPromoteToTab() {
This is not being called anymore. I think I broke this in:
https://chromium.googlesource.com/chromium/src/+/bfd690eeadca2e8f308fb09c225a...

Could you put a TODO assigned to me to fix this issue?

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelAnimation.java
(right):

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelAnimation.java:113:
public void setSupportsExpandedState(boolean isSupported) {
A few comments:

1) We should not expose a method to change whether the expanded state is
supported. Instead we should have a "boolean supportsExpandedState()" that by
default returns false. ContextualSearchPanel should override this method and
return isFullscreenSizePanel().

2) These methods don't belong to the "Animation API" section.

3) Other method should call supportsExpandedState(). As far as I can tell the
method that should call it are:
  ContextualSearchPanelBase.updatePanelForMaximization()
  ContextualSearchPanelBase.findNearestPanelStateFromHeight()
  ContextualSearchPanelBase.setPanelState()
  ContextualSearchPanelBase.animateToProjectedState()

4) This method should be placed in the ContextualSearchPanelBase class, at least
for now.

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelAnimation.java:120:
public boolean getSupportsExpandedState() {
Methods that return boolean values should be prefixed with a logic expression
such as "is", "should"

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java
(right):

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:33:
public abstract class ContextualSearchPanelBase implements
ContextualSearchPromoHost {
Why do we need to make this public?

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelDelegate.java
(right):

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelDelegate.java:46:
int getSearchContentViewWidthPx();
Nit: getSearchContentViewWidthPx --> getContentWidthPx

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelDelegate.java:51:
int getSearchContentViewHeightPx();
Nit: getSearchContentViewHeightPx --> getContentHeightPx

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelDelegate.java:114:
boolean didTouchSearchContentView();
Nit: didTouchSearchContentView --> didTouchContent

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelDelegate.java:171:
boolean isContentViewShowing();
Nit: isContentViewShowing --> isContentShowing

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelDelegate.java:186:
void setWasSearchContentViewSeen();
Nit: setWasSearchContentViewSeen --> setWasContentSeen

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelDelegate.java:192:
void setSearchContentViewVisibility(boolean isVisible);
Please note that I'm removing this method in one of my upcoming CLs. It would be
nice if we could submit the two CLs that fix problems in our code before
submitting this change. Let's talk about this offline.

If we don't submit those changes first, and merge it in this CL, we should
rename this method to setContentVisibility(), although I would prefer not
exposing this method at all and instead exposing a
acknowledgeIntentToPrerender() as I'm doing if my CL.

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

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/ContextualSearchSupportedLayout.java:86:
mSearchPanel.getContentViewCore();
Now I think everything fits in a single line. The same applies for the other
changes in this file.

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/eventfilter/ContextualSearchEventFilter.java
(right):

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/eventfilter/ContextualSearchEventFilter.java:160:
protected float getSearchContentViewVerticalScroll() {
Nit: getSearchContentViewVerticalScroll --> getContentVerticalScroll

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

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:98:
private boolean mDidStartLoadingResolvedSearchRequest;
I should have noticed this before, but I don't understand why we have
mDidStartLoadingResolvedSearchRequest here since the OverlayPanelContent now
keeps track of whether the it started loading the URL.

Shouldn't we use the method exposed by the OverlayPanelContent instead?

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:103:
private boolean mDidLoadAnyUrl;
This seems to be never used.

[mdjones, 2015-10-09 00:05:21]

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

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:74:
private boolean mHasContentViewBeenTouched;
On 2015/10/08 21:23:50, pedrosimonetti wrote:
> Nit: mHasContentViewBeenTouched --> mHasContentBeenTouched

Done.

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:105:
public boolean isCustomTab() {
On 2015/10/08 21:23:50, pedrosimonetti wrote:
> The overlay panel should not care about custom tabs. This is a Contextual
Search
> requirement.
> 
> I would vote for deleting the ContextualSearchFeature class, and moving the
> implementation of the methods of that class (currently there are only two
> methods in there) to the OverlayPanel class, making them return default values
> (which will be overridden by ContextualSearchPanel).
> 
> This way we can move the isCustomTab() to the ContextualSearchPanel class.

I have removed it entirely.

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:183:
} else if (isCoordinateInsideBar(x, y)) {
On 2015/10/08 21:23:50, pedrosimonetti wrote:
> Add a condition to trigger handleBarClick():
> 
> } else if (isCoordinateInsideBar(x, y) && shouldHandleBarClick()) {
> 
> // Method should not be public
> protected boolean shouldHandleBarClick();

Using "interceptBarClick" now.

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:197:
if (!handleCompatibilityModeClick()) {
On 2015/10/08 21:23:49, pedrosimonetti wrote:
> Please note that when in compatibility mode, the panel will never expand, so
> it's safe to assume the panel is always peeking (given that we will only
receive
> click events if the panel is visible).
> 
> Therefore, it's safe to put the shouldHandleBarClick() outside this method,
> since there's no need to check if the bar is peeking.

Done.

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:212:
public boolean handleCompatibilityModeClick() {
On 2015/10/08 21:23:50, pedrosimonetti wrote:
> This class should not worry about compatibility mode. Instead we should use a
> protected shouldHandleBarClick() that can be overridden by
> ContextualSearchPanel.

Renamed to "interceptBarClick".

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:349:
// NOTE(pedrosimonetti): exposing superclass method to the interface.
On 2015/10/08 21:23:50, pedrosimonetti wrote:
> Nit: Remove obsolete comment.

Done.

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:366:
public boolean didTouchOverlayContentView() {
On 2015/10/08 21:23:50, pedrosimonetti wrote:
> Nit: didTouchOverlayContentView --> didTouchContent

Done.

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:373:
public boolean isContentViewShowing() {
On 2015/10/08 21:23:49, pedrosimonetti wrote:
> Nit: isContentViewShowing --> isContentShowing

Done.

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:414:
public boolean didLoadAnyUrl() {
On 2015/10/08 21:23:50, pedrosimonetti wrote:
> Similar to the comment I made in another CL, this should be called
> didStartLoadingUrl()

Done.

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java
(right):

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:55:
private boolean mDidLoadAnyUrl;
On 2015/10/08 21:23:50, pedrosimonetti wrote:
> Similar to the comment I made in another CL, this should be called
> mDidStartLoadingUrl

Done.

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:303:
public boolean didLoadAnyUrl() {
On 2015/10/08 21:23:50, pedrosimonetti wrote:
> Similar to the comment I made in another CL, this should be called
> didStartLoadingUrl()

Done.

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

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:76:
setSupportsExpandedState(isFullscreenSizePanel());
On 2015/10/08 21:23:50, pedrosimonetti wrote:
> See comment on ContextualSearchPanelAnimation.

removed.

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:88:
overlayPanelContent.setContentViewClient(new ContentViewClient() {
On 2015/10/08 21:23:50, pedrosimonetti wrote:
> I think it would be better to have a new OverlayPanelContent constructor that
> accepts a ContentViewClient (the default constructor would use a null client).
> This way we don't have to expose the setContentViewClient().

This will make testing difficult since it relies on having a factory method to
override.

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:426:
public boolean shouldAnimatePanelCloseOnPromoteToTab() {
On 2015/10/08 21:23:50, pedrosimonetti wrote:
> This is not being called anymore. I think I broke this in:
>
https://chromium.googlesource.com/chromium/src/+/bfd690eeadca2e8f308fb09c225a...
> 
> Could you put a TODO assigned to me to fix this issue?

Done.

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelAnimation.java
(right):

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelAnimation.java:113:
public void setSupportsExpandedState(boolean isSupported) {
On 2015/10/08 21:23:50, pedrosimonetti wrote:
> A few comments:
> 
> 1) We should not expose a method to change whether the expanded state is
> supported. Instead we should have a "boolean supportsExpandedState()" that by
> default returns false. ContextualSearchPanel should override this method and
> return isFullscreenSizePanel().
> 
> 2) These methods don't belong to the "Animation API" section.
> 
> 3) Other method should call supportsExpandedState(). As far as I can tell the
> method that should call it are:
>   ContextualSearchPanelBase.updatePanelForMaximization()
>   ContextualSearchPanelBase.findNearestPanelStateFromHeight()
>   ContextualSearchPanelBase.setPanelState()
>   ContextualSearchPanelBase.animateToProjectedState()
> 
> 4) This method should be placed in the ContextualSearchPanelBase class, at
least
> for now.

Done.

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelAnimation.java:120:
public boolean getSupportsExpandedState() {
On 2015/10/08 21:23:50, pedrosimonetti wrote:
> Methods that return boolean values should be prefixed with a logic expression
> such as "is", "should"

removed.

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java
(right):

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:33:
public abstract class ContextualSearchPanelBase implements
ContextualSearchPromoHost {
On 2015/10/08 21:23:50, pedrosimonetti wrote:
> Why do we need to make this public?

Done.

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelDelegate.java
(right):

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelDelegate.java:46:
int getSearchContentViewWidthPx();
On 2015/10/08 21:23:51, pedrosimonetti wrote:
> Nit: getSearchContentViewWidthPx --> getContentWidthPx

As per our discussion, we will remove this interface and call the public panel
methods directly. I added a todo at the top of this file.

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelDelegate.java:51:
int getSearchContentViewHeightPx();
On 2015/10/08 21:23:50, pedrosimonetti wrote:
> Nit: getSearchContentViewHeightPx --> getContentHeightPx

Removing this interface in favor of calling public methods.

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelDelegate.java:114:
boolean didTouchSearchContentView();
On 2015/10/08 21:23:50, pedrosimonetti wrote:
> Nit: didTouchSearchContentView --> didTouchContent

Removing this interface in favor of calling public methods.

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelDelegate.java:171:
boolean isContentViewShowing();
On 2015/10/08 21:23:50, pedrosimonetti wrote:
> Nit: isContentViewShowing --> isContentShowing

Removing this interface in favor of calling public methods.

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelDelegate.java:186:
void setWasSearchContentViewSeen();
On 2015/10/08 21:23:51, pedrosimonetti wrote:
> Nit: setWasSearchContentViewSeen --> setWasContentSeen

Removing this interface in favor of calling public methods.

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelDelegate.java:192:
void setSearchContentViewVisibility(boolean isVisible);
On 2015/10/08 21:23:50, pedrosimonetti wrote:
> Please note that I'm removing this method in one of my upcoming CLs. It would
be
> nice if we could submit the two CLs that fix problems in our code before
> submitting this change. Let's talk about this offline.
> 
> If we don't submit those changes first, and merge it in this CL, we should
> rename this method to setContentVisibility(), although I would prefer not
> exposing this method at all and instead exposing a
> acknowledgeIntentToPrerender() as I'm doing if my CL.

Acknowledged.

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

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/ContextualSearchSupportedLayout.java:86:
mSearchPanel.getContentViewCore();
On 2015/10/08 21:23:51, pedrosimonetti wrote:
> Now I think everything fits in a single line. The same applies for the other
> changes in this file.

Done.

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/eventfilter/ContextualSearchEventFilter.java
(right):

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/eventfilter/ContextualSearchEventFilter.java:160:
protected float getSearchContentViewVerticalScroll() {
On 2015/10/08 21:23:51, pedrosimonetti wrote:
> Nit: getSearchContentViewVerticalScroll --> getContentVerticalScroll

This will be renamed when the panel delegate is removed.

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

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:98:
private boolean mDidStartLoadingResolvedSearchRequest;
On 2015/10/08 21:23:51, pedrosimonetti wrote:
> I should have noticed this before, but I don't understand why we have
> mDidStartLoadingResolvedSearchRequest here since the OverlayPanelContent now
> keeps track of whether the it started loading the URL.
> 
> Shouldn't we use the method exposed by the OverlayPanelContent instead?

I don't think so, resolved requests are a specific feature of contextual search.

https://codereview.chromium.org/1372963005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:103:
private boolean mDidLoadAnyUrl;
On 2015/10/08 21:23:51, pedrosimonetti wrote:
> This seems to be never used.

Done.

[pedro (no code reviews), 2015-10-14 08:03:55]

Here are some more comments.

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

https://codereview.chromium.org/1372963005/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:94:
protected void onClose(StateChangeReason reason) {
FYI: My next CL will change this. Now we will destroy the whole
OverlayPanelContent when the Panel gets closed.

https://codereview.chromium.org/1403813003/

https://codereview.chromium.org/1372963005/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:105:
public int getControlContainerHeightResource() {
Make it protected.

https://codereview.chromium.org/1372963005/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:190:
public void handleBarClick(long time, float x, float y) {
Make it protected.

https://codereview.chromium.org/1372963005/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:204:
public boolean interceptBarClick() {
In order to make the purpose of this class clearer, I would recommend calling it
handleInterceptBarClick(). The reason is that "interceptBarClick" might give the
impression that this method is just checking whether the click should be
intercepted, and not necessarily handling the intercepted click.

I also recommend tweaking the comment to something like:

/**
 * Allows the click on the Bar to be intercepted.
 * @return Whether the click was intercepted by this function.
 */

https://codereview.chromium.org/1372963005/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:204:
public boolean interceptBarClick() {
Make it protected.

https://codereview.chromium.org/1372963005/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:217:
private boolean isCoordinateInsideOverlayPanel(float x, float y) {
Move public method to the top of this section. Move private method to the bottom
of this section.

https://codereview.chromium.org/1372963005/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:295:
public OverlayPanelContent createNewOverlayPanelContent() {
Also bear in mind that I'm changing how we create/destroy the
OverlayPanelContent.

https://codereview.chromium.org/1372963005/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:295:
public OverlayPanelContent createNewOverlayPanelContent() {
Make it protected.

https://codereview.chromium.org/1372963005/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:304:
if (mOverlayPanelContent != null) {
If the OverlayPanelContent is null (we create it dynamically), the delegate will
never be set. Shouldn't we store it in that case, and pass it to the constructor
(inside the createNewOverlayPanelContent() method) when we create a new Content?

Also, do we really need this method?

https://codereview.chromium.org/1372963005/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:313:
if (mOverlayPanelContent != null) {
Same as above. What happens when there's no Content?

Also, do we really need this method?

https://codereview.chromium.org/1372963005/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:321:
public OverlayPanelContent getOverlayPanelContent() {
Make it protected.

https://codereview.chromium.org/1372963005/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:332:
public boolean isPeeking() {
Please create a new code section at the top of this class (below the constructor
section) called "Main API" or "General API" and put the general methods in
there, like peekPanel(), closePanel(), isPeeking(), isExpanded(),
loadUrlInPanel(), didStartLoadingUrl().

Also, I would think that other general method like expandPanel(),
maximizePanel(), and possibly other belong to this class. Any reason why they
are not implemented here?

https://codereview.chromium.org/1372963005/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:357:
public boolean didTouchContent() {
Similarly, please create a new code section, below the new "General API"
section, called "Content" or "Panel Content", and move all methods that are
related to the Content to that place.

Such methods include acknowledgeContentTouched(), didTouchContent(),
isContentShowing(), getContentViewCore(), createNewOverlayPanelContent(),
setOverlayContentDelegate(), setOverlayContentProgressObserver(),
getOverlayPanelContent(), removeLastHistoryEntry(), updateTopControlsState()

https://codereview.chromium.org/1372963005/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:439:
public void setOverlayPanelContent(OverlayPanelContent panelContent) {
FYI: I'm replacing this with the OverlayPanelContentFactory, so tests can have
their own implementation of the Content.

https://codereview.chromium.org/1372963005/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/1372963005/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:80:
OverlayPanelContent overlayPanelContent = super.createNewOverlayPanelContent();
Instead of calling super and then setting custom delegate/observers, why not
simply instantiate a Content with the desired objects (without calling super)?

https://codereview.chromium.org/1372963005/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:435:
// TODO(pedrosimonetti): This is not currently used.
Where is this comment coming from? Is it still valid?

[mdjones, 2015-10-14 17:08:14]

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

https://codereview.chromium.org/1372963005/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:94:
protected void onClose(StateChangeReason reason) {
On 2015/10/14 08:03:54, pedrosimonetti wrote:
> FYI: My next CL will change this. Now we will destroy the whole
> OverlayPanelContent when the Panel gets closed.
> 
> https://codereview.chromium.org/1403813003/

Acknowledged.

https://codereview.chromium.org/1372963005/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:105:
public int getControlContainerHeightResource() {
On 2015/10/14 08:03:54, pedrosimonetti wrote:
> Make it protected.

Done.

https://codereview.chromium.org/1372963005/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:190:
public void handleBarClick(long time, float x, float y) {
On 2015/10/14 08:03:54, pedrosimonetti wrote:
> Make it protected.

Done.

https://codereview.chromium.org/1372963005/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:204:
public boolean interceptBarClick() {
On 2015/10/14 08:03:54, pedrosimonetti wrote:
> Make it protected.

Done.

https://codereview.chromium.org/1372963005/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:204:
public boolean interceptBarClick() {
On 2015/10/14 08:03:54, pedrosimonetti wrote:
> In order to make the purpose of this class clearer, I would recommend calling
it
> handleInterceptBarClick(). The reason is that "interceptBarClick" might give
the
> impression that this method is just checking whether the click should be
> intercepted, and not necessarily handling the intercepted click.
> 
> I also recommend tweaking the comment to something like:
> 
> /**
>  * Allows the click on the Bar to be intercepted.
>  * @return Whether the click was intercepted by this function.
>  */

How about "isInterceptingBarClick"?

https://codereview.chromium.org/1372963005/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:217:
private boolean isCoordinateInsideOverlayPanel(float x, float y) {
On 2015/10/14 08:03:54, pedrosimonetti wrote:
> Move public method to the top of this section. Move private method to the
bottom
> of this section.

Done.

https://codereview.chromium.org/1372963005/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:295:
public OverlayPanelContent createNewOverlayPanelContent() {
On 2015/10/14 08:03:54, pedrosimonetti wrote:
> Make it protected.

Done.

https://codereview.chromium.org/1372963005/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:295:
public OverlayPanelContent createNewOverlayPanelContent() {
On 2015/10/14 08:03:54, pedrosimonetti wrote:
> Also bear in mind that I'm changing how we create/destroy the
> OverlayPanelContent.

Acknowledged.

https://codereview.chromium.org/1372963005/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:304:
if (mOverlayPanelContent != null) {
On 2015/10/14 08:03:54, pedrosimonetti wrote:
> If the OverlayPanelContent is null (we create it dynamically), the delegate
will
> never be set. Shouldn't we store it in that case, and pass it to the
constructor
> (inside the createNewOverlayPanelContent() method) when we create a new
Content?
> 
> Also, do we really need this method?

This method has been removed.

https://codereview.chromium.org/1372963005/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:313:
if (mOverlayPanelContent != null) {
On 2015/10/14 08:03:54, pedrosimonetti wrote:
> Same as above. What happens when there's no Content?
> 
> Also, do we really need this method?

This method has been removed.

https://codereview.chromium.org/1372963005/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:321:
public OverlayPanelContent getOverlayPanelContent() {
On 2015/10/14 08:03:54, pedrosimonetti wrote:
> Make it protected.

Done.

https://codereview.chromium.org/1372963005/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:332:
public boolean isPeeking() {
On 2015/10/14 08:03:54, pedrosimonetti wrote:
> Please create a new code section at the top of this class (below the
constructor
> section) called "Main API" or "General API" and put the general methods in
> there, like peekPanel(), closePanel(), isPeeking(), isExpanded(),
> loadUrlInPanel(), didStartLoadingUrl().
> 
> Also, I would think that other general method like expandPanel(),
> maximizePanel(), and possibly other belong to this class. Any reason why they
> are not implemented here?

Since they have historically been implemented in the animation class and we
don't have a solution for general panel states, I think it's okay if they stay
there for now. This change simply focuses on separating the OverlayPanel from
ContextualSearchPanel.

https://codereview.chromium.org/1372963005/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:357:
public boolean didTouchContent() {
On 2015/10/14 08:03:54, pedrosimonetti wrote:
> Similarly, please create a new code section, below the new "General API"
> section, called "Content" or "Panel Content", and move all methods that are
> related to the Content to that place.
> 
> Such methods include acknowledgeContentTouched(), didTouchContent(),
> isContentShowing(), getContentViewCore(), createNewOverlayPanelContent(),
> setOverlayContentDelegate(), setOverlayContentProgressObserver(),
> getOverlayPanelContent(), removeLastHistoryEntry(), updateTopControlsState()

Done.

https://codereview.chromium.org/1372963005/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:439:
public void setOverlayPanelContent(OverlayPanelContent panelContent) {
On 2015/10/14 08:03:54, pedrosimonetti wrote:
> FYI: I'm replacing this with the OverlayPanelContentFactory, so tests can have
> their own implementation of the Content.

Acknowledged.

https://codereview.chromium.org/1372963005/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/1372963005/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:80:
OverlayPanelContent overlayPanelContent = super.createNewOverlayPanelContent();
On 2015/10/14 08:03:54, pedrosimonetti wrote:
> Instead of calling super and then setting custom delegate/observers, why not
> simply instantiate a Content with the desired objects (without calling super)?

Done.

https://codereview.chromium.org/1372963005/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:435:
// TODO(pedrosimonetti): This is not currently used.
On 2015/10/14 08:03:54, pedrosimonetti wrote:
> Where is this comment coming from? Is it still valid?

You told me to add this comment offline. I can remove it if there is no longer a
bug associated with it.

[pedro (no code reviews), 2015-10-15 06:05:25]

Thanks for the all the changes! I took another look and the code is looking
pretty good already, but I want to take another look once you merge the
regression fixes, before approving the CL.

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

https://codereview.chromium.org/1372963005/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:204:
public boolean interceptBarClick() {
On 2015/10/14 17:08:14, mdjones wrote:
> On 2015/10/14 08:03:54, pedrosimonetti wrote:
> > In order to make the purpose of this class clearer, I would recommend
calling
> it
> > handleInterceptBarClick(). The reason is that "interceptBarClick" might give
> the
> > impression that this method is just checking whether the click should be
> > intercepted, and not necessarily handling the intercepted click.
> > 
> > I also recommend tweaking the comment to something like:
> > 
> > /**
> >  * Allows the click on the Bar to be intercepted.
> >  * @return Whether the click was intercepted by this function.
> >  */
> 
> How about "isInterceptingBarClick"?

The problem with "isInterceptingBarClick" is that it implies the method doesn't
cause a change of state (it simply returns a boolean expression). We wan't a
name that implies that some other action will happen instead of the default
click action.

What about onInterceptBarClick? This would be in accordance with the Android
style:
http://developer.android.com/reference/android/view/ViewGroup.html#onIntercep...

https://codereview.chromium.org/1372963005/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:332:
public boolean isPeeking() {
On 2015/10/14 17:08:14, mdjones wrote:
> On 2015/10/14 08:03:54, pedrosimonetti wrote:
> > Please create a new code section at the top of this class (below the
> constructor
> > section) called "Main API" or "General API" and put the general methods in
> > there, like peekPanel(), closePanel(), isPeeking(), isExpanded(),
> > loadUrlInPanel(), didStartLoadingUrl().
> > 
> > Also, I would think that other general method like expandPanel(),
> > maximizePanel(), and possibly other belong to this class. Any reason why
they
> > are not implemented here?
> 
> Since they have historically been implemented in the animation class and we
> don't have a solution for general panel states, I think it's okay if they stay
> there for now. This change simply focuses on separating the OverlayPanel from
> ContextualSearchPanel.

Okay.

https://codereview.chromium.org/1372963005/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/1372963005/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:435:
// TODO(pedrosimonetti): This is not currently used.
On 2015/10/14 17:08:14, mdjones wrote:
> On 2015/10/14 08:03:54, pedrosimonetti wrote:
> > Where is this comment coming from? Is it still valid?
> 
> You told me to add this comment offline. I can remove it if there is no longer
a
> bug associated with it.

Oh, sorry I forgot about that. Keep the comment then because I need to
investigate it.

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

https://codereview.chromium.org/1372963005/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:74:
private boolean mHasContentBeenTouched;
I now realize this is only necessary for the tab promotion logic which is
specific to Contextual Search (to prevent the promotion bug we discussed about
offline), so it really belongs to the Contextual Search Panel.

https://codereview.chromium.org/1372963005/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:79:
private OverlayPanelContent mOverlayPanelContent;
I recently renamed this property to mContent, which is simpler and since this is
a private property of the Panel, it's still clear its meaning. I think we should
use the sorter name here.

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

https://codereview.chromium.org/1372963005/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:23:
* Controls the Contextual Search Panel.
This is the Overlay Panel Content, not the Contextual Search Panel, as the
comment says.

I think it's worth explaining a little bit more about the responsibility of this
class, which is to abstract the ContentViewCore for use in the OverlayPanel.

https://codereview.chromium.org/1372963005/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:55:
private boolean mDidStartLoadingUrl;
As we've discussed offline, the original mDidLoadAnyUrl had an specific purpose
to prevent a bug that has been fixed, and I think we should remove it soon and
maybe improve the promotion logic that now depends on it.

On the other hand, now we need another boolean to keep track when the content
was previously created (to become visible) but nothing is loaded.

https://codereview.chromium.org/1372963005/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:70:
private OverlayContentDelegate mOverlayContentDelegate;
I addressed Donn's comment to rename this property and I'm calling it
mContentDelegate, for simplicity reasons. Also, "overlay" is a quality of the
Panel, not the Content. We could call it mPanelContentDelegate, but since this
is a private member of the PanelContent, I think it's better to simply call it
mContentDelegate.

https://codereview.chromium.org/1372963005/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:70:
private OverlayContentDelegate mOverlayContentDelegate;
I now realized we are calling the class OverlayContentDelegate, but I think it
should be called OverlayPanelContentDelegate. Since it's a public class I think
it's better to keep the full name of the delegator (OverlayPanelContent) +
Delegate. I'm fine for doing this in a separate CL though. In this case, please
add a TODO at the top of the delegate class.

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

https://codereview.chromium.org/1372963005/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:431:
public boolean didTouchSearchContentView() {
didTouchSearchContentView --> didTouchContent

[mdjones, 2015-10-15 18:54:22]

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

https://codereview.chromium.org/1372963005/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:74:
private boolean mHasContentBeenTouched;
On 2015/10/15 06:05:24, pedrosimonetti wrote:
> I now realize this is only necessary for the tab promotion logic which is
> specific to Contextual Search (to prevent the promotion bug we discussed about
> offline), so it really belongs to the Contextual Search Panel.

Done.

https://codereview.chromium.org/1372963005/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:79:
private OverlayPanelContent mOverlayPanelContent;
On 2015/10/15 06:05:25, pedrosimonetti wrote:
> I recently renamed this property to mContent, which is simpler and since this
is
> a private property of the Panel, it's still clear its meaning. I think we
should
> use the sorter name here.

Your change handled this.

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

https://codereview.chromium.org/1372963005/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:23:
* Controls the Contextual Search Panel.
On 2015/10/15 06:05:25, pedrosimonetti wrote:
> This is the Overlay Panel Content, not the Contextual Search Panel, as the
> comment says.
> 
> I think it's worth explaining a little bit more about the responsibility of
this
> class, which is to abstract the ContentViewCore for use in the OverlayPanel.

Done.

https://codereview.chromium.org/1372963005/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:55:
private boolean mDidStartLoadingUrl;
On 2015/10/15 06:05:25, pedrosimonetti wrote:
> As we've discussed offline, the original mDidLoadAnyUrl had an specific
purpose
> to prevent a bug that has been fixed, and I think we should remove it soon and
> maybe improve the promotion logic that now depends on it.
> 
> On the other hand, now we need another boolean to keep track when the content
> was previously created (to become visible) but nothing is loaded.

Acknowledged.

https://codereview.chromium.org/1372963005/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:70:
private OverlayContentDelegate mOverlayContentDelegate;
On 2015/10/15 06:05:25, pedrosimonetti wrote:
> I now realized we are calling the class OverlayContentDelegate, but I think it
> should be called OverlayPanelContentDelegate. Since it's a public class I
think
> it's better to keep the full name of the delegator (OverlayPanelContent) +
> Delegate. I'm fine for doing this in a separate CL though. In this case,
please
> add a TODO at the top of the delegate class.

Done.

https://codereview.chromium.org/1372963005/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:70:
private OverlayContentDelegate mOverlayContentDelegate;
On 2015/10/15 06:05:25, pedrosimonetti wrote:
> I addressed Donn's comment to rename this property and I'm calling it
> mContentDelegate, for simplicity reasons. Also, "overlay" is a quality of the
> Panel, not the Content. We could call it mPanelContentDelegate, but since this
> is a private member of the PanelContent, I think it's better to simply call it
> mContentDelegate.

Acknowledged.

[David Trainor- moved to gerrit, 2015-10-16 01:14:46]

Whew... lgtm!

[pedro (no code reviews), 2015-10-16 17:42:05]

Found a few minor issues, but the code is looking good to me, provided those
issues are addressed.

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

https://codereview.chromium.org/1372963005/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:97:
if (mContent != null) {
The onClosed() should simply call destroy() and let it destroy the content.

https://codereview.chromium.org/1372963005/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:144:
public void destroy() {
Nit: Move the destroy method below the constructor.

https://codereview.chromium.org/1372963005/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:145:
// It is possible that an OverlayPanelContent was never created for this panel.
Please put this code in a protected destroyOverlayPanelContent() method, and
call it from here, in order to be consistent with how we create/destroy other
Panel components.

https://codereview.chromium.org/1372963005/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:153:
public void setOverlayPanelContent(OverlayPanelContent panelContent) {
This is not needed anymore, right? The tests now use the ContentFactory (this
method has been renamed to setOverlayPanelContentFactory).

https://codereview.chromium.org/1372963005/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:195:

The protected destroyOverlayPanelContent() should be placed here, below
getOverlayPanelContent().

https://codereview.chromium.org/1372963005/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:240:
return mActivity.getControlContainerHeightResource();
Shouldn't we check if mActivity is not null?

https://codereview.chromium.org/1372963005/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:338:
protected boolean interceptBarClick() {
In one of my previous comments, I suggested following Android style for naming
"intercept methods": onInterceptBarClick
https://codereview.chromium.org/1372963005/#msg11

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

https://codereview.chromium.org/1372963005/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:249:
destroy();
Remove the destroy() call here, the super should call it.

https://codereview.chromium.org/1372963005/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:645:
public void setProperty(Property prop, float value) {
Nit: This method is not specific to the SearchBar. Its general purpose is to
update/render animations. Please move this to the top of the "Animation
Handling" section above, since this is the main method that drives all
animations.

https://codereview.chromium.org/1372963005/diff/240001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java
(right):

https://codereview.chromium.org/1372963005/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:1417:
protected boolean isPromoAvailable() {
This method does not exist anymore. The availability of the Promo is controlled
by the Manager, which tells us to make the promo visibility when its available,
and use that cached value instead of checking the availability on every frame.

[mdjones, 2015-10-16 23:33:08]

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

https://codereview.chromium.org/1372963005/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:97:
if (mContent != null) {
On 2015/10/16 17:42:05, pedrosimonetti wrote:
> The onClosed() should simply call destroy() and let it destroy the content.

Done.

https://codereview.chromium.org/1372963005/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:144:
public void destroy() {
On 2015/10/16 17:42:04, pedrosimonetti wrote:
> Nit: Move the destroy method below the constructor.

Done.

https://codereview.chromium.org/1372963005/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:145:
// It is possible that an OverlayPanelContent was never created for this panel.
On 2015/10/16 17:42:04, pedrosimonetti wrote:
> Please put this code in a protected destroyOverlayPanelContent() method, and
> call it from here, in order to be consistent with how we create/destroy other
> Panel components.

Done.

https://codereview.chromium.org/1372963005/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:153:
public void setOverlayPanelContent(OverlayPanelContent panelContent) {
On 2015/10/16 17:42:04, pedrosimonetti wrote:
> This is not needed anymore, right? The tests now use the ContentFactory (this
> method has been renamed to setOverlayPanelContentFactory).

Done.

https://codereview.chromium.org/1372963005/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:195:

On 2015/10/16 17:42:04, pedrosimonetti wrote:
> The protected destroyOverlayPanelContent() should be placed here, below
> getOverlayPanelContent().

Done.

https://codereview.chromium.org/1372963005/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:240:
return mActivity.getControlContainerHeightResource();
On 2015/10/16 17:42:05, pedrosimonetti wrote:
> Shouldn't we check if mActivity is not null?

added assert

https://codereview.chromium.org/1372963005/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:338:
protected boolean interceptBarClick() {
On 2015/10/16 17:42:05, pedrosimonetti wrote:
> In one of my previous comments, I suggested following Android style for naming
> "intercept methods": onInterceptBarClick
> https://codereview.chromium.org/1372963005/#msg11

Done.

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

https://codereview.chromium.org/1372963005/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:249:
destroy();
On 2015/10/16 17:42:05, pedrosimonetti wrote:
> Remove the destroy() call here, the super should call it.

Done.

https://codereview.chromium.org/1372963005/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:645:
public void setProperty(Property prop, float value) {
On 2015/10/16 17:42:05, pedrosimonetti wrote:
> Nit: This method is not specific to the SearchBar. Its general purpose is to
> update/render animations. Please move this to the top of the "Animation
> Handling" section above, since this is the main method that drives all
> animations.

Done.

https://codereview.chromium.org/1372963005/diff/240001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java
(right):

https://codereview.chromium.org/1372963005/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:1417:
protected boolean isPromoAvailable() {
On 2015/10/16 17:42:05, pedrosimonetti wrote:
> This method does not exist anymore. The availability of the Promo is
controlled
> by the Manager, which tells us to make the promo visibility when its
available,
> and use that cached value instead of checking the availability on every frame.

removed

[pedro (no code reviews), 2015-10-16 23:41:28]

lgtm

Thanks for the changes! Code is looking pretty good.

[mdjones, 2015-10-16 23:44:24]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2015-10-16 23:44:24]

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

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

The CQ bit was unchecked by mdjones@chromium.org

[mdjones, 2015-10-16 23:44:40]

The CQ bit was checked by mdjones@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-16 23:45:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1372963005/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1372963005/280001

[commit-bot: I haz the power, 2015-10-17 00:25:55]

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

[commit-bot: I haz the power, 2015-10-17 00:25:56]

Dry run: 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-10-19 16:20:47]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2015-10-19 16:20:47]

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/1372963005/#ps300001
(title: "fix redundant null check")

[commit-bot: I haz the power, 2015-10-19 16:21:08]

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

[commit-bot: I haz the power, 2015-10-19 16:59:33]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2015-10-19 17:00:16]

Patchset 16 (id:??) landed as
https://crrev.com/9e956b03f1c7ca5e7734c1d781f239155fcaa354
Cr-Commit-Position: refs/heads/master@{#354791}