1530209 - Remove context argument from remaining methods

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P3)

Description

[Jonathan Kingston [:jkt] he/him]

Methods include:

DoOnStartRequest
nsIndexedToHTML::DoOnStartRequest
DispatchContent
InterceptStreamListener::OnStatus
InterceptStreamListener::OnProgress
InterceptStreamListener::DoOnDataAvailable
OnImageDataAvailable
nsDeflateConverter::PushAvailableData
do_OnDataAvailable
FireListenerNotifications
nsDirIndexParser::ProcessData
SendToListener

The intent is to remove mContext once these are resolved also.

Comment 1

[Jonathan Kingston [:jkt] he/him]

Also:

imgRequest::OnDataAvailable
OnImageDataComplete
PushAvailableData
OnDownloadComplete
nsDirIndexParser::ProcessData
nsIndexedToHTML::SendToListener
FireListenerNotifications
OnStopRequestInternal

Comment 2

[[e-mail]]

Also nsInputStreamPump::AsyncRead I guess?

Comment 3

[[e-mail]]

Oh, I see now that someone already filed bug 1528649 for that.

Comment 4

[Jonathan Kingston [:jkt] he/him]

I did have a patch that resolved this however I lost the rebasing battle, it's easy enough to resolve for a good first bug though.

Comment 5

[Anurag Aggarwal]

Hi @jkt,
I would like to work on this bug

Comment 6

[Vijitha]

Hi @jkt is anyone working on this? Can i work on it?

Comment 7

[Jonathan Kingston [:jkt] he/him]

Sorry Anurag I missed your message, could you confirm if you are working on this?

Comment 8

[Anurag Aggarwal]

Hi Jonathan, Sorry for late reply, didn't log in in last couple of days.
I will be working on this starting today.
Thanks!

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 6 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Comment 10

[Anton Bershanskiy]

Attachment: WIP: Bug 1530209 - Remove context argument where it is not needed

Comment 11

[Anton Bershanskiy]

:jkt would you be interested in reviewing a patch I drafted? (linked above)

I removed the context argument from all methods listed above except for OnDownloadComplete. I'm not certain whether I can remove it from there, since OnDownloadComplete is exposed to other contexts, e.g. netwerk/test/unit/test_bug263127.js (line 11). Can I just update OnDownloadComplete functions in C/C++ code, IDL bindings and that one JS file or is there more?

Comment 12

[Anton Bershanskiy]

Attachment: WIP: Bug 1530209 - Remove context argument where it is not needed

Depends on D114057

Comment 13

[Anton Bershanskiy]

Please ignore D114058 I uploaded it by accident while trying to force-push ot previous differential.

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Comment 15

[Shazib Summar [:ssummar]]

Attachment: Bug 1530209 - Removed context argument from various methods. r=hsinyi

Comment 16

[Shazib Summar [:ssummar]]

Hi there, I added a patch. Please let me know if any revisions are required.

Comment 17

[Shazib Summar [:ssummar]]

TLDR: Removing context param from FireListenerNotifications means I would need to modify nsIStreamConverter::AsyncConvertData however its used in PdfStreamConverter.asyncConvertData where the context parameter is used inside a conditional statement. How do I proceed?

Hi there. I've been working on a patch for this bug and so far I have modified all the functions except FireListenerNotifications. I need some guidance on how to move forward with this one.

nsUnknownDecoder::FireListenerNotifications uses a context parameter that its passes on to nsIEncodedChannel::doApplyContentConversion. This idl function would be simple enough to modify were it not overridden by HttpBaseChannel::DoApplyContentConversions. This function itself uses the context parameter too and passes it on to mozilla::net::nsHTTPCompressConv::AsyncConvertData. I could modify this function too but it overrides nsIStreamConverter::AsyncConvertData and this is where the problem is. The idl interface is exposed to PdfStreamConverter.asyncConvertData. This function uses the context parameter as a conditional statement inside its body.

Can you please give me your comments on this? I have pushed the updated patch (last one was failing some tests) which removes the context parameter from the rest of the functions. Thanks.

Comment 18

[Shazib Summar [:ssummar]]

Hello. Sorry for the ping but could you kindly give me some guidance regarding my last comment. I pinged jkt however, their last activity was in July.

Comment 19

[Valentin Gosu [:valentin] (he/him)]

Sorry for the late reply.

(In reply to Shazib Summar from comment #17)

TLDR: Removing context param from FireListenerNotifications means I would need to modify nsIStreamConverter::AsyncConvertData however its used in PdfStreamConverter.asyncConvertData where the context parameter is used inside a conditional statement. How do I proceed?

I think it's probably fine to remove it. We seem to pass nullptr to all calls of DoApplyContentConversions.

The idl interface is exposed to PdfStreamConverter.asyncConvertData. This function uses the context parameter as a conditional statement inside its body.

Gijs can confirm if the ctx is actually used.

Comment 20

[:Gijs (he/him)]

(In reply to Valentin Gosu [:valentin] (he/him) from comment #19)

The idl interface is exposed to PdfStreamConverter.asyncConvertData. This function uses the context parameter as a conditional statement inside its body.

Gijs can confirm if the ctx is actually used.

Yes, it's QI'd to nsIChannel and then passed to getConvertedType which uses the target browsing context of the channel's load info, the channel's URI and the channel's (loadinfo's) triggering principal. So I expect it shouldn't be removed there, though if we could specify the interface as nsIChannel that would help make the use of the parameter clearer than just having it be an arbitrary "context". Valentin, wdyt?

Comment 21

[Valentin Gosu [:valentin] (he/him)]

(In reply to :Gijs (he/him) from comment #20)

Yes, it's QI'd to nsIChannel and then passed to getConvertedType which uses the target browsing context of the channel's load info, the channel's URI and the channel's (loadinfo's) triggering principal. So I expect it shouldn't be removed there, though if we could specify the interface as nsIChannel that would help make the use of the parameter clearer than just having it be an arbitrary "context". Valentin, wdyt?

That sounds reasonable. We can probably do it in a separate patch from D129797.
Thanks!

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4173edfbf6a7
Removed context argument from various methods. r=necko-reviewers,valentin

Comment 23

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/4173edfbf6a7