1152473 - Remove InstallEvent

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

InstallEvent is no longer part of the ServiceWorker spec since the extra attributes it provided over ExtendableEvent are available on the registration.

Steps:
1) Remove dom/webidl/InstallEvent.webidl, update the moz.build in the same file and update dom/bindings/Bindings.conf
2) Remove InstallEvent from dom/workers/ServiceWorkerEvents.{h,cpp}
3) Change dom/workers/ServiceWorkerManager.cpp to replace the use of InstallEvent with ExtendableEvent in LifecycleEventWorkerRunnable::DispatchLifecycleEvent(). That will mean some of the code can be refactored in that function.

Comment 1

[Rahul]

Should NS_IMPL_ADDREF_INHERITED, and similar be removed too? Or there is some modification in them?

Comment 2

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Yes, everything related to InstallEvent can go.

Comment 3

[Rahul]

Attachment: b-1152473-p1.patch

Comment 4

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8591312 [details] [diff] [review]
b-1152473-p1.patch

Review of attachment 8591312 [details] [diff] [review]:
-----------------------------------------------------------------

Please remove the webidl file from version control and upload a new patch containing that change and the two changes below.

::: dom/workers/ServiceWorkerEvents.cpp
@@ +364,1 @@
>  NS_INTERFACE_MAP_END_INHERITING(ExtendableEvent)

This line has to be removed too.

::: dom/workers/ServiceWorkerManager.cpp
@@ +1080,5 @@
>    } else if (mEventName.EqualsASCII("activate")) {
>      ExtendableEventInit init;
>      init.mBubbles = false;
>      init.mCancelable = true;
>      event = ExtendableEvent::Constructor(target, mEventName, init);

"install" and "activate" now has the same code, so the blocks can be collapsed.

Comment 5

[Abhishek Singh Kushwah]

Hi,

Can i work on this bug?

Comment 6

[Rahul]

Attachment: b-1152473-p2.patch

Comment 7

[Rahul]

Attachment: b-1152473-p3.patch

Please do not mind the last patch. There was a silly mistake in that.

Comment 8

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8593343 [details] [diff] [review]
b-1152473-p3.patch

Review of attachment 8593343 [details] [diff] [review]:
-----------------------------------------------------------------

Please remove the AsInstallEvent() declarations https://dxr.mozilla.org/mozilla-central/search?q=AsInstallEvent&case=true&=mozilla-central&redirect=true

mozilla-central has changed, so you will have to rebase your patch on top of the latest central, particularly the changes on ServiceWorkerEvents.{h,cpp}

Please upload the new patch with those changes. There is no need to ask for review again, but feel free to if you feel more comfortable that way.

Comment 9

[Rahul]

Attachment: b-1152473-p4.patch

Comment 10

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Remove InstallEvent

minor compile fixes. Ehsan, could you r? the interfaces change?

Comment 11

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=024c9eb3a87b

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/50f15cd087be

Comment 13

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/50f15cd087be

Comment 14

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Thanks for the patch Rahul! It will land in a day or so.

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/50f15cd087be