Various optimizations to reduce the number of temporary allocations.

Various optimizations to reduce the number of temporary allocations
(depends on CL753603002).

- Replaced StringPrintf with multiple appends for improved performance.
- Reduced the number of temporary string allocations.
- Changed the algorithm by which an IDR is searched, using a standard linear
  search (removed the temporary set that was created).

BUG=

Committed: https://crrev.com/7d527ea37727f05e36ec15744d2ce7decac37ffb
Cr-Commit-Position: refs/heads/master@{#306743}

[Georges Khalil, 2014-11-21 19:44:41]

Patchset #1 (id:1) has been deleted

[Georges Khalil, 2014-11-21 20:28:13]

Patchset #1 (id:20001) has been deleted

[Georges Khalil, 2014-11-24 19:44:00]

Patchset #1 (id:40001) has been deleted

[Sigurður Ásgeirsson, 2014-11-25 01:37:00]

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

[Sigurður Ásgeirsson, 2014-11-25 01:37:01]

https://codereview.chromium.org/747013003/diff/60001/chrome/browser/themes/br...
File chrome/browser/themes/browser_theme_pack.cc (right):

https://codereview.chromium.org/747013003/diff/60001/chrome/browser/themes/br...
chrome/browser/themes/browser_theme_pack.cc:103: { PRS_THEME_FRAME,
IDR_THEME_FRAME,
it should be easy to mandate these be in-order and to use a unittest to make
sure it's so - in which case the search in log(n).

[Georges Khalil, 2014-12-01 16:13:51]

georgesak@chromium.org changed reviewers:
+ sky@chromium.org

[Georges Khalil, 2014-12-01 16:13:51]

PTAL.

[Nico, 2014-12-01 17:05:15]

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

[Nico, 2014-12-01 17:05:15]

> Replaced StringPrintf with multiple appends for improved performance.

This part does not lgtm.

https://codereview.chromium.org/747013003/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/font_settings/font_settings_api.cc (right):

https://codereview.chromium.org/747013003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/font_settings/font_settings_api.cc:92:
pref_name->append(script);
This kind of change does not lgtm. The previous code is much more readable.

If this is actually a performance issue, maybe we can improve StringPrintf(), or
port the google-internal StrCat() thing which I believe is pretty good about
allocations.

[viettrungluu, 2014-12-01 18:59:02]

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

[viettrungluu, 2014-12-01 18:59:03]

https://codereview.chromium.org/747013003/diff/60001/chrome/browser/themes/th...
File chrome/browser/themes/theme_properties.cc (right):

https://codereview.chromium.org/747013003/diff/60001/chrome/browser/themes/th...
chrome/browser/themes/theme_properties.cc:206: return
BrowserThemePack::FindThemeableImageIDRs(id);
I'd call this "IsThemeableImageIDR()", rather than "Find...()".

Note that this is, AFAICT (i.e., according to code search, which may miss uses
in Windows-only code and other places), the only use of GetThemeableImageIDRs().

I'd also comment that GetThemeableImageIDRs() creating a new set each time is
crazy, since the set is the same every time. (E.g., it could just as easily
return a const reference/pointer to a singleton set.)

[Peter Kasting, 2014-12-01 23:09:17]

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

[Peter Kasting, 2014-12-01 23:09:17]

https://codereview.chromium.org/747013003/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/font_settings/font_settings_api.cc (right):

https://codereview.chromium.org/747013003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/font_settings/font_settings_api.cc:92:
pref_name->append(script);
On 2014/12/01 17:05:15, Nico wrote:
> This kind of change does not lgtm. The previous code is much more readable.

FWIW, I actually find the new code significantly more readable, because prtinf
format strings have never made sense to me (I use them so rarely).

What about this, how's its perf?  This seems more readable than either, to me:

    *pref_name = map_name + '.' + script;

Or if that's not fast enough how about writing what this patch does but with
operators (which are better than the named functions for readability IMO):

    *pref_name = map_name;
    *pref_name += '.';
    *pref_name += script;

Nico, WDYT?

[Nico, 2014-12-01 23:19:07]

https://codereview.chromium.org/747013003/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/font_settings/font_settings_api.cc (right):

https://codereview.chromium.org/747013003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/font_settings/font_settings_api.cc:92:
pref_name->append(script);
On 2014/12/01 23:09:17, Peter Kasting wrote:
> On 2014/12/01 17:05:15, Nico wrote:
> > This kind of change does not lgtm. The previous code is much more readable.
> 
> FWIW, I actually find the new code significantly more readable, because prtinf
> format strings have never made sense to me (I use them so rarely).

Huh, this is very surprising to me. I thought printf strings are so prevalent in
all languages that they make sense to everyone, but I do remember finding them
pretty hard to read when they were new to me.

…cs.chromium.org finds oodles of uses of it just below chrome/ though, so I
guess at least quite a few people like and understand them?

> What about this, how's its perf?  This seems more readable than either, to me:
> 
>     *pref_name = map_name + '.' + script;

This one does one allocation per operator+, and since a new object is returned
for every + the allocation can't be amortized away – if you have N +s, this will
do O(n^2) character copies behind the scenes (and n allocations).

> Or if that's not fast enough how about writing what this patch does but with
> operators (which are better than the named functions for readability IMO):
> 
>     *pref_name = map_name;
>     *pref_name += '.';
>     *pref_name += script;

That's better than + since now the string can grow 2x when it needs to grow and
allocations get amortized. Personally, I can read printf strings much faster
than any manual concatenation, but I admit I kind of just assumed that that's
universally true.

(A printf should in theory be implementable with a single allocation internally,
which is fewer allocations than a few +=s, but I'm not sure if our
implementation gets this right.)

[Peter Kasting, 2014-12-01 23:35:31]

https://codereview.chromium.org/747013003/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/font_settings/font_settings_api.cc (right):

https://codereview.chromium.org/747013003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/font_settings/font_settings_api.cc:92:
pref_name->append(script);
On 2014/12/01 23:19:06, Nico wrote:
> On 2014/12/01 23:09:17, Peter Kasting wrote:
> > On 2014/12/01 17:05:15, Nico wrote:
> > > This kind of change does not lgtm. The previous code is much more
readable.
> > 
> > FWIW, I actually find the new code significantly more readable, because
prtinf
> > format strings have never made sense to me (I use them so rarely).
> 
> Huh, this is very surprising to me. I thought printf strings are so prevalent
in
> all languages that they make sense to everyone, but I do remember finding them
> pretty hard to read when they were new to me.
> 
> …cs.chromium.org finds oodles of uses of it just below chrome/ though, so I
> guess at least quite a few people like and understand them?

We have quite a few in our codebase, but I think in part it's because a lot of
people don't really know any other way to e.g. concat an int to a string, or
assume that StringPrintF is always going to be much faster than the equivalent
functionality using just the string methods.  I go out of my way to avoid them
in my own code because they just look like line noise to me :P

> > What about this, how's its perf?  This seems more readable than either, to
me:
> > 
> >     *pref_name = map_name + '.' + script;
> 
> This one does one allocation per operator+, and since a new object is returned
> for every + the allocation can't be amortized away – if you have N +s,
this will
> do O(n^2) character copies behind the scenes (and n allocations).

I was hoping that one of the new objects (the result of the whole RHS) would get
optimized away, so we'd just have one new object for "map_name + .", but yeah,
it's not as good as the way this patch is written.

Separately, I really hate the way this patch passes in |pref_name| when it's
neither an in-param nor an out-param.  I think the function should still take 3
args and should instead define pref_name as a local outside the loop if it wants
to avoid allocing and freeing as much.  This results in more allocations than
the way the patch currently stands, but fewer than the number the original code
did, and doesn't make the API misleading.

That said, since this is file-local, it's better than if this was in a .h
somewhere.

[cmumford, 2014-12-03 17:28:28]

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

[cmumford, 2014-12-03 17:28:29]

https://codereview.chromium.org/747013003/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/font_settings/font_settings_api.cc (right):

https://codereview.chromium.org/747013003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/font_settings/font_settings_api.cc:92:
pref_name->append(script);
On 2014/12/01 23:35:30, Peter Kasting wrote:
> On 2014/12/01 23:19:06, Nico wrote:
> > On 2014/12/01 23:09:17, Peter Kasting wrote:
> > > On 2014/12/01 17:05:15, Nico wrote:
> > > > This kind of change does not lgtm. The previous code is much more
> readable.
> > > 
> > > FWIW, I actually find the new code significantly more readable, because
> prtinf
> > > format strings have never made sense to me (I use them so rarely).
> > 
> > Huh, this is very surprising to me. I thought printf strings are so
prevalent
> in
> > all languages that they make sense to everyone, but I do remember finding
them
> > pretty hard to read when they were new to me.
> > 
> > …cs.chromium.org finds oodles of uses of it just below chrome/ though, so I
> > guess at least quite a few people like and understand them?
> 
> We have quite a few in our codebase, but I think in part it's because a lot of
> people don't really know any other way to e.g. concat an int to a string, or
> assume that StringPrintF is always going to be much faster than the equivalent
> functionality using just the string methods.  I go out of my way to avoid them
> in my own code because they just look like line noise to me :P
> 
> > > What about this, how's its perf?  This seems more readable than either, to
> me:
> > > 
> > >     *pref_name = map_name + '.' + script;
> > 
> > This one does one allocation per operator+, and since a new object is
returned
> > for every + the allocation can't be amortized away – if you have N +s,
> this will
> > do O(n^2) character copies behind the scenes (and n allocations).
> 
> I was hoping that one of the new objects (the result of the whole RHS) would
get
> optimized away, so we'd just have one new object for "map_name + .", but yeah,
> it's not as good as the way this patch is written.
> 
> Separately, I really hate the way this patch passes in |pref_name| when it's
> neither an in-param nor an out-param.  I think the function should still take
3
> args and should instead define pref_name as a local outside the loop if it
wants
> to avoid allocing and freeing as much.  This results in more allocations than
> the way the patch currently stands, but fewer than the number the original
code
> did, and doesn't make the API misleading.
> 
> That said, since this is file-local, it's better than if this was in a .h
> somewhere.

I wonder if creating a variadic template function to concat strings (and
possibly other types) would meet the goal of reducing allocations and also
improve readability. Something like
http://stackoverflow.com/questions/21806561/concatenating-strings-and-numbers...

[Georges Khalil, 2014-12-03 18:49:27]

Patchset #2 (id:80001) has been deleted

[Georges Khalil, 2014-12-03 18:54:08]

Patchset #2 (id:100001) has been deleted

[Georges Khalil, 2014-12-03 18:55:53]

Patchset #2 (id:120001) has been deleted

[Georges Khalil, 2014-12-03 19:00:22]

Patchset #2 (id:140001) has been deleted

[Georges Khalil, 2014-12-03 19:10:08]

PTAL.

- Changed FindThemeableImageIDR() to IsThemeableImageIDR().
- Removed GetThemeableImageIDRs().
- The string pref_name is now created again inside
  RegisterFontFamilyMapObserver() but outside the loop.
  
Note: we are now exploring different alternatives to the current StringPrintF
functions in order to optimize its efficiency.

https://codereview.chromium.org/747013003/diff/60001/chrome/browser/themes/th...
File chrome/browser/themes/theme_properties.cc (right):

https://codereview.chromium.org/747013003/diff/60001/chrome/browser/themes/th...
chrome/browser/themes/theme_properties.cc:206: return
BrowserThemePack::FindThemeableImageIDRs(id);
On 2014/12/01 18:59:02, viettrungluu wrote:
> I'd call this "IsThemeableImageIDR()", rather than "Find...()".
> 
> Note that this is, AFAICT (i.e., according to code search, which may miss uses
> in Windows-only code and other places), the only use of
GetThemeableImageIDRs().
> 
> I'd also comment that GetThemeableImageIDRs() creating a new set each time is
> crazy, since the set is the same every time. (E.g., it could just as easily
> return a const reference/pointer to a singleton set.)

Done.

[Nico, 2014-12-03 19:14:39]

https://codereview.chromium.org/747013003/diff/180001/chrome/browser/extensio...
File chrome/browser/extensions/api/font_settings/font_settings_api.cc (right):

https://codereview.chromium.org/747013003/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/font_settings/font_settings_api.cc:94:
registrar->Add(pref_name, callback);
This still does not lgtm.

[Georges Khalil, 2014-12-03 19:30:41]

On 2014/12/03 19:14:39, Nico wrote:
>
https://codereview.chromium.org/747013003/diff/180001/chrome/browser/extensio...
> File chrome/browser/extensions/api/font_settings/font_settings_api.cc (right):
> 
>
https://codereview.chromium.org/747013003/diff/180001/chrome/browser/extensio...
> chrome/browser/extensions/api/font_settings/font_settings_api.cc:94:
> registrar->Add(pref_name, callback);
> This still does not lgtm.

Would this be acceptable to you? I am trying to eliminate the StringPrintf,
which is much slower.

pref_name.assign(map_name);
pref_name += '.';
pref_name += script);

[Nico, 2014-12-03 19:43:04]

On 2014/12/03 19:30:41, Georges Khalil wrote:
> On 2014/12/03 19:14:39, Nico wrote:
> >
>
https://codereview.chromium.org/747013003/diff/180001/chrome/browser/extensio...
> > File chrome/browser/extensions/api/font_settings/font_settings_api.cc
(right):
> > 
> >
>
https://codereview.chromium.org/747013003/diff/180001/chrome/browser/extensio...
> > chrome/browser/extensions/api/font_settings/font_settings_api.cc:94:
> > registrar->Add(pref_name, callback);
> > This still does not lgtm.
> 
> Would this be acceptable to you? I am trying to eliminate the StringPrintf,
> which is much slower.
> 
> pref_name.assign(map_name);
> pref_name += '.';
> pref_name += script);

