Adding OverrideStringResource API to ResourceBundle.

Adding OverrideStringResource API to ResourceBundle.

This is to facilitate overriding UI strings from the variations service.

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

[grt (UTC plus 2), 2014-06-09 16:54:44]

https://codereview.chromium.org/322523002/diff/1/ui/base/resource/resource_bu...
File ui/base/resource/resource_bundle.cc (right):

https://codereview.chromium.org/322523002/diff/1/ui/base/resource/resource_bu...
ui/base/resource/resource_bundle.cc:447: if
(overridden_strings_.find(message_id) != overridden_strings_.end())
avoid doing two O(logN) scans when there's a hit with:
  IdToStringMap::const_iterator it = overridden_strings_.find(message_id);
  if (it != overridden_strings_.end())
    return it->second;

or something like that

https://codereview.chromium.org/322523002/diff/1/ui/base/resource/resource_bu...
File ui/base/resource/resource_bundle.h (right):

https://codereview.chromium.org/322523002/diff/1/ui/base/resource/resource_bu...
ui/base/resource/resource_bundle.h:254: const base::string16&
overridding_string);
nit: overridding_string -> string here and in comment. it's nice to not wrap
this, and the purpose is clear enough given the name of the method.

https://codereview.chromium.org/322523002/diff/1/ui/base/resource/resource_bu...
ui/base/resource/resource_bundle.h:402: std::map<int, base::string16>
overridden_strings_;
suggestion: make a typdef for this up at the top of the private section
(honoring the ordering prescribed by the style guide)
  typedef std::map<int, base::string16> IdToStringMap;

https://codereview.chromium.org/322523002/diff/1/ui/base/resource/resource_bu...
File ui/base/resource/resource_bundle_unittest.cc (right):

https://codereview.chromium.org/322523002/diff/1/ui/base/resource/resource_bu...
ui/base/resource/resource_bundle_unittest.cc:316: 
does it make sense to test that GetLocalizedString returns something other than
"My test data" for id 5 before OverrideStringResource is called?

[jwd, 2014-06-10 20:59:25]

https://codereview.chromium.org/322523002/diff/1/ui/base/resource/resource_bu...
File ui/base/resource/resource_bundle.cc (right):

https://codereview.chromium.org/322523002/diff/1/ui/base/resource/resource_bu...
ui/base/resource/resource_bundle.cc:447: if
(overridden_strings_.find(message_id) != overridden_strings_.end())
On 2014/06/09 16:54:44, grt wrote:
> avoid doing two O(logN) scans when there's a hit with:
>   IdToStringMap::const_iterator it = overridden_strings_.find(message_id);
>   if (it != overridden_strings_.end())
>     return it->second;
> 
> or something like that

Done.

https://codereview.chromium.org/322523002/diff/1/ui/base/resource/resource_bu...
File ui/base/resource/resource_bundle.h (right):

https://codereview.chromium.org/322523002/diff/1/ui/base/resource/resource_bu...
ui/base/resource/resource_bundle.h:254: const base::string16&
overridding_string);
On 2014/06/09 16:54:44, grt wrote:
> nit: overridding_string -> string here and in comment. it's nice to not wrap
> this, and the purpose is clear enough given the name of the method.

Done.

https://codereview.chromium.org/322523002/diff/1/ui/base/resource/resource_bu...
ui/base/resource/resource_bundle.h:402: std::map<int, base::string16>
overridden_strings_;
On 2014/06/09 16:54:44, grt wrote:
> suggestion: make a typdef for this up at the top of the private section
> (honoring the ordering prescribed by the style guide)
>   typedef std::map<int, base::string16> IdToStringMap;

Done.

https://codereview.chromium.org/322523002/diff/1/ui/base/resource/resource_bu...
File ui/base/resource/resource_bundle_unittest.cc (right):

https://codereview.chromium.org/322523002/diff/1/ui/base/resource/resource_bu...
ui/base/resource/resource_bundle_unittest.cc:316: 
On 2014/06/09 16:54:44, grt wrote:
> does it make sense to test that GetLocalizedString returns something other
than
> "My test data" for id 5 before OverrideStringResource is called?

