1137248 - Remove FetchEvent.default and FetchEvent.forwardTo

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Josh Matthews [:jdm]]

They were removed in the latest updates to the spec:
https://github.com/slightlyoff/ServiceWorker/issues/607
https://github.com/slightlyoff/ServiceWorker/issues/637

Let's remove them from FetchEvent.webidl and the C++ implementations from ServiceWorkerEvents.cpp.

Comment 1

[Miles Rakestraw]

Hello Josh Matthews.  I believe that I would like to work this bug.  I am a student at WOU currently enrolled in the Open Source Software Development class.  Thank You.

Comment 2

[Josh Matthews [:jdm]]

Go for it!

Comment 3

[Sushrut Girdhari (sg345)]

Attachment: version1.patch

Hi,

I have made an attempt to remove the listed functions. Please do let me know if any changes are required.

Thank you!

Comment 4

[Josh Matthews [:jdm]]

Hi Sushrut! In the future, please look carefully at bugs before you start working on them - Miles told me yesterday that he would attempt this, and I acknowledged that.

Miles, bug 1136950 would be a great "my first patch" task to work on instead, and bug 1136757 would be great if you would like a bit more of a challenge!

Comment 5

[Josh Matthews [:jdm]]

Comment on attachment 8570468 [details] [diff] [review]
version1.patch

Actually, since we're changing a WebIDL file, we also need the approval of a DOM peer (see https://wiki.mozilla.org/Modules/Core#Document_Object_Model). Andrea is a fine choice.

Comment 6

[Josh Matthews [:jdm]]

Comment on attachment 8570468 [details] [diff] [review]
version1.patch

Oops, I just noticed that this won't compile. Sushrut, did you rebuild after you made these changes?

Comment 7

[Sushrut Girdhari (sg345)]

I'm really sorry! I had bookmarked few of the bugs from bugsahoy some time back (and the comment was not posted then) and started working on them and didn't notice the comments while uploading the patch (since it was assigned to nobody at that time.) 

Anyway, I removed "already_AddRefed<Promise>"
 and it seems to compile fine now!

Comment 8

[Sushrut Girdhari (sg345)]

Attachment: version2.patch

Comment 9

[Josh Matthews [:jdm]]

Comment on attachment 8570492 [details] [diff] [review]
version2.patch

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

That looks better. Over to Andrea.

Comment 10

[Andrea Marchesini [:baku]]

Comment on attachment 8570492 [details] [diff] [review]
version2.patch

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

lgtm!

Comment 11

[Andrea Marchesini [:baku]]

Comment on attachment 8570492 [details] [diff] [review]
version2.patch

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

Wait... what about ServiceWorkerevents.h ?
You should remove Default() and ForwardTo() there too.

Comment 12

[Sushrut Girdhari (sg345)]

Attachment: version1.patch

Made appropriate changes.

Comment 13

[Andrea Marchesini [:baku]]

Comment on attachment 8571306 [details] [diff] [review]
version1.patch

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

submit a new patch with these changes.
Do you have access to try server? If not, let me know and I can push this patch for you and see if it breaks things.
Thank you for doing this.

::: dom/workers/ServiceWorkerEvents.h
@@ -41,5 @@
>  
>  public:
>    NS_DECL_ISUPPORTS_INHERITED
>    NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(FetchEvent, Event)
> -  NS_FORWARD_TO_EVENT

don't remove this.

@@ -100,5 @@
>  
>  public:
>    NS_DECL_ISUPPORTS_INHERITED
>    NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(ExtendableEvent, Event)
> -  NS_FORWARD_TO_EVENT

ditto.

@@ -158,5 @@
>  
>  public:
>    NS_DECL_ISUPPORTS_INHERITED
>    NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(InstallEvent, ExtendableEvent)
> -  NS_FORWARD_TO_EVENT

ditto.

Comment 14

[Sushrut Girdhari (sg345)]

Attachment: version2.patch

I do not have access to the try server. However, I have tried building firefox with the changes on my local machine and it seemed to compile fine.

Comment 15

[Andrea Marchesini [:baku]]

Comment on attachment 8571330 [details] [diff] [review]
version2.patch

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=068449c9865f

I pushed your patch to try. Let's see the result.
Ah btw, it doesn't apply correctly for some \r in your patch.

Comment 16

[Josh Matthews [:jdm]]

(In reply to Sushrut Girdhari (sg345) from comment #14)
> Created attachment 8571330 [details] [diff] [review]
> version2.patch
> 
> I do not have access to the try server. However, I have tried building
> firefox with the changes on my local machine and it seemed to compile fine.

You should apply for it by following the steps at https://www.mozilla.org/en-US/about/governance/policies/commit/ . I'll vouch for your access!

Comment 17

[Andrea Marchesini [:baku]]

Ok! The patch is green on try. There are a couple of orange failures but they are not related to your code. If you want I or jdm can land your patch, or you can add 'checkin-needed' in the keyboards for this bug. Thanks again for your patch!

Comment 18

[Sushrut Girdhari (sg345)]

(In reply to Andrea Marchesini (:baku) from comment #17)
> Ok! The patch is green on try. There are a couple of orange failures but
> they are not related to your code. If you want I or jdm can land your patch,
> or you can add 'checkin-needed' in the keyboards for this bug. Thanks again
> for your patch!
Could you please land the patch for me? Thanks!

Comment 19

[Josh Matthews [:jdm]]

Sushrut, you can go ahead and add the checkin-needed keyword to the Keywords field at the top of the bug. That will cause it to be checked in :)

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b09215fcb02f

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b09215fcb02f