Resolve FetchEvent.preloadResponse with undefined when Navigation Preload is disabled.

Resolve FetchEvent.preloadResponse with undefined when Navigation Preload is disabled.

Currently FetchEvent.preloadResponse is resolved with null when navigation preload is not enabled.
But according to the spec, it should be resolved with undefined.

https://github.com/w3c/ServiceWorker/commit/fbc0faa#diff-27b79860afe28f01aed4f1f6228367faR2901

BUG=649558
Review-Url: https://codereview.chromium.org/2478783002
Cr-Commit-Position: refs/heads/master@{#441638}
Committed: https://chromium.googlesource.com/chromium/src/+/e7c05695fbc1407a034269b83871159ca662cceb

[horo, 2016-11-04 05:05:14]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-04 05:05:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[horo, 2016-11-04 05:06:33]

Description was changed from

==========
resolve FetchEvent.preloadResponse with undefined when Navigation Preload is
disabled.

BUG=
==========

to

==========
Resolve FetchEvent.preloadResponse with undefined when Navigation Preload is
disabled.

Currently FetchEvent.preloadResponse is resolved with null.
But according to the spec discussion, it should be resolved with undefined.
https://github.com/w3c/ServiceWorker/pull/983#pullrequestreview-6410334

BUG=649558
==========

[horo, 2016-11-04 05:06:52]

Description was changed from

==========
Resolve FetchEvent.preloadResponse with undefined when Navigation Preload is
disabled.

Currently FetchEvent.preloadResponse is resolved with null.
But according to the spec discussion, it should be resolved with undefined.
https://github.com/w3c/ServiceWorker/pull/983#pullrequestreview-6410334

BUG=649558
==========

to

==========
Resolve FetchEvent.preloadResponse with undefined when Navigation Preload is
disabled.

Currently FetchEvent.preloadResponse is resolved with null.
But according to the spec discussion, it should be resolved with undefined.
https://github.com/w3c/ServiceWorker/pull/983#pullrequestreview-6410334

This CL depends on https://codereview.chromium.org/2475663003/.

BUG=649558
==========

[horo, 2016-11-04 05:07:25]

Description was changed from

==========
Resolve FetchEvent.preloadResponse with undefined when Navigation Preload is
disabled.

Currently FetchEvent.preloadResponse is resolved with null.
But according to the spec discussion, it should be resolved with undefined.
https://github.com/w3c/ServiceWorker/pull/983#pullrequestreview-6410334

This CL depends on https://codereview.chromium.org/2475663003/.

BUG=649558
==========

to

==========
Resolve FetchEvent.preloadResponse with undefined when Navigation Preload is
disabled.

Currently FetchEvent.preloadResponse is resolved with null when navigation is
not enabled.
But according to the spec discussion, it should be resolved with undefined.
https://github.com/w3c/ServiceWorker/pull/983#pullrequestreview-6410334

This CL depends on https://codereview.chromium.org/2475663003/.

BUG=649558
==========

[horo, 2016-11-04 05:07:40]

Description was changed from

==========
Resolve FetchEvent.preloadResponse with undefined when Navigation Preload is
disabled.

Currently FetchEvent.preloadResponse is resolved with null when navigation is
not enabled.
But according to the spec discussion, it should be resolved with undefined.
https://github.com/w3c/ServiceWorker/pull/983#pullrequestreview-6410334

This CL depends on https://codereview.chromium.org/2475663003/.

BUG=649558
==========

to

==========
Resolve FetchEvent.preloadResponse with undefined when Navigation Preload is
disabled.

Currently FetchEvent.preloadResponse is resolved with null when navigation
preload is not enabled.
But according to the spec discussion, it should be resolved with undefined.
https://github.com/w3c/ServiceWorker/pull/983#pullrequestreview-6410334

This CL depends on https://codereview.chromium.org/2475663003/.

BUG=649558
==========

[horo, 2016-11-04 05:08:06]

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

[horo, 2016-11-04 05:08:07]

falken@
Please review this CL.

Thank you.

[falken, 2016-11-04 05:39:57]

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

[falken, 2016-11-04 05:39:57]

This lgtm but can you have someone more familiar with v8/bindings look at it to
make sure this is the proper way to implement a Promise that can resolve to an
object or undefined? +haraken

[haraken, 2016-11-04 05:45:48]

On 2016/11/04 05:39:57, falken wrote:
> This lgtm but can you have someone more familiar with v8/bindings look at it
to
> make sure this is the proper way to implement a Promise that can resolve to an
> object or undefined? +haraken

Have we decided to support an undefined-able Promise in the spec?

If that is the case, I'd prefer implementing the logic in
ScriptPromiseResolver::resolve rather than implementing it in
FetchEvent::ResponseOrUndefined.

[commit-bot: I haz the power, 2016-11-04 06:30:04]

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

[commit-bot: I haz the power, 2016-11-04 06:30:05]

Dry run: This issue passed the CQ dry run.

[horo, 2016-11-04 08:57:28]

On 2016/11/04 05:45:48, haraken wrote:
> On 2016/11/04 05:39:57, falken wrote:
> > This lgtm but can you have someone more familiar with v8/bindings look at it
> to
> > make sure this is the proper way to implement a Promise that can resolve to
an
> > object or undefined? +haraken
> 
> Have we decided to support an undefined-able Promise in the spec?
> 
> If that is the case, I'd prefer implementing the logic in
> ScriptPromiseResolver::resolve rather than implementing it in
> FetchEvent::ResponseOrUndefined.

This is still under discussion.

OK.
I will create a CL to implement the logic in ScriptPromiseResolver when the
discussion is settled.
https://github.com/w3c/ServiceWorker/pull/983#pullrequestreview-6410334

[horo, 2017-01-05 06:32:55]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-05 06:33:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-05 06:44:40]

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

