Add support for variations controlled overriding UI strings to the variations service.

Add support for variations controlled overriding UI strings to the variations service.

This is based off of http://crrev.com/322523002

BUG=370033
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284578

[Alexei Svitkine (slow), 2014-07-03 17:54:41]

Some initial comments, will look more later.

https://codereview.chromium.org/370463003/diff/20001/chrome/browser/metrics/v...
File chrome/browser/metrics/variations/generated_resources_map.h (right):

https://codereview.chromium.org/370463003/diff/20001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/generated_resources_map.h:19: extern const
size_t kNumResources;
Add a comment. Also, probably move below the comment that follows it.

https://codereview.chromium.org/370463003/diff/20001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/generated_resources_map.h:31: int
GetResourceIndex(uint32_t hash);
Add a comment.

https://codereview.chromium.org/370463003/diff/20001/chrome/browser/metrics/v...
File chrome/browser/metrics/variations/generated_resources_map_lookup.cc
(right):

https://codereview.chromium.org/370463003/diff/20001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/generated_resources_map_lookup.cc:13: const
uint32_t* kElement = std::lower_bound(kResourceHashes, kEnd, hash);
Nit: kNaming convention is only used for compile-time constants. Since this is a
run-time constant, don't use it here.

https://codereview.chromium.org/370463003/diff/20001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/generated_resources_map_lookup.cc:15: if
(kElement == kEnd || *kElement != hash)
Is the first cond needed or will the second catch both of them? If so, just keep
the second cond but add a comment explaining it.

https://codereview.chromium.org/370463003/diff/20001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/generated_resources_map_lookup.cc:17: return 
kResourceIndices[kElement - kResourceHashes];
Nit: Remove extra space after return.

https://codereview.chromium.org/370463003/diff/20001/chrome/browser/metrics/v...
File chrome/browser/metrics/variations/variations_service.cc (right):

