PlzNavigate: Rewrite the url and frame type validation part of the external/wpt/service-workers/se…

PlzNavigate: Rewrite the url and frame type validation part of the  external/wpt/service-workers/service-worker/navigate-window.https.html layout test.

This test expects the ServiceWorkerProviderHosts to be in a specific order which breaks with PlzNavigate due to precreated hosts. It seemed to be a better
to rewrite the validation code to do an exhaustive match of the URLs and the frame types in the result list.

BUG=697307
Review-Url: https://codereview.chromium.org/2726793002
Cr-Commit-Position: refs/heads/master@{#454455}
Committed: https://chromium.googlesource.com/chromium/src/+/54e4fbdde6ae7aa7844a0d11489ba76c9f859afb

[ananta, 2017-03-01 03:31:32]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[ananta, 2017-03-01 03:31:56]

ananta@chromium.org changed reviewers:
+ jam@chromium.org, japhet@chromium.org

[commit-bot: I haz the power, 2017-03-01 03:32:28]

Dry run: 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-01 04:33:57]

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

[commit-bot: I haz the power, 2017-03-01 04:33:57]

Dry run: This issue passed the CQ dry run.

[Nate Chapin, 2017-03-02 00:06:31]

japhet@chromium.org changed reviewers:
+ jsbell@chromium.org

[Nate Chapin, 2017-03-02 00:06:32]

This looks ok to me, but I'd prefer a sanity check from someone who knows
service workers better.

