Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2349623003:
Add a Browser Task Scheduler Chrome Internals Page (Closed)
DescriptionAdd a Browser Task Scheduler Chrome Internals Page
This adds a simple status to show if the task scheduler is instantiated
or not to set up the infrastructure. More information will be added in
subsequent CLs.
BUG=553459
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/71aed696616be775fe6d0c47d0ba67a2bfc5cef9
Cr-Commit-Position: refs/heads/master@{#421255}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Mac/Linux Fix + String Change #
Total comments: 14
Patch Set 3 : CR Feedback #
Total comments: 10
Patch Set 4 : CR Feedback #Patch Set 5 : Move Files into task_scheduler_internals Directory and Add OWNERS #
Total comments: 2
Patch Set 6 : Remvoe allowexternalscript #Messages
Total messages: 73 (46 generated)
Description was changed from ========== Add a Browser Task Scheduler Chrome Internals Page This adds a simple status to show if the task scheduler is instantiated or not. BUG=553459 ========== to ========== Add a Browser Task Scheduler Chrome Internals Page This adds a simple status to show if the task scheduler is instantiated or not. BUG=553459 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
robliao@chromium.org changed reviewers: + fdoray@chromium.org, gab@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm in general but I'm not at all a resources/ + webui/ reviewer :-) https://codereview.chromium.org/2349623003/diff/1/chrome/browser/ui/webui/tas... File chrome/browser/ui/webui/task_scheduler_internals_ui.cc (right): https://codereview.chromium.org/2349623003/diff/1/chrome/browser/ui/webui/tas... chrome/browser/ui/webui/task_scheduler_internals_ui.cc:20: ? "Initialized" s/Initialized/Instantiated/ ?
robliao@chromium.org changed reviewers: + thakis@chromium.org, xiyuan@chromium.org
xiyuan@chromium.org: Please review changes in chrome/browser/resources/* chrome/browser/ui/webui/* thakis@chromium.org: Please review changes in chrome/BUILD.gn tools/gritsettings/resource_ids Thanks! https://codereview.chromium.org/2349623003/diff/1/chrome/browser/ui/webui/tas... File chrome/browser/ui/webui/task_scheduler_internals_ui.cc (right): https://codereview.chromium.org/2349623003/diff/1/chrome/browser/ui/webui/tas... chrome/browser/ui/webui/task_scheduler_internals_ui.cc:20: ? "Initialized" On 2016/09/19 20:46:19, gab wrote: > s/Initialized/Instantiated/ ? Sure, works for me. Done.
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2349623003/diff/20001/chrome/browser/resource... File chrome/browser/resources/task_scheduler_internals/index.html (right): https://codereview.chromium.org/2349623003/diff/20001/chrome/browser/resource... chrome/browser/resources/task_scheduler_internals/index.html:2: <html> Do we need to worry about i18n ? If so, this should be <html i18n-values="dir:textdirection;lang:language"> and C++ data source calls webui::SetLoadTimeDataDefaults(...) to fill in the data. https://codereview.chromium.org/2349623003/diff/20001/chrome/browser/resource... chrome/browser/resources/task_scheduler_internals/index.html:15: <h1>Task Scheduler Internals</h1> Do we need to worry about i18n? If so, the text should be in a grd file and populated via i18n-content attr (or friends). https://codereview.chromium.org/2349623003/diff/20001/chrome/browser/resource... File chrome/browser/resources/task_scheduler_internals_resources.grd (right): https://codereview.chromium.org/2349623003/diff/20001/chrome/browser/resource... chrome/browser/resources/task_scheduler_internals_resources.grd:4: <output filename="grit/task_scheduler_internals_resources.h" type="rc_header"> nit: wrap the line https://codereview.chromium.org/2349623003/diff/20001/chrome/browser/resource... chrome/browser/resources/task_scheduler_internals_resources.grd:7: <output filename="task_scheduler_internals_resources.pak" type="data_package" /> nit: wrap the line https://codereview.chromium.org/2349623003/diff/20001/chrome/browser/resource... chrome/browser/resources/task_scheduler_internals_resources.grd:11: <include name="IDR_TASK_SCHEDULER_INTERNALS_RESOURCES_INDEX_HTML" file="task_scheduler_internals/index.html" flattenhtml="true" allowexternalscript="false" type="BINDATA" /> nit: wrap the line https://codereview.chromium.org/2349623003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/task_scheduler_internals_ui_browsertest.cc (right): https://codereview.chromium.org/2349623003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/task_scheduler_internals_ui_browsertest.cc:36: IN_PROC_BROWSER_TEST_F(TaskSchedulerInternalsWebUIBrowserTest, Navigate) { You don't have to change. But fyi, webui test could be written in js. e.g. https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/sync_internals_b... https://codereview.chromium.org/2349623003/diff/20001/tools/gritsettings/reso... File tools/gritsettings/resource_ids (right): https://codereview.chromium.org/2349623003/diff/20001/tools/gritsettings/reso... tools/gritsettings/resource_ids:259: "includes": [28100], compoents_strings.grd above has a bunch of sub part. The gap (i.e. 28100-28020=80) seems not big enough.
non-expert lgtm
Thanks for the comments! https://codereview.chromium.org/2349623003/diff/20001/chrome/browser/resource... File chrome/browser/resources/task_scheduler_internals/index.html (right): https://codereview.chromium.org/2349623003/diff/20001/chrome/browser/resource... chrome/browser/resources/task_scheduler_internals/index.html:2: <html> On 2016/09/21 05:01:38, xiyuan wrote: > Do we need to worry about i18n ? If so, this should be > <html i18n-values="dir:textdirection;lang:language"> > > and C++ data source calls > webui::SetLoadTimeDataDefaults(...) > > to fill in the data. This is an internal page for Chromium. Doesn't appear to be necessary for the other internals page. We have no intention of i18n'ing this page. https://codereview.chromium.org/2349623003/diff/20001/chrome/browser/resource... chrome/browser/resources/task_scheduler_internals/index.html:15: <h1>Task Scheduler Internals</h1> On 2016/09/21 05:01:38, xiyuan wrote: > Do we need to worry about i18n? If so, the text should be in a grd file and > populated via i18n-content attr (or friends). See above. https://codereview.chromium.org/2349623003/diff/20001/chrome/browser/resource... File chrome/browser/resources/task_scheduler_internals_resources.grd (right): https://codereview.chromium.org/2349623003/diff/20001/chrome/browser/resource... chrome/browser/resources/task_scheduler_internals_resources.grd:4: <output filename="grit/task_scheduler_internals_resources.h" type="rc_header"> On 2016/09/21 05:01:38, xiyuan wrote: > nit: wrap the line Done. https://codereview.chromium.org/2349623003/diff/20001/chrome/browser/resource... chrome/browser/resources/task_scheduler_internals_resources.grd:7: <output filename="task_scheduler_internals_resources.pak" type="data_package" /> On 2016/09/21 05:01:38, xiyuan wrote: > nit: wrap the line Done. https://codereview.chromium.org/2349623003/diff/20001/chrome/browser/resource... chrome/browser/resources/task_scheduler_internals_resources.grd:11: <include name="IDR_TASK_SCHEDULER_INTERNALS_RESOURCES_INDEX_HTML" file="task_scheduler_internals/index.html" flattenhtml="true" allowexternalscript="false" type="BINDATA" /> On 2016/09/21 05:01:38, xiyuan wrote: > nit: wrap the line Done. https://codereview.chromium.org/2349623003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/task_scheduler_internals_ui_browsertest.cc (right): https://codereview.chromium.org/2349623003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/task_scheduler_internals_ui_browsertest.cc:36: IN_PROC_BROWSER_TEST_F(TaskSchedulerInternalsWebUIBrowserTest, Navigate) { On 2016/09/21 05:01:38, xiyuan wrote: > You don't have to change. But fyi, webui test could be written in js. > > e.g. > https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/sync_internals_b... We may be introducing some native based tests in the future. https://codereview.chromium.org/2349623003/diff/20001/tools/gritsettings/reso... File tools/gritsettings/resource_ids (right): https://codereview.chromium.org/2349623003/diff/20001/tools/gritsettings/reso... tools/gritsettings/resource_ids:259: "includes": [28100], On 2016/09/21 05:01:38, xiyuan wrote: > compoents_strings.grd above has a bunch of sub part. The gap (i.e. > 28100-28020=80) seems not big enough. Adjusted. The last index for components_strings.h currently is 28724.
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/09/21 22:48:45, xiyuan wrote: > lgtm Ping for thakis. Thanks!
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
thakis@chromium.org changed reviewers: + dbeam@chromium.org
lgtm, but I'm adding a webui/OWNER too
this is way too much code can't you just respond with a plain-text nextwork resource like about:histograms?
On 2016/09/23 17:59:03, Dan Beam wrote: > this is way too much code > > can't you just respond with a plain-text nextwork resource like > about:histograms? This is the initial checkin to get the infra set up. We plan to add more information to this page.
Description was changed from ========== Add a Browser Task Scheduler Chrome Internals Page This adds a simple status to show if the task scheduler is instantiated or not. BUG=553459 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add a Browser Task Scheduler Chrome Internals Page This adds a simple status to show if the task scheduler is instantiated or not to set up the infrastructure. More information will be added in subsequent CLs. BUG=553459 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Add a Browser Task Scheduler Chrome Internals Page This adds a simple status to show if the task scheduler is instantiated or not to set up the infrastructure. More information will be added in subsequent CLs. BUG=553459 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add a Browser Task Scheduler Chrome Internals Page This adds a simple status to show if the task scheduler is instantiated or not to set up the infrastructure. More information will be added in subsequent CLs. BUG=553459 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
On 2016/09/23 17:48:57, Nico wrote: > lgtm, but I'm adding a webui/OWNER too For the record, xiyuan is also a webui owner as well.
https://codereview.chromium.org/2349623003/diff/40001/chrome/browser/resource... File chrome/browser/resources/task_scheduler_internals/index.html (right): https://codereview.chromium.org/2349623003/diff/40001/chrome/browser/resource... chrome/browser/resources/task_scheduler_internals/index.html:7: --> you don't need this (very few other html files have it) https://codereview.chromium.org/2349623003/diff/40001/chrome/browser/resource... chrome/browser/resources/task_scheduler_internals/index.html:16: Status: <span i18n-content="status"></span> Status: <span>$i18n{status}</span> https://codereview.chromium.org/2349623003/diff/40001/chrome/browser/resource... chrome/browser/resources/task_scheduler_internals/index.html:17: <script src="chrome://resources/js/i18n_template.js"></script> don't use this, use $i18n{} https://codereview.chromium.org/2349623003/diff/40001/chrome/browser/resource... File chrome/browser/resources/task_scheduler_internals_resources.grd (right): https://codereview.chromium.org/2349623003/diff/40001/chrome/browser/resource... chrome/browser/resources/task_scheduler_internals_resources.grd:17: flattenhtml="true" i don't think you need flattenhtml="true" https://codereview.chromium.org/2349623003/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/task_scheduler_internals_ui_browsertest.cc (right): https://codereview.chromium.org/2349623003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/task_scheduler_internals_ui_browsertest.cc:22: std::string url_string("chrome://"); nit: why not use kChromeUIScheme here?
can we also put this into a sub directory or add per-file owners? many of these internals pages were removed (or are scheduled to be removed) for eraser. having clear owners and a very clear purpose for each webui page is important, as they add size to chrome for all users (including android and ios in some cases).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patch 4 covers the feedback. Thanks for the review! > can we also put this into a sub directory or add per-file owners? Done. Covered in Patch 5 https://codereview.chromium.org/2349623003/diff/40001/chrome/browser/resource... File chrome/browser/resources/task_scheduler_internals/index.html (right): https://codereview.chromium.org/2349623003/diff/40001/chrome/browser/resource... chrome/browser/resources/task_scheduler_internals/index.html:7: --> On 2016/09/23 18:08:59, Dan Beam wrote: > you don't need this (very few other html files have it) Sounds like a cleanup is in order. 3829 hits on html files for "The Chromium Authors. All rights reserved." https://cs.chromium.org/search/?q=%22The+Chromium+Authors.+All+rights+reserve... I've removed it here. https://codereview.chromium.org/2349623003/diff/40001/chrome/browser/resource... chrome/browser/resources/task_scheduler_internals/index.html:16: Status: <span i18n-content="status"></span> On 2016/09/23 18:09:00, Dan Beam wrote: > Status: <span>$i18n{status}</span> Nice! This is exactly what I want. Done. https://codereview.chromium.org/2349623003/diff/40001/chrome/browser/resource... chrome/browser/resources/task_scheduler_internals/index.html:17: <script src="chrome://resources/js/i18n_template.js"></script> On 2016/09/23 18:09:00, Dan Beam wrote: > don't use this, use $i18n{} Done. https://codereview.chromium.org/2349623003/diff/40001/chrome/browser/resource... File chrome/browser/resources/task_scheduler_internals_resources.grd (right): https://codereview.chromium.org/2349623003/diff/40001/chrome/browser/resource... chrome/browser/resources/task_scheduler_internals_resources.grd:17: flattenhtml="true" On 2016/09/23 18:09:00, Dan Beam wrote: > i don't think you need flattenhtml="true" Done. https://codereview.chromium.org/2349623003/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/task_scheduler_internals_ui_browsertest.cc (right): https://codereview.chromium.org/2349623003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/task_scheduler_internals_ui_browsertest.cc:22: std::string url_string("chrome://"); On 2016/09/23 18:09:00, Dan Beam wrote: > nit: why not use kChromeUIScheme here? Done.
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Ping for dbeam. Thanks.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2349623003/diff/80001/chrome/browser/resource... File chrome/browser/resources/task_scheduler_internals/resources.grd (right): https://codereview.chromium.org/2349623003/diff/80001/chrome/browser/resource... chrome/browser/resources/task_scheduler_internals/resources.grd:17: allowexternalscript="false" nit: i think this option only has an effect with flattenhtml="true", which you're no longer using. can you remove allowexternalscript="false" as well?
https://codereview.chromium.org/2349623003/diff/80001/chrome/browser/resource... File chrome/browser/resources/task_scheduler_internals/resources.grd (right): https://codereview.chromium.org/2349623003/diff/80001/chrome/browser/resource... chrome/browser/resources/task_scheduler_internals/resources.grd:17: allowexternalscript="false" On 2016/09/26 18:10:34, Dan Beam wrote: > nit: i think this option only has an effect with flattenhtml="true", which > you're no longer using. can you remove allowexternalscript="false" as well? Done.
The CQ bit was checked by robliao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org, fdoray@chromium.org, xiyuan@chromium.org, thakis@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2349623003/#ps100001 (title: "Remvoe allowexternalscript")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by robliao@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by robliao@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by robliao@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add a Browser Task Scheduler Chrome Internals Page This adds a simple status to show if the task scheduler is instantiated or not to set up the infrastructure. More information will be added in subsequent CLs. BUG=553459 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add a Browser Task Scheduler Chrome Internals Page This adds a simple status to show if the task scheduler is instantiated or not to set up the infrastructure. More information will be added in subsequent CLs. BUG=553459 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Add a Browser Task Scheduler Chrome Internals Page This adds a simple status to show if the task scheduler is instantiated or not to set up the infrastructure. More information will be added in subsequent CLs. BUG=553459 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add a Browser Task Scheduler Chrome Internals Page This adds a simple status to show if the task scheduler is instantiated or not to set up the infrastructure. More information will be added in subsequent CLs. BUG=553459 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/71aed696616be775fe6d0c47d0ba67a2bfc5cef9 Cr-Commit-Position: refs/heads/master@{#421255} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/71aed696616be775fe6d0c47d0ba67a2bfc5cef9 Cr-Commit-Position: refs/heads/master@{#421255} |
