Move functionality for ContentViewCore to ContextualSearchPanel

Move functionality for ContentViewCore to ContextualSearchPanel

This change moves code related to the content of the contextual
search panel to the panel class.

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

[mdjones, 2015-08-26 01:20:12]

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

[mdjones, 2015-08-26 01:20:13]

Follow-up to https://codereview.chromium.org/1283223004/
PTAL

[David Trainor- moved to gerrit, 2015-08-28 22:02:18]

lgtm.  I'm assuming that the nitty gritty logic details are just moves and not
actual logic changes.

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

https://codereview.chromium.org/1304013002/diff/50001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:104:
private ContextualSearchContentController mContentController;
This parameter feels weird because we're just setting it to this haha.  I saw
your comment on testing though.

https://codereview.chromium.org/1304013002/diff/50001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:153:
mWebContentsDelegate = new WebContentsDelegateAndroid() {
I'm assuming all of the actual logic is moved from the manager.  Is that right?

https://codereview.chromium.org/1304013002/diff/50001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagementDelegate.java
(right):

https://codereview.chromium.org/1304013002/diff/50001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagementDelegate.java:142:
void onContextualSearchRequestNavigation(boolean isFailure);
@param

https://codereview.chromium.org/1304013002/diff/50001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagementDelegate.java:148:
void onContentViewVisibilityChanged(boolean isVisible);
@param

[mdjones, 2015-08-28 23:51:58]

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

https://codereview.chromium.org/1304013002/diff/50001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:104:
private ContextualSearchContentController mContentController;
On 2015/08/28 22:02:18, David Trainor wrote:
> This parameter feels weird because we're just setting it to this haha.  I saw
> your comment on testing though.

Yeah, the manager does something similar. I'm not particularly fond of this,
however, it avoids adding another class and the associated get/set methods.

https://codereview.chromium.org/1304013002/diff/50001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:153:
mWebContentsDelegate = new WebContentsDelegateAndroid() {
On 2015/08/28 22:02:18, David Trainor wrote:
> I'm assuming all of the actual logic is moved from the manager.  Is that
right?

Correct

https://codereview.chromium.org/1304013002/diff/50001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagementDelegate.java
(right):

https://codereview.chromium.org/1304013002/diff/50001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagementDelegate.java:142:
void onContextualSearchRequestNavigation(boolean isFailure);
On 2015/08/28 22:02:18, David Trainor wrote:
> @param

Done.

https://codereview.chromium.org/1304013002/diff/50001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagementDelegate.java:148:
void onContentViewVisibilityChanged(boolean isVisible);
On 2015/08/28 22:02:18, David Trainor wrote:
> @param

Already there

[pedro (no code reviews), 2015-09-01 04:00:37]

Great progress Matthew! Things are looking good, except that some logic should
remain in the manager. For now, let's move/expose things we need to the
CSManagerDelegate and CSPanelDelegate and then we'll plan an API that will work
in a general fashion.

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

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:38:
implements ContextualSearchPanelDelegate, ContextualSearchContentController {
Instead of adding a new interface now, since we're "just" moving things around,
I would suggest adding necessary APIs to the CSPanelDelegate. Then we can more
carefully plan the APIs we'll need to make the Panel reusable.

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:75:
// The animation duration of a URL being promoted to a tab when triggered by an
A lot of these new properties should stay in the CSManager (see remaining
comments).

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:77:
// so that it completes before the navigation.
Add JavaDoc comments for the properties that remain here, for consistency
reasons with the rest of the class.

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:151:
mContentController = this;
We won't need the controller if we move necessary methods to the panel delegate.

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:325:
private void onExternalNavigation(String url) {
Move this method to the CSManger.

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:326:
if (isFullscreenSizePanel()) {
This first block of code should be moved to a new method in CSPanel called
updateTopControlsState(), with no parameters, and expose it in the
CSPanelDelegate to be used from this method.

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:469:
// ContextualSearchContentController
Since we're removing the interface, let's call this section "Content View"

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:513:
mDidPromoteSearchNavigation = false;
Create a method in the manager called handleDidStartLoading(), expose it via the
manager's delegate, and keep the property mDidPromoteSearchNavigation in the
manager as well.

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:520:
if (isMainFrame) onExternalNavigation(validatedUrl);
Create a method in the manager called didStartProvisionalLoadForFrame(), expose
it via the
manager's delegate. Its implementation will be:

if (isMainFrame) onExternalNavigation(validatedUrl);

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:527:
handleDidNavigateMainFrame(url, httpResultCode);
Also move this method to the manager and call it using the delegate.

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:533:
getManagementDelegate().onSearchResultsLoaded();
For consistency with the other changes suggested, rename the method to
handleDidFinishLoad(). Later we'll choose the appropriate API names.

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:540:
getManagementDelegate().onContentViewCreated(mContentViewCore);
Nit: add empty line to separate the onContentViewCreated() call from the
intercept stuff.

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:568:
public void handleDidNavigateMainFrame(String url, int httpResultCode) {
Keep this method in the manager, exposing handleDidNavigateMainFrame() to the
manager's delegate

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:591:
// Utilities
Remove this section and keep everything here under the "Content View" umbrella.

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:631:
private boolean isHttpFailureCode(int httpResultCode) {
Move this method to the CSManager.

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:638:
private boolean shouldPromoteSearchNavigation() {
Move this method to the CSManager.

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:669:
if (mExternalNavHandler.shouldOverrideUrlLoading(params)
The panel should not know about promotion.

Add a shouldIgnoreContentNavigation(ExternalNavigationHandler
externalNavigationHandler, NavigationParams navigationParams) to CSManager and
call it from here.

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:676:
ContentViewCore baseContentView = getManagementDelegate().getBaseContentView();
We should not expose getBaseContentView in ManagerDelegate. We should not need
to log to console at all, so we might not need to get the CV. I suggest
confirming with who originally wrote this piece of code why this is here and if
it's okay to remove it.

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:791:
public void setWasSearchContentViewSeen() {
We don't need this method exposed in the interface anymore, so we can delete it
from here.

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:836:
public void createNewPanelContentView() {
We should not expose this method. Also, with the changes suggested for not
exposing the content view creation/destruction, you should be able to create the
private createNewContentView() directly.

See comment on ContextualSearchFakeServer#createNewContentView

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:841:
public void loadUrlInPanel(String url) {
I believe we don't need this method, since we can call the public loadUrl()
directly.

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

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelDelegate.java:97:
void setWasSearchContentViewSeen();
We don't need this method exposed in the interface anymore, and should probably
reduce its visibility (by deleting the overridden method in CSPanel).

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelDelegate.java:131:
ContentViewCore getContentViewCore();
Put a TODO here to not expose this method. Ideally the ContentView usage would
be completely transparent to the consumer, and we should plan our API design to
incorporate changes to make this happen.

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java
(right):

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:277:
@Nullable public ContentViewCore getBaseContentView() {
We should not make this public. See comment on CSPanel.

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchFakeServer.java
(right):

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchFakeServer.java:87:
public void createNewContentView() {
We only need to override the content view creation because in test mode we
should not create it (to prevent network requests), and instead, we only need to
know whether a content view would be created.

I think it would be better to make createNewContentView() and
destroyContentView() private and provide:
  - an extra contructor in CSPanel, used just for testing, with an extra
parameter to inform whether to disable content view creation for testing, and
update the code to prevent the content view creation while keeping track of
whether it would have been created.
  - a new method wouldHaveCreatedContentView(), used just for testing.

[mdjones, 2015-09-02 22:31:33]

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

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:38:
implements ContextualSearchPanelDelegate, ContextualSearchContentController {
On 2015/09/01 04:00:37, pedrosimonetti wrote:
> Instead of adding a new interface now, since we're "just" moving things
around,
> I would suggest adding necessary APIs to the CSPanelDelegate. Then we can more
> carefully plan the APIs we'll need to make the Panel reusable.

As we discussed, adding the infrastructure to make this work with a single
interface is far more work than simply splitting them; especially since this is
closer to how they will end up.

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:75:
// The animation duration of a URL being promoted to a tab when triggered by an
On 2015/09/01 04:00:36, pedrosimonetti wrote:
> A lot of these new properties should stay in the CSManager (see remaining
> comments).

Acknowledged.

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:77:
// so that it completes before the navigation.
On 2015/09/01 04:00:37, pedrosimonetti wrote:
> Add JavaDoc comments for the properties that remain here, for consistency
> reasons with the rest of the class.

Done.

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:151:
mContentController = this;
On 2015/09/01 04:00:37, pedrosimonetti wrote:
> We won't need the controller if we move necessary methods to the panel
delegate.

Added TODO for later CL.

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:325:
private void onExternalNavigation(String url) {
On 2015/09/01 04:00:36, pedrosimonetti wrote:
> Move this method to the CSManger.

Done.

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:326:
if (isFullscreenSizePanel()) {
On 2015/09/01 04:00:36, pedrosimonetti wrote:
> This first block of code should be moved to a new method in CSPanel called
> updateTopControlsState(), with no parameters, and expose it in the
> CSPanelDelegate to be used from this method.

Done.

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:469:
// ContextualSearchContentController
On 2015/09/01 04:00:36, pedrosimonetti wrote:
> Since we're removing the interface, let's call this section "Content View"

This will be handled in a previous TODO.

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:513:
mDidPromoteSearchNavigation = false;
On 2015/09/01 04:00:36, pedrosimonetti wrote:
> Create a method in the manager called handleDidStartLoading(), expose it via
the
> manager's delegate, and keep the property mDidPromoteSearchNavigation in the
> manager as well.

This will come with the next CL.

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:520:
if (isMainFrame) onExternalNavigation(validatedUrl);
On 2015/09/01 04:00:36, pedrosimonetti wrote:
> Create a method in the manager called didStartProvisionalLoadForFrame(),
expose
> it via the
> manager's delegate. Its implementation will be:
> 
> if (isMainFrame) onExternalNavigation(validatedUrl);

This will come with the next CL.

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:527:
handleDidNavigateMainFrame(url, httpResultCode);
On 2015/09/01 04:00:36, pedrosimonetti wrote:
> Also move this method to the manager and call it using the delegate.

This will come with the next CL.

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:533:
getManagementDelegate().onSearchResultsLoaded();
On 2015/09/01 04:00:36, pedrosimonetti wrote:
> For consistency with the other changes suggested, rename the method to
> handleDidFinishLoad(). Later we'll choose the appropriate API names.

This will come with the next CL.

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:540:
getManagementDelegate().onContentViewCreated(mContentViewCore);
On 2015/09/01 04:00:36, pedrosimonetti wrote:
> Nit: add empty line to separate the onContentViewCreated() call from the
> intercept stuff.

Done.

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:568:
public void handleDidNavigateMainFrame(String url, int httpResultCode) {
On 2015/09/01 04:00:36, pedrosimonetti wrote:
> Keep this method in the manager, exposing handleDidNavigateMainFrame() to the
> manager's delegate

Done.

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:591:
// Utilities
On 2015/09/01 04:00:36, pedrosimonetti wrote:
> Remove this section and keep everything here under the "Content View"
umbrella.

Done.

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:631:
private boolean isHttpFailureCode(int httpResultCode) {
On 2015/09/01 04:00:36, pedrosimonetti wrote:
> Move this method to the CSManager.

Done.

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:638:
private boolean shouldPromoteSearchNavigation() {
On 2015/09/01 04:00:36, pedrosimonetti wrote:
> Move this method to the CSManager.

Done.

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:669:
if (mExternalNavHandler.shouldOverrideUrlLoading(params)
On 2015/09/01 04:00:36, pedrosimonetti wrote:
> The panel should not know about promotion.
> 
> Add a shouldIgnoreContentNavigation(ExternalNavigationHandler
> externalNavigationHandler, NavigationParams navigationParams) to CSManager and
> call it from here.

Done.

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:676:
ContentViewCore baseContentView = getManagementDelegate().getBaseContentView();
On 2015/09/01 04:00:36, pedrosimonetti wrote:
> We should not expose getBaseContentView in ManagerDelegate. We should not need
> to log to console at all, so we might not need to get the CV. I suggest
> confirming with who originally wrote this piece of code why this is here and
if
> it's okay to remove it.

Done.

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:791:
public void setWasSearchContentViewSeen() {
On 2015/09/01 04:00:36, pedrosimonetti wrote:
> We don't need this method exposed in the interface anymore, so we can delete
it
> from here.

Done.

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:836:
public void createNewPanelContentView() {
On 2015/09/01 04:00:36, pedrosimonetti wrote:
> We should not expose this method. Also, with the changes suggested for not
> exposing the content view creation/destruction, you should be able to create
the
> private createNewContentView() directly.
> 
> See comment on ContextualSearchFakeServer#createNewContentView

TODO added.

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:841:
public void loadUrlInPanel(String url) {
On 2015/09/01 04:00:37, pedrosimonetti wrote:
> I believe we don't need this method, since we can call the public loadUrl()
> directly.

This abstraction is needed for testing.

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

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelDelegate.java:97:
void setWasSearchContentViewSeen();
On 2015/09/01 04:00:37, pedrosimonetti wrote:
> We don't need this method exposed in the interface anymore, and should
probably
> reduce its visibility (by deleting the overridden method in CSPanel).

Done.

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelDelegate.java:131:
ContentViewCore getContentViewCore();
On 2015/09/01 04:00:37, pedrosimonetti wrote:
> Put a TODO here to not expose this method. Ideally the ContentView usage would
> be completely transparent to the consumer, and we should plan our API design
to
> incorporate changes to make this happen.

Done.

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java
(right):

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:277:
@Nullable public ContentViewCore getBaseContentView() {
On 2015/09/01 04:00:37, pedrosimonetti wrote:
> We should not make this public. See comment on CSPanel.

Done.

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchFakeServer.java
(right):

https://codereview.chromium.org/1304013002/diff/70001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchFakeServer.java:87:
public void createNewContentView() {
On 2015/09/01 04:00:37, pedrosimonetti wrote:
> We only need to override the content view creation because in test mode we
> should not create it (to prevent network requests), and instead, we only need
to
> know whether a content view would be created.
> 
> I think it would be better to make createNewContentView() and
> destroyContentView() private and provide:
>   - an extra contructor in CSPanel, used just for testing, with an extra
> parameter to inform whether to disable content view creation for testing, and
> update the code to prevent the content view creation while keeping track of
> whether it would have been created.
>   - a new method wouldHaveCreatedContentView(), used just for testing.

Adding the new constructor instead of injecting the functionality would create
significant changes in the tests; we would need a way to tell the layout manager
to create a testing panel instead of a normal one.

[pedro (no code reviews), 2015-09-02 22:59:26]

lgtm

Thanks for all the changes, the code is looking much better now!

I left you a few more nits, but otherwise the CL is looking pretty good to me.

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

https://codereview.chromium.org/1304013002/diff/110001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:344:
public void updateTopControlState() {
Move this to the delegate section below.

https://codereview.chromium.org/1304013002/diff/110001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:639:
return getManagementDelegate().shouldIgnoreNavigation(mExternalNavHandler,
Add a TODO to consider passing a boolean instead representing whether the
interception is allowed, as the first parameter.

https://codereview.chromium.org/1304013002/diff/110001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagementDelegate.java
(right):

https://codereview.chromium.org/1304013002/diff/110001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagementDelegate.java:71:
//void setSearchContentViewVisibility(boolean isVisible);
Remove this method, if not used.

https://codereview.chromium.org/1304013002/diff/110001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagementDelegate.java:177:
boolean shouldIgnoreNavigation(ExternalNavigationHandler externalNavHandler,
Nit: I think this should be called shouldInterceptNavigation, since it's more
specific. We're not just going to ignore it, we will intercept it (which means
it will be forward to somewhere else).

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

https://codereview.chromium.org/1304013002/diff/110001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:909:
mSearchContentViewDelegate.setContextualSearchContentViewCore(contentView);
Put a TODO here to move the ownership of the SearchContentViewDelegate to the
Panel, since it doesn't make sense keeping it in the CSManager anymore.

[mdjones, 2015-09-03 17:07:32]

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

https://codereview.chromium.org/1304013002/diff/110001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:344:
public void updateTopControlState() {
On 2015/09/02 22:59:25, pedrosimonetti wrote:
> Move this to the delegate section below.

Done.

https://codereview.chromium.org/1304013002/diff/110001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:639:
return getManagementDelegate().shouldIgnoreNavigation(mExternalNavHandler,
On 2015/09/02 22:59:26, pedrosimonetti wrote:
> Add a TODO to consider passing a boolean instead representing whether the
> interception is allowed, as the first parameter.

Done.

https://codereview.chromium.org/1304013002/diff/110001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagementDelegate.java
(right):

https://codereview.chromium.org/1304013002/diff/110001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagementDelegate.java:71:
//void setSearchContentViewVisibility(boolean isVisible);
On 2015/09/02 22:59:26, pedrosimonetti wrote:
> Remove this method, if not used.

Whoops. Done.

https://codereview.chromium.org/1304013002/diff/110001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagementDelegate.java:177:
boolean shouldIgnoreNavigation(ExternalNavigationHandler externalNavHandler,
On 2015/09/02 22:59:26, pedrosimonetti wrote:
> Nit: I think this should be called shouldInterceptNavigation, since it's more
> specific. We're not just going to ignore it, we will intercept it (which means
> it will be forward to somewhere else).

Done.

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

https://codereview.chromium.org/1304013002/diff/110001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:909:
mSearchContentViewDelegate.setContextualSearchContentViewCore(contentView);
On 2015/09/02 22:59:26, pedrosimonetti wrote:
> Put a TODO here to move the ownership of the SearchContentViewDelegate to the
> Panel, since it doesn't make sense keeping it in the CSManager anymore.

Done.

[mdjones, 2015-09-03 17:09:43]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2015-09-03 17:09:44]

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/1304013002/#ps130001
(title: "Address remaining comments")

[commit-bot: I haz the power, 2015-09-03 17:09:58]

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

[commit-bot: I haz the power, 2015-09-03 17:42:21]

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

[commit-bot: I haz the power, 2015-09-03 17:42:21]

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-03 21:18:42]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2015-09-03 21:18:42]

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

[commit-bot: I haz the power, 2015-09-03 21:18:58]

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

[commit-bot: I haz the power, 2015-09-03 21:51:11]

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

[commit-bot: I haz the power, 2015-09-03 21:51:11]

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-04 16:59:05]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2015-09-04 16:59:06]

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/1304013002/#ps170001
(title: "Fix proguard error")

[commit-bot: I haz the power, 2015-09-04 16:59:38]

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

[commit-bot: I haz the power, 2015-09-04 17:53:48]

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

[commit-bot: I haz the power, 2015-09-04 17:53:49]

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-04 18:28:48]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2015-09-04 18:28:48]

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/1304013002/#ps190001
(title: "Shift null checks")

[commit-bot: I haz the power, 2015-09-04 18:29:06]

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

[commit-bot: I haz the power, 2015-09-04 20:22:50]

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

[commit-bot: I haz the power, 2015-09-04 20:22:50]

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-04 21:18:55]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2015-09-04 21:18:56]

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/1304013002/#ps210001
(title: " ")

[commit-bot: I haz the power, 2015-09-04 21:19:32]

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

[commit-bot: I haz the power, 2015-09-04 22:17:06]

Committed patchset #12 (id:210001)

[commit-bot: I haz the power, 2015-09-04 22:17:40]

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