Service Worker: Correct Web IDL of FetchEvent.respondWith method.

Service Worker: Correct Web IDL of FetchEvent.respondWith method.

As per the spec, FetchEvent.respondWith() method's single argument should be of
type Promise<Response>. When a Response object is given as an argument, Web IDL
casts it to a promise via Promise.resolve. So this CL simply moves 
ScriptPromise::cast from Blink to V8 layer and does not change any behavior.

Spec: https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#respond-with-method
Spec discussion: https://github.com/slightlyoff/ServiceWorker/issues/724

BUG=513972
Committed: https://crrev.com/d59b9e54578d473164b5f04dc4b4d147f9924e35
git-svn-id: svn://svn.chromium.org/blink/trunk@199499 bbb929c8-8fbe-4397-9dbb-9b2b20218538

[jungkees, 2015-07-25 12:41:45]

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

[jungkees, 2015-07-25 12:42:50]

PTAL

[zino, 2015-07-25 21:28:34]

On 2015/07/25 12:42:50, jungkees wrote:
> PTAL

l-g-t-m with nit.

[zino, 2015-07-25 21:29:21]

l-g-t-m with nit.

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

https://codereview.chromium.org/1255933004/diff/40001/Source/modules/servicew...
Source/modules/serviceworkers/RespondWithObserver.h:8: #include
"bindings/core/v8/ScriptPromise.h"
nit: I think it is better to use a class forward declaration instead of adding
header.

[jungkees, 2015-07-26 08:35:23]

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

[jungkees, 2015-07-26 08:37:40]

Thanks for the peer-review zino@.

OWNERS, PTAL.

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

https://codereview.chromium.org/1255933004/diff/40001/Source/modules/servicew...
Source/modules/serviceworkers/RespondWithObserver.h:8: #include
"bindings/core/v8/ScriptPromise.h"
That's right. I addressed it. Thanks for review.

[falken, 2015-07-27 00:44:41]

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

[falken, 2015-07-27 00:44:41]

Does this patch change any behavior?

Anyway, it probably makes sense to make the IDL more closely match the spec
(which is Response or Promise<Response>) now.

I'd like yhirano to take a look, there maybe  a reason we did "any" instead of
"Promise<Response>"

https://codereview.chromium.org/1255933004/diff/70001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/serviceworker/resources/fetch-event-respond-with-argument-iframe.html
(right):

https://codereview.chromium.org/1255933004/diff/70001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/fetch-event-respond-with-argument-iframe.html:39:
{ name: 'other-value', expect_load: false },
'object' or 'object-instance' seems more clear than 'other-value'

Would it make sense to add a test for 'null' just for more completeness?

[jungkees, 2015-07-27 01:20:33]

On 2015/07/27 00:44:41, falken wrote:
> Does this patch change any behavior?
> 

No, it doesn't change any behavior. ScriptPromise::cast simply moves from Blink
to V8 layer.

> Anyway, it probably makes sense to make the IDL more closely match the spec
> (which is Response or Promise<Response>) now.
> 

This is the very reason I started this CL. The IDL that you quoted has just been
changed to (Promise<Response>) based on this discussion:
https://github.com/slightlyoff/ServiceWorker/issues/724. (According to the
discussion, the previous union types were something wrong in terms of the Web
IDL definition.)

> I'd like yhirano to take a look, there maybe  a reason we did "any" instead of
> "Promise<Response>"
> 
>
https://codereview.chromium.org/1255933004/diff/70001/LayoutTests/http/tests/...
> File
>
LayoutTests/http/tests/serviceworker/resources/fetch-event-respond-with-argument-iframe.html
> (right):
> 
>
https://codereview.chromium.org/1255933004/diff/70001/LayoutTests/http/tests/...
>
LayoutTests/http/tests/serviceworker/resources/fetch-event-respond-with-argument-iframe.html:39:
> { name: 'other-value', expect_load: false },
> 'object' or 'object-instance' seems more clear than 'other-value'
> 
> Would it make sense to add a test for 'null' just for more completeness?

[jungkees, 2015-07-27 01:21:32]

Added a comment about adding null in the test.

https://codereview.chromium.org/1255933004/diff/70001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/serviceworker/resources/fetch-event-respond-with-argument-iframe.html
(right):

https://codereview.chromium.org/1255933004/diff/70001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/fetch-event-respond-with-argument-iframe.html:39:
{ name: 'other-value', expect_load: false },
IMO, any single case which is neither a Response nor a Promise<Response> is
enough here as it merely turns to something like Promise.resolve(value). Then
the promise gets through the existing implementation where other than a valid
Response object will end up with a network error.

[yhirano, 2015-07-27 04:02:38]

On 2015/07/27 00:44:41, falken wrote:
> I'd like yhirano to take a look, there maybe  a reason we did "any" instead of
> "Promise<Response>"
> 

any -> Promise automatic translation was implemented in May 2014[1], and this
|any| was written in Feb 2014[2]. I think that is the reason.
1: https://src.chromium.org/viewvc/blink?revision=174792&view=revision
2: https://src.chromium.org/viewvc/blink?revision=168073&view=revision

[yhirano, 2015-07-27 05:50:38]

lgtm

[jungkees, 2015-07-27 06:22:09]

On 2015/07/27 05:50:38, yhirano wrote:
> lgtm

Thanks for review yhirano@. falken@, can I just go head commit it without SW
OWNER's lgtm?

[falken, 2015-07-27 07:45:25]

yhirano lgtm is good for me

[jungkees, 2015-07-27 07:49:23]

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

[commit-bot: I haz the power, 2015-07-27 07:49:26]

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

[commit-bot: I haz the power, 2015-07-27 08:32:25]

Committed patchset #5 (id:70001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199499

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

Patchset 5 (id:??) landed as
https://crrev.com/d59b9e54578d473164b5f04dc4b4d147f9924e35