[Dashboard] Start using /list_tests's test_path_dict mode in test-picker

[Dashboard] Start using /list_tests's test_path_dict mode in test-picker

As of this CL, it should be immediately possible to factor away test-picker's
request for the subtest dict in favor of an approach that uses the new endpoint.
The functionality in this CL subsumes report-page's need for the functionality
of test-picker that depended on the subtest dict, and now it is only test-picker
itself that uses the subtest dict (to populate its menus).

In parallel, this CL offers up a couple more avenues for interesting work:
1. Request parallelization. We will need to clean up simple_xhr probably in
   favor of fetch (see
   https://github.com/catapult-project/catapult/issues/3389).
2. group_report ref build fix. To surface ref builds correctly in group_report,
   we need to modify the endpoint to be able to look at siblings in addition to
   children, and we need to get rid of addSeriesGroup.

In addition to the picker speedup, two necessary pieces that were not done in
this CL, but should be done immediately as followups, are:

1. Deleting addSeriesGroup. See
   https://github.com/catapult-project/catapult/issues/3385.
2. Better error handling in the picker, see
   https://github.com/catapult-project/catapult/issues/3390.
3. Deleting testselection. It's basically dead code but there's one helper
   still used by chart-container that may or may not be necessary.

Demo: https://dev-eakuefner-d97f1223-dot-chromeperf.appspot.com/report

BUG=catapult:#3228

Review-Url: https://codereview.chromium.org/2716683004
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/9fff60268747d20514d711257117c53e5c1f0b7c
Review-Url: https://codereview.chromium.org/2716683004
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/7b2dc0f0d46756afbd7d972fccd29f6e31c7b385

[eakuefner, 2017-02-24 23:56:50]

eakuefner@chromium.org changed reviewers:
+ simonhatch@chromium.org, sullivan@chromium.org

[eakuefner, 2017-02-24 23:56:52]

The progress is a little skeletal here so thanks for bearing with me. I want to
present this CL for some high-level context of what I'm thinking about and then
flesh it out using small CLs and a bottom-up approach.

If my assumptions as laid out in the comments make sense, then I plan to proceed
as follows:
1. Allow chart-container to accept lists of tests and call /graph_json using the
test_path_list mode when it gets a list
2a. Make test-picker perform the XHR to /list_tests with its selection
(necessary so that it can enable/disable the add button properly) and have
report-page pass the result to chart-container. At this point,
test-picker.addTestPathFromSubtestDict will be dead code.
2b. Replace all remaining subtestDict usages in test-picker (which all have to
do with populating the menus) with requests to /list_tests. This solves the
performance problem
3. Have report-page use /list_tests to resolve the test_path_dicts stored in the
page state. This is orthogonal to eliminating sendSubtestRequest from
test-picker, so it can be done as a followup.
4. Update all other users of chart-container to use the test_path_list mode
added in step 1 and delete the test_path_dict functionality.

https://codereview.chromium.org/2716683004/diff/80001/dashboard/dashboard/ele...
File dashboard/dashboard/elements/report-page.html (right):

https://codereview.chromium.org/2716683004/diff/80001/dashboard/dashboard/ele...
dashboard/dashboard/elements/report-page.html:142: this.addChart(chartStates[i],
false);
This is the other addChart callsite. Each entry in chartStates is a
test_path_dict, so realistically, we should perform one XHR per chartState
(probably using the XHR body to chain them) and then once we've gotten the last
one resolved, call addChart with the resulting list of test paths. This will
mean that both report-page and test-picker will be responsible for performing
/list_tests XHRs; report-page in the sid case and test-picker in the adding
tests from the picker case.

https://codereview.chromium.org/2716683004/diff/80001/dashboard/dashboard/ele...
dashboard/dashboard/elements/report-page.html:245: if (selection) {
So, this check confuses me. The issue is that I think this check is redundant,
and the reason it matters is that checking whether a selection is valid now
requires doing the XHR to list_tests. The reason I think it's redundant is that
updateAddButtonState in test-picker will only enable the button in the first
place if the selection is valid. If that makes sense, then I think I will remove
this.

https://codereview.chromium.org/2716683004/diff/80001/dashboard/dashboard/ele...
dashboard/dashboard/elements/report-page.html:246: this.addChart(selection,
true);
What I want to do here is update chart-container such that it can accept a list
OR a dict (just as i've updated /graph_json to do the same). Once all users of
chart-container are converted to pass lists to chart-container (so that
chart-container can call /graph_json exclusively using the test_path_list mode
instead of the test_path_dict mode), I can remove the dict functionality.

[sullivan, 2017-02-25 00:49:14]

I think this is the right direction, but I'm a little confused about the flow of
how data is requested from the server.

https://codereview.chromium.org/2716683004/diff/80001/dashboard/dashboard/ele...
File dashboard/dashboard/elements/report-page.html (right):

https://codereview.chromium.org/2716683004/diff/80001/dashboard/dashboard/ele...
dashboard/dashboard/elements/report-page.html:245: if (selection) {
On 2017/02/24 23:56:52, eakuefner wrote:
> So, this check confuses me. The issue is that I think this check is redundant,
> and the reason it matters is that checking whether a selection is valid now
> requires doing the XHR to list_tests. The reason I think it's redundant is
that
> updateAddButtonState in test-picker will only enable the button in the first
> place if the selection is valid. If that makes sense, then I think I will
remove
> this.

I think this is all correct and you can remove it.

https://codereview.chromium.org/2716683004/diff/80001/dashboard/dashboard/ele...
dashboard/dashboard/elements/report-page.html:246: this.addChart(selection,
true);
On 2017/02/24 23:56:51, eakuefner wrote:
> What I want to do here is update chart-container such that it can accept a
list
> OR a dict (just as i've updated /graph_json to do the same). Once all users of
> chart-container are converted to pass lists to chart-container (so that
> chart-container can call /graph_json exclusively using the test_path_list mode
> instead of the test_path_dict mode), I can remove the dict functionality.

That sounds great to me, assuming it can be factored as well as you did in
graph_json (we just have the one place we're doing conversion to deal with
test_path_dicts and the underlying data model is a list).

https://codereview.chromium.org/2716683004/diff/80001/dashboard/dashboard/ele...
File dashboard/dashboard/elements/test-picker.html (right):

https://codereview.chromium.org/2716683004/diff/80001/dashboard/dashboard/ele...
dashboard/dashboard/elements/test-picker.html:389: this.enableAddSeries =
this.getCurrentSelection() instanceof Array;
A comment about what this does in English would be helpful!

https://codereview.chromium.org/2716683004/diff/80001/dashboard/dashboard/ele...
dashboard/dashboard/elements/test-picker.html:457: * is a valid selection.
I think it would be great to clarify a few things:

1) Non-null return value is a list of test paths (right)?
2) Looks like currentSelectedTests_ is returned if there is a valid selection,
what is that?
3) There is a side effect of sendTestListRequest, how does that work?

[eakuefner, 2017-02-27 18:50:12]

Thanks for the comments. I'm moving on to implementing the steps of the plan I
laid out, and, those being landed, my plan is to come back to this CL.

https://codereview.chromium.org/2716683004/diff/80001/dashboard/dashboard/ele...
File dashboard/dashboard/elements/test-picker.html (right):

https://codereview.chromium.org/2716683004/diff/80001/dashboard/dashboard/ele...
dashboard/dashboard/elements/test-picker.html:457: * is a valid selection.
On 2017/02/25 at 00:49:14, sullivan wrote:
> I think it would be great to clarify a few things:
> 
> 1) Non-null return value is a list of test paths (right)?

Yes. Will write a JSDoc comment for this when I come back to it.

> 2) Looks like currentSelectedTests_ is returned if there is a valid selection,
what is that?

