Fix oninstall-script tests for service worker

Fix oninstall-script tests for service worker

According to the spec and the WPT test, an exception occurring during oninstall
should not cause installation to fail. This change makes the test pass.

Note that this checks removal may affect the behavior of all ExtendableEvents; 
however it is expected to be fine as explained in the bug.

BUG=685531
Review-Url: https://codereview.chromium.org/2734783002
Cr-Commit-Position: refs/heads/master@{#458339}
Committed: https://chromium.googlesource.com/chromium/src/+/a626eb843a373043835437074f4c7de51baf00f4

[yiyix, 2017-03-06 06:31:56]

Description was changed from

==========
trial


fix

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

to

==========
trial


fix

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

[yiyix, 2017-03-06 06:32:05]

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

[commit-bot: I haz the power, 2017-03-06 06:32:20]

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-03-06 08:21:01]

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

[commit-bot: I haz the power, 2017-03-06 08:21:01]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[yiyix, 2017-03-06 18:33:40]

Patchset #1 (id:1) has been deleted

[yiyix, 2017-03-06 18:33:46]

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

[yiyix, 2017-03-06 18:34:23]

Description was changed from

==========
trial


fix

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

to

==========
Fix onintall-script tests for service worker

BUG=685531
==========

[commit-bot: I haz the power, 2017-03-06 18:34:34]

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-03-06 20:05:47]

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

[commit-bot: I haz the power, 2017-03-06 20:05:48]

Dry run: 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_...)

[yiyix, 2017-03-07 01:11:52]

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

[commit-bot: I haz the power, 2017-03-07 01:12:32]

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

[yiyix, 2017-03-07 01:16:59]

Patchset #1 (id:20001) has been deleted

[yiyix, 2017-03-07 01:24:14]

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

[commit-bot: I haz the power, 2017-03-07 01:24:49]

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-03-07 02:50:39]

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

[commit-bot: I haz the power, 2017-03-07 02:50:40]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[yiyix, 2017-03-07 05:16:54]

Patchset #1 (id:40001) has been deleted

[yiyix, 2017-03-08 15:28:38]

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

[commit-bot: I haz the power, 2017-03-08 15:29:00]

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-03-08 16:15:32]

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

[commit-bot: I haz the power, 2017-03-08 16:15:33]

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_...)

[yiyix, 2017-03-15 19:59:05]

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

[commit-bot: I haz the power, 2017-03-15 19:59:34]

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-03-15 21:05:48]

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

[commit-bot: I haz the power, 2017-03-15 21:05:49]

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

[yiyix, 2017-03-15 22:20:51]

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

[commit-bot: I haz the power, 2017-03-15 22:21:41]

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-03-15 23:26:30]

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

[commit-bot: I haz the power, 2017-03-15 23:26:30]

Dry run: 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_...)

[yiyix, 2017-03-21 02:02:33]

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

[yiyix, 2017-03-21 02:02:44]

Patchset #1 (id:60001) has been deleted

[yiyix, 2017-03-21 02:02:53]

Patchset #1 (id:80001) has been deleted

[yiyix, 2017-03-21 02:03:00]

Patchset #1 (id:100001) has been deleted

[commit-bot: I haz the power, 2017-03-21 02:03:19]

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-03-21 03:32:52]

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

[commit-bot: I haz the power, 2017-03-21 03:32:53]

Dry run: This issue passed the CQ dry run.

[yiyix, 2017-03-21 04:09:21]

Description was changed from

==========
Fix onintall-script tests for service worker

BUG=685531
==========

to

==========
Fix onintall-script tests for service worker

Remove unnecessary checks for install events.

BUG=685531
==========

[yiyix, 2017-03-21 04:09:22]

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

[yiyix, 2017-03-21 04:09:59]

@Falken, could you please take a look? Thank you very much.

[falken, 2017-03-21 04:27:38]

This looks good.

nit in CL description: onintall -> oninstall

Also for the CL description, we could be more clear why this change is
happening. Perhaps:

"According to the spec and the WPT test, an exception occurring during oninstall
should not cause installation to fail. This change makes the test pass."

It's also be worth mentioning that this affects the behavior of all
ExtendableEvent, but that this change is expected to be OK (and to see the
comment on the bug).

https://codereview.chromium.org/2734783002/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp
(right):

https://codereview.chromium.org/2734783002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp:203:
return WorkerGlobalScope::dispatchEventInternal(event);
Since it's now just calling the superclass's function, we can just remove this
function.

https://codereview.chromium.org/2734783002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp:214:
observer->didDispatchEvent(thread()->isForciblyTerminated());
Note for future reference: we could probably revisit whether we need this check
as well, but it doesn't need to be in this patch.