No. As I said above, if StringPrintf is inefficient*, we should make it more
efficient, not rewrite the codebase to not use it.

*: (note that there's no data that the allocations actually have any measurable
cost in non-instrumented builds.)

[Georges Khalil, 2014-12-03 19:46:43]

On 2014/12/03 19:43:04, Nico wrote:
> On 2014/12/03 19:30:41, Georges Khalil wrote:
> > On 2014/12/03 19:14:39, Nico wrote:
> > >
> >
>
https://codereview.chromium.org/747013003/diff/180001/chrome/browser/extensio...
> > > File chrome/browser/extensions/api/font_settings/font_settings_api.cc
> (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/747013003/diff/180001/chrome/browser/extensio...
> > > chrome/browser/extensions/api/font_settings/font_settings_api.cc:94:
> > > registrar->Add(pref_name, callback);
> > > This still does not lgtm.
> > 
> > Would this be acceptable to you? I am trying to eliminate the StringPrintf,
> > which is much slower.
> > 
> > pref_name.assign(map_name);
> > pref_name += '.';
> > pref_name += script);
> 
> No. As I said above, if StringPrintf is inefficient*, we should make it more
> efficient, not rewrite the codebase to not use it.
> 
> *: (note that there's no data that the allocations actually have any
measurable
> cost in non-instrumented builds.)

