ServiceWorker: Registering a malformed script should fail [2/3]

ServiceWorker: Registering a malformed script should fail [2/3]

Before this series of patches, registration succeeds even if the worker script
has a syntax error because the installing sequence does not check the result of
the script evaluation.

This makes EmbeddedWorkerInstance wait for the completion and reject an install
promise if the evaluation fails.

Patch dependency:
[1] Blink: https://codereview.chromium.org/692003002/
[2] Chromium: THIS PATCH
[3] Blink: https://codereview.chromium.org/697833004/

BUG=426344
TEST=run-webkit-tests --debug http/tests/serviceworker/
TEST=content_unittests --gtest_filter=ServiceWorker*

Committed: https://crrev.com/fce743b03558b4ea6605c8eb0c43918e02eddf9b
Cr-Commit-Position: refs/heads/master@{#302970}

[nhiroki, 2014-10-31 09:12:43]

Patchset #1 (id:1) has been deleted

[nhiroki, 2014-11-04 03:46:38]

Patchset #1 (id:20001) has been deleted

[nhiroki, 2014-11-04 03:50:19]

Patchset #1 (id:40001) has been deleted

[nhiroki, 2014-11-04 04:03:29]

nhiroki@chromium.org changed reviewers:
+ falken@chromium.org

[nhiroki, 2014-11-04 04:03:52]

Ptal, thanks!

[nhiroki, 2014-11-04 09:01:07]

Patchset #2 (id:80001) has been deleted

[falken, 2014-11-05 01:47:37]

LGTM

[nhiroki, 2014-11-05 02:03:01]

nhiroki@chromium.org changed reviewers:
+ nasko@chromium.org

[nhiroki, 2014-11-05 02:03:02]

+nasko@, can you review embedded_worker_messages.h?

[nasko, 2014-11-05 17:40:23]

LGTM with a nit.

https://codereview.chromium.org/697593002/diff/120001/content/browser/service...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/697593002/diff/120001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host.cc:621: int
embedded_worker_id, bool success) {
nit: each parameter on new line

[nhiroki, 2014-11-06 03:04:41]

Thank you!

https://codereview.chromium.org/697593002/diff/120001/content/browser/service...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/697593002/diff/120001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host.cc:621: int
embedded_worker_id, bool success) {
On 2014/11/05 17:40:23, nasko wrote:
> nit: each parameter on new line

Done.

[nhiroki, 2014-11-06 03:06:51]

falken@: Can you take another look? I updated EmbeddedWorkerInstanceTest to
handle WorkerScriptEvaluated message (Patch Set 4). Thanks!

[falken, 2014-11-06 03:15:32]

https://codereview.chromium.org/697593002/diff/140001/content/browser/service...
File content/browser/service_worker/embedded_worker_instance_unittest.cc
(right):

https://codereview.chromium.org/697593002/diff/140001/content/browser/service...
content/browser/service_worker/embedded_worker_instance_unittest.cc:79:
base::RunLoop().RunUntilIdle();
I'm not sure I understand base::RunLoop. Do you need both Run() and
RunUntilIdle()?

https://codereview.chromium.org/697593002/diff/140001/content/browser/service...
content/browser/service_worker/embedded_worker_instance_unittest.cc:81: //
Script evaluated message should be notified (by EmbeddedWorkerTestHelper).
I don't understand this comment, both the wording and its relation to the code.
I think it means "The 'script evaluated' message should have been received"...
but it's not clear what that has to do with the next line of code.

[nhiroki, 2014-11-06 04:12:47]

https://codereview.chromium.org/697593002/diff/140001/content/browser/service...
File content/browser/service_worker/embedded_worker_instance_unittest.cc
(right):

https://codereview.chromium.org/697593002/diff/140001/content/browser/service...
content/browser/service_worker/embedded_worker_instance_unittest.cc:79:
base::RunLoop().RunUntilIdle();
On 2014/11/06 03:15:31, falken wrote:
> I'm not sure I understand base::RunLoop. Do you need both Run() and
> RunUntilIdle()?

Yes, this is not necessary. Removed.

(I thought the same thing but left it as it was because I wanted to avoid
unrelated fix.)

https://codereview.chromium.org/697593002/diff/140001/content/browser/service...
content/browser/service_worker/embedded_worker_instance_unittest.cc:81: //
Script evaluated message should be notified (by EmbeddedWorkerTestHelper).
On 2014/11/06 03:15:31, falken wrote:
> I don't understand this comment, both the wording and its relation to the
code.
> I think it means "The 'script evaluated' message should have been received"...

Fixed.

> but it's not clear what that has to do with the next line of code.

|status| is set in the callback |SaveStatusAndCall| and the callback is executed
when the 'script evaluated' message arrived. This comment meant to say that the
expectation makes sure the message has been received.

Actually, the message reachability is confirmed by |run_loop.Run()|, so this
comment seems confusing. Removed.

[falken, 2014-11-06 04:36:30]

lgtm

https://codereview.chromium.org/697593002/diff/160001/content/browser/service...
File content/browser/service_worker/embedded_worker_instance_unittest.cc
(right):

https://codereview.chromium.org/697593002/diff/160001/content/browser/service...
content/browser/service_worker/embedded_worker_instance_unittest.cc:81: // The
'WorkerStarted' message should be notified.
Ah, this is confusing grammar. You notify a listener of a message, you don't
notify a message itself. I think this should be: "The WorkerStarted message
should have been sent (by EmbeddedWorkerTestHelper)."

[nhiroki, 2014-11-06 04:44:53]

Patchset #6 (id:180001) has been deleted

[nhiroki, 2014-11-06 04:47:13]

https://codereview.chromium.org/697593002/diff/160001/content/browser/service...
File content/browser/service_worker/embedded_worker_instance_unittest.cc
(right):

https://codereview.chromium.org/697593002/diff/160001/content/browser/service...
content/browser/service_worker/embedded_worker_instance_unittest.cc:81: // The
'WorkerStarted' message should be notified.
On 2014/11/06 04:36:30, falken wrote:
> Ah, this is confusing grammar. You notify a listener of a message, you don't
> notify a message itself. I think this should be: "The WorkerStarted message
> should have been sent (by EmbeddedWorkerTestHelper)."

Oh... sorry for my injudicious changes :( Fixed.

[nhiroki, 2014-11-06 04:47:22]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2014-11-06 04:48:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/697593002/200001

[commit-bot: I haz the power, 2014-11-06 06:30:41]

Committed patchset #6 (id:200001)

[commit-bot: I haz the power, 2014-11-06 06:31:30]

Patchset 6 (id:??) landed as
https://crrev.com/fce743b03558b4ea6605c8eb0c43918e02eddf9b
Cr-Commit-Position: refs/heads/master@{#302970}