Removing ServiceWorker's InstallEvent Interface.

Removing ServiceWorker's InstallEvent Interface.

As InstallEvent removed from ServiceWoker in this issue. https://github.com/slightlyoff/ServiceWorker/issues/661

and changed in https://github.com/slightlyoff/ServiceWorker/commit/4d172287dc5f92530ac251f68a670f5f4de9540c

Intent to Remove Link: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/546dbZxremk


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

[Paritosh Kumar, 2015-04-06 08:10:17]

The CQ bit was checked by paritosh.in@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2015-04-06 08:10:27]

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

[commit-bot: I haz the power, 2015-04-06 08:10:29]

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

[commit-bot: I haz the power, 2015-04-06 08:10:30]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Paritosh Kumar, 2015-04-06 08:18:34]

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

[Paritosh Kumar, 2015-04-06 08:18:34]

Please have a look.

[falken, 2015-04-06 08:25:32]

This is a web-exposed change so it probably should go through Blink's intent to
ship process. Would you be willing to write an "intent to ship" mail?

It'd also be nice to have a LayoutTest to check that the event passed to
oninstall is an ExtendableEvent.

Also, the CL description should be updated to mention why the change is being
made (basically, link to the spec bug/change).

[falken, 2015-04-08 02:06:46]

On 2015/04/06 08:25:32, falken wrote:
> This is a web-exposed change so it probably should go through Blink's intent
to
> ship process. Would you be willing to write an "intent to ship" mail?
> 
> It'd also be nice to have a LayoutTest to check that the event passed to
> oninstall is an ExtendableEvent.
> 
> Also, the CL description should be updated to mention why the change is being
> made (basically, link to the spec bug/change).

Thanks for writing the "intent to ship". Now we are OK proceed.

Can you write a test for this part of the spec:
"Create a trusted event e that uses the ExtendableEvent interface, with the
event type install, which does not bubble, is not cancelable, and has no default
action."

[Paritosh Kumar, 2015-04-08 05:08:18]

Patchset #2 (id:20001) has been deleted

[Paritosh Kumar, 2015-04-08 05:29:30]

Thanks Matt

Added a test in this CL.

> > It'd also be nice to have a LayoutTest to check that the event passed to
> > oninstall is an ExtendableEvent.

> Can you write a test for this part of the spec:
> "Create a trusted event e that uses the ExtendableEvent interface, with the
> event type install, which does not bubble, is not cancelable, and has no
default
> action."

This tests the event type is install, which does not bubble and not cancelable.
But, It doesn't check the change made in this CL, (i.e. event uses
ExtendableEvent).

I'm not sure but tried with interfaceName() that wont work, I think this is not
an Web exposed API.

[falken, 2015-04-08 05:44:46]

Thanks for the test, looks pretty good.

https://codereview.chromium.org/1067513002/diff/40001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/install-event-type.html (right):

https://codereview.chromium.org/1067513002/diff/40001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/install-event-type.html:19: return
service_worker_unregister_and_register(t, 'resources/oninstall-event-type.js',
'resources/oninstall-event-type.js')
By convention, service worker script filenames end with "-worker.js"
It's also nice to give the scope a name similar to the test name and pass that:

var scope = 'install-event-type'

Also, please wrap lines at 80 cols:
https://www.chromium.org/blink/serviceworker/testing

https://codereview.chromium.org/1067513002/diff/40001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/install-event-type.html:24:
assert_equals(did_install, true,'The worker was installed');
You can use assert_true rather than assert_equals

Also it's missing a space after the comma

https://codereview.chromium.org/1067513002/diff/40001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/install-event-type.html:29: 
nit: extra empty line

https://codereview.chromium.org/1067513002/diff/40001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/resources/install-event-type.js
(right):

https://codereview.chromium.org/1067513002/diff/40001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/install-event-type.js:2:
importScripts('/resources/testharness-helpers.js');
I think you don't need this file, so please remove it.

https://codereview.chromium.org/1067513002/diff/40001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/install-event-type.js:4:
self.oninstall = function(event) {
I think you can do assert_true(event instanceof 'ExtendableEvent')

[falken, 2015-04-08 05:44:46]

Thanks for the test, looks pretty good.

[Paritosh Kumar, 2015-04-08 06:44:06]

Thanks Matt.

Updated. Please have a look.

https://codereview.chromium.org/1067513002/diff/40001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/install-event-type.html (right):

https://codereview.chromium.org/1067513002/diff/40001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/install-event-type.html:19: return
service_worker_unregister_and_register(t, 'resources/oninstall-event-type.js',
'resources/oninstall-event-type.js')
Thanks. Done.

OO, I was using 'resources/oninstall-event-type.js' instead of
'resources/install-event-type.js'.

given scope name similar to test name.

https://codereview.chromium.org/1067513002/diff/40001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/install-event-type.html:24:
assert_equals(did_install, true,'The worker was installed');
On 2015/04/08 05:44:46, falken wrote:
> You can use assert_true rather than assert_equals
> 
> Also it's missing a space after the comma

Yeah.. Thanks.

https://codereview.chromium.org/1067513002/diff/40001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/install-event-type.html:29: 
On 2015/04/08 05:44:46, falken wrote:
> nit: extra empty line

Done.

https://codereview.chromium.org/1067513002/diff/40001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/resources/install-event-type.js
(right):

https://codereview.chromium.org/1067513002/diff/40001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/install-event-type.js:2:
importScripts('/resources/testharness-helpers.js');
On 2015/04/08 05:44:46, falken wrote:
> I think you don't need this file, so please remove it.

Agree. Thanks.

https://codereview.chromium.org/1067513002/diff/40001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/install-event-type.js:4:
self.oninstall = function(event) {
On 2015/04/08 05:44:46, falken wrote:
> I think you can do assert_true(event instanceof 'ExtendableEvent')

Ohhk, Thanks. I forgot to use instanceof.

[falken, 2015-04-08 08:44:44]

lgtm!

[Paritosh Kumar, 2015-04-08 08:49:00]

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

[Paritosh Kumar, 2015-04-08 08:49:01]

Adding Mike for review.

@Mike: Please have a look.

[Mike West, 2015-04-08 08:53:39]

LGTM. I like deleting code. Thanks for including the link to the intent to
remove.

[Paritosh Kumar, 2015-04-08 09:12:41]

The CQ bit was checked by paritosh.in@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2015-04-08 09:13:03]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1067513002/60001

[Paritosh Kumar, 2015-04-08 09:34:12]

The CQ bit was unchecked by paritosh.in@samsung.com

[Paritosh Kumar, 2015-04-08 10:46:08]

The CQ bit was checked by paritosh.in@samsung.com

[commit-bot: I haz the power, 2015-04-08 10:46:20]

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

[commit-bot: I haz the power, 2015-04-08 11:01:06]

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