Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2920953002:
Auto-always/never can at most triggered twice (new translate infobar) (Closed)
|
Created:
3 years, 6 months ago by Marti Wong Modified:
3 years, 6 months ago CC:
chromium-reviews, Leo Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAuto-always/never can at most triggered twice (new translate infobar)
Currently, in the new translate infobar,
"Always translate" will be triggered automatically when a language
is consecutively translated for 5 times. And user can click on "Cancel"
to undo the "Always translate".
However, it is quite annoying that the auto-always will keep triggering
again and again for every 5 translations.
After discussion, we should limit the maximum number of auto-always to
2 times.
Same for auto-never.
BUG=728931
Review-Url: https://codereview.chromium.org/2920953002
Cr-Commit-Position: refs/heads/master@{#479226}
Committed: https://chromium.googlesource.com/chromium/src/+/fb6d187db2e842f01db923c87d7958b52175c37f
Patch Set 1 : #
Total comments: 10
Patch Set 2 : add '#if define' #
Total comments: 4
Patch Set 3 : fix #
Messages
Total messages: 29 (20 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== Auto-always/never can at most triggered twice (new translate infobar) BUG=728931 ========== to ========== Auto-always/never can at most triggered twice (new translate infobar) Currently, in the new translate infobar, "Always translate" will be triggered automatically when a language is consecutively translated for 5 times. And user can click on "Cancel" to undo the "Always translate". However, it is quite annoying that the auto-always will keep triggering again and again for every 5 translations. After discussion, we should limit the maximum number of auto-always to 2 times. Same for auto-never. BUG=728931 ==========
martiw@chromium.org changed reviewers: + groby@chromium.org
Hi Rachel, PTAL. Thank you~! https://codereview.chromium.org/2920953002/diff/60001/chrome/browser/ui/andro... File chrome/browser/ui/android/infobars/translate_compact_infobar.cc (right): https://codereview.chromium.org/2920953002/diff/60001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/translate_compact_infobar.cc:174: bool return_value = (delegate->GetTranslationDeniedCount() + off_by_one == This is a little bit tricky here. After code refactoring (crrev.com/2899893004). This checking is done before increment of the counter (instead of after). which causes the off by 1 problem. (we wanted it triggered at 7th times but it triggered at 8th times). After the auto-never is triggered once, the counter starts at 1 so there is no off by 1 issue then. https://codereview.chromium.org/2920953002/diff/60001/components/translate/co... File components/translate/core/browser/translate_prefs.h (right): https://codereview.chromium.org/2920953002/diff/60001/components/translate/co... components/translate/core/browser/translate_prefs.h:90: static const char kPrefTranslateAutoAlwaysCount[]; 2 questions: - Do I need someone's approval for adding a new pref? - AutoAlwaysCount, AutoNeverCount are used in android only. Do I need #if defined(OS_ANDROID) for them? https://codereview.chromium.org/2920953002/diff/60001/components/translate/co... components/translate/core/browser/translate_prefs.h:231: base::DictionaryValue* GetTranslationDeniedCountDictionary(); Should I add "const" at the end of this line?
Before you land this: I like this. I think this makes sense beyond Android. You might want to reach out to napper@/yyushinka@ to see if we want this on desktop, too. That said, LGTM % nits https://codereview.chromium.org/2920953002/diff/60001/components/translate/co... File components/translate/core/browser/translate_prefs.cc (right): https://codereview.chromium.org/2920953002/diff/60001/components/translate/co... components/translate/core/browser/translate_prefs.cc:196: ResetTranslationAutoAlwaysCount(language); Note: You might want to clear out the entire pref. (Is there a reason it should survive for any languages not in GetLanguageList?) https://codereview.chromium.org/2920953002/diff/60001/components/translate/co... File components/translate/core/browser/translate_prefs.h (right): https://codereview.chromium.org/2920953002/diff/60001/components/translate/co... components/translate/core/browser/translate_prefs.h:90: static const char kPrefTranslateAutoAlwaysCount[]; On 2017/06/02 07:37:13, Marti Wong wrote: > 2 questions: > - Do I need someone's approval for adding a new pref? > - AutoAlwaysCount, AutoNeverCount are used in android only. > Do I need #if defined(OS_ANDROID) for them? Prefs don't need approval (AFAIK :). If they're Android-only, yes, you should #ifdef them... but - should they be Android only? https://codereview.chromium.org/2920953002/diff/60001/components/translate/co... components/translate/core/browser/translate_prefs.h:231: base::DictionaryValue* GetTranslationDeniedCountDictionary(); On 2017/06/02 07:37:13, Marti Wong wrote: > Should I add "const" at the end of this line? Probably yes. Sidebar: Either solution is iffy. const is technically correct, since it doesn't allow the object to be modified. And yet, misleading, since it returns a handle to global state that can be modified. So... ¯\_(ツ)_/¯
On 2017/06/07 17:02:50, groby wrote: > Before you land this: I like this. I think this makes sense beyond Android. You > might want to reach out to napper@/yyushinka@ to see if we want this on desktop, > too. > > That said, LGTM % nits > > https://codereview.chromium.org/2920953002/diff/60001/components/translate/co... > File components/translate/core/browser/translate_prefs.cc (right): > > https://codereview.chromium.org/2920953002/diff/60001/components/translate/co... > components/translate/core/browser/translate_prefs.cc:196: > ResetTranslationAutoAlwaysCount(language); > Note: You might want to clear out the entire pref. (Is there a reason it should > survive for any languages not in GetLanguageList?) > > https://codereview.chromium.org/2920953002/diff/60001/components/translate/co... > File components/translate/core/browser/translate_prefs.h (right): > > https://codereview.chromium.org/2920953002/diff/60001/components/translate/co... > components/translate/core/browser/translate_prefs.h:90: static const char > kPrefTranslateAutoAlwaysCount[]; > On 2017/06/02 07:37:13, Marti Wong wrote: > > 2 questions: > > - Do I need someone's approval for adding a new pref? > > - AutoAlwaysCount, AutoNeverCount are used in android only. > > Do I need #if defined(OS_ANDROID) for them? > > Prefs don't need approval (AFAIK :). If they're Android-only, yes, you should > #ifdef them... but - should they be Android only? > > https://codereview.chromium.org/2920953002/diff/60001/components/translate/co... > components/translate/core/browser/translate_prefs.h:231: base::DictionaryValue* > GetTranslationDeniedCountDictionary(); > On 2017/06/02 07:37:13, Marti Wong wrote: > > Should I add "const" at the end of this line? > > Probably yes. > > Sidebar: Either solution is iffy. const is technically correct, since it doesn't > allow the object to be modified. And yet, misleading, since it returns a handle > to global state that can be modified. So... ¯\_(ツ)_/¯ Thanks alot! Rachel. I will discuss with napper@ and yyushinka@. 2 things: 1. Is translate_prefs stored on the device locally? or it will be synced among the devices/platforms with the same google account? If they will be synced among platforms. #ifdef may cause bug. 2. I am not sure if I should clean up the following (may be in another CL) becuz it seems they are not used by anyone: - GetTranslationDeniedCountDictionary() - GetTranslationAcceptedCountDictionary() - HasWhitelistedLanguagePairs() - HasBlacklistedSites() - HasBlockedLanguages()
The CQ bit was checked by martiw@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by martiw@chromium.org
martiw@chromium.org changed reviewers: + mdjones@chromium.org
Hi Matthew, PTAL: translate_compact_infobar.h/.cc Thanks~! https://codereview.chromium.org/2920953002/diff/60001/components/translate/co... File components/translate/core/browser/translate_prefs.cc (right): https://codereview.chromium.org/2920953002/diff/60001/components/translate/co... components/translate/core/browser/translate_prefs.cc:196: ResetTranslationAutoAlwaysCount(language); On 2017/06/07 17:02:49, groby wrote: > Note: You might want to clear out the entire pref. (Is there a reason it should > survive for any languages not in GetLanguageList?) Thanks. Done. https://codereview.chromium.org/2920953002/diff/60001/components/translate/co... File components/translate/core/browser/translate_prefs.h (right): https://codereview.chromium.org/2920953002/diff/60001/components/translate/co... components/translate/core/browser/translate_prefs.h:90: static const char kPrefTranslateAutoAlwaysCount[]; On 2017/06/07 17:02:49, groby wrote: > On 2017/06/02 07:37:13, Marti Wong wrote: > > 2 questions: > > - Do I need someone's approval for adding a new pref? > > - AutoAlwaysCount, AutoNeverCount are used in android only. > > Do I need #if defined(OS_ANDROID) for them? > > Prefs don't need approval (AFAIK :). If they're Android-only, yes, you should > #ifdef them... but - should they be Android only? Talked to Yana. These are for Android only, at least for now. So I will use '#if defined'. We could remove '#if defined' in case other platform needs it in future. https://codereview.chromium.org/2920953002/diff/60001/components/translate/co... components/translate/core/browser/translate_prefs.h:231: base::DictionaryValue* GetTranslationDeniedCountDictionary(); It seems this functions are never been used. I will leave it untouched by now. And see if we need to clean them up in another CL. https://codereview.chromium.org/2920953002/diff/60001/components/translate/co... components/translate/core/browser/translate_prefs.h:243: base::DictionaryValue* GetTranslationAutoNeverCountDictionary() const; Removed. functions never been used.
https://codereview.chromium.org/2920953002/diff/80001/chrome/browser/ui/andro... File chrome/browser/ui/android/infobars/translate_compact_infobar.cc (right): https://codereview.chromium.org/2920953002/diff/80001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/translate_compact_infobar.cc:172: int off_by_one = auto_never_count == 0 ? 1 : 0; Just out of curiosity, why is this only true if auto_never_count is 0? Wouldn't it be off by one all the time? https://codereview.chromium.org/2920953002/diff/80001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/translate_compact_infobar.cc:174: bool return_value = (delegate->GetTranslationDeniedCount() + off_by_one == nit: "return_value" could probably be a bit more descriptive, maybe "never_translate".
The CQ bit was checked by martiw@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by martiw@chromium.org
Patchset #3 (id:100001) has been deleted
The CQ bit was checked by martiw@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by martiw@chromium.org
Thx. See if it's okay. https://codereview.chromium.org/2920953002/diff/80001/chrome/browser/ui/andro... File chrome/browser/ui/android/infobars/translate_compact_infobar.cc (right): https://codereview.chromium.org/2920953002/diff/80001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/translate_compact_infobar.cc:172: int off_by_one = auto_never_count == 0 ? 1 : 0; On 2017/06/09 17:43:37, mdjones wrote: > Just out of curiosity, why is this only true if auto_never_count is 0? Wouldn't > it be off by one all the time? At the beginning, DeniedCount starts at 0. And there is off-by-one. But after auto-never is triggered once, DeniedCount starts at 1. (DeniedCount is incremented after auto-never is triggered.) So there is no off-by-one by then. I modified the comment to explain more clearly. https://codereview.chromium.org/2920953002/diff/80001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/translate_compact_infobar.cc:174: bool return_value = (delegate->GetTranslationDeniedCount() + off_by_one == On 2017/06/09 17:43:36, mdjones wrote: > nit: "return_value" could probably be a bit more descriptive, maybe > "never_translate". Done.
lgtm
The CQ bit was checked by martiw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from groby@chromium.org Link to the patchset: https://codereview.chromium.org/2920953002/#ps120001 (title: "fix")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1497398104318580,
"parent_rev": "7c0adb62e0216573114bb7f01c1157da3eaf8c8b", "commit_rev":
"fb6d187db2e842f01db923c87d7958b52175c37f"}
Message was sent while issue was closed.
Description was changed from ========== Auto-always/never can at most triggered twice (new translate infobar) Currently, in the new translate infobar, "Always translate" will be triggered automatically when a language is consecutively translated for 5 times. And user can click on "Cancel" to undo the "Always translate". However, it is quite annoying that the auto-always will keep triggering again and again for every 5 translations. After discussion, we should limit the maximum number of auto-always to 2 times. Same for auto-never. BUG=728931 ========== to ========== Auto-always/never can at most triggered twice (new translate infobar) Currently, in the new translate infobar, "Always translate" will be triggered automatically when a language is consecutively translated for 5 times. And user can click on "Cancel" to undo the "Always translate". However, it is quite annoying that the auto-always will keep triggering again and again for every 5 translations. After discussion, we should limit the maximum number of auto-always to 2 times. Same for auto-never. BUG=728931 Review-Url: https://codereview.chromium.org/2920953002 Cr-Commit-Position: refs/heads/master@{#479226} Committed: https://chromium.googlesource.com/chromium/src/+/fb6d187db2e842f01db923c87d79... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:120001) as https://chromium.googlesource.com/chromium/src/+/fb6d187db2e842f01db923c87d79... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