Done, but doesn't test the successful return of actual non-overridden data. The
call before override hits resource_bundle.cc(458) that checks if there are any
loaded resources.

[grt (UTC plus 2), 2014-06-11 01:51:18]

lgtm with nits.

https://codereview.chromium.org/322523002/diff/20001/ui/base/resource/resourc...
File ui/base/resource/resource_bundle.cc (right):

https://codereview.chromium.org/322523002/diff/20001/ui/base/resource/resourc...
ui/base/resource/resource_bundle.cc:315: int message_id, const base::string16&
overridding_string) {
overridding_string -> string to match the declaration in the header file

https://codereview.chromium.org/322523002/diff/20001/ui/base/resource/resourc...
ui/base/resource/resource_bundle.cc:315: int message_id, const base::string16&
overridding_string) {
did "git cl format" do this wrapping? if not, i believe each param should be on
its own line if the first param needs to be wrapped
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...).

https://codereview.chromium.org/322523002/diff/20001/ui/base/resource/resourc...
File ui/base/resource/resource_bundle.h (right):

https://codereview.chromium.org/322523002/diff/20001/ui/base/resource/resourc...
ui/base/resource/resource_bundle.h:276: typedef base::hash_map<int,
base::string16> IdToStringMap;
#include "base/containers/hash_tables.h"

[jwd, 2014-07-03 17:41:31]

Added a reset of the overrides when the locale is changed.

https://codereview.chromium.org/322523002/diff/20001/ui/base/resource/resourc...
File ui/base/resource/resource_bundle.cc (right):

https://codereview.chromium.org/322523002/diff/20001/ui/base/resource/resourc...
ui/base/resource/resource_bundle.cc:315: int message_id, const base::string16&
overridding_string) {
On 2014/06/11 01:51:18, grt wrote:
> overridding_string -> string to match the declaration in the header file

Done.

https://codereview.chromium.org/322523002/diff/20001/ui/base/resource/resourc...
ui/base/resource/resource_bundle.cc:315: int message_id, const base::string16&
overridding_string) {
On 2014/06/11 01:51:18, grt wrote:
> did "git cl format" do this wrapping? if not, i believe each param should be
on
> its own line if the first param needs to be wrapped
>
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...).

Done.

https://codereview.chromium.org/322523002/diff/20001/ui/base/resource/resourc...
File ui/base/resource/resource_bundle.h (right):

https://codereview.chromium.org/322523002/diff/20001/ui/base/resource/resourc...
ui/base/resource/resource_bundle.h:276: typedef base::hash_map<int,
base::string16> IdToStringMap;
On 2014/06/11 01:51:18, grt wrote:
> #include "base/containers/hash_tables.h"

Done.

[jwd, 2014-07-03 18:37:28]

PTAL

[grt (UTC plus 2), 2014-07-03 19:00:35]

https://codereview.chromium.org/322523002/diff/60001/ui/base/resource/resourc...
File ui/base/resource/resource_bundle.h (right):

https://codereview.chromium.org/322523002/diff/60001/ui/base/resource/resourc...
ui/base/resource/resource_bundle.h:255: void OverrideLocaleStringResource(int
message_id,
how will this be used so that it's safe?

[jwd, 2014-07-11 19:23:28]

+tony for owners review.

https://codereview.chromium.org/322523002/diff/60001/ui/base/resource/resourc...
File ui/base/resource/resource_bundle.h (right):

https://codereview.chromium.org/322523002/diff/60001/ui/base/resource/resourc...
ui/base/resource/resource_bundle.h:255: void OverrideLocaleStringResource(int
message_id,
On 2014/07/03 19:00:34, grt wrote:
> how will this be used so that it's safe?

It's only used in prethreadcreate...
It's only called though SetupMetricsAndFieldTrials which is called from
ChromeBrowserMainParts::PreCreateThreadsImpl.

[tony, 2014-07-11 19:44:09]

Seems reasonable to me.  LGTM

[jwd, 2014-07-11 19:48:13]

The CQ bit was checked by jwd@chromium.org

[commit-bot: I haz the power, 2014-07-11 19:50:28]

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

[commit-bot: I haz the power, 2014-07-12 00:47:35]

Change committed as 282758