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)
MailNews Core
Backend
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: neil, Assigned: neil)
Details
Attachments
(1 file, 1 obsolete file)
3.08 KB,
patch
|
Bienvenu
:
review+
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•15 years ago
|
||
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)
Assignee | ||
Comment 2•15 years ago
|
||
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 3•15 years ago
|
||
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+
Assignee | ||
Comment 4•15 years ago
|
||
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.
Description
•