Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(815)

Issue 2747453003: Refactor histogram-set-view to an MVC pattern. (Closed)

Created:
3 years, 9 months ago by benjhayden
Modified:
3 years, 8 months ago
Reviewers:
eakuefner
CC:
catapult-reviews_chromium.org, tracing-review_chromium.org
Target Ref:
refs/heads/master
Project:
catapult
Visibility:
Public.

Description

Refactor histogram-set-view to an MVC pattern. Currently, histogram-set-table contains both the top-controls and the table. The state is tied up in the DOM, which causes unnecessary complexity that causes correctness and performance issues, and precludes deep-linking. This CL removes the top controls from histogram-set-table and adds the new histogram-set-controls element to histogram-set-view. histogram-set-view now coordinates the controls and the table, and handles some behaviors that don't belong in either, like "zero Histograms" and download-csv. histogram-set-table is now only responsible for building the table using HistogramSetTableRow, which builds -table-cells and -name-cells. histogram-set-table now reports more fine-grained progress updates as it builds the table, and uses the same updateContents_ for both the initial build() and subsequent onViewStateUpdate_(). A followup CL will reorganize/delete the bad old histogram-set-table tests in favor of the new view/control tests. The bad old ones are kept in this CL to demonstrate that this CL doesn't break them. http://www/~benjhayden/2747453003-before.html http://www/~benjhayden/2747453003-after.html BUG=catapult:#3289 Review-Url: https://codereview.chromium.org/2747453003 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/ff1708abdd475e0973aab73e707fd13e41b63c7c

Patch Set 1 : #

Patch Set 2 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+2339 lines, -1765 lines) Patch
M tracing/trace_viewer.gypi View 1 4 chunks +6 lines, -0 lines 0 comments Download
M tracing/tracing/base/iteration_helpers.html View 1 1 chunk +5 lines, -1 line 0 comments Download
M tracing/tracing/base/view_state.html View 1 1 chunk +6 lines, -2 lines 0 comments Download
M tracing/tracing/ui/side_panel/metrics_side_panel.html View 4 chunks +12 lines, -5 lines 0 comments Download
M tracing/tracing/ui/side_panel/metrics_side_panel_test.html View 1 chunk +3 lines, -3 lines 0 comments Download
M tracing/tracing/value/histogram_importer.html View 3 chunks +23 lines, -24 lines 0 comments Download
A tracing/tracing/value/histogram_parameter_collector.html View 1 1 chunk +135 lines, -0 lines 0 comments Download
A tracing/tracing/value/histogram_parameter_collector_test.html View 1 1 chunk +25 lines, -0 lines 0 comments Download
A tracing/tracing/value/histogram_set_hierarchy.html View 1 1 chunk +317 lines, -0 lines 0 comments Download
A tracing/tracing/value/ui/histogram-set-view.md View 1 1 chunk +71 lines, -0 lines 0 comments Download
M tracing/tracing/value/ui/histogram_set_controls.html View 1 7 chunks +70 lines, -25 lines 0 comments Download
M tracing/tracing/value/ui/histogram_set_controls_test.html View 15 chunks +51 lines, -55 lines 0 comments Download
M tracing/tracing/value/ui/histogram_set_table.html View 1 4 chunks +262 lines, -706 lines 0 comments Download
M tracing/tracing/value/ui/histogram_set_table_cell.html View 1 11 chunks +132 lines, -78 lines 0 comments Download
M tracing/tracing/value/ui/histogram_set_table_name_cell.html View 1 7 chunks +126 lines, -57 lines 0 comments Download
M tracing/tracing/value/ui/histogram_set_table_row.html View 1 4 chunks +123 lines, -316 lines 0 comments Download
M tracing/tracing/value/ui/histogram_set_table_test.html View 1 26 chunks +793 lines, -472 lines 0 comments Download
M tracing/tracing/value/ui/histogram_set_view.html View 1 4 chunks +93 lines, -12 lines 0 comments Download
M tracing/tracing/value/ui/histogram_set_view_state.html View 2 chunks +3 lines, -3 lines 0 comments Download
M tracing/tracing/value/ui/histogram_set_view_test.html View 2 chunks +44 lines, -3 lines 0 comments Download
M tracing/tracing/value/ui/histogram_span.html View 5 chunks +39 lines, -3 lines 0 comments Download

Messages

Total messages: 119 (110 generated)
benjhayden
I think this is finally ready. Sorry it's huge. I think it's about as small ...
3 years, 8 months ago (2017-04-03 04:06:46 UTC) #88
eakuefner
Happy to take a look at this tomorrow or Tuesday. Maybe we can have Simon ...
3 years, 8 months ago (2017-04-03 04:18:53 UTC) #89
eakuefner
lgtm
3 years, 8 months ago (2017-04-04 20:26:04 UTC) #96
benjhayden
When I profiled the after.html linked in the CL description, I noticed that filtering Rows ...
3 years, 8 months ago (2017-04-05 04:29:48 UTC) #98
benjhayden
On 2017/04/05 at 04:29:48, benjhayden wrote: > When I profiled the after.html linked in the ...
3 years, 8 months ago (2017-04-05 22:34:15 UTC) #99
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2747453003/1740001
3 years, 8 months ago (2017-04-06 16:24:13 UTC) #108
commit-bot: I haz the power
Try jobs failed on following builders: Catapult Presubmit on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Presubmit/builds/6796)
3 years, 8 months ago (2017-04-06 16:27:04 UTC) #110
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2747453003/1780001
3 years, 8 months ago (2017-04-06 23:00:09 UTC) #116
commit-bot: I haz the power
3 years, 8 months ago (2017-04-06 23:24:02 UTC) #119
Message was sent while issue was closed.
Committed patchset #2 (id:1780001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...

Powered by Google App Engine
This is Rietveld 408576698