Reset counters for all language when reset translate settings

components/translate/core/browser/translate_prefs.cc

Index: components/translate/core/browser/translate_prefs.cc
diff --git a/components/translate/core/browser/translate_prefs.cc b/components/translate/core/browser/translate_prefs.cc
index 0d13a31201d3445c6a28f7ee68e0d805ca9f0d20..d2a17ff517e9ae32dfce6653b7420406e2088c5a 100644
--- a/components/translate/core/browser/translate_prefs.cc
+++ b/components/translate/core/browser/translate_prefs.cc
@@ -180,16 +180,9 @@ void TranslatePrefs::ResetToDefaults() {
   ClearBlacklistedSites();
   ClearWhitelistedLanguagePairs();
 
-  std::vector<std::string> languages;
-  GetLanguageList(&languages);
-  for (std::vector<std::string>::const_iterator it = languages.begin();
-       it != languages.end(); ++it) {
-    const std::string& language = *it;
-    ResetTranslationAcceptedCount(language);
-    ResetTranslationDeniedCount(language);
-    ResetTranslationIgnoredCount(language);
-  }
-
+  prefs_->ClearPref(kPrefTranslateDeniedCount);

[Leo, 2017/06/07 00:21:23]

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. 

[Marti Wong, 2017/06/07 01:06:42]

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?

[groby-ooo-7-16, 2017/06/07 16:43:49]

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.)

[Marti Wong, 2017/06/08 03:30:50]

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());
  }

+  prefs_->ClearPref(kPrefTranslateIgnoredCount);
+  prefs_->ClearPref(kPrefTranslateAcceptedCount);
   prefs_->ClearPref(kPrefTranslateLastDeniedTimeForLanguage);
   prefs_->ClearPref(kPrefTranslateTooOftenDeniedForLanguage);
 }