Allow choosing target language in more languages menu(translate infobar)

Allow choosing target language in more languages menu(translate infobar)

Allow choosing target language in the source language list.

1. When user chooses target language in the source language list,
   The infobar will dismiss, and no translation takes place.
   This will not be regarded as a "translation declined" and it
   will not triggered auto-never or increment the declined counter.

2. User still cannot choose source language in target language list.

BUG=734654
Review-Url: https://codereview.chromium.org/2951963002
Cr-Commit-Position: refs/heads/master@{#483906}
Committed: https://chromium.googlesource.com/chromium/src/+/f1bc64b52ad0ed1ea64baf2e29e37e0bc4575f59

[Marti Wong, 2017-06-21 06:00:26]

Patchset #1 (id:1) has been deleted

[Marti Wong, 2017-06-21 06:02:01]

Description was changed from

==========
Allow choosing target language in more languages menu(translate infobar)

Allow choosing target language in the source language list.

1. When user chooses target language in the source language list,
   The infobar will dismiss, and no translation takes place.

2. User still cannot choose source language in target language list.

BUG=734654
==========

to

==========
Allow choosing target language in more languages menu(translate infobar)

Allow choosing target language in the source language list.

1. When user chooses target language in the source language list,
   The infobar will dismiss, and no translation takes place.
   This will not be regarded as a "translation declined" and it
   will not triggered auto-never or increment the declined counter.

2. User still cannot choose source language in target language list.

BUG=734654
==========

[Marti Wong, 2017-06-21 06:23:23]

martiw@chromium.org changed reviewers:
+ googleo@chromium.org

[Marti Wong, 2017-06-21 06:23:24]

https://codereview.chromium.org/2951963002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java
(right):

https://codereview.chromium.org/2951963002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:417:
closeInfobar(true);
The closing of infobar will not be regarded as a 'declined' because...

At this point, mMenuExpanded is true, and...
  closeInfobar(true) --> 
  TranslateCompactInfoBar#ShouldAutoNeverTranslate() -->
  action_flags_ |= FLAG_EXPAND_MENU; -->
  TranslateCompactInfoBar#IsDeclinedByUser() will always return false

That makes sure auto-never will not be triggered.

[Leo, 2017-06-21 06:37:01]

Thanks for the change, have a question about the same source.

https://codereview.chromium.org/2951963002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenuHelper.java
(right):

https://codereview.chromium.org/2951963002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenuHelper.java:71:
code = mOptions.allLanguages().get(i).mLanguageCode;
what will happen if user select the same language as source?

https://codereview.chromium.org/2951963002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenuHelper.java:78:
// Don't show source language in the target language list.
If we remove current source from source list and remove current target from
target list.

You just need to modify the logic inside the loop.

if (((menuType == TranslateMenu.MEMU_TARGET_LANGUAGE ) &&
code.equals(mOptions.targetLanguageCode()) 
|| ((menuType == TranslateMenu.MENU_SOURCE_LANGUAGE ) &&
code.equals(mOptions.sourceLanguageCode())) {
  continue;
}

[Marti Wong, 2017-06-21 07:16:23]

Thanks Leo!  PTAL again.

https://codereview.chromium.org/2951963002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenuHelper.java
(right):

https://codereview.chromium.org/2951963002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenuHelper.java:71:
code = mOptions.allLanguages().get(i).mLanguageCode;
On 2017/06/21 06:37:01, Leo wrote:
> what will happen if user select the same language as source?

Thanks for the catch.
Yes, I forgot to remove source language from source language list.
will fix this.

https://codereview.chromium.org/2951963002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenuHelper.java:78:
// Don't show source language in the target language list.
On 2017/06/21 06:37:01, Leo wrote:
> If we remove current source from source list and remove current target from
> target list.
> 
> You just need to modify the logic inside the loop.
> 
> if (((menuType == TranslateMenu.MEMU_TARGET_LANGUAGE ) &&
> code.equals(mOptions.targetLanguageCode()) 
> || ((menuType == TranslateMenu.MENU_SOURCE_LANGUAGE ) &&
> code.equals(mOptions.sourceLanguageCode())) {
>   continue;
> }

Thanks.  I done similar change.
avoid the source language in both source and target list.
while
avoid the target language only in the target list.

[Leo, 2017-06-21 07:58:50]

lgtm

[Marti Wong, 2017-06-21 08:09:08]

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

[Marti Wong, 2017-06-21 08:09:09]

Hi Matthew, PTAL.  thx!

[mdjones, 2017-06-30 16:55:58]

lgtm % nit

https://codereview.chromium.org/2951963002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenuHelper.java
(right):

https://codereview.chromium.org/2951963002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenuHelper.java:68:
String code;
nit: Move this back into the for-loop. Java won't create a new object each time
unless you change the string.

[Marti Wong, 2017-07-01 06:15:24]

Thanks Matthew.  CL committing...

https://codereview.chromium.org/2951963002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenuHelper.java
(right):

https://codereview.chromium.org/2951963002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenuHelper.java:68:
String code;
On 2017/06/30 16:55:58, mdjones wrote:
> nit: Move this back into the for-loop. Java won't create a new object each
time
> unless you change the string.

Done. thanks for the tip.

[Marti Wong, 2017-07-01 06:15:28]

The CQ bit was checked by martiw@chromium.org

[Marti Wong, 2017-07-01 06:15:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
googleo@chromium.org, mdjones@chromium.org
Link to the patchset: https://codereview.chromium.org/2951963002/#ps60001
(title: "fix")

[commit-bot: I haz the power, 2017-07-01 06:15:31]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-01 07:01:00]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1498889728853720,
"parent_rev": "716a39de127774035bcba8fec1b9c4cd93f629d5", "commit_rev":
"f1bc64b52ad0ed1ea64baf2e29e37e0bc4575f59"}

[commit-bot: I haz the power, 2017-07-01 07:01:15]

Description was changed from

==========
Allow choosing target language in more languages menu(translate infobar)

Allow choosing target language in the source language list.

1. When user chooses target language in the source language list,
   The infobar will dismiss, and no translation takes place.
   This will not be regarded as a "translation declined" and it
   will not triggered auto-never or increment the declined counter.

2. User still cannot choose source language in target language list.

BUG=734654
==========

to

==========
Allow choosing target language in more languages menu(translate infobar)

Allow choosing target language in the source language list.

1. When user chooses target language in the source language list,
   The infobar will dismiss, and no translation takes place.
   This will not be regarded as a "translation declined" and it
   will not triggered auto-never or increment the declined counter.

2. User still cannot choose source language in target language list.

BUG=734654

Review-Url: https://codereview.chromium.org/2951963002
Cr-Commit-Position: refs/heads/master@{#483906}
Committed:
https://chromium.googlesource.com/chromium/src/+/f1bc64b52ad0ed1ea64baf2e29e3...
==========

[commit-bot: I haz the power, 2017-07-01 07:01:16]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/f1bc64b52ad0ed1ea64baf2e29e3...

[Marti Wong, 2017-07-01 08:16:29]

A revert of this CL (patchset #3 id:60001) has been created in
https://codereview.chromium.org/2964233003/ by martiw@chromium.org.

The reason for reverting is: buildbot failure in chromium.win on Win x64 Builder
(dbg)
And this is the CL being blamed.
I will revert this CL first although it seems nothing to do with the failure..

[Marti Wong, 2017-07-02 04:07:31]

On 2017/07/01 08:16:29, Marti Wong wrote:
> A revert of this CL (patchset #3 id:60001) has been created in
> https://codereview.chromium.org/2964233003/ by mailto:martiw@chromium.org.
> 
> The reason for reverting is: buildbot failure in chromium.win on Win x64
Builder
> (dbg)
> And this is the CL being blamed.
> I will revert this CL first although it seems nothing to do with the failure..

Tree is open and this CL is unrelated to the windows compilation error.
I didn't do the revert and closed it.