Issue 1886953003: ServiceWorker: Add a test to check the behavior after reloading iframe

Created:
4 years, 8 months ago by shimazu (google)

Modified:
4 years, 8 months ago

Reviewers:
falken, nhiroki

CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, kenjibaheux+watch_chromium.org, tzik, serviceworker-reviews, nhiroki, kinuko+serviceworker, blink-reviews, horo+watch_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

ServiceWorker: Add a test to check the behavior after reloading iframe There seems to be some different behaviors between firefox and chrome when reloading iframe controlled by a service worker after unregistering or re-registering. This new test is to check if "Clear" and "Activate" process are not invoked after reloading an iframe controlled by a service worker. Expected behavior is to execute neither "Clear" nor "Activate" process according to comments at http://crrev.com/1865103003#msg29 Currently the behavior of chromium is different from the expected one, so this should be fixed by the another bug/CL. BUG=594032 TEST=./third_party/WebKit/Tools/Scripts/run-webkit-tests -t Default http/tests/serviceworker/chromium/reloading-iframe.html

Patch Set 1 #

Patch Set 2 : Wrap at 80 columns #

Total comments: 8

Created: 4 years, 8 months ago

Download [raw] [tar.bz2]

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+112 lines, -39 lines)			Patch
	A	third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/reloading-iframe.html	View	1	1 chunk	+112 lines, -0 lines	8 comments	Download
	M	third_party/WebKit/LayoutTests/http/tests/serviceworker/unregister-then-register.html	View		1 chunk	+0 lines, -39 lines	0 comments	Download

Messages

Total messages: 4 (1 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

shimazu (google)

PTAL This test shows "Activate" process is not invoked after reloading though "Clear" process is ...

4 years, 8 months ago (2016-04-14 09:17:27 UTC) #2

falken

One round of comments. > Expected behavior is to execute neither "Clear" nor "Activate" process ...

4 years, 8 months ago (2016-04-14 18:34:57 UTC) #3

One round of comments.

> Expected behavior is to execute neither "Clear" nor "Activate"
process according to comments at http://crrev.com/1865103003#msg29

I said it'd be surprised if it's expected behavior :)

I'd like to raise a spec issue to confirm, and just wanted to first check chrome
and firefox's behavior regarding clear and activate when reloading the last
iframe.

https://codereview.chromium.org/1886953003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/reloading-iframe.html
(right):

https://codereview.chromium.org/1886953003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/reloading-iframe.html:1:
<!DOCTYPE html>
I think we should keep this outside of chromium/ because it's desired
destination is web-platform-tests. Of course, the implementations and tests
would have to be fixed before upstreaming, but we would catch it during that
process instead of ignoring it because it's in chromium/.

https://codereview.chromium.org/1886953003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/reloading-iframe.html:18:
service_worker_unregister(t, scope);
Can this just be r.unregister()?

https://codereview.chromium.org/1886953003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/reloading-iframe.html:23:
'.installing should be set just after registration');
Let's remove this assert. Most of our tests have this
wait_for_state(r.installing) boilerplate.

https://codereview.chromium.org/1886953003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/reloading-iframe.html:33:
'after registration');
Let's remove this assert, it should be covered by another test.

https://codereview.chromium.org/1886953003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/reloading-iframe.html:51:
'controlled by this worker');
Let's make these:
assert_equals(registration.waiting, new_worker)
assert_equals(registration.active, old_worker)

https://codereview.chromium.org/1886953003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/reloading-iframe.html:63:
'controlled by this worker');
The descriptions should be updated to mention the reload.

https://codereview.chromium.org/1886953003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/reloading-iframe.html:66:
'after re-registration');
Can this assert be more specific? c.scriptURL == old_worker.scriptURL? I'm
guessing c == old_worker doesn't work because they are different contexts. Also
the description should mention the reload, not re-registration.

https://codereview.chromium.org/1886953003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/reloading-iframe.html:69:
'invoke Activate process of the registration');
In spec language, it's called Activate algorithm. I think this can just be
shortened to "Reloading the only controlled client (iframe) does not trigger
activation"

falken

4 years, 8 months ago (2016-04-14 18:38:28 UTC) #4

On 2016/04/14 18:34:57, falken wrote:
> One round of comments.
> 
> > Expected behavior is to execute neither "Clear" nor "Activate"
> process according to comments at http://crrev.com/1865103003#msg29
> 
> I said it'd be surprised if it's expected behavior :)

Correction: surprised if it's not expected.

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages