Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2881193003:
Add a button to chart-title to populate the test-picker. (Closed)
|
Created:
3 years, 7 months ago by benjhayden Modified:
3 years, 7 months ago CC:
catapult-reviews_chromium.org, perf-dashboard-reviews_chromium.org Target Ref:
refs/heads/master Project:
catapult Visibility:
Public. |
DescriptionAdd a button to chart-title to populate the test-picker.
https://dev-benjhayden-7b9c4420-dot-chromeperf.appspot.com/report?sid=ca6c52111f8337a0f6478573cd639ed39dbd8eb82d903d8c3932651f452b9fb9
http://i.imgur.com/BAFCy8r.png
BUG=catapult:#2641
Review-Url: https://codereview.chromium.org/2881193003
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/621c724fe4f9788003cd88b4009eabc6eae408c4
Patch Set 1 #
Total comments: 6
Patch Set 2 : comments #Patch Set 3 : new button #Patch Set 4 : #
Total comments: 1
Patch Set 5 : comments #
Messages
Total messages: 23 (10 generated)
Description was changed from ========== Populate test-picker on chart-title click. BUG=catapult:#2641 ========== to ========== Populate test-picker on chart-title click. https://dev-benjhayden-8c558205-dot-chromeperf.appspot.com/report?sid=1008595... BUG=catapult:#2641 ==========
benjhayden@chromium.org changed reviewers: + eakuefner@chromium.org, simonhatch@chromium.org
PTAL
https://codereview.chromium.org/2881193003/diff/1/dashboard/dashboard/element... File dashboard/dashboard/elements/report-page.html (right): https://codereview.chromium.org/2881193003/diff/1/dashboard/dashboard/element... dashboard/dashboard/elements/report-page.html:135: const testPaths = chartTitle.getTestPaths(); Can you write some comments that explain why this stanza is here? I'm specifically unfamiliar with chartTitle.getTestPaths, so starting with clarifying why we call that here would help. https://codereview.chromium.org/2881193003/diff/1/dashboard/dashboard/element... dashboard/dashboard/elements/report-page.html:147: for (let itemIndex = 0; itemIndex < suiteMenu.items.length; ++itemIndex) { Could you write this as a for..of loop?
sullivan@chromium.org changed reviewers: + sullivan@chromium.org
I think with the method I suggested below, it would be easier to add tests (you would test the chart title click handling separately from the testpicker updating). https://codereview.chromium.org/2881193003/diff/1/dashboard/dashboard/element... File dashboard/dashboard/elements/report-page.html (right): https://codereview.chromium.org/2881193003/diff/1/dashboard/dashboard/element... dashboard/dashboard/elements/report-page.html:133: if (chartTitle.tagName !== 'CHART-TITLE') return; The event handling here is really different than the normal style for the perf dashboard. The perf dashboard uses polymer's annotated event handling: https://www.polymer-project.org/1.0/docs/devguide/events#annotated-listeners So we would normally set it up like this: 1) In chart-title.html, update the existing onClick method to do the parsing to get the suite, bot, and subtests the testpicker should have and fire an event with the information (https://www.polymer-project.org/1.0/docs/api/Polymer.Base#method-fire) 2) In testpicker, handle the event and set the menu as requested. I prefer this convention because: * It's clearer where the code that handles events on <chart-title> lives * The code that relates to chart title parsing lives in <chart-title> * The code that updates the testpicker menu lives in <test-picker>
Description was changed from ========== Populate test-picker on chart-title click. https://dev-benjhayden-8c558205-dot-chromeperf.appspot.com/report?sid=1008595... BUG=catapult:#2641 ========== to ========== Populate test-picker on chart-title click. https://dev-benjhayden-d3a2117d-dot-chromeperf.appspot.com/report?sid=baa27b7... BUG=catapult:#2641 ==========
Patchset #2 (id:20001) has been deleted
PTAL Sorry I didn't know the right way to fix #2641. :-( Sorry I have dumb design questions below. :-( I made report-page listen to an event that chart-title fires named "titleclicked" when the user clicks on the enabled link in chart-title, but that seems to have caused complications. It looks like chart-title can contain either * a single disabled link containing the text of a suite-level path, OR * a single enabled link containing the text of a suite-level path PLUS a disabled link containing the text of the rest of the test path. It looks like chart-title doesn't fire titleclicked when the user clicks on the last component of the title. It looks like, when the user clicks on the enabled suite link in chart-title, the chart forgets its test path, stops displaying data, and lists all subtests under the test suite in the legend. So now there's no way to auto-populate the test picker without trashing a chart. Should auto-populating the test-picker be controlled by a new button in test-picker? Is the current behavior when clicking the links in chart-title useful? https://codereview.chromium.org/2881193003/diff/1/dashboard/dashboard/element... File dashboard/dashboard/elements/report-page.html (right): https://codereview.chromium.org/2881193003/diff/1/dashboard/dashboard/element... dashboard/dashboard/elements/report-page.html:133: if (chartTitle.tagName !== 'CHART-TITLE') return; On 2017/05/16 at 13:35:13, sullivan wrote: > The event handling here is really different than the normal style for the perf dashboard. The perf dashboard uses polymer's annotated event handling: https://www.polymer-project.org/1.0/docs/devguide/events#annotated-listeners > > So we would normally set it up like this: > > 1) In chart-title.html, update the existing onClick method to do the parsing to get the suite, bot, and subtests the testpicker should have and fire an event with the information (https://www.polymer-project.org/1.0/docs/api/Polymer.Base#method-fire) > 2) In testpicker, handle the event and set the menu as requested. > > I prefer this convention because: > * It's clearer where the code that handles events on <chart-title> lives > * The code that relates to chart title parsing lives in <chart-title> > * The code that updates the testpicker menu lives in <test-picker> Done. https://codereview.chromium.org/2881193003/diff/1/dashboard/dashboard/element... dashboard/dashboard/elements/report-page.html:135: const testPaths = chartTitle.getTestPaths(); On 2017/05/16 at 02:08:53, eakuefner wrote: > Can you write some comments that explain why this stanza is here? I'm specifically unfamiliar with chartTitle.getTestPaths, so starting with clarifying why we call that here would help. Changed to use event.titleParts. https://codereview.chromium.org/2881193003/diff/1/dashboard/dashboard/element... dashboard/dashboard/elements/report-page.html:147: for (let itemIndex = 0; itemIndex < suiteMenu.items.length; ++itemIndex) { On 2017/05/16 at 02:08:52, eakuefner wrote: > Could you write this as a for..of loop? Done.
Sorry I dove into the code so fast! Can we move discussion about what the UI should look like back to the bug?
Description was changed from ========== Populate test-picker on chart-title click. https://dev-benjhayden-d3a2117d-dot-chromeperf.appspot.com/report?sid=baa27b7... BUG=catapult:#2641 ========== to ========== Add a button to chart-title to populate the test-picker. https://dev-benjhayden-a077e9f7-dot-chromeperf.appspot.com/report?sid=ca6c521... BUG=catapult:#2641 ==========
PTAL
Really like the new button, works nicely. https://codereview.chromium.org/2881193003/diff/80001/dashboard/dashboard/ele... File dashboard/dashboard/elements/report-page.html (right): https://codereview.chromium.org/2881193003/diff/80001/dashboard/dashboard/ele... dashboard/dashboard/elements/report-page.html:128: async populateTestPicker_(event) { I think it'd be cleaner if you just had chart-title do the parsing, and have test picker handle the event instead of this conversion step happening in report-page.
On 2017/05/19 19:43:53, shatch wrote: > Really like the new button, works nicely. > > https://codereview.chromium.org/2881193003/diff/80001/dashboard/dashboard/ele... > File dashboard/dashboard/elements/report-page.html (right): > > https://codereview.chromium.org/2881193003/diff/80001/dashboard/dashboard/ele... > dashboard/dashboard/elements/report-page.html:128: async > populateTestPicker_(event) { > I think it'd be cleaner if you just had chart-title do the parsing, and have > test picker handle the event instead of this conversion step happening in > report-page. +1
Description was changed from ========== Add a button to chart-title to populate the test-picker. https://dev-benjhayden-a077e9f7-dot-chromeperf.appspot.com/report?sid=ca6c521... BUG=catapult:#2641 ========== to ========== Add a button to chart-title to populate the test-picker. https://dev-benjhayden-7b9c4420-dot-chromeperf.appspot.com/report?sid=ca6c521... http://i.imgur.com/BAFCy8r.png BUG=catapult:#2641 ==========
On 2017/05/19 at 19:43:53, simonhatch wrote: > Really like the new button, works nicely. > > https://codereview.chromium.org/2881193003/diff/80001/dashboard/dashboard/ele... > File dashboard/dashboard/elements/report-page.html (right): > > https://codereview.chromium.org/2881193003/diff/80001/dashboard/dashboard/ele... > dashboard/dashboard/elements/report-page.html:128: async populateTestPicker_(event) { > I think it'd be cleaner if you just had chart-title do the parsing, and have test picker handle the event instead of this conversion step happening in report-page. As discussed offline, I moved the test-path restructuring to chart-title, but the test picker doesn't receive the event because it isn't in the chart-title's dom hierarchy.
On 2017/05/25 at 19:21:02, benjhayden wrote: > On 2017/05/19 at 19:43:53, simonhatch wrote: > > Really like the new button, works nicely. > > > > https://codereview.chromium.org/2881193003/diff/80001/dashboard/dashboard/ele... > > File dashboard/dashboard/elements/report-page.html (right): > > > > https://codereview.chromium.org/2881193003/diff/80001/dashboard/dashboard/ele... > > dashboard/dashboard/elements/report-page.html:128: async populateTestPicker_(event) { > > I think it'd be cleaner if you just had chart-title do the parsing, and have test picker handle the event instead of this conversion step happening in report-page. > > As discussed offline, I moved the test-path restructuring to chart-title, but the test picker doesn't receive the event because it isn't in the chart-title's dom hierarchy. PTAL, still need lgtms.
lgtm
The CQ bit was checked by benjhayden@chromium.org
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": 100001, "attempt_start_ts": 1495751048443340,
"parent_rev": "43f23c5d1d94df7282984a968e14731d6f7c9dd3", "commit_rev":
"621c724fe4f9788003cd88b4009eabc6eae408c4"}
Message was sent while issue was closed.
Description was changed from ========== Add a button to chart-title to populate the test-picker. https://dev-benjhayden-7b9c4420-dot-chromeperf.appspot.com/report?sid=ca6c521... http://i.imgur.com/BAFCy8r.png BUG=catapult:#2641 ========== to ========== Add a button to chart-title to populate the test-picker. https://dev-benjhayden-7b9c4420-dot-chromeperf.appspot.com/report?sid=ca6c521... http://i.imgur.com/BAFCy8r.png BUG=catapult:#2641 Review-Url: https://codereview.chromium.org/2881193003 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
