On the attached test case, if you try to add the second email address listed, it will get added but the star won't change (it will stay empty), reloading the message will show the star full. Same applies when you remove the contact. The first address works properly. I think it's a recent regression - (less than 2 week), Gary can you hunt it ?

In the threepane window.

At the moment, this blocks - the star doesn't get updated and the address can get added to the address book multiple times. I see it with the test email in a standalone message window. I've not reproduced it on all email combinations, but I have reproduced it on some.

Jim, any chance this is a regression from one of your recent changes?

I don't think I've had any recent check-ins. I can confirm this on the latest Windows nightly, though. Perhaps more interesting: I tried this with TB 3.0.4 and I get a similar, but slightly different error. Adding the various people in the To: header works, but adding Ludovic (who's CC'ed) exhibits this same problem, provided you haven't clicked "more" on the To: list.

Created an attachment (id=443315) [details] > email with which I can recreate the issue. > > On the attached test case, if you try to add the second email address listed, > it will get added but the star won't change (it will stay empty), reloading the > message will show the star full. Same applies when you remove the contact. The > first address works properly. If by "second email address", you mean Phil Lacy, I can't reproduce that. If, however, by "second email address" you mean yourself, I can reproduce that. In that case, I see this in the error console: Error: hideEmailAddressPopup is not defined Source File: chrome://messenger/content/messageWindow.xul Line: 1 which suggests that this is actually a DUP of bug 563615. > I think it's a recent regression - (less than 2 week), Gary can you hunt it ? Which is interesting, since Jim's comment in bug 563615 suggests that this was at least theoretically introduced in 2009. Perhaps the bug was introduced then, but something else hid it until now?

(In reply to comment #5) > Created an attachment (id=443315) [details] [details] > > email with which I can recreate the issue. > > > > On the attached test case, if you try to add the second email address listed, > > it will get added but the star won't change (it will stay empty), reloading the > > message will show the star full. Same applies when you remove the contact. The > > first address works properly. > > If by "second email address", you mean Phil Lacy, I can't reproduce that. I only saw this when I loaded it up in the three-pane. > If, however, by "second email address" you mean yourself, I can reproduce that. > In that case, I see this in the error console: > > Error: hideEmailAddressPopup is not defined > Source File: chrome://messenger/content/messageWindow.xul > Line: 1 > > which suggests that this is actually a DUP of bug 563615. > > > I think it's a recent regression - (less than 2 week), Gary can you hunt it ? > > Which is interesting, since Jim's comment in bug 563615 suggests that this was > at least theoretically introduced in 2009. Perhaps the bug was introduced > then, but something else hid it until now? I'm pretty sure this is a different error. Bug 563615 *should* only be causing an issue where, once you mouse over an address in the standalone window, that address never gets un-highlighted (until you close the window).

I meant second email as being phil-lacy's.

I'm looking at this now, and it appears that the DOM contains several instances of the email-address node for Phil Lacy (ditto for Nikolay). The first of Phil's nodes is getting updated properly, but that one is hidden, so it has no practical effect. There also appears to be a problem with Nikolay's node since it doesn't have a cardDetails attribute.

This patch is pretty minimalist; it fixes the problem (as far as I can tell) in the easiest way I could manage. There might be some performance regressions but I think "right and slow" is better than "wrong and fast". :) Basically here's what got fixed: * Counting the number of cached address nodes in the header field didn't account for trailing commas (present when "more" is there). * Only one cached address node was used. The others were hidden and then more were appended at the end, causing chaos.

Ooooh, good catch, Jim! I bet this is actually my regression. We do already have some performance problems in that code that block 3.1, so I suspect this needs to be combined with the work I'll be doing in bug 560695.

The "right" way to fix this is probably to change the logic from: for each cached node: update node for each remaining address: create new node to for each address: if cached node exists: update node else: create new node

I'm looking at implementing the method I described in comment 11, and one thing that's especially bothersome with the current code is that sometimes there's a trailing comma node and sometimes there isn't. I see two ways around this: 1) make the comma part of the "more" indicator 2) always add a trailing comma, but hide it when there is no "more" I'm going to go with #2 for now.

Since it sounds like this is going to get incorporated into a larger patch (bug 560695), I'm going to try and avoid writing tests of my own for this, but this lightly-tested version appears to work and is pretty close to what the original meant to do, I think. It also has a net reduction in code, which is usually a good thing.

The main reason it was looking like it would make sense to merge those two patches together was because I was fearing having to make non-trivial surgery on the performance patch that have weird interactions with this patch. Sorry to disappoint, but the performance fix turned out to be relatively straightforward, and it is compatible with the most recent patch here, so I think it probably makes more sense to continue to drive them separately. Note that after applying this patch, the other one needs to be manually rebased before it will apply (and vice versa), but that's purely syntactic patch-level interface; there's no semantic-level interference between the two patches.

s/interface/interference/

Ok, here's a patch with tests. The updated test-message-header.js fails without the patch in mailWidgets.xml and passes with it. dmose, I flagged you for review since you've already looked at this area recently, but no worries if you forward it onto someone else.

Comment on attachment 444447 [details] [diff] [review] Patch + MozMill tests Nicely done! I like the state this leaves the code in, and a fine choice to get rid of some old unused code. r+ conditional on two changes: * Depending on there already being an address that doesn't have a card in the address book seems fragile, and I'm concerned it could lead to random tinderbox oranges if some other unrelated part of this file changes. Instead of traversing the list looking for an address without a card, maybe just take the first address and delete any existing card? * test_more_widget is already a bit too long, so I'd rather see the new code pulled out into another function in the interest of readability rather than just inserted into the existing function. Maybe subtest_more_widget_star_click() or something. I'd like to say that I really appreciate that you worked through the fix and test for this bug on such short notice, and since I'm reviewing at the last minute, I'm happy to go ahead and make those changes myself if you don't have time. I can't do it tonight, but I'll happily do it tomorrow and land before code freeze.

Ok, I pulled the test_more_widget code out into three subtests: * subtest_more_widget_display * subtest_more_widget_click * subtest_more_widget_star_click I also added a helper function to ensure that no card exists. I'll eventually be pulling that out of this file (bug 474721 wants a function like that for testing too), but I'm just putting it in here for now for the sake of simplicity.

Looks great; thanks, Jim! Merged to the tip of the 1.9.2 branch and updated a comment.

Landed: http://hg.mozilla.org/comm-central/rev/95cbf01b138b http://hg.mozilla.org/releases/comm-1.9.2/rev/91b530f8d342

I made a mistake and only checked the test in on branch, not trunk. Adding in-testsuite? so this fact doesn't get lost. I've added it to my calendar to land tomorrow.

Comment on attachment 445484 [details] [diff] [review] patch v.next, trunk version Turns out I _actually_ landed the right patch, with tests, on both trunk and branch, but uploaded the wrong thing here which then confused me after the fact. Good times!