Introducing FetchEventInit Dictionary.

Introducing FetchEventInit Dictionary.

Spec: https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#fetch-event-section

BUG=411193

[Paritosh Kumar, 2015-04-17 13:54:08]

Patchset #1 (id:1) has been deleted

[Paritosh Kumar, 2015-04-17 13:55:28]

paritosh.in@samsung.com changed reviewers:
+ falken@chromium.org

[Paritosh Kumar, 2015-04-17 13:55:29]

@falken: Adding FetchEventInit, please have a look. Is it right way or not?

[falken, 2015-04-20 01:14:19]

This looks pretty good, some things to do before it landing:

- Add a test
- Link to the spec in the CL descrption 
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#fe...
- Use BUG=411193

[Paritosh Kumar, 2015-04-20 06:55:08]

On 2015/04/20 01:14:19, falken wrote:
> This looks pretty good, some things to do before it landing:
> 
> - Add a test
> - Link to the spec in the CL descrption 
>
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#fe...
> - Use BUG=411193

Thanks falken

Added test for Fetch event in interfaces-worker.js.
Please have a look.

[Paritosh Kumar, 2015-04-20 06:58:39]

PTAL.

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

https://codereview.chromium.org/1079073003/diff/40001/Source/modules/servicew...
Source/modules/serviceworkers/FetchEvent.h:37: void setIsReload(bool);
As, now we are setting isReload through FetchEventInit, I think this is useless.
or Do you have any use case from your view?

[falken, 2015-04-20 09:15:18]

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

[falken, 2015-04-20 09:15:19]

lgtm

I think this qualifies as a trivial change so Intent to Ship is not needed. But
an API OWNER should review the IDL change. +tkent

https://codereview.chromium.org/1079073003/diff/40001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/resources/interfaces-worker.js
(right):

https://codereview.chromium.org/1079073003/diff/40001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/interfaces-worker.js:82:
assert_equals(new FetchEvent('FetchEvent', {request : req, isReload :
true}).request.url, 'http://localhost/');
We should wrap at 80 cols: https://www.chromium.org/blink/serviceworker/testing

It's also a good idea to add a small description to the assert_equals, so if the
test fails the error log won't just be something like "Assert failed: expected
false but got true". E.g., 

assert_equals(new FetchEvent('FetchEvent').cancelable, false,
              'default FetchEvent.cancelable should be false');

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

https://codereview.chromium.org/1079073003/diff/40001/Source/modules/servicew...
Source/modules/serviceworkers/FetchEvent.h:37: void setIsReload(bool);
On 2015/04/20 06:58:39, Paritosh Kumar wrote:
> As, now we are setting isReload through FetchEventInit, I think this is
useless.
> or Do you have any use case from your view?

Yep let's remove it.

[Paritosh Kumar, 2015-04-20 09:57:20]

Thanks falken

Addressed your comments.

https://codereview.chromium.org/1079073003/diff/40001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/resources/interfaces-worker.js
(right):

https://codereview.chromium.org/1079073003/diff/40001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/interfaces-worker.js:82:
assert_equals(new FetchEvent('FetchEvent', {request : req, isReload :
true}).request.url, 'http://localhost/');
Thanks.

> We should wrap at 80 cols:

Done.

> It's also a good idea to add a small description to the assert_equals, so if
the

Thanks. Done.

> assert_equals(new FetchEvent('FetchEvent').cancelable, false,
>               'default FetchEvent.cancelable should be false');

Ohh! It should be true. Thanks. Done.

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

https://codereview.chromium.org/1079073003/diff/40001/Source/modules/servicew...
Source/modules/serviceworkers/FetchEvent.h:37: void setIsReload(bool);
Thanks. Removed.

[tkent, 2015-04-20 23:31:41]

it's ok to skip intent-to-ship.  This is a trivial addition, and no
compatibility risk.

https://codereview.chromium.org/1079073003/diff/80001/Source/modules/servicew...
File Source/modules/serviceworkers/FetchEvent.cpp (right):

