Allow embedders to share code to override UI strings.

Allow embedders to share code to override UI strings.

Create new class UIStringOverrider that implements the logic to use the
data generated by generate_resources_map.py to override UI strings.

BUG=534257
Committed: https://crrev.com/58b0a3f0ba85f77bedd15f357f6314298949b7c2
Cr-Commit-Position: refs/heads/master@{#351102}

[sdefresne, 2015-09-24 16:43:03]

sdefresne@chromium.org changed reviewers:
+ asvitkine@chromium.org

[sdefresne, 2015-09-24 16:43:03]

This is a first step to componentize this code.

Next steps will be (if you agree with the plan):
- move the generation script into the component
- write .gni/.gypi to facilitate usage of the generation script
- change the generation script to accept arguments to control
  - namespace of the generated code
  - support multiple input files
- update chrome compilation to pass all the overridable strings .h
 

WDYT?

[Alexei Svitkine (slow), 2015-09-24 21:51:23]

https://codereview.chromium.org/1363243004/diff/1/chrome/browser/metrics/vari...
File chrome/browser/metrics/variations/ui_string_overrider_unittest.cc (right):

https://codereview.chromium.org/1363243004/diff/1/chrome/browser/metrics/vari...
chrome/browser/metrics/variations/ui_string_overrider_unittest.cc:15: :
provider_(kResourceHashes, kResourceIndices, kNumResources) {}
Nit: This formatting doesn't seem right. git cl format?

https://codereview.chromium.org/1363243004/diff/1/components/variations/servi...
File components/variations/service/ui_string_overrider.h (right):

https://codereview.chromium.org/1363243004/diff/1/components/variations/servi...
components/variations/service/ui_string_overrider.h:46: size_t num_resources_;
Can you declare all of these as const? ie. so they're not changed after ctor
assignment.

const size_t
const uint32_t* const
etc.

https://codereview.chromium.org/1363243004/diff/1/components/variations/servi...
File components/variations/service/variations_service.cc (right):

https://codereview.chromium.org/1363243004/diff/1/components/variations/servi...
components/variations/service/variations_service.cc:253:
base::Unretained(&ui_string_overrider_)));
Can this pass a ref to the object instead of a callback?

[blundell, 2015-09-25 13:06:55]

blundell@chromium.org changed reviewers:
+ blundell@chromium.org

[blundell, 2015-09-25 13:06:56]

https://codereview.chromium.org/1363243004/diff/1/components/variations/servi...
File components/variations/service/variations_service.cc (right):

https://codereview.chromium.org/1363243004/diff/1/components/variations/servi...
components/variations/service/variations_service.cc:253:
base::Unretained(&ui_string_overrider_)));
On 2015/09/24 21:51:23, Alexei Svitkine (slow) wrote:
> Can this pass a ref to the object instead of a callback?

//components/variations can't depend on //ui/base, as the latter introduces a
dependency on //net, which causes Cronet to choke. :(

[Alexei Svitkine (slow), 2015-09-25 14:45:57]

https://codereview.chromium.org/1363243004/diff/1/chrome/chrome_tests_unit.gypi
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/1363243004/diff/1/chrome/chrome_tests_unit.gy...
chrome/chrome_tests_unit.gypi:144:
'browser/metrics/variations/ui_string_overrider_unittest.cc',
Why isn't this test in components/variations/service?

https://codereview.chromium.org/1363243004/diff/1/components/variations/servi...
File components/variations/service/variations_service.cc (right):

https://codereview.chromium.org/1363243004/diff/1/components/variations/servi...
components/variations/service/variations_service.cc:253:
base::Unretained(&ui_string_overrider_)));
On 2015/09/25 13:06:56, blundell wrote:
> On 2015/09/24 21:51:23, Alexei Svitkine (slow) wrote:
> > Can this pass a ref to the object instead of a callback?
> 
> //components/variations can't depend on //ui/base, as the latter introduces a
> dependency on //net, which causes Cronet to choke. :(

Gotcha. Maybe worth mentioning that in the comment above so someone doesn't try
to make the same mistake later.

[sdefresne, 2015-09-28 08:57:33]

Please take another look.

https://codereview.chromium.org/1363243004/diff/1/chrome/browser/metrics/vari...
File chrome/browser/metrics/variations/ui_string_overrider_unittest.cc (right):

https://codereview.chromium.org/1363243004/diff/1/chrome/browser/metrics/vari...
chrome/browser/metrics/variations/ui_string_overrider_unittest.cc:15: :
provider_(kResourceHashes, kResourceIndices, kNumResources) {}
On 2015/09/24 at 21:51:23, Alexei Svitkine (OOO) wrote:
> Nit: This formatting doesn't seem right. git cl format?

Acknowledged and fixed.

