Service Worker: Correct Web IDL of ExtendableEvent.waitUntil method.

Service Worker: Correct Web IDL of ExtendableEvent.waitUntil method.

As per the spec, ExtendableEvent.waitUntil() method's argument should be of type
Promise<any> instead of any. For the non-promise value, the Web IDL
automatically casts it to a promise via Promise.resolve. So this CL moves the
ScriptPromise::cast from Blink to V8 layer. This change does not affect the 
existing implementation.

Spec: https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#wait-until-method

BUG=514042
Committed: https://crrev.com/2223c104b56eaed9abc1d1f69723a2c7c8f61bda
git-svn-id: svn://svn.chromium.org/blink/trunk@199643 bbb929c8-8fbe-4397-9dbb-9b2b20218538

[jungkees, 2015-07-26 12:44:06]

jungkee.song@samsung.com changed reviewers:
+ jinho.bang@samsung.com, l.gombos@samsung.com

[jungkees, 2015-07-26 12:44:31]

PTAL

[zino, 2015-07-27 06:04:34]

On 2015/07/26 12:44:31, jungkees wrote:
> PTAL

There is no any regression.
Looks good to me.

[jungkees, 2015-07-27 06:25:14]

jungkee.song@samsung.com changed reviewers:
+ falken@chromium.org, kinuko@chromium.org, michaeln@chromium.org,
nhiroki@chromium.org, yhirano@chromium.org

[jungkees, 2015-07-27 06:28:00]

Thanks for peer-review zino@.
OWNERs, PTAL.

[yhirano, 2015-07-27 12:21:24]

https://codereview.chromium.org/1256133002/diff/20001/Source/modules/servicew...
File Source/modules/serviceworkers/ExtendableEvent.h (right):

https://codereview.chromium.org/1256133002/diff/20001/Source/modules/servicew...
Source/modules/serviceworkers/ExtendableEvent.h:52: void waitUntil(ScriptState*,
ScriptPromise&, ExceptionState&);
Can you use |ScriptPromise| instead of |ScriptPromise&|?

https://codereview.chromium.org/1256133002/diff/20001/Source/modules/servicew...
File Source/modules/serviceworkers/WaitUntilObserver.h (right):

https://codereview.chromium.org/1256133002/diff/20001/Source/modules/servicew...
Source/modules/serviceworkers/WaitUntilObserver.h:43: void
waitUntil(ScriptState*, ScriptPromise&, ExceptionState&);
Can you use |ScriptPromise| instead of |ScriptPromise&| ?

[jungkees, 2015-07-27 12:37:49]

Thanks for review. I left a question. Could you please explain?

https://codereview.chromium.org/1256133002/diff/20001/Source/modules/servicew...
File Source/modules/serviceworkers/ExtendableEvent.h (right):

https://codereview.chromium.org/1256133002/diff/20001/Source/modules/servicew...
Source/modules/serviceworkers/ExtendableEvent.h:52: void waitUntil(ScriptState*,
ScriptPromise&, ExceptionState&);
Sure, but could you explain why? Both scriptPromise and exceptionState are
passed references. Is there any case where the object created in V8 layer
destroyed before the object passed to Blink layer? If it should, I'll probably
have to change the same for FetchEvent.respondWith code where I used references.
Thanks!

[yhirano, 2015-07-27 12:45:32]

https://codereview.chromium.org/1256133002/diff/20001/Source/modules/servicew...
File Source/modules/serviceworkers/ExtendableEvent.h (right):

https://codereview.chromium.org/1256133002/diff/20001/Source/modules/servicew...
Source/modules/serviceworkers/ExtendableEvent.h:52: void waitUntil(ScriptState*,
ScriptPromise&, ExceptionState&);
On 2015/07/27 12:37:49, jungkees wrote:
> Sure, but could you explain why? Both scriptPromise and exceptionState are
> passed references. Is there any case where the object created in V8 layer
> destroyed before the object passed to Blink layer? If it should, I'll probably
> have to change the same for FetchEvent.respondWith code where I used
references.
> Thanks! 

