Automatically trigger Always-Translate in new translation infobar

Automatically trigger Always-Translate
In the new translation infobar, when the number consecutive translations
reaches a threshold, it will trigger Always-Translate automatically.
This CL is trying to implement this feature without altering the
behavior of the existing infobars.

BUG=703887
Review-Url: https://codereview.chromium.org/2871783003
Cr-Commit-Position: refs/heads/master@{#471196}
Committed: https://chromium.googlesource.com/chromium/src/+/8628a9e95249d1900319f645beb2bbe3b055ff75

[Marti Wong, 2017-05-09 04:17:48]

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

[Marti Wong, 2017-05-09 04:17:48]

PTAL. thanks!

[Marti Wong, 2017-05-09 06:53:07]

martiw@chromium.org changed reviewers:
+ dfalcantara@chromium.org, groby@chromium.org

[Marti Wong, 2017-05-09 06:53:08]

PTAL.  thanks~!

https://codereview.chromium.org/2871783003/diff/1/components/translate/core/b...
File components/translate/core/browser/translate_infobar_delegate.cc (right):

https://codereview.chromium.org/2871783003/diff/1/components/translate/core/b...
components/translate/core/browser/translate_infobar_delegate.cc:385: if
(IsCompactUIEnabled())
In Compact UI, the declined is only counted during ProcessButton(ACTION_CANCEL)
(in translate_compact_infobar.cc).

[Marti Wong, 2017-05-10 08:26:49]

Hi Matthew,
PTAL.  
thank you again~!

[Marti Wong, 2017-05-10 08:27:13]

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

[Marti Wong, 2017-05-10 08:27:13]

Hi Matthew,
PTAL.
thank you again~!

[groby-ooo-7-16, 2017-05-10 14:24:11]

On 2017/05/10 08:27:13, Marti Wong wrote:
> Hi Matthew,
> PTAL.
> thank you again~!

Any chance we can do this in the TranslateUIDelegate instead? We're trying to
centralize all the logic independent of UI.

[mdjones, 2017-05-10 17:19:25]

lgtm

[Marti Wong, 2017-05-11 00:35:41]

On 2017/05/10 14:24:11, groby wrote:
> On 2017/05/10 08:27:13, Marti Wong wrote:
> > Hi Matthew,
> > PTAL.
> > thank you again~!
> 
> Any chance we can do this in the TranslateUIDelegate instead? We're trying to
> centralize all the logic independent of UI.

Thanks Rachel.
It seems we will do auto-always behavior for Android only (and possibly iOS
later).
TranslateUIDelegate might not be a good place.
Do you think I should move the logic (kAcceptCountThreshold,
ShouldAutoAlwaysTranslate) to TranslateInfobarDelegate instead?

[Marti Wong, 2017-05-11 03:54:59]

Description was changed from

==========
Automatically trigger Always-Translate
In the new translation infobar, when the number consecutive translations
reaches a threshold, it will trigger Always-Translate automatically.
This CL is trying to implement this feature without altering the
behavior of the existing infobars.

BUG=703887
==========

to

==========
Automatically trigger Always-Translate
In the new translation infobar, when the number consecutive translations
reaches a threshold, it will trigger Always-Translate automatically.
This CL is trying to implement this feature without altering the
behavior of the existing infobars.

BUG=703887
==========

[Marti Wong, 2017-05-11 03:54:59]

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

[Marti Wong, 2017-05-11 04:48:51]

Hi Rachel,

I moved the logic to TranslateInfoBarDelegate.
At least it centralizes the logic for infobars.
PTAL see if it is okay.

If not, I can revert back and try other way.
Thank you~!

[groby1, 2017-05-11 14:39:48]

On 2017/05/11 04:48:51, Marti Wong wrote:
> Hi Rachel,
> 
> I moved the logic to TranslateInfoBarDelegate.
> At least it centralizes the logic for infobars.
> PTAL see if it is okay.
> 
> If not, I can revert back and try other way.
> Thank you~!

I think that should work. (Centralized would be better, but that's a larger
refactor, I think. Out of scope for this CL)

I can haz tests?

[Marti Wong, 2017-05-12 00:57:02]

On 2017/05/11 14:39:48, groby1 wrote:
> On 2017/05/11 04:48:51, Marti Wong wrote:
> > Hi Rachel,
> > 
> > I moved the logic to TranslateInfoBarDelegate.
> > At least it centralizes the logic for infobars.
> > PTAL see if it is okay.
> > 
> > If not, I can revert back and try other way.
> > Thank you~!
> 
> I think that should work. (Centralized would be better, but that's a larger
> refactor, I think. Out of scope for this CL)
> 
> I can haz tests?

Hi Rachel,
thanks for the advice!
It seems there is no translate_infobar_delegate_unittest.cc yet.  To create it,
I think I'll need to take a while to learn how.
As this CL is blocking other features like Auto-Never, logging.  And this CL is
not a risky change.
Do you think it's appropriate to commit this first and deal with the test later?

Also, Leo is thinking to refactor the native codes.  After that, the unittest
may need to be refactored or rewrited later.
May be we could make the test at that time.

[groby-ooo-7-16, 2017-05-12 01:01:58]

On 2017/05/12 00:57:02, Marti Wong wrote:
> On 2017/05/11 14:39:48, groby1 wrote:
> > On 2017/05/11 04:48:51, Marti Wong wrote:
> > > Hi Rachel,
> > > 
> > > I moved the logic to TranslateInfoBarDelegate.
> > > At least it centralizes the logic for infobars.
> > > PTAL see if it is okay.
> > > 
> > > If not, I can revert back and try other way.
> > > Thank you~!
> > 
> > I think that should work. (Centralized would be better, but that's a larger
> > refactor, I think. Out of scope for this CL)
> > 
> > I can haz tests?
> 
> Hi Rachel,
> thanks for the advice!
> It seems there is no translate_infobar_delegate_unittest.cc yet.  To create
it,
> I think I'll need to take a while to learn how.
> As this CL is blocking other features like Auto-Never, logging.  And this CL
is
> not a risky change.
> Do you think it's appropriate to commit this first and deal with the test
later?
> 
> Also, Leo is thinking to refactor the native codes.  After that, the unittest
> may need to be refactored or rewrited later.
> May be we could make the test at that time.

I'm normally opposed to doing tests later, but... TranslateInfoBarDelegate takes
a few difficult dependencies to create, and there is no unittest for it that you
could base it on. So, yes, LGTM - but please try to add a test as soon as
possible.

[Marti Wong, 2017-05-12 01:14:56]

On 2017/05/12 01:01:58, groby wrote:
> On 2017/05/12 00:57:02, Marti Wong wrote:
> > On 2017/05/11 14:39:48, groby1 wrote:
> > > On 2017/05/11 04:48:51, Marti Wong wrote:
> > > > Hi Rachel,
> > > > 
> > > > I moved the logic to TranslateInfoBarDelegate.
> > > > At least it centralizes the logic for infobars.
> > > > PTAL see if it is okay.
> > > > 
> > > > If not, I can revert back and try other way.
> > > > Thank you~!
> > > 
> > > I think that should work. (Centralized would be better, but that's a
larger
> > > refactor, I think. Out of scope for this CL)
> > > 
> > > I can haz tests?
> > 
> > Hi Rachel,
> > thanks for the advice!
> > It seems there is no translate_infobar_delegate_unittest.cc yet.  To create
> it,
> > I think I'll need to take a while to learn how.
> > As this CL is blocking other features like Auto-Never, logging.  And this CL
> is
> > not a risky change.
> > Do you think it's appropriate to commit this first and deal with the test
> later?
> > 
> > Also, Leo is thinking to refactor the native codes.  After that, the
unittest
> > may need to be refactored or rewrited later.
> > May be we could make the test at that time.
> 
> I'm normally opposed to doing tests later, but... TranslateInfoBarDelegate
takes
> a few difficult dependencies to create, and there is no unittest for it that
you
> could base it on. So, yes, LGTM - but please try to add a test as soon as
> possible.

Thanks Rachel,
I will do the test next week after I finished Auto-Never.

[Marti Wong, 2017-05-12 01:17:25]

The CQ bit was checked by martiw@chromium.org

[Marti Wong, 2017-05-12 01:17:26]

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

[commit-bot: I haz the power, 2017-05-12 01:18:29]

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-05-12 02:51:16]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494551845825990,
"parent_rev": "96d623e0e8954edf9c9908ff5ccfa9580b4946a7", "commit_rev":
"8628a9e95249d1900319f645beb2bbe3b055ff75"}

[commit-bot: I haz the power, 2017-05-12 02:51:26]

Description was changed from

==========
Automatically trigger Always-Translate
In the new translation infobar, when the number consecutive translations
reaches a threshold, it will trigger Always-Translate automatically.
This CL is trying to implement this feature without altering the
behavior of the existing infobars.

BUG=703887
==========

to

==========
Automatically trigger Always-Translate
In the new translation infobar, when the number consecutive translations
reaches a threshold, it will trigger Always-Translate automatically.
This CL is trying to implement this feature without altering the
behavior of the existing infobars.

BUG=703887

Review-Url: https://codereview.chromium.org/2871783003
Cr-Commit-Position: refs/heads/master@{#471196}
Committed:
https://chromium.googlesource.com/chromium/src/+/8628a9e95249d1900319f645beb2...
==========

[commit-bot: I haz the power, 2017-05-12 02:51:28]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/8628a9e95249d1900319f645beb2...