Fair enough. I still think StringPrintf should not be used for concatenations of
strings that can be done much more efficiently. I guess the best way is to have
a function that can concatenate an arbitrarily number of strings, such as
strcat.
We will look into it.

[Peter Kasting, 2014-12-03 20:05:17]

On 2014/12/03 19:46:43, Georges Khalil wrote:
> On 2014/12/03 19:43:04, Nico wrote:
> > On 2014/12/03 19:30:41, Georges Khalil wrote:
> > > On 2014/12/03 19:14:39, Nico wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/747013003/diff/180001/chrome/browser/extensio...
> > > > File chrome/browser/extensions/api/font_settings/font_settings_api.cc
> > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/747013003/diff/180001/chrome/browser/extensio...
> > > > chrome/browser/extensions/api/font_settings/font_settings_api.cc:94:
> > > > registrar->Add(pref_name, callback);
> > > > This still does not lgtm.
> > > 
> > > Would this be acceptable to you? I am trying to eliminate the
StringPrintf,
> > > which is much slower.
> > > 
> > > pref_name.assign(map_name);
> > > pref_name += '.';
> > > pref_name += script);
> > 
> > No. As I said above, if StringPrintf is inefficient*, we should make it more
> > efficient, not rewrite the codebase to not use it.
> > 
> > *: (note that there's no data that the allocations actually have any
> measurable
> > cost in non-instrumented builds.)
> 
> Fair enough. I still think StringPrintf should not be used for concatenations
of
> strings that can be done much more efficiently. I guess the best way is to
have
> a function that can concatenate an arbitrarily number of strings, such as
> strcat.
> We will look into it.

