Closed Bug 1137248 Opened 6 years ago Closed 6 years ago

Remove FetchEvent.default and FetchEvent.forwardTo

Categories

(Core :: DOM: Workers, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: jdm, Assigned: developer, Mentored)

Details

(Whiteboard: [lang=c++][good first bug])

Attachments

(1 file, 3 obsolete files)

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.
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.
Go for it!
Attached patch version1.patch (obsolete) — Splinter Review
Hi,

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

Thank you!
Attachment #8570468 - Flags: review?(josh)
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!
Assignee: nobody → developer
Attachment #8570468 - Flags: review?(josh) → review+
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.
Attachment #8570468 - Flags: review?(amarchesini)
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?
Attachment #8570468 - Flags: review?(amarchesini)
Attachment #8570468 - Flags: review-
Attachment #8570468 - Flags: review+
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!
Attached patch version2.patch (obsolete) — Splinter Review
Attachment #8570492 - Flags: review?(josh)
Comment on attachment 8570492 [details] [diff] [review]
version2.patch

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

That looks better. Over to Andrea.
Attachment #8570492 - Flags: review?(josh) → review?(amarchesini)
Comment on attachment 8570492 [details] [diff] [review]
version2.patch

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

lgtm!
Attachment #8570492 - Flags: review?(amarchesini) → review+
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.
Attachment #8570492 - Flags: review+ → review-
Attached patch version1.patch (obsolete) — Splinter Review
Made appropriate changes.
Attachment #8571306 - Flags: review?(amarchesini)
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.
Attachment #8571306 - Flags: review?(amarchesini) → review+
Attached patch version2.patchSplinter Review
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.
Attachment #8570492 - Attachment is obsolete: true
Attachment #8571306 - Attachment is obsolete: true
Attachment #8571330 - Flags: review?(amarchesini)
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.
Attachment #8571330 - Flags: review?(amarchesini) → review+
(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!
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!
Flags: needinfo?(developer)
(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!
Flags: needinfo?(developer)
Flags: needinfo?(amarchesini)
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 :)
Flags: needinfo?(amarchesini)
Keywords: checkin-needed
Attachment #8570468 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/b09215fcb02f
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.