[yiyix, 2017-03-21 05:04:39]

Description was changed from

==========
Fix onintall-script tests for service worker

Remove unnecessary checks for install events.

BUG=685531
==========

to

==========
Fix onintall-script tests for service worker

According to the spec and the WPT test, an exception occurring during oninstall
should not cause installation to fail. This change makes the test pass.

Note that this checks removal may affect the behavior of all ExtendableEvents; 
however it is expected to be fine as explained in the bug.

BUG=685531
==========

[yiyix, 2017-03-21 05:44:27]

@falken, Could you please check the new patch?
Thank you.

https://codereview.chromium.org/2734783002/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp
(right):

https://codereview.chromium.org/2734783002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp:203:
return WorkerGlobalScope::dispatchEventInternal(event);
On 2017/03/21 04:27:37, falken wrote:
> Since it's now just calling the superclass's function, we can just remove this
> function.

Good call. Removed.

https://codereview.chromium.org/2734783002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp:214:
observer->didDispatchEvent(thread()->isForciblyTerminated());
On 2017/03/21 04:27:37, falken wrote:
> Note for future reference: we could probably revisit whether we need this
check
> as well, but it doesn't need to be in this patch.

Would it be preferable to add a TODO?

[falken, 2017-03-21 05:52:16]

lgtm 

a remaining typo in the CL description: onintall -> oninstall

https://codereview.chromium.org/2734783002/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp
(right):

https://codereview.chromium.org/2734783002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp:214:
observer->didDispatchEvent(thread()->isForciblyTerminated());
On 2017/03/21 05:44:27, yiyix wrote:
> On 2017/03/21 04:27:37, falken wrote:
> > Note for future reference: we could probably revisit whether we need this
> check
> > as well, but it doesn't need to be in this patch.
> 
> Would it be preferable to add a TODO?

Hm, I think it's OK without it. The code will probably be like this for a while.

[yiyix, 2017-03-21 05:54:37]

Description was changed from

==========
Fix onintall-script tests for service worker

According to the spec and the WPT test, an exception occurring during oninstall
should not cause installation to fail. This change makes the test pass.

Note that this checks removal may affect the behavior of all ExtendableEvents; 
however it is expected to be fine as explained in the bug.

BUG=685531
==========

to

==========
Fix oninstall-script tests for service worker

According to the spec and the WPT test, an exception occurring during oninstall
should not cause installation to fail. This change makes the test pass.

Note that this checks removal may affect the behavior of all ExtendableEvents; 
however it is expected to be fine as explained in the bug.

BUG=685531
==========

[yiyix, 2017-03-21 05:54:57]

On 2017/03/21 05:52:16, falken wrote:
> lgtm 
> 
> a remaining typo in the CL description: onintall -> oninstall
> 
>
https://codereview.chromium.org/2734783002/diff/140001/third_party/WebKit/Sou...
> File
> third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp
> (right):
> 
>
https://codereview.chromium.org/2734783002/diff/140001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp:214:
> observer->didDispatchEvent(thread()->isForciblyTerminated());
> On 2017/03/21 05:44:27, yiyix wrote:
> > On 2017/03/21 04:27:37, falken wrote:
> > > Note for future reference: we could probably revisit whether we need this
> > check
> > > as well, but it doesn't need to be in this patch.
> > 
> > Would it be preferable to add a TODO?
> 
> Hm, I think it's OK without it. The code will probably be like this for a
while.

Updated the description. Sorry about it.

[yiyix, 2017-03-21 05:55:06]

The CQ bit was checked by yiyix@chromium.org

[commit-bot: I haz the power, 2017-03-21 05:55:22]

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-03-21 07:24:12]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1490075706912720,
"parent_rev": "aaf64ecd8fdbdf8e35f6c7bfe5706df02b777a62", "commit_rev":
"a626eb843a373043835437074f4c7de51baf00f4"}

[commit-bot: I haz the power, 2017-03-21 07:25:13]

Description was changed from

==========
Fix oninstall-script tests for service worker

According to the spec and the WPT test, an exception occurring during oninstall
should not cause installation to fail. This change makes the test pass.

Note that this checks removal may affect the behavior of all ExtendableEvents; 
however it is expected to be fine as explained in the bug.

BUG=685531
==========

to

==========
Fix oninstall-script tests for service worker

According to the spec and the WPT test, an exception occurring during oninstall
should not cause installation to fail. This change makes the test pass.

Note that this checks removal may affect the behavior of all ExtendableEvents; 
however it is expected to be fine as explained in the bug.

BUG=685531

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

[commit-bot: I haz the power, 2017-03-21 07:25:14]

Committed patchset #3 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/a626eb843a373043835437074f4c...