Last Comment Bug 747362 - No account selected when opening the account settings dialog while the inbox of the unified view is selected
: No account selected when opening the account settings dialog while the inbox ...
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Account Manager (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 14.0
Assigned To: Florian Quèze [:florian] [:flo] (PTO until August 29th)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-20 06:20 PDT by Florian Quèze [:florian] [:flo] (PTO until August 29th)
Modified: 2012-04-23 07:36 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch (1.17 KB, patch)
2012-04-20 06:20 PDT, Florian Quèze [:florian] [:flo] (PTO until August 29th)
no flags Details | Diff | Splinter Review
Patch v2 (1.21 KB, patch)
2012-04-20 06:54 PDT, Florian Quèze [:florian] [:flo] (PTO until August 29th)
mozilla: review+
Details | Diff | Splinter Review

Description Florian Quèze [:florian] [:flo] (PTO until August 29th) 2012-04-20 06:20:37 PDT
Created attachment 616952 [details] [diff] [review]
Patch

I see this in the error console when this happens:
JavaScript error: chrome://messenger/content/AccountManager.js, line 176: childrenNode.childNodes[i]._account is undefined


I was afraid this was caused by some of my changes for bug 742238 so I investigated. The problem is that when the 'Inbox' of the unified view is selected, the code opening the Account Settings… dialog wants to select a "Unified folders" account that isn't in the list, so the code looking for this account iterates through the whole list of accounts and fails on the "Outgoing Server (SMTP)" tree item that doesn't have an _account property.

The attached patch prevents the code from failing on tree items that don't have the _account property and to fall back to the first account when the requested account isn't in the list.
Comment 1 :aceman 2012-04-20 06:34:19 PDT
I can confirm the error. I like the patch.
But as you "document.getElementById("account-tree-children")" twice in the common path, wouldn't it be better to:
var childrenNode = document.getElementById("account-tree-children");
var accountNode = childrenNode.firstChild;

(I have seen cases where even "document.getElementById("account-tree-children") is null" (when playing with edge cases in bug 723277 comment 44), but this patch does not make that worse, I think. I'll look at that in other bug.)
Comment 2 Florian Quèze [:florian] [:flo] (PTO until August 29th) 2012-04-20 06:54:39 PDT
Created attachment 616962 [details] [diff] [review]
Patch v2

Updated patch taking into account the suggestion from comment 1.

(In reply to :aceman from comment #1)

> (I have seen cases where even
> "document.getElementById("account-tree-children") is null" (when playing
> with edge cases in bug 723277 comment 44), but this patch does not make that
> worse, I think.

It seems you meant bug 713277 comment 44.
Comment 3 :aceman 2012-04-20 07:00:24 PDT
(In reply to Florian Quèze from comment #2)
> It seems you meant bug 713277 comment 44.
Yes, sorry.
Comment 4 David :Bienvenu 2012-04-20 12:57:10 PDT
Comment on attachment 616962 [details] [diff] [review]
Patch v2

looks reasonable, though I'd prefer let for the local vars, thx!
Comment 5 Florian Quèze [:florian] [:flo] (PTO until August 29th) 2012-04-23 07:36:47 PDT
http://hg.mozilla.org/comm-central/rev/9bb214321f4c

Note You need to log in before you can comment on or make changes to this bug.