https://codereview.chromium.org/1079073003/diff/80001/Source/modules/servicew...
Source/modules/serviceworkers/FetchEvent.cpp:61: m_isReload =
initializer.isReload();
m_isReload is not initialized for |else| path.

https://codereview.chromium.org/1079073003/diff/80001/Source/modules/servicew...
Source/modules/serviceworkers/FetchEvent.cpp:71: m_isReload =
initializer.isReload();
Ditto.

https://codereview.chromium.org/1079073003/diff/80001/Source/modules/servicew...
File Source/modules/serviceworkers/FetchEvent.h (right):

https://codereview.chromium.org/1079073003/diff/80001/Source/modules/servicew...
Source/modules/serviceworkers/FetchEvent.h:41: FetchEvent(const AtomicString&
type, const FetchEventInit&);
Remove this constructor.  We can use |new FetchEvent(type, init, nullptr)|.

https://codereview.chromium.org/1079073003/diff/80001/Source/modules/servicew...
File Source/modules/serviceworkers/FetchEventInit.idl (right):

https://codereview.chromium.org/1079073003/diff/80001/Source/modules/servicew...
Source/modules/serviceworkers/FetchEventInit.idl:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
Copyright 2015

[Paritosh Kumar, 2015-04-21 05:35:57]

Patchset #5 (id:100001) has been deleted

[Paritosh Kumar, 2015-04-21 05:37:14]

Thanks tkent

Addressed your comments. Please have a look.

https://codereview.chromium.org/1079073003/diff/80001/Source/modules/servicew...
File Source/modules/serviceworkers/FetchEvent.cpp (right):

https://codereview.chromium.org/1079073003/diff/80001/Source/modules/servicew...
Source/modules/serviceworkers/FetchEvent.cpp:61: m_isReload =
initializer.isReload();
I think this "if" is not required as initializer.hasIsReload will return always
true. Since, We are initializing isReload in FetchEventInit with false, this
will never be null.

https://codereview.chromium.org/1079073003/diff/80001/Source/modules/servicew...
File Source/modules/serviceworkers/FetchEvent.h (right):

https://codereview.chromium.org/1079073003/diff/80001/Source/modules/servicew...
Source/modules/serviceworkers/FetchEvent.h:41: FetchEvent(const AtomicString&
type, const FetchEventInit&);
Ah not required. Removed. Thanks.

https://codereview.chromium.org/1079073003/diff/80001/Source/modules/servicew...
File Source/modules/serviceworkers/FetchEventInit.idl (right):

https://codereview.chromium.org/1079073003/diff/80001/Source/modules/servicew...
Source/modules/serviceworkers/FetchEventInit.idl:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2015/04/20 23:31:41, tkent wrote:
> Copyright 2015

Thanks. Done.

[tkent, 2015-04-21 05:39:54]

lgtm

[Paritosh Kumar, 2015-04-21 05:47:29]

On 2015/04/21 05:39:54, tkent wrote:
> lgtm

Thanks tkent, But the issue
https://github.com/slightlyoff/ServiceWorker/issues/683
closed with change in specs of FetchEventInit.

Now, we should extend ExtendableEventInit in FetchEventInit and ExtendableEvent
in FetchEvent.

Will we do this change in this cl, or in different cl?

[tkent, 2015-04-21 07:20:20]

> Now, we should extend ExtendableEventInit in FetchEventInit and
ExtendableEvent
> in FetchEvent.
> 
> Will we do this change in this cl, or in different cl?

I prefer closing this CL and creating another CL because this CL has stale
LGTMs.

[Paritosh Kumar, 2015-04-21 09:49:05]

On 2015/04/21 07:20:20, tkent wrote:
> > Now, we should extend ExtendableEventInit in FetchEventInit and
> ExtendableEvent
> > in FetchEvent.
> > 
> > Will we do this change in this cl, or in different cl?
> 
> I prefer closing this CL and creating another CL because this CL has stale
> LGTMs.

Closing this cl, as updated in https://codereview.chromium.org/1099073002/.