Closed Bug 475359 Opened 15 years ago Closed 15 years ago

ASSERTION: space only keyword: 'keywords.IsEmpty() || keywords.CharAt(0) != ' ''

Categories

(MailNews Core :: Backend, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: neil, Assigned: neil)

Details

Attachments

(1 file, 1 obsolete file)

Steps to reproduce problem:
1. Tag a message [keywords now "tag1"]
2. Add another tag [keywords now "tag1 tag2"]
3. Remove the second tag [keywords now "tag1 " note the extra space]
4. Add a tag again [keywords now "tag1  tag2"]
5. Remove the first tag [keywords now " tag2"]
The following conditions are required to trigger the problem:
1. There must be at least one tag present at all times
2. The first tag being removed must be the last tag to have been added
3. The second tag being removed must be the first tag to have been added
Attached patch Proposed patch (obsolete) — Splinter Review
The first two hunks simply remove an unused variable and hoist some code out of the loop. (I guess that if you had a lot of messages then repeatedly splitting the string would waste some time.) The third hunk does the actual work; rather than deleting spaces after the keyword I decided to delete spaces before the keyword, since that was more likely to fix existing profiles. However this just reverses the problem: now deleting the first keyword will leave a leading space, so I leave a check in to delete the space after the keyword but only if it is the first one in the string. Finally, another alleged optimisation crept in, in that if no keywords were removed then there's no point setting the header. Please let me know if I've overlooked something while doing these optimisations!
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #358866 - Flags: superreview?(bienvenu)
Attachment #358866 - Flags: review?(bienvenu)
As per bug 476150, I should have used nsTArray<nsCString>.
Attachment #358866 - Attachment is obsolete: true
Attachment #359747 - Flags: superreview?(bienvenu)
Attachment #359747 - Flags: review?(bienvenu)
Attachment #358866 - Flags: superreview?(bienvenu)
Attachment #358866 - Flags: review?(bienvenu)
Comment on attachment 359747 [details] [diff] [review]
nsTArray<nsCString> patch

looks good, thx. A couple nits:

K&R braces style has snuck in to a mostly non K&R file/function :-) 

And while you're at it, can you add a space before &length here?:

+        if (MsgFindKeyword(keywordArray[j], keywords, &startOffset,&length))
Attachment #359747 - Flags: superreview?(bienvenu)
Attachment #359747 - Flags: superreview+
Attachment #359747 - Flags: review?(bienvenu)
Attachment #359747 - Flags: review+
Pushed changeset 2a5433caac6e to comm-central.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.