Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2420973002:
Plumb Task Scheduler Histograms to the Task Scheduler Internals Page (Closed)
DescriptionPlumb Task Scheduler Histograms to the Task Scheduler Internals Page
This sets up the plumbing to get data from the Task Scheduler to the
internals page. The first target are some selected histograms.
BUG=553459
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/6f59a9ad1c00c218de6239ae17e8f0987fa589d7
Cr-Commit-Position: refs/heads/master@{#426520}
Patch Set 1 #
Total comments: 12
Patch Set 2 : CR Feedback #
Total comments: 3
Patch Set 3 : CR Feedback #
Total comments: 28
Patch Set 4 : CR Feedback #Patch Set 5 : Rebase to 8e0d67c #Patch Set 6 : The Return of std::move #Messages
Total messages: 39 (21 generated)
Description was changed from ========== Plumb Task Scheduler Histograms to the Task Scheduler Internals Page This sets up the plumbing to get data from the Task Scheduler to the internals page. The first target are some selected histograms. BUG=553459 ========== to ========== Plumb Task Scheduler Histograms to the Task Scheduler Internals Page This sets up the plumbing to get data from the Task Scheduler to the internals page. The first target are some selected histograms. BUG=553459 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Plumb Task Scheduler Histograms to the Task Scheduler Internals Page This sets up the plumbing to get data from the Task Scheduler to the internals page. The first target are some selected histograms. BUG=553459 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Plumb Task Scheduler Histograms to the Task Scheduler Internals Page This sets up the plumbing to get data from the Task Scheduler to the internals page. The first target are some selected histograms. BUG=553459 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
robliao@chromium.org changed reviewers: + fdoray@chromium.org
The code looks good. What are the benefits of having histograms on this page vs. Ctrl+F in chrome://histograms? Could we keep this page for more advanced information (maybe realtime?) https://codereview.chromium.org/2420973002/diff/1/base/task_scheduler/task_sc... File base/task_scheduler/task_scheduler_impl.cc (right): https://codereview.chromium.org/2420973002/diff/1/base/task_scheduler/task_sc... base/task_scheduler/task_scheduler_impl.cc:56: std::vector<const HistogramBase*> TaskSchedulerImpl::GetHistograms() { ... GetHistograms() const { https://codereview.chromium.org/2420973002/diff/1/base/task_scheduler/task_sc... base/task_scheduler/task_scheduler_impl.cc:58: for (const auto& worker_pool : worker_pools_) { Could worker pools have a GetHistograms() method? That way we wouldn't have to change task_scheduler_impl.cc when we add new histograms in a worker pool. Also, it would be nice if this code supported SchedulerWorkerPool impls with different sets of histograms (e.g. a SchedulerWorkerPool backed by the Windows thread pool will not have detach histograms). https://codereview.chromium.org/2420973002/diff/1/chrome/browser/resources/ta... File chrome/browser/resources/task_scheduler_internals/index.js (right): https://codereview.chromium.org/2420973002/diff/1/chrome/browser/resources/ta... chrome/browser/resources/task_scheduler_internals/index.js:45: dataRow.appendChild(data); Could we reuse the ASCII charts from chrome://histograms/? https://codereview.chromium.org/2420973002/diff/1/chrome/browser/ui/webui/tas... File chrome/browser/ui/webui/task_scheduler_internals/task_scheduler_internals_ui.cc (right): https://codereview.chromium.org/2420973002/diff/1/chrome/browser/ui/webui/tas... chrome/browser/ui/webui/task_scheduler_internals/task_scheduler_internals_ui.cc:10: #include "base/bind_helpers.h" Gab told me that including bind_helpers.h is not required https://codereview.chromium.org/2180953002/diff/1/base/task_runner.cc#newcode8 https://codereview.chromium.org/2420973002/diff/1/chrome/browser/ui/webui/tas... chrome/browser/ui/webui/task_scheduler_internals/task_scheduler_internals_ui.cc:47: return std::move(values); no move required when returning a local variable https://www.chromium.org/rvalue-references#TOC-11.-Don-t-return-references.-D.... http://stackoverflow.com/a/14856553
On 2016/10/14 20:59:06, fdoray wrote: > The code looks good. > > What are the benefits of having histograms on this page vs. Ctrl+F in > chrome://histograms? Could we keep this page for more advanced information > (maybe realtime?) Or even a trick I recently discovered: chrome://histograms/TaskScheduler :-O! I imagine this page as a realtime feed of stats, if piping histograms helps with giving a live view of their value updated on a timer in a follow-up CL then that's cool, otherwise I don't think that's what we want here. Eventually maybe a live plot of the histogram's value would be even better than a textual live value as I expect this will flicker too fast to be useful text. Polymer UI?! ;-) > > https://codereview.chromium.org/2420973002/diff/1/base/task_scheduler/task_sc... > File base/task_scheduler/task_scheduler_impl.cc (right): > > https://codereview.chromium.org/2420973002/diff/1/base/task_scheduler/task_sc... > base/task_scheduler/task_scheduler_impl.cc:56: std::vector<const HistogramBase*> > TaskSchedulerImpl::GetHistograms() { > ... GetHistograms() const { > > https://codereview.chromium.org/2420973002/diff/1/base/task_scheduler/task_sc... > base/task_scheduler/task_scheduler_impl.cc:58: for (const auto& worker_pool : > worker_pools_) { > Could worker pools have a GetHistograms() method? > > That way we wouldn't have to change task_scheduler_impl.cc when we add new > histograms in a worker pool. Also, it would be nice if this code supported > SchedulerWorkerPool impls with different sets of histograms (e.g. a > SchedulerWorkerPool backed by the Windows thread pool will not have detach > histograms). > > https://codereview.chromium.org/2420973002/diff/1/chrome/browser/resources/ta... > File chrome/browser/resources/task_scheduler_internals/index.js (right): > > https://codereview.chromium.org/2420973002/diff/1/chrome/browser/resources/ta... > chrome/browser/resources/task_scheduler_internals/index.js:45: > dataRow.appendChild(data); > Could we reuse the ASCII charts from chrome://histograms/? > > https://codereview.chromium.org/2420973002/diff/1/chrome/browser/ui/webui/tas... > File > chrome/browser/ui/webui/task_scheduler_internals/task_scheduler_internals_ui.cc > (right): > > https://codereview.chromium.org/2420973002/diff/1/chrome/browser/ui/webui/tas... > chrome/browser/ui/webui/task_scheduler_internals/task_scheduler_internals_ui.cc:10: > #include "base/bind_helpers.h" > Gab told me that including bind_helpers.h is not required > https://codereview.chromium.org/2180953002/diff/1/base/task_runner.cc#newcode8 > > https://codereview.chromium.org/2420973002/diff/1/chrome/browser/ui/webui/tas... > chrome/browser/ui/webui/task_scheduler_internals/task_scheduler_internals_ui.cc:47: > return std::move(values); > no move required when returning a local variable > > https://www.chromium.org/rvalue-references#TOC-11.-Don-t-return-references.-D.... > > http://stackoverflow.com/a/14856553
On 2016/10/17 19:22:20, gab (OOO) wrote: > On 2016/10/14 20:59:06, fdoray wrote: > > The code looks good. > > > > What are the benefits of having histograms on this page vs. Ctrl+F in > > chrome://histograms? Could we keep this page for more advanced information > > (maybe realtime?) > > Or even a trick I recently discovered: chrome://histograms/TaskScheduler :-O! > > I imagine this page as a realtime feed of stats, if piping histograms helps with > giving a live view of their value updated on a timer in a follow-up CL then > that's cool, otherwise I don't think that's what we want here. > > Eventually maybe a live plot of the histogram's value would be even better than > a textual live value as I expect this will flicker too fast to be useful text. > Polymer UI?! ;-) Though I was reading this morning on chromium-dev that Polymer UI isn't possible for chrome internals page on mobile :-( > > > > > > https://codereview.chromium.org/2420973002/diff/1/base/task_scheduler/task_sc... > > File base/task_scheduler/task_scheduler_impl.cc (right): > > > > > https://codereview.chromium.org/2420973002/diff/1/base/task_scheduler/task_sc... > > base/task_scheduler/task_scheduler_impl.cc:56: std::vector<const > HistogramBase*> > > TaskSchedulerImpl::GetHistograms() { > > ... GetHistograms() const { > > > > > https://codereview.chromium.org/2420973002/diff/1/base/task_scheduler/task_sc... > > base/task_scheduler/task_scheduler_impl.cc:58: for (const auto& worker_pool : > > worker_pools_) { > > Could worker pools have a GetHistograms() method? > > > > That way we wouldn't have to change task_scheduler_impl.cc when we add new > > histograms in a worker pool. Also, it would be nice if this code supported > > SchedulerWorkerPool impls with different sets of histograms (e.g. a > > SchedulerWorkerPool backed by the Windows thread pool will not have detach > > histograms). > > > > > https://codereview.chromium.org/2420973002/diff/1/chrome/browser/resources/ta... > > File chrome/browser/resources/task_scheduler_internals/index.js (right): > > > > > https://codereview.chromium.org/2420973002/diff/1/chrome/browser/resources/ta... > > chrome/browser/resources/task_scheduler_internals/index.js:45: > > dataRow.appendChild(data); > > Could we reuse the ASCII charts from chrome://histograms/? > > > > > https://codereview.chromium.org/2420973002/diff/1/chrome/browser/ui/webui/tas... > > File > > > chrome/browser/ui/webui/task_scheduler_internals/task_scheduler_internals_ui.cc > > (right): > > > > > https://codereview.chromium.org/2420973002/diff/1/chrome/browser/ui/webui/tas... > > > chrome/browser/ui/webui/task_scheduler_internals/task_scheduler_internals_ui.cc:10: > > #include "base/bind_helpers.h" > > Gab told me that including bind_helpers.h is not required > > https://codereview.chromium.org/2180953002/diff/1/base/task_runner.cc#newcode8 > > > > > https://codereview.chromium.org/2420973002/diff/1/chrome/browser/ui/webui/tas... > > > chrome/browser/ui/webui/task_scheduler_internals/task_scheduler_internals_ui.cc:47: > > return std::move(values); > > no move required when returning a local variable > > > > > https://www.chromium.org/rvalue-references#TOC-11.-Don-t-return-references.-D.... > > > > http://stackoverflow.com/a/14856553
Patchset #2 (id:20001) has been deleted
robliao@chromium.org changed reviewers: + gab@chromium.org
> What are the benefits of having histograms on this page vs. Ctrl+F in chrome://histograms? Could we keep this page for more advanced information (maybe realtime?) The goal of the task scheduler internals page is to provide a dashboard on the state of the scheduler. Histograms would naturally belong here. The benefit of putting the histograms here also gives us more control on how we present them, like realtime dynamic updates. > Though I was reading this morning on chromium-dev that Polymer UI isn't possible for chrome internals page on mobile :-( Polymer would be way down the line. It could be something to consider once we get all the data piped through. https://codereview.chromium.org/2420973002/diff/1/base/task_scheduler/task_sc... File base/task_scheduler/task_scheduler_impl.cc (right): https://codereview.chromium.org/2420973002/diff/1/base/task_scheduler/task_sc... base/task_scheduler/task_scheduler_impl.cc:56: std::vector<const HistogramBase*> TaskSchedulerImpl::GetHistograms() { On 2016/10/14 20:59:05, fdoray wrote: > ... GetHistograms() const { Done. https://codereview.chromium.org/2420973002/diff/1/base/task_scheduler/task_sc... base/task_scheduler/task_scheduler_impl.cc:58: for (const auto& worker_pool : worker_pools_) { On 2016/10/14 20:59:06, fdoray wrote: > Could worker pools have a GetHistograms() method? > > That way we wouldn't have to change task_scheduler_impl.cc when we add new > histograms in a worker pool. Also, it would be nice if this code supported > SchedulerWorkerPool impls with different sets of histograms (e.g. a > SchedulerWorkerPool backed by the Windows thread pool will not have detach > histograms). Done. https://codereview.chromium.org/2420973002/diff/1/chrome/browser/resources/ta... File chrome/browser/resources/task_scheduler_internals/index.js (right): https://codereview.chromium.org/2420973002/diff/1/chrome/browser/resources/ta... chrome/browser/resources/task_scheduler_internals/index.js:45: dataRow.appendChild(data); On 2016/10/14 20:59:06, fdoray wrote: > Could we reuse the ASCII charts from chrome://histograms/? We're going to have spiffy HTML tables that will update dynamically :-). https://codereview.chromium.org/2420973002/diff/1/chrome/browser/ui/webui/tas... File chrome/browser/ui/webui/task_scheduler_internals/task_scheduler_internals_ui.cc (right): https://codereview.chromium.org/2420973002/diff/1/chrome/browser/ui/webui/tas... chrome/browser/ui/webui/task_scheduler_internals/task_scheduler_internals_ui.cc:10: #include "base/bind_helpers.h" On 2016/10/14 20:59:06, fdoray wrote: > Gab told me that including bind_helpers.h is not required > https://codereview.chromium.org/2180953002/diff/1/base/task_runner.cc#newcode8 Oddly enough, it's even further than that. bind.h -> bind_internal.h -> bind_helpers.h. The guidance from the style guide is If you rely on symbols from bar.h, don't count on the fact that you included foo.h which (currently) includes bar.h: include bar.h yourself, unless foo.h explicitly demonstrates its intent to provide you the symbols of bar.h. https://codereview.chromium.org/2420973002/diff/1/chrome/browser/ui/webui/tas... chrome/browser/ui/webui/task_scheduler_internals/task_scheduler_internals_ui.cc:47: return std::move(values); On 2016/10/14 20:59:06, fdoray wrote: > no move required when returning a local variable > > https://www.chromium.org/rvalue-references#TOC-11.-Don-t-return-references.-D.... > > http://stackoverflow.com/a/14856553 Done.
lgtm % nits for base/task_scheduler, I'll defer to someone else for the JS/CSS/WebUI stuff as I know nothing about it https://codereview.chromium.org/2420973002/diff/1/chrome/browser/ui/webui/tas... File chrome/browser/ui/webui/task_scheduler_internals/task_scheduler_internals_ui.cc (right): https://codereview.chromium.org/2420973002/diff/1/chrome/browser/ui/webui/tas... chrome/browser/ui/webui/task_scheduler_internals/task_scheduler_internals_ui.cc:10: #include "base/bind_helpers.h" On 2016/10/17 22:36:57, robliao wrote: > On 2016/10/14 20:59:06, fdoray wrote: > > Gab told me that including bind_helpers.h is not required > > https://codereview.chromium.org/2180953002/diff/1/base/task_runner.cc#newcode8 > > Oddly enough, it's even further than that. > bind.h -> bind_internal.h -> bind_helpers.h. > > The guidance from the style guide is > If you rely on symbols from bar.h, don't count on the fact that you included > foo.h which (currently) includes bar.h: include bar.h yourself, unless foo.h > explicitly demonstrates its intent to provide you the symbols of bar.h. I think it does demonstrate its intent by having bind.h refer to callback.md which documents to use the bind helpers without mentioning the need for an extra include. I'm a big fan of IWUU but in this case I think bind.h is known to come with everything you need to use it (i.e. it would be silly for every callsite to include both bind.h and bind_helpers.h). https://codereview.chromium.org/2420973002/diff/40001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool_impl.h (right): https://codereview.chromium.org/2420973002/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl.h:101: void ReportHistograms(std::vector<const HistogramBase*>* histograms) const; This isn't "reporting" histograms (i.e. sending to to UMA), it's "getting" histograms IMO and since TaskScheduler already uses GetHistograms, it seems natural to use this here as well.
lgtm https://codereview.chromium.org/2420973002/diff/40001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler_impl.cc (right): https://codereview.chromium.org/2420973002/diff/40001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl.cc:58: for (const auto& worker_pool : worker_pools_) { no braces
robliao@chromium.org changed reviewers: + dbeam@chromium.org
dbeam@chromium.org: Please review changes in chrome/browser/* Thanks! https://codereview.chromium.org/2420973002/diff/40001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool_impl.h (right): https://codereview.chromium.org/2420973002/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl.h:101: void ReportHistograms(std::vector<const HistogramBase*>* histograms) const; On 2016/10/18 14:28:48, gab wrote: > This isn't "reporting" histograms (i.e. sending to to UMA), it's "getting" > histograms IMO and since TaskScheduler already uses GetHistograms, it seems > natural to use this here as well. Done.
robliao@chromium.org changed reviewers: + xiyuan@chromium.org - dbeam@chromium.org
xiyuan: Please review chrome/browser/* while dbeam is OOO. Thanks!
lgtm https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/resource... File chrome/browser/resources/task_scheduler_internals/index.css (right): https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/resource... chrome/browser/resources/task_scheduler_internals/index.css:17: #details { nit: get rid of this and use hidden in html: <div id="details" hidden> in js: $('details').hidden = !data.instantiated https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/resource... File chrome/browser/resources/task_scheduler_internals/index.js (right): https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/resource... chrome/browser/resources/task_scheduler_internals/index.js:16: title.innerHTML = histogram.name; nit: title.textContent = histogram.name; avoid using innerHTML when we can. https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/resource... chrome/browser/resources/task_scheduler_internals/index.js:23: unavailable.innerHTML = 'No Data Recorded'; nit: .innerHTML -> .textContent https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/resource... chrome/browser/resources/task_scheduler_internals/index.js:41: header.innerHTML = bucket.min + '-' + bucket.max; nit: .innerHTML -> .textContent https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/resource... chrome/browser/resources/task_scheduler_internals/index.js:44: data.innerHTML = bucket.count; nit: .innerHTML -> .textContent https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/resource... chrome/browser/resources/task_scheduler_internals/index.js:57: $('status').innerHTML = nit: .innerHTML -> .textContent
https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/resource... File chrome/browser/resources/task_scheduler_internals/index.css (right): https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/resource... chrome/browser/resources/task_scheduler_internals/index.css:17: #details { On 2016/10/19 17:49:57, xiyuan wrote: > nit: get rid of this and use hidden > > in html: > <div id="details" hidden> > > in js: > $('details').hidden = !data.instantiated Done. https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/resource... File chrome/browser/resources/task_scheduler_internals/index.js (right): https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/resource... chrome/browser/resources/task_scheduler_internals/index.js:16: title.innerHTML = histogram.name; On 2016/10/19 17:49:57, xiyuan wrote: > nit: title.textContent = histogram.name; > > avoid using innerHTML when we can. Done. https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/resource... chrome/browser/resources/task_scheduler_internals/index.js:23: unavailable.innerHTML = 'No Data Recorded'; On 2016/10/19 17:49:57, xiyuan wrote: > nit: .innerHTML -> .textContent Done. https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/resource... chrome/browser/resources/task_scheduler_internals/index.js:41: header.innerHTML = bucket.min + '-' + bucket.max; On 2016/10/19 17:49:57, xiyuan wrote: > nit: .innerHTML -> .textContent Done. https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/resource... chrome/browser/resources/task_scheduler_internals/index.js:44: data.innerHTML = bucket.count; On 2016/10/19 17:49:57, xiyuan wrote: > nit: .innerHTML -> .textContent Done. https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/resource... chrome/browser/resources/task_scheduler_internals/index.js:57: $('status').innerHTML = On 2016/10/19 17:49:57, xiyuan wrote: > nit: .innerHTML -> .textContent Done.
Patchset #5 (id:100001) has been deleted
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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
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...
https://codereview.chromium.org/2420973002/diff/1/chrome/browser/ui/webui/tas... File chrome/browser/ui/webui/task_scheduler_internals/task_scheduler_internals_ui.cc (right): https://codereview.chromium.org/2420973002/diff/1/chrome/browser/ui/webui/tas... chrome/browser/ui/webui/task_scheduler_internals/task_scheduler_internals_ui.cc:47: return std::move(values); On 2016/10/17 22:36:57, robliao wrote: > On 2016/10/14 20:59:06, fdoray wrote: > > no move required when returning a local variable > > > > > https://www.chromium.org/rvalue-references#TOC-11.-Don-t-return-references.-D.... > > > > http://stackoverflow.com/a/14856553 > > Done. It appears this is actually needed! https://www.chromium.org/rvalue-references#TOC-11.-Don-t-return-references.-D.... There's an exception for returning derived types below.
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
The patchset sent to the CQ was uploaded after l-g-t-m from fdoray@chromium.org, gab@chromium.org, xiyuan@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/2420973002/#ps140001 (title: "The Return of std::move")
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 ========== Plumb Task Scheduler Histograms to the Task Scheduler Internals Page This sets up the plumbing to get data from the Task Scheduler to the internals page. The first target are some selected histograms. BUG=553459 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Plumb Task Scheduler Histograms to the Task Scheduler Internals Page This sets up the plumbing to get data from the Task Scheduler to the internals page. The first target are some selected histograms. BUG=553459 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Plumb Task Scheduler Histograms to the Task Scheduler Internals Page This sets up the plumbing to get data from the Task Scheduler to the internals page. The first target are some selected histograms. BUG=553459 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Plumb Task Scheduler Histograms to the Task Scheduler Internals Page This sets up the plumbing to get data from the Task Scheduler to the internals page. The first target are some selected histograms. BUG=553459 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/6f59a9ad1c00c218de6239ae17e8f0987fa589d7 Cr-Commit-Position: refs/heads/master@{#426520} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/6f59a9ad1c00c218de6239ae17e8f0987fa589d7 Cr-Commit-Position: refs/heads/master@{#426520}
Message was sent while issue was closed.
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
Message was sent while issue was closed.
sorry, i had some comments (that might be stale now), but i took time to write them a while ago... so if anything is worthwhile maybe address in a followup (now that you're landed) https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/resource... File chrome/browser/resources/task_scheduler_internals/index.css (right): https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/resource... chrome/browser/resources/task_scheduler_internals/index.css:18: visibility: hidden; can we just use the [hidden] attribute in HTML instead? https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/resource... File chrome/browser/resources/task_scheduler_internals/index.js (right): https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/resource... chrome/browser/resources/task_scheduler_internals/index.js:5: var TaskSchedulerInternals = {}; nit: this could be of the format var TaskSchedulerInternals = { updateHistograms: function(...) {...}, }; https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/resource... chrome/browser/resources/task_scheduler_internals/index.js:6: can we get a little more specific? /** @typedef {min: number, max: number, count: number} */ var Bucket; /** @typedef {name: string, buckets: !Array<Bucket>} */ var Histogram; https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/resource... chrome/browser/resources/task_scheduler_internals/index.js:9: * @param {Array<Object>} histograms Array of histogram objects. nit: !Array<!Histogram> https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/resource... chrome/browser/resources/task_scheduler_internals/index.js:16: title.innerHTML = histogram.name; all of these .innerHTML setters could actually use .textContent instead, right? https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/resource... chrome/browser/resources/task_scheduler_internals/index.js:41: header.innerHTML = bucket.min + '-' + bucket.max; btw, we can now use `template replacements`, so this could be: `${bucket.min}-${bucket.max}` if you want https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/resource... chrome/browser/resources/task_scheduler_internals/index.js:59: $('details').style.visibility = data.instantiated ? 'visible' : 'hidden'; $('details').hidden = !data.instantiated; https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/task_scheduler_internals/task_scheduler_internals_ui.cc (right): https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/task_scheduler_internals/task_scheduler_internals_ui.cc:85: web_ui()->CallJavascriptFunctionUnsafe( nit: can you use the safe version? in your case i think it'd just be AllowJavascript(); CallJavascriptFunction(...);
Message was sent while issue was closed.
On 2016/10/29 01:13:24, Dan Beam wrote: > sorry, i had some comments (that might be stale now), but i took time to write > them a while ago... so if anything is worthwhile maybe address in a followup > (now that you're landed) > > https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/resource... > File chrome/browser/resources/task_scheduler_internals/index.css (right): > > https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/resource... > chrome/browser/resources/task_scheduler_internals/index.css:18: visibility: > hidden; > can we just use the [hidden] attribute in HTML instead? > > https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/resource... > File chrome/browser/resources/task_scheduler_internals/index.js (right): > > https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/resource... > chrome/browser/resources/task_scheduler_internals/index.js:5: var > TaskSchedulerInternals = {}; > nit: this could be of the format > > var TaskSchedulerInternals = { > updateHistograms: function(...) {...}, > }; > > https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/resource... > chrome/browser/resources/task_scheduler_internals/index.js:6: > can we get a little more specific? > > /** @typedef {min: number, max: number, count: number} */ > var Bucket; > > /** @typedef {name: string, buckets: !Array<Bucket>} */ > var Histogram; > > https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/resource... > chrome/browser/resources/task_scheduler_internals/index.js:9: * @param > {Array<Object>} histograms Array of histogram objects. > nit: !Array<!Histogram> > > https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/resource... > chrome/browser/resources/task_scheduler_internals/index.js:16: title.innerHTML = > histogram.name; > all of these .innerHTML setters could actually use .textContent instead, right? > > https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/resource... > chrome/browser/resources/task_scheduler_internals/index.js:41: header.innerHTML > = bucket.min + '-' + bucket.max; > btw, we can now use `template replacements`, so this could be: > > `${bucket.min}-${bucket.max}` > > if you want > > https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/resource... > chrome/browser/resources/task_scheduler_internals/index.js:59: > $('details').style.visibility = data.instantiated ? 'visible' : 'hidden'; > $('details').hidden = !data.instantiated; > > https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/ui/webui... > File > chrome/browser/ui/webui/task_scheduler_internals/task_scheduler_internals_ui.cc > (right): > > https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/ui/webui... > chrome/browser/ui/webui/task_scheduler_internals/task_scheduler_internals_ui.cc:85: > web_ui()->CallJavascriptFunctionUnsafe( > nit: can you use the safe version? in your case i think it'd just be > > AllowJavascript(); > CallJavascriptFunction(...); Thanks! I'll set this up in a follow up CL.
Message was sent while issue was closed.
https://codereview.chromium.org/2484043004/ now tracks the CR changes. https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/resource... File chrome/browser/resources/task_scheduler_internals/index.css (right): https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/resource... chrome/browser/resources/task_scheduler_internals/index.css:18: visibility: hidden; On 2016/10/29 01:13:24, Dan Beam wrote: > can we just use the [hidden] attribute in HTML instead? On Trunk: Hidden attribute used at commit. https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/resource... File chrome/browser/resources/task_scheduler_internals/index.js (right): https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/resource... chrome/browser/resources/task_scheduler_internals/index.js:5: var TaskSchedulerInternals = {}; On 2016/10/29 01:13:24, Dan Beam wrote: > nit: this could be of the format > > var TaskSchedulerInternals = { > updateHistograms: function(...) {...}, > }; Done. https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/resource... chrome/browser/resources/task_scheduler_internals/index.js:6: On 2016/10/29 01:13:24, Dan Beam wrote: > can we get a little more specific? > > /** @typedef {min: number, max: number, count: number} */ > var Bucket; > > /** @typedef {name: string, buckets: !Array<Bucket>} */ > var Histogram; Done. https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/resource... chrome/browser/resources/task_scheduler_internals/index.js:9: * @param {Array<Object>} histograms Array of histogram objects. On 2016/10/29 01:13:24, Dan Beam wrote: > nit: !Array<!Histogram> Done. https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/resource... chrome/browser/resources/task_scheduler_internals/index.js:16: title.innerHTML = histogram.name; On 2016/10/29 01:13:24, Dan Beam wrote: > all of these .innerHTML setters could actually use .textContent instead, right? On trunk: textContent was used at commit time. https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/resource... chrome/browser/resources/task_scheduler_internals/index.js:41: header.innerHTML = bucket.min + '-' + bucket.max; On 2016/10/29 01:13:24, Dan Beam wrote: > btw, we can now use `template replacements`, so this could be: > > `${bucket.min}-${bucket.max}` > > if you want Done. https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/resource... chrome/browser/resources/task_scheduler_internals/index.js:59: $('details').style.visibility = data.instantiated ? 'visible' : 'hidden'; On 2016/10/29 01:13:24, Dan Beam wrote: > $('details').hidden = !data.instantiated; On trunk: This was done at commit time. https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/task_scheduler_internals/task_scheduler_internals_ui.cc (right): https://codereview.chromium.org/2420973002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/task_scheduler_internals/task_scheduler_internals_ui.cc:85: web_ui()->CallJavascriptFunctionUnsafe( On 2016/10/29 01:13:24, Dan Beam wrote: > nit: can you use the safe version? in your case i think it'd just be > > AllowJavascript(); > CallJavascriptFunction(...); Done. |
