[Contextual Search] Use Mojo API for Quick Answers.

[Contextual Search] Use Mojo API for Quick Answers.

Update the Contextual Search manager to use the new Contextual Search component for mojo communication.

Still TODO: put the answer in the Bar.

BUG=523554
Committed: https://crrev.com/8f3387ec549ec8b5d0f20f652a5e93179b65252c
Cr-Commit-Position: refs/heads/master@{#368963}

[Donn Denman, 2015-12-16 22:06:00]

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

[Donn Denman, 2015-12-16 22:06:00]

Pedro, PTAL at this CL.

You did a review a while ago for a CL that included these changes.  Since then I
split it into 2, the CS component has now landed, and this CL uses the
component.  Here's one of the files from that review, in case that helps you
gain context:
https://codereview.chromium.org/1385663002/diff/80001/chrome/android/java/src...

[pedro (no code reviews), 2015-12-22 23:15:39]

Donn, sorry for the delay on this one. Here are some comments. Since I will be
OOO for some time, I'd like to ask you to ask another person to finish reviewing
this code.

Overall it looks fine for me, but I'm not familiar with using Mojo, so it would
be nice having others looking at this change too.

https://codereview.chromium.org/1532743002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java
(right):

https://codereview.chromium.org/1532743002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java:387:
boolean shouldEnableOverlayApi() {
Nit: I would call it shouldEnableJavaScriptApi() or isJavaScriptApiEnabled(),
since "overlay" is not a quality of the Api. Another option would be
shouldEnableWebContentApi().

https://codereview.chromium.org/1532743002/diff/1/chrome/browser/android/cont...
File chrome/browser/android/contextualsearch/contextual_search_manager.h
(right):

https://codereview.chromium.org/1532743002/diff/1/chrome/browser/android/cont...
chrome/browser/android/contextualsearch/contextual_search_manager.h:63: void
AddViewForContextualSearchApi(JNIEnv* env,
Nit: It would be nice being consistent with the terminology. If you use
"JavaScriptApi" in the CSPolicy method, then you should call it the same here,
like AddViewForJavaScriptApi(), for example.

[Donn Denman, 2016-01-08 18:59:08]

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

[Donn Denman, 2016-01-08 18:59:09]

Matt, PTAL at the Mojo portions of this CL.

I took Pedro's suggestion to heart and renamed the interface defined by our
component for the JavaScript API to be compatible all the way through our code. 
That should help connect the dots between our (new) component, and the rest of
Contextual Search browser code.

TIA!

https://chromiumcodereview.appspot.com/1532743002/diff/1/chrome/browser/andro...
File chrome/browser/android/contextualsearch/contextual_search_manager.h
(right):

https://chromiumcodereview.appspot.com/1532743002/diff/1/chrome/browser/andro...
chrome/browser/android/contextualsearch/contextual_search_manager.h:63: void
AddViewForContextualSearchApi(JNIEnv* env,
On 2015/12/22 23:15:39, pedrosimonetti (OOO) wrote:
> Nit: It would be nice being consistent with the terminology. If you use
> "JavaScriptApi" in the CSPolicy method, then you should call it the same here,
> like AddViewForJavaScriptApi(), for example.

Done.

[mdjones, 2016-01-09 00:59:38]

See comment, otherwise mojo lgtm.

https://codereview.chromium.org/1532743002/diff/20001/chrome/browser/android/...
File chrome/browser/android/contextualsearch/contextual_search_manager.h
(right):

https://codereview.chromium.org/1532743002/diff/20001/chrome/browser/android/...
chrome/browser/android/contextualsearch/contextual_search_manager.h:66: void
AddViewForContextualSearchJsApi(JNIEnv* env,
I would consider renaming to something like
"enableJSApiFor[ContentViewCore|Page](...)" as the ContentViewCore view isn't
really being added to anything.

[Donn Denman, 2016-01-11 20:31:33]

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

[Donn Denman, 2016-01-11 20:31:34]

Thanks Matt!

https://chromiumcodereview.appspot.com/1532743002/diff/20001/chrome/browser/a...
File chrome/browser/android/contextualsearch/contextual_search_manager.h
(right):

https://chromiumcodereview.appspot.com/1532743002/diff/20001/chrome/browser/a...
chrome/browser/android/contextualsearch/contextual_search_manager.h:66: void
AddViewForContextualSearchJsApi(JNIEnv* env,
On 2016/01/09 00:59:38, mdjones wrote:
> I would consider renaming to something like
> "enableJSApiFor[ContentViewCore|Page](...)" as the ContentViewCore view isn't
> really being added to anything.

Done.

[Donn Denman, 2016-01-11 20:36:52]

donnd@chromium.org changed reviewers:
+ twellington@chromium.org

[Donn Denman, 2016-01-11 20:36:52]

Theresa, PTAL.

In case you want to patch this in and test it, I usually open an overlay,
inspect it, and type into the console:
chrome.contextualSearch.setCaption('YES!', true);
and look for a log line in the output that says the caption was set (code in
ContextualSearchManager#onSetCaption).

We should have an almost-end-to-end way to activate it soon.
 
Ping me if you have questions!

[Theresa, 2016-01-12 18:46:33]

lgtm w/ one nit and one question

https://chromiumcodereview.appspot.com/1532743002/diff/40001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java
(right):

https://chromiumcodereview.appspot.com/1532743002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:766:
// TODO(donnd): notify the UX of the caption and log doesAnswer.
nit: s/UX/UI

https://chromiumcodereview.appspot.com/1532743002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:1018:
// TODO(donnd): Consider moving to OverlayPanelContent.
Is the caption useful for overlays in general or just CS? If it were moved,
would the contextual search methods we call below get moved somewhere too?

[Donn Denman, 2016-01-12 18:58:25]

Thanks Theresa!

https://chromiumcodereview.appspot.com/1532743002/diff/40001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java
(right):

https://chromiumcodereview.appspot.com/1532743002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:766:
// TODO(donnd): notify the UX of the caption and log doesAnswer.
On 2016/01/12 18:46:33, Theresa Wellington wrote:
> nit: s/UX/UI

Done.

https://chromiumcodereview.appspot.com/1532743002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:1018:
// TODO(donnd): Consider moving to OverlayPanelContent.
On 2016/01/12 18:46:33, Theresa Wellington wrote:
> Is the caption useful for overlays in general or just CS? If it were moved,
> would the contextual search methods we call below get moved somewhere too?

I'm assuming that the caption could be useful for overlays in general.  However,
the code here just enables the Contextual Search API to trigger the caption, so
I don't think it applies to any overlay, which is why I didn't actually move the
code to OverlayPanelContent.  Pedro originally suggested it be moved, but I
think we need to revisit that discussion when he returns in order to fully
resolve this question.

[Donn Denman, 2016-01-12 18:58:28]

The CQ bit was checked by donnd@chromium.org

[Donn Denman, 2016-01-12 18:58:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
mdjones@chromium.org, twellington@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/1532743002/#ps60001
(title: "Updated a comment.")

[commit-bot: I haz the power, 2016-01-12 19:00:21]

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

[commit-bot: I haz the power, 2016-01-12 20:07:25]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-01-12 20:08:06]

Description was changed from

==========
[Contextual Search] Use Mojo API for Quick Answers.

Update the Contextual Search manager to use the new Contextual Search component
for mojo communication.

Still TODO: put the answer in the Bar.

BUG=523554
==========

to

==========
[Contextual Search] Use Mojo API for Quick Answers.

Update the Contextual Search manager to use the new Contextual Search component
for mojo communication.

Still TODO: put the answer in the Bar.

BUG=523554

Committed: https://crrev.com/8f3387ec549ec8b5d0f20f652a5e93179b65252c
Cr-Commit-Position: refs/heads/master@{#368963}
==========

[commit-bot: I haz the power, 2016-01-12 20:08:08]

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