Open
Bug 880602
Opened 12 years ago
Updated 2 years ago
if the current default account is not suitable to be default, pick a better default server as soon as it becomes available
Categories
(MailNews Core :: Account Manager, defect)
MailNews Core
Account Manager
Tracking
(Not tracked)
ASSIGNED
People
(Reporter: aceman, Assigned: aceman)
References
(Blocks 1 open bug)
Details
Attachments
(3 files, 9 obsolete files)
|
(deleted),
patch
|
mkmelin
:
review+
BenB
:
review-
|
Details | Diff | Splinter Review |
|
(deleted),
patch
|
BenB
:
review+
|
Details | Diff | Splinter Review |
|
(deleted),
patch
|
aceman
:
review+
|
Details | Diff | Splinter Review |
Currently, if there is no good account to become the default server, nsMsgAccountManager::GetDefaultAccount chooses any first account to be the default and sticks to it later, even if a better account becomes available (is created) later. This attributes to problems like bug 854098.
I propose to change it so that this happens:
1. if only Local Account exists, it is reported as default.
2. if a rss/news account is created, still Local Accounts is the default (the first account)
3. if a pop3/imap4 account is created now TB will automatically switch to it as the default account.
4. if any new pop3/imap4 accounts are created after that, the account from step 3. is still the default.
Bug 880464 and bug 437139 is fixing this in the account wizard level.
I propose to also add some logic for this inside nsMsgAccountManager::GetDefaultAccount so in case this is not caught in the first line of defense (account wizard) we fix the default account later (or of a better account is created bypassing the wizard).
|
||
Comment 1•12 years ago
|
||
aceman, can we improve bug summary to something like intelligently/rationally set default server?
Sure, probably clicked too fast :)
Summary: if the current default server → if the current default account is not suitable to be default, pick a better default server as soon as it becomes available
> 2. if a rss/news account is created, still Local Accounts is the default
This should be covered by bug 880464. Once bug 342632 is fixed, a better value would be "null" rather than Local Folders to indicate that no suitable default account is available.
> 3. if a pop3/imap4 account is created now TB will automatically switch to it
> as the default account.
Covered in bug 880464 as well, after moving that hunk over from bug 437139.
> 4. if any new pop3/imap4 accounts are created after that, the account from
> step 3. is still the default.
I thought this is by design? If you have a suitable default account defined and create a new mail account, the current account should stay the default unless you explicitly flip it.
Depends on: 880464
(In reply to rsx11m from comment #3)
> > 4. if any new pop3/imap4 accounts are created after that, the account from
> > step 3. is still the default.
> I thought this is by design?
Eh, stupid me - you aren't proposing to change that, just reiterating the desired behavior which also happens to be the current behavior... :-)
Thus, the main point to consider here is to ensure that Local Folders is picked when no default account is available and no account has canBeDefaultServer set, or to find the first(?) server which has canBeDefaultServer set and make it the new default server.
(Quoting Florian Quèze from bug 739258 comment #2)
> (In reply to :aceman from bug 739258 comment #1)
>
> > Instead of picking the one at '0', what about iterating over the accounts
> > and using the first that has canBeDefaultServer = true?
>
> It's what the 20 or so lines above the code I've quoted do, see
> http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgAccountManager.cpp#756
So, that part seems to be covered as well already.
I said already in the description that bug 880464 also fixes this in AW :) I try to add another line of defense here for cases where somehow the user (or extension) bypasses AW.
|
||
Comment 8•12 years ago
|
||
Is this bug for "defaultaccount setting upon account creation" only.
No plan to cover "defaultaccount setting upon or after account deletion(and modification too)"?
Yes exactly, this is ALSO about account deletion. I plan to check for a better account any time the Default account is queried.
|
Assignee | |
Comment 10•11 years ago
|
||
So this is my idea. I am not sure if the NS_ADDREF is needed, but I think all tests passed for me.
The change of m_defaultAccount to defaultAccount in nsMsgAccountManager::GetSortOrder fixes a crash after I tried to create a newsserver as the first account in TB. Before the patch m_defaultAccount and GetDefaultAccount would return the same value so it didn't matter. But now they differ. The change seems to be the right thing to do. The function probably originally wanted to use defaultAccount otherwise it wouldn't call GetDefaultAccount and used m_defaultAccount immediately. It also checks defaultAccount for !null, not m_defaultAccount, so that seems inconsistent.
Attachment #8425792 -
Flags: review?(neil)
|
||
Comment 11•11 years ago
|
||
Comment on attachment 8425792 [details] [diff] [review]
patch
>- if (m_defaultAccount.get() == aAccount)
>+ if (m_defaultAccount && (m_defaultAccount.get() == aAccount))
This change makes no sense at all.
>- SetDefaultAccount(firstAccount);
>+ // In this case we intentionally do not store the account
>+ // into m_defaultAccount so that GetDefaultAccount re-evaluates
>+ // the accounts each time it is called in the hope the user
>+ // sets up a better account soon.
>+ NS_ADDREF(*aDefaultAccount = firstAccount);
>+ return NS_OK;
Sorry, I don't think this is the right approach, because this could make GetDefaultAccount return different accounts without sending out notifications as the "default" account changes. I suppose it would be OK for GetDefaultAccount to look for a server that can be default if the current default account's server can't be default though.
Attachment #8425792 -
Flags: review?(neil) → review-
|
|
Assignee | |
Comment 12•11 years ago
|
||
(In reply to neil@parkwaycc.co.uk from comment #11)
> >- if (m_defaultAccount.get() == aAccount)
> >+ if (m_defaultAccount && (m_defaultAccount.get() == aAccount))
> This change makes no sense at all.
Prevents dereferencing null as m_defaultAccount now can be null. Just a safety measure, no specific observed crash yet.
> >- SetDefaultAccount(firstAccount);
> >+ // In this case we intentionally do not store the account
> >+ // into m_defaultAccount so that GetDefaultAccount re-evaluates
> >+ // the accounts each time it is called in the hope the user
> >+ // sets up a better account soon.
> >+ NS_ADDREF(*aDefaultAccount = firstAccount);
> >+ return NS_OK;
> Sorry, I don't think this is the right approach, because this could make
> GetDefaultAccount return different accounts without sending out
> notifications as the "default" account changes. I suppose it would be OK for
> GetDefaultAccount to look for a server that can be default if the current
> default account's server can't be default though.
So I can make it send the notifications (do the parts of SetDefaultServer that are relevant). Would that be enough?
Flags: needinfo?(neil)
|
|
||
Comment 13•11 years ago
|
||
(In reply to aceman from comment #12)
> (In reply to comment #11)
> > >- if (m_defaultAccount.get() == aAccount)
> > >+ if (m_defaultAccount && (m_defaultAccount.get() == aAccount))
> > This change makes no sense at all.
> Prevents dereferencing null as m_defaultAccount now can be null. Just a
> safety measure, no specific observed crash yet.
m_defaultAccount can be assigned to null, but that just changes the value of .get() to return null. (And in fact the .get() is mostly useless as the equality operator does a .get() automatically.)
> I can make it send the notifications (do the parts of SetDefaultServer
> that are relevant). Would that be enough?
How would you know what the previously returned default server was?
Flags: needinfo?(neil)
|
|
Assignee | |
Comment 14•11 years ago
|
||
(In reply to neil@parkwaycc.co.uk from comment #13)
> > I can make it send the notifications (do the parts of SetDefaultServer
> > that are relevant). Would that be enough?
> How would you know what the previously returned default server was?
So may I have a new variable?
|
|
||
Comment 15•11 years ago
|
||
(In reply to aceman from comment #14)
> (In reply to comment #13)
> > > I can make it send the notifications (do the parts of SetDefaultServer
> > > that are relevant). Would that be enough?
> > How would you know what the previously returned default server was?
> So may I have a new variable?
Two variables to hold the real and virtual default server would be silly, but a flag of some sort would be OK I guess, it would save you from having to query the current default server to see if it was unsuitable each time.
|
|
Assignee | |
Comment 16•11 years ago
|
||
OK, let's try this.
Attachment #8425792 -
Attachment is obsolete: true
Attachment #8429467 -
Flags: review?(neil)
|
|
||
Comment 17•11 years ago
|
||
Comment on attachment 8429467 [details] [diff] [review]
patch v2
>+ if (m_defaultAccountTemporary) {
...
>+ if (NS_FAILED(rv) || !m_defaultAccount || m_defaultAccountTemporary) {
Won't m_defaultAccountTemporary still be true here anyway?
> if (!m_defaultAccount) {
> // Absolutely no usable account found. Error out.
> NS_ERROR("Default account is null, when not expected!");
>+ SetDefaultAccount(nullptr);
(Why is this needed?)
>+ m_defaultAccountTemporary = !aDefaultAccount;
(This is for the deleting default account case?)
|
|
Assignee | |
Comment 18•11 years ago
|
||
(In reply to neil@parkwaycc.co.uk from comment #17)
> > if (!m_defaultAccount) {
> > // Absolutely no usable account found. Error out.
> > NS_ERROR("Default account is null, when not expected!");
> >+ SetDefaultAccount(nullptr);
> (Why is this needed?)
So that m_defaultAccountTemporary is set inside SetDefaultAccount(nullptr) without open-coding it here :)
> >+ m_defaultAccountTemporary = !aDefaultAccount;
> (This is for the deleting default account case?)
Too, but also for the SetDefaultAccount(nullptr) calls I added.
|
|
||
Comment 19•11 years ago
|
||
(In reply to aceman from comment #18)
> (In reply to comment #17)
> > > if (!m_defaultAccount) {
> > > // Absolutely no usable account found. Error out.
> > > NS_ERROR("Default account is null, when not expected!");
> > >+ SetDefaultAccount(nullptr);
> > (Why is this needed?)
> So that m_defaultAccountTemporary is set inside SetDefaultAccount(nullptr)
> without open-coding it here :)
But how did m_defaultAccount become null in the first place?
|
|
Assignee | |
Comment 20•10 years ago
|
||
Another try, now with a test.
Attachment #8429467 -
Attachment is obsolete: true
Attachment #8429467 -
Flags: review?(neil)
Attachment #8496470 -
Flags: review?(neil)
|
|
||
Comment 21•10 years ago
|
||
Comment on attachment 8496470 [details] [diff] [review]
patch v3
So, this doesn't look as if it will quite work when all the accounts are unsuitable, but the preference is set to one anyway.
The first time you call GetDefaultAccount it will notice the unsuitable account set as default, and try to find a better account.
The second time you call GetDefaultAccount it will notice the current account is unsuitable, and fall back to the first account.
>+ if (!firstAccount)
firstAccount is always null here.
>+ SetDefaultAccount(m_defaultAccount);
This looks confusing, as it's not clear you're just clearing m_defaultAccountTemporary.
|
|
Assignee | |
Comment 22•10 years ago
|
||
Attachment #8496470 -
Attachment is obsolete: true
Attachment #8496470 -
Flags: review?(neil)
Attachment #8503649 -
Flags: review?(neil)
|
|
Assignee | |
Comment 23•10 years ago
|
||
(In reply to neil@parkwaycc.co.uk from comment #21)
> Comment on attachment 8496470 [details] [diff] [review]
> patch v3
>
> So, this doesn't look as if it will quite work when all the accounts are
> unsuitable, but the preference is set to one anyway.
Yes, I set the pref so that we save the account for next start and keep it until a better one is found. If we didn't save it some other "first" account could take over next time.
I hope I solved the other found problems, thanks.
|
|
||
Comment 24•10 years ago
|
||
Comment on attachment 8503649 [details] [diff] [review]
patch v4
> uint32_t count = m_accounts.Length();
> if (!count) {
>- *aDefaultAccount = nullptr;
>+ NS_WARNING("No valid default account found. No accounts are set up.");
>+ SetDefaultAccount(*aDefaultAccount = nullptr);
[Not sure how you can have a default account that you need to clear when there are no accounts.]
>+ nsCOMPtr<nsIMsgAccount> firstAccount;
>+
>+ if (!m_defaultAccount) {
...
>+ } else {
>+ // The current default account is only temporary.
>+ // If we find no better one later on, keep this one to avoid random
>+ // default account changes.
>+ firstAccount = m_defaultAccount;
>+ }
The logic is very confusing here. Better would be
// We prefer to have a suitable account as the default account.
// But if we don't have a suitable account, prefer to keep the existing unsuitable account as the temporary default account.
nsCOMPtr<nsIMsgAccount> firstAccount = m_defaultAccount;
if (!firstAccount) {
...
}
>+ nsCString defaultKey;
>+ rv = m_prefs->GetCharPref(PREF_MAIL_ACCOUNTMANAGER_DEFAULTACCOUNT, getter_Copies(defaultKey));
>+ if (NS_SUCCEEDED(rv)) {
>+ rv = GetAccount(defaultKey, getter_AddRefs(m_defaultAccount));
>+ if (NS_SUCCEEDED(rv)) {
>+ nsCOMPtr <nsIMsgIncomingServer> server;
>+ // server could be null if created by an unloaded extension
>+ (void) m_defaultAccount->GetIncomingServer(getter_AddRefs(server));
>+
>+ bool canBeDefaultServer = false;
>+ if (server)
>+ {
>+ (void) server->GetCanBeDefaultServer(&canBeDefaultServer);
>+ // If we find no better default account later, use this one.
>+ firstAccount = m_defaultAccount;
>+ }
>+ if (!canBeDefaultServer)
>+ m_defaultAccount = nullptr;
>+ }
>+ }
>+ if (m_defaultAccount)
>+ m_defaultAccountTemporary = false;
And this is confusing too; if the preferred account can be default, why not just call SetDefaultAccount and call it a day?
|
|
Assignee | |
Comment 25•10 years ago
|
||
Maybe this is a bit better?
Attachment #8503649 -
Attachment is obsolete: true
Attachment #8503649 -
Flags: review?(neil)
Attachment #8530949 -
Flags: review?(neil)
|
|
Assignee | |
Comment 26•10 years ago
|
||
Neil: ping :)
|
|
||
Comment 27•10 years ago
|
||
Comment on attachment 8530949 [details] [diff] [review]
patch v5
>+ rv = GetAccount(defaultKey, getter_AddRefs(m_defaultAccount));
This should be firstAccount. If it can be the default it will become m_defaultAccount when you call SetDefaultAccount.
>+ bool canBeDefaultServer = false;
>+ if (server) {
>+ (void) server->GetCanBeDefaultServer(&canBeDefaultServer);
>+ // If we find no better default account later, use this one.
>+ firstAccount = m_defaultAccount;
>+ }
>+ if (!canBeDefaultServer)
>+ m_defaultAccount = nullptr;
>+ else
>+ SetDefaultAccount(m_defaultAccount);
You can then move this code inside the if (server) block.
>+ m_defaultAccount = nullptr;
[Should you set m_defaultAccountTemporary here?]
>+ bool m_defaultAccountTemporary;
Please put this near other bools otherwise it will waste alignment space.
|
|
||
Comment 28•10 years ago
|
||
Comment on attachment 8530949 [details] [diff] [review]
patch v5
>+ // We prefer to have a suitable account as the default account.
>+ // But if we don't have a suitable account, prefer to keep the existing
>+ // unsuitable account as the temporary default account.
>+ nsCOMPtr<nsIMsgAccount> firstAccount = m_defaultAccount;
Also I think it might be possible to move this nearer its use.
|
||
Comment 29•10 years ago
|
||
Is there any reason this patch died?
|
|
Assignee | |
Comment 30•10 years ago
|
||
It was just postponed as I did not want to push it into TB38 so late. I can resume finishing it now.
|
|
Assignee | |
Comment 31•10 years ago
|
||
(In reply to neil@parkwaycc.co.uk from comment #27)
> Comment on attachment 8530949 [details] [diff] [review]
> patch v5
> >+ m_defaultAccount = nullptr;
> [Should you set m_defaultAccountTemporary here?]
I think it is still at "true" so that is what it should stay at here. The other branch sets it to false in SetDefaultAccount().
Aryx, please push to try server, all tests.
Attachment #8530949 -
Attachment is obsolete: true
Attachment #8530949 -
Flags: review?(neil)
Flags: needinfo?(archaeopteryx)
Attachment #8598210 -
Flags: review?(neil)
|
|
||
Comment 32•10 years ago
|
||
Flags: needinfo?(archaeopteryx)
|
|
||
Comment 33•10 years ago
|
||
Pushed again because previous build failed due to m-c changes: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=060ae869c9f0
|
|
||
Comment 34•9 years ago
|
||
Is this worth having someone test a try build?
|
|
Assignee | |
Comment 35•9 years ago
|
||
I think not. There is a test outlining the steps to check. And the test does it so I don't think manual testing is needed.
But it could use a repush to try server as the previous run had many unrelated failures. Hopefully today the trunk is more green.
Flags: needinfo?(archaeopteryx)
|
|
||
Comment 36•9 years ago
|
||
Flags: needinfo?(aryx.bugmail)
|
|
Assignee | |
Comment 37•9 years ago
|
||
Interestingly only the Windows debug builds did crash in my test. I found the test pointed 2 accounts to the same server object. It seems it crashed when setting up the storage for one of the accounts. The accounts were of different types (IMAP and POP) so there could be some clash. And Windows does not like if open files are deleted.
Aryx please push the new version.
Attachment #8598210 -
Attachment is obsolete: true
Attachment #8598210 -
Flags: review?(neil)
Flags: needinfo?(aryx.bugmail)
|
|
||
Comment 38•9 years ago
|
||
New Try push: https://hg.mozilla.org/try-comm-central/rev/ccbf0af72f01
Flags: needinfo?(aryx.bugmail)
|
|
||
Comment 39•8 years ago
|
||
(In reply to Sebastian H. [:aryx][:archaeopteryx] from comment #38)
> New Try push: https://hg.mozilla.org/try-comm-central/rev/ccbf0af72f01
next step ?
|
|
Assignee | |
Comment 40•6 years ago
|
||
If bug 342632 is implemented first, that quite simplifies the problem and there is actually no further change needed to GetDefaultAccount(). There is no longer an unusable account returned as default that we would need to track (as some temporary) and replace with a better one once a new one is available.
So I just add the test here that should prove the feature.
Try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&selectedJob=208213396&revision=8dca8c5b99bc4a973f60e00fedfec63eaa2d4172
Attachment #8658861 -
Attachment is obsolete: true
Attachment #9020581 -
Flags: review?(neil)
Attachment #9020581 -
Flags: feedback?(ben.bucksch)
|
||
Comment 41•6 years ago
|
||
Comment on attachment 9020581 [details] [diff] [review]
880602-test.patch
Review of attachment 9020581 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks for the test.
Attachment #9020581 -
Flags: feedback?(ben.bucksch) → feedback+
|
|
||
Comment 42•6 years ago
|
||
And yes, the approach makes sense to me.
No longer blocks: null_default_server
Depends on: null_default_server
|
|
Assignee | |
Comment 43•6 years ago
|
||
So this will now pick a new default account if there is none yet and a new account is created. But it must wait only after incomingServer is set to check if it is usable.
The check in LoadAccounts() is for the case we have a default stored in the prefs at startup but it is not usable (e.g. due to unloaded addon). Then m_defaultAccount stays at nullptr and we will automatically choose another default.
Try run with all the parts:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&selectedJob=212721397&revision=7fbe2ab075cf4acb8d3db0f982ac8bd398f2b50f
Well, there could be a case where the account is OK a and we suddenly set its server to an unusable type. I still need to cover that case.
Attachment #9026284 -
Flags: feedback?(mkmelin+mozilla)
Attachment #9026284 -
Flags: feedback?(ben.bucksch)
|
|
Assignee | |
Comment 44•6 years ago
|
||
The account creation hunk again, where we do not need to set the default account explicitly as it will be set automatically in the SetIncomingServer().
Attachment #9026285 -
Flags: review?(ben.bucksch)
|
|
Assignee | |
Comment 45•6 years ago
|
||
The test again.
Attachment #9020581 -
Attachment is obsolete: true
Attachment #9020581 -
Flags: review?(neil)
Attachment #9026286 -
Flags: review?(ben.bucksch)
|
|
||
Comment 46•6 years ago
|
||
Comment on attachment 9026285 [details] [diff] [review]
880602-AC.patch
I like the idea that you're setting the default acccount in SetIncomingServer()
Attachment #9026285 -
Flags: review?(ben.bucksch) → review+
|
|
||
Comment 47•6 years ago
|
||
Comment on attachment 9026286 [details] [diff] [review]
880602-test.patch
Review of attachment 9026286 [details] [diff] [review]:
-----------------------------------------------------------------
This is a really nice and useful unit test. I like the cases you implemented, and the story you told (the comments), with the explanations.
How tests should look like! Good work!
::: mailnews/base/test/unit/test_accountDefault.js
@@ +24,5 @@
> + let nntpAccount = MailServices.accounts.createAccount();
> + nntpAccount.incomingServer = nntpServer;
> + nntpAccount.addIdentity(identity);
> +
> + Assert.equal(nntpAccount.key, "account1");
This is not a useful check. It if the key was "nttpAccountForFred", it's not a bug.
Same below.
(The length check makes sense, because it ensures that the account was created.)
@@ +54,5 @@
> +
> + Assert.equal(MailServices.accounts.accounts.length, 3);
> + Assert.equal(imapAccount.key, "account3");
> + // Now IMAP is much better and can be default. We switch to it automatically.
> + Assert.equal(MailServices.accounts.defaultAccount, imapAccount);
Good
@@ +70,5 @@
> + popAccount.addIdentity(identity);
> +
> + Assert.equal(MailServices.accounts.accounts.length, 4);
> + Assert.equal(popAccount.key, "account4");
> + // POP3 is no better if we already have IMAP so IMAP is still default.
spelling NIT: comma before "so"
// POP3 is no better than the IMAP account that we already have, so IMAP is still the default.
same below: another ", so"
ditto below: comma before "but", twice
Attachment #9026286 -
Flags: review?(ben.bucksch) → review+
|
|
||
Comment 48•6 years ago
|
||
(r+ of course under the condition that my 2 small remarks are fixed, everywhere in the file.)
|
|
Assignee | |
Comment 49•6 years ago
|
||
(In reply to Ben Bucksch (:BenB) from comment #47)
> This is a really nice and useful unit test. I like the cases you
> implemented, and the story you told (the comments), with the explanations.
>
> How tests should look like! Good work!
Thanks :)
> ::: mailnews/base/test/unit/test_accountDefault.js
> @@ +24,5 @@
> > + let nntpAccount = MailServices.accounts.createAccount();
> > + nntpAccount.incomingServer = nntpServer;
> > + nntpAccount.addIdentity(identity);
> > +
> > + Assert.equal(nntpAccount.key, "account1");
>
> This is not a useful check. It if the key was "nttpAccountForFred", it's not
> a bug.
We made a fix in the past (bug 485839) to make the account keys unique for the lifetime of the profile. If suddenly the keys started to appear like nttpAccountForFred, which may not be unique, we would like to detect it. This may not be the right test file to do it, so I can remove this. It seems we have a test for this behaviour in test_acctRepair.js.
|
||
Comment 51•6 years ago
|
||
(In reply to Ben Bucksch (:BenB) from comment #47)
> spelling NIT: comma before "so"
> ditto below: comma before "but", twice
Check https://www.grammarly.com/blog/comma-before-so/. Yes, this sentence should have one:
POP3 is no better than the IMAP account that we already have, so IMAP is still the default.
I looked at the cases below, all should have commas as Ben requested.
|
|
Assignee | |
Comment 52•6 years ago
|
||
Thanks.
Attachment #9026286 -
Attachment is obsolete: true
Attachment #9029136 -
Flags: review+
Attachment #9026284 -
Flags: review?(mkmelin+mozilla)
|
||
Comment 53•6 years ago
|
||
Comment on attachment 9026284 [details] [diff] [review]
880602.patch
Review of attachment 9026284 [details] [diff] [review]:
-----------------------------------------------------------------
LGTM, r=mkmelin
Attachment #9026284 -
Flags: review?(mkmelin+mozilla)
Attachment #9026284 -
Flags: review+
Attachment #9026284 -
Flags: feedback?(mkmelin+mozilla)
|
|
Assignee | |
Comment 54•6 years ago
|
||
Ben, do you want to check the main patch (the feedback request)?
Flags: needinfo?(ben.bucksch)
|
|
||
Comment 55•6 years ago
|
||
aceman,
I didn't comment on this, because I don't feel well about this particular patch. There are a number of changes in there that to me looks somewhat dangerous in the sense that they might break something, but I cannot pinpoint an exact situation.
Also, I do not understand why you make these changes in the first place, what the concept behind it is.
What remains is the feeling that this patch might break stuff, but it remains a feeling and nothing factual. I don't want to be resisting stuff just because I don't understand it, so I stayed silent.
Concretely, the parts that came up as red flags for me:
- // Nothing usable was found.
- return SetDefaultAccountInternal(nullptr);
Why would you do that? You can't do anything meaningful, so why not leave things as they are? Apparently, they must be working, so why touch the working status quo (of a certain default account in the profile) and "fix" it with a null account? It worked so far, so you're not really improving much, but you could break stuff, if your algo goes wrong or you overlooked a situation.
- if (m_defaultAccount.get() == aAccount)
- AutosetDefaultAccount();
- if (m_defaultAccount.get() == aAccount) {
- rv = AutosetDefaultAccount();
- NS_ENSURE_SUCCESS(rv, rv);
Why this ENSURE_SUCCESS being added? You're in Remove account? Are you sure that you want to prevent the removal of the account in case this was the default and that was the last suitable account?
If you are sure that you want to prevent it, make sure you a) leave things in a working state and b) that you inform the user why he cannot remove this account. You're not doing either of those. Specifically, it seems that the user would end up with a half-removed account and a broken setup. That's why I think this particular change is wrong.
(Personally, I'd prefer to keep code style changes and logic changes in separate patches. I find these irrelevant changes distracting.)
- You're calling GetDefaultAccount() and if there's no default account autoset default account on every load. Do we need that? Is that the core of what you're trying to achieve here, that we ensure on startup that we set a proper default account?
If that's your goal, please describe that clearly, why you think we should do that, which specific situation this improves, and limit the patch only to this one single change.
I think you could ignore my comment 4. and 1. if you have a good reason, 3. is style and I think 2. is important to fix.
I'm going to give r- for point 2. specifically, because it really concerns me.
Sorry for being so negative about the patch. I didn't want to be negative and wasn't sure, so I stayed silent, but since you insisted on my comment...
Flags: needinfo?(ben.bucksch)
|
|
||
Comment 56•6 years ago
|
||
Attachment #9026284 -
Flags: feedback?(ben.bucksch) → review-
|
|
||
Updated•5 years ago
|
|
|
||
Updated•4 years ago
|
|
|
||
Updated•4 years ago
|
|
||
Updated•2 years ago
|
Severity: minor → S4
You need to log in
before you can comment on or make changes to this bug.
Description
•