Remove context argument from remaining methods
Categories
(Core :: DOM: Core & HTML, enhancement, P3)
Tracking
()
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.
Reporter | ||
Comment 1•5 years ago
|
||
Also:
imgRequest::OnDataAvailable
OnImageDataComplete
PushAvailableData
OnDownloadComplete
nsDirIndexParser::ProcessData
nsIndexedToHTML::SendToListener
FireListenerNotifications
OnStopRequestInternal
Updated•5 years ago
|
Updated•5 years ago
|
Comment 2•5 years ago
|
||
Also nsInputStreamPump::AsyncRead I guess?
Comment 3•5 years ago
|
||
Oh, I see now that someone already filed bug 1528649 for that.
Reporter | ||
Comment 4•5 years ago
|
||
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•4 years ago
|
||
Hi @jkt,
I would like to work on this bug
Hi @jkt is anyone working on this? Can i work on it?
Reporter | ||
Comment 7•4 years ago
|
||
Sorry Anurag I missed your message, could you confirm if you are working on this?
Comment 8•4 years ago
|
||
Hi Jonathan, Sorry for late reply, didn't log in in last couple of days.
I will be working on this starting today.
Thanks!
Reporter | ||
Updated•4 years ago
|
Comment 9•3 years ago
|
||
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•3 years ago
|
||
Comment 11•3 years ago
|
||
: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?
Updated•3 years ago
|
Comment 12•3 years ago
|
||
Depends on D114057
Comment 13•3 years ago
|
||
Please ignore D114058 I uploaded it by accident while trying to force-push ot previous differential.
Comment 14•3 years ago
|
||
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 | ||
Comment 15•2 years ago
|
||
Updated•2 years ago
|
Assignee | ||
Comment 16•2 years ago
|
||
Hi there, I added a patch. Please let me know if any revisions are required.
Updated•2 years ago
|
Assignee | ||
Comment 17•2 years ago
|
||
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.
Assignee | ||
Comment 18•2 years ago
|
||
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•2 years ago
|
||
Sorry for the late reply.
(In reply to Shazib Summar from comment #17)
TLDR: Removing context param from
FireListenerNotifications
means I would need to modifynsIStreamConverter::AsyncConvertData
however its used inPdfStreamConverter.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•2 years ago
|
||
(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•2 years ago
|
||
(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•2 years ago
|
||
Pushed by valentin.gosu@gmail.com: https://hg.mozilla.org/integration/autoland/rev/4173edfbf6a7 Removed context argument from various methods. r=necko-reviewers,valentin
Comment 23•2 years ago
|
||
bugherder |
Description
•