Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2200493002:
using ulp to improve TranslateManager GetTargetLanguage() and InitiateTranslation() (Closed)
Descriptionusing ulp to improve TranslateManager GetTargetLanguage() and InitiateTranslation()
Design doc http://go/ulp_chrome_translate
BUG=632897
Committed: https://crrev.com/9658047b62714181886bd2fed9f8246e635b70f3
Cr-Commit-Position: refs/heads/master@{#410939}
Patch Set 1 #Patch Set 2 : add unit test for translate_prefs.cc #Patch Set 3 : Make the TranslateManager change test friendly #Patch Set 4 : add unit tests #Patch Set 5 : fix windows test breakage #Patch Set 6 : fix chromeos breakage #Patch Set 7 : add unit tests #
Total comments: 60
Patch Set 8 : address review comments #Patch Set 9 : Change based on 8/3 design review and simplified the use of ULP #
Total comments: 20
Patch Set 10 : address review comments #Patch Set 11 : merge #Patch Set 12 : attempt to fix iOS #Patch Set 13 : fix global state problem in unittests #
Total comments: 6
Messages
Total messages: 71 (44 generated)
add unit test for translate_prefs.cc
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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Make the TranslateManager change test friendly
add unit tests
fix windows breakage due to uninitialized ptr
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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
fix chromeos brekage
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.
add unit tests
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.
ftang@chromium.org changed reviewers: + groby@chromium.org
See design doc in http://go/ulp_chrome_translate
Sorry, a whole bunch of comments - the design doc must have slipped out of my read queue :( https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... File components/translate/core/browser/translate_browser_metrics_unittest.cc (right): https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_browser_metrics_unittest.cc:176: translate::TranslateBrowserMetrics::ReportInitiationStatus( I really don't think this entire test makes sense - it's testing that the UMA reporting macro works. (Sure, one or two tests, but you don't need full coverage here) https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... File components/translate/core/browser/translate_manager.cc (right): https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_manager.cc:62: const char kTranslateLanguageByULPTrialName[] = "TranslateLanguageByULP"; Don't need the trial name - just use GetVariationParamsByFeature https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_manager.cc:67: const char kInitiateTranslationReadingConfidenceThreshold[] = I'm not sure what this threshold (and the following) is supposed to control - can you add a comment, please? https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_manager.cc:76: bool InListAboveThreshold(const std::string language_code, That combines logic and retrieval - how about just a GetLanguageThreshold function that takes a probability list? https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_manager.cc:80: // We only consider the language if the probability > the thresold. c++11 loop iteration, please for(const auto& it : list) { if (it.second > threshold) .... https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_manager.cc:270: base::StringToDouble( Probably worth factoring out to make this a bit more readable. https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_manager.cc:291: TranslatePrefs::LanguageProbabilityList reading; It seems odd we get this data piece by piece, and then process it in several different places. Personally, I think we should either get a single chunk of ULPData from the preferences, or alternatively, have a GetReadingProbability/GetWritingProbability for a single language. What do you think? https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_manager.cc:438: std::string language = TranslateDownloadManager::GetLanguageCode( Please keep a comment that this is the user's UI language. https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_manager.cc:445: translate::ToTranslateLanguageSynonym(&language); I'm not happy we duplicate this here and in GetTargetLanguageFromULP. Is there any way we can remove that? (I know, we currently need this because ULP only overrides if it's valid...) https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_manager.cc:447: TranslateExperiment::OverrideUiLanguage(prefs->GetCountry(), &language); Are you sure you want to do this after ULP? This targets specifically people who are in a given country, with a given UI language. (ID/MY, with en_XX as UI language) https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_manager.cc:472: double reading_confidence_threshold = We have default values duplicated across two functions - we should probably define them as constants instead. https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_manager.cc:486: for (auto it = reading.begin(); it != reading.end(); ++it) { C++11 style loop, please. https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_manager.cc:486: for (auto it = reading.begin(); it != reading.end(); ++it) { We do a lot of "iterate and compare language code" - maybe this should be transcoded in a map? https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_manager.cc:490: translate::ToTranslateLanguageSynonym(&ulp_language); It seems we _always_ treat ulp languages as TranslateLanguageSynonym - maybe we should convert them once, when reading the data? (And also filter out the invalid ones) https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_manager.cc:491: // Also the language to be supported by Translate. s/to be/is/ https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_manager.cc:494: } In general, this is very deeply nested. I'd prefer it if you instead did early-out - i.e. std::string target_language; if (GetReading <= reading_confidence_threshold) return target_language; for(..) { if (it->second > reading_probability) { language = it->first; } } if (!TDM::IsSupported) language.clear(); return language; https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... File components/translate/core/browser/translate_manager_unittest.cc (right): https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_manager_unittest.cc:242: class MockTranslatePrefs : public TranslatePrefs { Please don't mock translate prefs. This seems to just repeat the test from TranslatePrefs as far as I can tell? But even if it doesn't, you already have TestingPrefSyncable available via prefs_/translate_prefs_ https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... File components/translate/core/browser/translate_prefs.cc (right): https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_prefs.cc:23: const char TranslatePrefs::kPrefLanguageProfile[] = "language_profile"; If ULP is translate specific, it should be translate_language_profile. If it's not, there should probably be a follow-up bug to separate it out into its own component. https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_prefs.cc:585: // Functions to read from User Language Profile Probably might want to document that via a test instead. https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_prefs.cc:619: LanguageProbabilityList* out_value) const { That's a lot of ad-hoc work - a JSONValueConverter might be a better idea. If you want to keep manual parsing, a bunch of comments below - otherwise, you can ignore those. https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_prefs.cc:623: if (dict == nullptr || dict->empty() || 1) No need to check dict->empty, because GetDictionary will then return a nullptr 2) Please don't do a bunch of work in if statements. This would probably cleaner as if (!dict) return 0.0; if (!dict->GetDictionary(path, &entries)) return 0.0; https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_prefs.cc:630: !entries->GetList(kItems, &list) || list == nullptr || list->empty()) Same here - if GetList is true, list is not null. https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_prefs.cc:634: for (size_t i = 0; i < list->GetSize(); i++) { ListValue is iterable, so C++11 iteration works for (const auto& entry : list) https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_prefs.cc:636: if (list->GetDictionary(i, &item) && item != nullptr && !item->empty() && See above :) https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_prefs.cc:655: int TranslatePrefs::GetUniversialLanguageSettings( I didn't see any consumer of this function yet? https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_prefs.cc:656: std::vector<std::string>* out_value) const { Why use an outvalue? Just return std::vector<std::string> (There is no cost, since it is eligible for NRVO) https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_prefs.cc:660: if (dict == nullptr || dict->empty() || !dict->GetList(kSettings, &list) || See above. https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... File components/translate/core/browser/translate_prefs.h (right): https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_prefs.h:170: typedef std::pair<std::string, double> LanguageProbability; LanguageAndProbability, please (so it's obvious this is a pair) https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_prefs.h:185: // Output the order list of Universal Language Settings (ULS) to |list| What is the order list? https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_prefs.h:186: // and return the number of item on the list or 0 if there are no ULS list. The number of items should be available via |list->size()|, correct? So we probably don't need to return that. https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... File components/translate/core/browser/translate_prefs_unittest.cc (right): https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_prefs_unittest.cc:224: base::DictionaryValue profile; I'd recommend building this as JSON, and then using JSONReader to build the final prefs entry. That should be a bit more readable. (Plus it documents the structure)
address review comments
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...
Description was changed from ========== using ulp to improve TranslateManager GetTargetLanguage() and InitiateTranslation() BUG=632897 ========== to ========== using ulp to improve TranslateManager GetTargetLanguage() and InitiateTranslation() Design doc http://go/ulp_chrome_translate BUG=632897 ==========
PTAL . Also I change some part in the design doc to reflect it. http://go/ulp_chrome_translate https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... File components/translate/core/browser/translate_browser_metrics_unittest.cc (right): https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_browser_metrics_unittest.cc:176: translate::TranslateBrowserMetrics::ReportInitiationStatus( On 2016/08/02 00:38:20, groby wrote: > I really don't think this entire test makes sense - it's testing that the UMA > reporting macro works. (Sure, one or two tests, but you don't need full coverage > here) What do you suggest me to do wi this? Not making change to this file? https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... File components/translate/core/browser/translate_manager.cc (right): https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_manager.cc:62: const char kTranslateLanguageByULPTrialName[] = "TranslateLanguageByULP"; On 2016/08/02 00:38:20, groby wrote: > Don't need the trial name - just use GetVariationParamsByFeature Done. https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_manager.cc:67: const char kInitiateTranslationReadingConfidenceThreshold[] = On 2016/08/02 00:38:20, groby wrote: > I'm not sure what this threshold (and the following) is supposed to control - > can you add a comment, please? Done. https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_manager.cc:76: bool InListAboveThreshold(const std::string language_code, On 2016/08/02 00:38:20, groby wrote: > That combines logic and retrieval - how about just a GetLanguageThreshold > function that takes a probability list? Done. https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_manager.cc:80: // We only consider the language if the probability > the thresold. On 2016/08/02 00:38:20, groby wrote: > c++11 loop iteration, please > > for(const auto& it : list) { > if (it.second > threshold) > .... > Done. https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_manager.cc:270: base::StringToDouble( On 2016/08/02 00:38:20, groby wrote: > Probably worth factoring out to make this a bit more readable. Done. https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_manager.cc:291: TranslatePrefs::LanguageProbabilityList reading; On 2016/08/02 00:38:20, groby wrote: > It seems odd we get this data piece by piece, and then process it in several > different places. > > Personally, I think we should either get a single chunk of ULPData from the > preferences, or alternatively, have a > GetReadingProbability/GetWritingProbability for a single language. > > What do you think? Made some changes, not sure it fit what you ask for. Take a look the new code first. https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_manager.cc:438: std::string language = TranslateDownloadManager::GetLanguageCode( On 2016/08/02 00:38:20, groby wrote: > Please keep a comment that this is the user's UI language. Done. https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_manager.cc:445: translate::ToTranslateLanguageSynonym(&language); On 2016/08/02 00:38:20, groby wrote: > I'm not happy we duplicate this here and in GetTargetLanguageFromULP. Is there > any way we can remove that? (I know, we currently need this because ULP only > overrides if it's valid...) Done. https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_manager.cc:447: TranslateExperiment::OverrideUiLanguage(prefs->GetCountry(), &language); On 2016/08/02 00:38:20, groby wrote: > Are you sure you want to do this after ULP? This targets specifically people who > are in a given country, with a given UI language. (ID/MY, with en_XX as UI > language) Done. https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_manager.cc:472: double reading_confidence_threshold = On 2016/08/02 00:38:20, groby wrote: > We have default values duplicated across two functions - we should probably > define them as constants instead. no they are not. We are not reuse threhold in these two functions. I also do not want to hard code the threshold since we don't know what is the right threshold. So I like to put them into the config which can be change by experiment config. https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_manager.cc:486: for (auto it = reading.begin(); it != reading.end(); ++it) { On 2016/08/02 00:38:20, groby wrote: > We do a lot of "iterate and compare language code" - maybe this should be > transcoded in a map? I think about that, but then we will lost the "order". https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_manager.cc:486: for (auto it = reading.begin(); it != reading.end(); ++it) { On 2016/08/02 00:38:20, groby wrote: > C++11 style loop, please. Done. https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_manager.cc:490: translate::ToTranslateLanguageSynonym(&ulp_language); On 2016/08/02 00:38:20, groby wrote: > It seems we _always_ treat ulp languages as TranslateLanguageSynonym - maybe we > should convert them once, when reading the data? (And also filter out the > invalid ones) Done in the TranslatePrefs now. https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_manager.cc:491: // Also the language to be supported by Translate. On 2016/08/02 00:38:20, groby wrote: > s/to be/is/ Acknowledged. https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... File components/translate/core/browser/translate_manager_unittest.cc (right): https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_manager_unittest.cc:242: class MockTranslatePrefs : public TranslatePrefs { On 2016/08/02 00:38:21, groby wrote: > Please don't mock translate prefs. This seems to just repeat the test from > TranslatePrefs as far as I can tell? > > But even if it doesn't, you already have TestingPrefSyncable available via > prefs_/translate_prefs_ I have to mock the TranslatePrefs so I can test the logic (considering the threshold, etc) in TranslateManager.cc . There are no repeated test. https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... File components/translate/core/browser/translate_prefs.cc (right): https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_prefs.cc:585: // Functions to read from User Language Profile On 2016/08/02 00:38:21, groby wrote: > Probably might want to document that via a test instead. Acknowledged. https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_prefs.cc:623: if (dict == nullptr || dict->empty() || On 2016/08/02 00:38:21, groby wrote: > 1) No need to check dict->empty, because GetDictionary will then return a > nullptr > > 2) Please don't do a bunch of work in if statements. This would probably cleaner > as > > if (!dict) > return 0.0; > if (!dict->GetDictionary(path, &entries)) > return 0.0; > Done. https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_prefs.cc:630: !entries->GetList(kItems, &list) || list == nullptr || list->empty()) On 2016/08/02 00:38:21, groby wrote: > Same here - if GetList is true, list is not null. Done. https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_prefs.cc:636: if (list->GetDictionary(i, &item) && item != nullptr && !item->empty() && On 2016/08/02 00:38:21, groby wrote: > See above :) Done. https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_prefs.cc:655: int TranslatePrefs::GetUniversialLanguageSettings( On 2016/08/02 00:38:21, groby wrote: > I didn't see any consumer of this function yet? you are right. Removed. We should keep the ULP prefs in the prefs but no need to get the information out for TranslatePrefs. https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_prefs.cc:656: std::vector<std::string>* out_value) const { On 2016/08/02 00:38:21, groby wrote: > Why use an outvalue? Just return std::vector<std::string> (There is no cost, > since it is eligible for NRVO) > > since I remove this function. It does not matter any more. https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_prefs.cc:660: if (dict == nullptr || dict->empty() || !dict->GetList(kSettings, &list) || On 2016/08/02 00:38:21, groby wrote: > See above. Acknowledged. https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... File components/translate/core/browser/translate_prefs.h (right): https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_prefs.h:170: typedef std::pair<std::string, double> LanguageProbability; On 2016/08/02 00:38:21, groby wrote: > LanguageAndProbability, please (so it's obvious this is a pair) Done. https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_prefs.h:185: // Output the order list of Universal Language Settings (ULS) to |list| On 2016/08/02 00:38:21, groby wrote: > What is the order list? function removed. https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_prefs.h:186: // and return the number of item on the list or 0 if there are no ULS list. On 2016/08/02 00:38:21, groby wrote: > The number of items should be available via |list->size()|, correct? So we > probably don't need to return that. function removed. https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... File components/translate/core/browser/translate_prefs_unittest.cc (right): https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_prefs_unittest.cc:224: base::DictionaryValue profile; On 2016/08/02 00:38:21, groby wrote: > I'd recommend building this as JSON, and then using JSONReader to build the > final prefs entry. That should be a bit more readable. (Plus it documents the > structure) > Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
groby- please HOLD your review on this. We have some discussion in the design review and will make a big change now (simplified some)
change based on 8/3 design review to simplified the use of ULP
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Thanks for the quick changes! https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... File components/translate/core/browser/translate_browser_metrics_unittest.cc (right): https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_browser_metrics_unittest.cc:176: translate::TranslateBrowserMetrics::ReportInitiationStatus( On 2016/08/03 02:01:17, ftang wrote: > On 2016/08/02 00:38:20, groby wrote: > > I really don't think this entire test makes sense - it's testing that the UMA > > reporting macro works. (Sure, one or two tests, but you don't need full > coverage > > here) > > What do you suggest me to do wi this? Not making change to this file? Yes, I really think we can skip this. It's a lot of bookkeeping, and I don't see any potential errors uncovered by this. https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... File components/translate/core/browser/translate_manager.cc (right): https://codereview.chromium.org/2200493002/diff/110001/components/translate/c... components/translate/core/browser/translate_manager.cc:472: double reading_confidence_threshold = On 2016/08/03 02:01:17, ftang wrote: > On 2016/08/02 00:38:20, groby wrote: > > We have default values duplicated across two functions - we should probably > > define them as constants instead. > > no they are not. We are not reuse threhold in these two functions. > I also do not want to hard code the threshold since we don't know what is the > right threshold. So I like to put them into the config which can be change by > experiment config. I'm absolutely on board with configuring via Finch. I specifically refer to the values that you use as default if StringToDouble fails (i.e. 0.75 for reading confidence, 0.5 for reading probability, etc.) These should probably be constants. https://codereview.chromium.org/2200493002/diff/150001/components/translate/c... File components/translate/core/browser/translate_manager.cc (right): https://codereview.chromium.org/2200493002/diff/150001/components/translate/c... components/translate/core/browser/translate_manager.cc:81: const double kDefaultTargetLanguageULPProbabilityThreshold = 0.55; Can we share the same defaults? Since they can be overridden for an experiment anyways, I'm not sure we need to have separate constants. https://codereview.chromium.org/2200493002/diff/150001/components/translate/c... components/translate/core/browser/translate_manager.cc:492: for (const auto& it : reading) { Question: Isn't the list sorted by decreasing probability? In which case we can just test the first element? https://codereview.chromium.org/2200493002/diff/150001/components/translate/c... File components/translate/core/browser/translate_manager.h (right): https://codereview.chromium.org/2200493002/diff/150001/components/translate/c... components/translate/core/browser/translate_manager.h:26: // Feature flag for "Translate Language by ULP" project. You can probably skip this comment - it says nothing the code doesn't say. https://codereview.chromium.org/2200493002/diff/150001/components/translate/c... File components/translate/core/browser/translate_manager_unittest.cc (right): https://codereview.chromium.org/2200493002/diff/150001/components/translate/c... components/translate/core/browser/translate_manager_unittest.cc:242: class MockTranslatePrefs : public TranslatePrefs { Please don't mock this - use actual TranslatePrefs https://codereview.chromium.org/2200493002/diff/150001/components/translate/c... File components/translate/core/browser/translate_prefs.cc (right): https://codereview.chromium.org/2200493002/diff/150001/components/translate/c... components/translate/core/browser/translate_prefs.cc:584: // The structure of User Language Profile looks like below: The structure is already explained in hte test, no need to duplicate that here. https://codereview.chromium.org/2200493002/diff/150001/components/translate/c... components/translate/core/browser/translate_prefs.cc:630: const base::ListValue* preference = nullptr; please move ctor to where this is used (after next if) https://codereview.chromium.org/2200493002/diff/150001/components/translate/c... components/translate/core/browser/translate_prefs.cc:655: // Put into the vector only if the normalized version is supported. It's not a vector yet :) Maybe "discard if the normalized version is unsupported"? https://codereview.chromium.org/2200493002/diff/150001/components/translate/c... components/translate/core/browser/translate_prefs.cc:657: probability_map[language] += probability; I'm not sure why the bots think probability might not be initialized... just move language and probability into the loop, intialize probability as 0.0 https://codereview.chromium.org/2200493002/diff/150001/components/translate/c... File components/translate/core/browser/translate_prefs.h (right): https://codereview.chromium.org/2200493002/diff/150001/components/translate/c... components/translate/core/browser/translate_prefs.h:174: // ordered list of <string, double> pair. Return the confidence of the list What is the list ordered by? I assume the double? https://codereview.chromium.org/2200493002/diff/150001/components/translate/c... components/translate/core/browser/translate_prefs.h:176: virtual double GetReadingFromUserLanguageProfile( Please don't make this virtual (relates to the "no mock" comment)
address review comments
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/2200493002/diff/150001/components/translate/c... File components/translate/core/browser/translate_manager.cc (right): https://codereview.chromium.org/2200493002/diff/150001/components/translate/c... components/translate/core/browser/translate_manager.cc:81: const double kDefaultTargetLanguageULPProbabilityThreshold = 0.55; On 2016/08/04 02:33:42, groby wrote: > Can we share the same defaults? Since they can be overridden for an experiment > anyways, I'm not sure we need to have separate constants. No, we really cannot use the same threshold. These are two kind of different decision and it will be very difficult to tune and figure the right threshold later if we decide to combine them. Think about this- one set of the (confidence/probability) threshold is for deciding which ONE language is likely to be the user prefer to translate the page into, and the second one is to decide which SET of languages that we would like to suppressed the showing the Translate Bubble UI. It is likely we will choose the threshold more strict on one but more relax on the other. https://codereview.chromium.org/2200493002/diff/150001/components/translate/c... components/translate/core/browser/translate_manager.cc:492: for (const auto& it : reading) { On 2016/08/04 02:33:42, groby wrote: > Question: Isn't the list sorted by decreasing probability? In which case we can > just test the first element? Done. You are right, the for loop is needed before the normalization of the language code. I forget to change it after that. Sorry. https://codereview.chromium.org/2200493002/diff/150001/components/translate/c... File components/translate/core/browser/translate_manager.h (right): https://codereview.chromium.org/2200493002/diff/150001/components/translate/c... components/translate/core/browser/translate_manager.h:26: // Feature flag for "Translate Language by ULP" project. On 2016/08/04 02:33:42, groby wrote: > You can probably skip this comment - it says nothing the code doesn't say. Done. https://codereview.chromium.org/2200493002/diff/150001/components/translate/c... File components/translate/core/browser/translate_manager_unittest.cc (right): https://codereview.chromium.org/2200493002/diff/150001/components/translate/c... components/translate/core/browser/translate_manager_unittest.cc:242: class MockTranslatePrefs : public TranslatePrefs { On 2016/08/04 02:33:42, groby wrote: > Please don't mock this - use actual TranslatePrefs Done. https://codereview.chromium.org/2200493002/diff/150001/components/translate/c... File components/translate/core/browser/translate_prefs.cc (right): https://codereview.chromium.org/2200493002/diff/150001/components/translate/c... components/translate/core/browser/translate_prefs.cc:584: // The structure of User Language Profile looks like below: On 2016/08/04 02:33:42, groby wrote: > The structure is already explained in hte test, no need to duplicate that here. Done. https://codereview.chromium.org/2200493002/diff/150001/components/translate/c... components/translate/core/browser/translate_prefs.cc:630: const base::ListValue* preference = nullptr; On 2016/08/04 02:33:42, groby wrote: > please move ctor to where this is used (after next if) Done. https://codereview.chromium.org/2200493002/diff/150001/components/translate/c... components/translate/core/browser/translate_prefs.cc:655: // Put into the vector only if the normalized version is supported. On 2016/08/04 02:33:42, groby wrote: > It's not a vector yet :) Maybe "discard if the normalized version is > unsupported"? Done. https://codereview.chromium.org/2200493002/diff/150001/components/translate/c... components/translate/core/browser/translate_prefs.cc:657: probability_map[language] += probability; On 2016/08/04 02:33:42, groby wrote: > I'm not sure why the bots think probability might not be initialized... just > move language and probability into the loop, intialize probability as 0.0 Done. https://codereview.chromium.org/2200493002/diff/150001/components/translate/c... File components/translate/core/browser/translate_prefs.h (right): https://codereview.chromium.org/2200493002/diff/150001/components/translate/c... components/translate/core/browser/translate_prefs.h:174: // ordered list of <string, double> pair. Return the confidence of the list On 2016/08/04 02:33:43, groby wrote: > What is the list ordered by? I assume the double? Done. https://codereview.chromium.org/2200493002/diff/150001/components/translate/c... components/translate/core/browser/translate_prefs.h:176: virtual double GetReadingFromUserLanguageProfile( On 2016/08/04 02:33:42, groby wrote: > Please don't make this virtual (relates to the "no mock" comment) Done.
lgtm
The CQ bit was checked by ftang@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 on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
merge
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 Link to the patchset: https://codereview.chromium.org/2200493002/#ps190001 (title: "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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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 on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
attempt to fix iOS
fix global state problem in unittests
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 Link to the patchset: https://codereview.chromium.org/2200493002/#ps230001 (title: "fix global state problem in unittests")
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 ========== using ulp to improve TranslateManager GetTargetLanguage() and InitiateTranslation() Design doc http://go/ulp_chrome_translate BUG=632897 ========== to ========== using ulp to improve TranslateManager GetTargetLanguage() and InitiateTranslation() Design doc http://go/ulp_chrome_translate BUG=632897 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:230001)
Message was sent while issue was closed.
Description was changed from ========== using ulp to improve TranslateManager GetTargetLanguage() and InitiateTranslation() Design doc http://go/ulp_chrome_translate BUG=632897 ========== to ========== using ulp to improve TranslateManager GetTargetLanguage() and InitiateTranslation() Design doc http://go/ulp_chrome_translate BUG=632897 Committed: https://crrev.com/9658047b62714181886bd2fed9f8246e635b70f3 Cr-Commit-Position: refs/heads/master@{#410939} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/9658047b62714181886bd2fed9f8246e635b70f3 Cr-Commit-Position: refs/heads/master@{#410939}
Message was sent while issue was closed.
thestig@chromium.org changed reviewers: + thestig@chromium.org
Message was sent while issue was closed.
(Drive by) Some generic coding improvement suggestions. https://codereview.chromium.org/2200493002/diff/230001/components/translate/c... File components/translate/core/browser/translate_manager.cc (right): https://codereview.chromium.org/2200493002/diff/230001/components/translate/c... components/translate/core/browser/translate_manager.cc:108: double GetDoubleFromMap(std::map<std::string, std::string>& map, So "git cl lint" noticed this is being passed by non-const reference... map[key] will actually created a new entry with the key |key| if it did not exist before. Is that intentional? Not sure if this matters. https://codereview.chromium.org/2200493002/diff/230001/components/translate/c... components/translate/core/browser/translate_manager.cc:296: if (translate_client_->GetTranslatePrefs()->GetReadingFromUserLanguageProfile( nit: "if (eval) return true; return false" -> return eval; https://codereview.chromium.org/2200493002/diff/230001/components/translate/c... components/translate/core/browser/translate_manager.cc:489: if (reading.size() > 0 && nit: !reading.empty() is a wee bit shorter https://codereview.chromium.org/2200493002/diff/230001/components/translate/c... File components/translate/core/browser/translate_manager.h (right): https://codereview.chromium.org/2200493002/diff/230001/components/translate/c... components/translate/core/browser/translate_manager.h:15: #include "base/gtest_prod_util.h" Not needed, but I imagine it's here because you wanted to use FRIEND_TEST_ALL_PREFIXES at some point earlier, and then forgot to remove it? https://codereview.chromium.org/2200493002/diff/230001/components/translate/c... File components/translate/core/browser/translate_prefs.h (right): https://codereview.chromium.org/2200493002/diff/230001/components/translate/c... components/translate/core/browser/translate_prefs.h:170: typedef std::pair<std::string, double> LanguageAndProbability; nit: using LanguageAndProbability = std::pair... is preferred. https://codereview.chromium.org/2200493002/diff/230001/components/translate/c... File components/translate/core/browser/translate_prefs_unittest.cc (right): https://codereview.chromium.org/2200493002/diff/230001/components/translate/c... components/translate/core/browser/translate_prefs_unittest.cc:246: EXPECT_EQ(2UL, list.size()); This should be an ASSERT_EQ(), otherwise if this fails the next |list| access can cause the entire unit tests binary to crash. Ditto below on lines 294 and 331. |