A string concatenator only solves some uses of StringPrintf(), and still
requires code to be rewritten to use it.  I happen to like your change here, but
if Nico doesn't, I agree that just making StringPrintf() more efficient is
better than introducing Yet Another Way To Muck With Strings.  I would prefer to
choose either += or StringPrintf() over some sort of strcat().

[Georges Khalil, 2014-12-03 20:31:43]

On 2014/12/03 20:05:17, Peter Kasting wrote:
> On 2014/12/03 19:46:43, Georges Khalil wrote:
> > On 2014/12/03 19:43:04, Nico wrote:
> > > On 2014/12/03 19:30:41, Georges Khalil wrote:
> > > > On 2014/12/03 19:14:39, Nico wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/747013003/diff/180001/chrome/browser/extensio...
> > > > > File chrome/browser/extensions/api/font_settings/font_settings_api.cc
> > > (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/747013003/diff/180001/chrome/browser/extensio...
> > > > > chrome/browser/extensions/api/font_settings/font_settings_api.cc:94:
> > > > > registrar->Add(pref_name, callback);
> > > > > This still does not lgtm.
> > > > 
> > > > Would this be acceptable to you? I am trying to eliminate the
> StringPrintf,
> > > > which is much slower.
> > > > 
> > > > pref_name.assign(map_name);
> > > > pref_name += '.';
> > > > pref_name += script);
> > > 
> > > No. As I said above, if StringPrintf is inefficient*, we should make it
more
> > > efficient, not rewrite the codebase to not use it.
> > > 
> > > *: (note that there's no data that the allocations actually have any
> > measurable
> > > cost in non-instrumented builds.)
> > 
> > Fair enough. I still think StringPrintf should not be used for
concatenations
> of
> > strings that can be done much more efficiently. I guess the best way is to
> have
> > a function that can concatenate an arbitrarily number of strings, such as
> > strcat.
> > We will look into it.
> 
> A string concatenator only solves some uses of StringPrintf(), and still
> requires code to be rewritten to use it.  I happen to like your change here,
but
> if Nico doesn't, I agree that just making StringPrintf() more efficient is
> better than introducing Yet Another Way To Muck With Strings.  I would prefer
to
> choose either += or StringPrintf() over some sort of strcat().

