Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1903593002:
Allows to manually change the stored permanent country in Variations from the translate-internals d… (Closed)
|
Created:
4 years, 8 months ago by hamelphi Modified:
4 years, 8 months ago CC:
chromium-reviews, arv+watch_chromium.org, asvitkine+watch_chromium.org, hajimehoshi Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllows to manually change the stored permanent country in Variations from the translate-internals dashboard.
R=asvitkine@chromium.org,toyoshim@chromium.org
BUG=599325
Committed: https://crrev.com/2d297251935984323c30d2faf501aa13fd5d479a
Cr-Commit-Position: refs/heads/master@{#389501}
Patch Set 1 #Patch Set 2 : Finished unittest #Patch Set 3 : nits #Patch Set 4 : UI polishing #
Total comments: 27
Patch Set 5 : #
Total comments: 14
Patch Set 6 : #Patch Set 7 : #
Total comments: 6
Patch Set 8 : #Patch Set 9 : #
Total comments: 11
Patch Set 10 : #
Total comments: 4
Patch Set 11 : #Messages
Total messages: 30 (8 generated)
Generally looks good. Can you post a screenshot of how the UI looks like as an attachment on the bug? https://codereview.chromium.org/1903593002/diff/60001/chrome/browser/resource... File chrome/browser/resources/translate_internals/translate_internals.js (right): https://codereview.chromium.org/1903593002/diff/60001/chrome/browser/resource... chrome/browser/resources/translate_internals/translate_internals.js:255: Nit: Remove extra line. https://codereview.chromium.org/1903593002/diff/60001/chrome/browser/resource... chrome/browser/resources/translate_internals/translate_internals.js:272: // FIXME: what is the max length of a country code? We can probably just not set this. https://codereview.chromium.org/1903593002/diff/60001/chrome/browser/translat... File chrome/browser/translate/chrome_translate_client.cc (right): https://codereview.chromium.org/1903593002/diff/60001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client.cc:179: } else { No need for else if there's an early return. https://codereview.chromium.org/1903593002/diff/60001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client.cc:180: LOG(ERROR) << "Could not get variations service."; Nit: Make this a DLOG() so we don't bloat the production binary with these strings. https://codereview.chromium.org/1903593002/diff/60001/chrome/browser/translat... File chrome/browser/translate/chrome_translate_client.h (right): https://codereview.chromium.org/1903593002/diff/60001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client.h:60: // kVariationsPermanentConsistencyCountry. Returns true if the variable has Nit: No need to say "Helper method". Also, the pref name is an implementation detail. Maybe describe the purpose instead: "Overrides the stored permanent consistency country in prefs. This is intended to be invoked from a debug UI to allow easy testing of different-country configurations." https://codereview.chromium.org/1903593002/diff/60001/components/variations/s... File components/variations/service/variations_service.cc (right): https://codereview.chromium.org/1903593002/diff/60001/components/variations/s... components/variations/service/variations_service.cc:869: // FIXME: Can we check for valid country? Probably not really feasible. https://codereview.chromium.org/1903593002/diff/60001/components/variations/s... components/variations/service/variations_service.cc:870: if(country_override.empty()) { Nit: space after if. Also, no {}'s on a one-line if - same on line 884. You can run "git cl format" https://codereview.chromium.org/1903593002/diff/60001/components/variations/s... components/variations/service/variations_service.cc:885: return false; Not sure we actually need this logic. What's the danger of just doing it unconditionally? https://codereview.chromium.org/1903593002/diff/60001/components/variations/s... components/variations/service/variations_service.cc:893: new_list_value); Can you make a helper function for this logic since it seems to be used in the other function that changes this? https://codereview.chromium.org/1903593002/diff/60001/components/variations/s... File components/variations/service/variations_service_unittest.cc (right): https://codereview.chromium.org/1903593002/diff/60001/components/variations/s... components/variations/service/variations_service_unittest.cc:762: if (test.pref_value_before != "") { Nit: !.empty()
https://codereview.chromium.org/1903593002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc (right): https://codereview.chromium.org/1903593002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:255: std::string country = translate_prefs->GetCountry(); I actually don't quite follow this logic. What does the translate prefs GetCountry() have to do with the variations country? I think the two are separate, so if you want to fill in the existing variations country, I think you need to get it from variations service. Or am I missing something here? https://codereview.chromium.org/1903593002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:259: dict.Set("country", new base::StringValue(translate_prefs->GetCountry())); Nit: You have |country| in a local var already. https://codereview.chromium.org/1903593002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/translate_internals/translate_internals_handler.h (right): https://codereview.chromium.org/1903593002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/translate_internals/translate_internals_handler.h:83: // Sends the stored permanent country to JavaScript. Nit: Document |is_update| param. Also, JavaScript -> Javascript
https://codereview.chromium.org/1903593002/diff/60001/chrome/browser/resource... File chrome/browser/resources/translate_internals/translate_internals.js (right): https://codereview.chromium.org/1903593002/diff/60001/chrome/browser/resource... chrome/browser/resources/translate_internals/translate_internals.js:255: On 2016/04/20 20:30:23, Alexei Svitkine wrote: > Nit: Remove extra line. Done. https://codereview.chromium.org/1903593002/diff/60001/chrome/browser/resource... chrome/browser/resources/translate_internals/translate_internals.js:272: // FIXME: what is the max length of a country code? On 2016/04/20 20:30:23, Alexei Svitkine wrote: > We can probably just not set this. Done. https://codereview.chromium.org/1903593002/diff/60001/chrome/browser/translat... File chrome/browser/translate/chrome_translate_client.cc (right): https://codereview.chromium.org/1903593002/diff/60001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client.cc:179: } else { On 2016/04/20 20:30:23, Alexei Svitkine wrote: > No need for else if there's an early return. Removed changes from chrome_translate_client. https://codereview.chromium.org/1903593002/diff/60001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client.cc:180: LOG(ERROR) << "Could not get variations service."; On 2016/04/20 20:30:23, Alexei Svitkine wrote: > Nit: Make this a DLOG() so we don't bloat the production binary with these > strings. Removed changes from chrome_translate_client. https://codereview.chromium.org/1903593002/diff/60001/chrome/browser/translat... File chrome/browser/translate/chrome_translate_client.h (right): https://codereview.chromium.org/1903593002/diff/60001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client.h:60: // kVariationsPermanentConsistencyCountry. Returns true if the variable has On 2016/04/20 20:30:23, Alexei Svitkine wrote: > Nit: No need to say "Helper method". Also, the pref name is an implementation > detail. > > Maybe describe the purpose instead: > > "Overrides the stored permanent consistency country in prefs. This is intended > to be invoked from a debug UI to allow easy testing of different-country > configurations." Done. https://codereview.chromium.org/1903593002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc (right): https://codereview.chromium.org/1903593002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:255: std::string country = translate_prefs->GetCountry(); On 2016/04/20 20:43:54, Alexei Svitkine wrote: > I actually don't quite follow this logic. What does the translate prefs > GetCountry() have to do with the variations country? I think the two are > separate, so if you want to fill in the existing variations country, I think you > need to get it from variations service. Or am I missing something here? You are right. I don't need to go through ChromeTranslateClient at all. I can access variations directly. Fixed. https://codereview.chromium.org/1903593002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:259: dict.Set("country", new base::StringValue(translate_prefs->GetCountry())); On 2016/04/20 20:43:54, Alexei Svitkine wrote: > Nit: You have |country| in a local var already. Done. https://codereview.chromium.org/1903593002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/translate_internals/translate_internals_handler.h (right): https://codereview.chromium.org/1903593002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/translate_internals/translate_internals_handler.h:83: // Sends the stored permanent country to JavaScript. On 2016/04/20 20:43:54, Alexei Svitkine wrote: > Nit: Document |is_update| param. Also, JavaScript -> Javascript Done. https://codereview.chromium.org/1903593002/diff/60001/components/variations/s... File components/variations/service/variations_service.cc (right): https://codereview.chromium.org/1903593002/diff/60001/components/variations/s... components/variations/service/variations_service.cc:869: // FIXME: Can we check for valid country? On 2016/04/20 20:30:24, Alexei Svitkine wrote: > Probably not really feasible. Acknowledged. https://codereview.chromium.org/1903593002/diff/60001/components/variations/s... components/variations/service/variations_service.cc:870: if(country_override.empty()) { On 2016/04/20 20:30:24, Alexei Svitkine wrote: > Nit: space after if. Also, no {}'s on a one-line if - same on line 884. > > You can run "git cl format" Done. https://codereview.chromium.org/1903593002/diff/60001/components/variations/s... components/variations/service/variations_service.cc:885: return false; On 2016/04/20 20:30:24, Alexei Svitkine wrote: > Not sure we actually need this logic. What's the danger of just doing it > unconditionally? There is no danger. However, this allows us to tell Js if the pref has been updated, so that we can tell the user he needs to restart his browser. https://codereview.chromium.org/1903593002/diff/60001/components/variations/s... components/variations/service/variations_service.cc:893: new_list_value); On 2016/04/20 20:30:24, Alexei Svitkine wrote: > Can you make a helper function for this logic since it seems to be used in the > other function that changes this? Done. https://codereview.chromium.org/1903593002/diff/60001/components/variations/s... File components/variations/service/variations_service_unittest.cc (right): https://codereview.chromium.org/1903593002/diff/60001/components/variations/s... components/variations/service/variations_service_unittest.cc:762: if (test.pref_value_before != "") { On 2016/04/20 20:30:24, Alexei Svitkine wrote: > Nit: !.empty() Done.
https://codereview.chromium.org/1903593002/diff/60001/components/variations/s... File components/variations/service/variations_service.cc (right): https://codereview.chromium.org/1903593002/diff/60001/components/variations/s... components/variations/service/variations_service.cc:885: return false; On 2016/04/20 21:39:47, hamelphi wrote: > On 2016/04/20 20:30:24, Alexei Svitkine wrote: > > Not sure we actually need this logic. What's the danger of just doing it > > unconditionally? > > There is no danger. However, this allows us to tell Js if the pref has been > updated, so that we can tell the user he needs to restart his browser. That makes sense. However, I feel it's strange to handle "failed to set it" and "it doesn't need to be changed" in the same way. Can these two cases be differentiated? On the other hand, maybe we can simply not treat the "country empty" case as a failure and instead completely clear the pref? This would simulate the experience a user would have if there was no country pref (e.g. first run). https://codereview.chromium.org/1903593002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc (right): https://codereview.chromium.org/1903593002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:184: if (variations_service) Nit: {} since body is multi-line. Alternatively, maybe you can name it |service| and avoid needing to wrap? https://codereview.chromium.org/1903593002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:256: std::string country = ""; Nit: std::string default ctor is the empty string, so need for "= " https://codereview.chromium.org/1903593002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:259: if (variations_service) { Nit: No {}'s if the body is multi-line. https://codereview.chromium.org/1903593002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/translate_internals/translate_internals_handler.h (right): https://codereview.chromium.org/1903593002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/translate_internals/translate_internals_handler.h:84: // |is_update| tells Javascript if the country has been updated or not. Nit: Maybe a better name is |was_updated|? https://codereview.chromium.org/1903593002/diff/80001/components/variations/s... File components/variations/service/variations_service.cc (right): https://codereview.chromium.org/1903593002/diff/80001/components/variations/s... components/variations/service/variations_service.cc:850: const std::string& country) { Nit: Make this a free-standing function in an anon namespace at the top of the file.
https://codereview.chromium.org/1903593002/diff/80001/chrome/browser/resource... File chrome/browser/resources/translate_internals/translate_internals.js (right): https://codereview.chromium.org/1903593002/diff/80001/chrome/browser/resource... chrome/browser/resources/translate_internals/translate_internals.js:270: var input = document.createElement('textarea'); Instead of a textarea, use an input control. This way, the UI won't show a resize box and I think you don't need to set cols/rows. https://codereview.chromium.org/1903593002/diff/80001/chrome/browser/resource... chrome/browser/resources/translate_internals/translate_internals.js:275: var button = document.createElement('button'); What's the reason for creating these programmatically vs. just having them in the HTML?
https://codereview.chromium.org/1903593002/diff/80001/chrome/browser/resource... File chrome/browser/resources/translate_internals/translate_internals.js (right): https://codereview.chromium.org/1903593002/diff/80001/chrome/browser/resource... chrome/browser/resources/translate_internals/translate_internals.js:270: var input = document.createElement('textarea'); On 2016/04/20 21:48:41, Alexei Svitkine wrote: > Instead of a textarea, use an input control. This way, the UI won't show a > resize box and I think you don't need to set cols/rows. Done. https://codereview.chromium.org/1903593002/diff/80001/chrome/browser/resource... chrome/browser/resources/translate_internals/translate_internals.js:275: var button = document.createElement('button'); On 2016/04/20 21:48:41, Alexei Svitkine wrote: > What's the reason for creating these programmatically vs. just having them in > the HTML? Then we can display something else if we can't get/set the country from variations service. https://codereview.chromium.org/1903593002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc (right): https://codereview.chromium.org/1903593002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:184: if (variations_service) On 2016/04/20 21:46:44, Alexei Svitkine wrote: > Nit: {} since body is multi-line. > > Alternatively, maybe you can name it |service| and avoid needing to wrap? Done. https://codereview.chromium.org/1903593002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:256: std::string country = ""; On 2016/04/20 21:46:44, Alexei Svitkine wrote: > Nit: std::string default ctor is the empty string, so need for "= " Done. https://codereview.chromium.org/1903593002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:259: if (variations_service) { On 2016/04/20 21:46:44, Alexei Svitkine wrote: > Nit: No {}'s if the body is multi-line. Done. https://codereview.chromium.org/1903593002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/translate_internals/translate_internals_handler.h (right): https://codereview.chromium.org/1903593002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/translate_internals/translate_internals_handler.h:84: // |is_update| tells Javascript if the country has been updated or not. On 2016/04/20 21:46:44, Alexei Svitkine wrote: > Nit: Maybe a better name is |was_updated|? Done. https://codereview.chromium.org/1903593002/diff/80001/components/variations/s... File components/variations/service/variations_service.cc (right): https://codereview.chromium.org/1903593002/diff/80001/components/variations/s... components/variations/service/variations_service.cc:850: const std::string& country) { On 2016/04/20 21:46:44, Alexei Svitkine wrote: > Nit: Make this a free-standing function in an anon namespace at the top of the > file. It needs to be a class method since we need access to local_state_.
Description was changed from ========== Allows to manually change the stored permanent country in Variations from the translate-internals dashboard. I am still working on the unittest, and the html/js UI can use some polishing (e.g. add a warning to restart browser when updating the country), but please review the backend code to see if it makes sense. R=asvitkine@chromium.org BUG=599325 ========== to ========== Allows to manually change the stored permanent country in Variations from the translate-internals dashboard. R=asvitkine@chromium.org BUG=599325 ==========
code lgtm % remaining comments You will need someone from chrome/browser/ui/webui/translate_internals/OWNERS to also approve the change. https://codereview.chromium.org/1903593002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/translate_internals/translate_internals.js (right): https://codereview.chromium.org/1903593002/diff/120001/chrome/browser/resourc... chrome/browser/resources/translate_internals/translate_internals.js:276: button.classList.add('override_country'); Are you defining the css for this somewhere? https://codereview.chromium.org/1903593002/diff/120001/components/variations/... File components/variations/service/variations_service.cc (right): https://codereview.chromium.org/1903593002/diff/120001/components/variations/... components/variations/service/variations_service.cc:848: void VariationsService::SetStoredPermanentCountry(const base::Version& version, Nit: SetStored -> Store https://codereview.chromium.org/1903593002/diff/120001/components/variations/... components/variations/service/variations_service.cc:869: bool VariationsService::ForceSetStoredPermanentCountry( Nit: How about OverrideStoredPermanentCountry
hamelphi@chromium.org changed reviewers: + toyoshim@chromium.org
https://codereview.chromium.org/1903593002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/translate_internals/translate_internals.js (right): https://codereview.chromium.org/1903593002/diff/120001/chrome/browser/resourc... chrome/browser/resources/translate_internals/translate_internals.js:276: button.classList.add('override_country'); On 2016/04/21 15:21:21, Alexei Svitkine wrote: > Are you defining the css for this somewhere? Ah, no. But I guess it is not needed. Removed the class. https://codereview.chromium.org/1903593002/diff/120001/components/variations/... File components/variations/service/variations_service.cc (right): https://codereview.chromium.org/1903593002/diff/120001/components/variations/... components/variations/service/variations_service.cc:848: void VariationsService::SetStoredPermanentCountry(const base::Version& version, On 2016/04/21 15:21:21, Alexei Svitkine wrote: > Nit: SetStored -> Store Done. https://codereview.chromium.org/1903593002/diff/120001/components/variations/... components/variations/service/variations_service.cc:869: bool VariationsService::ForceSetStoredPermanentCountry( On 2016/04/21 15:21:21, Alexei Svitkine wrote: > Nit: How about OverrideStoredPermanentCountry Done.
Description was changed from ========== Allows to manually change the stored permanent country in Variations from the translate-internals dashboard. R=asvitkine@chromium.org BUG=599325 ========== to ========== Allows to manually change the stored permanent country in Variations from the translate-internals dashboard. R=asvitkine@chromium.org,toyoshim@chromium.org, BUG=599325 ==========
Description was changed from ========== Allows to manually change the stored permanent country in Variations from the translate-internals dashboard. R=asvitkine@chromium.org,toyoshim@chromium.org, BUG=599325 ========== to ========== Allows to manually change the stored permanent country in Variations from the translate-internals dashboard. R=asvitkine@chromium.org,toyoshim@chromium.org BUG=599325 ==========
Code looks good, but there are minor question and suggestion. https://codereview.chromium.org/1903593002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/translate_internals/translate_internals.js (right): https://codereview.chromium.org/1903593002/diff/160001/chrome/browser/resourc... chrome/browser/resources/translate_internals/translate_internals.js:271: input.type = 'text'; Question: Is it safe to allow arbitrary string for this country code replacing? https://codereview.chromium.org/1903593002/diff/160001/chrome/browser/resourc... chrome/browser/resources/translate_internals/translate_internals.js:283: if ('update' in details && details['update']) { IIUC, this configuration isn't directly used by Translate, but used by Finch, right? Even so, I'm fine to place this UI here, but probably we need clearer comment here to describe what this configuration affect. I'm just afraid users and developers are confused and think this should change Chrome recognizing native language and foreign language detection.
hajimehoshi@chromium.org changed reviewers: + hajimehoshi@chromium.org
https://codereview.chromium.org/1903593002/diff/160001/components/variations/... File components/variations/service/variations_service.cc (right): https://codereview.chromium.org/1903593002/diff/160001/components/variations/... components/variations/service/variations_service.cc:886: if (!needs_update) I'd write it without a variable like: if (!got_stored_country || stored_country != country_override) https://codereview.chromium.org/1903593002/diff/160001/components/variations/... File components/variations/service/variations_service.h (right): https://codereview.chromium.org/1903593002/diff/160001/components/variations/... components/variations/service/variations_service.h:125: // Forces an override of kVariationsPermanentConsistencyCountry. Returns true nit: Remove a space char after 'of' https://codereview.chromium.org/1903593002/diff/160001/components/variations/... File components/variations/service/variations_service_unittest.cc (right): https://codereview.chromium.org/1903593002/diff/160001/components/variations/... components/variations/service/variations_service_unittest.cc:734: std::string kPrefUs = version_info::GetVersionNumber() + ",us"; const
https://codereview.chromium.org/1903593002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/translate_internals/translate_internals.js (right): https://codereview.chromium.org/1903593002/diff/160001/chrome/browser/resourc... chrome/browser/resources/translate_internals/translate_internals.js:271: input.type = 'text'; On 2016/04/22 07:10:08, toyoshim wrote: > Question: Is it safe to allow arbitrary string for this country code replacing? Yep, it's safe. It's just stored in prefs and used in == comparisons to what is specified in experiment configs. So if you put something strange, it will just never match. https://codereview.chromium.org/1903593002/diff/160001/chrome/browser/resourc... chrome/browser/resources/translate_internals/translate_internals.js:283: if ('update' in details && details['update']) { On 2016/04/22 07:10:08, toyoshim wrote: > IIUC, this configuration isn't directly used by Translate, but used by Finch, > right? > > Even so, I'm fine to place this UI here, but probably we need clearer comment > here to describe what this configuration affect. I'm just afraid users and > developers are confused and think this should change Chrome recognizing native > language and foreign language detection. Maybe we can title it something like "Override Variations Country"? With maybe a tooltip explaining how it's used.
https://codereview.chromium.org/1903593002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/translate_internals/translate_internals.js (right): https://codereview.chromium.org/1903593002/diff/160001/chrome/browser/resourc... chrome/browser/resources/translate_internals/translate_internals.js:283: if ('update' in details && details['update']) { On 2016/04/22 14:50:13, Alexei Svitkine wrote: > On 2016/04/22 07:10:08, toyoshim wrote: > > IIUC, this configuration isn't directly used by Translate, but used by Finch, > > right? > > > > Even so, I'm fine to place this UI here, but probably we need clearer comment > > here to describe what this configuration affect. I'm just afraid users and > > developers are confused and think this should change Chrome recognizing native > > language and foreign language detection. > > Maybe we can title it something like "Override Variations Country"? With maybe a > tooltip explaining how it's used. Added a text explanation about the variable. Alexei, you know more than I do about the use of this variable. Let me know if you think about more relevant info to add. https://codereview.chromium.org/1903593002/diff/160001/components/variations/... File components/variations/service/variations_service.cc (right): https://codereview.chromium.org/1903593002/diff/160001/components/variations/... components/variations/service/variations_service.cc:886: if (!needs_update) On 2016/04/22 07:22:57, hajimehoshi wrote: > I'd write it without a variable like: if (!got_stored_country || stored_country > != country_override) Done. https://codereview.chromium.org/1903593002/diff/160001/components/variations/... File components/variations/service/variations_service.h (right): https://codereview.chromium.org/1903593002/diff/160001/components/variations/... components/variations/service/variations_service.h:125: // Forces an override of kVariationsPermanentConsistencyCountry. Returns true On 2016/04/22 07:22:57, hajimehoshi wrote: > nit: Remove a space char after 'of' Done. https://codereview.chromium.org/1903593002/diff/160001/components/variations/... File components/variations/service/variations_service_unittest.cc (right): https://codereview.chromium.org/1903593002/diff/160001/components/variations/... components/variations/service/variations_service_unittest.cc:734: std::string kPrefUs = version_info::GetVersionNumber() + ",us"; On 2016/04/22 07:22:57, hajimehoshi wrote: > const Done.
https://codereview.chromium.org/1903593002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/translate_internals/translate_internals.js (right): https://codereview.chromium.org/1903593002/diff/180001/chrome/browser/resourc... chrome/browser/resources/translate_internals/translate_internals.js:270: var tooltip = document.createElement('div'); Nit: a tooltip is usually what appears on mouseover. So, I was suggesting just setting it via title="" on the h2 you have in the HTML. See example here: http://www.w3schools.com/tags/tryit.asp?filename=tryhtml5_global_title https://codereview.chromium.org/1903593002/diff/180001/chrome/browser/resourc... chrome/browser/resources/translate_internals/translate_internals.js:274: 'current country when Chrome is updated.'); Nit: I'd change the second sentence to: Normally, this value gets automatically updated with the a new value received from the variations server when Chrome is updated.
https://codereview.chromium.org/1903593002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/translate_internals/translate_internals.js (right): https://codereview.chromium.org/1903593002/diff/180001/chrome/browser/resourc... chrome/browser/resources/translate_internals/translate_internals.js:270: var tooltip = document.createElement('div'); On 2016/04/22 17:24:59, Alexei Svitkine wrote: > Nit: a tooltip is usually what appears on mouseover. > > So, I was suggesting just setting it via title="" on the h2 you have in the > HTML. > > See example here: > > http://www.w3schools.com/tags/tryit.asp?filename=tryhtml5_global_title Done. https://codereview.chromium.org/1903593002/diff/180001/chrome/browser/resourc... chrome/browser/resources/translate_internals/translate_internals.js:274: 'current country when Chrome is updated.'); On 2016/04/22 17:24:59, Alexei Svitkine wrote: > Nit: I'd change the second sentence to: > > Normally, this value gets automatically updated with the a new value received > from the variations server when Chrome is updated. Done.
lgtm
LGTM. By the way, may I ask a question about variation configurations, if you know? Even on Chromium, I notice that variations value shown in chrome://version/ is changed after some chromium restarts. But, with this CL, chrome://translate-internals/#prefs says "Could not load country info from Variations." that seems to mean Chromium does not take a configuration from a server. I thought Chromium also connects to a server to take configurations because the value in chrome://version is changed. But is this my understanding wrong?
On 2016/04/25 04:49:39, toyoshim wrote: > LGTM. > > By the way, may I ask a question about variation configurations, if you know? > Even on Chromium, I notice that variations value shown in chrome://version/ is > changed after some chromium restarts. > But, with this CL, chrome://translate-internals/#prefs says "Could not load > country info from Variations." that seems to mean Chromium does not take a > configuration from a server. > > I thought Chromium also connects to a server to take configurations because the > value in chrome://version is changed. But is this my understanding wrong? The set of variation configs are stored in local state from the last time you ran with a variations server with that same user data dir. Subsequently, they get evaluated on start up - which is when the client decides which study its in. This can change because some studies can be marked "session randomized", which means a different group is dice rolled on every start up. (Most studies, however, are permanent consistency so they keep their dice-rolled group even between start ups.)
The CQ bit was checked by hamelphi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1903593002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1903593002/200001
Message was sent while issue was closed.
Description was changed from ========== Allows to manually change the stored permanent country in Variations from the translate-internals dashboard. R=asvitkine@chromium.org,toyoshim@chromium.org BUG=599325 ========== to ========== Allows to manually change the stored permanent country in Variations from the translate-internals dashboard. R=asvitkine@chromium.org,toyoshim@chromium.org BUG=599325 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Allows to manually change the stored permanent country in Variations from the translate-internals dashboard. R=asvitkine@chromium.org,toyoshim@chromium.org BUG=599325 ========== to ========== Allows to manually change the stored permanent country in Variations from the translate-internals dashboard. R=asvitkine@chromium.org,toyoshim@chromium.org BUG=599325 Committed: https://crrev.com/2d297251935984323c30d2faf501aa13fd5d479a Cr-Commit-Position: refs/heads/master@{#389501} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/2d297251935984323c30d2faf501aa13fd5d479a Cr-Commit-Position: refs/heads/master@{#389501}
Message was sent while issue was closed.
Alexel, thank you for explanation. |
