Closed
Bug 690761
Opened 13 years ago
Closed 13 years ago
Switch nsIAbManager::convertQueryStringToExpression aQueryString parameter to nsAUTF8String
Categories
(Thunderbird :: Address Book, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 10.0
People
(Reporter: mconley, Assigned: mconley)
Details
Attachments
(1 file, 2 obsolete files)
5.46 KB,
patch
|
standard8
:
review+
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
nsIAbManager::convertQueryStringToExpression parameter "aQueryString" is currently typed as a string. We should be using nsAUTF8String instead, to support non-ASCII search queries from Javascript.
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → mconley
Assignee | ||
Comment 1•13 years ago
|
||
Try builds coming in here: http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=f18d55416b56
Assignee | ||
Updated•13 years ago
|
Attachment #563746 -
Flags: review?(mbanner)
Comment 2•13 years ago
|
||
Comment on attachment 563746 [details] [diff] [review] Patch v1 >- nsIAbBooleanExpression convertQueryStringToExpression(in string aQueryString); >+ nsIAbBooleanExpression convertQueryStringToExpression(in AUTF8String aQueryString); We need to bump the uuid here so that binary extensions can detect the differences. >- return nsAbQueryStringToExpression::Convert(aQueryString, _retval); >+ return nsAbQueryStringToExpression::Convert(aQueryString.BeginReading(), .get() is better and more commonly used. Alternately (and maybe even better) would be to change nsAbQueryStringToExpression::Convert to take a const nsCString & parameter and avoid the nsCString -> char * -> nsCString conversion that we seem to have everywhere we use it.
Attachment #563746 -
Flags: review?(mbanner) → review-
Assignee | ||
Comment 3•13 years ago
|
||
Thanks for the review! Totally forgot about bumping the UUID - done for nsIAbManager. I've also updated nsAbQueryStringToExpression::Convert to take a const &nsACString. Try builds coming in here: http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=884121c6c4cb
Attachment #563746 -
Attachment is obsolete: true
Assignee | ||
Comment 4•13 years ago
|
||
And I also forgot the special implementations on the other two platforms. A quick grepping shows that this should cover all uses of nsAbQueryStringToExpression::Convert. Try builds coming in here: http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=516579eb43f5
Attachment #565240 -
Attachment is obsolete: true
Comment 5•13 years ago
|
||
Mike any reasons for not requesting review ?
Assignee | ||
Comment 6•13 years ago
|
||
Ah, whoops - this one slipped off my radar. Thanks, Ludo!
Assignee | ||
Updated•13 years ago
|
Attachment #565335 -
Flags: review?(mbanner)
Comment 7•13 years ago
|
||
Comment on attachment 565335 [details] [diff] [review] Patch v3 Review of attachment 565335 [details] [diff] [review]: ----------------------------------------------------------------- r=me, this also needs sr. ::: mailnews/addrbook/src/nsAbQueryStringToExpression.cpp @@ +60,3 @@ > > nsCOMPtr<nsISupports> s; > + rv = ParseExpression (&queryChars, getter_AddRefs(s)); nit: As you're here, no space between ParseExpression and the open bracket here.
Attachment #565335 -
Flags: superreview?(dbienvenu)
Attachment #565335 -
Flags: review?(mbanner)
Attachment #565335 -
Flags: review+
Updated•13 years ago
|
Attachment #565335 -
Flags: superreview?(dbienvenu) → superreview+
Assignee | ||
Comment 8•13 years ago
|
||
Thanks, committed as http://hg.mozilla.org/comm-central/rev/d4943eb34ac0
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•13 years ago
|
Target Milestone: --- → Thunderbird 10.0
Comment 9•13 years ago
|
||
Comment on attachment 565335 [details] [diff] [review] Patch v3 >- return nsAbQueryStringToExpression::Convert(aQueryString, _retval); >+ return nsAbQueryStringToExpression::Convert(aQueryString, >+ _retval); [This change not strictly necessary!] .get() wouldn't have worked, because aQueryString is an nsACString. (Had you been unable to fix nsAbQueryStringToExpression, you could have mucked about with PromiseFlatCString.)
You need to log in
before you can comment on or make changes to this bug.
Description
•