Translate: Hide the bubble if the user denies translating 2 times within 24 hours

Translate: Hide the bubble if the user denies translating 2 times within 24 hours

This CL hides the bubble if the user denies the translation like
cliking 'Nope' or clicking somewhere outside of the bubble two times
within 24 hours. The state is saved at the (Translate) Preferences and
synced. If the state goes to the 'hide' mode, Chrome won't show the
Translate bubble automatically any more.

In an incognito window, the bubble won't be shown automatically any
more.

BUG=379035
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=279588

[hajimehoshi, 2014-06-19 11:38:09]

PTAL

sky: browser_view.cc
toyoshim: the others

[sky, 2014-06-19 15:46:46]

https://codereview.chromium.org/345743002/diff/1/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/345743002/diff/1/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/browser_view.cc:1170: if (IsOffTheRecord())
This code is not platform specific. Can it be moved to non-platform specific
location? Perhaps browser_commands.cc:Translate?

[hajimehoshi, 2014-06-20 11:38:45]

+droger (CC)

Thank you!

https://codereview.chromium.org/345743002/diff/1/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/345743002/diff/1/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/browser_view.cc:1170: if (IsOffTheRecord())
On 2014/06/19 15:46:46, sky wrote:
> This code is not platform specific. Can it be moved to non-platform specific
> location? Perhaps browser_commands.cc:Translate?

Done. Moved to ChromeTranslateClient.

BrowserCommands is now used only when clicking the translate icon. Clicking icon
calls ShowTranslateBubble directly for now. The other triggers (the context menu
and automatic translation) uses ChromeTranslateClient. It seems better to unify
these ways to use ChromeTranslateClient.

[droger, 2014-06-20 12:09:29]

Some nits:

https://codereview.chromium.org/345743002/diff/60001/components/translate/cor...
File components/translate/core/browser/translate_prefs.h (right):

https://codereview.chromium.org/345743002/diff/60001/components/translate/cor...
components/translate/core/browser/translate_prefs.h:92: void
UpdateBubbleLastClosedTime();
Can we have comments for these methods?

https://codereview.chromium.org/345743002/diff/60001/components/translate/cor...
components/translate/core/browser/translate_prefs.h:93: bool IsBubbleHidden()
const;
This class is used both by infobars and bubbles, and does not mention any
particular UI type.
I'd prefer more generic names. Example: UpdateTranslationDialogClosedTime() and
IsTranslationDialogHidden().

https://codereview.chromium.org/345743002/diff/60001/components/translate/cor...
File components/translate/core/browser/translate_ui_delegate.cc (right):

https://codereview.chromium.org/345743002/diff/60001/components/translate/cor...
components/translate/core/browser/translate_ui_delegate.cc:183:
prefs_->UpdateBubbleLastClosedTime();
Nit: While we are at it, we should also change the word "infobar" on line 187 to
be more generic. Examples: "UI", "prompt" or "dialog".

[hajimehoshi, 2014-06-20 14:15:46]

https://codereview.chromium.org/345743002/diff/60001/components/translate/cor...
File components/translate/core/browser/translate_prefs.h (right):

https://codereview.chromium.org/345743002/diff/60001/components/translate/cor...
components/translate/core/browser/translate_prefs.h:93: bool IsBubbleHidden()
const;
On 2014/06/20 12:09:29, droger wrote:
> This class is used both by infobars and bubbles, and does not mention any
> particular UI type.
> I'd prefer more generic names. Example: UpdateTranslationDialogClosedTime()
and
> IsTranslationDialogHidden().

Hmm, the confusing thing is that the new feature to hide the UI is applied only
for bubble. Bubble won't be shown automatically forever with two or more
denials, but we don't change Infobar's behavior with this CL.

[Takashi Toyoshima, 2014-06-23 11:13:31]

https://codereview.chromium.org/345743002/diff/60001/components/translate/cor...
File components/translate/core/browser/translate_prefs.h (right):

https://codereview.chromium.org/345743002/diff/60001/components/translate/cor...
components/translate/core/browser/translate_prefs.h:93: bool IsBubbleHidden()
const;
As droger said, having bubble UI specific functions inside this class is a kind
of layering violation. We should not mention a specific UI implementation here.
So even these function are used only by bubble UI, we should use abstracted
names here.

[hajimehoshi, 2014-06-24 06:17:54]

Thank you!

https://codereview.chromium.org/345743002/diff/60001/components/translate/cor...
File components/translate/core/browser/translate_prefs.h (right):

https://codereview.chromium.org/345743002/diff/60001/components/translate/cor...
components/translate/core/browser/translate_prefs.h:93: bool IsBubbleHidden()
const;
On 2014/06/23 11:13:30, Takashi Toyoshima (chromium) wrote:
> As droger said, having bubble UI specific functions inside this class is a
kind
> of layering violation. We should not mention a specific UI implementation
here.
> So even these function are used only by bubble UI, we should use abstracted
> names here.

OK, I renamed them to UpdateLastDeniedTime and IsTooOftenDenied. Done.

https://codereview.chromium.org/345743002/diff/60001/components/translate/cor...
File components/translate/core/browser/translate_ui_delegate.cc (right):

https://codereview.chromium.org/345743002/diff/60001/components/translate/cor...
components/translate/core/browser/translate_ui_delegate.cc:183:
prefs_->UpdateBubbleLastClosedTime();
On 2014/06/20 12:09:29, droger wrote:
> Nit: While we are at it, we should also change the word "infobar" on line 187
to
> be more generic. Examples: "UI", "prompt" or "dialog".

Done.

[droger, 2014-06-24 08:16:41]

lgtm

[hajimehoshi, 2014-06-24 08:31:44]

The CQ bit was checked by hajimehoshi@chromium.org

[commit-bot: I haz the power, 2014-06-24 08:32:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/345743002/80001

[commit-bot: I haz the power, 2014-06-24 10:18:19]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-06-24 11:38:14]

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

[commit-bot: I haz the power, 2014-06-24 11:38:15]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[hajimehoshi, 2014-06-25 03:34:52]

The CQ bit was checked by hajimehoshi@chromium.org

[commit-bot: I haz the power, 2014-06-25 03:36:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/345743002/80001

[commit-bot: I haz the power, 2014-06-25 04:46:30]

Change committed as 279588