Shows a warning when adding fetch handler after the first evaluation (issue 2319563003 by shimazu@chromium.org)

306 views
Skip to first unread message

shi...@chromium.org

unread,
Sep 8, 2016, 12:20:43 AM9/8/16
to fal...@chromium.org, chromium...@chromium.org, mich...@chromium.org, jsbell+ser...@chromium.org, tz...@chromium.org, servicewor...@chromium.org, nhi...@chromium.org, fal...@chromium.org, har...@chromium.org, kinuko+ser...@chromium.org, blink-...@chromium.org, horo+...@chromium.org
Reviewers: falken
CL: https://codereview.chromium.org/2319563003/

Message:
ptal

Description:
Shows a warning when adding fetch handler after the first evaluation

As shown at 15 on [1], "The user agents are encouraged to show a warning that
the event listeners must be added on the very first evaluation of the worker
script.". This patch adds warning messages for fetch event and foreignfetch
event.

[1]:
https://w3c.github.io/ServiceWorker/spec/service_worker/index.html#run-service-worker-algorithm

BUG=644617

Affected files (+6, -0 lines):
M third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp


Index: third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp
diff --git a/third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp b/third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp
index 144f0b649f54ede118ad8d47fb2d97f6b80cdeed..3f93b669566d98010e3a0b91774f315812aa3d8d 100644
--- a/third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp
+++ b/third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp
@@ -159,6 +159,12 @@ bool ServiceWorkerGlobalScope::addEventListenerInternal(const AtomicString& even
} else if (eventType == EventTypeNames::activate) {
ConsoleMessage* consoleMessage = ConsoleMessage::create(JSMessageSource, WarningMessageLevel, "Event handler of 'activate' event must be added on the initial evaluation of worker script.");
addConsoleMessage(consoleMessage);
+ } else if (eventType == EventTypeNames::fetch) {
+ ConsoleMessage* consoleMessage = ConsoleMessage::create(JSMessageSource, WarningMessageLevel, "Event handler of 'fetch' event must be added on the initial evaluation of worker script.");
+ addConsoleMessage(consoleMessage);
+ } else if (eventType == EventTypeNames::foreignfetch) {
+ ConsoleMessage* consoleMessage = ConsoleMessage::create(JSMessageSource, WarningMessageLevel, "Event handler of 'foreignfetch' event must be added on the initial evaluation of worker script.");
+ addConsoleMessage(consoleMessage);
}
}
return WorkerGlobalScope::addEventListenerInternal(eventType, listener, options);


fal...@chromium.org

unread,
Sep 9, 2016, 3:49:18 AM9/9/16
to shimaz...@chromium.org, chromium...@chromium.org, mich...@chromium.org, jsbell+ser...@chromium.org, tz...@chromium.org, servicewor...@chromium.org, nhi...@chromium.org, har...@chromium.org, kinuko+ser...@chromium.org, blink-...@chromium.org, horo+...@chromium.org
Sorry to get to this late.

Is there any reason not to just show this warning for all event types?

https://codereview.chromium.org/2319563003/

shi...@chromium.org

unread,
Sep 13, 2016, 11:49:15 PM9/13/16
to fal...@chromium.org, chromium...@chromium.org, mich...@chromium.org, jsbell+ser...@chromium.org, tz...@chromium.org, servicewor...@chromium.org, nhi...@chromium.org, fal...@chromium.org, har...@chromium.org, kinuko+ser...@chromium.org, blink-...@chromium.org, horo+...@chromium.org
On 2016/09/09 07:49:16, falken wrote:
> Sorry to get to this late.
>
> Is there any reason not to just show this warning for all event types?

No, that's because just I didn't understand the meaning of 'the event listeners'
(I thought 'the' meant listeners related to the service workers)!
Updated.


https://codereview.chromium.org/2319563003/

fal...@chromium.org

unread,
Sep 13, 2016, 11:56:25 PM9/13/16
to shimaz...@chromium.org, chromium...@chromium.org, mich...@chromium.org, jsbell+ser...@chromium.org, tz...@chromium.org, servicewor...@chromium.org, nhi...@chromium.org, har...@chromium.org, kinuko+ser...@chromium.org, blink-...@chromium.org, horo+...@chromium.org
Thanks! Can you dupe bugs 594160 and 644617 together and update the CL
description?

Also I think we now understand more about how to test things that happen in
DevTools. Is it possible to write a test for this (probably in
http/inspector/service-workers)?

https://codereview.chromium.org/2319563003/

shi...@chromium.org

