If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Get rid of FetchDriverObserver::OnResponseEnd()

RESOLVED FIXED in mozilla38

Status

()

Core
DOM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: nsm, Assigned: Tejas Srinivasan, Mentored)

Tracking

33 Branch
mozilla38
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment)

Since switching to streams everywhere, an 'end' indication is no longer required. It just complicates our worker release.
Blocks: 1039846
Mentor: nsm.nikhil@gmail.com
Whiteboard: [good first bug][lang=C++]
(Assignee)

Comment 1

3 years ago
I'd like to work on this bug. I went through the class definition of FetchDriverObserver, but couldn't find a member function OnResponseEnd(). Can you please point me to the relevant code?
(In reply to Tejas Srinivasan from comment #1)
> I'd like to work on this bug. I went through the class definition of
> FetchDriverObserver, but couldn't find a member function OnResponseEnd().
> Can you please point me to the relevant code?

The relevant code will be there once the patches in Bug 1039846 have landed. I will land them today.
The changes have landed. Please go ahead with fixing this. Thanks!
(Assignee)

Comment 4

3 years ago
So do I just need to remove the OnResponseEnd() function from the FetchDriverObserver class and its child classes (declaration and definitions)? Is there anything else to do? Do I remove the two function calls as well ([1], [2])?

[1] : https://dxr.mozilla.org/mozilla-central/source/dom/fetch/FetchDriver.cpp#475
[2] : https://dxr.mozilla.org/mozilla-central/source/dom/fetch/FetchDriver.cpp#486

Sorry for the noob doubts, I'm kinda new around here.
So after some thinking, we can't really get rid of OnResponseEnd() entirely. On workers, we add a feature to keep the worker alive until the fetch is done. If we remove the feature in OnResponseAvailable() we will end up allowing the worker to terminate before the response is complete. So, let's get rid of MainThreadFetchResolver::OnResponseEnd(), and provide a default empty definition of it in FetchDriverObserver.
(Assignee)

Comment 6

3 years ago
(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #5)

> So, let's get rid of MainThreadFetchResolver::OnResponseEnd(), and provide a default
> empty definition of it in FetchDriverObserver.

OK, but what about the two function calls? Will any modification be required on that end?
No changes.
(Assignee)

Comment 8

3 years ago
Created attachment 8549429 [details] [diff] [review]
remove_OnResponseEnd.patch

Kindly review the patch I have attached. Thanks.
Attachment #8549429 - Flags: review?(nsm.nikhil)
Attachment #8549429 - Flags: review?(nsm.nikhil) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/d172fffe7074
Assignee: nobody → tejas.srinivasan95
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d172fffe7074
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.