Closed Bug 690761 Opened 13 years ago Closed 13 years ago

Switch nsIAbManager::convertQueryStringToExpression aQueryString parameter to nsAUTF8String

Categories

(Thunderbird :: Address Book, defect)

x86
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 10.0

People

(Reporter: mconley, Assigned: mconley)

Details

Attachments

(1 file, 2 obsolete files)

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
Attachment #563746 - Flags: review?(mbanner)
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-
Attached patch Patch v2 (obsolete) — Splinter Review
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
Attached patch Patch v3Splinter Review
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!
Attachment #565335 - Flags: review?(mbanner)
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+
Attachment #565335 - Flags: superreview?(dbienvenu) → superreview+
Thanks, committed as http://hg.mozilla.org/comm-central/rev/d4943eb34ac0
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 10.0
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.