Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2090283002:
add experimental param to control the triggering threshold to default Always Translate checked (Closed)
Descriptionadd experimental param to control the triggering threshold to default Always Translate checked
BUG=622854
Committed: https://crrev.com/2364a394e98147f92f80ddeb5a1c56abbb547951
Cr-Commit-Position: refs/heads/master@{#403232}
Patch Set 1 #Patch Set 2 : add entry to histograms.xml to fix AboutFlagsHistogramTest.CheckHistograms #Patch Set 3 : use variations::GetVariationParams instead #Patch Set 4 : fix unit test breakage #
Total comments: 10
Patch Set 5 : move the code to translate_ui_delegate per review comment #Patch Set 6 : use auto #
Total comments: 14
Patch Set 7 : update #
Total comments: 3
Patch Set 8 : address review issues #
Messages
Total messages: 69 (28 generated)
The CQ bit was checked by ftang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2090283002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
add entry to histograms.xml to fix AboutFlagsHistogramTest.CheckHistograms
The CQ bit was checked by ftang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2090283002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== add experimental flag to control the triggering threshold to default Always Translate checked BUG= ========== to ========== add experimental flag to control the triggering threshold to default Always Translate checked BUG=622854 ==========
ftang@chromium.org changed reviewers: + groby@chromium.org
The right way to handle this seems to me a set of experiments that controls that number via an experiment param. See go/finch-params
Use variations::GetVariationParams instead
Description was changed from ========== add experimental flag to control the triggering threshold to default Always Translate checked BUG=622854 ========== to ========== add experimental param to control the triggering threshold to default Always Translate checked BUG=622854 ==========
The CQ bit was checked by ftang@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: ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
fix unit test breakage
The CQ bit was checked by ftang@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.
PTAL
https://codereview.chromium.org/2090283002/diff/60001/components/translate/co... File components/translate/core/browser/translate_prefs.cc (right): https://codereview.chromium.org/2090283002/diff/60001/components/translate/co... components/translate/core/browser/translate_prefs.cc:456: bool TranslatePrefs::ShouldAlwaysTranslateBeCheckedByDefault( This is moving even more policy decisions into prefs. When will we start moving them out instead? https://codereview.chromium.org/2090283002/diff/60001/components/translate/co... components/translate/core/browser/translate_prefs.cc:459: int num_of_translation_trigger_default_checked = -1; StringToInt *will* force this to 0 on error. https://codereview.chromium.org/2090283002/diff/60001/components/translate/co... components/translate/core/browser/translate_prefs.cc:462: std::map<std::string, std::string>::const_iterator it = auto it = params.find Or, if you want even shorter, if (!variations::GetVariationsParam(...)) return false; base::StringToInt(params[translate::kNumOfTranslationTriggerAlways], &threshold); if (threshold <= 0) return false; return GetTranslationAcceptedCount(language) >= threshold; (Auto-creation of map members guarantees an empty string if the key is not found, StringToInt will set Threshold to 0 when invalid or empty) https://codereview.chromium.org/2090283002/diff/60001/components/translate/co... File components/translate/core/browser/translate_prefs.h (right): https://codereview.chromium.org/2090283002/diff/60001/components/translate/co... components/translate/core/browser/translate_prefs.h:42: // Translate" checkbox defaul to checked. nit:"default" https://codereview.chromium.org/2090283002/diff/60001/components/translate/co... components/translate/core/browser/translate_prefs.h:43: extern const char kNumOfTranslationTriggerAlways[]; The name is slightly unclear. Maybe kAlwaysTranslateOfferThreshold?
move the code to translate_ui_delegate per review comment
The CQ bit was checked by ftang@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.
Use auto
PTAL https://codereview.chromium.org/2090283002/diff/60001/components/translate/co... File components/translate/core/browser/translate_prefs.cc (right): https://codereview.chromium.org/2090283002/diff/60001/components/translate/co... components/translate/core/browser/translate_prefs.cc:456: bool TranslatePrefs::ShouldAlwaysTranslateBeCheckedByDefault( On 2016/06/27 23:00:34, groby wrote: > This is moving even more policy decisions into prefs. When will we start moving > them out instead? moved https://codereview.chromium.org/2090283002/diff/60001/components/translate/co... components/translate/core/browser/translate_prefs.cc:459: int num_of_translation_trigger_default_checked = -1; On 2016/06/27 23:00:34, groby wrote: > StringToInt *will* force this to 0 on error. Done. https://codereview.chromium.org/2090283002/diff/60001/components/translate/co... components/translate/core/browser/translate_prefs.cc:462: std::map<std::string, std::string>::const_iterator it = On 2016/06/27 23:00:34, groby wrote: > auto it = params.find > > Or, if you want even shorter, > if (!variations::GetVariationsParam(...)) > return false; > > base::StringToInt(params[translate::kNumOfTranslationTriggerAlways], > &threshold); > if (threshold <= 0) > return false; > > return GetTranslationAcceptedCount(language) >= threshold; > > > (Auto-creation of map members guarantees an empty string if the key is not > found, StringToInt will set Threshold to 0 when invalid or empty) Done. https://codereview.chromium.org/2090283002/diff/60001/components/translate/co... File components/translate/core/browser/translate_prefs.h (right): https://codereview.chromium.org/2090283002/diff/60001/components/translate/co... components/translate/core/browser/translate_prefs.h:42: // Translate" checkbox defaul to checked. On 2016/06/27 23:00:34, groby wrote: > nit:"default" Done. https://codereview.chromium.org/2090283002/diff/60001/components/translate/co... components/translate/core/browser/translate_prefs.h:43: extern const char kNumOfTranslationTriggerAlways[]; On 2016/06/27 23:00:34, groby wrote: > The name is slightly unclear. Maybe kAlwaysTranslateOfferThreshold? Done.
The CQ bit was checked by ftang@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.
Almost there - this is mostly nits. Thank you for your patience! https://codereview.chromium.org/2090283002/diff/100001/components/translate/c... File components/translate/core/browser/translate_prefs.cc (right): https://codereview.chromium.org/2090283002/diff/100001/components/translate/c... components/translate/core/browser/translate_prefs.cc:40: "always-translate-offer-threshold"; Our prefs usually use underscores instead of hyphens - I'd suggest doing the same for variation params (unless finch requires this style?) https://codereview.chromium.org/2090283002/diff/100001/components/translate/c... File components/translate/core/browser/translate_prefs.h (right): https://codereview.chromium.org/2090283002/diff/100001/components/translate/c... components/translate/core/browser/translate_prefs.h:38: // The trail (study) name in finch study config. trial :) https://codereview.chromium.org/2090283002/diff/100001/components/translate/c... components/translate/core/browser/translate_prefs.h:41: // The name of the parameter for the number of translation trigger the "Always ....for the number of translations, *after which* the "Always.... https://codereview.chromium.org/2090283002/diff/100001/components/translate/c... File components/translate/core/browser/translate_ui_delegate.cc (right): https://codereview.chromium.org/2090283002/diff/100001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:314: auto it = params.find(translate::kAlwaysTranslateOfferThreshold); Why not the shorter variant? (I'm fine with using find, I'm just curious) https://codereview.chromium.org/2090283002/diff/100001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:323: if (always_translate_offer_threshold < 0) { <= 0? https://codereview.chromium.org/2090283002/diff/100001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:334: return prefs_->GetTranslationAcceptedCount(GetOriginalLanguageCode()) == You really want greater-equals here. Otherwise, you miss all the cases where users already have a sufficient accept count before they receive this upgrade. https://codereview.chromium.org/2090283002/diff/100001/components/translate/c... File components/translate/core/browser/translate_ui_delegate_unittest.cc (right): https://codereview.chromium.org/2090283002/diff/100001/components/translate/c... components/translate/core/browser/translate_ui_delegate_unittest.cc:214: params[translate::kAlwaysTranslateOfferThreshold] = "2"; Use C++11 initializer lists: std::map<std::string, std::string> params = { {kAlwaysTranslateOfferThreshold, "2"} };
update
PTAL https://codereview.chromium.org/2090283002/diff/100001/components/translate/c... File components/translate/core/browser/translate_prefs.cc (right): https://codereview.chromium.org/2090283002/diff/100001/components/translate/c... components/translate/core/browser/translate_prefs.cc:40: "always-translate-offer-threshold"; On 2016/06/28 17:35:20, groby wrote: > Our prefs usually use underscores instead of hyphens - I'd suggest doing the > same for variation params (unless finch requires this style?) Done. https://codereview.chromium.org/2090283002/diff/100001/components/translate/c... File components/translate/core/browser/translate_prefs.h (right): https://codereview.chromium.org/2090283002/diff/100001/components/translate/c... components/translate/core/browser/translate_prefs.h:38: // The trail (study) name in finch study config. On 2016/06/28 17:35:20, groby wrote: > trial :) Done. https://codereview.chromium.org/2090283002/diff/100001/components/translate/c... components/translate/core/browser/translate_prefs.h:41: // The name of the parameter for the number of translation trigger the "Always On 2016/06/28 17:35:20, groby wrote: > ....for the number of translations, *after which* the "Always.... > Done. https://codereview.chromium.org/2090283002/diff/100001/components/translate/c... File components/translate/core/browser/translate_ui_delegate.cc (right): https://codereview.chromium.org/2090283002/diff/100001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:314: auto it = params.find(translate::kAlwaysTranslateOfferThreshold); On 2016/06/28 17:35:20, groby wrote: > Why not the shorter variant? (I'm fine with using find, I'm just curious) Done. https://codereview.chromium.org/2090283002/diff/100001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:323: if (always_translate_offer_threshold < 0) { On 2016/06/28 17:35:20, groby wrote: > <= 0? Done. https://codereview.chromium.org/2090283002/diff/100001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:334: return prefs_->GetTranslationAcceptedCount(GetOriginalLanguageCode()) == On 2016/06/28 17:35:20, groby wrote: > You really want greater-equals here. Otherwise, you miss all the cases where > users already have a sufficient accept count before they receive this upgrade. The problem of that is, then if the user explicitly uncheck the checked checkbox, it will show up again as checked the next time, which is not the right thing to do. https://codereview.chromium.org/2090283002/diff/100001/components/translate/c... File components/translate/core/browser/translate_ui_delegate_unittest.cc (right): https://codereview.chromium.org/2090283002/diff/100001/components/translate/c... components/translate/core/browser/translate_ui_delegate_unittest.cc:214: params[translate::kAlwaysTranslateOfferThreshold] = "2"; On 2016/06/28 17:35:20, groby wrote: > Use C++11 initializer lists: > > std::map<std::string, std::string> params = { {kAlwaysTranslateOfferThreshold, > "2"} }; cool. we can use C++11 in Chrome too? First time to know.
The CQ bit was checked by ftang@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 groby@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ftang@chromium.org changed reviewers: + rkaplow@chromium.org
rkaplow - need your LGTM since I added +components/variations to components/translate/DEPS
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
ftang@chromium.org changed reviewers: + asvitkine@google.com
asvitkine@chromium.org changed reviewers: + asvitkine@chromium.org
lgtm https://codereview.chromium.org/2090283002/diff/120001/components/translate/c... File components/translate/core/browser/translate_ui_delegate.cc (right): https://codereview.chromium.org/2090283002/diff/120001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:311: int threshold = 0; Nit: move this above to where it's used - i.e. above StringToInt call. https://codereview.chromium.org/2090283002/diff/120001/components/translate/c... File components/translate/core/browser/translate_ui_delegate_unittest.cc (right): https://codereview.chromium.org/2090283002/diff/120001/components/translate/c... components/translate/core/browser/translate_ui_delegate_unittest.cc:211: new base::FieldTrialList(NULL)); Nit: Just create this on the stack. i.e. base::FieldTrialList trial_list(nullptr);
address review issues
https://codereview.chromium.org/2090283002/diff/120001/components/translate/c... File components/translate/core/browser/translate_ui_delegate.cc (right): https://codereview.chromium.org/2090283002/diff/120001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:311: int threshold = 0; On 2016/06/30 13:51:27, Alexei Svitkine (very slow) wrote: > Nit: move this above to where it's used - i.e. above StringToInt call. Done.
The CQ bit was checked by ftang@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 ftang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from groby@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2090283002/#ps140001 (title: "address review issues")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== add experimental param to control the triggering threshold to default Always Translate checked BUG=622854 ========== to ========== add experimental param to control the triggering threshold to default Always Translate checked BUG=622854 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== add experimental param to control the triggering threshold to default Always Translate checked BUG=622854 ========== to ========== add experimental param to control the triggering threshold to default Always Translate checked BUG=622854 Committed: https://crrev.com/2364a394e98147f92f80ddeb5a1c56abbb547951 Cr-Commit-Position: refs/heads/master@{#403232} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/2364a394e98147f92f80ddeb5a1c56abbb547951 Cr-Commit-Position: refs/heads/master@{#403232} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