currentSelectedTests_ is always populated by sendListTestsRequest, which will be
called whenever getCurrentSelection finds that the selected test path has
changed.

> 3) There is a side effect of sendTestListRequest, how does that work?

For any given selection, we need to resolve it into a list of test paths, which
is done using sendListTestsRequest. We need to resolve it early so that we can
check whether the add button should be enabled or disabled (since it should only
be enabled if /list_tests says that there are some core tests for the given
path). I'll try to be as clear as I can about the interactions with /list_tests
as we come to this point.

[eakuefner, 2017-03-13 15:46:32]

Description was changed from

==========
[WIP] [Dashboard] Start using /list_tests's test_path_dict mode in test-picker

A followup CL will remove testselection, which still has a dependency from
chart-container, entirely.

BUG=catapult:#3228
==========

to

==========
[Dashboard] Start using /list_tests's test_path_dict mode in test-picker

A followup CL will remove testselection, which still has a dependency from
chart-container, entirely.

Demo: https://dev-eakuefner-d97f1223-dot-chromeperf.appspot.com/report

BUG=catapult:#3228
==========

[eakuefner, 2017-03-13 15:53:58]

Folks, at last, please take a look. I think this CL is necessarily dense, so I
look forward to your questions and comments.

https://codereview.chromium.org/2716683004/diff/120001/dashboard/dashboard/el...
File dashboard/dashboard/elements/report-page.html (right):

