Add tests for Request.isReloadNavigation #10192

yutakahirano · 2018-03-27T06:27:52Z

This is for w3c/ServiceWorker#1167.

yutakahirano · 2018-03-27T06:28:07Z

@annevk @jakearchibald @wanderview

ghost · 2018-03-27T06:44:34Z

Build PASSED

Started: 2018-04-03 03:16:23
Finished: 2018-04-03 03:22:30

View more information about this build on:

annevk · 2018-03-29T07:24:32Z

I think we should have more tests:

history.go(0)
A manual test you have to reload yourself
A POSTed navigation followed by a reload
History traversal followed by a reload.

yutakahirano · 2018-04-03T01:51:10Z

I think we should have more tests:

history.go(0)

A manual test you have to reload yourself

A POSTed navigation followed by a reload

History traversal followed by a reload.

Done.

yutakahirano · 2018-04-09T02:29:19Z

@annevk, are you happy with this change?

annevk

I'm not sure I'm the right person to review the service worker bits. Probably best to ask someone else.

annevk · 2018-04-10T07:18:06Z

fetch/api/request/request-navigation-type.html

+<script>
+test(() => {
+  assert_false(new Request('/').isReloadNavigation, 'isReloadNavigation');
+}, 'default values for navigation type attributes');


Can we merge this in somewhere else that checks default values?

annevk · 2018-04-10T07:19:43Z

Why does this only test this property btw? Don't we need tests for the other two properties two and their combinations?

yutakahirano · 2018-04-10T07:43:39Z

@matto @wanderview can you take a look at SW related tests?

yutakahirano · 2018-04-10T07:55:54Z

Why does this only test this property btw? Don't we need tests for the other two properties two and their combinations?

I like small changes and I would like to focus on one attribute at a time. I can leave whatwg/fetch#685 open until we land tests for other attributes, if you like.

mfalken · 2018-04-23T06:33:14Z

service-workers/service-worker/fetch-event-is-reload-navigation-manual.https.html

+  assert_equals(frame.contentDocument.body.textContent,
+      'method = GET, isReloadNavigation = true');
+  frame.remove();
+  await service_worker_unregister_and_done(t, scope);


can just call reg.unregister() instead (not sure it matters if you await or return it)

mfalken · 2018-04-23T06:39:59Z

service-workers/service-worker/fetch-event.https.html

+      if (frame) {
+        frame.remove();
+      }
+      await service_worker_unregister(t, scope);


mfalken · 2018-04-23T06:48:19Z

service-workers/service-worker/fetch-event-is-reload-navigation-manual.https.html

+  });
+  assert_equals(frame.contentDocument.body.textContent,
+      'method = GET, isReloadNavigation = true');
+  frame.remove();


It might be nice to test the normal case of a top-level navigation also, which should be possible since this is a manual case, but I imagine that can be convoluted.

mfalken · 2018-04-23T06:48:53Z

service-workers/service-worker/fetch-event.https.html

+      }
+      await service_worker_unregister(t, scope);
+    }
+  }, 'Service Worker responds to fetch event with the correct isReloadNavigation value (historg.go(0))');


mfalken · 2018-04-23T06:57:28Z

service-workers/service-worker/fetch-event.https.html

+      assert_equals(frame.contentDocument.body.textContent,
+                    'method = GET, isReloadNavigation = false');
+      // Wait a while for the history entry creation.
+      await wait(10);


This seems likely to be flaky on at least some browsers. Is it something needed for Chrome? If the spec says this creation should be synchronous, we could possibly remove the delay and have Chrome flakily fail the test (and move it to another file so not all of fetch-event.https.html is flaky).

If the spec allows async, could we instead poll for the change with a delay between turns of the loop?

This is for Blink and WebKit, and there is not flakiness. I added some comments.

mfalken · 2018-04-23T07:01:23Z

service-workers/service-worker/fetch-event.https.html

+      }
+      await service_worker_unregister(t, scope);
+    }
+  }, 'Service Worker responds to fetch event with the correct isReloadNavigation value (historg.go(0))');


I don't think the "Service Worker responds..." part is really needed but I know existing tests have this. Maybe just call it:
'FetchEvent#request.isReloadNavigation is true for history.go(0)"

mfalken · 2018-04-23T07:01:43Z

service-workers/service-worker/fetch-event.https.html

+      }
+      await service_worker_unregister(t, scope);
+    }
+  }, 'Service Worker responds to fetch event with the correct isReloadNavigation value (with history traversal)');


similar about "Service Worker responds..."

mfalken · 2018-04-23T07:02:36Z

service-workers/service-worker/fetch-event-is-reload-navigation-manual.https.html

+      'method = GET, isReloadNavigation = true');
+  frame.remove();
+  await service_worker_unregister_and_done(t, scope);
+}, 'Service Worker responds to fetch event with the correct isReloadNavigation value (manual reload)');


I don't think the "Service Worker responds to..." is really needed. Maybe just "FetchEvent#request.isReloadNavigation is true for manual reload."

mfalken

lgtm

mfalken · 2018-04-24T05:25:12Z

service-workers/service-worker/fetch-event.https.html

@@ -6,6 +6,9 @@
 <body>
 <script>
 var worker = 'resources/fetch-event-test-worker.js';
+function wait(ms) {
+    return new Promise(resolve => step_timeout(resolve, ms));


is it ok to call step_timeout without a test object, like test.step_timeout?

I think so, it's exposed explicitly.

mfalken · 2018-04-24T05:28:43Z

service-workers/service-worker/fetch-event.https.html

+        await reg.unregister();
+      }
+    }
+  }, 'Service Worker responds to fetch event with the correct isReloadNavigation value (location.reload())');


FetchEvent#request.isReloadNavigation is correct (location.reload())