https://codereview.chromium.org/370463003/diff/20001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_service.cc:203: void
OverrideUIString(uint32_t hash, const base::string16& string) {
Add a comment.

https://codereview.chromium.org/370463003/diff/20001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_service.cc:204: int index =
GetResourceIndex(hash);
Check for -1.

https://codereview.chromium.org/370463003/diff/20001/components/variations/pr...
File components/variations/processed_study.cc (right):

https://codereview.chromium.org/370463003/diff/20001/components/variations/pr...
components/variations/processed_study.cc:97: if (study_->experiment(i).name() ==
name) {
Nit: No need for {}'s.

https://codereview.chromium.org/370463003/diff/20001/components/variations/pr...
File components/variations/proto/study.proto (right):

https://codereview.chromium.org/370463003/diff/20001/components/variations/pr...
components/variations/proto/study.proto:112: optional fixed32 name_hash = 1;  //
e.g. Hash("IDS_BOOKMARK_BAR_FOLDER_NAME")
Nit: Wrapping. But also, add some comments about this.

https://codereview.chromium.org/370463003/diff/20001/components/variations/va...
File components/variations/variations_seed_processor.cc (right):

https://codereview.chromium.org/370463003/diff/20001/components/variations/va...
components/variations/variations_seed_processor.cc:66:
Study_Experiment_OverrideUIString  experiment_overrides =
Nit: Remove extra space.

https://codereview.chromium.org/370463003/diff/20001/components/variations/va...
components/variations/variations_seed_processor.cc:69:
base::ASCIIToUTF16(experiment_overrides.value()));
The value may not be ascii. Use UTF8ToUTF16.

https://codereview.chromium.org/370463003/diff/20001/components/variations/va...
File components/variations/variations_seed_processor_unittest.cc (right):

https://codereview.chromium.org/370463003/diff/20001/components/variations/va...
components/variations/variations_seed_processor_unittest.cc:95: void
override(uint32_t hash, const base::string16& string) {
Nit: I don't think this should use hacker_style. Instead, make it a proper
method.

[Alexei Svitkine (slow), 2014-07-04 15:48:52]

A few more comments your way.

https://codereview.chromium.org/370463003/diff/20001/components/variations/va...
File components/variations/variations_seed_processor.cc (right):

https://codereview.chromium.org/370463003/diff/20001/components/variations/va...
components/variations/variations_seed_processor.cc:62: void
ApplyUIStringOverrides(
Add a comment please.

https://codereview.chromium.org/370463003/diff/20001/components/variations/va...
components/variations/variations_seed_processor.cc:163: if
(processed_study.is_expired())
Nit: Add {}'s here.

https://codereview.chromium.org/370463003/diff/20001/components/variations/va...
components/variations/variations_seed_processor.cc:169: int experiment_index =
processed_study.GetExperimentIndexByName(group_name);
Maybe add a bool to keep track if any experiment had UI string overrides (via
the loop above) and do an early return if they haven't - to avoid this extra
work unless necessary.

https://codereview.chromium.org/370463003/diff/20001/components/variations/va...
components/variations/variations_seed_processor.cc:173:
DCHECK_GE(experiment_index, 0);
Change to NE(-1,

[jwd, 2014-07-07 15:52:21]

https://codereview.chromium.org/370463003/diff/20001/chrome/browser/metrics/v...
File chrome/browser/metrics/variations/generated_resources_map.h (right):

https://codereview.chromium.org/370463003/diff/20001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/generated_resources_map.h:19: extern const
size_t kNumResources;
On 2014/07/03 17:54:40, Alexei Svitkine wrote:
> Add a comment. Also, probably move below the comment that follows it.

Done.

https://codereview.chromium.org/370463003/diff/20001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/generated_resources_map.h:31: int
GetResourceIndex(uint32_t hash);
On 2014/07/03 17:54:40, Alexei Svitkine wrote:
> Add a comment.

Done.

https://codereview.chromium.org/370463003/diff/20001/chrome/browser/metrics/v...
File chrome/browser/metrics/variations/generated_resources_map_lookup.cc
(right):

https://codereview.chromium.org/370463003/diff/20001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/generated_resources_map_lookup.cc:13: const
uint32_t* kElement = std::lower_bound(kResourceHashes, kEnd, hash);
On 2014/07/03 17:54:40, Alexei Svitkine wrote:
> Nit: kNaming convention is only used for compile-time constants. Since this is
a
> run-time constant, don't use it here.

Done.

https://codereview.chromium.org/370463003/diff/20001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/generated_resources_map_lookup.cc:15: if
(kElement == kEnd || *kElement != hash)
On 2014/07/03 17:54:40, Alexei Svitkine wrote:
> Is the first cond needed or will the second catch both of them? If so, just
keep
> the second cond but add a comment explaining it.

If element == kEnd, then it would be dereferencing memory beyond the bound of
the array.

https://codereview.chromium.org/370463003/diff/20001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/generated_resources_map_lookup.cc:17: return 
kResourceIndices[kElement - kResourceHashes];
On 2014/07/03 17:54:40, Alexei Svitkine wrote:
> Nit: Remove extra space after return.

Done.

https://codereview.chromium.org/370463003/diff/20001/chrome/browser/metrics/v...
File chrome/browser/metrics/variations/variations_service.cc (right):

https://codereview.chromium.org/370463003/diff/20001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_service.cc:203: void
OverrideUIString(uint32_t hash, const base::string16& string) {
On 2014/07/03 17:54:40, asvitkine (OOO back July 14th) wrote:
> Add a comment.

Done.

https://codereview.chromium.org/370463003/diff/20001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_service.cc:204: int index =
GetResourceIndex(hash);
On 2014/07/03 17:54:40, asvitkine (OOO back July 14th) wrote:
> Check for -1.

Done.

https://codereview.chromium.org/370463003/diff/20001/components/variations/pr...
File components/variations/processed_study.cc (right):

https://codereview.chromium.org/370463003/diff/20001/components/variations/pr...
components/variations/processed_study.cc:97: if (study_->experiment(i).name() ==
name) {
On 2014/07/03 17:54:40, asvitkine (OOO back July 14th) wrote:
> Nit: No need for {}'s.

Done.

https://codereview.chromium.org/370463003/diff/20001/components/variations/pr...
File components/variations/proto/study.proto (right):

https://codereview.chromium.org/370463003/diff/20001/components/variations/pr...
components/variations/proto/study.proto:112: optional fixed32 name_hash = 1;  //
e.g. Hash("IDS_BOOKMARK_BAR_FOLDER_NAME")
On 2014/07/03 17:54:40, asvitkine (OOO back July 14th) wrote:
> Nit: Wrapping. But also, add some comments about this.

Done.

https://codereview.chromium.org/370463003/diff/20001/components/variations/va...
File components/variations/variations_seed_processor.cc (right):

https://codereview.chromium.org/370463003/diff/20001/components/variations/va...
components/variations/variations_seed_processor.cc:62: void
ApplyUIStringOverrides(
On 2014/07/04 15:48:51, asvitkine (OOO back July 14th) wrote:
> Add a comment please.

Done.

https://codereview.chromium.org/370463003/diff/20001/components/variations/va...
components/variations/variations_seed_processor.cc:66:
Study_Experiment_OverrideUIString  experiment_overrides =
On 2014/07/03 17:54:40, asvitkine (OOO back July 14th) wrote:
> Nit: Remove extra space.

Done.

https://codereview.chromium.org/370463003/diff/20001/components/variations/va...
components/variations/variations_seed_processor.cc:69:
base::ASCIIToUTF16(experiment_overrides.value()));
On 2014/07/03 17:54:41, asvitkine (OOO back July 14th) wrote:
> The value may not be ascii. Use UTF8ToUTF16.

Done.

https://codereview.chromium.org/370463003/diff/20001/components/variations/va...
components/variations/variations_seed_processor.cc:163: if
(processed_study.is_expired())
On 2014/07/04 15:48:51, asvitkine (OOO back July 14th) wrote:
> Nit: Add {}'s here.

Done.

https://codereview.chromium.org/370463003/diff/20001/components/variations/va...
components/variations/variations_seed_processor.cc:169: int experiment_index =
processed_study.GetExperimentIndexByName(group_name);
On 2014/07/04 15:48:51, asvitkine (OOO back July 14th) wrote:
> Maybe add a bool to keep track if any experiment had UI string overrides (via
> the loop above) and do an early return if they haven't - to avoid this extra
> work unless necessary.

Done.

https://codereview.chromium.org/370463003/diff/20001/components/variations/va...
components/variations/variations_seed_processor.cc:173:
DCHECK_GE(experiment_index, 0);
On 2014/07/04 15:48:52, asvitkine (OOO back July 14th) wrote:
> Change to NE(-1, 

Done.

https://codereview.chromium.org/370463003/diff/20001/components/variations/va...
File components/variations/variations_seed_processor_unittest.cc (right):

https://codereview.chromium.org/370463003/diff/20001/components/variations/va...
components/variations/variations_seed_processor_unittest.cc:95: void
override(uint32_t hash, const base::string16& string) {
On 2014/07/03 17:54:41, asvitkine (OOO back July 14th) wrote:
> Nit: I don't think this should use hacker_style. Instead, make it a proper
> method.

Done.

[Alexei Svitkine (slow), 2014-07-15 16:44:52]

https://codereview.chromium.org/370463003/diff/40001/chrome/browser/metrics/v...
File chrome/browser/metrics/variations/variations_service.cc (right):

https://codereview.chromium.org/370463003/diff/40001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_service.cc:206: int index =
GetResourceIndex(hash);
Nit: It might be clearer to use |resource_id| terminology here rather than
|index|.

https://codereview.chromium.org/370463003/diff/40001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_service.cc:210:
ui::ResourceBundle::GetSharedInstance().OverrideStringResource(index, string);
Has the method been renamed to OverrideLocaleStringResource?

https://codereview.chromium.org/370463003/diff/40001/components/variations/pr...
File components/variations/proto/study.proto (right):

https://codereview.chromium.org/370463003/diff/40001/components/variations/pr...
components/variations/proto/study.proto:116: optional fixed32 name_hash = 1;  //
e.g. Hash("IDS_BOOKMARK_BAR_UNDO")
Nit: Move this line comment to be at the end of the comment above this member.

https://codereview.chromium.org/370463003/diff/40001/components/variations/pr...
components/variations/proto/study.proto:117: optional string value = 2;  // e.g.
"Undo"
Nit: Put comment above the field instead with a short description sentence
first, e.g.

"The override text, e.g. "Undo."

https://codereview.chromium.org/370463003/diff/40001/components/variations/va...
File components/variations/variations_seed_processor.cc (right):

https://codereview.chromium.org/370463003/diff/40001/components/variations/va...
components/variations/variations_seed_processor.cc:66: const
VariationsSeedProcessor::UIStringOverrideCallback& override_string) {
Nit: Maybe clearer to rename this |override_callback|, since _string suffix
sounds like it may be an actual string. Also change it in the functions below.

https://codereview.chromium.org/370463003/diff/40001/components/variations/va...
components/variations/variations_seed_processor.cc:68:
Study_Experiment_OverrideUIString experiment_overrides =
Nit: const ref, to avoid copying - also this is a single override so rename to
|experiment_override| or just |override|.

https://codereview.chromium.org/370463003/diff/40001/components/variations/va...
components/variations/variations_seed_processor.cc:149: bool hasOverrides =
false;
Nit: has_overrides

https://codereview.chromium.org/370463003/diff/40001/components/variations/va...
components/variations/variations_seed_processor.cc:183:
DCHECK_NE(experiment_index, -1);
Nit: Swap params.

[jwd, 2014-07-16 21:33:05]

https://codereview.chromium.org/370463003/diff/40001/chrome/browser/metrics/v...
File chrome/browser/metrics/variations/variations_service.cc (right):

https://codereview.chromium.org/370463003/diff/40001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_service.cc:206: int index =
GetResourceIndex(hash);
On 2014/07/15 16:44:52, Alexei Svitkine wrote:
> Nit: It might be clearer to use |resource_id| terminology here rather than
> |index|.

Done.

https://codereview.chromium.org/370463003/diff/40001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_service.cc:210:
ui::ResourceBundle::GetSharedInstance().OverrideStringResource(index, string);
On 2014/07/15 16:44:52, Alexei Svitkine wrote:
> Has the method been renamed to OverrideLocaleStringResource?

Yup, rebased now.

https://codereview.chromium.org/370463003/diff/40001/components/variations/pr...
File components/variations/proto/study.proto (right):

https://codereview.chromium.org/370463003/diff/40001/components/variations/pr...
components/variations/proto/study.proto:116: optional fixed32 name_hash = 1;  //
e.g. Hash("IDS_BOOKMARK_BAR_UNDO")
On 2014/07/15 16:44:52, Alexei Svitkine wrote:
> Nit: Move this line comment to be at the end of the comment above this member.

Done.

https://codereview.chromium.org/370463003/diff/40001/components/variations/pr...
components/variations/proto/study.proto:117: optional string value = 2;  // e.g.
"Undo"
On 2014/07/15 16:44:52, Alexei Svitkine wrote:
> Nit: Put comment above the field instead with a short description sentence
> first, e.g.
> 
> "The override text, e.g. "Undo."

Done.

https://codereview.chromium.org/370463003/diff/40001/components/variations/va...
File components/variations/variations_seed_processor.cc (right):

https://codereview.chromium.org/370463003/diff/40001/components/variations/va...
components/variations/variations_seed_processor.cc:66: const
VariationsSeedProcessor::UIStringOverrideCallback& override_string) {
On 2014/07/15 16:44:52, Alexei Svitkine wrote:
> Nit: Maybe clearer to rename this |override_callback|, since _string suffix
> sounds like it may be an actual string. Also change it in the functions below.

Done.

https://codereview.chromium.org/370463003/diff/40001/components/variations/va...
components/variations/variations_seed_processor.cc:68:
Study_Experiment_OverrideUIString experiment_overrides =
On 2014/07/15 16:44:52, Alexei Svitkine wrote:
> Nit: const ref, to avoid copying - also this is a single override so rename to
> |experiment_override| or just |override|.

Done.

https://codereview.chromium.org/370463003/diff/40001/components/variations/va...
components/variations/variations_seed_processor.cc:149: bool hasOverrides =
false;
On 2014/07/15 16:44:52, Alexei Svitkine wrote:
> Nit: has_overrides

Done.

https://codereview.chromium.org/370463003/diff/40001/components/variations/va...
components/variations/variations_seed_processor.cc:183:
DCHECK_NE(experiment_index, -1);
On 2014/07/15 16:44:52, Alexei Svitkine wrote:
> Nit: Swap params.

Done.

[Alexei Svitkine (slow), 2014-07-17 13:43:31]

LGTM

https://codereview.chromium.org/370463003/diff/80001/chrome/chrome_tests_unit...
File chrome/chrome_tests_unit.gypi (left):

https://codereview.chromium.org/370463003/diff/80001/chrome/chrome_tests_unit...
chrome/chrome_tests_unit.gypi:1088:
'browser/metrics/variations/variations_http_header_provider_unittest.cc',
This diff looks weird. Do you need to rebase?

https://codereview.chromium.org/370463003/diff/80001/components/variations/pr...
File components/variations/proto/study.proto (right):

https://codereview.chromium.org/370463003/diff/80001/components/variations/pr...
components/variations/proto/study.proto:112: // hash of the resource name to be
overridden.
Nit: The second sentence is redundant given there's a comment below about
|name_hash| - either incorporate it int the comment below or just remove
altogether.

https://codereview.chromium.org/370463003/diff/80001/components/variations/pr...
components/variations/proto/study.proto:118: // The new value of the string
being overridden.
Add an empty line above this.

https://codereview.chromium.org/370463003/diff/80001/components/variations/va...
File components/variations/variations_seed_processor.cc (right):

https://codereview.chromium.org/370463003/diff/80001/components/variations/va...
components/variations/variations_seed_processor.cc:63: // Executes
|override_callback| on every override defined by |experiment|.
Nit: override_callback -> callback

https://codereview.chromium.org/370463003/diff/80001/components/variations/va...
components/variations/variations_seed_processor.cc:71:
base::UTF8ToUTF16(override.value()));
Nit: Either align or check if it now fits on the line above.

[jwd, 2014-07-18 20:10:29]

https://codereview.chromium.org/370463003/diff/80001/chrome/chrome_tests_unit...
File chrome/chrome_tests_unit.gypi (left):

https://codereview.chromium.org/370463003/diff/80001/chrome/chrome_tests_unit...
chrome/chrome_tests_unit.gypi:1088:
'browser/metrics/variations/variations_http_header_provider_unittest.cc',
On 2014/07/17 13:43:31, Alexei Svitkine wrote:
> This diff looks weird. Do you need to rebase?

Done.

https://codereview.chromium.org/370463003/diff/80001/components/variations/pr...
File components/variations/proto/study.proto (right):

https://codereview.chromium.org/370463003/diff/80001/components/variations/pr...
components/variations/proto/study.proto:112: // hash of the resource name to be
overridden.
On 2014/07/17 13:43:31, Alexei Svitkine wrote:
> Nit: The second sentence is redundant given there's a comment below about
> |name_hash| - either incorporate it int the comment below or just remove
> altogether.

Done.

https://codereview.chromium.org/370463003/diff/80001/components/variations/pr...
components/variations/proto/study.proto:118: // The new value of the string
being overridden.
On 2014/07/17 13:43:31, Alexei Svitkine wrote:
> Add an empty line above this.

Done.

https://codereview.chromium.org/370463003/diff/80001/components/variations/va...
File components/variations/variations_seed_processor.cc (right):

https://codereview.chromium.org/370463003/diff/80001/components/variations/va...
components/variations/variations_seed_processor.cc:63: // Executes
|override_callback| on every override defined by |experiment|.
On 2014/07/17 13:43:31, Alexei Svitkine wrote:
> Nit: override_callback -> callback

Done.

https://codereview.chromium.org/370463003/diff/80001/components/variations/va...
components/variations/variations_seed_processor.cc:71:
base::UTF8ToUTF16(override.value()));
On 2014/07/17 13:43:31, Alexei Svitkine wrote:
> Nit: Either align or check if it now fits on the line above.

Done.

[jwd, 2014-07-21 13:17:55]

+sky@ for OWNERS on chrome_resources.gyp

[sky, 2014-07-21 16:32:32]

chrome_resources.gyp LGTM

[jwd, 2014-07-21 20:59:54]

The CQ bit was checked by jwd@chromium.org

[commit-bot: I haz the power, 2014-07-21 21:00:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jwd@chromium.org/370463003/120001

[jwd, 2014-07-21 21:32:49]

The CQ bit was checked by jwd@chromium.org

[commit-bot: I haz the power, 2014-07-21 21:33:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jwd@chromium.org/370463003/140001

[commit-bot: I haz the power, 2014-07-22 01:19:36]

Change committed as 284578