Improve ability of key mailnews components to be overridden with JS

RESOLVED FIXED in Thunderbird 46.0

Status

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: rkent, Assigned: rkent)

Tracking

(Blocks 1 bug)

Trunk
Thunderbird 46.0
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird44 wontfix, thunderbird45+ fixed, thunderbird46 fixed, thunderbird_esr38 wontfix)

Details

Attachments

(1 attachment, 1 obsolete attachment)

No description provided.
Assignee: nobody → rkent
Status: NEW → ASSIGNED
Attachment #8708725 - Flags: review?(Pidgeot18)
This needs to land in TB 45 if we are going to support JsAccount there.
Comment on attachment 8708725 [details] [diff] [review]
Collect interface changes from bug 430716

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

r- for the localDatabaseType change. This can be upgraded to an r+ if you remove those related changes.

::: mailnews/base/public/nsIMsgIncomingServer.idl
@@ +95,5 @@
> +   * The schema for the nsIMsgDatabase implementation, such as "mailbox" or
> +   * "imap", that will be used to construct the database instance used by
> +   * message folders associated with this server.
> +   */
> +  readonly attribute ACString localDatabaseType;

As mentioned over IRC, it's not clear why you need this, given that it's easy to map a new contract-id over an existing CID.

::: mailnews/compose/public/nsIMsgAttachmentHandler.idl
@@ +5,5 @@
> +#include "nsISupports.idl"
> +interface nsIFile;
> +
> +// This interface provides minimal XPCONNECT access to objects of type
> +// nsMsgAttachmentHandler.

(See comments on my other review here).

::: mailnews/local/src/nsLocalMailFolder.cpp
@@ +3084,5 @@
>  // this method will go away.
>  // sometimes this gets called when we don't have the server yet, so
>  // that's why we're not calling GetServer()
> +NS_IMETHODIMP
> +nsMsgLocalMailFolder::GetIncomingServerType(nsACString& aServerType)

Hmm, on further reflection, I wonder if this will break account types that try to use mailbox: folders, since there's a hard-coded list here.
Attachment #8708725 - Flags: review?(Pidgeot18) → review-
This is what I'll land so that we'll have the key interface changes needed for TB45. I'm going to carry through the r+ from comment 3 "This can be upgraded to an r+ if you remove those related changes."

"> +NS_IMETHODIMP
> +nsMsgLocalMailFolder::GetIncomingServerType(nsACString& aServerType)

Hmm, on further reflection, I wonder if this will break account types that try to use mailbox: folders, since there's a hard-coded list here."

I don't really understand the question. Yes it is true that more work is needed to allow nsLocalFolders to accept other server types and still work, which is what is currently done with many server types. But it is possible to do a simple JS Extension of nsLocalFolders and override this method if needed, but it might be easier in the future to allow the server type to be set here by a JS extension without having to override the full local folder object.
Attachment #8708725 - Attachment is obsolete: true
I removed trailing spaces in existing files as part of doing this patch, which introduced trivial changes in a variety of unrelated locations.
Comment on attachment 8708737 [details] [diff] [review]
rev 2, remove idl changes for localDatabaseType, and fix review issues

carrying forward jcranmer's offer of r+ with changes (that are implemented)
Attachment #8708737 - Flags: review+
https://hg.mozilla.org/comm-central/rev/969a3a5cb50f
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 46.0
Comment on attachment 8708737 [details] [diff] [review]
rev 2, remove idl changes for localDatabaseType, and fix review issues