jsbell@, would you mind looking at this, or pointing us at the right person?
AFAICT, at question is whether there are ordering guarantees in the output of
Clients.matchAll (https://www.w3.org/TR/service-workers/#clients-getall). My
reading is no, but this wpt seems to enforce ordering.

[ananta, 2017-03-02 01:04:27]

On 2017/03/02 00:06:32, Nate Chapin wrote:
> This looks ok to me, but I'd prefer a sanity check from someone who knows
> service workers better.
> 
> jsbell@, would you mind looking at this, or pointing us at the right person?
> AFAICT, at question is whether there are ordering guarantees in the output of
> Clients.matchAll (https://www.w3.org/TR/service-workers/#clients-getall). My
> reading is no, but this wpt seems to enforce ordering.

The DidGetClients() function here
https://cs.chromium.org/chromium/src/content/browser/service_worker/service_w...
sorts the list based on the active tabs showing up ahead. In PlzNavigate the
precreated hosts show up as active
before the rest.

[jsbell, 2017-03-02 20:19:49]

lgtm but...

https://codereview.chromium.org/2726793002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/navigate-window.https.html
(right):

https://codereview.chromium.org/2726793002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/navigate-window.https.html:88:
// Check if the URLs and the frame type in the result list match with those in
nit: wrap to 80 columns?

(since this file is _mostly_ wrapped there anyway)

https://codereview.chromium.org/2726793002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/navigate-window.https.html:88:
// Check if the URLs and the frame type in the result list match with those in
Would it be easier to just to sort the lists by url?

expected.sort((a, b) => a.url < b.url ? -1 : b.url < a.url ? 1 : 0);
resultList.sort((a, b) => a.url < b.url ? -1 : b.url < a.url ? 1 : 0);

... and keep the existing (simpler?) comparison loop?

https://codereview.chromium.org/2726793002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/navigate-window.https.html:94:
if (expected[i].url == resultList[j].url) {
nit: this file seems to prefer === to ==

[ananta, 2017-03-02 23:21:10]

https://codereview.chromium.org/2726793002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/navigate-window.https.html
(right):

https://codereview.chromium.org/2726793002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/navigate-window.https.html:88:
// Check if the URLs and the frame type in the result list match with those in
On 2017/03/02 20:19:49, jsbell wrote:
> Would it be easier to just to sort the lists by url?
> 
> expected.sort((a, b) => a.url < b.url ? -1 : b.url < a.url ? 1 : 0);
> resultList.sort((a, b) => a.url < b.url ? -1 : b.url < a.url ? 1 : 0);
> 
> ... and keep the existing (simpler?) comparison loop?

Thanks. Done. I added a compare_urls comparator function.

https://codereview.chromium.org/2726793002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/navigate-window.https.html:88:
// Check if the URLs and the frame type in the result list match with those in
On 2017/03/02 20:19:49, jsbell wrote:
> nit: wrap to 80 columns?
> 
> (since this file is _mostly_ wrapped there anyway)

Removed this comment

https://codereview.chromium.org/2726793002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/navigate-window.https.html:94:
if (expected[i].url == resultList[j].url) {
On 2017/03/02 20:19:49, jsbell wrote:
> nit: this file seems to prefer === to ==

Not needed after sorting.

[ananta, 2017-03-02 23:24:23]

The CQ bit was checked by ananta@chromium.org

[ananta, 2017-03-02 23:24:23]

The patchset sent to the CQ was uploaded after l-g-t-m from jsbell@chromium.org
Link to the patchset: https://codereview.chromium.org/2726793002/#ps80001
(title: "Address review comments")

[commit-bot: I haz the power, 2017-03-02 23:25:12]

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-03 00:53:00]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488497063490240,
"parent_rev": "eb1ff6b20500834aa8283148b8a7b78f2938ca27", "commit_rev":
"54e4fbdde6ae7aa7844a0d11489ba76c9f859afb"}

[commit-bot: I haz the power, 2017-03-03 00:53:40]

Description was changed from

==========
PlzNavigate: Rewrite the url and frame type validation part of the 
external/wpt/service-workers/service-worker/navigate-window.https.html layout
test.

This test expects the ServiceWorkerProviderHosts to be in a specific order which
breaks with PlzNavigate due to precreated hosts. It seemed to be a better
to rewrite the validation code to do an exhaustive match of the URLs and the
frame types in the result list.

BUG=697307
==========

to

==========
PlzNavigate: Rewrite the url and frame type validation part of the 
external/wpt/service-workers/service-worker/navigate-window.https.html layout
test.

This test expects the ServiceWorkerProviderHosts to be in a specific order which
breaks with PlzNavigate due to precreated hosts. It seemed to be a better
to rewrite the validation code to do an exhaustive match of the URLs and the
frame types in the result list.

BUG=697307

Review-Url: https://codereview.chromium.org/2726793002
Cr-Commit-Position: refs/heads/master@{#454455}
Committed:
https://chromium.googlesource.com/chromium/src/+/54e4fbdde6ae7aa7844a0d11489b...
==========

[commit-bot: I haz the power, 2017-03-03 00:53:41]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/54e4fbdde6ae7aa7844a0d11489b...

[Marijn Kruisselbrink, 2017-03-03 01:11:18]

" at question is whether there are ordering guarantees in the output of
Clients.matchAll (https://www.w3.org/TR/service-workers/#clients-getall). My
reading is no, but this wpt seems to enforce ordering."

The correct spec would be https://w3c.github.io/ServiceWorker/#clients-getall
(the editors draft), and see there step 5) in the algorithm: "Sort
matchedClients such that:", which as far as I can tell does fully define the
ordering of clients (assuming clients were created in a well defined order).
There are some open issues to clarify what exactly the order is in some edge
cases, but there are most certainly ordering guarantees.

[falken, 2017-03-03 01:44:08]

On 2017/03/02 01:04:27, ananta wrote:
> On 2017/03/02 00:06:32, Nate Chapin wrote:
> > This looks ok to me, but I'd prefer a sanity check from someone who knows
> > service workers better.
> > 
> > jsbell@, would you mind looking at this, or pointing us at the right person?
> > AFAICT, at question is whether there are ordering guarantees in the output
of
> > Clients.matchAll (https://www.w3.org/TR/service-workers/#clients-getall). My
> > reading is no, but this wpt seems to enforce ordering.
> 
> The DidGetClients() function here
>
https://cs.chromium.org/chromium/src/content/browser/service_worker/service_w...
> sorts the list based on the active tabs showing up ahead. In PlzNavigate the
> precreated hosts show up as active
> before the rest.

That sorting is required by the spec:
https://bugs.chromium.org/p/chromium/issues/detail?id=461411

I think mek is right, the spec guarantees an ordering for matchAll and the WPT
test should be faithful to that. The WPT test is shared by Firefox and other
browsers. Could we revert this change?

[falken, 2017-03-03 01:45:47]

On 2017/03/03 01:44:08, falken wrote:
> On 2017/03/02 01:04:27, ananta wrote:
> > On 2017/03/02 00:06:32, Nate Chapin wrote:
> > > This looks ok to me, but I'd prefer a sanity check from someone who knows
> > > service workers better.
> > > 
> > > jsbell@, would you mind looking at this, or pointing us at the right
person?
> > > AFAICT, at question is whether there are ordering guarantees in the output
> of
> > > Clients.matchAll (https://www.w3.org/TR/service-workers/#clients-getall).
My
> > > reading is no, but this wpt seems to enforce ordering.
> > 
> > The DidGetClients() function here
> >
>
https://cs.chromium.org/chromium/src/content/browser/service_worker/service_w...
> > sorts the list based on the active tabs showing up ahead. In PlzNavigate the
> > precreated hosts show up as active
> > before the rest.
> 
> That sorting is required by the spec:
> https://bugs.chromium.org/p/chromium/issues/detail?id=461411
> 
> I think mek is right, the spec guarantees an ordering for matchAll and the WPT
> test should be faithful to that. The WPT test is shared by Firefox and other
> browsers. Could we revert this change?

Ah, I see you just responded on the bug. Looking...