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

ServiceWorker: Registering a malformed script should fail [1/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.

To enable the installing sequence to wait for the script evaluation, this adds
WorkerReportingProxy::didEvaluteWorkerScript() to be called when the
evaluation is completed and to propagate the result up to the Chromium-side.

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

BUG=426344
TEST=compile (test will be added by [3])

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184854

[nhiroki, 2014-10-31 09:40:07]

Patchset #1 (id:1) has been deleted

[nhiroki, 2014-10-31 09:40:13]

Patchset #1 (id:20001) has been deleted

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

Patchset #1 (id:40001) has been deleted

[nhiroki, 2014-11-04 03:54:23]

Patchset #1 (id:60001) has been deleted

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

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

[nhiroki, 2014-11-04 04:02:49]

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

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

Hi, can you review this?

[falken, 2014-11-04 05:41:26]

looks good

What happens with a worker that does while(1) {}? I guess register job will wait
indefinitely and the queue for the scope will be blocked, same as waitUntil(new
Promise(){})
We don't have to fix that in this CL but we should make a note in the bug
tracker.

https://codereview.chromium.org/692003002/diff/80001/Source/core/workers/Work...
File Source/core/workers/WorkerReportingProxy.h (right):

https://codereview.chromium.org/692003002/diff/80001/Source/core/workers/Work...
Source/core/workers/WorkerReportingProxy.h:43: // APIs used by workers to report
console activity.
this looks out of date now... "console and worker activity"?

https://codereview.chromium.org/692003002/diff/80001/Source/core/workers/Work...
Source/core/workers/WorkerReportingProxy.h:53: // evaluation succeeded.
One might wonder what it means for an evaluation to "succeed", maybe say
"evaluation completed with no uncaught exception"

https://codereview.chromium.org/692003002/diff/80001/Source/core/workers/Work...
Source/core/workers/WorkerReportingProxy.h:54: virtual void
workerScriptEvaluated(bool result) = 0;
"result" is not so clear of a name, how about bool success?

[nhiroki, 2014-11-04 09:03:12]

Thank you for reviewing! Updated.

https://codereview.chromium.org/692003002/diff/80001/Source/core/workers/Work...
File Source/core/workers/WorkerReportingProxy.h (right):

https://codereview.chromium.org/692003002/diff/80001/Source/core/workers/Work...
Source/core/workers/WorkerReportingProxy.h:43: // APIs used by workers to report
console activity.
On 2014/11/04 05:41:26, falken wrote:
> this looks out of date now... "console and worker activity"?

Done.

https://codereview.chromium.org/692003002/diff/80001/Source/core/workers/Work...
Source/core/workers/WorkerReportingProxy.h:53: // evaluation succeeded.
On 2014/11/04 05:41:26, falken wrote:
> One might wonder what it means for an evaluation to "succeed", maybe say
> "evaluation completed with no uncaught exception"

Right, "succeed" is not descriptive. Rephrased.

https://codereview.chromium.org/692003002/diff/80001/Source/core/workers/Work...
Source/core/workers/WorkerReportingProxy.h:54: virtual void
workerScriptEvaluated(bool result) = 0;
On 2014/11/04 05:41:26, falken wrote:
> "result" is not so clear of a name, how about bool success?

Done.

[nhiroki, 2014-11-04 09:06:45]

On 2014/11/04 05:41:26, falken wrote:
> looks good
> 
> What happens with a worker that does while(1) {}? I guess register job will
wait
> indefinitely and the queue for the scope will be blocked, same as
waitUntil(new
> Promise(){})
> We don't have to fix that in this CL but we should make a note in the bug
> tracker.

You're right. I made sure that an infinite loop can block the registration. I'll
comment on the issue or file a new bug.

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

LGTM

[nhiroki, 2014-11-05 01:58:47]

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

[nhiroki, 2014-11-05 02:00:35]

+tkent@, can you review this? (especially,
public/web/WebServiceWorkerContextClient.h)

[tkent, 2014-11-05 02:10:22]

https://codereview.chromium.org/692003002/diff/100001/Source/core/workers/Wor...
File Source/core/workers/WorkerReportingProxy.h (right):

https://codereview.chromium.org/692003002/diff/100001/Source/core/workers/Wor...
Source/core/workers/WorkerReportingProxy.h:54: virtual void
workerScriptEvaluated(bool success) = 0;
Please name this as |didEvaluateWorkerScript|.  |will| and |did| are common
prefixes for notification receiver functions in Blink.

We usually avoid bool arguments, however the bool argument of this function is
acceptable in this case because literal |true| or |false| won't appear in the
source code.

https://codereview.chromium.org/692003002/diff/100001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/692003002/diff/100001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:326: // Notify proxy that a worker script
has been evaluated.
This comment is helpless.  Please remove it.

[nhiroki, 2014-11-05 02:26:50]

Patchset #3 (id:120001) has been deleted

[nhiroki, 2014-11-05 03:09:48]

Thank you for reviewing. Updated.

https://codereview.chromium.org/692003002/diff/100001/Source/core/workers/Wor...
File Source/core/workers/WorkerReportingProxy.h (right):

https://codereview.chromium.org/692003002/diff/100001/Source/core/workers/Wor...
Source/core/workers/WorkerReportingProxy.h:54: virtual void
workerScriptEvaluated(bool success) = 0;
On 2014/11/05 02:10:22, tkent wrote:
> Please name this as |didEvaluateWorkerScript|.  |will| and |did| are common
> prefixes for notification receiver functions in Blink.
> 
> We usually avoid bool arguments, however the bool argument of this function is
> acceptable in this case because literal |true| or |false| won't appear in the
> source code.

Done.

https://codereview.chromium.org/692003002/diff/100001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/692003002/diff/100001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:326: // Notify proxy that a worker script
has been evaluated.
On 2014/11/05 02:10:22, tkent wrote:
> This comment is helpless.  Please remove it.

Done.

[yhirano, 2014-11-05 03:41:09]

yhirano@chromium.org changed reviewers:
+ yhirano@chromium.org

[yhirano, 2014-11-05 03:41:10]

bindings/ LGTM

https://codereview.chromium.org/692003002/diff/140001/Source/bindings/core/v8...
File Source/bindings/core/v8/WorkerScriptController.h (right):

https://codereview.chromium.org/692003002/diff/140001/Source/bindings/core/v8...
Source/bindings/core/v8/WorkerScriptController.h:55: bool evaluate(const
ScriptSourceCode&, RefPtrWillBeRawPtr<ErrorEvent>* = 0);
// Returns true if the evaluation is done successfully.

[haraken, 2014-11-05 04:26:49]

LGTM

https://codereview.chromium.org/692003002/diff/140001/public/web/WebServiceWo...
File public/web/WebServiceWorkerContextClient.h (right):

https://codereview.chromium.org/692003002/diff/140001/public/web/WebServiceWo...
public/web/WebServiceWorkerContextClient.h:99: virtual void
didEvaluateWorkerScript(bool result) { }

result => success

[tkent, 2014-11-05 04:57:21]

lgtm

[nhiroki, 2014-11-05 05:33:40]

Thank you all!

https://codereview.chromium.org/692003002/diff/140001/Source/bindings/core/v8...
File Source/bindings/core/v8/WorkerScriptController.h (right):

https://codereview.chromium.org/692003002/diff/140001/Source/bindings/core/v8...
Source/bindings/core/v8/WorkerScriptController.h:55: bool evaluate(const
ScriptSourceCode&, RefPtrWillBeRawPtr<ErrorEvent>* = 0);
On 2014/11/05 03:41:10, yhirano wrote:
> // Returns true if the evaluation is done successfully.

Done.

https://codereview.chromium.org/692003002/diff/140001/public/web/WebServiceWo...
File public/web/WebServiceWorkerContextClient.h (right):

https://codereview.chromium.org/692003002/diff/140001/public/web/WebServiceWo...
public/web/WebServiceWorkerContextClient.h:99: virtual void
didEvaluateWorkerScript(bool result) { }
On 2014/11/05 04:26:49, haraken wrote:
> 
> result => success

Done.

[nhiroki, 2014-11-05 05:34:09]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2014-11-05 05:35:12]

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

[commit-bot: I haz the power, 2014-11-05 08:08:49]

Committed patchset #4 (id:160001) as 184854