[commit-bot: I haz the power, 2017-01-05 06:44:41]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[horo, 2017-01-05 07:07:15]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-05 07:07:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[horo, 2017-01-05 07:09:22]

Patchset #4 (id:60001) has been deleted

[commit-bot: I haz the power, 2017-01-05 07:09:25]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[commit-bot: I haz the power, 2017-01-05 07:11:24]

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

[commit-bot: I haz the power, 2017-01-05 07:11:27]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[horo, 2017-01-05 07:13:46]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-05 07:13:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[horo, 2017-01-05 07:20:33]

Description was changed from

==========
Resolve FetchEvent.preloadResponse with undefined when Navigation Preload is
disabled.

Currently FetchEvent.preloadResponse is resolved with null when navigation
preload is not enabled.
But according to the spec discussion, it should be resolved with undefined.
https://github.com/w3c/ServiceWorker/pull/983#pullrequestreview-6410334

This CL depends on https://codereview.chromium.org/2475663003/.

BUG=649558
==========

to

==========
Resolve FetchEvent.preloadResponse with undefined when Navigation Preload is
disabled.

Currently FetchEvent.preloadResponse is resolved with null when navigation
preload is not enabled.
But according to the spec, it should be resolved with undefined.

https://github.com/w3c/ServiceWorker/commit/fbc0faa#diff-27b79860afe28f01aed4...

BUG=649558
==========

[horo, 2017-01-05 07:23:43]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[horo, 2017-01-05 07:23:56]

On 2016/11/04 05:45:48, haraken wrote:
> On 2016/11/04 05:39:57, falken wrote:
> > This lgtm but can you have someone more familiar with v8/bindings look at it
> to
> > make sure this is the proper way to implement a Promise that can resolve to
an
> > object or undefined? +haraken
> 
> Have we decided to support an undefined-able Promise in the spec?
> 
> If that is the case, I'd prefer implementing the logic in
> ScriptPromiseResolver::resolve rather than implementing it in
> FetchEvent::ResponseOrUndefined.

Now, the behavior is written in the Service Worker spec.
https://github.com/w3c/ServiceWorker/commit/fbc0faa#diff-27b79860afe28f01aed4...
> Else, resolve |preloadResponse| with undefined.

I uploaded Patch Set 5 which introduces
ScriptPromiseProperty::resolveWithUndefined().

haraken@
Could you please review this?

