1830600 - A lot of nsIMsgWindow.idl is obsolete/not working - need auditing and rework

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Magnus Melin [:mkmelin]]

At least some parts of nsIMsgWindow.idl is not operational in Supernova (and some probably wasn't in great state earlier either).
https://searchfox.org/comm-central/source/mailnews/base/public/nsIMsgWindow.idl

See bug 1828331 comment 11 for one example. Some of this is surely leading to silent failures.

Comment 1

[BB]

Please see bug 1758743 for more.

Comment 2

[Mark Banner (:standard8)]

TBH I've always not liked nsIMsgWindow for the assumptions that are made - that the message pane is the only message browser, and that every window has status feedback etc.

The problem is, it is a work around for what I consider to be a bad architecture. Rather than being explicit and passing in the appropriate browser, listeners or whatever required, we pass in an nsIMsgWindow.

This has the effect of hiding the real issue - what is actually happening, is that the back-end protocol handling is reaching directly into the front-end window and interacting directly. The back-end therefore is making assumptions about the state of the front-end, and that's not a good structure to have.

The other issue here, is that it is assumed that every operation has an nsIMsgWindow associated with it, which with the things we want to do these days, is not necessarily true. So we end up creating fake nsIMsgWindow objects to handle it.

Overall, I've long thought it would be better to pass in the appropriate listeners to the back-end, e.g. status feedback or protocol state, or maybe the browser element if that's needed for streaming or some particular reason (though I'd question if the info could be routed via listeners instead). Preferably, nsIMsgWindow should go away.

Comment 3

[Magnus Melin [:mkmelin]]

Yes, we should try to remove it. I'll try to dismantle and see if there are any blockers.

Comment 4

[Magnus Melin [:mkmelin]]

Attachment: Bug 1830600 - remove nsIMsgWindow.authPrompt. r=ikey

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/ee0eebb68f84
remove nsIMsgWindow.authPrompt. r=ikey

Comment 6

[Magnus Melin [:mkmelin]]

Attachment: Bug 1830600 - remove nsIMsgWindowCommands.selectFolder (and make lots of actions it used to handle work). r=BenC,darktrojan

Functionality that used selectFolder were broken, some of it in many ways.

Fixed:

When handed a feed: url, the subscribed feed is now shown.
You can give a feed url from command line, like
thunderbird -P test http://feeds.bbci.co.uk/news/politics/rss.xml

When handed a news: url, the group is selected (or subscribed).
thunderbird -P test news://raspberrypi.local:119/my.testing

IMAP folder URL handling now working, so you can

• click and imap url in a message you receive
• open imap folder by url
  For testing check folder properties for what the folder url is, then give it as argument from command line. Also send it as a link to yourself and click that to open.

Handling of non-subscribed folders isn't apparently working in imap-cpp (for other reasons, not sure which). It would need more work. I left a comment in imap-js about it too.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e0539a23f82b
remove nsIMsgWindowCommands.selectFolder (and make lots of actions it used to handle work). r=BenC,darktrojan

Comment 8

[Vladimir Panteleev]

Hi,

+      NS_ASSERTION(imapFolder,
+                   nsPrintfCString("No folder for imap url: %s", spec).get());

I think this should have been:

+                   nsPrintfCString("No folder for imap url: %s", spec.get()).get());

as otherwise I get

0:03.92 /home/vladimir/work/extern/mozilla-unified/comm/mailnews/imap/src/nsImapService.cpp:2542:66: error: cannot pass non-trivial object of type 'nsAutoCString' (aka 'nsTAutoStringN<char, AutoStringDefaultStorageSize>') to variadic constructor; expected type from format string was 'char *' [-Wnon-pod-varargs]

Comment 9

[Ben Campbell]

Attachment: Bug 1830600 - Followup to fix minor bustage. r=mkmelin

Compiler didn't like nsAutoCString being used in varags for a printf.
Was also found and fixed in parallel by Vladimir Panteleev.

Comment 10

[Ben Campbell]

Never mind - already fixed by Bug 1834498

Comment 11

[Magnus Melin [:mkmelin]]

Attachment: Bug 1830600 - Remove nsIMsgWindow.windowCommands and related code. r=BenC

Downloading partial pop3 messages was broken.
There was some oddities in how/where switching to the new updated header was handled. I've now
moved it to the place that seems it should be responsible for such things.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/374d48392626
Remove nsIMsgWindow.windowCommands and related code. r=BenC

Comment 13

[Magnus Melin [:mkmelin]]

Attachment: Bug 1830600 - Remove nsIMsgWindow.notificationCallbacks. r=BenC

The only place there were ever set is in the ConfigVerifier, and that doesn't seem to need them.
Bad certificats are handled through the urllistener, since some years now.

Comment 14

[Magnus Melin [:mkmelin]]

Attachment: Bug 1830600 - Remove unneeded usages of setting appType. r=#thunderbird-reviewers

As I understand it, apptype is/was used in the suite (seamonkey) to to get different behaviors for loading content in the browser window vs. mail window.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5ad63468cbe2
Remove nsIMsgWindow.notificationCallbacks. r=BenC

Comment 16

[Alessandro Castellani [:aleca]]

Thanks for doing this clean up.
Since we're in the last stretch of 115 let's try to avoid leave-open bugs. As Rob requested in the past let's use separate bugs so it's easier to track uplifts if needed.

Comment 17

[Magnus Melin [:mkmelin]]

Yeah I'll file a new bug for the additional work needed.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2f627670b2bd
Remove unneeded usages of setting appType. r=aleca