https://codereview.chromium.org/2716683004/diff/120001/dashboard/dashboard/el...
dashboard/dashboard/elements/report-page.html:266: onAddChartButtonClicked:
function(event) {
Because the instantiation test only covers the sid case, we should definitely
add test coverage for this path. It's funky because the work this path does is
slightly different, but no less worthwhile to test. If the sid case had had a
test, I would have been able to skip the deploy-rinse-repeat cycle for making it
work. Unfortunately I've very much been in a hacker mindset trying to get this
done because of how long it's taken already. However, my experience doing so
suggests that it would be awesome to do TDD even for these complex, migratory
types of changes in the future, even if the test requires a fair amount of
setup, which in this case would be extensive XHR mocking.

https://codereview.chromium.org/2716683004/diff/120001/dashboard/dashboard/el...
File dashboard/dashboard/elements/test-picker.html (right):

https://codereview.chromium.org/2716683004/diff/120001/dashboard/dashboard/el...
dashboard/dashboard/elements/test-picker.html:467: if (level <= 2) return null;
Just wanted to call this out as a point that gave me quite a bit of trouble. So,
the way this works is, /list_tests's test_path_dict mode will blow up if it
receives a path that has fewer than 4 parts. However, these are not path parts,
because masters don't have their own menu (doh). So in my original CL I had
level <= 3, but Simon changed it to 2, which broke the invalid-returns-null test
case that I added. Of course, specifying 3 parts in the picker should be
completely valid (or at least, satisfies the contract for input to /list_tests),
since the "bot" selection implicitly specifies a master (in particular, the one
to which the bot belongs). So I simply modified the test not to specify a
subtest and it passes.

https://codereview.chromium.org/2716683004/diff/140001/dashboard/dashboard/el...
File dashboard/dashboard/elements/report-page.html (right):

https://codereview.chromium.org/2716683004/diff/140001/dashboard/dashboard/el...
dashboard/dashboard/elements/report-page.html:149: const mainPath = pair[0];
I didn't realize destructuring was approved, so I'll probably upload another
patch set before landing so that I don't have to index by hand like this.

https://codereview.chromium.org/2716683004/diff/140001/dashboard/dashboard/el...
dashboard/dashboard/elements/report-page.html:197:
Promise.all(resolvedChartStates).then(
Promises are fucking awesome. To recap what just happened here, I called
Promise.all in the loop to form a promise based on the selected/unselected XHR
for each test, and then I call it here to group all of those promises together.

If simple_xhr were based on promises, I wouldn't have had to even call `new
Promise()` in this file at all. I'd like to open a bug about modernizing
simple_xhr.

By the way, before I figured out how to do this using promises, I was attempting
to busy wait using a while loop; of course that failed horribly because it
blocked the main thread. So that led me to Google how to sleep in JS, which made
me realize that, duh, promises are _the_ way to batch XHRs (or asynchronous
activity of any kind, really) clientside. Our request parallelization efforts
should surely block on converting simple_xhr to promises.

[eakuefner, 2017-03-13 15:56:11]

BTW, things to try in the demo:

1. Add a test using the picker
2. Use the draggy thing to add a test to an existing container
3. Try loading a report using ?sid

[sullivan, 2017-03-13 16:20:07]

The actual code of the CL looks good to me, would like to see the CL description
or a linked doc go into more depth about the next steps (for example, how are we
actually going to speed up the menus? At what point would parallel XHRs for all
graphs be unblocked?).

Also I think some TODOs on end-user error handling (or code comments on how it
does work, if it does) would be good.

https://codereview.chromium.org/2716683004/diff/140001/dashboard/dashboard/el...
File dashboard/dashboard/elements/chart-container.html (left):

https://codereview.chromium.org/2716683004/diff/140001/dashboard/dashboard/el...
dashboard/dashboard/elements/chart-container.html:701: *     or unselected
series.
This was incorrect before, right?

https://codereview.chromium.org/2716683004/diff/140001/dashboard/dashboard/el...
File dashboard/dashboard/elements/chart-container.html (right):

https://codereview.chromium.org/2716683004/diff/140001/dashboard/dashboard/el...
dashboard/dashboard/elements/chart-container.html:695: addSeriesGroup2:
function(
Should be a clear function-level comment explaining why we have addSeriesGroup
and addSeriesGroup2 and linking the bug about the long-term plan for these
functions.

https://codereview.chromium.org/2716683004/diff/140001/dashboard/dashboard/el...
File dashboard/dashboard/elements/report-page.html (right):

https://codereview.chromium.org/2716683004/diff/140001/dashboard/dashboard/el...
dashboard/dashboard/elements/report-page.html:167: reject('Error from
/list_tests.');
How do these get surfaced to the user?

https://codereview.chromium.org/2716683004/diff/140001/dashboard/dashboard/el...
dashboard/dashboard/elements/report-page.html:197:
Promise.all(resolvedChartStates).then(
On 2017/03/13 15:53:57, eakuefner wrote:
> Promises are fucking awesome. To recap what just happened here, I called
> Promise.all in the loop to form a promise based on the selected/unselected XHR
> for each test, and then I call it here to group all of those promises
together.
> 
> If simple_xhr were based on promises, I wouldn't have had to even call `new
> Promise()` in this file at all. I'd like to open a bug about modernizing
> simple_xhr.
> 
> By the way, before I figured out how to do this using promises, I was
attempting
> to busy wait using a while loop; of course that failed horribly because it
> blocked the main thread. So that led me to Google how to sleep in JS, which
made
> me realize that, duh, promises are _the_ way to batch XHRs (or asynchronous
> activity of any kind, really) clientside. Our request parallelization efforts
> should surely block on converting simple_xhr to promises.

I'm really glad you did the research here because it should vastly simplify the
code for parallelizing XHRs to /graph_json when we start working on that :)

https://codereview.chromium.org/2716683004/diff/140001/dashboard/dashboard/el...
File dashboard/dashboard/elements/test-picker.html (right):

https://codereview.chromium.org/2716683004/diff/140001/dashboard/dashboard/el...
dashboard/dashboard/elements/test-picker.html:522: if (error) console.log('Error
from retCurrentSelection.');
What does the user see if this happens? Loading spinner for the menu never goes
away? I know this CL is big but there should be a TODO+bug to handle the error.

[eakuefner, 2017-03-14 21:23:02]

Description was changed from

==========
[Dashboard] Start using /list_tests's test_path_dict mode in test-picker

A followup CL will remove testselection, which still has a dependency from
chart-container, entirely.

Demo: https://dev-eakuefner-d97f1223-dot-chromeperf.appspot.com/report

BUG=catapult:#3228
==========

to

==========
[Dashboard] Start using /list_tests's test_path_dict mode in test-picker

As of this CL, it should be immediately possible to factor away test-picker's
request for the subtest dict in favor of an approach that uses the new endpoint.
The functionality in this CL subsumes report-page's need for the functionality
of test-picker that depended on the subtest dict, and now it is only test-picker
itself that uses the subtest dict (to populate its menus).

In parallel, this CL offers up a couple more avenues for interesting work:
1. Request parallelization. We will need to clean up simple_xhr probably in
   favor of fetch (see
   https://github.com/catapult-project/catapult/issues/3389).
2. group_report ref build fix. To surface ref builds correctly in group_report,
   we need to modify the endpoint to be able to look at siblings in addition to
   children, and we need to get rid of addSeriesGroup.

In addition to the picker speedup, two necessary pieces that were not done in
this CL, but should be done immediately as followups, are:

1. Deleting addSeriesGroup. See
   https://github.com/catapult-project/catapult/issues/3385.
2. Better error handling in the picker, see
   https://github.com/catapult-project/catapult/issues/3390.

Demo: https://dev-eakuefner-d97f1223-dot-chromeperf.appspot.com/report

BUG=catapult:#3228
==========

[eakuefner, 2017-03-14 21:23:18]

Annie, please take another look. I've edited the description of my CL as you
have suggested.

https://codereview.chromium.org/2716683004/diff/140001/dashboard/dashboard/el...
File dashboard/dashboard/elements/chart-container.html (left):

https://codereview.chromium.org/2716683004/diff/140001/dashboard/dashboard/el...
dashboard/dashboard/elements/chart-container.html:701: *     or unselected
series.
On 2017/03/13 at 16:20:07, sullivan wrote:
> This was incorrect before, right?

Yes. Likely just a typo; I fixed another such mistake in the latest patch set.

https://codereview.chromium.org/2716683004/diff/140001/dashboard/dashboard/el...
File dashboard/dashboard/elements/chart-container.html (right):

https://codereview.chromium.org/2716683004/diff/140001/dashboard/dashboard/el...
dashboard/dashboard/elements/chart-container.html:695: addSeriesGroup2:
function(
On 2017/03/13 at 16:20:07, sullivan wrote:
> Should be a clear function-level comment explaining why we have addSeriesGroup
and addSeriesGroup2 and linking the bug about the long-term plan for these
functions.

https://github.com/catapult-project/catapult/issues/3385

https://codereview.chromium.org/2716683004/diff/140001/dashboard/dashboard/el...
File dashboard/dashboard/elements/report-page.html (right):

https://codereview.chromium.org/2716683004/diff/140001/dashboard/dashboard/el...
dashboard/dashboard/elements/report-page.html:149: const mainPath = pair[0];
On 2017/03/13 at 15:53:57, eakuefner wrote:
> I didn't realize destructuring was approved, so I'll probably upload another
patch set before landing so that I don't have to index by hand like this.

Actually, I'll do this as a follow-up; it's low-priority.

https://codereview.chromium.org/2716683004/diff/140001/dashboard/dashboard/el...
dashboard/dashboard/elements/report-page.html:167: reject('Error from
/list_tests.');
On 2017/03/13 at 16:20:07, sullivan wrote:
> How do these get surfaced to the user?

Turns out, they don't. To get any output from this, you need to pass a second
argument to .then which handles the reason. I've added this to my call to `then`
below. It just performs a console.log, which we anyway did before. I believe
that innovating on this should be part of the followup bug that you proposed.

https://codereview.chromium.org/2716683004/diff/140001/dashboard/dashboard/el...
dashboard/dashboard/elements/report-page.html:197:
Promise.all(resolvedChartStates).then(
On 2017/03/13 at 16:20:07, sullivan wrote:
> On 2017/03/13 15:53:57, eakuefner wrote:
> > Promises are fucking awesome. To recap what just happened here, I called
> > Promise.all in the loop to form a promise based on the selected/unselected
XHR
> > for each test, and then I call it here to group all of those promises
together.
> > 
> > If simple_xhr were based on promises, I wouldn't have had to even call `new
> > Promise()` in this file at all. I'd like to open a bug about modernizing
> > simple_xhr.
> > 
> > By the way, before I figured out how to do this using promises, I was
attempting
> > to busy wait using a while loop; of course that failed horribly because it
> > blocked the main thread. So that led me to Google how to sleep in JS, which
made
> > me realize that, duh, promises are _the_ way to batch XHRs (or asynchronous
> > activity of any kind, really) clientside. Our request parallelization
efforts
> > should surely block on converting simple_xhr to promises.
> 
> I'm really glad you did the research here because it should vastly simplify
the code for parallelizing XHRs to /graph_json when we start working on that :)

Bug filed: https://github.com/catapult-project/catapult/issues/3389

https://codereview.chromium.org/2716683004/diff/140001/dashboard/dashboard/el...
File dashboard/dashboard/elements/test-picker.html (right):

https://codereview.chromium.org/2716683004/diff/140001/dashboard/dashboard/el...
dashboard/dashboard/elements/test-picker.html:522: if (error) console.log('Error
from retCurrentSelection.');
On 2017/03/13 at 16:20:07, sullivan wrote:
> What does the user see if this happens? Loading spinner for the menu never
goes away? I know this CL is big but there should be a TODO+bug to handle the
error.

Ideally we should use promises to set this.loading = false if _either_ of these
XHRs fail (currently we only do it for the main listTestsXhr below). Bug filed:
https://github.com/catapult-project/catapult/issues/3390

[sullivan, 2017-03-14 21:29:38]

lgtm

[eakuefner, 2017-03-14 21:30:15]

Description was changed from

==========
[Dashboard] Start using /list_tests's test_path_dict mode in test-picker

As of this CL, it should be immediately possible to factor away test-picker's
request for the subtest dict in favor of an approach that uses the new endpoint.
The functionality in this CL subsumes report-page's need for the functionality
of test-picker that depended on the subtest dict, and now it is only test-picker
itself that uses the subtest dict (to populate its menus).

In parallel, this CL offers up a couple more avenues for interesting work:
1. Request parallelization. We will need to clean up simple_xhr probably in
   favor of fetch (see
   https://github.com/catapult-project/catapult/issues/3389).
2. group_report ref build fix. To surface ref builds correctly in group_report,
   we need to modify the endpoint to be able to look at siblings in addition to
   children, and we need to get rid of addSeriesGroup.

In addition to the picker speedup, two necessary pieces that were not done in
this CL, but should be done immediately as followups, are:

1. Deleting addSeriesGroup. See
   https://github.com/catapult-project/catapult/issues/3385.
2. Better error handling in the picker, see
   https://github.com/catapult-project/catapult/issues/3390.

Demo: https://dev-eakuefner-d97f1223-dot-chromeperf.appspot.com/report

BUG=catapult:#3228
==========

to

==========
[Dashboard] Start using /list_tests's test_path_dict mode in test-picker

As of this CL, it should be immediately possible to factor away test-picker's
request for the subtest dict in favor of an approach that uses the new endpoint.
The functionality in this CL subsumes report-page's need for the functionality
of test-picker that depended on the subtest dict, and now it is only test-picker
itself that uses the subtest dict (to populate its menus).

In parallel, this CL offers up a couple more avenues for interesting work:
1. Request parallelization. We will need to clean up simple_xhr probably in
   favor of fetch (see
   https://github.com/catapult-project/catapult/issues/3389).
2. group_report ref build fix. To surface ref builds correctly in group_report,
   we need to modify the endpoint to be able to look at siblings in addition to
   children, and we need to get rid of addSeriesGroup.

In addition to the picker speedup, two necessary pieces that were not done in
this CL, but should be done immediately as followups, are:

1. Deleting addSeriesGroup. See
   https://github.com/catapult-project/catapult/issues/3385.
2. Better error handling in the picker, see
   https://github.com/catapult-project/catapult/issues/3390.
3. Deleting testselection. It's basically dead code but there's one helper
   still used by chart-container that may or may not be necessary.

Demo: https://dev-eakuefner-d97f1223-dot-chromeperf.appspot.com/report

BUG=catapult:#3228
==========

[eakuefner, 2017-03-14 21:30:27]

The CQ bit was checked by eakuefner@chromium.org

[commit-bot: I haz the power, 2017-03-14 21:30:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 22:11:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-14 22:11:14]

Try jobs failed on following builders:
  Catapult Windows Tryserver on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Wi...)

[eakuefner, 2017-03-15 20:59:02]

The CQ bit was checked by eakuefner@chromium.org

[commit-bot: I haz the power, 2017-03-15 20:59:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[eakuefner, 2017-03-15 21:08:37]

The CQ bit was unchecked by eakuefner@chromium.org

[eakuefner, 2017-03-15 21:11:54]

The CQ bit was checked by eakuefner@chromium.org

[eakuefner, 2017-03-15 21:11:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
sullivan@chromium.org
Link to the patchset: https://codereview.chromium.org/2716683004/#ps200001
(title: "clearXhrMock ugh")

[commit-bot: I haz the power, 2017-03-15 21:12:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-15 21:40:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-15 21:40:38]

Failed to apply patch for dashboard/dashboard/elements/chart-container.html:
While running git apply --index -p1;
  error: patch failed: dashboard/dashboard/elements/chart-container.html:694
  error: dashboard/dashboard/elements/chart-container.html: patch does not apply

Patch:       dashboard/dashboard/elements/chart-container.html
Index: dashboard/dashboard/elements/chart-container.html
diff --git a/dashboard/dashboard/elements/chart-container.html
b/dashboard/dashboard/elements/chart-container.html
index
db5df34880dfd07d29f16c0133d65f1892cc3e71..e7e3c6996ce85cde0aa0edf8ce431bc5b78db0b1
100644
--- a/dashboard/dashboard/elements/chart-container.html
+++ b/dashboard/dashboard/elements/chart-container.html
@@ -578,7 +578,7 @@ triaging functionality in the chart.
          *
          * @param {Array.<Array>} testPathAndSelected A list of
          *     pair of test path and a list of selected series.
-         * @return {boolean} collapse Whether unchecked and unimportant
+         * @param {boolean} collapse Whether unchecked and unimportant
          *     series should start off hidden.
          */
         addSeriesGroup: function(testPathAndSelected, collapse) {
@@ -645,22 +645,85 @@ triaging functionality in the chart.
         },
 
         /**
+         * Adds series group to graph explicitly.
+         *
+         * Since adding the test_path_dict mode for /list_tests, the preferred
+         * flow for resolving a test path into lists of selected and unselected
+         * tests is to use this endpoint to resolve them directly. This
function
+         * should be functionally equivalent to addSeriesGroup except for this
+         * resolution step, and the goal is to replace it
+         * (https://github.com/catapult-project/catapult/issues/3385).
+         *
+         * @param {string} mainPath The main path for this series group
+         * @param {string[]} selectedPaths The selected subpaths
+         * @param {string[]} unselectedPaths The unselected subpaths
+         * @param {boolean} collapse Whether unchecked and unimportant
+         *     series should start off hidden.
+         */
+        addSeriesGroup2: function(
+            mainPath, selectedPaths, unselectedPaths, collapse) {
+          // Checks if test path already exists.
+          const testPathSet = new Set(
+              this.seriesGroupList.map(group => group.path));
+
+          if (testPathSet.has(mainPath)) {
+            return;
+          }
+
+          const tests = [];
+          for (const selectedPath of selectedPaths) {
+            tests.push({
+              name: selectedPath.split('/').slice(-1)[0],
+              selected: true
+            });
+          }
+
+          const seriesGroup = {
+            path: mainPath,
+            tests: tests,
+            collapse: collapse,
+            numPendingRequests: 0
+          };
+
+          this.push('seriesGroupList', seriesGroup);
+
+          if (selectedPaths.length > 0) {
+            this.sendGraphJsonRequest(selectedPaths, true, mainPath);
+          }
+
+          if (unselectedPaths.length > 0) {
+            this.sendGraphJsonRequest(unselectedPaths, false, mainPath);
+          }
+
+          this.updateSlider();
+          this.$.title.update();
+        },
+
+
+        /**
          * Sends a request for graph JSON.
          *
          * @param {Object} testPathDictOrList Dictionary of test path to list
of
          *     selected series, or flat list of test paths.
-         * @return {boolean} isSelected Whether this request is for selected
+         * @param {boolean} isSelected Whether this request is for selected
          *     or unselected series.
+         * @param {String} mainPath If a list of tests is supplied, the main
+         *     path to which they belong, for updating the counter.
          */
-        sendGraphJsonRequest: function(testPathDictOrList, isSelected) {
+        sendGraphJsonRequest: function(
+            testPathDictOrList, isSelected, mainPath) {
+          // TODO(eakuefner): Get rid of mainPath once loading counter is
+          // cleaned up
           var params = JSON.parse(JSON.stringify(this.graphParams));
 
           if (testPathDictOrList instanceof Array) {
             var testPaths = testPathDictOrList;
             params.test_path_list = testPathDictOrList;
+            var pathsForUpdate = [mainPath];
           } else {
             var testPaths = Object.keys(testPathDictOrList);
             params.test_path_dict = testPathDictOrList;
+            var pathsForUpdate = testPaths;
           }
 
           // TODO(eakuefner): Figure out how to remove this parameter since in
@@ -669,7 +732,7 @@ triaging functionality in the chart.
             params['is_selected'] = true;
           }
 
-          this.updateSeriesGroupLoadingCounter(testPaths, true);
+          this.updateSeriesGroupLoadingCounter(pathsForUpdate, true);
 
           var req = simple_xhr.send(
               '/graph_json',
@@ -684,7 +747,7 @@ triaging functionality in the chart.
                 }
 
                 this.updateSeriesGroupLoadingCounter(
-                    testPaths, false);
+                    pathsForUpdate, false);
                 this.$['loading-div'].style.display = 'none';
               }.bind(this),
               function(error) {
@@ -694,7 +757,7 @@ triaging functionality in the chart.
                   console.warn(error);
                 }
                 this.updateSeriesGroupLoadingCounter(
-                    testPaths, false);
+                    pathsForUpdate, false);
                 this.$['loading-div'].style.display = 'none';
               }.bind(this)
           );
@@ -2398,7 +2461,8 @@ triaging functionality in the chart.
           }
           var data = JSON.parse(dataTransfer.getData('data'));
           if (data) {
-            this.addSeriesGroup(data, true);
+            this.addSeriesGroup2(
+                data.mainPath, data.selectedPaths, data.unselectedPaths, true);
           }
           this.fireChartStateChangedEvent(this.seriesGroupList);
         },

[eakuefner, 2017-03-15 21:52:00]

The CQ bit was checked by eakuefner@chromium.org

[eakuefner, 2017-03-15 21:52:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
sullivan@chromium.org
Link to the patchset: https://codereview.chromium.org/2716683004/#ps220001
(title: "rebase")

[commit-bot: I haz the power, 2017-03-15 21:52:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-15 22:14:12]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1489614720388950,
"parent_rev": "4bfab43f3b024518ade6e57bca405bc3af8fe2d3", "commit_rev":
"9fff60268747d20514d711257117c53e5c1f0b7c"}

[commit-bot: I haz the power, 2017-03-15 22:14:16]

Description was changed from

==========
[Dashboard] Start using /list_tests's test_path_dict mode in test-picker

As of this CL, it should be immediately possible to factor away test-picker's
request for the subtest dict in favor of an approach that uses the new endpoint.
The functionality in this CL subsumes report-page's need for the functionality
of test-picker that depended on the subtest dict, and now it is only test-picker
itself that uses the subtest dict (to populate its menus).

In parallel, this CL offers up a couple more avenues for interesting work:
1. Request parallelization. We will need to clean up simple_xhr probably in
   favor of fetch (see
   https://github.com/catapult-project/catapult/issues/3389).
2. group_report ref build fix. To surface ref builds correctly in group_report,
   we need to modify the endpoint to be able to look at siblings in addition to
   children, and we need to get rid of addSeriesGroup.

In addition to the picker speedup, two necessary pieces that were not done in
this CL, but should be done immediately as followups, are:

1. Deleting addSeriesGroup. See
   https://github.com/catapult-project/catapult/issues/3385.
2. Better error handling in the picker, see
   https://github.com/catapult-project/catapult/issues/3390.
3. Deleting testselection. It's basically dead code but there's one helper
   still used by chart-container that may or may not be necessary.

Demo: https://dev-eakuefner-d97f1223-dot-chromeperf.appspot.com/report

BUG=catapult:#3228
==========

to

==========
[Dashboard] Start using /list_tests's test_path_dict mode in test-picker

As of this CL, it should be immediately possible to factor away test-picker's
request for the subtest dict in favor of an approach that uses the new endpoint.
The functionality in this CL subsumes report-page's need for the functionality
of test-picker that depended on the subtest dict, and now it is only test-picker
itself that uses the subtest dict (to populate its menus).

In parallel, this CL offers up a couple more avenues for interesting work:
1. Request parallelization. We will need to clean up simple_xhr probably in
   favor of fetch (see
   https://github.com/catapult-project/catapult/issues/3389).
2. group_report ref build fix. To surface ref builds correctly in group_report,
   we need to modify the endpoint to be able to look at siblings in addition to
   children, and we need to get rid of addSeriesGroup.

In addition to the picker speedup, two necessary pieces that were not done in
this CL, but should be done immediately as followups, are:

1. Deleting addSeriesGroup. See
   https://github.com/catapult-project/catapult/issues/3385.
2. Better error handling in the picker, see
   https://github.com/catapult-project/catapult/issues/3390.
3. Deleting testselection. It's basically dead code but there's one helper
   still used by chart-container that may or may not be necessary.

Demo: https://dev-eakuefner-d97f1223-dot-chromeperf.appspot.com/report

BUG=catapult:#3228

Review-Url: https://codereview.chromium.org/2716683004
Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2017-03-15 22:14:17]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...

[eakuefner, 2017-03-16 19:03:42]

A revert of this CL (patchset #12 id:220001) has been created in
https://codereview.chromium.org/2750313003/ by eakuefner@chromium.org.

The reason for reverting is: Broke restoring reports from page_state. Fix in
progress..

[eakuefner, 2017-03-16 23:40:56]

The CQ bit was checked by eakuefner@chromium.org

[eakuefner, 2017-03-16 23:40:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
sullivan@chromium.org
Link to the patchset: https://codereview.chromium.org/2716683004/#ps240001
(title: "fix v8 case")

[commit-bot: I haz the power, 2017-03-16 23:41:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-17 00:03:39]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1489707656811610,
"parent_rev": "7addbd8ae56107109f95cdd82e5fc3a35c318b23", "commit_rev":
"7b2dc0f0d46756afbd7d972fccd29f6e31c7b385"}

[commit-bot: I haz the power, 2017-03-17 00:03:43]

Description was changed from

==========
[Dashboard] Start using /list_tests's test_path_dict mode in test-picker

As of this CL, it should be immediately possible to factor away test-picker's
request for the subtest dict in favor of an approach that uses the new endpoint.
The functionality in this CL subsumes report-page's need for the functionality
of test-picker that depended on the subtest dict, and now it is only test-picker
itself that uses the subtest dict (to populate its menus).

In parallel, this CL offers up a couple more avenues for interesting work:
1. Request parallelization. We will need to clean up simple_xhr probably in
   favor of fetch (see
   https://github.com/catapult-project/catapult/issues/3389).
2. group_report ref build fix. To surface ref builds correctly in group_report,
   we need to modify the endpoint to be able to look at siblings in addition to
   children, and we need to get rid of addSeriesGroup.

In addition to the picker speedup, two necessary pieces that were not done in
this CL, but should be done immediately as followups, are:

1. Deleting addSeriesGroup. See
   https://github.com/catapult-project/catapult/issues/3385.
2. Better error handling in the picker, see
   https://github.com/catapult-project/catapult/issues/3390.
3. Deleting testselection. It's basically dead code but there's one helper
   still used by chart-container that may or may not be necessary.

Demo: https://dev-eakuefner-d97f1223-dot-chromeperf.appspot.com/report

BUG=catapult:#3228

Review-Url: https://codereview.chromium.org/2716683004
Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

to

==========
[Dashboard] Start using /list_tests's test_path_dict mode in test-picker

As of this CL, it should be immediately possible to factor away test-picker's
request for the subtest dict in favor of an approach that uses the new endpoint.
The functionality in this CL subsumes report-page's need for the functionality
of test-picker that depended on the subtest dict, and now it is only test-picker
itself that uses the subtest dict (to populate its menus).

In parallel, this CL offers up a couple more avenues for interesting work:
1. Request parallelization. We will need to clean up simple_xhr probably in
   favor of fetch (see
   https://github.com/catapult-project/catapult/issues/3389).
2. group_report ref build fix. To surface ref builds correctly in group_report,
   we need to modify the endpoint to be able to look at siblings in addition to
   children, and we need to get rid of addSeriesGroup.

In addition to the picker speedup, two necessary pieces that were not done in
this CL, but should be done immediately as followups, are:

1. Deleting addSeriesGroup. See
   https://github.com/catapult-project/catapult/issues/3385.
2. Better error handling in the picker, see
   https://github.com/catapult-project/catapult/issues/3390.
3. Deleting testselection. It's basically dead code but there's one helper
   still used by chart-container that may or may not be necessary.

Demo: https://dev-eakuefner-d97f1223-dot-chromeperf.appspot.com/report

BUG=catapult:#3228

Review-Url: https://codereview.chromium.org/2716683004
Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
Review-Url: https://codereview.chromium.org/2716683004
Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2017-03-17 00:03:44]

Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...