1531608 - Remove leftover channel context parameter uses

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Description

[Ben Campbell]

Following on from Bug 1531307, there are still a few lingering uses of a context param on nsIChannel-related code.

Ones I've seen so far:

• nsMsgProtcol::m_channelContext
  Used in nsMsgProtocol::OnStopRequest() one place to suppress multiple warnings, and in various derived protocols as a parameter to progress updates.
• nsImapMockChannel::m_channelContext
  Used to pass on to nsIInputStreamPump::AsyncRead() - I think it's never used, so it should be safe to replace with nullptr...
• parameters to OnStatus() and OnProgress in nsIProgressEventSink

In M-C, nsIInputStreamPump::AsyncRead() currently accepts a context param. Probably safe to assume that'll get removed sometime soon too.

Comment 1

[Jorg K (CEST = GMT+2)]

Yes, AsyncRead() just ignores the parameter:
https://searchfox.org/mozilla-central/rev/8308eb7ea14318f53b55f3289c2bb9b712265318/netwerk/base/nsInputStreamPump.cpp#283

While I was doing some reformatting, I removed one here:
https://hg.mozilla.org/comm-central/rev/00dbf8bfc8eb2a26dcdbea3a13bb2e9085dde109#l28.616

I'll do a patch for the rest now.

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 1531608-AsyncRead.patch

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=4d531b0794f28a6d65cd14926458627c1018c69b

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: 1531608-AsyncRead.patch (v2)

Removed a few more NNTP bits. Also removed the member variable from IMAP. NNTP seems to use the one from nsMsgProtocol.h.

I'm not sure whether that stuff can go from nsMsgProtocol.cpp completely. I'd say yes, if it weren't for:
https://searchfox.org/comm-central/rev/6674517c6b2de7ad88628b568b0e8e8e46b65019/mailnews/base/util/nsMsgProtocol.cpp#408

Since IMAP has it's own variable, it's safe to remove.

EDIT: Interdiff to previous patch:
https://bugzilla.mozilla.org/attachment.cgi?oldid=9064348&action=interdiff&newid=9064349&headers=1
Those changes should affect the try run.

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: 1531608-AsyncRead.patch (v3)

This is more drastic. I still left the trickery re. the the alert, but replaced it with a bool. There were also more calls in POP3 that needed m_channelContext removed. Let's see:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=44a039417a1cd5ae9f8803be168c133aeef9fae1

Comment 5

[Jorg K (CEST = GMT+2)]

OK, the try is green. I'll split the patch in two:

Part 1, pass nullptr where the context parameter is ignored:
nsInputStreamPump::AsyncRead():
https://searchfox.org/mozilla-central/rev/8308eb7ea14318f53b55f3289c2bb9b712265318/netwerk/base/nsInputStreamPump.cpp#283

OnStatus() and OnProgress():
https://searchfox.org/comm-central/rev/6674517c6b2de7ad88628b568b0e8e8e46b65019/mailnews/base/src/nsMsgStatusFeedback.cpp#250
https://searchfox.org/comm-central/rev/6674517c6b2de7ad88628b568b0e8e8e46b65019/mailnews/base/src/nsMsgStatusFeedback.cpp#259
https://searchfox.org/comm-central/rev/6674517c6b2de7ad88628b568b0e8e8e46b65019/mailnews/base/src/nsMsgProgress.cpp#247
https://searchfox.org/comm-central/rev/6674517c6b2de7ad88628b568b0e8e8e46b65019/mailnews/base/src/nsMsgProgress.cpp#256

Part 2: The tricky handling of the alert.

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: 1531608-AsyncRead.patch (v4)

This is part 1. Just passing nullptr where the context parameter is ignored anyway. A bit of IDL infrastructure can go as well.

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: 1531608-boolflag.patch

OK, this is part 2. I'm not sure whether the whole thing couldn't be removed altogether, but let's at least lose the notion of the context and make it a bool flag.

The green try originated from the merged patch before I decided to pull it apart for clarity reasons.

Comment 8

[Jorg K (CEST = GMT+2)]

Filed bug 1551306 to see whether M-C want to remove the unneeded context parameters. That shouldn't stop us passing null for the context since all relevant code ignores the context now.

Comment 9

[:aceman]

Comment on attachment 9064508 [details] [diff] [review]
1531608-AsyncRead.patch (v4)

Review of attachment 9064508 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.

Comment 10

[Jorg K (CEST = GMT+2)]

I'm going to land the first part now since I'm in clean-up mode. The second can wait for Ben's review.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/dcd63367f2e4
Pass null as second parameter to AsyncRead(), OnStatus() and OnProgress(). r=aceman

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/73ea6eacc7f9
Follow-up: reformat. rs=reformat DONTBUILD

Comment 13

[Ben Campbell]

Comment on attachment 9064508 [details] [diff] [review]
1531608-AsyncRead.patch (v4)

Not that the extra review is needed, but I have had a look and it all seems good to me.

Comment 14

[Ben Campbell]

Comment on attachment 9064510 [details] [diff] [review]
1531608-boolflag.patch

Review of attachment 9064510 [details] [diff] [review]:
-----------------------------------------------------------------

It does feel like `nsMsgProtocol` shouldn't be calling `mailSession->AlertUser()` (the only reason the `m_hasChannel` flag is needed), but I don't understand the bigger picture well enough to really know for sure.
Either way, it's always so nice seeing a patch remove more than it adds, so r+ from me!

::: mailnews/base/util/nsMsgProtocol.h
@@ +159,5 @@
>    nsCOMPtr<nsIURI> m_originalUrl;  // the original url
>    nsCOMPtr<nsIURI> m_url;          // the running url
>    nsCOMPtr<nsISupports> m_consumer;
>    nsCOMPtr<nsIStreamListener> m_channelListener;
> +  bool m_hasChannel;

Would `m_isChannel` be a more accurate name?

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/57d0ff007e02
Replace m_channelContext with m_isChannel bool flag in nsMsgProtocol.cpp. r=BenC DONTBUILD

Comment 16

[Jorg K (CEST = GMT+2)]

Thanks, Ben. Landed with m_isChannel as suggested. For the record, second part landed at TB 70. Maybe we should backport this for consistency.

Comment 17

[Jorg K (CEST = GMT+2)]

TB 69 beta 2:
https://hg.mozilla.org/releases/comm-beta/rev/160f774b6e653cf0b968120459c987c587dfff36

Comment 18

[Jorg K (CEST = GMT+2)]

TB 68.0 ESR:
https://hg.mozilla.org/releases/comm-esr68/rev/08f45a2b590d50ed640972bca5fe007c72ff569b