Closed Bug 1120652 Opened 6 years ago Closed 6 years ago

Get rid of FetchDriverObserver::OnResponseEnd()

Categories

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

33 Branch
x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: nsm, Assigned: tejas.srinivasan95, Mentored)

References

Details

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

Attachments

(1 file)

Since switching to streams everywhere, an 'end' indication is no longer required. It just complicates our worker release.
Mentor: nsm.nikhil
Whiteboard: [good first bug][lang=C++]
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!
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.
(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?
Kindly review the patch I have attached. Thanks.
Attachment #8549429 - Flags: review?(nsm.nikhil)
Attachment #8549429 - Flags: review?(nsm.nikhil) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d172fffe7074
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.