ScriptPromise is a value and copying it is not expensive. I would rather use
value than non-const reference in such cases. ExceptionState is different: It is
not copyable, and that non-const reference intends to update the state.

I'm sorry I overlooked the point in the last CL.

[jungkees, 2015-07-27 14:23:33]

I'll upload a new snapshot addressing it. Thanks.

https://codereview.chromium.org/1256133002/diff/20001/Source/modules/servicew...
File Source/modules/serviceworkers/ExtendableEvent.h (right):

https://codereview.chromium.org/1256133002/diff/20001/Source/modules/servicew...
Source/modules/serviceworkers/ExtendableEvent.h:52: void waitUntil(ScriptState*,
ScriptPromise&, ExceptionState&);
On 2015/07/27 12:45:32, yhirano wrote:
> ScriptPromise is a value and copying it is not expensive. I would rather use
> value than non-const reference in such cases. ExceptionState is different: It
is
> not copyable, and that non-const reference intends to update the state.
> 

Agreed. Thanks for the explanation!

> I'm sorry I overlooked the point in the last CL.

I'll address this point there too.

[jungkees, 2015-07-27 15:00:37]

Uploaded a new snapshot. PTAL.

[yhirano, 2015-07-29 06:28:40]

lgtm

https://codereview.chromium.org/1256133002/diff/40001/Source/modules/servicew...
File Source/modules/serviceworkers/ExtendableEvent.h (right):

https://codereview.chromium.org/1256133002/diff/40001/Source/modules/servicew...
Source/modules/serviceworkers/ExtendableEvent.h:34: #include
"bindings/core/v8/ScriptValue.h"
-ScriptValue
+bindings/core/v8/ScriptPromoise.h

https://codereview.chromium.org/1256133002/diff/40001/Source/modules/servicew...
File Source/modules/serviceworkers/WaitUntilObserver.h (right):

https://codereview.chromium.org/1256133002/diff/40001/Source/modules/servicew...
Source/modules/serviceworkers/WaitUntilObserver.h:21: class ScriptValue;
-ScriptValue

[jungkees, 2015-07-29 07:55:46]

Addressed the comments. Thanks for review!

https://codereview.chromium.org/1256133002/diff/40001/Source/modules/servicew...
File Source/modules/serviceworkers/ExtendableEvent.h (right):

https://codereview.chromium.org/1256133002/diff/40001/Source/modules/servicew...
Source/modules/serviceworkers/ExtendableEvent.h:34: #include
"bindings/core/v8/ScriptValue.h"
Addressed.

https://codereview.chromium.org/1256133002/diff/40001/Source/modules/servicew...
File Source/modules/serviceworkers/WaitUntilObserver.h (right):

https://codereview.chromium.org/1256133002/diff/40001/Source/modules/servicew...
Source/modules/serviceworkers/WaitUntilObserver.h:21: class ScriptValue;
On 2015/07/29 06:28:40, yhirano wrote:
> -ScriptValue

It seems reportError() uses the type.

[jungkees, 2015-07-29 07:56:11]

The CQ bit was checked by jungkee.song@samsung.com

[jungkees, 2015-07-29 07:56:11]

The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/1256133002/#ps60001
(title: "Address comment: add/delete headers.")

[commit-bot: I haz the power, 2015-07-29 07:56:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1256133002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1256133002/60001

[yhirano, 2015-07-29 08:00:02]

https://codereview.chromium.org/1256133002/diff/40001/Source/modules/servicew...
File Source/modules/serviceworkers/WaitUntilObserver.h (right):

https://codereview.chromium.org/1256133002/diff/40001/Source/modules/servicew...
Source/modules/serviceworkers/WaitUntilObserver.h:21: class ScriptValue;
On 2015/07/29 07:55:46, jungkees wrote:
> On 2015/07/29 06:28:40, yhirano wrote:
> > -ScriptValue
> 
> It seems reportError() uses the type.

You're right, sorry.

[commit-bot: I haz the power, 2015-07-29 08:01:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-29 08:01:07]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/3...)

