Closed Bug 1530209 Opened 3 years ago Closed 2 months ago

Remove context argument from remaining methods

Categories

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

65 Branch
enhancement

Tracking

()

RESOLVED FIXED
96 Branch
Tracking Status
firefox96 --- fixed

People

(Reporter: jkt, Assigned: ssummar, Mentored)

References

Details

(Keywords: good-first-bug)

Attachments

(3 files)

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.

Also:

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

Priority: -- → P3
Component: DOM → DOM: Core & HTML

Also nsInputStreamPump::AsyncRead I guess?

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

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.

Mentor: jkt
Keywords: good-first-bug
OS: Unspecified → All
Hardware: Unspecified → All
See Also: → 1528649

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

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

Flags: needinfo?(jkt)

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

Flags: needinfo?(jkt) → needinfo?(kanurag94)

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

Flags: needinfo?(kanurag94)
Assignee: nobody → kanurag94

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

Assignee: kanurag94 → nobody

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

Assignee: nobody → qopxelzg
Status: NEW → ASSIGNED

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

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

Assignee: qopxelzg → nobody
Status: ASSIGNED → NEW
Assignee: nobody → ssummar.bee16seecs
Status: NEW → ASSIGNED

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

Attachment #9248217 - Attachment description: Bug 1530209 - Removed context arguement from various methods. r=hsinyi → Bug 1530209 - Removed context argument from various methods. r=hsinyi

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.

Flags: needinfo?(jonathan)

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.

Flags: needinfo?(valentin.gosu)

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.

Flags: needinfo?(valentin.gosu)
Flags: needinfo?(jonathan)
Flags: needinfo?(gijskruitbosch+bugs)

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

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(valentin.gosu)

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

Flags: needinfo?(valentin.gosu)
Pushed by valentin.gosu@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4173edfbf6a7
Removed context argument from various methods. r=necko-reviewers,valentin
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → 96 Branch
You need to log in before you can comment on or make changes to this bug.