mfalken · 2018-04-24T05:29:43Z

service-workers/service-worker/fetch-event.https.html

+      frame = await with_iframe(scope);
+      assert_equals(frame.contentDocument.body.textContent,
+                    'method = GET, isReloadNavigation = false');
+      // Blink and WebKit doesn't create a history entry for a navigation requested here, so


I removed the word as suggested below.

mfalken · 2018-04-24T05:35:04Z

service-workers/service-worker/fetch-event.https.html

+      assert_equals(frame.contentDocument.body.textContent,
+                    'method = GET, isReloadNavigation = false');
+      // Blink and WebKit doesn't create a history entry for a navigation requested here, so
+      // we need to use setTimeout to avoid that. See https://bugs.webkit.org/show_bug.cgi?id=42861.


maybe more direct as: Use setTimeout(0) to ensure the history entry is created for Blink and WebKit. See .

mfalken · 2018-04-24T05:36:08Z

service-workers/service-worker/fetch-event.https.html

+        frame.addEventListener('load', resolve);
+        frame.src = anotherUrl;
+      });
+      assert_equals(frame.contentDocument.body.textContent, "Here\'s a simple html file.\n");


can remove the '' when using double-quotes (same comment throughout)

annevk · 2018-04-24T10:17:57Z

I think the one thing that isn't tested here yet is whether this survives the Cache API. @wanderview suggested that it should. I don't know if that requires changes to the Cache API specification or not.

wanderview · 2018-04-24T13:34:22Z

The Cache API spec just stores the Request in a conceptual map, so it implies it should persist. It most likely does need code changes in actual implementations to persist through whatever disk schema is used.

yutakahirano · 2018-04-25T04:55:54Z

I'm not sure if we need Cache API tests (IIUC no test tests that for any attributes), but here it is. see service-workers/cache-storage files. @wanderview, @inexorabletash, can you take a look?

We don't need Cache API spec changes.

inexorabletash

/cache-storage/ changes LGTM, with a couple optional suggestions.

inexorabletash · 2018-04-26T17:32:34Z

service-workers/cache-storage/resources/cache-keys-attributes-for-service-worker.js

@@ -0,0 +1,23 @@
+self.addEventListener('fetch', function(event) {


nit: why not arrow function (=>) here?

inexorabletash · 2018-04-26T17:47:47Z

service-workers/cache-storage/resources/cache-keys-attributes-for-service-worker.js

+    if (url.endsWith('?ignore')) {
+      return;
+    }
+    const match = url.match(/\?name=(\w*)/)


Can we use the URL type to simplify parsing? e.g.

const params = new URL(event.request.url).searchParams;
if (params.has('ignore')) ...

if (!params.has('name')) ...

const name = params.get('name');

yutakahirano · 2018-04-26T23:07:21Z

Thanks, comments are addressed.

And also a member on the request concept (reload-navigation flag) to support this API. See w3c/ServiceWorker#1167 for the discussion that led to this. Tests: web-platform-tests/wpt#10192. Corresponding HTML change: whatwg/html#3592.

See w3c/ServiceWorker#1167 for the discussion that led to this change and whatwg/fetch#685 for the infrastructure in Fetch this builds upon. Tests: web-platform-tests/wpt#10192.

yutakahirano added 2 commits March 27, 2018 15:22

initial

6e2ca77

fix

d3cec90

wpt-pr-bot added fetch service-workers wg-sw labels Mar 27, 2018

wpt-pr-bot requested review from annevk, asutherland, beidson, jdm, mfalken, mkruisselbrink, mnot, wanderview and youennf March 27, 2018 06:28

yutakahirano added 4 commits April 2, 2018 16:25

fix

cd067a4

fix

45015a8

fix

6bf7012

fix

1614d65

fix

34bd5b1

annevk reviewed Apr 10, 2018

View reviewed changes

yutakahirano mentioned this pull request Apr 19, 2018

Introduce reload-navigation flag whatwg/fetch#685

Merged

fix

4325cea

mfalken requested changes Apr 23, 2018

View reviewed changes

yutakahirano added 2 commits April 23, 2018 16:18

wip

20f6049

fix

6af27ec

mfalken approved these changes Apr 24, 2018

View reviewed changes

yutakahirano added 4 commits April 24, 2018 14:40

fix

dc6406d

fix

3019e45

fix

5acf250

fix

7208979

fix

b98931a

wpt-pr-bot requested a review from inexorabletash April 25, 2018 04:53

fix

041ab04

inexorabletash approved these changes Apr 26, 2018

View reviewed changes

fix

a6f528f

annevk merged commit 58ee169 into master Apr 27, 2018

annevk deleted the yhirano/reload-navigation-type branch April 27, 2018 07:17

		@@ -0,0 +1,23 @@
		self.addEventListener('fetch', function(event) {

Add tests for Request.isReloadNavigation #10192

Add tests for Request.isReloadNavigation #10192

Conversation

yutakahirano commented Mar 27, 2018 • edited by wpt-pr-bot Loading

yutakahirano commented Mar 27, 2018

ghost commented Mar 27, 2018 • edited by ghost Loading

Build PASSED

annevk commented Mar 29, 2018

yutakahirano commented Apr 3, 2018

yutakahirano commented Apr 9, 2018

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk commented Apr 10, 2018

yutakahirano commented Apr 10, 2018

yutakahirano commented Apr 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfalken left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk commented Apr 24, 2018

wanderview commented Apr 24, 2018

yutakahirano commented Apr 25, 2018

inexorabletash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yutakahirano commented Apr 26, 2018

yutakahirano commented Mar 27, 2018 •

edited by wpt-pr-bot

Loading

ghost commented Mar 27, 2018 •

edited by ghost

Loading