[jungkees, 2015-07-29 08:08:14]

I think I need an lgtm from a SW OWNER. PTAL

[falken, 2015-07-29 08:14:55]

https://codereview.chromium.org/1256133002/diff/60001/Source/modules/servicew...
File Source/modules/serviceworkers/ExtendableEvent.idl (right):

https://codereview.chromium.org/1256133002/diff/60001/Source/modules/servicew...
Source/modules/serviceworkers/ExtendableEvent.idl:37: [CallWith=ScriptState,
RaisesException] void waitUntil(Promise<any> f);
what does f stand for? I see it's in the spec as well. Should it be something
more understandable or is this a common convention?

[nhiroki, 2015-07-29 08:19:03]

lgtm

[jungkees, 2015-07-29 08:28:31]

The CQ bit was checked by jungkee.song@samsung.com

[commit-bot: I haz the power, 2015-07-29 08:28:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1256133002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1256133002/60001

[jungkees, 2015-07-29 08:34:19]

Missed falken's comment before committing it. Left my thought on the name of the
IDL argument.

https://codereview.chromium.org/1256133002/diff/60001/Source/modules/servicew...
File Source/modules/serviceworkers/ExtendableEvent.idl (right):

https://codereview.chromium.org/1256133002/diff/60001/Source/modules/servicew...
Source/modules/serviceworkers/ExtendableEvent.idl:37: [CallWith=ScriptState,
RaisesException] void waitUntil(Promise<any> f);
On 2015/07/29 08:14:55, falken wrote:
> what does f stand for? I see it's in the spec as well. Should it be something
> more understandable or is this a common convention?

Ah, I didn't see this comment before seeing an lgtm from nhiroki and committing
it.

I also had a same question. I don't think it is a convention, but couldn't come
up with a better generic name that represents just a *functional entity* that
can be either a promise or a value there.

I guess Jake or Alex initially named it so. I'm willing to change it if we have
a better one for it.

[falken, 2015-07-29 08:40:48]

On 2015/07/29 08:34:19, jungkees wrote:
> Missed falken's comment before committing it. Left my thought on the name of
the
> IDL argument.
> 
>
https://codereview.chromium.org/1256133002/diff/60001/Source/modules/servicew...
> File Source/modules/serviceworkers/ExtendableEvent.idl (right):
> 
>
https://codereview.chromium.org/1256133002/diff/60001/Source/modules/servicew...
> Source/modules/serviceworkers/ExtendableEvent.idl:37: [CallWith=ScriptState,
> RaisesException] void waitUntil(Promise<any> f);
> On 2015/07/29 08:14:55, falken wrote:
> > what does f stand for? I see it's in the spec as well. Should it be
something
> > more understandable or is this a common convention?
> 
> Ah, I didn't see this comment before seeing an lgtm from nhiroki and
committing
> it.
> 
> I also had a same question. I don't think it is a convention, but couldn't
come
> up with a better generic name that represents just a *functional entity* that
> can be either a promise or a value there.
> 
> I guess Jake or Alex initially named it so. I'm willing to change it if we
have
> a better one for it.

Not a big deal, it's OK to commit and track the spec.

[jungkees, 2015-07-29 08:42:36]

On 2015/07/29 08:40:48, falken wrote:
> Not a big deal, it's OK to commit and track the spec.

Thanks!

[commit-bot: I haz the power, 2015-07-29 09:00:02]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199643

[commit-bot: I haz the power, 2015-09-23 11:52:57]

Patchset 4 (id:??) landed as
https://crrev.com/2223c104b56eaed9abc1d1f69723a2c7c8f61bda