Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1690773003:
[Template expressions] expanding $i18n expressions on webui html (Closed)
Description[Template expressions] expanding $i18n expressions on webui html
This CL adds template replacement for webui html file.
Because both the JS and C++ replacement system are running
(until the JS system* is phased out), this CL adds
overhead to the load time of webui pages such as (on a fast
machine)
C++ time:
- chrome://settings - approx. 0.000083 seconds
- chrome://downloads - approx. 0.000041 seconds
- chrome://md-settings - approx. 0.0011 seconds
JS time:
- chrome://settings - approx. 0.019 seconds
- chrome://downloads - approx. 0.0057 seconds
- chrome://md-settings - approx. 0.074 seconds
The file
chrome/browser/resources/settings/on_startup_page/on_startup_page.html
is included as an example that uses the feature added
by this CL.
*i18n_template.js
BUG=506009
Committed: https://crrev.com/411da121ddca19cea770c25a4334330c427e13bf
Cr-Commit-Position: refs/heads/master@{#377394}
Patch Set 1 #
Total comments: 8
Patch Set 2 : adding example replacement #Patch Set 3 : removed extra space in comment #
Total comments: 10
Patch Set 4 : changed example to extentions page #Patch Set 5 : added colons in comments #
Total comments: 2
Patch Set 6 : movinging example changes to separate CL #Patch Set 7 : changed comments #
Messages
Total messages: 30 (12 generated)
Description was changed from ========== [Template expressions] expanding $i18n expressions on webui html This CL adds template replacement for webui html file. Because both the old and new replacement system is running (until the old system is phased out), this CL adds overhead to the load time of webui pages such as (on a fast machine) - chrome://settings - approx. 0.000083 seconds - chrome://downloads - approx. 0.000041 seconds - chrome://md-settings - approx. 0.001129 seconds BUG=506009 ========== to ========== [Template expressions] expanding $i18n expressions on webui html This CL adds template replacement for webui html file. Because both the JS and C++ replacement system are running (until the JS system* is phased out), this CL adds overhead to the load time of webui pages such as (on a fast machine) C++ time: - chrome://settings - approx. 0.000083 seconds - chrome://downloads - approx. 0.000041 seconds - chrome://md-settings - approx. 0.001129 seconds JS time: - chrome://settings - approx. 0.0199 seconds - chrome://downloads - approx. 0.0057 seconds - chrome://md-settings - approx. 0.0745 seconds *i18n_template.js BUG=506009 ==========
Description was changed from ========== [Template expressions] expanding $i18n expressions on webui html This CL adds template replacement for webui html file. Because both the JS and C++ replacement system are running (until the JS system* is phased out), this CL adds overhead to the load time of webui pages such as (on a fast machine) C++ time: - chrome://settings - approx. 0.000083 seconds - chrome://downloads - approx. 0.000041 seconds - chrome://md-settings - approx. 0.001129 seconds JS time: - chrome://settings - approx. 0.0199 seconds - chrome://downloads - approx. 0.0057 seconds - chrome://md-settings - approx. 0.0745 seconds *i18n_template.js BUG=506009 ========== to ========== [Template expressions] expanding $i18n expressions on webui html This CL adds template replacement for webui html file. Because both the JS and C++ replacement system are running (until the JS system* is phased out), this CL adds overhead to the load time of webui pages such as (on a fast machine) C++ time: - chrome://settings - approx. 0.000083 seconds - chrome://downloads - approx. 0.000041 seconds - chrome://md-settings - approx. 0.0011 seconds JS time: - chrome://settings - approx. 0.019 seconds - chrome://downloads - approx. 0.0057 seconds - chrome://md-settings - approx. 0.074 seconds *i18n_template.js BUG=506009 ==========
dschuyler@chromium.org changed reviewers: + dbeam@chromium.org
can you add an example that uses WebUIDataSourceImpl::replacements_? https://codereview.chromium.org/1690773003/diff/1/content/browser/webui/web_u... File content/browser/webui/web_ui_data_source_impl.cc (right): https://codereview.chromium.org/1690773003/diff/1/content/browser/webui/web_u... content/browser/webui/web_ui_data_source_impl.cc:112: base::UTF16ToUTF8(GetContentClient()->GetLocalizedString(ids)); wat https://codereview.chromium.org/1690773003/diff/1/content/browser/webui/web_u... content/browser/webui/web_ui_data_source_impl.cc:124: // TODO(dschuyler) Change name of |localized_strings_| to |load_time_flags_| nit: load_time_data_ https://codereview.chromium.org/1690773003/diff/1/content/browser/webui/web_u... content/browser/webui/web_ui_data_source_impl.cc:125: // or similar. These values haven't been found as strings for one space between sentences in comments https://codereview.chromium.org/1690773003/diff/1/content/browser/webui/web_u... File content/browser/webui/web_ui_data_source_impl.h (left): https://codereview.chromium.org/1690773003/diff/1/content/browser/webui/web_u... content/browser/webui/web_ui_data_source_impl.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. for future reference: i wouldn't bother doing this (leaving old copyrights untouched is probably better)
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/1690773003/diff/1/content/browser/webui/web_u... File content/browser/webui/web_ui_data_source_impl.cc (right): https://codereview.chromium.org/1690773003/diff/1/content/browser/webui/web_u... content/browser/webui/web_ui_data_source_impl.cc:112: base::UTF16ToUTF8(GetContentClient()->GetLocalizedString(ids)); On 2016/02/11 02:12:32, Dan Beam wrote: > wat Done. https://codereview.chromium.org/1690773003/diff/1/content/browser/webui/web_u... content/browser/webui/web_ui_data_source_impl.cc:124: // TODO(dschuyler) Change name of |localized_strings_| to |load_time_flags_| On 2016/02/11 02:12:32, Dan Beam wrote: > nit: load_time_data_ Done. https://codereview.chromium.org/1690773003/diff/1/content/browser/webui/web_u... content/browser/webui/web_ui_data_source_impl.cc:125: // or similar. These values haven't been found as strings for On 2016/02/11 02:12:32, Dan Beam wrote: > one space between sentences in comments Done. https://codereview.chromium.org/1690773003/diff/1/content/browser/webui/web_u... File content/browser/webui/web_ui_data_source_impl.h (left): https://codereview.chromium.org/1690773003/diff/1/content/browser/webui/web_u... content/browser/webui/web_ui_data_source_impl.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2016/02/11 02:12:32, Dan Beam wrote: > for future reference: i wouldn't bother doing this (leaving old copyrights > untouched is probably better) Acknowledged.
Description was changed from ========== [Template expressions] expanding $i18n expressions on webui html This CL adds template replacement for webui html file. Because both the JS and C++ replacement system are running (until the JS system* is phased out), this CL adds overhead to the load time of webui pages such as (on a fast machine) C++ time: - chrome://settings - approx. 0.000083 seconds - chrome://downloads - approx. 0.000041 seconds - chrome://md-settings - approx. 0.0011 seconds JS time: - chrome://settings - approx. 0.019 seconds - chrome://downloads - approx. 0.0057 seconds - chrome://md-settings - approx. 0.074 seconds *i18n_template.js BUG=506009 ========== to ========== [Template expressions] expanding $i18n expressions on webui html This CL adds template replacement for webui html file. Because both the JS and C++ replacement system are running (until the JS system* is phased out), this CL adds overhead to the load time of webui pages such as (on a fast machine) C++ time: - chrome://settings - approx. 0.000083 seconds - chrome://downloads - approx. 0.000041 seconds - chrome://md-settings - approx. 0.0011 seconds JS time: - chrome://settings - approx. 0.019 seconds - chrome://downloads - approx. 0.0057 seconds - chrome://md-settings - approx. 0.074 seconds The file chrome/browser/resources/settings/on_startup_page/on_startup_page.html is included as an example that uses the feature added by this CL. *i18n_template.js BUG=506009 ==========
Description was changed from ========== [Template expressions] expanding $i18n expressions on webui html This CL adds template replacement for webui html file. Because both the JS and C++ replacement system are running (until the JS system* is phased out), this CL adds overhead to the load time of webui pages such as (on a fast machine) C++ time: - chrome://settings - approx. 0.000083 seconds - chrome://downloads - approx. 0.000041 seconds - chrome://md-settings - approx. 0.0011 seconds JS time: - chrome://settings - approx. 0.019 seconds - chrome://downloads - approx. 0.0057 seconds - chrome://md-settings - approx. 0.074 seconds The file chrome/browser/resources/settings/on_startup_page/on_startup_page.html is included as an example that uses the feature added by this CL. *i18n_template.js BUG=506009 ========== to ========== [Template expressions] expanding $i18n expressions on webui html This CL adds template replacement for webui html file. Because both the JS and C++ replacement system are running (until the JS system* is phased out), this CL adds overhead to the load time of webui pages such as (on a fast machine) C++ time: - chrome://settings - approx. 0.000083 seconds - chrome://downloads - approx. 0.000041 seconds - chrome://md-settings - approx. 0.0011 seconds JS time: - chrome://settings - approx. 0.019 seconds - chrome://downloads - approx. 0.0057 seconds - chrome://md-settings - approx. 0.074 seconds The file chrome/browser/resources/settings/on_startup_page/on_startup_page.html is included as an example that uses the feature added by this CL. *i18n_template.js BUG=506009 ==========
https://codereview.chromium.org/1690773003/diff/50001/chrome/browser/resource... File chrome/browser/resources/settings/on_startup_page/on_startup_page.html (right): https://codereview.chromium.org/1690773003/diff/50001/chrome/browser/resource... chrome/browser/resources/settings/on_startup_page/on_startup_page.html:26: <span>$i18n{onStartupOpenSpecific}</span> could you do this on like, any other webui page instead? ;P https://codereview.chromium.org/1690773003/diff/50001/content/browser/webui/w... File content/browser/webui/web_ui_data_source_impl.cc (right): https://codereview.chromium.org/1690773003/diff/50001/content/browser/webui/w... content/browser/webui/web_ui_data_source_impl.cc:225: // template expansion upon. TODO(dschuyler): ^ also, don't wrap before 80 chars https://codereview.chromium.org/1690773003/diff/50001/content/browser/webui/w... content/browser/webui/web_ui_data_source_impl.cc:227: std::string final_version = ui::ReplaceTemplateExpressions( final_version -> replaced, translated
https://codereview.chromium.org/1690773003/diff/50001/chrome/browser/resource... File chrome/browser/resources/settings/on_startup_page/on_startup_page.html (right): https://codereview.chromium.org/1690773003/diff/50001/chrome/browser/resource... chrome/browser/resources/settings/on_startup_page/on_startup_page.html:26: <span>$i18n{onStartupOpenSpecific}</span> On 2016/02/12 23:45:14, Dan Beam wrote: > could you do this on like, any other webui page instead? ;P Done. https://codereview.chromium.org/1690773003/diff/50001/content/browser/webui/w... File content/browser/webui/web_ui_data_source_impl.cc (right): https://codereview.chromium.org/1690773003/diff/50001/content/browser/webui/w... content/browser/webui/web_ui_data_source_impl.cc:225: // template expansion upon. On 2016/02/12 23:45:14, Dan Beam wrote: > TODO(dschuyler): > ^ > > also, don't wrap before 80 chars Done. https://codereview.chromium.org/1690773003/diff/50001/content/browser/webui/w... content/browser/webui/web_ui_data_source_impl.cc:227: std::string final_version = ui::ReplaceTemplateExpressions( On 2016/02/12 23:45:14, Dan Beam wrote: > final_version -> replaced, translated Done.
https://codereview.chromium.org/1690773003/diff/50001/content/browser/webui/w... File content/browser/webui/web_ui_data_source_impl.cc (right): https://codereview.chromium.org/1690773003/diff/50001/content/browser/webui/w... content/browser/webui/web_ui_data_source_impl.cc:225: // template expansion upon. On 2016/02/13 00:23:24, dschuyler wrote: > On 2016/02/12 23:45:14, Dan Beam wrote: > > TODO(dschuyler): > > ^ > > > > also, don't wrap before 80 chars > > Done. this is what you have written 3 times in this CL: TODO(dschuyler) text this is what I should be TODO(dschuyler): text note the colon after the "TODO(dschuyler)"
https://codereview.chromium.org/1690773003/diff/50001/content/browser/webui/w... File content/browser/webui/web_ui_data_source_impl.cc (right): https://codereview.chromium.org/1690773003/diff/50001/content/browser/webui/w... content/browser/webui/web_ui_data_source_impl.cc:225: // template expansion upon. On 2016/02/13 00:28:57, Dan Beam wrote: > On 2016/02/13 00:23:24, dschuyler wrote: > > On 2016/02/12 23:45:14, Dan Beam wrote: > > > TODO(dschuyler): > > > ^ > > > > > > also, don't wrap before 80 chars > > > > Done. > > this is what you have written 3 times in this CL: > > TODO(dschuyler) text > > this is what I should be what I think it should be > > TODO(dschuyler): text > > note the colon after the "TODO(dschuyler)"
https://codereview.chromium.org/1690773003/diff/50001/content/browser/webui/w... File content/browser/webui/web_ui_data_source_impl.cc (right): https://codereview.chromium.org/1690773003/diff/50001/content/browser/webui/w... content/browser/webui/web_ui_data_source_impl.cc:225: // template expansion upon. On 2016/02/13 00:28:57, Dan Beam wrote: > On 2016/02/13 00:23:24, dschuyler wrote: > > On 2016/02/12 23:45:14, Dan Beam wrote: > > > TODO(dschuyler): > > > ^ > > > > > > also, don't wrap before 80 chars > > > > Done. > > this is what you have written 3 times in this CL: > > TODO(dschuyler) text > > this is what I should be > > TODO(dschuyler): text > > note the colon after the "TODO(dschuyler)" Done. https://codereview.chromium.org/1690773003/diff/50001/content/browser/webui/w... content/browser/webui/web_ui_data_source_impl.cc:225: // template expansion upon. On 2016/02/13 00:29:17, Dan Beam wrote: > On 2016/02/13 00:28:57, Dan Beam wrote: > > On 2016/02/13 00:23:24, dschuyler wrote: > > > On 2016/02/12 23:45:14, Dan Beam wrote: > > > > TODO(dschuyler): > > > > ^ > > > > > > > > also, don't wrap before 80 chars > > > > > > Done. > > > > this is what you have written 3 times in this CL: > > > > TODO(dschuyler) text > > > > this is what I should be > > what I think it should be > > > > > TODO(dschuyler): text > > > > note the colon after the "TODO(dschuyler)" Acknowledged.
https://codereview.chromium.org/1690773003/diff/90001/chrome/browser/resource... File chrome/browser/resources/extensions/extensions.html (right): https://codereview.chromium.org/1690773003/diff/90001/chrome/browser/resource... chrome/browser/resources/extensions/extensions.html:120: <span id="no-extensions-message">$i18n{extensionSettingsNoExtensions}</span> i understand that you're doing this at least once... but why not for a whole page? ideally, any page that you do it on would have significantly less (or no) i18n-content attributes left
https://codereview.chromium.org/1690773003/diff/90001/chrome/browser/resource... File chrome/browser/resources/extensions/extensions.html (right): https://codereview.chromium.org/1690773003/diff/90001/chrome/browser/resource... chrome/browser/resources/extensions/extensions.html:120: <span id="no-extensions-message">$i18n{extensionSettingsNoExtensions}</span> On 2016/02/13 00:41:22, Dan Beam wrote: > i understand that you're doing this at least once... but why not for a whole > page? > > ideally, any page that you do it on would have significantly less (or no) > i18n-content attributes left I've moved the example to a separate CL 1693173002. That CL has a more thorough example of moving from i18n-content="" expansion to $i18n{} replacements.
lgtm with those new comments we wrote in person
The CQ bit was checked by dschuyler@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1690773003/130001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1690773003/130001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
dschuyler@chromium.org changed reviewers: + avi@chromium.org
@avi for owner approval.
lgtm stampity stamp
The CQ bit was checked by dschuyler@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1690773003/130001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1690773003/130001
Message was sent while issue was closed.
Description was changed from ========== [Template expressions] expanding $i18n expressions on webui html This CL adds template replacement for webui html file. Because both the JS and C++ replacement system are running (until the JS system* is phased out), this CL adds overhead to the load time of webui pages such as (on a fast machine) C++ time: - chrome://settings - approx. 0.000083 seconds - chrome://downloads - approx. 0.000041 seconds - chrome://md-settings - approx. 0.0011 seconds JS time: - chrome://settings - approx. 0.019 seconds - chrome://downloads - approx. 0.0057 seconds - chrome://md-settings - approx. 0.074 seconds The file chrome/browser/resources/settings/on_startup_page/on_startup_page.html is included as an example that uses the feature added by this CL. *i18n_template.js BUG=506009 ========== to ========== [Template expressions] expanding $i18n expressions on webui html This CL adds template replacement for webui html file. Because both the JS and C++ replacement system are running (until the JS system* is phased out), this CL adds overhead to the load time of webui pages such as (on a fast machine) C++ time: - chrome://settings - approx. 0.000083 seconds - chrome://downloads - approx. 0.000041 seconds - chrome://md-settings - approx. 0.0011 seconds JS time: - chrome://settings - approx. 0.019 seconds - chrome://downloads - approx. 0.0057 seconds - chrome://md-settings - approx. 0.074 seconds The file chrome/browser/resources/settings/on_startup_page/on_startup_page.html is included as an example that uses the feature added by this CL. *i18n_template.js BUG=506009 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:130001)
Message was sent while issue was closed.
Description was changed from ========== [Template expressions] expanding $i18n expressions on webui html This CL adds template replacement for webui html file. Because both the JS and C++ replacement system are running (until the JS system* is phased out), this CL adds overhead to the load time of webui pages such as (on a fast machine) C++ time: - chrome://settings - approx. 0.000083 seconds - chrome://downloads - approx. 0.000041 seconds - chrome://md-settings - approx. 0.0011 seconds JS time: - chrome://settings - approx. 0.019 seconds - chrome://downloads - approx. 0.0057 seconds - chrome://md-settings - approx. 0.074 seconds The file chrome/browser/resources/settings/on_startup_page/on_startup_page.html is included as an example that uses the feature added by this CL. *i18n_template.js BUG=506009 ========== to ========== [Template expressions] expanding $i18n expressions on webui html This CL adds template replacement for webui html file. Because both the JS and C++ replacement system are running (until the JS system* is phased out), this CL adds overhead to the load time of webui pages such as (on a fast machine) C++ time: - chrome://settings - approx. 0.000083 seconds - chrome://downloads - approx. 0.000041 seconds - chrome://md-settings - approx. 0.0011 seconds JS time: - chrome://settings - approx. 0.019 seconds - chrome://downloads - approx. 0.0057 seconds - chrome://md-settings - approx. 0.074 seconds The file chrome/browser/resources/settings/on_startup_page/on_startup_page.html is included as an example that uses the feature added by this CL. *i18n_template.js BUG=506009 Committed: https://crrev.com/411da121ddca19cea770c25a4334330c427e13bf Cr-Commit-Position: refs/heads/master@{#377394} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/411da121ddca19cea770c25a4334330c427e13bf Cr-Commit-Position: refs/heads/master@{#377394} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