I've been looking at making StringPrintf faster, even rewrote it using variadic
templates, but it doesn't really help. It still needs to scan for formatting and
that takes time. I am currently writing a string concatenation function that is
simple to use and works with all primitive types, should have a CL by the end of
tomorrow. I would love both you and Nico's input on it, when I upload it.

[Georges Khalil, 2014-12-03 20:32:19]

PTAL.

- Restored StringPrintf, as per Nico's comments.

[Nico, 2014-12-03 20:34:34]

https://codereview.chromium.org/747013003/diff/200001/chrome/browser/extensio...
File chrome/browser/extensions/api/font_settings/font_settings_api.cc (left):

https://codereview.chromium.org/747013003/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/api/font_settings/font_settings_api.cc:89: std::string
pref_name = base::StringPrintf("%s.%s", map_name, script);
doesn't look restored?

[Georges Khalil, 2014-12-03 20:40:55]

PTAL.

- My bad, there is 2 of them and I only restored one. Brain cramp.

[Nico, 2014-12-03 20:44:31]

lgtm with "- Replaced StringPrintf with multiple appends for improved
performance." removed from the cl description.

Personally, I'm not a fan of unjustified reserve() calls either, but up to you.

https://codereview.chromium.org/747013003/diff/220001/chrome/browser/extensio...
File chrome/browser/extensions/api/font_settings/font_settings_api.cc (right):

