1120652 - Get rid of FetchDriverObserver::OnResponseEnd()

Status: RESOLVED FIXED

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

Description

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

Since switching to streams everywhere, an 'end' indication is no longer required. It just complicates our worker release.

Comment 1

[Tejas Srinivasan]

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?

Comment 2

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

(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.

Comment 3

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

The changes have landed. Please go ahead with fixing this. Thanks!

Comment 4

[Tejas Srinivasan]

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.

Comment 5

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

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.

Comment 6

[Tejas Srinivasan]

(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?

Comment 7

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

No changes.

Comment 8

[Tejas Srinivasan]

Attachment: remove_OnResponseEnd.patch

Kindly review the patch I have attached. Thanks.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

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

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