[Approval Request Comment]
After this has baked for a day or two, I'll try to land it in aurora. Let me do it, Jorg.
Attachment #8708737 - Flags: approval-comm-aurora?
(In reply to Kent James (:rkent) from comment #8)
> [Approval Request Comment]
> After this has baked for a day or two, I'll try to land it in aurora. Let me
> do it, Jorg.
Sure, all yours ;-)
While I understand there is time pressure, I'm not sure it's a great idea to land something like this while all test coverage is busted without at least successful try push results. Please don't uplift until it's been confirmed this doesn't break tests.
(In reply to aleth [:aleth] from comment #10)
> While I understand there is time pressure, I'm not sure it's a great idea to
> land something like this while all test coverage is busted without at least
> successful try push results. Please don't uplift until it's been confirmed
> this doesn't break tests.

PS It should be possible to do a try push by using a m-c revision from before bug 1239352.
(In reply to aleth [:aleth] from comment #10)
> While I understand there is time pressure, I'm not sure it's a great idea to
> land something like this while all test coverage is busted without at least
> successful try push results. Please don't uplift until it's been confirmed
> this doesn't break tests.

I have argued repeatedly that the current system of keeping comm-central broken half of the time, and full of failed tests from m-c uplifts the rest of the time, is not an acceptable way to run things. This issue gets particularly acute near release times, when we absolutely must get stuff landed on c-c that are critical to the release process for uplift to other repos. I request again, let's stop this madness of keeping c-c perma-broken or perma-failed.

So last year I was forced to land things on c-c even when it was technically closed, and I expect to have to continue that process this year. I have little choice if we are going to get critical things into releases. As for try runs with modified mozilla revs, I thought there were known problems with that so I have been reluctant to use that approach, but I could try again.
(In reply to Kent James (:rkent) from comment #12)
> I have argued repeatedly that the current system of keeping comm-central
> broken half of the time, and full of failed tests from m-c uplifts the rest
> of the time, is not an acceptable way to run things. This issue gets
> particularly acute near release times, when we absolutely must get stuff
> landed on c-c that are critical to the release process for uplift to other
> repos. I request again, let's stop this madness of keeping c-c perma-broken
> or perma-failed.
> So last year I was forced to land things on c-c even when it was technically
> closed, and I expect to have to continue that process this year. I have
> little choice if we are going to get critical things into releases. 

c-c has not been closed for most of this cycle, but let's not reopen this discussion in this bug.


> As for
> try runs with modified mozilla revs, I thought there were known problems
> with that so I have been reluctant to use that approach, but I could try
> again.

It should be enough to push to try together with a m-c patch that backs out https://hg.mozilla.org/mozilla-central/rev/7c17147a801f:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a4cea69a06aa
(In reply to aleth [:aleth] from comment #13)
> It should be enough to push to try together with a m-c patch that backs out
> https://hg.mozilla.org/mozilla-central/rev/7c17147a801f:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try-comm-
> central&revision=a4cea69a06aa

Looks like that didn't work :-(
OK, now the tests are back:

New xpcshell failure - please check if it is indeed caused by this bug:

PROCESS-CRASH | mailnews/compose/test/unit/test_autoReply.js | application crashed [@ nsMsgCompose::SendMsgToServer(int,nsIMsgIdentity *,char const *)]
Flags: needinfo?(rkent)
(In reply to aleth [:aleth] from comment #13)
> (In reply to Kent James (:rkent) from comment #12)
> > I request again, let's stop this madness of keeping c-c perma-broken
> > or perma-failed.
> 
> c-c has not been closed for most of this cycle, but let's not reopen this
> discussion in this bug.

So where can we discuss this? Nobody likes c-c to be perma-broken but what/who can do something with it? Few of us can fix things on the server.
I looked at the patch and I'd like to ask what is the preferred style for comments in .idl files. Sometimes you add triple slashes (///), sometimes double (//) even though the file uses /* */ even for one-line comments.
(In reply to aleth [:aleth] from comment #15)
> New xpcshell failure - please check if it is indeed caused by this bug:
> 
> PROCESS-CRASH | mailnews/compose/test/unit/test_autoReply.js | application
> crashed [@ nsMsgCompose::SendMsgToServer(int,nsIMsgIdentity *,char const *)]

Confirmed on try.
Flags: needinfo?(rkent)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Modified an pushed: https://hg.mozilla.org/comm-central/rev/75b05edca192

The crashing was in a new section that was not critical to the need to get interface changes landed, so I removed that section. I filed followup bug 1240823 to add it back with correct parameters.
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
(In reply to :aceman from comment #17)
> I looked at the patch and I'd like to ask what is the preferred style for
> comments in .idl files. Sometimes you add triple slashes (///), sometimes
> double (//) even though the file uses /* */ even for one-line comments.

I've always followed DOxygen standards which allow either /** ... */ or ///

I tend to use /// for short comments (like a one or two line description of an attribute) and /** for longer block comments. But looking around a little in Mozilla code, I don't see a lot of triple slashes.
Comment on attachment 8708737 [details] [diff] [review]
rev 2, remove idl changes for localDatabaseType, and fix review issues

https://hg.mozilla.org/releases/comm-aurora/rev/def4ade3380d
Attachment #8708737 - Flags: approval-comm-aurora? → approval-comm-aurora+
You need to log in before you can comment on or make changes to this bug.