https://codereview.chromium.org/1363243004/diff/1/chrome/chrome_tests_unit.gypi
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/1363243004/diff/1/chrome/chrome_tests_unit.gy...
chrome/chrome_tests_unit.gypi:144:
'browser/metrics/variations/ui_string_overrider_unittest.cc',
On 2015/09/25 at 14:45:56, Alexei Svitkine (OOO) wrote:
> Why isn't this test in components/variations/service?

Currently the test dependents on the data generated from chrome/ .grd file
(chrome/browser/metrics/variations_generated_resources_map.cc). Once I've
componentized the script and wrote .gni/.gyi files to help using it, I'll
rewrite the test to depends on components/components_strings.grd and move it to
//components/variations/service.

https://codereview.chromium.org/1363243004/diff/1/components/variations/servi...
File components/variations/service/ui_string_overrider.h (right):

https://codereview.chromium.org/1363243004/diff/1/components/variations/servi...
components/variations/service/ui_string_overrider.h:46: size_t num_resources_;
On 2015/09/24 at 21:51:23, Alexei Svitkine (OOO) wrote:
> Can you declare all of these as const? ie. so they're not changed after ctor
assignment.
> 
> const size_t
> const uint32_t* const
> etc.

Done.

https://codereview.chromium.org/1363243004/diff/1/components/variations/servi...
File components/variations/service/variations_service.cc (right):

https://codereview.chromium.org/1363243004/diff/1/components/variations/servi...
components/variations/service/variations_service.cc:253:
base::Unretained(&ui_string_overrider_)));
On 2015/09/25 at 14:45:56, Alexei Svitkine (OOO) wrote:
> On 2015/09/25 13:06:56, blundell wrote:
> > On 2015/09/24 21:51:23, Alexei Svitkine (slow) wrote:
> > > Can this pass a ref to the object instead of a callback?
> > 
> > //components/variations can't depend on //ui/base, as the latter introduces
a
> > dependency on //net, which causes Cronet to choke. :(
> 
> Gotcha. Maybe worth mentioning that in the comment above so someone doesn't
try to make the same mistake later.

Acknowledged and added a comment.

[Alexei Svitkine (slow), 2015-09-28 15:01:46]

lgtm

https://codereview.chromium.org/1363243004/diff/20001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/ui_string_overrider_unittest.cc (right):

https://codereview.chromium.org/1363243004/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/variations/ui_string_overrider_unittest.cc:12: class
UIStringOverriderTest : public ::testing::Test {
Nit: Please add a TODO to componentize this test.

https://codereview.chromium.org/1363243004/diff/20001/components/variations/s...
File components/variations/service/ui_string_overrider.h (right):

https://codereview.chromium.org/1363243004/diff/20001/components/variations/s...
components/variations/service/ui_string_overrider.h:13: // Provides a mapping
from hashes of generated reosurce names to their IDs. The
Nit: resource

https://codereview.chromium.org/1363243004/diff/20001/components/variations/s...
components/variations/service/ui_string_overrider.h:47: };
Please add a comment why there's now DISALLOW_ here - i.e. that tthis class is
meant to be copyable - or add the macro if that's not the case.

[sdefresne, 2015-09-28 16:53:56]

Thank you for the review.

https://codereview.chromium.org/1363243004/diff/20001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/ui_string_overrider_unittest.cc (right):

https://codereview.chromium.org/1363243004/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/variations/ui_string_overrider_unittest.cc:12: class
UIStringOverriderTest : public ::testing::Test {
On 2015/09/28 at 15:01:46, Alexei Svitkine wrote:
> Nit: Please add a TODO to componentize this test.

Done.

https://codereview.chromium.org/1363243004/diff/20001/components/variations/s...
File components/variations/service/ui_string_overrider.h (right):

https://codereview.chromium.org/1363243004/diff/20001/components/variations/s...
components/variations/service/ui_string_overrider.h:13: // Provides a mapping
from hashes of generated reosurce names to their IDs. The
On 2015/09/28 at 15:01:46, Alexei Svitkine wrote:
> Nit: resource

Done.

https://codereview.chromium.org/1363243004/diff/20001/components/variations/s...
components/variations/service/ui_string_overrider.h:47: };
On 2015/09/28 at 15:01:46, Alexei Svitkine wrote:
> Please add a comment why there's now DISALLOW_ here - i.e. that tthis class is
meant to be copyable - or add the macro if that's not the case.

Done.

[sdefresne, 2015-09-28 16:54:01]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2015-09-28 16:54:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1363243004/#ps40001
(title: "Add comment to test and class and fix typo")

[commit-bot: I haz the power, 2015-09-28 16:55:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1363243004/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1363243004/40001

[commit-bot: I haz the power, 2015-09-28 19:21:31]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-09-28 19:24:49]

Patchset 3 (id:??) landed as
https://crrev.com/58b0a3f0ba85f77bedd15f357f6314298949b7c2
Cr-Commit-Position: refs/heads/master@{#351102}