342632 - (null_default_server) Allow defaultAccount to return NULL and declare it a NS_OK success (was Enable Exception thrown opening mail when there are no accounts)

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Andrew Schultz]

With current trunk, if I open mail in a profile with no accounts, I get

Error: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIMsgAccountManager.defaultAccount]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://messenger/content/msgMail3PaneWindow.js :: MigrateJunkMailSettings :: line 1601"  data: no]

This is from bug 257990.  Same bug appears to exist in TBird.

Comment 1

[Andrew Schultz]

Attachment: patch

Comment 2

[Andrew Schultz]

alternatively, nsMsgAccountManager::GetDefaultAccount could return NS_OK instead of NS_ERROR_FAILURE when there are no accounts.

Comment 3

[[e-mail]]

Comment on attachment 226930 [details] [diff] [review]
patch

I guess the other approach would be try { if (defaultAccount.incomingServer) { ... } }

Comment 4

[Andrew Schultz]

Attachment: patch v2, just return null

This implements comment 2.  I also fixed up the callers that were expecting it to throw an exception for this case.

Thunderbird has an extra bit in loadStartFolder that needed to move, but I wasn't quite sure of its purpose (handling news URIs for unsubscribed newsgroups?).  I moved it and added it to SeaMonkey.

Comment 5

[Andrew Schultz]

Attachment: patch v2 -w, for review

Comment 6

[[e-mail]]

Comment on attachment 226959 [details] [diff] [review]
patch v2 -w, for review

>   nsCOMPtr <nsIMsgAccount> account;
>   rv = accountManager->GetDefaultAccount(getter_AddRefs(account));
>-  if (NS_FAILED(rv)) {
>+  if (NS_FAILED(rv) || !account) {
Do you need to check both rv and account? [Also again below]

>       nsresult rv;
>       nsCOMPtr <nsIMsgAccountManager> accountManager = 
>         do_GetService(NS_MSGACCOUNTMANAGER_CONTRACTID, &rv); 
>       NS_ENSURE_SUCCESS(rv,MAPI_E_LOGIN_FAILURE);
>       nsCOMPtr <nsIMsgAccount> account;
>       nsCOMPtr <nsIMsgIdentity> identity;
>        rv = accountManager->GetDefaultAccount(getter_AddRefs(account));
>       NS_ENSURE_SUCCESS(rv,MAPI_E_LOGIN_FAILURE);
>+      if (!account)
>+        return MAPI_E_LOGIN_FAILURE;
>       account->GetDefaultIdentity(getter_AddRefs(identity));
>       NS_ENSURE_SUCCESS(rv,MAPI_E_LOGIN_FAILURE);
>       identity->GetKey(&id_key);
So, I wonder what this last NS_ENSURE_SUCCESS is checking...
(f.y.i. GetDefaultIdentity only fails if the prefs are corrupt).

(In reply to comment #4)
>I wasn't quite sure of its purpose
Well I think you would need to be sure before thinking of copying it.

Comment 7

[[e-mail]]

Comment on attachment 226959 [details] [diff] [review]
patch v2 -w, for review

I think the msgMail3PaneWindow changes are wrong here (I can't see why RDF.GetResource would fail for instance).

Comment 8

[Scott MacGregor]

Comment on attachment 226930 [details] [diff] [review]
patch

clearing the review request, this patch is obsolete now that you are working on patch 2 right?

Comment 9

[Andrew Schultz]

Scott, I don't which approach you'd prefer.  The second approach seems more logical to me.  What's holding up the second patch is mostly this chunk:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mail/base/content/msgMail3PaneWindow.js&rev=1.88&mark=972-976#969

that you added as part of rev 1.26 ("Temporary [2+ years ago! :)] work around to make thunderbird subscribe to newsgroups you are not already").  I need it move around and don't know what it's try to accomplish (see comment 7).

Comment 10

[Scott MacGregor]

Comment on attachment 226959 [details] [diff] [review]
patch v2 -w, for review

it looks like Neil already minused this patch so unsetting the sr request.

Comment 11

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

A clean Mac trunk Thunderbird debug build outputs the following on quitting the Account Wizard:

[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIMsgAccountManager.defaultAccount]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://messenger/content/msgMail3PaneWindow.js :: loadStartFolder :: line 846"  data: no]Exception in LoadStartFolder caused by no default account.  We know about this

Since this affects Thunderbird as well, should the bug be moved to Core -> MailNews ?

Comment 12

[Magnus Melin [:mkmelin]]

[moving...] 

Comment 13

[:aceman]

What about reviving this patch?
But it seems the property is used in many places
(http://mxr.mozilla.org/comm-central/search?string=.defaultAccount&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central)
where many just apply other properties to it (like .key, or .imcommingServer). Wouldn't they fail now and need fixing?

Comment 14

[Magnus Melin [:mkmelin]]

> Wouldn't they fail now and need fixing?

Most wouldn't since you shouldn't exercise the functions using it without an account set up. (E.g. the trigger menu item would be disabled.)

Comment 15

[:aceman]

So basically where there is a try {} catch{} block today, it could just be changed to a check if(!AM.defaultAccount) ?

Is there any consensus today that this is still wanted?
This is also requested in bug 739258 due to IM landing.

I think I understand what the patch is doing and could try to pick it up.
If Andrew is no longer working on it, of course.

Comment 16

[Magnus Melin [:mkmelin]]

Something like that yes, unless there are other throwers in the block. I'd say go for it.

Comment 17

[:aceman]

Looks like now is a good time to try this breakage when we have the time until TB24 :)

My plan would be to split the work into 3 patches to lessen chance of bitrot:
1. update all Javascript callers to check if defaultAccount is null and bail out safely.
http://mxr.mozilla.org/comm-central/search?string=.defaultAccount&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central
a) Thunderbird part
b) Seamonkey part

2. update all C++ callers to to check if defaultAccount is null and bail out safely (mostly add NS_ENSURE_TRUE (account) with the same error value as current NS_ENSURE_SUCCESS(rv) returns).
http://mxr.mozilla.org/comm-central/search?string=getdefaultAccount&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central

3. change mailnews/base/src/nsMsgAccountManager.cpp to return NS_OK when a null account is returned.

When each step is finished the patch should be checked in. So that when patch 3. is checked in, the whole codebase is prepared for it and will not break.

Also, is there a TB24 for developers page like Firefox has? This would be nice to promote so that in future devs remember to check defaultAccount for null.

But before I start here, we need to move with bug 749200 so that some callers go away :)

Comment 18

[:aceman]

But I think I can't remove all the try {} catch() blocks around .defaultServer because it can fail for other reasons then no proper server available. So I leave them in and just add a new check in case of success to check for null and do what is needed. Often is is just bailout as the catch branch would do. But sometimes we can use the information that there is no default account more than if we just got a generic error.

Comment 19

[Ben Bucksch (:BenB)]

Are you sure this still exists? account creation dialog has always (since 2009) been quering defaultAccount from JS and testing for null/false, and has not been throwing:

Original version from 2009:
https://searchfox.org/comm-central/rev/257141c7d72ce7959639e94c2600181b0ba8391d/mailnews/base/prefs/content/accountcreation/createInBackend.js#150
  if (!accountManager.defaultAccount)
    accountManager.defaultAccount = account;

Today (for RSS):
https://searchfox.org/comm-central/source/mail/components/accountcreation/content/createInBackend.js#176
  if (inServer.canBeDefaultServer && (!MailServices.accounts.defaultAccount ||
                                      !MailServices.accounts.defaultAccount
                                       .incomingServer.canBeDefaultServer))
    MailServices.accounts.defaultAccount = account;

This is working fine. Even the call to verifyLocalFoldersAccount(), which creates Local Folders, comes *after* this. So, I'm a little baffled about this bug. But then again, maybe there's another condition that ensure that the account creation dialog doesn't run into this bug. I don't know.

In any case, I agree that defaultAccount should not throw and just return null.

Comment 20

[:aceman]

There are still several cases in the function where we return NS_ERROR_FAILURE (e.g. at https://dxr.mozilla.org/comm-central/source/comm/mailnews/base/src/nsMsgAccountManager.cpp#738) and that shows as an exception in JS. If there are no accounts yet, it will throw. It may just be that we removed most places asking for the default account before accounts are set up, or we just wrapped them inside try {} catch{}.

(In reply to Ben Bucksch (:BenB) from comment #19)
> https://searchfox.org/comm-central/rev/
> 257141c7d72ce7959639e94c2600181b0ba8391d/mailnews/base/prefs/content/
> accountcreation/createInBackend.js#150
>   if (!accountManager.defaultAccount)
>     accountManager.defaultAccount = account;


There is var account = accountManager.createAccount(); just above this so it seems there is at least one account and this one is returned as the default (however unusable it may be).

> https://searchfox.org/comm-central/source/mail/components/accountcreation/
> content/createInBackend.js#176
>   if (inServer.canBeDefaultServer && (!MailServices.accounts.defaultAccount
> ||
>                                       !MailServices.accounts.defaultAccount
>                                        .incomingServer.canBeDefaultServer))
>     MailServices.accounts.defaultAccount = account;

Looks the same:
let account = MailServices.accounts.createAccount(); on line 173 above this.

Comment 21

[Ben Bucksch (:BenB)]

Ah, I see. defaultAccount returns null, if there are accounts, but no default account, but throws, when there are no accounts at all. Got it.

Yes, that should be fixed.

Comment 22

[:aceman]

(In reply to Ben Bucksch (:BenB) from comment #21)
> Ah, I see. defaultAccount returns null, if there are accounts, but no

No, it never returns null without throwing. If at least one account exists, it returns that one, even if it has the property CanBeDefaultAccount set to false so it is unusable as default account (e.g. a Chat account).

> default account, but throws, when there are no accounts at all. Got it.

Yes, it throws only if there are no accounts all. So in the account setup after you created a new account, then you can query defaultAccount and that one will be returned.

So possibly that quoted code is a no-op, as account will now be returned as the default account automatically, if there was none:
if (!accountManager.defaultAccount)
    accountManager.defaultAccount = account;


> Yes, that should be fixed.

Yes, I can look at it again.

Comment 23

[Ben Bucksch (:BenB)]

> If at least one account exists, it returns that one

Are you saying that the code cited in comment 19 is a no-op? !accountManager.defaultAccount will never be true?
Then we really need to fix this bug here for that code to work :).

Comment 24

[:aceman]

(In reply to Ben Bucksch (:BenB) from comment #23)
> Are you saying that the code cited in comment 19 is a no-op?
> !accountManager.defaultAccount will never be true?

Yes, that is what I said, in the current state.

I'm working on the patch here. It may also be that there is no need for such a code as a if defaultAccount is null, a new usable account will be picked up automatically (even if it is the currently created one).

Comment 25

[:aceman]

Attachment: 342632.patch

So this would be the backend patch that changes the logic and returns null if there is no usable account (e.g. accounts without valid servers or that have canBeDefaultServer = false are rejected) and does not throw on that.
It also has the necessary changes at the C++ callers to check also the returned value.

Comment 26

[:aceman]

Attachment: 342632-mailnews.patch

Comment 27

[:aceman]

Attachment: 342632-TB.patch

Comment 28

[:aceman]

Attachment: 342632-AC.patch

It shouldn't be needed to set the default account to a newly created account as it will be done automatically if defaultAccount is null till now.

Comment 29

[:aceman]

Attachment: 342632-cal.patch

Comment 30

[:aceman]

Attachment: 342632-SM.patch

Comment 31

[:aceman]

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&selectedJob=208197583&revision=7854d42c82b87a3c4fd94407314a058dd2ff74f7

Comment 32

[:aceman]

Attachment: 342632.patch v1.1

Becky import needs a small tweak for its tests which do not define any account outside Local Folders, but expect to get a default account to store filters to. Previously the Local Folders were returned, now nothing. So let's allow it to choose Local folders explicitly as filters can be stored there. This could be useful also outside of tests as a user-visible feature.

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&selectedJob=208214986&revision=8dca8c5b99bc4a973f60e00fedfec63eaa2d4172

Comment 33

[Ben Bucksch (:BenB)]

Patch looks good to me.

> MsgSendlater::GetIdentityFromKey()

Did you verify that the caller(s) can deal with null for identity being returned?

Comment 34

[Ben Bucksch (:BenB)]

Comment on attachment 9020595 [details] [diff] [review]
342632.patch v1.1

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

Patch looks good to me.

> MsgSendlater::GetIdentityFromKey()

Did you verify that the caller(s) can deal with null for identity being returned?

Comment 35

[Ben Bucksch (:BenB)]

My reviews are assuming that you tested this carefully, all possible situations.

Comment 36

[:aceman]

I went by the assumption and code reading, that most callers of the defaultAccount that also fetched an identity already had to cope with e.g. Local folders being returned as default, that has no identities. So they already had code to cope (e.g. the often just pick the first identity globally). See the 342632-TB.patch for the JS callers.

GetIdentityFromKey() also returned account->GetDefaultIdentity() on potential Local folders account (when default), which with 0 identities returned null and NS_OK. So I now return the same for null default account. So I could assume the callers must cope with a null returned identity. If not, it is a pre-existing bug that is not made worse now.

It seems there are 2 callers in nsMsgSendLater.cpp and they do not check if identity is not null, they just proceed. If you want I can make them bail out in that case. It is possible we wouldn't even get to the callers with a sendable message if there would be no account with identities. But I can add the checks for safety.

Comment 37

[Ben Bucksch (:BenB)]

> If you want I can...

I don't want any specific solution, just to make sure the code doesn't break with this change. If you verified that by reading code and manual testing, that's fine with me.

Comment 38

[Frank-Rainer Grahl (:frg)]

Comment on attachment 9020577 [details] [diff] [review]
342632-SM.patch

aceman in /suite/mailnews/compose/MsgComposeCommands.js

> +      identities = defaultAccount.identities;
>     if (identities.length == 0)

Does this work when identities was never assigned?
Shouldn't it be
 if (!identities || identities.length == 0)

The TB patch has different code there and seems to be ok. Remaining parts looks good. I would like to wait with r+ till the TB patch got r+. Can't test it currently. We are broken here.

Comment 39

[:aceman]

(In reply to Frank-Rainer Grahl (:frg) from comment #38)
> Does this work when identities was never assigned?
> Shouldn't it be
>  if (!identities || identities.length == 0)

Yes, thanks.

Comment 40

[:aceman]

Attachment: 342632-SM.patch v1.1

Comment 41

[:aceman]

Attachment: 342632.patch v1.2

Added the checks if GetIdentityFromKey() is null.

Comment 42

[Magnus Melin [:mkmelin]]

Comment on attachment 9020573 [details] [diff] [review]
342632-mailnews.patch

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

::: mailnews/base/prefs/content/AccountWizard.js
@@ +54,5 @@
>  var gCurrentAccount;
>  
> +// The default account before we create a new account.
> +// We need to store this as just asking for the default account may switch
> +// it to the newly created one if there was none before.

well that's unexpected. can we avoid such side effects?

Comment 43

[Magnus Melin [:mkmelin]]

Comment on attachment 9020574 [details] [diff] [review]
342632-TB.patch

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

::: mail/base/content/folderPane.js
@@ +2748,5 @@
>     * @param aParent - the folder to run the search terms on
>     */
>    newVirtualFolder: function ftc_newVFolder(aName, aSearchTerms, aParent) {
>      let folder = aParent || gFolderTreeView.getSelectedFolders()[0];
> +    if (!folder) {

you could just tag || GetDefaultAccountRootFolder() onto the previous line

::: mail/base/content/mailWindowOverlay.js
@@ +2093,1 @@
>      destinationFolder = preselectedFolder;

agreed, I'd just remove this

@@ +2744,5 @@
>  }
>  
>  function GetDefaultAccountRootFolder()
>  {
> +  var account = accountManager.defaultAccount;

make it "let" while you're at it

Comment 44

[Magnus Melin [:mkmelin]]

Comment on attachment 9022311 [details] [diff] [review]
342632.patch v1.2

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

::: mailnews/base/src/nsMsgAccountManager.cpp
@@ +759,5 @@
> +          // If this can serve as default server, set it as default and
> +          // break out of the loop.
> +          if (canBeDefaultServer) {
> +            SetDefaultAccount(account);
> +            break;

I'd prefer if this kind of functionality isn't in a getter (esentially)

Comment 45

[:aceman]

(In reply to Magnus Melin [:mkmelin] from comment #42)
> ::: mailnews/base/prefs/content/AccountWizard.js
> > +// The default account before we create a new account.
> > +// We need to store this as just asking for the default account may switch
> > +// it to the newly created one if there was none before.
> 
> well that's unexpected. can we avoid such side effects?

This is not new, I just documented the existing behaviour.

(In reply to Magnus Melin [:mkmelin] from comment #44)
> ::: mailnews/base/src/nsMsgAccountManager.cpp
> @@ +759,5 @@
> > +          // If this can serve as default server, set it as default and
> > +          // break out of the loop.
> > +          if (canBeDefaultServer) {
> > +            SetDefaultAccount(account);
> > +            break;
> 
> I'd prefer if this kind of functionality isn't in a getter (esentially)

This is not new, we always picked a new default account in this 'getter' if there was none. I just tighten the conditions for which account can be picked.

So how would you propose all this to work?
Also take bug 880602 into account. We also had bug reports where users were stuck on some unusable account (like Local Folders) being the default account, even though they already created a usable account (POP or IMAP), but TB hasn't switched to that.

Would you rather we redetermine default account only on account removal or addition? When adding, would you mind switching to a better one automatically? Or does that need to be done manually (code that explicitly sets new default?).
Also do not forget the case when after TB restart a server may become invalid (if it was created by a disabled account) so we may want to automatically switch default account again.

Comment 46

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #45)
> (In reply to Magnus Melin [:mkmelin] from comment #42)
> > ::: mailnews/base/prefs/content/AccountWizard.js
> > > +// The default account before we create a new account.
> > > +// We need to store this as just asking for the default account may switch
> > > +// it to the newly created one if there was none before.
> > 
> > well that's unexpected. can we avoid such side effects?
> 
> This is not new, I just documented the existing behaviour.
> 
> (In reply to Magnus Melin [:mkmelin] from comment #44)
> > ::: mailnews/base/src/nsMsgAccountManager.cpp
> > @@ +759,5 @@
> > > +          // If this can serve as default server, set it as default and
> > > +          // break out of the loop.
> > > +          if (canBeDefaultServer) {
> > > +            SetDefaultAccount(account);
> > > +            break;
> > 
> > I'd prefer if this kind of functionality isn't in a getter (esentially)
> 
> This is not new, we always picked a new default account in this 'getter' if
> there was none. I just tighten the conditions for which account can be
> picked.
> 
> So how would you propose all this to work?
> Also take bug 880602 into account. We also had bug reports where users were
> stuck on some unusable account (like Local Folders) being the default
> account, even though they already created a usable account (POP or IMAP),
> but TB hasn't switched to that.

Yes such things would happen exact due to someone not realising get default account would have a side effect :/

> Would you rather we redetermine default account only on account removal or
> addition? When adding, would you mind switching to a better one
> automatically? 

Yes at removal and addition we should check. 
I don't think there's a need to switch to a better one, as we'd never have the situation of a "bad" one. Setting default account should ensure it's a suitable account if it doesn't already.

> Also do not forget the case when after TB restart a server may become
> invalid (if it was created by a disabled account) so we may want to
> automatically switch default account again.

I don't understand what you mean by this "created by a disabled account".

Comment 47

[:aceman]

(In reply to Magnus Melin [:mkmelin] from comment #46)
> > > I'd prefer if this kind of functionality isn't in a getter (esentially)
> > 
> > This is not new, we always picked a new default account in this 'getter' if
> > there was none. I just tighten the conditions for which account can be
> > picked.
> > 
> > So how would you propose all this to work?
> > Also take bug 880602 into account. We also had bug reports where users were
> > stuck on some unusable account (like Local Folders) being the default
> > account, even though they already created a usable account (POP or IMAP),
> > but TB hasn't switched to that.
> 
> Yes such things would happen exact due to someone not realising get default
> account would have a side effect :/

This is not users fault, but that default account stored a bad account permanently. We now try to make TB fix itself automatically.
 
> > Would you rather we redetermine default account only on account removal or
> > addition? When adding, would you mind switching to a better one
> > automatically? 
> 
> Yes at removal and addition we should check. 

OK, so I can split that part back into bug 880602.

> I don't think there's a need to switch to a better one, as we'd never have
> the situation of a "bad" one.

OK, so in this bug we only fix this part, never store a "bad" account as default, but return null.

> Setting default account should ensure it's a
> suitable account if it doesn't already.

Currently there is no check when setting default account. We could add that.

> > Also do not forget the case when after TB restart a server may become
> > invalid (if it was created by a disabled account) so we may want to
> > automatically switch default account again.
> 
> I don't understand what you mean by this "created by a disabled account".

Sorry, I meant disabled addon. So what if we store a default server that is valid and becomes invalid after restart (disabling the addon)? We now check that the default account must have a valid/working server. If we determine that at startup, should we return null, or switch to another account?

Comment 48

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #47)
> Sorry, I meant disabled addon. So what if we store a default server that is
> valid and becomes invalid after restart (disabling the addon)? We now check
> that the default account must have a valid/working server. If we determine
> that at startup, should we return null, or switch to another account?

I think both, return null and then explicitly switch to another if needed.

There's probably other thing we should also do in that case (regarding informing the user), where an account ceases to exist due to support for the type being removed/disabled.

Comment 49

[Ben Bucksch (:BenB)]

Magnus: I had the same thoughts that you did - I thought that a getter should not "fix up" and save the preferences permanently.

However, then I thought that
a) the existing code already does that, so we can presume that's fine, unless we have evidence to the contrary
b) it does kind-of make sense to centralize the handling of "default account" here in this code. The alternative would be that all code that creates a (potentially default) account will have to handle this and set the default account, if there is none already, yada yada, so I thought it's maybe a good idea to keep this all confined here.

So, while I originally thought the same thing as Magnus, I kind-of started to think that this patch makes sense and is a good approach.

I think both approaches are legit. I would suggest to keep the changes here minimal. We can always change the place where the default account is being set later in another bug. But this bug here has a different purpose: To ensure that defaultAccount never returns null and callers are adapted to that.

Comment 50

[Frank-Rainer Grahl (:frg)]

Comment on attachment 9021173 [details] [diff] [review]
342632-SM.patch v1.1

Thanks and r+

Comment 51

[:aceman]

(In reply to Ben Bucksch (:BenB) from comment #49)
> set later in another bug. But this bug here has a different purpose: To
> ensure that defaultAccount never returns null and callers are adapted to
> that.

No, it is the other way round. Up to now, it never returned null, but now it can and callers need to adapt.

Comment 52

[:aceman]

Attachment: 342632-core.patch

OK, I have reworked this as you wanted, the getter does not change the default account internally, it only reads the pref.

The default account is only chosen anew when the current account is removed.

The choosing of a new default account if there is none yet and a usable account is created will be done in bug 880602.

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

Comment 53

[:aceman]

Attachment: 342632-mailnewsJS.patch

The suggested check of validity of the passed in new default account in SetDefaultAccount() caused quite some test failures because many of them were setting the default account to Local folders or other unusable account (IMAPpump.js), even though a proper account was available. So I had to fix those here.

Comment 54

[:aceman]

Attachment: 342632-mailnews.patch

Comment 55

[Magnus Melin [:mkmelin]]

Comment on attachment 9025931 [details] [diff] [review]
342632-core.patch

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

::: mailnews/base/src/nsMsgAccountManager.cpp
@@ +721,5 @@
>    return m_prefs->SetCharPref(PREF_MAIL_ACCOUNTMANAGER_ACCOUNTS,
>                                mAccountKeyList);
>  }
>  
> +/* Get the default account. If no default account, return null. */

nit: since you're touching this, and it is documentation. Use /** */
/* */ is just uncommented

@@ +764,5 @@
> +  return rv;
> +}
> +
> +/**
> + * Pick the first account that can be default.

... and make it the default.

@@ +767,5 @@
> +/**
> + * Pick the first account that can be default.
> + */
> +nsresult
> +nsMsgAccountManager::FindDefaultAccount()

naming is a bit odd, perhaps AutosetDefaultAccount?

Comment 56

[Magnus Melin [:mkmelin]]

Comment on attachment 9025933 [details] [diff] [review]
342632-mailnews.patch

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

LGTM, r=mkmelin

Comment 57

[:aceman]

Attachment: 342632-core.patch v1.1

Thanks, fixed the nits.

Comment 58

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/38c2cc49dccc
Allow defaultAccount to return success with nullptr result when there is no usable account. r=mkmelin
https://hg.mozilla.org/comm-central/rev/a6f1f63789af
Adapt callers to defaultAccount being possibly null - mailnews/. r=mkmelin
https://hg.mozilla.org/comm-central/rev/df8923d94a7a
Adapt callers to defaultAccount being possibly null - mailnews JS. r=mkmelin
https://hg.mozilla.org/comm-central/rev/b5de626e716d
Adapt callers to defaultAccount being possibly null - calendar/. r=Fallen
https://hg.mozilla.org/comm-central/rev/d7cafc35543e
Adapt callers to defaultAccount being possibly null - mail/. r=mkmelin
https://hg.mozilla.org/comm-central/rev/6b3a824d8c82
Adapt callers to defaultAccount being possibly null - Seamonkey. r=frg

Comment 59

[:aceman]

The change in account creation (342632-AC.patch) was intentionally not landed as if the default account is currently null, no new one will be chosen even if one is created. So the explicit setting of it is needed.
I move that patch to bug 880602 where the automatic choosing should be implemented.