[commit-bot: I haz the power, 2017-01-05 07:23:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[haraken, 2017-01-05 07:56:25]

LGTM

https://codereview.chromium.org/2478783002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptPromiseProperty.h (right):

https://codereview.chromium.org/2478783002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptPromiseProperty.h:160: return
v8::Undefined(isolate);

Can we add DCHECK(m_resolved == ResolvedType()) ?

[horo, 2017-01-05 08:13:08]

Thank you.

https://codereview.chromium.org/2478783002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptPromiseProperty.h (right):

https://codereview.chromium.org/2478783002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptPromiseProperty.h:160: return
v8::Undefined(isolate);
On 2017/01/05 07:56:24, haraken wrote:
> 
> Can we add DCHECK(m_resolved == ResolvedType()) ?

No we can't.


In file included from
../../third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.cpp:26:
In file included from
../../third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.h:30:
../../third_party/WebKit/Source/bindings/core/v8/ScriptPromiseProperty.h:160:21:
error: invalid operands to binary expression ('blink::ToV8UndefinedGenerator'
and 'blink::ToV8UndefinedGenerator')
  DCHECK(m_resolved == ResolvedType());
         ~~~~~~~~~~ ^  ~~~~~~~~~~~~~~

[haraken, 2017-01-05 08:23:05]

On 2017/01/05 08:13:08, horo wrote:
> Thank you.
> 
>
https://codereview.chromium.org/2478783002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/bindings/core/v8/ScriptPromiseProperty.h
(right):
> 
>
https://codereview.chromium.org/2478783002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/bindings/core/v8/ScriptPromiseProperty.h:160: return
> v8::Undefined(isolate);
> On 2017/01/05 07:56:24, haraken wrote:
> > 
> > Can we add DCHECK(m_resolved == ResolvedType()) ?
> 
> No we can't.
> 
> 
> In file included from
> ../../third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.cpp:26:
> In file included from
> ../../third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.h:30:
>
../../third_party/WebKit/Source/bindings/core/v8/ScriptPromiseProperty.h:160:21:
> error: invalid operands to binary expression ('blink::ToV8UndefinedGenerator'
> and 'blink::ToV8UndefinedGenerator')
>   DCHECK(m_resolved == ResolvedType());
>          ~~~~~~~~~~ ^  ~~~~~~~~~~~~~~

Makes sense.

[horo, 2017-01-05 08:25:19]

The CQ bit was unchecked by horo@chromium.org

[horo, 2017-01-05 08:25:24]

The CQ bit was checked by horo@chromium.org

[horo, 2017-01-05 08:25:25]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org
Link to the patchset: https://codereview.chromium.org/2478783002/#ps100001
(title: "ScriptPromiseProperty::resolveWithUndefined()")

[commit-bot: I haz the power, 2017-01-05 08:25:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-05 10:06:01]

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

[commit-bot: I haz the power, 2017-01-05 10:06:05]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[horo, 2017-01-05 10:21:12]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2017-01-05 10:21:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-05 12:50:10]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1483611672409830,
"parent_rev": "201ed21575315baf7363b7e0e9cd4e0d2bb66d92", "commit_rev":
"e7c05695fbc1407a034269b83871159ca662cceb"}

[commit-bot: I haz the power, 2017-01-05 12:50:40]

Description was changed from

==========
Resolve FetchEvent.preloadResponse with undefined when Navigation Preload is
disabled.

Currently FetchEvent.preloadResponse is resolved with null when navigation
preload is not enabled.
But according to the spec, it should be resolved with undefined.

https://github.com/w3c/ServiceWorker/commit/fbc0faa#diff-27b79860afe28f01aed4...

BUG=649558
==========

to

==========
Resolve FetchEvent.preloadResponse with undefined when Navigation Preload is
disabled.

Currently FetchEvent.preloadResponse is resolved with null when navigation
preload is not enabled.
But according to the spec, it should be resolved with undefined.

https://github.com/w3c/ServiceWorker/commit/fbc0faa#diff-27b79860afe28f01aed4...

BUG=649558

Review-Url: https://codereview.chromium.org/2478783002
Cr-Commit-Position: refs/heads/master@{#441638}
Committed:
https://chromium.googlesource.com/chromium/src/+/e7c05695fbc1407a034269b83871...
==========

[commit-bot: I haz the power, 2017-01-05 12:50:42]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/e7c05695fbc1407a034269b83871...