|
|
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. |
DescriptionRefactor 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 : #Messages
Total messages: 119 (110 generated)
Description was changed from ========== Refactor HistogramSetTable into MVC. BUG=catapult:#3289 ========== to ========== Refactor histogram-set-table to MVC. Currently, histogram-set-table contains both the top-controls and the table. The configuration is scattered throughout the various UI elements: * searchQuery * referenceDisplayLabel * displayStatistic * showAll * fullNameColumn * sortColumnIndex * sortDescending * tableState * expanded rows * open cells * brushed bins * merged sample diagnostics Whenever any configuration changes, the histogram-set-table must be very careful to react to it without losing any other configuration or re-computing too much. The change handlers have grown organically to the point where it is difficult to make changes without breaking something. The scattered configuration also precludes deep-linking. This CL collects all of the configuration into a new HistogramSetConfiguration class, which is an MVC Model. Whenever any field is changed, it calls listener.onConfigurationChanged(delta) on all of its listeners (MVC Views/Controllers). This CL moves the top controls to a new dom-module, histogram-set-controls. This module is an MVC View-Controller for the top half of the Configuration fields. The controls are moved out of histogram-set-table to histogram-set-view. histogram-set-view is now responsible for setting up the controls and the table, and handling some stuff that doesn'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 is now an MVC Controller for the bottom half of the Configuration fields, and a View for all of them. A followup CL will add another View-Controller to mirror the Configuration to/from the URL hash, possibly using app-route or app-state. BUG=catapult:#3289 ==========
Description was changed from ========== Refactor histogram-set-table to MVC. Currently, histogram-set-table contains both the top-controls and the table. The configuration is scattered throughout the various UI elements: * searchQuery * referenceDisplayLabel * displayStatistic * showAll * fullNameColumn * sortColumnIndex * sortDescending * tableState * expanded rows * open cells * brushed bins * merged sample diagnostics Whenever any configuration changes, the histogram-set-table must be very careful to react to it without losing any other configuration or re-computing too much. The change handlers have grown organically to the point where it is difficult to make changes without breaking something. The scattered configuration also precludes deep-linking. This CL collects all of the configuration into a new HistogramSetConfiguration class, which is an MVC Model. Whenever any field is changed, it calls listener.onConfigurationChanged(delta) on all of its listeners (MVC Views/Controllers). This CL moves the top controls to a new dom-module, histogram-set-controls. This module is an MVC View-Controller for the top half of the Configuration fields. The controls are moved out of histogram-set-table to histogram-set-view. histogram-set-view is now responsible for setting up the controls and the table, and handling some stuff that doesn'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 is now an MVC Controller for the bottom half of the Configuration fields, and a View for all of them. A followup CL will add another View-Controller to mirror the Configuration to/from the URL hash, possibly using app-route or app-state. BUG=catapult:#3289 ========== to ========== Refactor histogram-set-table to MVC. Currently, histogram-set-table contains both the top-controls and the table. The configuration is scattered throughout the various UI elements: * searchQuery * referenceDisplayLabel * displayStatistic * showAll * fullNameColumn * sortColumnIndex * sortDescending * tableState * expanded rows * open cells * brushed bins * merged sample diagnostics Whenever any configuration changes, the histogram-set-table must be very careful to react to it without losing any other configuration or re-computing too much. The change handlers have grown organically to the point where it is difficult to make changes without breaking something. The scattered configuration also precludes deep-linking. This CL collects all of the configuration into a new HistogramSetConfiguration class, which is an MVC Model. Whenever any field is changed, it calls listener.onConfigurationChanged(delta) on all of its listeners (MVC Views/Controllers). This CL moves the top controls to a new dom-module, histogram-set-controls. This module is an MVC View-Controller for the top half of the Configuration fields. The controls are moved out of histogram-set-table to histogram-set-view. histogram-set-view is now responsible for setting up the controls and the table, and handling some stuff that doesn'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 is now an MVC Controller for the bottom half of the Configuration fields, and a View for all of them. A followup CL will enable deep-linking by adding another View-Controller to mirror the Configuration to/from the URL hash, possibly using app-route or app-state. BUG=catapult:#3289 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Patchset #1 (id:140001) has been deleted
Patchset #1 (id:160001) has been deleted
Patchset #2 (id:200001) has been deleted
Patchset #2 (id:220001) has been deleted
Patchset #2 (id:240001) has been deleted
Patchset #1 (id:180001) has been deleted
Description was changed from ========== Refactor histogram-set-table to MVC. Currently, histogram-set-table contains both the top-controls and the table. The configuration is scattered throughout the various UI elements: * searchQuery * referenceDisplayLabel * displayStatistic * showAll * fullNameColumn * sortColumnIndex * sortDescending * tableState * expanded rows * open cells * brushed bins * merged sample diagnostics Whenever any configuration changes, the histogram-set-table must be very careful to react to it without losing any other configuration or re-computing too much. The change handlers have grown organically to the point where it is difficult to make changes without breaking something. The scattered configuration also precludes deep-linking. This CL collects all of the configuration into a new HistogramSetConfiguration class, which is an MVC Model. Whenever any field is changed, it calls listener.onConfigurationChanged(delta) on all of its listeners (MVC Views/Controllers). This CL moves the top controls to a new dom-module, histogram-set-controls. This module is an MVC View-Controller for the top half of the Configuration fields. The controls are moved out of histogram-set-table to histogram-set-view. histogram-set-view is now responsible for setting up the controls and the table, and handling some stuff that doesn'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 is now an MVC Controller for the bottom half of the Configuration fields, and a View for all of them. A followup CL will enable deep-linking by adding another View-Controller to mirror the Configuration to/from the URL hash, possibly using app-route or app-state. BUG=catapult:#3289 ========== to ========== Refactor histogram-set-view/table to use HistogramSetConfiguration. Currently, histogram-set-table contains both the top-controls and the table. Whenever any configuration changes, the histogram-set-table is very careful to react to it without losing any other configuration or re-computing too much. The change handlers have grown organically to the point where it is difficult to make changes without breaking something. The scattered configuration also 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 is now responsible for setting up the controls and the table, and handling some stuff that doesn'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 is now an MVC Controller for the bottom half of the Configuration fields, and a View for all of them. histogram-set-table now reports more fine-grained progress updates as it builds the table, and uses the same updateContents_ for both building and responding to any configuration changes. A followup CL will enable deep-linking by adding another View-Controller to mirror the Configuration to/from the URL hash, possibly using app-route or app-state. BUG=catapult:#3289 ==========
Patchset #1 (id:260001) has been deleted
Description was changed from ========== Refactor histogram-set-view/table to use HistogramSetConfiguration. Currently, histogram-set-table contains both the top-controls and the table. Whenever any configuration changes, the histogram-set-table is very careful to react to it without losing any other configuration or re-computing too much. The change handlers have grown organically to the point where it is difficult to make changes without breaking something. The scattered configuration also 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 is now responsible for setting up the controls and the table, and handling some stuff that doesn'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 is now an MVC Controller for the bottom half of the Configuration fields, and a View for all of them. histogram-set-table now reports more fine-grained progress updates as it builds the table, and uses the same updateContents_ for both building and responding to any configuration changes. A followup CL will enable deep-linking by adding another View-Controller to mirror the Configuration to/from the URL hash, possibly using app-route or app-state. BUG=catapult:#3289 ========== to ========== Refactor histogram-set-view/table to use HistogramSetConfiguration. Currently, histogram-set-table contains both the top-controls and the table. Whenever any configuration changes, the histogram-set-table is very careful to react to it without losing any other configuration or re-computing too much. The change handlers have grown organically to the point where it is difficult to make changes without breaking something. The scattered configuration also 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 is now responsible for setting up the controls and the table, and handling some stuff that doesn'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 is now a View-Controller for the bottom half of the Configuration fields. histogram-set-table now reports more fine-grained progress updates as it builds the table, and uses the same updateContents_ for both building and responding to any configuration changes. A followup CL will enable deep-linking by adding another View-Controller to mirror the Configuration to/from the URL hash, possibly using app-route or app-state. BUG=catapult:#3289 ==========
Patchset #1 (id:280001) has been deleted
Patchset #1 (id:300001) has been deleted
Patchset #1 (id:320001) has been deleted
Patchset #1 (id:340001) has been deleted
Description was changed from ========== Refactor histogram-set-view/table to use HistogramSetConfiguration. Currently, histogram-set-table contains both the top-controls and the table. Whenever any configuration changes, the histogram-set-table is very careful to react to it without losing any other configuration or re-computing too much. The change handlers have grown organically to the point where it is difficult to make changes without breaking something. The scattered configuration also 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 is now responsible for setting up the controls and the table, and handling some stuff that doesn'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 is now a View-Controller for the bottom half of the Configuration fields. histogram-set-table now reports more fine-grained progress updates as it builds the table, and uses the same updateContents_ for both building and responding to any configuration changes. A followup CL will enable deep-linking by adding another View-Controller to mirror the Configuration to/from the URL hash, possibly using app-route or app-state. BUG=catapult:#3289 ========== to ========== Refactor histogram-set-view to an MVC pattern. Currently, histogram-set-table contains both the top-controls and the table. Whenever any configuration changes, the histogram-set-table is very careful to react to it without losing any other configuration or re-computing too much. The change handlers have grown organically to the point where it is difficult to make changes without breaking something. The scattered configuration also 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 is now responsible for setting up the controls and the table, and handling some stuff that doesn'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 is now a View-Controller for the bottom half of the Configuration fields. histogram-set-table now reports more fine-grained progress updates as it builds the table, and uses the same updateContents_ for both building and responding to any configuration changes. A followup CL will enable deep-linking by adding another View-Controller to mirror the Configuration to/from the URL hash, possibly using app-route or app-state. BUG=catapult:#3289 ==========
Patchset #1 (id:360001) has been deleted
Patchset #1 (id:380001) has been deleted
Patchset #1 (id:400001) has been deleted
Patchset #1 (id:420001) has been deleted
Patchset #1 (id:440001) has been deleted
Patchset #1 (id:460001) has been deleted
Patchset #1 (id:480001) has been deleted
Patchset #1 (id:500001) has been deleted
Patchset #1 (id:520001) has been deleted
Patchset #1 (id:540001) has been deleted
Patchset #1 (id:560001) has been deleted
Description was changed from ========== Refactor histogram-set-view to an MVC pattern. Currently, histogram-set-table contains both the top-controls and the table. Whenever any configuration changes, the histogram-set-table is very careful to react to it without losing any other configuration or re-computing too much. The change handlers have grown organically to the point where it is difficult to make changes without breaking something. The scattered configuration also 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 is now responsible for setting up the controls and the table, and handling some stuff that doesn'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 is now a View-Controller for the bottom half of the Configuration fields. histogram-set-table now reports more fine-grained progress updates as it builds the table, and uses the same updateContents_ for both building and responding to any configuration changes. A followup CL will enable deep-linking by adding another View-Controller to mirror the Configuration to/from the URL hash, possibly using app-route or app-state. BUG=catapult:#3289 ========== to ========== Refactor histogram-set-view to an MVC pattern. Currently, histogram-set-table contains both the top-controls and the table. Whenever any configuration changes, the histogram-set-table is very careful to react to it without losing any other configuration or re-computing too much. The change handlers have grown organically to the point where it is difficult to make changes without breaking something. The scattered configuration also 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 is now responsible for setting up the controls and the table, and handling some stuff that doesn'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 is now a View-Controller for the bottom half of the Configuration fields. histogram-set-table now reports more fine-grained progress updates as it builds the table, and uses the same updateContents_ for both building and responding to any configuration changes. A followup CL will enable deep-linking by adding another View-Controller to mirror the Configuration to/from the URL hash, possibly using app-route or app-state. Another followup CL will 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. BUG=catapult:#3289 ==========
Patchset #1 (id:580001) has been deleted
Patchset #1 (id:600001) has been deleted
Patchset #1 (id:620001) has been deleted
Patchset #1 (id:640001) has been deleted
Patchset #1 (id:660001) has been deleted
Patchset #1 (id:680001) has been deleted
Patchset #1 (id:700001) has been deleted
Patchset #1 (id:700001) has been deleted
Patchset #1 (id:720001) has been deleted
Patchset #1 (id:720001) has been deleted
Patchset #1 (id:740001) has been deleted
Patchset #1 (id:760001) has been deleted
Patchset #1 (id:780001) has been deleted
Patchset #1 (id:800001) has been deleted
Patchset #1 (id:820001) has been deleted
Patchset #1 (id:840001) has been deleted
Patchset #1 (id:860001) has been deleted
Patchset #1 (id:880001) has been deleted
Patchset #1 (id:900001) has been deleted
Description was changed from ========== Refactor histogram-set-view to an MVC pattern. Currently, histogram-set-table contains both the top-controls and the table. Whenever any configuration changes, the histogram-set-table is very careful to react to it without losing any other configuration or re-computing too much. The change handlers have grown organically to the point where it is difficult to make changes without breaking something. The scattered configuration also 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 is now responsible for setting up the controls and the table, and handling some stuff that doesn'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 is now a View-Controller for the bottom half of the Configuration fields. histogram-set-table now reports more fine-grained progress updates as it builds the table, and uses the same updateContents_ for both building and responding to any configuration changes. A followup CL will enable deep-linking by adding another View-Controller to mirror the Configuration to/from the URL hash, possibly using app-route or app-state. Another followup CL will 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. BUG=catapult:#3289 ========== to ========== Refactor histogram-set-view to an MVC pattern. Currently, histogram-set-table contains both the top-controls and the table. Whenever any configuration changes, the histogram-set-table is very careful to react to it without losing any other configuration or re-computing too much. The change handlers have grown organically to the point where it is difficult to make changes without breaking something. The scattered configuration also 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 is now responsible for setting up the controls and the table, and handling some stuff that doesn'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 is now a View-Controller for the bottom half of the Configuration fields. histogram-set-table now reports more fine-grained progress updates as it builds the table, and uses the same updateContents_ for both building and responding to any configuration changes. A followup CL will enable deep-linking by adding another View-Controller to mirror the Configuration to/from the URL hash, possibly using app-route or app-state. Another 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. BUG=catapult:#3289 ==========
Patchset #1 (id:920001) has been deleted
Patchset #1 (id:940001) has been deleted
Patchset #1 (id:960001) has been deleted
Patchset #1 (id:980001) has been deleted
Patchset #1 (id:1000001) has been deleted
Patchset #1 (id:1020001) has been deleted
Patchset #1 (id:1040001) has been deleted
Patchset #1 (id:1060001) has been deleted
Patchset #1 (id:1080001) has been deleted
Patchset #1 (id:1100001) has been deleted
Patchset #1 (id:1120001) has been deleted
Patchset #1 (id:1140001) has been deleted
Patchset #1 (id:1160001) has been deleted
Patchset #1 (id:1180001) has been deleted
Description was changed from ========== Refactor histogram-set-view to an MVC pattern. Currently, histogram-set-table contains both the top-controls and the table. Whenever any configuration changes, the histogram-set-table is very careful to react to it without losing any other configuration or re-computing too much. The change handlers have grown organically to the point where it is difficult to make changes without breaking something. The scattered configuration also 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 is now responsible for setting up the controls and the table, and handling some stuff that doesn'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 is now a View-Controller for the bottom half of the Configuration fields. histogram-set-table now reports more fine-grained progress updates as it builds the table, and uses the same updateContents_ for both building and responding to any configuration changes. A followup CL will enable deep-linking by adding another View-Controller to mirror the Configuration to/from the URL hash, possibly using app-route or app-state. Another 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. BUG=catapult:#3289 ========== to ========== 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 enable deep-linking by adding another View-Controller to mirror the Configuration to/from the URL, possibly using app-route or app-state. Another 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. BUG=catapult:#3289 ==========
Description was changed from ========== 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 enable deep-linking by adding another View-Controller to mirror the Configuration to/from the URL, possibly using app-route or app-state. Another 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. BUG=catapult:#3289 ========== to ========== 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. BUG=catapult:#3289 ==========
Patchset #1 (id:900002) has been deleted
Patchset #1 (id:1210001) has been deleted
Patchset #1 (id:1230001) has been deleted
Patchset #1 (id:1250001) has been deleted
Patchset #1 (id:1270001) has been deleted
Patchset #1 (id:1290001) has been deleted
Patchset #1 (id:1310001) has been deleted
Patchset #1 (id:1330001) has been deleted
Patchset #1 (id:1350001) has been deleted
Patchset #1 (id:1370001) has been deleted
Patchset #1 (id:1390001) has been deleted
Patchset #1 (id:1410001) has been deleted
Patchset #1 (id:1430001) has been deleted
Patchset #1 (id:1450001) has been deleted
Patchset #1 (id:1470001) has been deleted
benjhayden@chromium.org changed reviewers: + eakuefner@chromium.org
I think this is finally ready. Sorry it's huge. I think it's about as small as I can make it. It's a major rewrite, it was always going to be huge. Want to schedule a meeting to look over it? Want to ask anybody else for backup?
Happy to take a look at this tomorrow or Tuesday. Maybe we can have Simon also take a look interactively, since he'll be visiting?
Patchset #2 (id:1510001) has been deleted
Patchset #2 (id:1530001) has been deleted
Patchset #2 (id:1550001) has been deleted
Patchset #2 (id:1570001) has been deleted
Patchset #2 (id:1590001) has been deleted
Patchset #2 (id:1610001) has been deleted
lgtm
Description was changed from ========== 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. BUG=catapult:#3289 ========== to ========== 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 ==========
When I profiled the after.html linked in the CL description, I noticed that filtering Rows was slow because assigning subRows and cells on the cloned rows dispatches an update event. That was defensive code, but it made me realize that the Row instances in unfilteredRows_ never build any cells or use their ViewState, which suggests that they should actually be a different, simpler data structure. I factored the non-UI pieces out of HistogramSetTableRow to a new HistogramSetHierarchy class. This turned out to be surprisingly easy, and should facilitate testing. After histogram-set-table filters HistogramSetHierarchies, it maps them directly to Rows, but Rows don't have to worry about copying cells from the Hierarchies because Hierarchies don't have cells. The ViewState lifetimes are simpler. I'll debug and profile and test this tomorrow. The other performance regression in this CL is a new forced layout in name-cell.attached() -> isOverflowing, which is puzzling because that didn't change. I could push that into a tr.b.animationFrame().then(), but I'll see if I can find a more principled fix tomorrow.
On 2017/04/05 at 04:29:48, benjhayden wrote: > When I profiled the after.html linked in the CL description, I noticed that filtering Rows was slow because assigning subRows and cells on the cloned rows dispatches an update event. > That was defensive code, but it made me realize that the Row instances in unfilteredRows_ never build any cells or use their ViewState, which suggests that they should actually be a different, simpler data structure. > I factored the non-UI pieces out of HistogramSetTableRow to a new HistogramSetHierarchy class. This turned out to be surprisingly easy, and should facilitate testing. > After histogram-set-table filters HistogramSetHierarchies, it maps them directly to Rows, but Rows don't have to worry about copying cells from the Hierarchies because Hierarchies don't have cells. The ViewState lifetimes are simpler. > I'll debug and profile and test this tomorrow. That seemed to work out well. > > The other performance regression in this CL is a new forced layout in name-cell.attached() -> isOverflowing, which is puzzling because that didn't change. > I could push that into a tr.b.animationFrame().then(), but I'll see if I can find a more principled fix tomorrow. The more principled fix turned out to be moving the isOverflowing check from name-cell up into histogram-set-table, which was already doing a similar check in build(). Now the only performance regressions appear to be the few ms in the few new |await update('...')| statements. I'll commit this tomorrow if there are no further comments. Thanks!
Patchset #2 (id:1620001) has been deleted
Patchset #2 (id:1640001) has been deleted
Patchset #2 (id:1660001) has been deleted
Patchset #2 (id:1680001) has been deleted
Patchset #2 (id:1700001) has been deleted
Patchset #2 (id:1700001) has been deleted
The CQ bit was checked by benjhayden@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eakuefner@chromium.org Link to the patchset: https://codereview.chromium.org/2747453003/#ps1740001 (title: "rebase")
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: Catapult Presubmit on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Pr...)
Patchset #3 (id:1740001) has been deleted
Patchset #2 (id:1720001) has been deleted
Patchset #2 (id:1760001) has been deleted
The CQ bit was checked by benjhayden@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eakuefner@chromium.org Link to the patchset: https://codereview.chromium.org/2747453003/#ps1780001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 1780001, "attempt_start_ts": 1491519602771830, "parent_rev": "eb0e9e69a06a9a678a7c65b1bce65f0026777904", "commit_rev": "ff1708abdd475e0973aab73e707fd13e41b63c7c"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/catapu... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:1780001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |