1240327 - Improve ability of key mailnews components to be overridden with JS

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Comment 1

[Kent James (:rkent)]

Attachment: Collect interface changes from bug 430716

Comment 2

[Kent James (:rkent)]

This needs to land in TB 45 if we are going to support JsAccount there.

Comment 3

[Joshua Cranmer [:jcranmer]]

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.

Comment 4

[Kent James (:rkent)]

Attachment: rev 2, remove idl changes for localDatabaseType, and fix review issues

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.

Comment 5

[Kent James (:rkent)]

I removed trailing spaces in existing files as part of doing this patch, which introduced trivial changes in a variety of unrelated locations.

Comment 6

[Kent James (:rkent)]

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)

Comment 7

[Kent James (:rkent)]

https://hg.mozilla.org/comm-central/rev/969a3a5cb50f

Comment 8

[Kent James (:rkent)]

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.

Comment 9

[Jorg K (CEST = GMT+2)]

(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 ;-)

Comment 10

[aleth [:aleth]]

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.

Comment 11

[aleth [:aleth]]

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

Comment 12

[Kent James (:rkent)]

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

Comment 13

[aleth [:aleth]]

(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

Comment 14

[aleth [:aleth]]

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

Comment 15

[aleth [:aleth]]

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 *)]

Comment 16

[:aceman]

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

Comment 17

[:aceman]

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.

Comment 18

[aleth [:aleth]]

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

Comment 19

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/ae76d6797570dba31be02a1f354548e94ffcdec0
Backout 969a3a5cb50f (Bug 1240327) for xpcshell crashes.

Comment 20

[Kent James (:rkent)]

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.

Comment 21

[Kent James (:rkent)]

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

[Kent James (:rkent)]

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