Overlay content is its own class

Overlay content is its own class

An overlay panel now owns an OverlayPanelContent object that controls
access to a ContentViewCore and allows observation of loading related
events.

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

[mdjones, 2015-09-09 01:12:22]

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

[mdjones, 2015-09-09 01:12:23]

PTAL. One method that seems out of place is getNewInterceptNavigationDelegate in
OverlayContentObserver.java. I'll find a better place for that, otherwise I
think this is ok to start reviewing.

[pedro (no code reviews), 2015-09-10 17:58:39]

Hey Matt, I took a first look at your change and here are some comments.

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

https://codereview.chromium.org/1326643003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayContentObserver.java:44:
public void onVisibilityChanged(boolean isVisible) {}
Nit: Since you created a section for ContentView below, I'd move this to that
section. Also, all method names start with onContentView, so I'd call this
onContentViewVisibilityChanged.

https://codereview.chromium.org/1326643003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayContentObserver.java:49:
public void onContentViewSeen() {}
Nit: Since you created a section for ContentView below, I'd move this to that
section.

https://codereview.chromium.org/1326643003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayContentObserver.java:54:
public InterceptNavigationDelegate getNewInterceptNavigationDelegate() {
As you said, this seems out of place for the observer pattern. One option would
be renaming this interface to OverlayContentDelegate, then it would be okay
having method other than event handlers.

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

https://codereview.chromium.org/1326643003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:86:
*/
@param progressObserver

https://codereview.chromium.org/1326643003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:129:
public void createNewContentView() {
JavaDoc here and elsewhere

https://codereview.chromium.org/1326643003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:210:
destroyContentView();
No need to destroy here, createNewContentView will do just that.

https://codereview.chromium.org/1326643003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:226:
mContentViewCore.getWebContents().updateTopControlsState(enableHiding,
enableShowing,
Include:
if (mContentViewCore != null && mContentViewCore.getWebContents() != null) {

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

https://codereview.chromium.org/1326643003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayProgressBarObserver.java:10:
public class OverlayProgressBarObserver {
I would call this OverlayContentProgressObserver or
OverlayContentLoadingProgressObserver, because:

1) You are observing the progress, not the bar (it could have any graphical
representation).
2) The progress belongs to the Content(View) not the Overlay (position)

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

https://codereview.chromium.org/1326643003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:83:
private boolean mIsContentViewShowing;
This is not being used anymore, remove it.

https://codereview.chromium.org/1326643003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:131:
mOverlayPanelContent = createNewOverlayPanelContent();
I think we should postpone the creation of the OverlayPanelContent to the last
minute, that is, when we need to display the ContentView or load an URL in it,
and destroy it when we don't need anymore. 

Please note that ContextualSearchPanel will be instantiated even when
ContextualSearch is not enabled.

https://codereview.chromium.org/1326643003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:168:
public class SearchOverlayContentObserver extends OverlayContentObserver {
Shouldn't this belong to the ContextualSearchManager?

https://codereview.chromium.org/1326643003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:200:
mTabRedirectHandler = new TabRedirectHandler(mActivity);
This belongs to the OverlayPanelContent

https://codereview.chromium.org/1326643003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:285:
mOverlayPanelContent.destroyContentView();
Instead of calling destroyContentView(), which should not be exposed, I think we
should create a new OverlayPanelContent for every new search, and destroy the
whole thing here, so we would call instead:

mOverlayPanelContent.destroy()
mOverlayPanelContent = null;

And the destoy() method will take care of destroying the underlying ContentView

https://codereview.chromium.org/1326643003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:521:
private class InterceptNavigationDelegateImpl implements
InterceptNavigationDelegate {
This belongs to the OverlayPanelContent

https://codereview.chromium.org/1326643003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:691:
return mIsContentViewShowing;
This should use the OverlayPanelContent method.

[mdjones, 2015-09-14 23:58:53]

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

https://codereview.chromium.org/1326643003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayContentObserver.java:44:
public void onVisibilityChanged(boolean isVisible) {}
On 2015/09/10 17:58:39, pedrosimonetti wrote:
> Nit: Since you created a section for ContentView below, I'd move this to that
> section. Also, all method names start with onContentView, so I'd call this
> onContentViewVisibilityChanged.

Done.

https://codereview.chromium.org/1326643003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayContentObserver.java:49:
public void onContentViewSeen() {}
On 2015/09/10 17:58:38, pedrosimonetti wrote:
> Nit: Since you created a section for ContentView below, I'd move this to that
> section.

Done.

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

https://codereview.chromium.org/1326643003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:86:
*/
On 2015/09/10 17:58:39, pedrosimonetti wrote:
> @param progressObserver

Done.

https://codereview.chromium.org/1326643003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:129:
public void createNewContentView() {
On 2015/09/10 17:58:39, pedrosimonetti wrote:
> JavaDoc here and elsewhere

Done.

https://codereview.chromium.org/1326643003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:210:
destroyContentView();
On 2015/09/10 17:58:39, pedrosimonetti wrote:
> No need to destroy here, createNewContentView will do just that.

Done.

https://codereview.chromium.org/1326643003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:226:
mContentViewCore.getWebContents().updateTopControlsState(enableHiding,
enableShowing,
On 2015/09/10 17:58:39, pedrosimonetti wrote:
> Include:
> if (mContentViewCore != null && mContentViewCore.getWebContents() != null) {

Done.

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

https://codereview.chromium.org/1326643003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayProgressBarObserver.java:10:
public class OverlayProgressBarObserver {
On 2015/09/10 17:58:39, pedrosimonetti wrote:
> I would call this OverlayContentProgressObserver or
> OverlayContentLoadingProgressObserver, because:
> 
> 1) You are observing the progress, not the bar (it could have any graphical
> representation).
> 2) The progress belongs to the Content(View) not the Overlay (position)

Done.

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

https://codereview.chromium.org/1326643003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:83:
private boolean mIsContentViewShowing;
On 2015/09/10 17:58:39, pedrosimonetti wrote:
> This is not being used anymore, remove it.

Done.

https://codereview.chromium.org/1326643003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:131:
mOverlayPanelContent = createNewOverlayPanelContent();
On 2015/09/10 17:58:39, pedrosimonetti wrote:
> I think we should postpone the creation of the OverlayPanelContent to the last
> minute, that is, when we need to display the ContentView or load an URL in it,
> and destroy it when we don't need anymore. 
> 
> Please note that ContextualSearchPanel will be instantiated even when
> ContextualSearch is not enabled.

Done.

https://codereview.chromium.org/1326643003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:168:
public class SearchOverlayContentObserver extends OverlayContentObserver {
On 2015/09/10 17:58:39, pedrosimonetti wrote:
> Shouldn't this belong to the ContextualSearchManager?

I was going to make this a separate CL as well, however, the move was pretty
straight forward. Done.

https://codereview.chromium.org/1326643003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:200:
mTabRedirectHandler = new TabRedirectHandler(mActivity);
On 2015/09/10 17:58:39, pedrosimonetti wrote:
> This belongs to the OverlayPanelContent

Done.

https://codereview.chromium.org/1326643003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:285:
mOverlayPanelContent.destroyContentView();
On 2015/09/10 17:58:39, pedrosimonetti wrote:
> Instead of calling destroyContentView(), which should not be exposed, I think
we
> should create a new OverlayPanelContent for every new search, and destroy the
> whole thing here, so we would call instead:
> 
> mOverlayPanelContent.destroy()
> mOverlayPanelContent = null;
> 
> And the destoy() method will take care of destroying the underlying
ContentView

Agreed, however, I think this should be a separate CL.

https://codereview.chromium.org/1326643003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:521:
private class InterceptNavigationDelegateImpl implements
InterceptNavigationDelegate {
On 2015/09/10 17:58:39, pedrosimonetti wrote:
> This belongs to the OverlayPanelContent

Done.

https://codereview.chromium.org/1326643003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:691:
return mIsContentViewShowing;
On 2015/09/10 17:58:39, pedrosimonetti wrote:
> This should use the OverlayPanelContent method.

Done.

[pedro (no code reviews), 2015-09-16 20:24:20]

Hey Matt, here are some more comments. Feel free to chat in person if you have
any questions.

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

https://codereview.chromium.org/1326643003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayContentDelegate.java:47:
public boolean shouldInterceptNavigation(ExternalNavigationHandler
externalNavHandler,
Nit: Can we change this to pass 2 booleans instead? The method signature would
be:
public boolean shouldInterceptNavigation(boolean canOverrideUrl, boolean
isExternalProtocol)

Where:
canOverrideUrl = externalNavHandler.shouldOverrideUrlLoading(params)!=
OverrideUrlLoadingResult.NO_OVERRIDE
isExternalProtocol = navigationParams.isExternalProtocol

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

https://codereview.chromium.org/1326643003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:171:
public void createNewContentView() {
Nit: Put a TODO to make this private.

https://codereview.chromium.org/1326643003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:238:
public void destroyContentView() {
Nit: Put a TODO to make this private.

https://codereview.chromium.org/1326643003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:281:
// Expose ContentViewCore method to owner.
Nit: remove comment (does not add any extra value).

https://codereview.chromium.org/1326643003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:407:
public void destroy() {
Shouldn't this destroy the whole thing? (including the ContentView)

https://codereview.chromium.org/1326643003/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/1326643003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:133:
if (!isFullscreenSizePanel()) {
Can we move this to the OverlayPanelContent class? The Panel should not know
about the ContentViewClient, which is an implementation details of the
ContentViewCore implementation we're trying to abstract.

It would depend on an interface to get the desired width and height from the
Panel.

https://codereview.chromium.org/1326643003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:647:
if (mOverlayPanelContent != null) {
I think it would be clearer if we were using the ternary operator:

return mOverlayPanelContent != null ?
        mOverlayPanelContent.getContentViewCore() : null;

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

https://codereview.chromium.org/1326643003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:773:
public class SearchOverlayContentDelegate extends OverlayContentDelegate {
I kinda think that the Panel should be the delegate of the Content, since
nobody, expect for the Panel, should know about the Content. It feels weird that
the Manager is the delegate of an internal component of the Panel (the Content).

We would still have to propagate some of those events from the Panel to the
Manager though. I guess part of the problem is that we still have logic in the
Manager that should belong to the Panel.

https://codereview.chromium.org/1326643003/diff/100001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchFakeServer.java
(right):

https://codereview.chromium.org/1326643003/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchFakeServer.java:99:
// Override to do nothing.
This comment is not informative. "Override to prevent it from calling native
code" or something similar would be better.

[mdjones, 2015-09-18 23:12:44]

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

https://codereview.chromium.org/1326643003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayContentDelegate.java:47:
public boolean shouldInterceptNavigation(ExternalNavigationHandler
externalNavHandler,
On 2015/09/16 20:24:20, pedrosimonetti wrote:
> Nit: Can we change this to pass 2 booleans instead? The method signature would
> be:
> public boolean shouldInterceptNavigation(boolean canOverrideUrl, boolean
> isExternalProtocol)
> 
> Where:
> canOverrideUrl = externalNavHandler.shouldOverrideUrlLoading(params)!=
> OverrideUrlLoadingResult.NO_OVERRIDE
> isExternalProtocol = navigationParams.isExternalProtocol

This will be in a different Cl since it causes non-trivial changes to testing.

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

https://codereview.chromium.org/1326643003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:171:
public void createNewContentView() {
On 2015/09/16 20:24:20, pedrosimonetti wrote:
> Nit: Put a TODO to make this private.

Done.

https://codereview.chromium.org/1326643003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:238:
public void destroyContentView() {
On 2015/09/16 20:24:20, pedrosimonetti wrote:
> Nit: Put a TODO to make this private.

Done.

https://codereview.chromium.org/1326643003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:281:
// Expose ContentViewCore method to owner.
On 2015/09/16 20:24:20, pedrosimonetti wrote:
> Nit: remove comment (does not add any extra value).

Done.

https://codereview.chromium.org/1326643003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:407:
public void destroy() {
On 2015/09/16 20:24:20, pedrosimonetti wrote:
> Shouldn't this destroy the whole thing? (including the ContentView)

Done.

https://codereview.chromium.org/1326643003/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/1326643003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:133:
if (!isFullscreenSizePanel()) {
On 2015/09/16 20:24:20, pedrosimonetti wrote:
> Can we move this to the OverlayPanelContent class? The Panel should not know
> about the ContentViewClient, which is an implementation details of the
> ContentViewCore implementation we're trying to abstract.
> 
> It would depend on an interface to get the desired width and height from the
> Panel.

Currently this is a contextual search specific feature as we plan to only
support full screen size. This will not be a part of the panel base class in the
future.

https://codereview.chromium.org/1326643003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:647:
if (mOverlayPanelContent != null) {
On 2015/09/16 20:24:20, pedrosimonetti wrote:
> I think it would be clearer if we were using the ternary operator:
> 
> return mOverlayPanelContent != null ?
>         mOverlayPanelContent.getContentViewCore() : null;

For me, personally, this is harder to understand at a glance due to the long
names. However, if you really want to use the ternary operator, I will change
it.

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

https://codereview.chromium.org/1326643003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:773:
public class SearchOverlayContentDelegate extends OverlayContentDelegate {
On 2015/09/16 20:24:20, pedrosimonetti wrote:
> I kinda think that the Panel should be the delegate of the Content, since
> nobody, expect for the Panel, should know about the Content. It feels weird
that
> the Manager is the delegate of an internal component of the Panel (the
Content).
> 
> We would still have to propagate some of those events from the Panel to the
> Manager though. I guess part of the problem is that we still have logic in the
> Manager that should belong to the Panel.

>> Shouldn't this belong to the ContextualSearchManager?

I agree with your initial comment of having this class in the manager. All this
class really does is call manager functions, so I believe this is the correct
place for it.

https://codereview.chromium.org/1326643003/diff/100001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchFakeServer.java
(right):

https://codereview.chromium.org/1326643003/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchFakeServer.java:99:
// Override to do nothing.
On 2015/09/16 20:24:20, pedrosimonetti wrote:
> This comment is not informative. "Override to prevent it from calling native
> code" or something similar would be better.

Done.

[pedro (no code reviews), 2015-09-23 00:00:12]

lgtm

Hey Matthew, sorry for the long delay. This change is looking good. The only
thing I would try to change is the new boolean added in the
ContextualSearchManager class.

I would like to play around with your patch and it has some conflicts. Could you
please rebase whenever you have a chance?

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

https://codereview.chromium.org/1326643003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:141:
// NOTE: This constructor is intentionally sparse for testing.
Nit: Include your ldap on NOTE comments, so that others know who to ask if they
have questions.

// NOTE(mdjones): <comment>

https://codereview.chromium.org/1326643003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:162:
if (mContentViewCore != null) {
Please incorporate my recent change to prevent the blank Panel if it gets
submitted before this change.

https://codereview.chromium.org/1360873002/

https://codereview.chromium.org/1326643003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:242:
setVisibility(false);
Similarly, please incorporate my recent change to prevent the blank Panel if it
gets
submitted before this change.

https://codereview.chromium.org/1360873002/

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

https://codereview.chromium.org/1326643003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:124:
overlayPanelContent.setChromeActivity(mActivity);
Why not passing the activity to the constructor?

https://codereview.chromium.org/1326643003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:591:
if (mOverlayPanelContent == null) {
Nit: See comment below. Use a getter instead:

return getOverlayPanelContent().isContentViewShowing();

https://codereview.chromium.org/1326643003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:605:
if (mOverlayPanelContent == null) {
Nit: Instead of creating it here, please add a private method to return a new
panel content if one doesn't already exist, so that this method would be written
like:

getOverlayPanelContent().loadUrl(url);

Similarly, do the same thing on other places where you are checking for whether
the OverlayPanelContent is null.

https://codereview.chromium.org/1326643003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:651:
if (mOverlayPanelContent == null) {
Nit: Use a getter in here as well, as suggested in other places.

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

https://codereview.chromium.org/1326643003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:483:
mShouldLoadDelayedSearch = false;
I'm not very happy with adding a new boolean, because the logic in the Manager
is already pretty complicated, but I cannot think of a better solution at the
moment.

Things to keep in mind:

1) If we decide to keep the boolean, it should actually be placed inside the
block below, because not all taps will load content before the user pulls the
Panel up.

2) Also note that even when it does, a tap will not load it *immediately*, as
stated in comment. Instead if will be loaded after a round trip to the server to
find out the resolved search term. For this reason, please update the phrasing
of the comment.

I will try to find a better solution, but I cannot apply your patch due to merge
conflicts :(

https://codereview.chromium.org/1326643003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:1141:
|| mShouldLoadDelayedSearch) {
Could you please include a comment here detailing why exactly we need the new
boolean?

[mdjones, 2015-09-23 00:29:31]

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

[mdjones, 2015-09-23 00:29:32]

Your turn to take a look :)

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

https://codereview.chromium.org/1326643003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:141:
// NOTE: This constructor is intentionally sparse for testing.
On 2015/09/23 00:00:11, pedrosimonetti wrote:
> Nit: Include your ldap on NOTE comments, so that others know who to ask if
they
> have questions.
> 
> // NOTE(mdjones): <comment>

Done.

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

https://codereview.chromium.org/1326643003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:124:
overlayPanelContent.setChromeActivity(mActivity);
On 2015/09/23 00:00:11, pedrosimonetti wrote:
> Why not passing the activity to the constructor?

Originally when the content was created, the activity was not available so it
needed to be set. This is a remnant of that. I have included it in the
constructor now.

https://codereview.chromium.org/1326643003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:591:
if (mOverlayPanelContent == null) {
On 2015/09/23 00:00:11, pedrosimonetti wrote:
> Nit: See comment below. Use a getter instead:
> 
> return getOverlayPanelContent().isContentViewShowing();

In this case I'm not sure we want to create a panel if it was null. The caller
is simply checking if the panel is showing and does not necessarily need one to
be created.

https://codereview.chromium.org/1326643003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:605:
if (mOverlayPanelContent == null) {
On 2015/09/23 00:00:12, pedrosimonetti wrote:
> Nit: Instead of creating it here, please add a private method to return a new
> panel content if one doesn't already exist, so that this method would be
written
> like:
> 
> getOverlayPanelContent().loadUrl(url);
> 
> Similarly, do the same thing on other places where you are checking for
whether
> the OverlayPanelContent is null.

Done.

https://codereview.chromium.org/1326643003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:651:
if (mOverlayPanelContent == null) {
On 2015/09/23 00:00:12, pedrosimonetti wrote:
> Nit: Use a getter in here as well, as suggested in other places.

Done.

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

https://codereview.chromium.org/1326643003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:483:
mShouldLoadDelayedSearch = false;
On 2015/09/23 00:00:12, pedrosimonetti wrote:
> I'm not very happy with adding a new boolean, because the logic in the Manager
> is already pretty complicated, but I cannot think of a better solution at the
> moment.
> 
> Things to keep in mind:
> 
> 1) If we decide to keep the boolean, it should actually be placed inside the
> block below, because not all taps will load content before the user pulls the
> Panel up.
> 
> 2) Also note that even when it does, a tap will not load it *immediately*, as
> stated in comment. Instead if will be loaded after a round trip to the server
to
> find out the resolved search term. For this reason, please update the phrasing
> of the comment.
> 
> I will try to find a better solution, but I cannot apply your patch due to
merge
> conflicts :(

Acknowledged.

https://codereview.chromium.org/1326643003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:1141:
|| mShouldLoadDelayedSearch) {
On 2015/09/23 00:00:12, pedrosimonetti wrote:
> Could you please include a comment here detailing why exactly we need the new
> boolean?

Done.

[David Trainor- moved to gerrit, 2015-09-24 16:40:34]

Mostly nits.  A few questions.  But if these are addressed lgtm.

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

https://codereview.chromium.org/1326643003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayContentProgressObserver.java:15:
public void onProgressBarStarted() {}
How do these relate to onContentLoadStarted/Finished?

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

https://codereview.chromium.org/1326643003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:77:
// http://crbug.com/522266 : An instance of InterceptNavigationDelegateImpl
should be kept in
Does this mean we're not ref counting the object correctly in native?  Is the
ownership model incorrect?

https://codereview.chromium.org/1326643003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:110:
OverlayContentProgressObserver progressObserver, ChromeActivity activity) {
update javadoc for ChromeActivity

https://codereview.chromium.org/1326643003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:111:
nativeInit();
Set the native pointer to the return value of this (unless you're doing
native/java inheritance)?  Then you can remove the setNativePointer method.

https://codereview.chromium.org/1326643003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:159:
* Create a new ContetnViewCore that will be managed by this panel.
Contetn -> Content

https://codereview.chromium.org/1326643003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:219:
if (mInterceptNavigationDelegate != null) {
this can't be null right?

https://codereview.chromium.org/1326643003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:243:
setVisibility(false);
createContentView doesn't call setVisibility(true).  Should these match?  Fine
either way, just thinking out loud here.

https://codereview.chromium.org/1326643003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:341:
mContentViewClient = viewClient;
Do we want to call mContentViewCore.setContentViewClient() here if it isn't
null?

https://codereview.chromium.org/1326643003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:359:
public void clearNativePanelContentPtr() {
Make the called by native methods private IMO.

https://codereview.chromium.org/1326643003/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/1326643003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:604:
if (mOverlayPanelContent == null) {
return mOverlayPanelContent != null &&
mOverlayPanelContent.isContentViewShowing();

https://codereview.chromium.org/1326643003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:630:
mOverlayPanelContent.updateTopControlsState(false, true, false);
Check for null?

https://codereview.chromium.org/1326643003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:638:
return mOverlayPanelContent.didLoadAnyUrl();
mOverlayPanelContent != null && ?

https://codereview.chromium.org/1326643003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:643:
if (mOverlayPanelContent != null) {
Could inline these as well, but either way works.

https://codereview.chromium.org/1326643003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:653:
mOverlayPanelContent.removeLastHistoryEntry(historyUrl, urlTimeMs);
null check?

https://codereview.chromium.org/1326643003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:661:
public void destroy() {
javadoc

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

https://codereview.chromium.org/1326643003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:793:
onExternalNavigation(url);
Would it make sense to inline some of these methods (or make them private for
now) so that ContextualSearchManager's interface shrinks a bit?

[mdjones, 2015-09-24 22:23:10]

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

https://codereview.chromium.org/1326643003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayContentProgressObserver.java:15:
public void onProgressBarStarted() {}
On 2015/09/24 16:40:33, David Trainor wrote:
> How do these relate to onContentLoadStarted/Finished?

Start and finish should happen at about the same time as the content loading
version. However, this is controlled by a ContentViewClient rather than an
WebContentsObserver so I chose to keep the methods separate.

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

https://codereview.chromium.org/1326643003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:77:
// http://crbug.com/522266 : An instance of InterceptNavigationDelegateImpl
should be kept in
On 2015/09/24 16:40:33, David Trainor wrote:
> Does this mean we're not ref counting the object correctly in native?  Is the
> ownership model incorrect?

It seems like it. It may be that it was originally being destroyed (I believe
the native side used to be more frequently created and destroyed). Since only a
single instance of this was ever made, it would be garbage collected since it
was no longer being held in native and therefore be null. I'll look into
removing this in a later change.

https://codereview.chromium.org/1326643003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:110:
OverlayContentProgressObserver progressObserver, ChromeActivity activity) {
On 2015/09/24 16:40:33, David Trainor wrote:
> update javadoc for ChromeActivity

Done.

https://codereview.chromium.org/1326643003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:111:
nativeInit();
On 2015/09/24 16:40:33, David Trainor wrote:
> Set the native pointer to the return value of this (unless you're doing
> native/java inheritance)?  Then you can remove the setNativePointer method.

Done.

https://codereview.chromium.org/1326643003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:159:
* Create a new ContetnViewCore that will be managed by this panel.
On 2015/09/24 16:40:33, David Trainor wrote:
> Contetn -> Content

Done.

https://codereview.chromium.org/1326643003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:219:
if (mInterceptNavigationDelegate != null) {
On 2015/09/24 16:40:33, David Trainor wrote:
> this can't be null right?

Not any more. Done.

https://codereview.chromium.org/1326643003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:243:
setVisibility(false);
On 2015/09/24 16:40:33, David Trainor wrote:
> createContentView doesn't call setVisibility(true).  Should these match?  Fine
> either way, just thinking out loud here.

Eventually I would like it to be that way, but there are some cases where a
ContentViewCore is created and we may not show it.

https://codereview.chromium.org/1326643003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:341:
mContentViewClient = viewClient;
On 2015/09/24 16:40:33, David Trainor wrote:
> Do we want to call mContentViewCore.setContentViewClient() here if it isn't
> null?

Done.

https://codereview.chromium.org/1326643003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:359:
public void clearNativePanelContentPtr() {
On 2015/09/24 16:40:33, David Trainor wrote:
> Make the called by native methods private IMO.

Done.

https://codereview.chromium.org/1326643003/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/1326643003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:604:
if (mOverlayPanelContent == null) {
On 2015/09/24 16:40:34, David Trainor wrote:
> return mOverlayPanelContent != null &&
> mOverlayPanelContent.isContentViewShowing();

Done.

https://codereview.chromium.org/1326643003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:630:
mOverlayPanelContent.updateTopControlsState(false, true, false);
On 2015/09/24 16:40:34, David Trainor wrote:
> Check for null?

The idea was that there is no code path that calls this where
mOverlayPanelContent was null, but it doesn't hurt to have it.

https://codereview.chromium.org/1326643003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:638:
return mOverlayPanelContent.didLoadAnyUrl();
On 2015/09/24 16:40:34, David Trainor wrote:
> mOverlayPanelContent != null && ?

Done.

https://codereview.chromium.org/1326643003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:643:
if (mOverlayPanelContent != null) {
On 2015/09/24 16:40:34, David Trainor wrote:
> Could inline these as well, but either way works.

Done.

https://codereview.chromium.org/1326643003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:653:
mOverlayPanelContent.removeLastHistoryEntry(historyUrl, urlTimeMs);
On 2015/09/24 16:40:34, David Trainor wrote:
> null check?

Done.

https://codereview.chromium.org/1326643003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:661:
public void destroy() {
On 2015/09/24 16:40:34, David Trainor wrote:
> javadoc

Done.

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

https://codereview.chromium.org/1326643003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:793:
onExternalNavigation(url);
On 2015/09/24 16:40:34, David Trainor wrote:
> Would it make sense to inline some of these methods (or make them private for
> now) so that ContextualSearchManager's interface shrinks a bit?

Inline makes sense.

[mdjones, 2015-09-24 22:23:33]

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

[commit-bot: I haz the power, 2015-09-24 22:24:30]

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

[commit-bot: I haz the power, 2015-09-24 22:56:22]

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

[commit-bot: I haz the power, 2015-09-24 22:56:23]

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-09-25 00:16:07]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2015-09-25 00:16:08]

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

[commit-bot: I haz the power, 2015-09-25 00:16:23]

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

[commit-bot: I haz the power, 2015-09-25 02:27:12]

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

[commit-bot: I haz the power, 2015-09-25 02:27:13]

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

[mdjones, 2015-09-25 15:30:07]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2015-09-25 15:30:07]

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/1326643003/#ps220001
(title: "flip booleans for testing renamed api")

[commit-bot: I haz the power, 2015-09-25 15:30:48]

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

[commit-bot: I haz the power, 2015-09-25 16:26:29]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-09-25 16:27:26]

Patchset 12 (id:??) landed as
https://crrev.com/32a8a653581e79e552b16f4c960967ef6b3c301b
Cr-Commit-Position: refs/heads/master@{#350842}