https://codereview.chromium.org/747013003/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/api/font_settings/font_settings_api.cc:88:
pref_name.reserve(512);  // Reduces number of grows.
(fwif, this too is something i'd only add if there's an actual measurable
benefit from it, ideally with some perf test)

https://codereview.chromium.org/747013003/diff/220001/chrome/browser/ui/prefs...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

https://codereview.chromium.org/747013003/diff/220001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs/prefs_tab_helper.cc:137: pref_name.reserve(512);  //
Reduces number of grows.
likewise

[Georges Khalil, 2014-12-03 21:10:21]

On 2014/12/03 20:44:31, Nico wrote:
> lgtm with "- Replaced StringPrintf with multiple appends for improved
> performance." removed from the cl description.
> 
> Personally, I'm not a fan of unjustified reserve() calls either, but up to
you.
> 
>
https://codereview.chromium.org/747013003/diff/220001/chrome/browser/extensio...
> File chrome/browser/extensions/api/font_settings/font_settings_api.cc (right):
> 
>
https://codereview.chromium.org/747013003/diff/220001/chrome/browser/extensio...
> chrome/browser/extensions/api/font_settings/font_settings_api.cc:88:
> pref_name.reserve(512);  // Reduces number of grows.
> (fwif, this too is something i'd only add if there's an actual measurable
> benefit from it, ideally with some perf test)
> 
>
https://codereview.chromium.org/747013003/diff/220001/chrome/browser/ui/prefs...
> File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):
> 
>
https://codereview.chromium.org/747013003/diff/220001/chrome/browser/ui/prefs...
> chrome/browser/ui/prefs/prefs_tab_helper.cc:137: pref_name.reserve(512);  //
> Reduces number of grows.
> likewise

I am going to remove the calls to reserve before committing. Because we're back
to using StringPrintf, they are now useless.

Thanks for the LGTM, committing after one last patch.

[Georges Khalil, 2014-12-03 21:10:49]

The CQ bit was checked by georgesak@chromium.org

