Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2919313003:
Reset counters for all language when reset translate settings (Closed)
|
Created:
3 years, 6 months ago by Marti Wong Modified:
3 years, 5 months ago CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionReset counters for all language when reset translate settings.
When I reset translate settings,
(menu -> Settings -> Site settings -> Translate -> Reset)
The accepted/denied/ignored counters of all languages should
be reset to 0.
However, currently, only the counters of the languages in accept_languages_pref_ will be reset.
Accepted languages means "languages the user understands or does not want translated".
But Denied, Ignored, Accepted counters are for those languages user might not understand. That's why some counters are not reset in that For-loop.
BUG=728930
Review-Url: https://codereview.chromium.org/2919313003
Cr-Commit-Position: refs/heads/master@{#482168}
Committed: https://chromium.googlesource.com/chromium/src/+/c09df0ce5984176ea0590a779ab15c6ce54e4731
Patch Set 1 #
Total comments: 4
Patch Set 2 : sync and merge #Messages
Total messages: 41 (25 generated)
Description was changed from ========== Reset counters for all language when reset translate settings BUG=728930 ========== to ========== Reset counters for all language when reset translate settings. When I reset translate settings, (menu -> Settings -> Site settings -> Translate -> Reset) The accepted/denied/ignored counters of all languages should be reset to 0. However, currently, only the counters of the languages in accept_languages_pref_ will be reset. BUG=728930 ==========
martiw@chromium.org changed reviewers: + groby@chromium.org
Hi Rachel, PTAL. thanks!
googleo@chromium.org changed reviewers: + googleo@google.com
googleo@chromium.org changed reviewers: + googleo@chromium.org
A little concern about the removing, because it breaks the original logic. https://codereview.chromium.org/2919313003/diff/1/components/translate/core/b... File components/translate/core/browser/translate_prefs.cc (right): https://codereview.chromium.org/2919313003/diff/1/components/translate/core/b... components/translate/core/browser/translate_prefs.cc:183: prefs_->ClearPref(kPrefTranslateDeniedCount); I think it's better to figure out the root cause why "Japanese" is missed in GetLanguageList(&languages). which mentioned in the bug. Seems removing the loop condition is a little aggressive.
googleo@chromium.org changed reviewers: + napper@chromium.org - googleo@google.com
+ napper
https://codereview.chromium.org/2919313003/diff/1/components/translate/core/b... File components/translate/core/browser/translate_prefs.cc (right): https://codereview.chromium.org/2919313003/diff/1/components/translate/core/b... components/translate/core/browser/translate_prefs.cc:183: prefs_->ClearPref(kPrefTranslateDeniedCount); On 2017/06/07 00:21:23, Leo wrote: > I think it's better to figure out the root cause why "Japanese" is missed in > GetLanguageList(&languages). which mentioned in the bug. > > Seems removing the loop condition is a little aggressive. In the repro case of crbug.com/728930, Japanese is missing because: GetLanguageList here will get the languages from accept_languages_pref_ And accepted languages means "languages the user understands or does not want translated". (These are English, Chinese in the repro case.) But Denied, Ignored, Accepted counters are for those languages user might not understand. (So Japanese is not in included in that For loop.) If we don't do ClearPref here, we will need 3 For loops, one For loop for each counter. Pseudocode: DictionaryValue acceptedCountDict = GetDictionary of AcceptedCount; foreach ( language in acceptedCountDict ) prefs->ResetTranslationAcceptedCount(language); DictionaryValue deniedCountDict = GetDictionary of DeniedCount; foreach ( language in deniedCountDict ) prefs->ResetTranslationDeniedCount(language); DictionaryValue ignoredCountDict = GetDictionary of IgnoredCount; foreach ( language in ignoredCountDict ) prefs->ResetTranslationIgnoredCount(language); The outcome will be the same. But the code will be more complicated and less efficient. Any thoughts?
https://codereview.chromium.org/2919313003/diff/1/components/translate/core/b... File components/translate/core/browser/translate_prefs.cc (right): https://codereview.chromium.org/2919313003/diff/1/components/translate/core/b... components/translate/core/browser/translate_prefs.cc:183: prefs_->ClearPref(kPrefTranslateDeniedCount); On 2017/06/07 01:06:42, Marti Wong wrote: > On 2017/06/07 00:21:23, Leo wrote: > > I think it's better to figure out the root cause why "Japanese" is missed in > > GetLanguageList(&languages). which mentioned in the bug. > > > > Seems removing the loop condition is a little aggressive. > > In the repro case of crbug.com/728930, Japanese is missing because: > GetLanguageList here will get the languages from accept_languages_pref_ > And accepted languages means "languages the user understands or does not want > translated". > (These are English, Chinese in the repro case.) > > But Denied, Ignored, Accepted counters are for those languages user might not > understand. > (So Japanese is not in included in that For loop.) > > If we don't do ClearPref here, we will need 3 For loops, one For loop for each > counter. > > Pseudocode: > > DictionaryValue acceptedCountDict = GetDictionary of AcceptedCount; > foreach ( language in acceptedCountDict ) > prefs->ResetTranslationAcceptedCount(language); > > DictionaryValue deniedCountDict = GetDictionary of DeniedCount; > foreach ( language in deniedCountDict ) > prefs->ResetTranslationDeniedCount(language); > > DictionaryValue ignoredCountDict = GetDictionary of IgnoredCount; > foreach ( language in ignoredCountDict ) > prefs->ResetTranslationIgnoredCount(language); > > The outcome will be the same. > But the code will be more complicated and less efficient. > Any thoughts? Hm. It might be worth digging through history for the original author and asking them why they choose the loop, instead of clearing the entire pref. (At first glance, ClearPref seems the right choice - but I'm not sure if that's missing some subtlety.)
On 2017/06/07 16:43:49, groby wrote: > https://codereview.chromium.org/2919313003/diff/1/components/translate/core/b... > File components/translate/core/browser/translate_prefs.cc (right): > > https://codereview.chromium.org/2919313003/diff/1/components/translate/core/b... > components/translate/core/browser/translate_prefs.cc:183: > prefs_->ClearPref(kPrefTranslateDeniedCount); > On 2017/06/07 01:06:42, Marti Wong wrote: > > On 2017/06/07 00:21:23, Leo wrote: > > > I think it's better to figure out the root cause why "Japanese" is missed in > > > GetLanguageList(&languages). which mentioned in the bug. > > > > > > Seems removing the loop condition is a little aggressive. > > > > In the repro case of crbug.com/728930, Japanese is missing because: > > GetLanguageList here will get the languages from accept_languages_pref_ > > And accepted languages means "languages the user understands or does not want > > translated". > > (These are English, Chinese in the repro case.) > > > > But Denied, Ignored, Accepted counters are for those languages user might not > > understand. > > (So Japanese is not in included in that For loop.) > > > > If we don't do ClearPref here, we will need 3 For loops, one For loop for each > > counter. > > > > Pseudocode: > > > > DictionaryValue acceptedCountDict = GetDictionary of AcceptedCount; > > foreach ( language in acceptedCountDict ) > > prefs->ResetTranslationAcceptedCount(language); > > > > DictionaryValue deniedCountDict = GetDictionary of DeniedCount; > > foreach ( language in deniedCountDict ) > > prefs->ResetTranslationDeniedCount(language); > > > > DictionaryValue ignoredCountDict = GetDictionary of IgnoredCount; > > foreach ( language in ignoredCountDict ) > > prefs->ResetTranslationIgnoredCount(language); > > > > The outcome will be the same. > > But the code will be more complicated and less efficient. > > Any thoughts? > > Hm. It might be worth digging through history for the original author and asking > them why they choose the loop, instead of clearing the entire pref. (At first > glance, ClearPref seems the right choice - but I'm not sure if that's missing > some subtlety.) Thanks Rachel. I will discuss with the author/code reviewers.
Description was changed from ========== Reset counters for all language when reset translate settings. When I reset translate settings, (menu -> Settings -> Site settings -> Translate -> Reset) The accepted/denied/ignored counters of all languages should be reset to 0. However, currently, only the counters of the languages in accept_languages_pref_ will be reset. BUG=728930 ========== to ========== Reset counters for all language when reset translate settings. When I reset translate settings, (menu -> Settings -> Site settings -> Translate -> Reset) The accepted/denied/ignored counters of all languages should be reset to 0. However, currently, only the counters of the languages in accept_languages_pref_ will be reset. Accepted languages means "languages the user understands or does not want translated". But Denied, Ignored, Accepted counters are for those languages user might not understand. That's why some counters are not reset in that For-loop. BUG=728930 ==========
martiw@chromium.org changed reviewers: + miguelg@chromium.org
Hi Miguel, This is Marti from the Chrome Sydney team. I am working on crrev.com/2919313003/ Which is about a bug in reseting Accepted/Denied/Ignored counts. Could you PTAL to the CL? The function ResetToDefaults() in translate_prefs.cc was added in crrev.com/71353005/ And you are one of the reviewers (it seems the owner left the team already.) I was wondering if there is any subtle reason for using the For-loop to reset the counts (instead of using ClearPref). Thanks, Marti https://codereview.chromium.org/2919313003/diff/1/components/translate/core/b... File components/translate/core/browser/translate_prefs.cc (right): https://codereview.chromium.org/2919313003/diff/1/components/translate/core/b... components/translate/core/browser/translate_prefs.cc:183: prefs_->ClearPref(kPrefTranslateDeniedCount); In case there is any reason we need to use For-loop instead of ClearPref, we could do that by the following code. I will ask Miguel (who is the reviewer of the CL creating ResetToDefaults()) to see if there is any subtle reason for using For-loop. const base::DictionaryValue* deniedCountDictionary = prefs_->GetDictionary(kPrefTranslateDeniedCount); for (base::DictionaryValue::Iterator iter(*deniedCountDictionary); !iter.IsAtEnd(); iter.Advance()) { ResetTranslationDeniedCount(iter.key()); } const base::DictionaryValue* ignoredCountDictionary = prefs_->GetDictionary(kPrefTranslateIgnoredCount); for (base::DictionaryValue::Iterator iter(*ignoredCountDictionary); !iter.IsAtEnd(); iter.Advance()) { ResetTranslationIgnoredCount(iter.key()); } const base::DictionaryValue* acceptedCountDictionary = prefs_->GetDictionary(kPrefTranslateAcceptedCount); for (base::DictionaryValue::Iterator iter(*acceptedCountDictionary); !iter.IsAtEnd(); iter.Advance()) { ResetTranslationAcceptedCount(iter.key()); }
On 2017/06/08 03:30:50, Marti Wong wrote: > Hi Miguel, > > This is Marti from the Chrome Sydney team. > I am working on crrev.com/2919313003/ > Which is about a bug in reseting Accepted/Denied/Ignored counts. > > Could you PTAL to the CL? > > The function ResetToDefaults() in translate_prefs.cc was added in > crrev.com/71353005/ > And you are one of the reviewers (it seems the owner left the team already.) > > I was wondering if there is any subtle reason for using the For-loop to reset > the counts (instead of using ClearPref). > > Thanks, > Marti > > https://codereview.chromium.org/2919313003/diff/1/components/translate/core/b... > File components/translate/core/browser/translate_prefs.cc (right): > > https://codereview.chromium.org/2919313003/diff/1/components/translate/core/b... > components/translate/core/browser/translate_prefs.cc:183: > prefs_->ClearPref(kPrefTranslateDeniedCount); > In case there is any reason we need to use For-loop instead of ClearPref, we > could do that by the following code. I will ask > Miguel (who is the reviewer of the CL creating ResetToDefaults()) to see if > there is any subtle reason for using For-loop. > > const base::DictionaryValue* deniedCountDictionary = > prefs_->GetDictionary(kPrefTranslateDeniedCount); > for (base::DictionaryValue::Iterator iter(*deniedCountDictionary); > !iter.IsAtEnd(); iter.Advance()) { > ResetTranslationDeniedCount(iter.key()); > } > > const base::DictionaryValue* ignoredCountDictionary = > prefs_->GetDictionary(kPrefTranslateIgnoredCount); > for (base::DictionaryValue::Iterator iter(*ignoredCountDictionary); > !iter.IsAtEnd(); iter.Advance()) { > ResetTranslationIgnoredCount(iter.key()); > } > > const base::DictionaryValue* acceptedCountDictionary = > prefs_->GetDictionary(kPrefTranslateAcceptedCount); > for (base::DictionaryValue::Iterator iter(*acceptedCountDictionary); > !iter.IsAtEnd(); iter.Advance()) { > ResetTranslationAcceptedCount(iter.key()); > } fwiw: I believe I just saw another CL also getting rid of the for loop. I can't find the link right now, but that might be landing soon.
On 2017/06/13 00:30:12, groby wrote: > On 2017/06/08 03:30:50, Marti Wong wrote: > > Hi Miguel, > > > > This is Marti from the Chrome Sydney team. > > I am working on crrev.com/2919313003/ > > Which is about a bug in reseting Accepted/Denied/Ignored counts. > > > > Could you PTAL to the CL? > > > > The function ResetToDefaults() in translate_prefs.cc was added in > > crrev.com/71353005/ > > And you are one of the reviewers (it seems the owner left the team already.) > > > > I was wondering if there is any subtle reason for using the For-loop to reset > > the counts (instead of using ClearPref). > > > > Thanks, > > Marti > > > > > https://codereview.chromium.org/2919313003/diff/1/components/translate/core/b... > > File components/translate/core/browser/translate_prefs.cc (right): > > > > > https://codereview.chromium.org/2919313003/diff/1/components/translate/core/b... > > components/translate/core/browser/translate_prefs.cc:183: > > prefs_->ClearPref(kPrefTranslateDeniedCount); > > In case there is any reason we need to use For-loop instead of ClearPref, we > > could do that by the following code. I will ask > > Miguel (who is the reviewer of the CL creating ResetToDefaults()) to see if > > there is any subtle reason for using For-loop. > > > > const base::DictionaryValue* deniedCountDictionary = > > prefs_->GetDictionary(kPrefTranslateDeniedCount); > > for (base::DictionaryValue::Iterator iter(*deniedCountDictionary); > > !iter.IsAtEnd(); iter.Advance()) { > > ResetTranslationDeniedCount(iter.key()); > > } > > > > const base::DictionaryValue* ignoredCountDictionary = > > prefs_->GetDictionary(kPrefTranslateIgnoredCount); > > for (base::DictionaryValue::Iterator iter(*ignoredCountDictionary); > > !iter.IsAtEnd(); iter.Advance()) { > > ResetTranslationIgnoredCount(iter.key()); > > } > > > > const base::DictionaryValue* acceptedCountDictionary = > > prefs_->GetDictionary(kPrefTranslateAcceptedCount); > > for (base::DictionaryValue::Iterator iter(*acceptedCountDictionary); > > !iter.IsAtEnd(); iter.Advance()) { > > ResetTranslationAcceptedCount(iter.key()); > > } > > fwiw: I believe I just saw another CL also getting rid of the for loop. I can't > find the link right now, but that might be landing soon. Hi groby@, are you one of the reviewers of that CL? I did some digging and couldn't find it :(
On 2017/06/13 09:15:37, Marti Wong wrote: > On 2017/06/13 00:30:12, groby wrote: > > On 2017/06/08 03:30:50, Marti Wong wrote: > > > Hi Miguel, > > > > > > This is Marti from the Chrome Sydney team. > > > I am working on crrev.com/2919313003/ > > > Which is about a bug in reseting Accepted/Denied/Ignored counts. > > > > > > Could you PTAL to the CL? > > > > > > The function ResetToDefaults() in translate_prefs.cc was added in > > > crrev.com/71353005/ > > > And you are one of the reviewers (it seems the owner left the team already.) > > > > > > I was wondering if there is any subtle reason for using the For-loop to > reset > > > the counts (instead of using ClearPref). > > > > > > Thanks, > > > Marti > > > > > > > > > https://codereview.chromium.org/2919313003/diff/1/components/translate/core/b... > > > File components/translate/core/browser/translate_prefs.cc (right): > > > > > > > > > https://codereview.chromium.org/2919313003/diff/1/components/translate/core/b... > > > components/translate/core/browser/translate_prefs.cc:183: > > > prefs_->ClearPref(kPrefTranslateDeniedCount); > > > In case there is any reason we need to use For-loop instead of ClearPref, we > > > could do that by the following code. I will ask > > > Miguel (who is the reviewer of the CL creating ResetToDefaults()) to see if > > > there is any subtle reason for using For-loop. > > > > > > const base::DictionaryValue* deniedCountDictionary = > > > prefs_->GetDictionary(kPrefTranslateDeniedCount); > > > for (base::DictionaryValue::Iterator iter(*deniedCountDictionary); > > > !iter.IsAtEnd(); iter.Advance()) { > > > ResetTranslationDeniedCount(iter.key()); > > > } > > > > > > const base::DictionaryValue* ignoredCountDictionary = > > > prefs_->GetDictionary(kPrefTranslateIgnoredCount); > > > for (base::DictionaryValue::Iterator iter(*ignoredCountDictionary); > > > !iter.IsAtEnd(); iter.Advance()) { > > > ResetTranslationIgnoredCount(iter.key()); > > > } > > > > > > const base::DictionaryValue* acceptedCountDictionary = > > > prefs_->GetDictionary(kPrefTranslateAcceptedCount); > > > for (base::DictionaryValue::Iterator iter(*acceptedCountDictionary); > > > !iter.IsAtEnd(); iter.Advance()) { > > > ResetTranslationAcceptedCount(iter.key()); > > > } > > > > fwiw: I believe I just saw another CL also getting rid of the for loop. I > can't > > find the link right now, but that might be landing soon. > > Hi groby@, are you one of the reviewers of that CL? I did some digging and > couldn't find it :( I'll just go w/ LGTM then - I can't currently find it either. Will mail review if I run across it
The CQ bit was checked by martiw@chromium.org
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 commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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/2919313003/#ps20001 (title: "sync and merge")
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 commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
Patchset #2 (id:20001) 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 commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
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 commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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/2919313003/#ps40001 (title: "sync and merge")
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": 40001, "attempt_start_ts": 1498372052173230,
"parent_rev": "b1c66c859236d65da176b4fef5181a5bfd941940", "commit_rev":
"c09df0ce5984176ea0590a779ab15c6ce54e4731"}
Message was sent while issue was closed.
Description was changed from ========== Reset counters for all language when reset translate settings. When I reset translate settings, (menu -> Settings -> Site settings -> Translate -> Reset) The accepted/denied/ignored counters of all languages should be reset to 0. However, currently, only the counters of the languages in accept_languages_pref_ will be reset. Accepted languages means "languages the user understands or does not want translated". But Denied, Ignored, Accepted counters are for those languages user might not understand. That's why some counters are not reset in that For-loop. BUG=728930 ========== to ========== Reset counters for all language when reset translate settings. When I reset translate settings, (menu -> Settings -> Site settings -> Translate -> Reset) The accepted/denied/ignored counters of all languages should be reset to 0. However, currently, only the counters of the languages in accept_languages_pref_ will be reset. Accepted languages means "languages the user understands or does not want translated". But Denied, Ignored, Accepted counters are for those languages user might not understand. That's why some counters are not reset in that For-loop. BUG=728930 Review-Url: https://codereview.chromium.org/2919313003 Cr-Commit-Position: refs/heads/master@{#482168} Committed: https://chromium.googlesource.com/chromium/src/+/c09df0ce5984176ea0590a779ab1... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001) as https://chromium.googlesource.com/chromium/src/+/c09df0ce5984176ea0590a779ab1... |
