Contextual Search Panel should own ContentViewCore

Contextual Search Panel should own ContentViewCore

This change moves ownership of the contextual search ContentViewCore
to the panel rather than the manager. Currently the panel delegate
exposes methods to retain functionality in the manager; this should
only be temporary.

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

[mdjones, 2015-08-17 21:50:00]

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

[pedro (no code reviews), 2015-08-18 21:30:47]

lgtm

You will still need an OWNERS review, but the code LGTM.

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

https://codereview.chromium.org/1283223004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:58:
private ContentViewCore mContentViewCore;
Nit: Although not mandatory, including JavaDoc comments are appreciated, for
consistency reasons.

https://codereview.chromium.org/1283223004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:521:

Nit: Let's put a separator here (// =============...) with something written to
indicate this is where all contentview stuff should be located, for now, just so
it will be simpler to move things in the future.

https://codereview.chromium.org/1283223004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:539:
public void resetContentViewCore() {
Nit: Put a TODO here to encapsulate all destroy-related things in a single
method. I believe we won't need all these individual methods/interfaces.

[mdjones, 2015-08-19 00:19:47]

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

[mdjones, 2015-08-19 00:19:47]

+dtrainor for owner approval

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

https://codereview.chromium.org/1283223004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:58:
private ContentViewCore mContentViewCore;
On 2015/08/18 21:30:47, pedrosimonetti wrote:
> Nit: Although not mandatory, including JavaDoc comments are appreciated, for
> consistency reasons.

Done.

https://codereview.chromium.org/1283223004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:521:

On 2015/08/18 21:30:47, pedrosimonetti wrote:
> Nit: Let's put a separator here (// =============...) with something written
to
> indicate this is where all contentview stuff should be located, for now, just
so
> it will be simpler to move things in the future.

Done.

https://codereview.chromium.org/1283223004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:539:
public void resetContentViewCore() {
On 2015/08/18 21:30:46, pedrosimonetti wrote:
> Nit: Put a TODO here to encapsulate all destroy-related things in a single
> method. I believe we won't need all these individual methods/interfaces.

Done.

[David Trainor- moved to gerrit, 2015-08-19 20:17:02]

a few nits.  If this is an intermediate CL we can worry about making the
interface foolproof in a follow up :).

https://codereview.chromium.org/1283223004/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/1283223004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:572:
nativeDestroyWebContents(mNativeContextualSearchPanelPtr);
Some of this state seems a bit scary.  Ie: it's possible to call
destroyWebContents then still hold on to mContentViewCore which is actually
backed by nothing.  Also resetContentViewCore drops the content view core
reference but still holds the web contents reference natively.
We should try to make the interface a bit more foolproof imo!.

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

https://codereview.chromium.org/1283223004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:787:
mSearchPanelDelegate.removeLastHistoryEntry(mSearchRequest.getSearchUrl(),
This also feels a little weird for that panel.  Will reader mode/other people be
doing this?  Maybe keep this here as a static call?

The only annoying thing is that you need a CancelableTaskTracker.  So maybe this
can't be static :(.

https://codereview.chromium.org/1283223004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:942:
mSearchPanelDelegate.setInterceptNavigationDelegate(new
InterceptNavigationDelegateImpl(),
Maybe we should make this a method on WebContents (java -> native) since we
already have a java class for it.

[David Trainor- moved to gerrit, 2015-08-19 21:20:04]

lgtm as long as we have a follow up on moving over functionality and reducing
the surface area of interface.

[mdjones, 2015-08-20 18:03:17]

https://codereview.chromium.org/1283223004/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/1283223004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:572:
nativeDestroyWebContents(mNativeContextualSearchPanelPtr);
On 2015/08/19 20:17:02, David Trainor wrote:
> Some of this state seems a bit scary.  Ie: it's possible to call
> destroyWebContents then still hold on to mContentViewCore which is actually
> backed by nothing.  Also resetContentViewCore drops the content view core
> reference but still holds the web contents reference natively.
> We should try to make the interface a bit more foolproof imo!.

Most of these exposed native methods should be temporary as I move code into the
panel.

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

https://codereview.chromium.org/1283223004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:942:
mSearchPanelDelegate.setInterceptNavigationDelegate(new
InterceptNavigationDelegateImpl(),
On 2015/08/19 20:17:02, David Trainor wrote:
> Maybe we should make this a method on WebContents (java -> native) since we
> already have a java class for it.

As per our discussion, this is not a method of WebContents in native.

[mdjones, 2015-08-20 18:03:22]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2015-08-20 18:03:22]

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/1283223004/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2015-08-20 18:05:11]

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

[commit-bot: I haz the power, 2015-08-20 23:30:39]

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

[commit-bot: I haz the power, 2015-08-20 23:30:40]

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-08-21 16:56:34]

The CQ bit was checked by mdjones@chromium.org

[commit-bot: I haz the power, 2015-08-21 16:57:15]

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

[commit-bot: I haz the power, 2015-08-21 20:22:12]

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

[commit-bot: I haz the power, 2015-08-21 20:22: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-08-21 21:26:11]

I added MockContextualSearchPanel that overrides the native panel methods that
are unsupported in the tests. Please take a quick look.

[pedro (no code reviews), 2015-08-21 21:32:47]

lgtm

I would prefer keeping the native methods private, but we can make that once we
encapsulate all ContentView logic in a separate class. So, I'm okay with the
changes, assuming we will fix this soon in a followup CL.

[David Trainor- moved to gerrit, 2015-08-22 05:05:38]

lgtm

https://codereview.chromium.org/1283223004/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/1283223004/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:588:
protected native long nativeInit();
I haven't seen us do this anywhere actually!  Interesting... I would have
recommended mocking out the calls to these native ones, not the native calls in
particular, but if this works... :)

[mdjones, 2015-08-24 15:32:32]

https://codereview.chromium.org/1283223004/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/1283223004/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:588:
protected native long nativeInit();
On 2015/08/22 05:05:37, David Trainor wrote:
> I haven't seen us do this anywhere actually!  Interesting... I would have
> recommended mocking out the calls to these native ones, not the native calls
in
> particular, but if this works... :)

The reason I chose this route is because native currently has little
functionality that affects the java class. Some of the calls are buried in other
methods and would require an extra wrapper layer to mock (setNativeWebContents).
Currently these methods have that wrapper, but they are only supposed to be a
temporary way for the manager to access the panel.

[mdjones, 2015-08-24 18:27:48]

The CQ bit was checked by mdjones@chromium.org

[commit-bot: I haz the power, 2015-08-24 18:28:27]

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

[commit-bot: I haz the power, 2015-08-24 19:13:32]

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

[commit-bot: I haz the power, 2015-08-24 19:13:33]

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-08-25 20:25:22]

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

[mdjones, 2015-08-25 20:25:23]

+mnaganov for trivial/temporary inclusion of WebContentsDelegateAndroid in
ContextualSearchEventFilterTest.java

[mnaganov (inactive), 2015-08-25 20:29:15]

lgtm

[mdjones, 2015-08-25 22:55:16]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2015-08-25 22:55:17]

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/1283223004/#ps140001
(title: "Override wrappers instead of native")

[commit-bot: I haz the power, 2015-08-25 22:55:59]

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

[commit-bot: I haz the power, 2015-08-25 23:42:30]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-08-25 23:43:33]

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