Switch nsIAbManager::convertQueryStringToExpression aQueryString parameter to nsAUTF8String

RESOLVED FIXED in Thunderbird 10.0


Address Book
7 years ago
6 years ago


(Reporter: mconley, Assigned: mconley)


Thunderbird 10.0

Firefox Tracking Flags

(Not tracked)



(1 attachment, 2 obsolete attachments)

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: nobody → mconley
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-
Created attachment 565240 [details] [diff] [review]
Patch v2

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
Created attachment 565335 [details] [diff] [review]
Patch v3

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
Mike any reasons for not requesting review ?
Ah, whoops - this one slipped off my radar.  Thanks, Ludo!
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+


6 years ago
Attachment #565335 - Flags: superreview?(dbienvenu) → superreview+
Thanks, committed as http://hg.mozilla.org/comm-central/rev/d4943eb34ac0
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 10.0

Comment 9

6 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.