[Contextual Search] Trigger translation on long-press.

[Contextual Search] Trigger translation on long-press.

Adds translation one-box triggering when the user selects
using long-press.  This currently requires server-side
auto-detection of the source language, but it should not
over-trigger due to a feature of the one-box (suppressed
when the source and target text ends up matching).

Adds a separate flag to control disabling this feature,
currently default-enabled.

BUG=413717
Committed: https://crrev.com/8e976c6fc4b0b450396f5c87f6774bfc2d683100
Cr-Commit-Position: refs/heads/master@{#361231}

[Donn Denman, 2015-11-19 23:19:47]

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

[Donn Denman, 2015-11-19 23:19:47]

Pedro, PTAL.  I'm thinking it would be nice to merge this so M48 would have full
translate one-box capability.

[pedro (no code reviews), 2015-11-20 22:04:44]

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

https://codereview.chromium.org/1463583004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:496:
boolean doForceTranslate = !mPolicy.disableLongPressTranslationOnebox();
Couple of comments:

If mPolicy.disableForceTranslationOnebox() returns true, I would think it would
prevent all translations, including the long press one, therefore we should
check it here as well. For readability reasons, I'd suggest creating
shouldTranslateLongPressSelection() that would encapsulate the whole logic.

Another thing is that this code block will be executed for tap selections that
have reached the resolution limit. Should those tap searches be translated as
well? I'm asking because the name disableLongPressTranslationOnebox() kinda
implies this will be just for long press selections, and that's not entirely
true.

[Donn Denman, 2015-11-20 23:10:23]

Pedro, PTAL.

I also added some tests.

Thanks for the great review!

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

https://codereview.chromium.org/1463583004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:496:
boolean doForceTranslate = !mPolicy.disableLongPressTranslationOnebox();
On 2015/11/20 22:04:43, pedrosimonetti wrote:
> Couple of comments:
> 
> If mPolicy.disableForceTranslationOnebox() returns true, I would think it
would
> prevent all translations, including the long press one, therefore we should
> check it here as well. For readability reasons, I'd suggest creating
> shouldTranslateLongPressSelection() that would encapsulate the whole logic.
> 
> Another thing is that this code block will be executed for tap selections that
> have reached the resolution limit. Should those tap searches be translated as
> well? I'm asking because the name disableLongPressTranslationOnebox() kinda
> implies this will be just for long press selections, and that's not entirely
> true.

Those are both good points!

I am now checking both flags, and I think the new flag should reflect
auto-detect activation rather then the longpress gesture.  The auto-detect is
what's risky, and as you point out this will be triggered any time we don't
resolve, including when the user is on https and undecided.

[pedro (no code reviews), 2015-11-20 23:56:25]

lgtm

I have a couple of nits, but otherwise it looks good to me!

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

https://codereview.chromium.org/1463583004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:496:
boolean doForceTranslate = !mPolicy.disableAutoDetectTranslationOnebox();
Nit: rename doForceTranslate to shouldAutoDetectTranslation?

https://codereview.chromium.org/1463583004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:501:
mSearchRequest.forceTranslation("",
Nit: Similarly, consider having a new CSRequest method called
autoDetectTranslation() or something, because passing an empty string is kinda
obscure.

[Donn Denman, 2015-11-21 01:14:00]

I did some refactoring, PTAL.  Thanks for the great suggestions!

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

https://codereview.chromium.org/1463583004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:496:
boolean doForceTranslate = !mPolicy.disableAutoDetectTranslationOnebox();
On 2015/11/20 23:56:25, pedrosimonetti wrote:
> Nit: rename doForceTranslate to shouldAutoDetectTranslation?

Done.

https://codereview.chromium.org/1463583004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:501:
mSearchRequest.forceTranslation("",
On 2015/11/20 23:56:25, pedrosimonetti wrote:
> Nit: Similarly, consider having a new CSRequest method called
> autoDetectTranslation() or something, because passing an empty string is kinda
> obscure.

Done.

[Donn Denman, 2015-11-23 22:44:48]

Pedro, I'd like you to take another look, since I did some non-trivial
refactoring.  Thanks for the review!

[pedro (no code reviews), 2015-11-23 23:42:25]

lgtm

https://chromiumcodereview.appspot.com/1463583004/diff/60001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java
(right):

https://chromiumcodereview.appspot.com/1463583004/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java:479:
if (ContextualSearchFieldTrial.disableForceTranslationOnebox()) return true;
Call method disableForceTranslationOnebox above.

[Donn Denman, 2015-11-23 23:53:33]

Thanks Pedro!

https://chromiumcodereview.appspot.com/1463583004/diff/60001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java
(right):

https://chromiumcodereview.appspot.com/1463583004/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java:479:
if (ContextualSearchFieldTrial.disableForceTranslationOnebox()) return true;
On 2015/11/23 23:42:25, pedrosimonetti wrote:
> Call method disableForceTranslationOnebox above.

Done.

[Donn Denman, 2015-11-23 23:53:39]

The CQ bit was checked by donnd@chromium.org

[Donn Denman, 2015-11-23 23:53:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
pedrosimonetti@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/1463583004/#ps80001
(title: "Tiny tweak.")

[commit-bot: I haz the power, 2015-11-23 23:55:59]

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

[commit-bot: I haz the power, 2015-11-24 01:19:02]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-11-24 01:19:44]

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