unread,
Sep 15, 2016, 5:50:56 AM9/15/16
to fal...@chromium.org, chromium...@chromium.org, mich...@chromium.org, jsbell+ser...@chromium.org, tz...@chromium.org, servicewor...@chromium.org, nhi...@chromium.org, fal...@chromium.org, har...@chromium.org, kinuko+ser...@chromium.org, blink-...@chromium.org, horo+...@chromium.org
On 2016/09/14 03:56:24, falken wrote:
> Thanks! Can you dupe bugs 594160 and 644617 together and update the CL
> description?
>
> Also I think we now understand more about how to test things that happen in
> DevTools. Is it possible to write a test for this (probably in
> http/inspector/service-workers)?

Merged and added a test:)
ptal again

https://codereview.chromium.org/2319563003/

fal...@chromium.org

unread,
Sep 19, 2016, 9:46:42 PM9/19/16
to shimaz...@chromium.org, chromium...@chromium.org, mich...@chromium.org, jsbell+ser...@chromium.org, tz...@chromium.org, servicewor...@chromium.org, nhi...@chromium.org, har...@chromium.org, kinuko+ser...@chromium.org, blink-...@chromium.org, horo+...@chromium.org
Thanks for the test!


https://codereview.chromium.org/2319563003/diff/60001/third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/lazy-addeventlisteners.html
File
third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/lazy-addeventlisteners.html
(right):

https://codereview.chromium.org/2319563003/diff/60001/third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/lazy-addeventlisteners.html#newcode20
third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/lazy-addeventlisteners.html:20:
<p>Warning should be shown at the console when calling addEventListener
after the first evaluation.</p>
"the first evaluation" is ambiguous.

"Tests that a warning is shown in the console if addEventListener is
called after initial evaluation of the service worker script."

https://codereview.chromium.org/2319563003/diff/60001/third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/resources/service-worker-lazy-addeventlistener.js
File
third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/resources/service-worker-lazy-addeventlistener.js
(right):

https://codereview.chromium.org/2319563003/diff/60001/third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/resources/service-worker-lazy-addeventlistener.js#newcode6
third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/resources/service-worker-lazy-addeventlistener.js:6:
}, 10);
setTimeout is always unfortunate: it places a lower bound on the time
required for the test to complete. Please use setTimeout 0 or redesign
the test to not require it: e.g., call addEventListener() inside an
install event handler.

https://codereview.chromium.org/2319563003/diff/60001/third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/resources/service-worker-lazy-addeventlistener.js#newcode7
third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/resources/service-worker-lazy-addeventlistener.js:7:
console.log('Here is in the first execution.');
I think you could simplify the test by removing these console.logs.

https://codereview.chromium.org/2319563003/diff/60001/third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp
File
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp
(right):

https://codereview.chromium.org/2319563003/diff/60001/third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp#newcode163
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp:163:
String message = String::format("Event handler of '%s' event must be
added on the initial evaluation of worker script.",
eventType.ascii().data());
Maybe I'm paranoid but I'd just utf8() instead of ascii(), performance
isn't an issue for this console warning.

https://codereview.chromium.org/2319563003/

shi...@chromium.org

unread,
Sep 20, 2016, 2:07:53 AM9/20/16
to fal...@chromium.org, chromium...@chromium.org, mich...@chromium.org, jsbell+ser...@chromium.org, tz...@chromium.org, servicewor...@chromium.org, nhi...@chromium.org, fal...@chromium.org, har...@chromium.org, kinuko+ser...@chromium.org, blink-...@chromium.org, horo+...@chromium.org
Updated.
PTAL again! :)



https://codereview.chromium.org/2319563003/diff/60001/third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/lazy-addeventlisteners.html
File
third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/lazy-addeventlisteners.html
(right):

https://codereview.chromium.org/2319563003/diff/60001/third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/lazy-addeventlisteners.html#newcode20
third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/lazy-addeventlisteners.html:20:
<p>Warning should be shown at the console when calling addEventListener
after the first evaluation.</p>
On 2016/09/20 01:46:41, falken wrote:
> "the first evaluation" is ambiguous.
>
> "Tests that a warning is shown in the console if addEventListener is
called
> after initial evaluation of the service worker script."

Done.


https://codereview.chromium.org/2319563003/diff/60001/third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/resources/service-worker-lazy-addeventlistener.js
File
third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/resources/service-worker-lazy-addeventlistener.js
(right):

https://codereview.chromium.org/2319563003/diff/60001/third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/resources/service-worker-lazy-addeventlistener.js#newcode6
third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/resources/service-worker-lazy-addeventlistener.js:6:
}, 10);
On 2016/09/20 01:46:41, falken wrote:
> setTimeout is always unfortunate: it places a lower bound on the time
required
> for the test to complete. Please use setTimeout 0 or redesign the test
to not
> require it: e.g., call addEventListener() inside an install event
handler.