[commit-bot: I haz the power, 2014-12-03 21:11:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/747013003/240001

[Nico, 2014-12-03 21:16:46]

sorry…

https://codereview.chromium.org/747013003/diff/240001/chrome/browser/extensio...
File chrome/browser/extensions/api/font_settings/font_settings_api.cc (right):

https://codereview.chromium.org/747013003/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/api/font_settings/font_settings_api.cc:87: std::string
pref_name;
Now that I look at it again: This is unused.

https://codereview.chromium.org/747013003/diff/240001/chrome/browser/ui/prefs...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

https://codereview.chromium.org/747013003/diff/240001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs/prefs_tab_helper.cc:136: std::string pref_name;
This too

[Georges Khalil, 2014-12-03 21:17:14]

The CQ bit was unchecked by georgesak@chromium.org

[Peter Kasting, 2014-12-03 21:21:03]

https://codereview.chromium.org/747013003/diff/240001/chrome/browser/extensio...
File chrome/browser/extensions/api/font_settings/font_settings_api.cc (right):

https://codereview.chromium.org/747013003/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/api/font_settings/font_settings_api.cc:90: std::string
pref_name = base::StringPrintf("%s.%s", map_name, script);
You're shadowing the outer variable with this inner one.  I think you want to
remove the type here.  (This is Nico's complaint, but with the opposite
resolution.  However, see next comment.)

https://codereview.chromium.org/747013003/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/api/font_settings/font_settings_api.cc:91:
registrar->Add(pref_name, callback);
What's the perf of just doing:

  registrar->Add(base::StringPrintf("%s.%s", map_name, script), callback);

?  If StringPrintf() has to construct an output string anyway, I can't imagine
that this is more allocations than the way you've written the code currently,
and it's shorter and avoids the somewhat confusing-looking declaration of
pref_name above the loop (I would normally see this code and move the
declaration back in to comply with the style guide's rule to "declare variables
in the most local scope").

https://codereview.chromium.org/747013003/diff/240001/chrome/browser/themes/b...
File chrome/browser/themes/browser_theme_pack.h (right):

https://codereview.chromium.org/747013003/diff/240001/chrome/browser/themes/b...
chrome/browser/themes/browser_theme_pack.h:71: static bool
IsThemeableImageIDRs(int id);
This name is confusing.  Why does it have an 's' on the end?  Why does the
comment just talk about the IDR "existing" when that's not what the function
does?  Seems more like this should be:

  // Returns whether the specified identifier is one of the images we persist in
the data pack.
  static bool IsPersistentImageID(int id);

https://codereview.chromium.org/747013003/diff/240001/chrome/browser/themes/t...
File chrome/browser/themes/theme_properties.cc (right):

https://codereview.chromium.org/747013003/diff/240001/chrome/browser/themes/t...
chrome/browser/themes/theme_properties.cc:204: bool
ThemeProperties::IsThemeableImage(int id) {
Can we eliminate this method and have consumers call the BrowserThemePack method
directly?

https://codereview.chromium.org/747013003/diff/240001/chrome/browser/ui/prefs...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

https://codereview.chromium.org/747013003/diff/240001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs/prefs_tab_helper.cc:140: registrar->Add(pref_name, obs);
My same question from the other file applies here.

[Georges Khalil, 2014-12-03 21:21:38]

On 2014/12/03 21:16:46, Nico wrote:
> sorry…
> 
>
https://codereview.chromium.org/747013003/diff/240001/chrome/browser/extensio...
> File chrome/browser/extensions/api/font_settings/font_settings_api.cc (right):
> 
>
https://codereview.chromium.org/747013003/diff/240001/chrome/browser/extensio...
> chrome/browser/extensions/api/font_settings/font_settings_api.cc:87:
std::string
> pref_name;
> Now that I look at it again: This is unused.
> 
>
https://codereview.chromium.org/747013003/diff/240001/chrome/browser/ui/prefs...
> File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):
> 
>
https://codereview.chromium.org/747013003/diff/240001/chrome/browser/ui/prefs...
> chrome/browser/ui/prefs/prefs_tab_helper.cc:136: std::string pref_name;
> This too

Another brain cramp, sorry. Obviously, I should not redeclare |pref_name| in
the loop. That was a copy/paste error. Fixing it in both places.

[Georges Khalil, 2014-12-03 21:44:06]

Patchset #8 (id:280001) has been deleted

[Georges Khalil, 2014-12-03 21:57:10]

Patchset #7 (id:260001) has been deleted

[Georges Khalil, 2014-12-03 21:58:32]

PTAL before committing as the changes are not trivial.

- Eliminated |pref_name| in two places.
- Renamed IsThemeableImageIDRs to IsPersistentImageID.
- Eliminated ThemeProperties::IsThemeableImage and replaced with
  BrowserThemePack::IsPersistentImageID.

https://codereview.chromium.org/747013003/diff/240001/chrome/browser/extensio...
File chrome/browser/extensions/api/font_settings/font_settings_api.cc (right):

https://codereview.chromium.org/747013003/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/api/font_settings/font_settings_api.cc:91:
registrar->Add(pref_name, callback);
On 2014/12/03 21:21:03, Peter Kasting wrote:
> What's the perf of just doing:
> 
>   registrar->Add(base::StringPrintf("%s.%s", map_name, script), callback);
> 
> ?  If StringPrintf() has to construct an output string anyway, I can't imagine
> that this is more allocations than the way you've written the code currently,
> and it's shorter and avoids the somewhat confusing-looking declaration of
> pref_name above the loop (I would normally see this code and move the
> declaration back in to comply with the style guide's rule to "declare
variables
> in the most local scope").

Agreed, no impact on performance. Done. Hope that works for you too Nico! :)

https://codereview.chromium.org/747013003/diff/240001/chrome/browser/themes/b...
File chrome/browser/themes/browser_theme_pack.h (right):

https://codereview.chromium.org/747013003/diff/240001/chrome/browser/themes/b...
chrome/browser/themes/browser_theme_pack.h:71: static bool
IsThemeableImageIDRs(int id);
On 2014/12/03 21:21:03, Peter Kasting wrote:
> This name is confusing.  Why does it have an 's' on the end?  Why does the
> comment just talk about the IDR "existing" when that's not what the function
> does?  Seems more like this should be:
> 
>   // Returns whether the specified identifier is one of the images we persist
in
> the data pack.
>   static bool IsPersistentImageID(int id);

Done.

https://codereview.chromium.org/747013003/diff/240001/chrome/browser/themes/t...
File chrome/browser/themes/theme_properties.cc (right):

https://codereview.chromium.org/747013003/diff/240001/chrome/browser/themes/t...
chrome/browser/themes/theme_properties.cc:204: bool
ThemeProperties::IsThemeableImage(int id) {
On 2014/12/03 21:21:03, Peter Kasting wrote:
> Can we eliminate this method and have consumers call the BrowserThemePack
method
> directly?

Sounds good. Done.

[Nico, 2014-12-03 22:02:35]

still lgtm

[Peter Kasting, 2014-12-03 22:08:39]

LGTM!

https://codereview.chromium.org/747013003/diff/300001/chrome/browser/themes/t...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/747013003/diff/300001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.cc:239: if
(!BrowserThemePack::IsPersistentImageID(id))
Optional: While you're here, a shorter way would be:

  return BrowserThemePack::IsPersistentImageID(id) &&
      theme_supplier_ && theme_supplier_->HasCustomImage(id);

(This also removes the unnecessary .get().)

[Georges Khalil, 2014-12-03 22:26:29]

Patchset #8 (id:320001) has been deleted

[Georges Khalil, 2014-12-03 22:29:26]

Patchset #8 (id:340001) has been deleted

[Georges Khalil, 2014-12-03 22:34:11]

On 2014/12/03 22:08:39, Peter Kasting wrote:
> LGTM!
> 
>
https://codereview.chromium.org/747013003/diff/300001/chrome/browser/themes/t...
> File chrome/browser/themes/theme_service.cc (right):
> 
>
https://codereview.chromium.org/747013003/diff/300001/chrome/browser/themes/t...
> chrome/browser/themes/theme_service.cc:239: if
> (!BrowserThemePack::IsPersistentImageID(id))
> Optional: While you're here, a shorter way would be:
> 
>   return BrowserThemePack::IsPersistentImageID(id) &&
>       theme_supplier_ && theme_supplier_->HasCustomImage(id);
> 
> (This also removes the unnecessary .get().)

Done, thanks!

Committing.

[Georges Khalil, 2014-12-03 22:34:22]

The CQ bit was checked by georgesak@chromium.org

[commit-bot: I haz the power, 2014-12-03 22:35:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/747013003/360001

[commit-bot: I haz the power, 2014-12-03 22:46:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-03 22:46:08]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Georges Khalil, 2014-12-04 00:52:04]

The CQ bit was checked by georgesak@chromium.org

[commit-bot: I haz the power, 2014-12-04 00:52:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/747013003/380001

[commit-bot: I haz the power, 2014-12-04 01:55:22]

Committed patchset #9 (id:380001)

[commit-bot: I haz the power, 2014-12-04 01:56:04]

Patchset 9 (id:??) landed as
https://crrev.com/7d527ea37727f05e36ec15744d2ce7decac37ffb
Cr-Commit-Position: refs/heads/master@{#306743}