Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1220793010:
[ui/base;css] adding string template expression replacement (Closed)
|
Created:
5 years, 5 months ago by dschuyler Modified:
4 years, 10 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, jshin+watch_chromium.org, groby-ooo-7-16, Evan Stade Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[ui/base;css] adding string template expression replacement
This CL provides a string replacement scheme using expressions of the form ${foo}, where foo is the name of a tag to be replaced. Several .css files in the CL have been converted to use this templating scheme rather than the numbered scheme ($1, $2, $3, etc.) previously used.
While the code is initially intended to be used for ui/css files, it would be generally useful to any code working with string data.
BUG=506009
Committed: https://crrev.com/6c937646c650fdded550560a6d443a7dae846819
Cr-Commit-Position: refs/heads/master@{#339215}
Patch Set 1 #Patch Set 2 : added comment #
Total comments: 4
Patch Set 3 : comment change; merge with master #
Total comments: 6
Patch Set 4 : changed text_default.css and new_tab_theme.css to use replacement map #Patch Set 5 : removed unsed vars; changed replace placeholder loop #Patch Set 6 : removed unneeded comments #Patch Set 7 : changed ReplaceTemplateExpressions name #Patch Set 8 : changing incognito css to use template expressions #Patch Set 9 : moved ReplaceTemplateExpressions code to ui/base #Patch Set 10 : Removing changes to base/strings #
Total comments: 16
Patch Set 11 : Removing test code #
Total comments: 9
Patch Set 12 : fix for gn #Patch Set 13 : Alternate template expression replacement, using find #
Total comments: 7
Patch Set 14 : Fix for incognito window #
Total comments: 21
Patch Set 15 : removed string16 versions of template expansions #
Total comments: 13
Patch Set 16 : removed unneeded typename #Patch Set 17 : removing one of the two replacement loops #Patch Set 18 : changed unit tests #Patch Set 19 : merge with master #
Total comments: 8
Patch Set 20 : removed dollar sign escape from template expressions #Patch Set 21 : better use of StringPiece in template expressions #
Total comments: 17
Patch Set 22 : removed include #Patch Set 23 : moved presubmit test, test #Patch Set 24 : crashing on invalid template expression #
Total comments: 8
Patch Set 25 : review changes #
Total comments: 4
Patch Set 26 : review changes #
Total comments: 1
Messages
Total messages: 57 (5 generated)
added comment
dschuyler@chromium.org changed reviewers: + dbeam@chromium.org
you'll need a base/ OWNER to tell you if this actually belongs in base/ or not (see base/OWNERS). we should also show the concrete usecase for this in this CL, e.g. webui::GetWebUiCssTextDefaults()[1] as well as NTPResourceCache::CreateNewTabCSS()[2]. [1] https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/webui/web_... [2] https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... https://codereview.chromium.org/1220793010/diff/20001/base/strings/string_util.h File base/strings/string_util.h (right): https://codereview.chromium.org/1220793010/diff/20001/base/strings/string_uti... base/strings/string_util.h:522: // Replace ${foo} in the format string with the value for the foo key in subst. subst -> |subst| https://codereview.chromium.org/1220793010/diff/20001/base/strings/string_uti... File base/strings/string_util_unittest.cc (right): https://codereview.chromium.org/1220793010/diff/20001/base/strings/string_uti... base/strings/string_util_unittest.cc:897: subst.insert(std::pair<std::string, std::string>("$$", "1i")); this test brings up a good question: what should we allow as key names? should we allow "{"? so ${{} is legal? I understand this code should probably be as simple/dumb as possible about substitution (so it's very clear how it will work), but I think this hampers readability of templates. I'd vote for only allowing [a-zA-Z]+ at first, therefore allowing only prettier substitutions while reading.
comment change; merge with master
https://codereview.chromium.org/1220793010/diff/40001/base/strings/string_uti... File base/strings/string_util.cc (right): https://codereview.chromium.org/1220793010/diff/40001/base/strings/string_uti... base/strings/string_util.cc:881: i != format_string.end(); ++i) { please remove auto-increment, IMO https://codereview.chromium.org/1220793010/diff/40001/base/strings/string_uti... File base/strings/string_util_unittest.cc (right): https://codereview.chromium.org/1220793010/diff/40001/base/strings/string_uti... base/strings/string_util_unittest.cc:913: "${a} $${b} $$${c} $"); please add one like $12 and ensure 12 is left over (or something)
estade@chromium.org changed reviewers: + estade@chromium.org
before you get much further you should make sure a base/ OWNER is okay with adding this functionality here https://codereview.chromium.org/1220793010/diff/40001/base/strings/string_util.h File base/strings/string_util.h (right): https://codereview.chromium.org/1220793010/diff/40001/base/strings/string_uti... base/strings/string_util.h:531: const std::map<std::string, std::string>& subst); don't abbreviate variable names
On 2015/07/07 17:15:17, Evan Stade wrote: > before you get much further you should make sure a base/ OWNER is okay with > adding this functionality here agreed https://codereview.chromium.org/1220793010/diff/40001/base/strings/string_util.h File base/strings/string_util.h (right): https://codereview.chromium.org/1220793010/diff/40001/base/strings/string_uti... base/strings/string_util.h:531: const std::map<std::string, std::string>& subst); On 2015/07/07 17:15:16, Evan Stade wrote: > don't abbreviate variable names yeah, i don't like it either. we could just fix the other occurrences that he's being consistent with.
Drive by... How will this work/conflict with ES'15 template literals?
On 2015/07/07 22:31:44, arv wrote: > Drive by... > > How will this work/conflict with ES'15 template literals? I don't think we're currently planning to pre-process JS (just HTML/CSS), so we'd have to take explicit steps to ensure this. Full disclosure: had no clue JS template literals were a similar syntax.
changed text_default.css and new_tab_theme.css to use replacement map
dschuyler@chromium.org changed reviewers: + thakis@chromium.org
https://codereview.chromium.org/1220793010/diff/20001/base/strings/string_util.h File base/strings/string_util.h (right): https://codereview.chromium.org/1220793010/diff/20001/base/strings/string_uti... base/strings/string_util.h:522: // Replace ${foo} in the format string with the value for the foo key in subst. On 2015/07/02 02:10:16, Dan Beam wrote: > subst -> |subst| Done. https://codereview.chromium.org/1220793010/diff/20001/base/strings/string_uti... File base/strings/string_util_unittest.cc (right): https://codereview.chromium.org/1220793010/diff/20001/base/strings/string_uti... base/strings/string_util_unittest.cc:897: subst.insert(std::pair<std::string, std::string>("$$", "1i")); On 2015/07/02 02:10:16, Dan Beam wrote: > this test brings up a good question: what should we allow as key names? should > we allow "{"? so ${{} is legal? > > I understand this code should probably be as simple/dumb as possible about > substitution (so it's very clear how it will work), but I think this hampers > readability of templates. > > I'd vote for only allowing [a-zA-Z]+ at first, therefore allowing only prettier > substitutions while reading. Could that be addressed through style review prior to committing changes? I agree that there is value in keeping the code as simple as may be. We do seem to do well with code reviews, for example. I also feel like it's the kind of thing where someone would later want underscores and then want hyphens -- oh, and numbers. It feels better to avoid all those revisions and test cases. https://codereview.chromium.org/1220793010/diff/40001/base/strings/string_uti... File base/strings/string_util.cc (right): https://codereview.chromium.org/1220793010/diff/40001/base/strings/string_uti... base/strings/string_util.cc:881: i != format_string.end(); ++i) { On 2015/07/07 02:23:31, Dan Beam wrote: > please remove auto-increment, IMO Done. https://codereview.chromium.org/1220793010/diff/40001/base/strings/string_uti... File base/strings/string_util_unittest.cc (right): https://codereview.chromium.org/1220793010/diff/40001/base/strings/string_uti... base/strings/string_util_unittest.cc:913: "${a} $${b} $$${c} $"); On 2015/07/07 02:23:31, Dan Beam wrote: > please add one like $12 and ensure 12 is left over (or something) Done.
does this have to be in base? (see text in base/OWNERS)
On 2015/07/07 23:08:11, Nico wrote: > does this have to be in base? (see text in base/OWNERS) https://codereview.chromium.org/1220793010/#msg4
On 2015/07/07 23:08:47, Dan Beam wrote: > On 2015/07/07 23:08:11, Nico wrote: > > does this have to be in base? (see text in base/OWNERS) > > https://codereview.chromium.org/1220793010/#msg4 Oh, yeah, that's why I was adding a base owner as a reviewer :) I wrote it up in this location because it feels to me like something generally useful. It should be at least as useful as the existing ReplaceStringPlaceholders (or more so if one prefers labels over index numbers). So, it seems like a good addition to base. What do you think?
removed unsed vars; changed replace placeholder loop
removed unneeded comments
On 2015/07/07 23:08:47, Dan Beam wrote: > On 2015/07/07 23:08:11, Nico wrote: > > does this have to be in base? (see text in base/OWNERS) > > https://codereview.chromium.org/1220793010/#msg4 I would say "no", but I guess I'd say that for the existing $1 substitutions too. Maybe they're there because grit uses them. …wow, this code has been in base since initial.commit. The history before that is murky. ("Seems like it's useful" is explicitly called out as "not enough" iirc.) Taking a step back while speaking of resource paks, I guess they won't support this new spelling – how are people supposed to know where this works? In any case this should probably have a slightly different name as function overloading should only be used for things that do effectively the same, and this new function behaves fairly different from the existing ones with the same name.
On 2015/07/07 23:36:27, Nico wrote: > On 2015/07/07 23:08:47, Dan Beam wrote: > > On 2015/07/07 23:08:11, Nico wrote: > > > does this have to be in base? (see text in base/OWNERS) > > > > https://codereview.chromium.org/1220793010/#msg4 > > I would say "no", but I guess I'd say that for the existing $1 substitutions > too. Maybe they're there because grit uses them. …wow, this code has been in > base since initial.commit. The history before that is murky. ("Seems like it's > useful" is explicitly called out as "not enough" iirc.) Would it make sense to have the ${tag} and $1 substitution code at the same code layer? That may be a different layer than base. They seem like two flavors of a similar functionality. I'd kind of like to put the ${tag} code in base or look at moving the $1 code up to whatever layer the ${tag} code moves to. (maybe not moving the $1 code right away, but may we have them at the same layer as a long term goal). Thakis, can you help point me to a layer that would be better suited for the ${tag} code, if not base? > > Taking a step back while speaking of resource paks, I guess they won't support > this new spelling – how are people supposed to know where this works? I need some help with the context - I don't follow... > > In any case this should probably have a slightly different name as function > overloading should only be used for things that do effectively the same, and > this new function behaves fairly different from the existing ones with the same > name. Cool. I wondered about that. I've renamed it to ReplaceTemplateExpressions.
On 2015/07/08 01:24:50, dschuyler wrote: > On 2015/07/07 23:36:27, Nico wrote: > > On 2015/07/07 23:08:47, Dan Beam wrote: > > > On 2015/07/07 23:08:11, Nico wrote: > > > > does this have to be in base? (see text in base/OWNERS) > > > > > > https://codereview.chromium.org/1220793010/#msg4 > > > > I would say "no", but I guess I'd say that for the existing $1 substitutions > > too. Maybe they're there because grit uses them. …wow, this code has been in > > base since initial.commit. The history before that is murky. ("Seems like it's > > useful" is explicitly called out as "not enough" iirc.) > > Would it make sense to have the ${tag} and $1 substitution code at the same code > layer? That may be a different layer than base. They seem like two flavors of > a similar functionality. I'd kind of like to put the ${tag} code in base or > look at moving the $1 code up to whatever layer the ${tag} code moves to. > (maybe not moving the $1 code right away, but may we have them at the same layer > as a long term goal). > > Thakis, can you help point me to a layer that would be better suited for the > ${tag} code, if not base? > > > > > Taking a step back while speaking of resource paks, I guess they won't support > > this new spelling – how are people supposed to know where this works? > > I need some help with the context - I don't follow... The $1 style placeholders work in grd files (e.g. https://code.google.com/p/chromium/codesearch#chromium/src/ash/ash_strings.gr...), in css files, in general in most resources. This new thing won't work everywhere from what I understand. How are people supposed to know which one to use? Currently it's easy since there's only one system. > > In any case this should probably have a slightly different name as function > > overloading should only be used for things that do effectively the same, and > > this new function behaves fairly different from the existing ones with the > same > > name. > > Cool. I wondered about that. I've renamed it to ReplaceTemplateExpressions.
On 2015/07/08 03:05:21, Nico wrote: > On 2015/07/08 01:24:50, dschuyler wrote: > > On 2015/07/07 23:36:27, Nico wrote: > > > On 2015/07/07 23:08:47, Dan Beam wrote: > > > > On 2015/07/07 23:08:11, Nico wrote: > > > > > does this have to be in base? (see text in base/OWNERS) > > > > > > > > https://codereview.chromium.org/1220793010/#msg4 > > > > > > I would say "no", but I guess I'd say that for the existing $1 substitutions > > > too. Maybe they're there because grit uses them. …wow, this code has been in > > > base since initial.commit. The history before that is murky. ("Seems like > it's > > > useful" is explicitly called out as "not enough" iirc.) > > > > Would it make sense to have the ${tag} and $1 substitution code at the same > code > > layer? That may be a different layer than base. They seem like two flavors > of > > a similar functionality. I'd kind of like to put the ${tag} code in base or > > look at moving the $1 code up to whatever layer the ${tag} code moves to. > > (maybe not moving the $1 code right away, but may we have them at the same > layer > > as a long term goal). > > > > Thakis, can you help point me to a layer that would be better suited for the > > ${tag} code, if not base? > > > > > > > > Taking a step back while speaking of resource paks, I guess they won't > support > > > this new spelling – how are people supposed to know where this works? > > > > I need some help with the context - I don't follow... > > The $1 style placeholders work in grd files (e.g. > https://code.google.com/p/chromium/codesearch#chromium/src/ash/ash_strings.gr...), > in css files, in general in most resources. This new thing won't work everywhere > from what I understand. How are people supposed to know which one to use? > Currently it's easy since there's only one system. FWIW: I think the named replacements are a better overall system and would recommend switching to that if it was sane (e.g. didn't have to replace it all over .grd files and teach translators, etc.). $## gets pretty hard to manager pretty quickly, IMO, and ${named} params are more descriptive/harder to get wrong, IMO. > > > > In any case this should probably have a slightly different name as function > > > overloading should only be used for things that do effectively the same, and > > > this new function behaves fairly different from the existing ones with the > > same > > > name. > > > > Cool. I wondered about that. I've renamed it to ReplaceTemplateExpressions.
in talking more with dschuyler@ about this, it seems that $# is wat better for 1-3 translations in .grd files and this probably only matters for 4+ translations, generally more in complex templates. because complex templates are generally used by UI, it's probably OK to put this in ui/base.
This new patch moves the replacement code out of base and into ui/base.
this is looking ~pretty cool~ https://codereview.chromium.org/1220793010/diff/180001/chrome/browser/resourc... File chrome/browser/resources/ntp4/new_incognito_tab_theme.css (right): https://codereview.chromium.org/1220793010/diff/180001/chrome/browser/resourc... chrome/browser/resources/ntp4/new_incognito_tab_theme.css:17: } don't mean to cramp your style but... can you remove? :P https://codereview.chromium.org/1220793010/diff/180001/chrome/browser/ui/webu... File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right): https://codereview.chromium.org/1220793010/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:604: new_tab_theme_css.length()); if we could do less *potential* copies, that'd be better https://codereview.chromium.org/1220793010/diff/180001/chrome/browser/web_dev... File chrome/browser/web_dev_style/css_checker.py (right): https://codereview.chromium.org/1220793010/diff/180001/chrome/browser/web_dev... chrome/browser/web_dev_style/css_checker.py:33: _remove_replacement_tags(s)))) maybe it's time to do: tmp = s tmp = _remove_grit(tmp) tmp = _remove_ats(tmp) tmp = _remove_comments(tmp) tmp = _remove_template_expressions(tmp) return tmp https://codereview.chromium.org/1220793010/diff/180001/chrome/browser/web_dev... chrome/browser/web_dev_style/css_checker.py:47: def _remove_replacement_tags(s): nit: _remove_template_expressions https://codereview.chromium.org/1220793010/diff/180001/chrome/browser/web_dev... chrome/browser/web_dev_style/css_checker.py:48: return re.sub(re.compile(r'\${[^}]*}', re.DOTALL), '', s) can haz test in chrome/browser/test_presubmit.py? https://codereview.chromium.org/1220793010/diff/180001/ui/base/template_expre... File ui/base/template_expressions.cc (right): https://codereview.chromium.org/1220793010/diff/180001/ui/base/template_expre... ui/base/template_expressions.cc:36: if (replacement != substitutions.end()) { hmmmm, this should probably crash or something, IMO https://codereview.chromium.org/1220793010/diff/180001/ui/base/template_expre... ui/base/template_expressions.cc:38: } nit: no curlies https://codereview.chromium.org/1220793010/diff/180001/ui/base/template_expre... File ui/base/template_expressions.h (right): https://codereview.chromium.org/1220793010/diff/180001/ui/base/template_expre... ui/base/template_expressions.h:23: const std::map<std::string, std::string>& substitutions); nit: \n https://codereview.chromium.org/1220793010/diff/180001/ui/base/template_expre... ui/base/template_expressions.h:24: } nit: } // namespace ui
Pretty cool. ui/base seems like a nice place. Please document clearly in the CL description where this works and where it doesn't. A few minor comments below. https://codereview.chromium.org/1220793010/diff/200001/ui/base/template_expre... File ui/base/template_expressions.cc (right): https://codereview.chromium.org/1220793010/diff/200001/ui/base/template_expre... ui/base/template_expressions.cc:11: template <class FormatStringType, class OutStringType> This is only used with one string type, no need for the template https://codereview.chromium.org/1220793010/diff/200001/ui/base/template_expre... ui/base/template_expressions.cc:27: while (i < format_string.end()) { nit: I feel this can probably be simplified by using stuff like strchr, or string.find('}') + substr There would be way fewer allocations if you used StringPiece instead of string for index (and I suppose as map keys, but even if you keep the key type you can just .as_string() at lookup time) https://codereview.chromium.org/1220793010/diff/200001/ui/base/template_expre... ui/base/template_expressions.cc:38: } should it be an error if you say ${foo} and foo doesn't exist? Silently replacing ${foo} with nothing is probably never the right thing? https://codereview.chromium.org/1220793010/diff/200001/ui/base/template_expre... File ui/base/template_expressions_unittest.cc (right): https://codereview.chromium.org/1220793010/diff/200001/ui/base/template_expre... ui/base/template_expressions_unittest.cc:25: "spaces}e,${}f,${&}g,${$}h,${$$}i", Add something nested like "${ab${cd}ef}" (I think that's parsed as one var string "ab${cd" that's looked up and then "ef}" as literal) empty var name ${}
I'm looking into the build failures... I need to work out the gyp and gn settings. https://codereview.chromium.org/1220793010/diff/180001/chrome/browser/resourc... File chrome/browser/resources/ntp4/new_incognito_tab_theme.css (right): https://codereview.chromium.org/1220793010/diff/180001/chrome/browser/resourc... chrome/browser/resources/ntp4/new_incognito_tab_theme.css:17: } On 2015/07/09 19:26:01, Dan Beam wrote: > don't mean to cramp your style but... can you remove? :P Done. https://codereview.chromium.org/1220793010/diff/180001/chrome/browser/web_dev... File chrome/browser/web_dev_style/css_checker.py (right): https://codereview.chromium.org/1220793010/diff/180001/chrome/browser/web_dev... chrome/browser/web_dev_style/css_checker.py:33: _remove_replacement_tags(s)))) On 2015/07/09 19:26:01, Dan Beam wrote: > maybe it's time to do: > > tmp = s > tmp = _remove_grit(tmp) > tmp = _remove_ats(tmp) > tmp = _remove_comments(tmp) > tmp = _remove_template_expressions(tmp) > return tmp Done. https://codereview.chromium.org/1220793010/diff/180001/chrome/browser/web_dev... chrome/browser/web_dev_style/css_checker.py:47: def _remove_replacement_tags(s): On 2015/07/09 19:26:01, Dan Beam wrote: > nit: _remove_template_expressions Done. https://codereview.chromium.org/1220793010/diff/180001/ui/base/template_expre... File ui/base/template_expressions.cc (right): https://codereview.chromium.org/1220793010/diff/180001/ui/base/template_expre... ui/base/template_expressions.cc:36: if (replacement != substitutions.end()) { On 2015/07/09 19:26:01, Dan Beam wrote: > hmmmm, this should probably crash or something, IMO Done. https://codereview.chromium.org/1220793010/diff/180001/ui/base/template_expre... ui/base/template_expressions.cc:38: } On 2015/07/09 19:26:01, Dan Beam wrote: > nit: no curlies Done. https://codereview.chromium.org/1220793010/diff/180001/ui/base/template_expre... File ui/base/template_expressions.h (right): https://codereview.chromium.org/1220793010/diff/180001/ui/base/template_expre... ui/base/template_expressions.h:23: const std::map<std::string, std::string>& substitutions); On 2015/07/09 19:26:01, Dan Beam wrote: > nit: \n Done. https://codereview.chromium.org/1220793010/diff/180001/ui/base/template_expre... ui/base/template_expressions.h:24: } On 2015/07/09 19:26:01, Dan Beam wrote: > nit: } // namespace ui Done. https://codereview.chromium.org/1220793010/diff/200001/ui/base/template_expre... File ui/base/template_expressions.cc (right): https://codereview.chromium.org/1220793010/diff/200001/ui/base/template_expre... ui/base/template_expressions.cc:11: template <class FormatStringType, class OutStringType> On 2015/07/09 19:57:45, Nico wrote: > This is only used with one string type, no need for the template The intention was to have more than one. I've added more than one now. https://codereview.chromium.org/1220793010/diff/200001/ui/base/template_expre... ui/base/template_expressions.cc:27: while (i < format_string.end()) { On 2015/07/09 19:57:45, Nico wrote: > nit: I feel this can probably be simplified by using stuff like strchr, or > string.find('}') + substr > > There would be way fewer allocations if you used StringPiece instead of string > for index (and I suppose as map keys, but even if you keep the key type you can > just .as_string() at lookup time) I've mocked up an alternate version that leans more on the string function members. Which version is preferred? https://codereview.chromium.org/1220793010/diff/200001/ui/base/template_expre... ui/base/template_expressions.cc:38: } On 2015/07/09 19:57:46, Nico wrote: > should it be an error if you say ${foo} and foo doesn't exist? Silently > replacing ${foo} with nothing is probably never the right thing? Done. https://codereview.chromium.org/1220793010/diff/200001/ui/base/template_expre... File ui/base/template_expressions_unittest.cc (right): https://codereview.chromium.org/1220793010/diff/200001/ui/base/template_expre... ui/base/template_expressions_unittest.cc:25: "spaces}e,${}f,${&}g,${$}h,${$$}i", On 2015/07/09 19:57:46, Nico wrote: > Add something nested like "${ab${cd}ef}" (I think that's parsed as one var > string "ab${cd" that's looked up and then "ef}" as literal) > > empty var name ${} Done.
The try bots are all green, so hopefully that means I've got the gyp and gn stuff correct. Two things I'd like advice on in this CL is which replacement loop is preferred and how best to handle unfound keys in the replacement map in a way that works for release builds as well as unit tests (see #if in template_expressions.cc). https://codereview.chromium.org/1220793010/diff/240001/ui/base/template_expre... File ui/base/template_expressions.cc (right): https://codereview.chromium.org/1220793010/diff/240001/ui/base/template_expre... ui/base/template_expressions.cc:19: #if 1 Both the #if section and the #else section work properly. It's a matter of preference about which is more readable, I believe. Which reads nicer to you? https://codereview.chromium.org/1220793010/diff/240001/ui/base/template_expre... ui/base/template_expressions.cc:40: // NOTREACHED() << "Missing template expression " << key; What is a good way to handle this case so that it does the right thing in release builds as well as unit tests? https://codereview.chromium.org/1220793010/diff/240001/ui/base/template_expre... ui/base/template_expressions.cc:68: // NOTREACHED() << "Missing template expression " << index; What is a good way to handle this case so that it does the right thing in release builds as well as unit tests?
https://codereview.chromium.org/1220793010/diff/260001/chrome/browser/resourc... File chrome/browser/resources/ntp4/new_tab_theme.css (right): https://codereview.chromium.org/1220793010/diff/260001/chrome/browser/resourc... chrome/browser/resources/ntp4/new_tab_theme.css:7: background-color: ${colorBackground}; /* COLOR_NTP_BACKGROUND */ are these comments now helpful? they just seem redundant https://codereview.chromium.org/1220793010/diff/260001/chrome/browser/web_dev... File chrome/browser/web_dev_style/css_checker.py (right): https://codereview.chromium.org/1220793010/diff/260001/chrome/browser/web_dev... chrome/browser/web_dev_style/css_checker.py:32: s = _remove_grit(s); this is python, no ; https://codereview.chromium.org/1220793010/diff/260001/ui/base/template_expre... File ui/base/template_expressions.cc (right): https://codereview.chromium.org/1220793010/diff/260001/ui/base/template_expre... ui/base/template_expressions.cc:19: #if 1 I understand you're asking for feedback, but I wouldn't use the code review system like this because you have me on edge thinking you're going to commit this code https://codereview.chromium.org/1220793010/diff/260001/ui/base/template_expre... ui/base/template_expressions.cc:73: // NOTREACHED() << "Missing template expression " << index; I think this means it'd only blow up in debug. do we want that? https://codereview.chromium.org/1220793010/diff/260001/ui/base/template_expre... ui/base/template_expressions.cc:86: } I find this second one more readable, I think Nico just wants better substr detection from std::string (though I don't really know how optimized it is... when it's just 1 char) https://codereview.chromium.org/1220793010/diff/260001/ui/base/template_expre... File ui/base/template_expressions.h (right): https://codereview.chromium.org/1220793010/diff/260001/ui/base/template_expre... ui/base/template_expressions.h:14: #include "base/strings/utf_string_conversions.h" move this to cc, add #include "base/strings/string_piece.h" #include "base/strings/string16.h" instead https://codereview.chromium.org/1220793010/diff/260001/ui/base/template_expre... ui/base/template_expressions.h:30: const std::map<std::string, std::string>& substitutions); remove any versions that aren't used from non-test code https://codereview.chromium.org/1220793010/diff/260001/ui/base/template_expre... File ui/base/template_expressions_unittest.cc (right): https://codereview.chromium.org/1220793010/diff/260001/ui/base/template_expre... ui/base/template_expressions_unittest.cc:13: substitutions[base::ASCIIToUTF16("one")] = base::ASCIIToUTF16("9a"); why are you using the version that requires this annoying base::ASCIIToUTF16() in mah code? https://codereview.chromium.org/1220793010/diff/260001/ui/base/template_expre... ui/base/template_expressions_unittest.cc:13: substitutions[base::ASCIIToUTF16("one")] = base::ASCIIToUTF16("9a"); instead of building a map (which I have to do in my head) and replacing a ton of things in a complex template (which I also have to attempt in my head), just do one thing at a time, IMO std::map<base::string16, base::string16> substitutions; substitutions["one"] = "9a"; EXPECT_STREQ(ReplaceTemplateExpressions("${one}"), "9a"); ... more substitutions (if necessary) + EXPECTs ... https://codereview.chromium.org/1220793010/diff/260001/ui/base/template_expre... ui/base/template_expressions_unittest.cc:24: EXPECT_EQ(ReplaceTemplateExpressions(base::ASCIIToUTF16("${ab${cd}ef}"), EXPECT_STREQ
https://codereview.chromium.org/1220793010/diff/260001/chrome/browser/resourc... File chrome/browser/resources/ntp4/new_tab_theme.css (right): https://codereview.chromium.org/1220793010/diff/260001/chrome/browser/resourc... chrome/browser/resources/ntp4/new_tab_theme.css:7: background-color: ${colorBackground}; /* COLOR_NTP_BACKGROUND */ On 2015/07/14 17:14:35, Dan Beam wrote: > are these comments now helpful? they just seem redundant Done. https://codereview.chromium.org/1220793010/diff/260001/chrome/browser/web_dev... File chrome/browser/web_dev_style/css_checker.py (right): https://codereview.chromium.org/1220793010/diff/260001/chrome/browser/web_dev... chrome/browser/web_dev_style/css_checker.py:32: s = _remove_grit(s); On 2015/07/14 17:14:35, Dan Beam wrote: > this is python, no ; Done. https://codereview.chromium.org/1220793010/diff/260001/ui/base/template_expre... File ui/base/template_expressions.cc (right): https://codereview.chromium.org/1220793010/diff/260001/ui/base/template_expre... ui/base/template_expressions.cc:19: #if 1 On 2015/07/14 17:14:35, Dan Beam wrote: > I understand you're asking for feedback, but I wouldn't use the code review > system like this because you have me on edge thinking you're going to commit > this code Acknowledged. https://codereview.chromium.org/1220793010/diff/260001/ui/base/template_expre... ui/base/template_expressions.cc:73: // NOTREACHED() << "Missing template expression " << index; On 2015/07/14 17:14:35, Dan Beam wrote: > I think this means it'd only blow up in debug. do we want that? Who should we ask? https://codereview.chromium.org/1220793010/diff/260001/ui/base/template_expre... ui/base/template_expressions.cc:86: } On 2015/07/14 17:14:35, Dan Beam wrote: > I find this second one more readable, I think Nico just wants better substr > detection from std::string (though I don't really know how optimized it is... > when it's just 1 char) Acknowledged. https://codereview.chromium.org/1220793010/diff/260001/ui/base/template_expre... File ui/base/template_expressions.h (right): https://codereview.chromium.org/1220793010/diff/260001/ui/base/template_expre... ui/base/template_expressions.h:14: #include "base/strings/utf_string_conversions.h" On 2015/07/14 17:14:35, Dan Beam wrote: > move this to cc, add > > #include "base/strings/string_piece.h" > #include "base/strings/string16.h" > > instead Done. https://codereview.chromium.org/1220793010/diff/260001/ui/base/template_expre... ui/base/template_expressions.h:30: const std::map<std::string, std::string>& substitutions); On 2015/07/14 17:14:35, Dan Beam wrote: > remove any versions that aren't used from non-test code Done. https://codereview.chromium.org/1220793010/diff/260001/ui/base/template_expre... File ui/base/template_expressions_unittest.cc (right): https://codereview.chromium.org/1220793010/diff/260001/ui/base/template_expre... ui/base/template_expressions_unittest.cc:13: substitutions[base::ASCIIToUTF16("one")] = base::ASCIIToUTF16("9a"); On 2015/07/14 17:14:35, Dan Beam wrote: > why are you using the version that requires this annoying base::ASCIIToUTF16() > in mah code? Done. https://codereview.chromium.org/1220793010/diff/260001/ui/base/template_expre... ui/base/template_expressions_unittest.cc:13: substitutions[base::ASCIIToUTF16("one")] = base::ASCIIToUTF16("9a"); On 2015/07/14 17:14:35, Dan Beam wrote: > instead of building a map (which I have to do in my head) and replacing a ton of > things in a complex template (which I also have to attempt in my head), just do > one thing at a time, IMO > > std::map<base::string16, base::string16> substitutions; > substitutions["one"] = "9a"; > EXPECT_STREQ(ReplaceTemplateExpressions("${one}"), "9a"); > > ... more substitutions (if necessary) + EXPECTs ... I'll work on this. https://codereview.chromium.org/1220793010/diff/260001/ui/base/template_expre... ui/base/template_expressions_unittest.cc:24: EXPECT_EQ(ReplaceTemplateExpressions(base::ASCIIToUTF16("${ab${cd}ef}"), On 2015/07/14 17:14:35, Dan Beam wrote: > EXPECT_STREQ Done.
looking pretty good, need to remove one of those methods, though https://codereview.chromium.org/1220793010/diff/280001/chrome/browser/resourc... File chrome/browser/resources/ntp4/new_tab_theme.css (left): https://codereview.chromium.org/1220793010/diff/280001/chrome/browser/resourc... chrome/browser/resources/ntp4/new_tab_theme.css:37: color: rgba($23, 0.5); /* COLOR_NTP_TEXT */ same NTP_* for different numbers... lame https://codereview.chromium.org/1220793010/diff/280001/chrome/browser/test_pr... File chrome/browser/test_presubmit.py (right): https://codereview.chromium.org/1220793010/diff/280001/chrome/browser/test_pr... chrome/browser/test_presubmit.py:59: self.ShouldPassCheck(line, self.checker.ClassesUseDashFormCheck) why are you running this specific check? https://codereview.chromium.org/1220793010/diff/280001/chrome/browser/ui/webu... File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right): https://codereview.chromium.org/1220793010/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:645: substitutions["colorTextCall"] = SkColorToRGBAString(color_text); what does Call mean here? https://codereview.chromium.org/1220793010/diff/280001/ui/base/template_expre... File ui/base/template_expressions_unittest.cc (right): https://codereview.chromium.org/1220793010/diff/280001/ui/base/template_expre... ui/base/template_expressions_unittest.cc:27: EXPECT_STREQ(ReplaceTemplateExpressions("", substitutions).c_str(), ""); it seems like you do the same thing pretty often. why not: namespace { void CheckTemplate(const std::map<base::StringPiece, std::string>& data, const char* template, const char* expected) { EXPECT_STREQ(expected, ReplaceTemplateExpressions(template, data).c_str()); } } // namespace ... std::map<base::StringPiece, std::string> substitutions; substitutions[""] = "4f"; CheckTemplate(substitutions, "${}", "4f");
Warning just in debug seems fine to me. https://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREAC... is the reference on this question. I like the first version a bit better, but if you two both prefer the second I can live with that too. https://codereview.chromium.org/1220793010/diff/240001/ui/base/template_expre... File ui/base/template_expressions.cc (right): https://codereview.chromium.org/1220793010/diff/240001/ui/base/template_expre... ui/base/template_expressions.cc:19: #if 1 On 2015/07/14 16:45:36, dschuyler wrote: > Both the #if section and the #else section work properly. It's a matter of > preference about which is more readable, I believe. Which reads nicer to you? I feel the first block is nicer, seems to be at a higher level of abstraction https://codereview.chromium.org/1220793010/diff/240001/ui/base/template_expre... ui/base/template_expressions.cc:40: // NOTREACHED() << "Missing template expression " << key; On 2015/07/14 16:45:36, dschuyler wrote: > What is a good way to handle this case so that it does the right thing in > release builds as well as unit tests? Right Thing in what sense?
https://codereview.chromium.org/1220793010/diff/240001/ui/base/template_expre... File ui/base/template_expressions.cc (right): https://codereview.chromium.org/1220793010/diff/240001/ui/base/template_expre... ui/base/template_expressions.cc:19: #if 1 On 2015/07/14 21:44:18, Nico wrote: > On 2015/07/14 16:45:36, dschuyler wrote: > > Both the #if section and the #else section work properly. It's a matter of > > preference about which is more readable, I believe. Which reads nicer to you? > > I feel the first block is nicer, seems to be at a higher level of abstraction I'm ok with either version. I have a leaning toward the lower version (#else side) because I can see what is happening easier. https://codereview.chromium.org/1220793010/diff/240001/ui/base/template_expre... ui/base/template_expressions.cc:40: // NOTREACHED() << "Missing template expression " << key; On 2015/07/14 21:44:18, Nico wrote: > On 2015/07/14 16:45:36, dschuyler wrote: > > What is a good way to handle this case so that it does the right thing in > > release builds as well as unit tests? > > Right Thing in what sense? Some of the unit test try things that will fail to test those cases. Right now, if we fail out when there is something wrong in the replacement, the unit test fails (even though I'm trying to test a failure case). I see value in testing both success and failure cases in unit tests, so I'd like to do so. If my opinion is wrong :) and I should only test successes, that would also answer this. https://codereview.chromium.org/1220793010/diff/280001/chrome/browser/resourc... File chrome/browser/resources/ntp4/new_tab_theme.css (left): https://codereview.chromium.org/1220793010/diff/280001/chrome/browser/resourc... chrome/browser/resources/ntp4/new_tab_theme.css:37: color: rgba($23, 0.5); /* COLOR_NTP_TEXT */ On 2015/07/14 21:23:40, Dan Beam wrote: > same NTP_* for different numbers... lame I think the original author may have been saying that it's the same color value formatted in different ways. https://codereview.chromium.org/1220793010/diff/280001/chrome/browser/ui/webu... File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right): https://codereview.chromium.org/1220793010/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:645: substitutions["colorTextCall"] = SkColorToRGBAString(color_text); On 2015/07/14 21:23:40, Dan Beam wrote: > what does Call mean here? Function Call. It's formatted like foo(bar, blah). I could have gone with Function or Func or something. https://codereview.chromium.org/1220793010/diff/280001/ui/base/template_expre... File ui/base/template_expressions_unittest.cc (right): https://codereview.chromium.org/1220793010/diff/280001/ui/base/template_expre... ui/base/template_expressions_unittest.cc:27: EXPECT_STREQ(ReplaceTemplateExpressions("", substitutions).c_str(), ""); On 2015/07/14 21:23:40, Dan Beam wrote: > it seems like you do the same thing pretty often. why not: > > namespace { > > void CheckTemplate(const std::map<base::StringPiece, std::string>& data, > const char* template, const char* expected) { > EXPECT_STREQ(expected, ReplaceTemplateExpressions(template, data).c_str()); > } > > } // namespace > > ... > > std::map<base::StringPiece, std::string> substitutions; > substitutions[""] = "4f"; > CheckTemplate(substitutions, "${}", "4f"); On why not, well it kinda feels like adding more code. Why not go back to EXPECT_EQ and let the base string do the string compare? :) I feel like I may be misunderstanding something. Why is converting it to a char* better than doing the operator== compare?
https://codereview.chromium.org/1220793010/diff/280001/chrome/browser/ui/webu... File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right): https://codereview.chromium.org/1220793010/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:645: substitutions["colorTextCall"] = SkColorToRGBAString(color_text); On 2015/07/15 00:11:04, dschuyler wrote: > On 2015/07/14 21:23:40, Dan Beam wrote: > > what does Call mean here? > > Function Call. It's formatted like foo(bar, blah). I could have gone with > Function or Func or something. change to colorTextRgba for my sanity https://codereview.chromium.org/1220793010/diff/280001/ui/base/template_expre... File ui/base/template_expressions_unittest.cc (right): https://codereview.chromium.org/1220793010/diff/280001/ui/base/template_expre... ui/base/template_expressions_unittest.cc:27: EXPECT_STREQ(ReplaceTemplateExpressions("", substitutions).c_str(), ""); On 2015/07/15 00:11:04, dschuyler wrote: > On 2015/07/14 21:23:40, Dan Beam wrote: > > it seems like you do the same thing pretty often. why not: > > > > namespace { > > > > void CheckTemplate(const std::map<base::StringPiece, std::string>& data, > > const char* template, const char* expected) { > > EXPECT_STREQ(expected, ReplaceTemplateExpressions(template, > data).c_str()); > > } > > > > } // namespace > > > > ... > > > > std::map<base::StringPiece, std::string> substitutions; > > substitutions[""] = "4f"; > > CheckTemplate(substitutions, "${}", "4f"); > > On why not, well it kinda feels like adding more code. because you have to add a helper method? > Why not go back to > EXPECT_EQ and let the base string do the string compare? :) I feel like I may > be misunderstanding something. Why is converting it to a char* better than > doing the operator== compare? are you asking why EXPECT_STREQ is better? because the answer to that is "it shows a better diff".
On 2015/07/15 16:36:25, Dan Beam wrote: > https://codereview.chromium.org/1220793010/diff/280001/chrome/browser/ui/webu... > File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right): > > https://codereview.chromium.org/1220793010/diff/280001/chrome/browser/ui/webu... > chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:645: > substitutions["colorTextCall"] = SkColorToRGBAString(color_text); > On 2015/07/15 00:11:04, dschuyler wrote: > > On 2015/07/14 21:23:40, Dan Beam wrote: > > > what does Call mean here? > > > > Function Call. It's formatted like foo(bar, blah). I could have gone with > > Function or Func or something. > > change to colorTextRgba for my sanity > > https://codereview.chromium.org/1220793010/diff/280001/ui/base/template_expre... > File ui/base/template_expressions_unittest.cc (right): > > https://codereview.chromium.org/1220793010/diff/280001/ui/base/template_expre... > ui/base/template_expressions_unittest.cc:27: > EXPECT_STREQ(ReplaceTemplateExpressions("", substitutions).c_str(), ""); > On 2015/07/15 00:11:04, dschuyler wrote: > > On 2015/07/14 21:23:40, Dan Beam wrote: > > > it seems like you do the same thing pretty often. why not: > > > > > > namespace { > > > > > > void CheckTemplate(const std::map<base::StringPiece, std::string>& data, > > > const char* template, const char* expected) { > > > EXPECT_STREQ(expected, ReplaceTemplateExpressions(template, > > data).c_str()); > > > } > > > > > > } // namespace > > > > > > ... > > > > > > std::map<base::StringPiece, std::string> substitutions; > > > substitutions[""] = "4f"; > > > CheckTemplate(substitutions, "${}", "4f"); > > > > On why not, well it kinda feels like adding more code. > > because you have to add a helper method? > > > Why not go back to > > EXPECT_EQ and let the base string do the string compare? :) I feel like I > may > > be misunderstanding something. Why is converting it to a char* better than > > doing the operator== compare? > > are you asking why EXPECT_STREQ is better? because the answer to that is "it > shows a better diff". I think that's where I'm concerned. The diffs look very similar to me. Here's an example with streq and with eq. [ RUN ] TemplateExpressionsTest.ReplaceTemplateExpressionsPieces ../../ui/base/template_expressions_unittest.cc:25: Failure Value of: "ef}e" Expected: ReplaceTemplateExpressions("${ab${cd}ef}", substitutions).c_str() Which is: "ef}" [ FAILED ] TemplateExpressionsTest.ReplaceTemplateExpressionsPieces (0 ms) [ RUN ] TemplateExpressionsTest.ReplaceTemplateExpressionsPieces ../../ui/base/template_expressions_unittest.cc:25: Failure Value of: "ef}e" Expected: ReplaceTemplateExpressions("${ab${cd}ef}", substitutions) Which is: "ef}" [ FAILED ] TemplateExpressionsTest.ReplaceTemplateExpressionsPieces (0 ms)
On 2015/07/15 16:55:47, dschuyler wrote: > On 2015/07/15 16:36:25, Dan Beam wrote: > > > https://codereview.chromium.org/1220793010/diff/280001/chrome/browser/ui/webu... > > File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right): > > > > > https://codereview.chromium.org/1220793010/diff/280001/chrome/browser/ui/webu... > > chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:645: > > substitutions["colorTextCall"] = SkColorToRGBAString(color_text); > > On 2015/07/15 00:11:04, dschuyler wrote: > > > On 2015/07/14 21:23:40, Dan Beam wrote: > > > > what does Call mean here? > > > > > > Function Call. It's formatted like foo(bar, blah). I could have gone with > > > Function or Func or something. > > > > change to colorTextRgba for my sanity > > > > > https://codereview.chromium.org/1220793010/diff/280001/ui/base/template_expre... > > File ui/base/template_expressions_unittest.cc (right): > > > > > https://codereview.chromium.org/1220793010/diff/280001/ui/base/template_expre... > > ui/base/template_expressions_unittest.cc:27: > > EXPECT_STREQ(ReplaceTemplateExpressions("", substitutions).c_str(), ""); > > On 2015/07/15 00:11:04, dschuyler wrote: > > > On 2015/07/14 21:23:40, Dan Beam wrote: > > > > it seems like you do the same thing pretty often. why not: > > > > > > > > namespace { > > > > > > > > void CheckTemplate(const std::map<base::StringPiece, std::string>& data, > > > > const char* template, const char* expected) { > > > > EXPECT_STREQ(expected, ReplaceTemplateExpressions(template, > > > data).c_str()); > > > > } > > > > > > > > } // namespace > > > > > > > > ... > > > > > > > > std::map<base::StringPiece, std::string> substitutions; > > > > substitutions[""] = "4f"; > > > > CheckTemplate(substitutions, "${}", "4f"); > > > > > > On why not, well it kinda feels like adding more code. > > > > because you have to add a helper method? > > > > > Why not go back to > > > EXPECT_EQ and let the base string do the string compare? :) I feel like I > > may > > > be misunderstanding something. Why is converting it to a char* better than > > > doing the operator== compare? > > > > are you asking why EXPECT_STREQ is better? because the answer to that is "it > > shows a better diff". > > I think that's where I'm concerned. The diffs look very similar to me. Here's > an example with streq and with eq. > > [ RUN ] TemplateExpressionsTest.ReplaceTemplateExpressionsPieces > ../../ui/base/template_expressions_unittest.cc:25: Failure > Value of: "ef}e" > Expected: ReplaceTemplateExpressions("${ab${cd}ef}", substitutions).c_str() > Which is: "ef}" > [ FAILED ] TemplateExpressionsTest.ReplaceTemplateExpressionsPieces (0 ms) > > > > [ RUN ] TemplateExpressionsTest.ReplaceTemplateExpressionsPieces > ../../ui/base/template_expressions_unittest.cc:25: Failure > Value of: "ef}e" > Expected: ReplaceTemplateExpressions("${ab${cd}ef}", substitutions) > Which is: "ef}" > [ FAILED ] TemplateExpressionsTest.ReplaceTemplateExpressionsPieces (0 ms) yes, I was wrong. it just handles NULLs[1]. I was thinking of assertMultilineEquals in python[2]. in that case: use whatever's simpler to read (if there has to be magic char* -> std::string() or something that's fine as well) [1] https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/src/... [2] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/closur...
https://codereview.chromium.org/1220793010/diff/260001/ui/base/template_expre... File ui/base/template_expressions_unittest.cc (right): https://codereview.chromium.org/1220793010/diff/260001/ui/base/template_expre... ui/base/template_expressions_unittest.cc:13: substitutions[base::ASCIIToUTF16("one")] = base::ASCIIToUTF16("9a"); On 2015/07/14 17:14:35, Dan Beam wrote: > instead of building a map (which I have to do in my head) and replacing a ton of > things in a complex template (which I also have to attempt in my head), just do > one thing at a time, IMO > > std::map<base::string16, base::string16> substitutions; > substitutions["one"] = "9a"; > EXPECT_STREQ(ReplaceTemplateExpressions("${one}"), "9a"); > > ... more substitutions (if necessary) + EXPECTs ... How about three entries? I've changed the keys/values to be more clear. And I've separated the tests more. Done. https://codereview.chromium.org/1220793010/diff/280001/chrome/browser/ui/webu... File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right): https://codereview.chromium.org/1220793010/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:645: substitutions["colorTextCall"] = SkColorToRGBAString(color_text); On 2015/07/15 16:36:25, Dan Beam wrote: > On 2015/07/15 00:11:04, dschuyler wrote: > > On 2015/07/14 21:23:40, Dan Beam wrote: > > > what does Call mean here? > > > > Function Call. It's formatted like foo(bar, blah). I could have gone with > > Function or Func or something. > > change to colorTextRgba for my sanity Done. https://codereview.chromium.org/1220793010/diff/280001/ui/base/template_expre... File ui/base/template_expressions_unittest.cc (right): https://codereview.chromium.org/1220793010/diff/280001/ui/base/template_expre... ui/base/template_expressions_unittest.cc:27: EXPECT_STREQ(ReplaceTemplateExpressions("", substitutions).c_str(), ""); On 2015/07/15 16:36:25, Dan Beam wrote: > On 2015/07/15 00:11:04, dschuyler wrote: > > On 2015/07/14 21:23:40, Dan Beam wrote: > > > it seems like you do the same thing pretty often. why not: > > > > > > namespace { > > > > > > void CheckTemplate(const std::map<base::StringPiece, std::string>& data, > > > const char* template, const char* expected) { > > > EXPECT_STREQ(expected, ReplaceTemplateExpressions(template, > > data).c_str()); > > > } > > > > > > } // namespace > > > > > > ... > > > > > > std::map<base::StringPiece, std::string> substitutions; > > > substitutions[""] = "4f"; > > > CheckTemplate(substitutions, "${}", "4f"); > > > > On why not, well it kinda feels like adding more code. > > because you have to add a helper method? > > > Why not go back to > > EXPECT_EQ and let the base string do the string compare? :) I feel like I > may > > be misunderstanding something. Why is converting it to a char* better than > > doing the operator== compare? > > are you asking why EXPECT_STREQ is better? because the answer to that is "it > shows a better diff". Based on the new info that the EXPECT_EQ works well, I've switched back to that. Done.
https://codereview.chromium.org/1220793010/diff/280001/chrome/browser/test_pr... File chrome/browser/test_presubmit.py (right): https://codereview.chromium.org/1220793010/diff/280001/chrome/browser/test_pr... chrome/browser/test_presubmit.py:59: self.ShouldPassCheck(line, self.checker.ClassesUseDashFormCheck) On 2015/07/14 21:23:40, Dan Beam wrote: > why are you running this specific check? ping https://codereview.chromium.org/1220793010/diff/360001/ui/base/template_expre... File ui/base/template_expressions_unittest.cc (right): https://codereview.chromium.org/1220793010/diff/360001/ui/base/template_expre... ui/base/template_expressions_unittest.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. nit: 2015 https://codereview.chromium.org/1220793010/diff/360001/ui/base/template_expre... ui/base/template_expressions_unittest.cc:33: substitutions["c"] = "7c"; nit: 9a 8b 7c -> x y z https://codereview.chromium.org/1220793010/diff/360001/ui/base/template_expre... ui/base/template_expressions_unittest.cc:35: ReplaceTemplateExpressions("$${a} $$${b} $$$${c} $$", substitutions), nit: $$ first https://codereview.chromium.org/1220793010/diff/360001/ui/base/template_expre... ui/base/template_expressions_unittest.cc:38: EXPECT_EQ(ReplaceTemplateExpressions("$$", substitutions), "$"); duplicative
https://codereview.chromium.org/1220793010/diff/200001/ui/base/template_expre... File ui/base/template_expressions.cc (right): https://codereview.chromium.org/1220793010/diff/200001/ui/base/template_expre... ui/base/template_expressions.cc:27: while (i < format_string.end()) { On 2015/07/09 19:57:45, Nico wrote: > nit: I feel this can probably be simplified by using stuff like strchr, or > string.find('}') + substr > > There would be way fewer allocations if you used StringPiece instead of string > for index (and I suppose as map keys, but even if you keep the key type you can > just .as_string() at lookup time) Since I went back to the other version of the loop, I revisited these suggestions. The index/key is now a StringPiece that uses find() to locate the '}'. https://codereview.chromium.org/1220793010/diff/360001/ui/base/template_expre... File ui/base/template_expressions_unittest.cc (right): https://codereview.chromium.org/1220793010/diff/360001/ui/base/template_expre... ui/base/template_expressions_unittest.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2015/07/15 18:56:41, Dan Beam wrote: > nit: 2015 Done. https://codereview.chromium.org/1220793010/diff/360001/ui/base/template_expre... ui/base/template_expressions_unittest.cc:33: substitutions["c"] = "7c"; On 2015/07/15 18:56:41, Dan Beam wrote: > nit: 9a 8b 7c -> x y z Done. https://codereview.chromium.org/1220793010/diff/360001/ui/base/template_expre... ui/base/template_expressions_unittest.cc:35: ReplaceTemplateExpressions("$${a} $$${b} $$$${c} $$", substitutions), On 2015/07/15 18:56:41, Dan Beam wrote: > nit: $$ first Based on our conversation, this is changing if we're removing the escape option. https://codereview.chromium.org/1220793010/diff/360001/ui/base/template_expre... ui/base/template_expressions_unittest.cc:38: EXPECT_EQ(ReplaceTemplateExpressions("$$", substitutions), "$"); On 2015/07/15 18:56:41, Dan Beam wrote: > duplicative Done.
https://codereview.chromium.org/1220793010/diff/400001/ui/base/template_expre... File ui/base/template_expressions.cc (right): https://codereview.chromium.org/1220793010/diff/400001/ui/base/template_expre... ui/base/template_expressions.cc:20: while (i < format_string.end()) { why not format_string.find("${")?
https://codereview.chromium.org/1220793010/diff/400001/ui/base/template_expre... File ui/base/template_expressions.cc (right): https://codereview.chromium.org/1220793010/diff/400001/ui/base/template_expre... ui/base/template_expressions.cc:20: while (i < format_string.end()) { On 2015/07/15 21:56:09, Dan Beam wrote: > why not format_string.find("${")? imo, it makes the code more complicated that it is currently.
https://codereview.chromium.org/1220793010/diff/400001/ui/base/template_expre... File ui/base/template_expressions.cc (right): https://codereview.chromium.org/1220793010/diff/400001/ui/base/template_expre... ui/base/template_expressions.cc:8: #include "base/strings/utf_string_conversions.h" ^ needed? https://codereview.chromium.org/1220793010/diff/400001/ui/base/template_expre... ui/base/template_expressions.cc:22: size_t offset = i + 2 - format_string.begin(); template_start or start https://codereview.chromium.org/1220793010/diff/400001/ui/base/template_expre... ui/base/template_expressions.cc:23: size_t length = format_string.find('}', offset) - offset; expression_key_size or key_size https://codereview.chromium.org/1220793010/diff/400001/ui/base/template_expre... ui/base/template_expressions.cc:30: } where did we land on } else { NOTREACHED(); } ? https://codereview.chromium.org/1220793010/diff/400001/ui/base/template_expre... File ui/base/template_expressions.h (right): https://codereview.chromium.org/1220793010/diff/400001/ui/base/template_expre... ui/base/template_expressions.h:14: #include "base/strings/string16.h" unneeded https://codereview.chromium.org/1220793010/diff/400001/ui/base/template_expre... File ui/base/template_expressions_unittest.cc (right): https://codereview.chromium.org/1220793010/diff/400001/ui/base/template_expre... ui/base/template_expressions_unittest.cc:7: #include "base/strings/utf_string_conversions.h" ^ needed?
https://codereview.chromium.org/1220793010/diff/400001/ui/base/template_expre... File ui/base/template_expressions.cc (right): https://codereview.chromium.org/1220793010/diff/400001/ui/base/template_expre... ui/base/template_expressions.cc:8: #include "base/strings/utf_string_conversions.h" On 2015/07/16 00:21:44, Dan Beam wrote: > ^ needed? Nope, thanks. Done. https://codereview.chromium.org/1220793010/diff/400001/ui/base/template_expre... ui/base/template_expressions.cc:22: size_t offset = i + 2 - format_string.begin(); On 2015/07/16 00:21:44, Dan Beam wrote: > template_start or start Done. https://codereview.chromium.org/1220793010/diff/400001/ui/base/template_expre... ui/base/template_expressions.cc:23: size_t length = format_string.find('}', offset) - offset; On 2015/07/16 00:21:44, Dan Beam wrote: > expression_key_size or key_size Acknowledged. https://codereview.chromium.org/1220793010/diff/400001/ui/base/template_expre... ui/base/template_expressions.cc:30: } On 2015/07/16 00:21:44, Dan Beam wrote: > where did we land on > > } else { > NOTREACHED(); > } > > ? The current code will leave ${unfound} in the result document. We can still put in an "else crash" easily enough. That will break the current unit tests though.
https://codereview.chromium.org/1220793010/diff/280001/chrome/browser/test_pr... File chrome/browser/test_presubmit.py (right): https://codereview.chromium.org/1220793010/diff/280001/chrome/browser/test_pr... chrome/browser/test_presubmit.py:59: self.ShouldPassCheck(line, self.checker.ClassesUseDashFormCheck) On 2015/07/15 18:56:41, Dan Beam wrote: > On 2015/07/14 21:23:40, Dan Beam wrote: > > why are you running this specific check? > > ping The code is removed now. https://codereview.chromium.org/1220793010/diff/400001/ui/base/template_expre... File ui/base/template_expressions.cc (right): https://codereview.chromium.org/1220793010/diff/400001/ui/base/template_expre... ui/base/template_expressions.cc:30: } On 2015/07/16 00:33:18, dschuyler wrote: > On 2015/07/16 00:21:44, Dan Beam wrote: > > where did we land on > > > > } else { > > NOTREACHED(); > > } > > > > ? > > The current code will leave ${unfound} in the result document. > > We can still put in an "else crash" easily enough. That will break the current > unit tests though. I've added a NOTREACHED for unfound keys.
lgtm once dan is happy (and with bugs below fixed) https://codereview.chromium.org/1220793010/diff/350014/ui/base/template_expre... File ui/base/template_expressions.cc (right): https://codereview.chromium.org/1220793010/diff/350014/ui/base/template_expre... ui/base/template_expressions.cc:20: if (*i == '$' && i < format_string.end() && i[1] == '{') { you want `i + 1 < format_string.end()` here I think (if so, add test coverage for this case) https://codereview.chromium.org/1220793010/diff/350014/ui/base/template_expre... ui/base/template_expressions.cc:21: size_t key_start = i + 2 - format_string.begin(); s/2/strlen("${") https://codereview.chromium.org/1220793010/diff/350014/ui/base/template_expre... ui/base/template_expressions.cc:22: size_t key_length = format_string.find('}', key_start) - key_start; find() returns npos on not found, which becomes a very large size_t. Check that '}' was actually found (and add test coverage for this) https://codereview.chromium.org/1220793010/diff/350014/ui/base/template_expre... ui/base/template_expressions.cc:27: i += key_length + 3; s/3/strlen("${}")
Oh, and as said somewhere above, please update the cl description to clearly say where this works.
https://codereview.chromium.org/1220793010/diff/350014/ui/base/template_expre... File ui/base/template_expressions.cc (right): https://codereview.chromium.org/1220793010/diff/350014/ui/base/template_expre... ui/base/template_expressions.cc:20: if (*i == '$' && i < format_string.end() && i[1] == '{') { On 2015/07/16 20:34:00, Nico wrote: > you want `i + 1 < format_string.end()` here I think (if so, add test coverage > for this case) There already is a test case that would go past the end, but it does not cause issue because it's still valid memory to read from. This would be an issue if the last character before an inaccessible page of memory was a '$'. Done. https://codereview.chromium.org/1220793010/diff/350014/ui/base/template_expre... ui/base/template_expressions.cc:21: size_t key_start = i + 2 - format_string.begin(); On 2015/07/16 20:34:00, Nico wrote: > s/2/strlen("${") Done. https://codereview.chromium.org/1220793010/diff/350014/ui/base/template_expre... ui/base/template_expressions.cc:22: size_t key_length = format_string.find('}', key_start) - key_start; On 2015/07/16 20:34:00, Nico wrote: > find() returns npos on not found, which becomes a very large size_t. Check that > '}' was actually found (and add test coverage for this) Done. https://codereview.chromium.org/1220793010/diff/350014/ui/base/template_expre... ui/base/template_expressions.cc:27: i += key_length + 3; On 2015/07/16 20:34:00, Nico wrote: > s/3/strlen("${}") Done.
let the tools work for you: http://i.imgur.com/Z9XY4MD.png https://codereview.chromium.org/1220793010/diff/400001/ui/base/template_expre... File ui/base/template_expressions.h (right): https://codereview.chromium.org/1220793010/diff/400001/ui/base/template_expre... ui/base/template_expressions.h:14: #include "base/strings/string16.h" On 2015/07/16 00:21:44, Dan Beam wrote: > unneeded ping https://codereview.chromium.org/1220793010/diff/400001/ui/base/template_expre... File ui/base/template_expressions_unittest.cc (right): https://codereview.chromium.org/1220793010/diff/400001/ui/base/template_expre... ui/base/template_expressions_unittest.cc:7: #include "base/strings/utf_string_conversions.h" On 2015/07/16 00:21:44, Dan Beam wrote: > ^ needed? ping https://codereview.chromium.org/1220793010/diff/470001/ui/base/template_expre... File ui/base/template_expressions.cc (right): https://codereview.chromium.org/1220793010/diff/470001/ui/base/template_expre... ui/base/template_expressions.cc:25: } nit: no curlies https://codereview.chromium.org/1220793010/diff/470001/ui/base/template_expre... ui/base/template_expressions.cc:31: i += key_length + strlen("${}"); nit: i += strlen("${" + key_length + strlen("}")
https://codereview.chromium.org/1220793010/diff/400001/ui/base/template_expre... File ui/base/template_expressions.h (right): https://codereview.chromium.org/1220793010/diff/400001/ui/base/template_expre... ui/base/template_expressions.h:14: #include "base/strings/string16.h" On 2015/07/17 01:57:18, Dan Beam wrote: > On 2015/07/16 00:21:44, Dan Beam wrote: > > unneeded > > ping Done. https://codereview.chromium.org/1220793010/diff/400001/ui/base/template_expre... File ui/base/template_expressions_unittest.cc (right): https://codereview.chromium.org/1220793010/diff/400001/ui/base/template_expre... ui/base/template_expressions_unittest.cc:7: #include "base/strings/utf_string_conversions.h" On 2015/07/17 01:57:18, Dan Beam wrote: > On 2015/07/16 00:21:44, Dan Beam wrote: > > ^ needed? > > ping Done. https://codereview.chromium.org/1220793010/diff/470001/ui/base/template_expre... File ui/base/template_expressions.cc (right): https://codereview.chromium.org/1220793010/diff/470001/ui/base/template_expre... ui/base/template_expressions.cc:25: } On 2015/07/17 01:57:18, Dan Beam wrote: > nit: no curlies Done. https://codereview.chromium.org/1220793010/diff/470001/ui/base/template_expre... ui/base/template_expressions.cc:31: i += key_length + strlen("${}"); On 2015/07/17 01:57:18, Dan Beam wrote: > nit: i += strlen("${" + key_length + strlen("}") Done.
lgtm
The CQ bit was checked by dschuyler@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org Link to the patchset: https://codereview.chromium.org/1220793010/#ps490001 (title: "revieew changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1220793010/490001
Message was sent while issue was closed.
Committed patchset #26 (id:490001)
Message was sent while issue was closed.
Patchset 26 (id:??) landed as https://crrev.com/6c937646c650fdded550560a6d443a7dae846819 Cr-Commit-Position: refs/heads/master@{#339215}
Message was sent while issue was closed.
https://codereview.chromium.org/1220793010/diff/490001/chrome/browser/resourc... File chrome/browser/resources/ntp4/new_tab_theme.css (right): https://codereview.chromium.org/1220793010/diff/490001/chrome/browser/resourc... chrome/browser/resources/ntp4/new_tab_theme.css:50: background: linear-gradient(top, don't make functional changes while refactoring next time (this broke the page switcher's background on chrome://apps and has made its way to stable) |