Done.


https://codereview.chromium.org/2319563003/diff/60001/third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/resources/service-worker-lazy-addeventlistener.js#newcode7
third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/resources/service-worker-lazy-addeventlistener.js:7:
console.log('Here is in the first execution.');
On 2016/09/20 01:46:41, falken wrote:
> I think you could simplify the test by removing these console.logs.

Done.


https://codereview.chromium.org/2319563003/diff/60001/third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp
File
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp
(right):

https://codereview.chromium.org/2319563003/diff/60001/third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp#newcode163
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp:163:
String message = String::format("Event handler of '%s' event must be
added on the initial evaluation of worker script.",
eventType.ascii().data());
On 2016/09/20 01:46:41, falken wrote:
> Maybe I'm paranoid but I'd just utf8() instead of ascii(), performance
isn't an
> issue for this console warning.

fal...@chromium.org

unread,
Sep 20, 2016, 2:10:54 AM9/20/16
to shimaz...@chromium.org, chromium...@chromium.org, mich...@chromium.org, jsbell+ser...@chromium.org, tz...@chromium.org, servicewor...@chromium.org, nhi...@chromium.org, har...@chromium.org, kinuko+ser...@chromium.org, blink-...@chromium.org, horo+...@chromium.org
lgtm, thank you.

CL description nit: add something on the first line to say this commit is about
service workers.

https://codereview.chromium.org/2319563003/

commit-bot@chromium.org via codereview.chromium.org

unread,
Sep 20, 2016, 2:20:27 AM9/20/16
to shimaz...@chromium.org, fal...@chromium.org, commi...@chromium.org, chromium...@chromium.org, mich...@chromium.org, jsbell+ser...@chromium.org, tz...@chromium.org, servicewor...@chromium.org, nhi...@chromium.org, fal...@chromium.org, har...@chromium.org, kinuko+ser...@chromium.org, blink-...@chromium.org, horo+...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Sep 20, 2016, 3:34:43 AM9/20/16
to shimaz...@chromium.org, fal...@chromium.org, commi...@chromium.org, chromium...@chromium.org, mich...@chromium.org, jsbell+ser...@chromium.org, tz...@chromium.org, servicewor...@chromium.org, nhi...@chromium.org, fal...@chromium.org, har...@chromium.org, kinuko+ser...@chromium.org, blink-...@chromium.org, horo+...@chromium.org
Try jobs failed on following builders:
win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/296013)

https://codereview.chromium.org/2319563003/

commit-bot@chromium.org via codereview.chromium.org

unread,
Sep 20, 2016, 10:27:47 PM9/20/16
to shimaz...@chromium.org, fal...@chromium.org, commi...@chromium.org, chromium...@chromium.org, mich...@chromium.org, jsbell+ser...@chromium.org, tz...@chromium.org, servicewor...@chromium.org, nhi...@chromium.org, fal...@chromium.org, har...@chromium.org, kinuko+ser...@chromium.org, blink-...@chromium.org, horo+...@chromium.org
CQ is trying da patch. Follow status at

commit-bot@chromium.org via codereview.chromium.org

unread,
Sep 20, 2016, 11:47:18 PM9/20/16
to shimaz...@chromium.org, fal...@chromium.org, commi...@chromium.org, chromium...@chromium.org, mich...@chromium.org, jsbell+ser...@chromium.org, tz...@chromium.org, servicewor...@chromium.org, nhi...@chromium.org, fal...@chromium.org, har...@chromium.org, kinuko+ser...@chromium.org, blink-...@chromium.org, horo+...@chromium.org
Committed patchset #7 (id:120001)

https://codereview.chromium.org/2319563003/

commit-bot@chromium.org via codereview.chromium.org

unread,
Sep 20, 2016, 11:49:01 PM9/20/16
to shimaz...@chromium.org, fal...@chromium.org, commi...@chromium.org, chromium...@chromium.org, mich...@chromium.org, jsbell+ser...@chromium.org, tz...@chromium.org, servicewor...@chromium.org, nhi...@chromium.org, fal...@chromium.org, har...@chromium.org, kinuko+ser...@chromium.org, blink-...@chromium.org, horo+...@chromium.org
Patchset 7 (id:??) landed as
https://crrev.com/e14cea541b991c7907123395ea663f39eea7dcfa
Cr-Commit-Position: refs/heads/master@{#419967}

https://codereview.chromium.org/2319563003/
Reply all
Reply to author
Forward
0 new messages