374862 - Port xpfe/components/search/src to frozen linkage

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[:Mook]

Firefox-sans-Places-on-xulrunner is broken (bugzilla comment 21).  The fix would be to move the component to frozen linkage, and that mostly means changing Suite-only stuff (since Firefox will be going to Places).

Will attach a patch in a second.  It depends on the patch in bug 372050 though.

(... Is this the right component? I can't find something that would make more sense)

Comment 1

[:Mook]

Attachment: patch to move xpfe/components/search/src to frozen linkage

I don't yet know who to ask for review, if anybody is interested :)

Comment 2

[:Mook]

Comment on attachment 259273 [details] [diff] [review]
patch to move xpfe/components/search/src to frozen linkage

I missed some details on how to use Find on strings, the patch is buggy.

Comment 3

[:Mook]

Attachment: Fix usage of nsACString::Find

updated patch

Comment 4

[:Mook]

Comment on attachment 259428 [details] [diff] [review]
Fix usage of nsACString::Find

I /think/ RFindInReadable and nsString_FindCharInSet are correct; the other changes are pretty mechanical.

Comment 5

[[e-mail]]

Comment on attachment 259428 [details] [diff] [review]
Fix usage of nsACString::Find

I don't suppose you could split this up into two patches, one of mechanical stuff which still uses the internal API, and when I'm happy with that then one of changes necessary to switch to using the external API? Hopefully that will also help me find more issues than the ones I struggled to spot below ;-)

>-          updateURL.AssignWithConversion(updateUni);
>+          updateURL.Assign(NS_LossyConvertUTF16toASCII(updateUni));
This should use LossyCopyUTF16toASCII

>-      if ((uriUni) && (!nsCRT::strncmp(uriUni,
>-               NS_ConvertASCIItoUTF16(kURINC_SearchCategoryEngineBasenamePrefix).get(),
>-        (int)sizeof(kURINC_SearchCategoryEngineBasenamePrefix) - 1)))
>+      if ((uriUni) && (!NS_strcmp(uriUni,
>+               NS_ConvertASCIItoUTF16(kURINC_SearchCategoryEngineBasenamePrefix).get())))
strcmp != strncmp (although you got it right the other time).

>   nsAutoString    uri;
>-  uri.AssignWithConversion(uriUni);
>+  uri.Assign(NS_ConvertASCIItoUTF16(uriUni));
This should be NS_ConvertASCIItoUTF16 uri(uriUni);

>   if ((isSearchCategoryURI(source)) && (categoryDataSource))
>   {
>     const char  *uri = nsnull;
>     source->GetValueConst(&uri);
>     if (!uri) return(NS_ERROR_UNEXPECTED);
>     nsAutoString  catURI;
>-    catURI.AssignWithConversion(uri);
>+    catURI.Assign(NS_ConvertASCIItoUTF16(uri));
> 
>     nsCOMPtr<nsIRDFResource>  category;
>     nsCAutoString     caturiC;
>-    caturiC.AssignWithConversion(catURI);
>+    caturiC.Assign(NS_LossyConvertUTF16toASCII(catURI));
>     if (NS_FAILED(rv = mRDFService->GetResource(caturiC,
>       getter_AddRefs(category))))
>       return(rv);
> 
>     rv = categoryDataSource->GetTarget(category, property, tv, target);
>     return(rv);
Now, the second time this pattern appears you succesfully optimised out the conversion to 16-bit and back. But it would be nice if you could optimise the get of a resource with the same URI as an existing resource ;-)

>+    categoryStr.Assign(NS_ConvertASCIItoUTF16(kURINC_SearchCategoryPrefix));
AssignLiteral works on const char[]

Comment 6

[:Mook]

Attachment: purely mechanical parts (Truncate->SetLength, Recycle-NS_Free)

This is just the purely mechanical parts - Truncate(int) -> SetLength(int), and Recycle() -> NS_Free.  It's nearly rubber-stamp level :p

The strings stuff actually requires some thinking; will attach that patch once this one gets accepted.  After that is the external API stuff.

(The strings one will still be internal API except all instances of AssignWithConversion replaced since that's where the majority of the change that needs thinking about is)

Comment 7

[:Mook]

Comment on attachment 259985 [details] [diff] [review]
purely mechanical parts (Truncate->SetLength, Recycle-NS_Free)

bah, forgot to ask for sr :/  Should I be asking somebody else instead?

(err, I'm guessing I do need sr in xpfe code)

Comment 8

[:Mook]

Attachment: Remove AssignWithConversion usage

This does just the AssignWithConversion stuff.  Diffed against after applying the last patch.  Sorry the diff metadata isn't right, so the bugzilla patch viewer isn't going to work as well...

Notes:

I checked for where most of the strings are coming from; many of the cases are actually UTF-8 instead of ASCII (i.e. the old code was incorrect).  Most of these are things like nsIRDFResource::GetValueConst().  I wasn't able to track everything though.

>-          updateURL.AssignWithConversion(updateUni);
>+          CopyUTF16toUTF8(nsDependentString(updateUni), updateURL);
The nsDependentString is because the external version doesn't have a version with PRUnichar* as the first param, and I didn't want to have to edit this line again in the next patch :)

nsInternetSearchService.h
>-  char *    getSearchURI(nsIRDFResource *src);
>+  PRUnichar * getSearchURI(nsIRDFResource *src);
Changed the signature because it was internally doing a UTF8 to UTF16 conversion, and most callers later did the reverse.  There's one caller that doesn't, but that's because it calls nsIBookmarksService::IsBookmarked on it.

Comment 9

[:Mook]

For the external API patch - is there a PRUnichar version of strncmp lying around somewhere that I just can't find?  (nsCRT isn't nsCRTGlue, sadly)

I was hoping that strcmp() with one of the two args being a constant (i.e. not going to overflow) should be okay...

Comment 10

[Mark Banner (:standard8)]

(In reply to comment #6)
> Created an attachment (id=259985) [details]
> purely mechanical parts (Truncate->SetLength, Recycle-NS_Free)

I just checked this in:

Checking in xpfe/components/search/src/nsInternetSearchService.cpp;
/cvsroot/mozilla/xpfe/components/search/src/nsInternetSearchService.cpp,v  <--  nsInternetSearchService.cpp
new revision: 1.253; previous revision: 1.252
done
Checking in xpfe/components/search/src/nsLocalSearchService.cpp;
/cvsroot/mozilla/xpfe/components/search/src/nsLocalSearchService.cpp,v  <--  nsLocalSearchService.cpp
new revision: 1.49; previous revision: 1.48

Comment 11

[[e-mail]]

Comment on attachment 260053 [details] [diff] [review]
Remove AssignWithConversion usage

>     nsCOMPtr<nsIRDFResource> aCategoryRes (do_QueryInterface(aCategoryNode));
>     if (!aCategoryRes)  break;
>     const char      *catResURI = nsnull;
>     aCategoryRes->GetValueConst(&catResURI);
>     if (!catResURI)   break;
>     nsAutoString    categoryStr;
>-    categoryStr.AssignWithConversion(kURINC_SearchCategoryPrefix);
>-    categoryStr.AppendWithConversion(catResURI);
>+    categoryStr.AssignLiteral(kURINC_SearchCategoryPrefix);
>+    categoryStr.Append(NS_ConvertUTF8toUTF16(catResURI));
> 
>     nsCOMPtr<nsIRDFResource>  searchCategoryRes;
>     if (NS_FAILED(rv = mRDFService->GetUnicodeResource(categoryStr,
>       getter_AddRefs(searchCategoryRes))))  break;
Is it me or is searchCategoryRes a duplicate of aCategoryRes?


>+          NS_ConvertUTF16toUTF8 url(uri);
>+          NS_Free(uri);
>+          rv = bookmarks->IsBookmarked(url.get(), &isBookmarkedFlag);
I think I would prefer NS_ConvertUTF16toUTF8(uri).get(); and finding some other way to free uri (or maybe make getSearchURI take an nsString&)

>           nsAutoString  uriString(uriUni);
This ought to be an nsDependentString, if you still keep it.

>+  nsString  url;
>+  url.Adopt(uri);
This ought to be nsAdoptingString url(uri), if you still keep it.


>+          NS_LossyConvertUTF16toASCII queryencodingstrC(queryEncodingStr);
>           if (NS_SUCCEEDED(rv = textToSubURI->ConvertAndEscape(queryencodingstrC.get(), uni, &charsetData)) && (charsetData))
I'd be tempted to inline the NS_LossyConvertUTF16toASCII().get()

>+            text.Assign(NS_ConvertASCIItoUTF16(charsetData));
CopyASCIItoUTF16? (not the only place)

>+        engineURI.Assign(NS_ConvertUTF8toUTF16(uri));
CopyUTF8toUTF16? (not the only place)

>+    queryStr.AssignLiteral("?");
>+    queryStr.Append(userVar);
>+    queryStr.AppendLiteral("=");
Consider using PRUnichar('x') instead of "x"

>+      queryStr.AssignLiteral("&");
>+      queryStr.Append(userVar);
>+      queryStr.AppendLiteral("=");
Does queryStr[0] = PRUnichar('&'); work?

>+          escapedSearchText.Assign(NS_LossyConvertUTF16toASCII(searchText));
LossyCopyUTF16toASCII? (not the only place)

>+          NS_LossyConvertUTF16toASCII aCharset(mQueryEncodingStr);
I'd rename, if not inline, this.

>+          if (NS_SUCCEEDED(rv) && (charsetData))
Might as well lose the ()s around charsetData.

>             nsAutoString  postStr;
I think you'd be better off constructing an nsCAutoString directly.

>-  nsAutoString  url;
>-  url.AssignWithConversion(uri);
>+  NS_ConvertUTF8toUTF16 url(uri);
>   nsIRDFLiteral *literal;
>   mRDFService->GetLiteral(url.get(), &literal);
>         *aResult = literal;
>         return NS_OK;
Where to start? Ignoring the return value of GetLiteral? Possible use of uninitialised variable? Once you've inlined url, you should be able to get this down to one line ;-)

>+			NS_ConvertUTF8toUTF16 url(uri);
> 			nsIRDFLiteral	*literal;
> 			gRDFService->GetLiteral(url.get(), &literal);
> 
> 			*target = literal;
> 			return(NS_OK);
Same goes here :-)

>-				        nsAutoString    valueStr;
>-				        valueStr.AssignWithConversion(value);
>+				        NS_ConvertASCIItoUTF16 valueStr(value);
> 				        tokens[loop].value = valueStr;
tokens[loop].value is an nsString, so you can CopyASCIItoUTF16 directly.

>+			NS_ConvertUTF8toUTF16 url(uri);
> 
> 			nsCOMPtr<nsIRDFLiteral> literal;
> 			rv = gRDFService->GetLiteral(url.get(),
>                                                      getter_AddRefs(literal));
Inline url please.

Try asking :bi for sr next time, maybe your requestee match was inconclusive?

Comment 12

[:Mook]

Attachment: updated to comments

(In reply to comment #11)
> (From update of attachment 260053 [details] [diff] [review])
> >     nsCOMPtr<nsIRDFResource> aCategoryRes (do_QueryInterface(aCategoryNode));
> >     if (!aCategoryRes)  break;
> >     const char      *catResURI = nsnull;
> >     aCategoryRes->GetValueConst(&catResURI);
> >     if (!catResURI)   break;
> >     nsAutoString    categoryStr;
> >-    categoryStr.AssignWithConversion(kURINC_SearchCategoryPrefix);
> >-    categoryStr.AppendWithConversion(catResURI);
> >+    categoryStr.AssignLiteral(kURINC_SearchCategoryPrefix);
> >+    categoryStr.Append(NS_ConvertUTF8toUTF16(catResURI));
> > 
> >     nsCOMPtr<nsIRDFResource>  searchCategoryRes;
> >     if (NS_FAILED(rv = mRDFService->GetUnicodeResource(categoryStr,
> >       getter_AddRefs(searchCategoryRes))))  break;
> Is it me or is searchCategoryRes a duplicate of aCategoryRes?
> 
categoryStr is prefixed by kURINC_SearchCategoryPrefix - e.g., if the old resouce was "urn:foo" the new one is "NC:SearchCategory?category=urn:foo".  I think.
> 
> >+          NS_ConvertUTF16toUTF8 url(uri);
> >+          NS_Free(uri);
> >+          rv = bookmarks->IsBookmarked(url.get(), &isBookmarkedFlag);
> I think I would prefer NS_ConvertUTF16toUTF8(uri).get(); and finding some other
> way to free uri (or maybe make getSearchURI take an nsString&)
Made getSearchURI use out var nsString &_retval
> 
> >           nsAutoString  uriString(uriUni);
> This ought to be an nsDependentString, if you still keep it.
(killed)
> 
> >+  nsString  url;
> >+  url.Adopt(uri);
> This ought to be nsAdoptingString url(uri), if you still keep it.
(killed)
> 
> 
> >+          NS_LossyConvertUTF16toASCII queryencodingstrC(queryEncodingStr);
> >           if (NS_SUCCEEDED(rv = textToSubURI->ConvertAndEscape(queryencodingstrC.get(), uni, &charsetData)) && (charsetData))
> I'd be tempted to inline the NS_LossyConvertUTF16toASCII().get()
done, though that param is getting hard to read :)
> 
> >+            text.Assign(NS_ConvertASCIItoUTF16(charsetData));
> CopyASCIItoUTF16? (not the only place)
> 
> >+        engineURI.Assign(NS_ConvertUTF8toUTF16(uri));
> CopyUTF8toUTF16? (not the only place)
fixed, fixed.  (some use dependent strings - external string copies don't take [PRUni]char*)
> 
> >+    queryStr.AssignLiteral("?");
> >+    queryStr.Append(userVar);
> >+    queryStr.AppendLiteral("=");
> Consider using PRUnichar('x') instead of "x"
done
> 
> >+      queryStr.AssignLiteral("&");
> >+      queryStr.Append(userVar);
> >+      queryStr.AppendLiteral("=");
> Does queryStr[0] = PRUnichar('&'); work?
No (it returns a r-value), but Replace(0, 1, '') does
I left it as assign/append because I thought it looked slightly clearer what it was doing, but changing that it fine too.
> 
> >+          escapedSearchText.Assign(NS_LossyConvertUTF16toASCII(searchText));
> LossyCopyUTF16toASCII? (not the only place)
fixed too (also possibly dependent strings)
> 
> >+          NS_LossyConvertUTF16toASCII aCharset(mQueryEncodingStr);
> I'd rename, if not inline, this.
inlined
> 
> >+          if (NS_SUCCEEDED(rv) && (charsetData))
> Might as well lose the ()s around charsetData.
fixed
> 
> >             nsAutoString  postStr;
> I think you'd be better off constructing an nsCAutoString directly.
fixed
> 
> >-  nsAutoString  url;
> >-  url.AssignWithConversion(uri);
> >+  NS_ConvertUTF8toUTF16 url(uri);
> >   nsIRDFLiteral *literal;
> >   mRDFService->GetLiteral(url.get(), &literal);
> >         *aResult = literal;
> >         return NS_OK;
> Where to start? Ignoring the return value of GetLiteral? Possible use of
> uninitialised variable? Once you've inlined url, you should be able to get this
> down to one line ;-)
fixed (and de-tabified) - I can't figure out which variabl might be uninitialized though
> 
> >+			NS_ConvertUTF8toUTF16 url(uri);
> > 			nsIRDFLiteral	*literal;
> > 			gRDFService->GetLiteral(url.get(), &literal);
> > 
> > 			*target = literal;
> > 			return(NS_OK);
> Same goes here :-)
done; the pulse case checked NS_FAILED and always returned NS_OK otherwise, I hope changing it to return the underlying result is okay...

> 
> >-				        nsAutoString    valueStr;
> >-				        valueStr.AssignWithConversion(value);
> >+				        NS_ConvertASCIItoUTF16 valueStr(value);
> > 				        tokens[loop].value = valueStr;
> tokens[loop].value is an nsString, so you can CopyASCIItoUTF16 directly.
fixed
> 
> >+			NS_ConvertUTF8toUTF16 url(uri);
> > 
> > 			nsCOMPtr<nsIRDFLiteral> literal;
> > 			rv = gRDFService->GetLiteral(url.get(),
> >                                                      getter_AddRefs(literal));
> Inline url please.
fixed
(there's some whitespace whackyness near here, it never lines up no matter what the tab width setting is...)
> 
> Try asking :bi for sr next time, maybe your requestee match was inconclusive?
>

Comment 13

[[e-mail]]

(In reply to comment #12)
>categoryStr is prefixed by kURINC_SearchCategoryPrefix
Ah, that explains it, thanks.

Comment 14

[[e-mail]]

Comment on attachment 260312 [details] [diff] [review]
updated to comments

>-  char  *uri = getSearchURI(aResource);
>-  if (!uri) return(NS_ERROR_UNEXPECTED);
>-  nsAutoString  url;
>-  url.AssignWithConversion(uri);
>-  NS_Free(uri);
>+  nsString url;
>+  if (!getSearchURI(aResource, url)) return(NS_ERROR_UNEXPECTED);
Nits: first, url works better as an nsAutoString; second, return should be on its own line (old code was wrong); third, return doesn't need ()s. Similarly for the other callers to getSearchURI.

>         contents[contentsLen] = '\0';
>-        sourceContents.AssignWithConversion(contents, contentsLen);
>+        CopyASCIItoUTF16(nsDependentCString(contents, contentsLen), sourceContents);
My string fu isn't up to knowing whether nsStringAPI.h supports dependent substrings but if it does then you could avoid writing the null byte.

>+		nsIRDFLiteral	**targetLiteral(NS_REINTERPRET_CAST(nsIRDFLiteral**,
>+		                                target));
Oops, I didn't realise that target wasn't an nsIRDFLiteral** this time - unfortunately this is an unsafe cast, and you'll have to go back to your previous patch for this.

Comment 15

[[e-mail]]

Comment on attachment 260053 [details] [diff] [review]
Remove AssignWithConversion usage

>     nsCOMPtr<nsIRDFResource> aCategoryRes (do_QueryInterface(aCategoryNode));
>     if (!aCategoryRes)  break;
>     const char      *catResURI = nsnull;
>     aCategoryRes->GetValueConst(&catResURI);
>     if (!catResURI)   break;
>     nsAutoString    categoryStr;
>-    categoryStr.AssignWithConversion(kURINC_SearchCategoryPrefix);
>-    categoryStr.AppendWithConversion(catResURI);
>+    categoryStr.AssignLiteral(kURINC_SearchCategoryPrefix);
>+    categoryStr.Append(NS_ConvertUTF8toUTF16(catResURI));
> 
>     nsCOMPtr<nsIRDFResource>  searchCategoryRes;
>     if (NS_FAILED(rv = mRDFService->GetUnicodeResource(categoryStr,
>       getter_AddRefs(searchCategoryRes))))  break;
This should use GetResource which takes a UTF-8 string avoiding conversions.

Comment 16

[[e-mail]]

(In reply to comment #14)
>(From update of attachment 260312 [details] [diff] [review])
>>+		nsIRDFLiteral	**targetLiteral(NS_REINTERPRET_CAST(nsIRDFLiteral**,
>>+		                                target));
>Oops, I didn't realise that target wasn't an nsIRDFLiteral** this time -
>unfortunately this is an unsafe cast, and you'll have to go back to your
>previous patch for this.
In fact, VC7 interprets this as a declaration of a function pointer...

Comment 17

[[e-mail]]

Comment on attachment 260312 [details] [diff] [review]
updated to comments

r=me with those nits fixed.

Comment 18

[:Mook]

Attachment: address comments (up to comment 17)

> My string fu isn't up to knowing whether nsStringAPI.h supports dependent
> substrings but if it does then you could avoid writing the null byte.
It does (at least, nsDependentSubstring exists).  It even has a Substring(char*, PRUint32) which nsString.h doesn't :)

Oh, and changed to nsAutoString even though I'll need to change it back for external string API :)

I've also missed a few instances of AppendWithConversion in that patch.

Not sure if I should carry r+ over or not :/

Comment 19

[[e-mail]]

(In reply to comment #18)
> Created an attachment (id=260456) [details]
> address comments (up to comment 17)
> 
> > My string fu isn't up to knowing whether nsStringAPI.h supports dependent
> > substrings but if it does then you could avoid writing the null byte.
> It does (at least, nsDependentSubstring exists).  It even has a
> Substring(char*, PRUint32) which nsString.h doesn't :)
> 
> Oh, and changed to nsAutoString even though I'll need to change it back for
> external string API :)
> 
> I've also missed a few instances of AppendWithConversion in that patch.
> 
> Not sure if I should carry r+ over or not :/
> 

Comment 20

[[e-mail]]

(In reply to comment #18)
>Oh, and changed to nsAutoString even though I'll need to change it back for
>external string API :)
Sigh. That sucks. In that case, feel free to change it back again.

Comment 21

[[e-mail]]

(In reply to comment #20)
>Sigh. That sucks.
But not as much as accidentally submitting a bugzilla comment :-(

Comment 22

[:Mook]

I think I'll be happy with just getting that checked in (and still use nsAutoString), then fix that up in a follow up that does move to the external string API.  That would probably lead to less of the back-and-force reviewing... :)

There's other stuff that still needs to be done before this will build using external API anyway.

Of course, if you really prefer re-reviewing things, I'm happy with that too :)

Comment 23

[[e-mail]]

Comment on attachment 260456 [details] [diff] [review]
address comments (up to comment 17)

>+  mBuffer.Append(NS_ConvertASCIItoUTF16(Substring(buffer, buffer + numBytes)));
>+  section.Append(NS_ConvertASCIItoUTF16(sectionToFind));
Sigh, no AppendASCIItoUTF16 :-(

>+  dsName.Assign(NS_LossyConvertUTF16toASCII(tokens[0].value));
LossyCopyUTF16toASCII?

Comment 24

[[e-mail]]

(In reply to comment #18)
>Oh, and changed to nsAutoString even though I'll need to change it back for
>external string API :)
My nsStringAPI.h has a typedef for it, so what's the problem?

Comment 25

[:Mook]

Attachment: Move to frozen linkage

This does the rest of the work to move it frozen linkage.
I'm still not sure about the @@ -681,32 +697,32 @@ InternetSearchDataSource::isSearchCatego chunk:

> -      if ((uriUni) && (!nsCRT::strncmp(uriUni,
> -               NS_ConvertASCIItoUTF16(kURINC_SearchCategoryEngineBasenamePrefix).get(),
> -        (int)sizeof(kURINC_SearchCategoryEngineBasenamePrefix) - 1)))
> +      if ((uriUni) && (!PL_strncmp(NS_LossyConvertUTF16toASCII(uriUni).get(),
> +               kURINC_SearchCategoryEngineBasenamePrefix,
> +               (int)sizeof(kURINC_SearchCategoryEngineBasenamePrefix) - 1)))
It's a URI, so a lossy convert should be okay here...  (I have to convert because there's no strncmp for PRUnichar strings)  The alternative is to use strcmp instead as I previously had :(

Comment 26

[Christian :Biesinger (don't email me, ping me on IRC)]

no, URIs are not always ASCII. Prefer UTF-8.

But couldn't you just use memcmp?

Comment 27

[Mark Banner (:standard8)]

Comment on attachment 260456 [details] [diff] [review]
address comments (up to comment 17)

I just checked this patch in on behalf of Mook.

Comment 28

[[e-mail]]

Comment on attachment 262915 [details] [diff] [review]
Move to frozen linkage

>+PRInt32 nsString_FindCharInSet(const nsAString& aString, const char *aPattern, PRInt32 aOffset = 0)
>+{
>+  PRInt32 result = -1;
>+  for (; *aPattern; ++aPattern) {
>+    PRInt32 attempt = aString.FindChar(PRUnichar(*aPattern), aOffset);
>+    if (attempt != -1 && (attempt < result || result == -1))
>+    {
>+      result = attempt;
>+    }
>+  }
>+  return result;
> }
This doesn't strike me as particularly efficient... maybe if you limit the find to end at the previous best match?

>+      if ((uriUni) && (!PL_strncmp(NS_LossyConvertUTF16toASCII(uriUni).get(),
>+               kURINC_SearchCategoryEngineBasenamePrefix,
>+               (int)sizeof(kURINC_SearchCategoryEngineBasenamePrefix) - 1)))
I guess this isn't amenable to StringBeginsWith...

>+              offset = 0;
>+              do {
>+                offset = escapedSearchText.Find(NS_LITERAL_CSTRING("%25"), offset);
>+                if (offset != -1)
>+                {
>+                  escapedSearchText.Replace(offset, 3, "+");
>+                }
>+              } while (offset != -1);
>+              offset = 0;
>+              do {
>+                offset = escapedSearchText.Find(NS_LITERAL_CSTRING("%2B25"), offset);
>+                if (offset != -1)
>+                {
>+                  escapedSearchText.Replace(offset, 5, "%25");
>+                }
>+              } while (offset != -1);
I think I'd prefer these written like this:
offset = escapedSearchText.Find(...);
while (offset != -1)
{
  escapedSearchText.Replace(...);
  offset = escapedSearchText.Find(..., offset);
}

>+      if (numericEncoding.Equals(NS_ConvertASCIItoUTF16(encodingList[i].numericEncoding)))
Doesn't Equals work on char*?

>   nsAutoString  iconURL;
>   if (icon)
>   {
>     nsCAutoString iconFileURL;
>     if (NS_FAILED(rv = NS_GetURLSpecFromFile(icon, iconFileURL)))
>       return(rv);
>-    AppendUTF8toUTF16(iconFileURL, iconURL);
>+    iconURL.Append(NS_ConvertUTF8toUTF16(iconFileURL));
>   }
iconURL is empty at this point, so you can CopyUTF8toUTF16 here.

>-    PRInt32 eol = buffer.FindCharInSet("\r\n", 0);
>+    PRInt32 eol = nsString_FindCharInSet(buffer, "\r\n", 0);
>     if (eol < 0)  break;
>     nsAutoString  line;
>     if (eol > 0)
>     {
>-      buffer.Left(line, eol);
>+      line = StringHead(buffer, eol);
>     }

>-    PRInt32 eol = buffer.FindCharInSet("\r\n", 0);
>+    PRInt32 eol = nsString_FindCharInSet(buffer, "\r\n", 0);
>     if (eol < 0)  break;
>     nsAutoString  line;
>     if (eol > 0)
>     {
>-      buffer.Left(line, eol);
>+      line = StringHead(buffer, eol);
>     }
These would be better written as a constructor for line.

>   nsAutoString  serverPathStr;
>   nsCAutoString serverPath;
>   aURL->GetPath(serverPath);
>   if (!serverPath.IsEmpty())
>   {
>+    serverPathStr.Append(NS_ConvertUTF8toUTF16(serverPath));
>+    serverPath.Truncate();
serverPathStr is empty at this point, so you can CopyUTF8toUTF16 here.

>+      NS_ConvertUTF16toUTF8 results(resultItem);
>+      printf("\n----- Search result: '%s'\n\n", results.get());

>+      NS_ConvertUTF16toUTF8 hrefCStr(hrefStr);
>+      printf("HREF: '%s'\n", hrefCStr.get());
Should inline these.

>     nsAutoString  entityStr;
>+    entityStr = Substring(nameStr, ampOffset, semiOffset - ampOffset + 1);
This should be a constructor.

>     // cut out any CRs or LFs
>-    while ((offset = nameStr.FindCharInSet("\n\r", 0)) >= 0)
>+    while ((offset = nsString_FindCharInSet(nameStr, "\n\r", 0)) >= 0)
>     {
>       nameStr.Cut(offset, 1);
>     }
This should use StripChars.

Comment 29

[:Mook]

Attachment: Address comments 26, 28

(In reply to comment #28)
> (From update of attachment 262915 [details] [diff] [review])
> >+PRInt32 nsString_FindCharInSet(const nsAString& aString, const char *aPattern, PRInt32 aOffset = 0)
> >+{
> >+  PRInt32 result = -1;
> >+  for (; *aPattern; ++aPattern) {
> >+    PRInt32 attempt = aString.FindChar(PRUnichar(*aPattern), aOffset);
> >+    if (attempt != -1 && (attempt < result || result == -1))
> >+    {
> >+      result = attempt;
> >+    }
> >+  }
> >+  return result;
> > }
> This doesn't strike me as particularly efficient... maybe if you limit the find
> to end at the previous best match?
Rewrote to do the finding manually (not using nsString::Find(), which doesn't allow limiting the find)
> 
> >+      if ((uriUni) && (!PL_strncmp(NS_LossyConvertUTF16toASCII(uriUni).get(),
> >+               kURINC_SearchCategoryEngineBasenamePrefix,
> >+               (int)sizeof(kURINC_SearchCategoryEngineBasenamePrefix) - 1)))
> I guess this isn't amenable to StringBeginsWith...

Sorry, I realized this method isn't used at all, so  I just killed it.
> 
> >+              offset = 0;
> >+              do {
> >+                offset = escapedSearchText.Find(NS_LITERAL_CSTRING("%25"), offset);
> >+                if (offset != -1)
> >+                {
> >+                  escapedSearchText.Replace(offset, 3, "+");
> >+                }
> >+              } while (offset != -1);
> >+              offset = 0;
> >+              do {
> >+                offset = escapedSearchText.Find(NS_LITERAL_CSTRING("%2B25"), offset);
> >+                if (offset != -1)
> >+                {
> >+                  escapedSearchText.Replace(offset, 5, "%25");
> >+                }
> >+              } while (offset != -1);
> I think I'd prefer these written like this:
> offset = escapedSearchText.Find(...);
> while (offset != -1)
> {
>   escapedSearchText.Replace(...);
>   offset = escapedSearchText.Find(..., offset);
> }
Fixed. (And simliar code near line 1839)
> 
> >+      if (numericEncoding.Equals(NS_ConvertASCIItoUTF16(encodingList[i].numericEncoding)))
> Doesn't Equals work on char*?
No (it takes a char_type* which is a PRUnichar*), but EqualsLiteral does. (Changed)
Also changed the following Assign + convert to AssignLiteral.
> 
> >   nsAutoString  iconURL;
> >   if (icon)
> >   {
> >     nsCAutoString iconFileURL;
> >     if (NS_FAILED(rv = NS_GetURLSpecFromFile(icon, iconFileURL)))
> >       return(rv);
> >-    AppendUTF8toUTF16(iconFileURL, iconURL);
> >+    iconURL.Append(NS_ConvertUTF8toUTF16(iconFileURL));
> >   }
> iconURL is empty at this point, so you can CopyUTF8toUTF16 here.
Fixed.
> 
> >-    PRInt32 eol = buffer.FindCharInSet("\r\n", 0);
> >+    PRInt32 eol = nsString_FindCharInSet(buffer, "\r\n", 0);
> >     if (eol < 0)  break;
> >     nsAutoString  line;
> >     if (eol > 0)
> >     {
> >-      buffer.Left(line, eol);
> >+      line = StringHead(buffer, eol);
> >     }
> 
> >-    PRInt32 eol = buffer.FindCharInSet("\r\n", 0);
> >+    PRInt32 eol = nsString_FindCharInSet(buffer, "\r\n", 0);
> >     if (eol < 0)  break;
> >     nsAutoString  line;
> >     if (eol > 0)
> >     {
> >-      buffer.Left(line, eol);
> >+      line = StringHead(buffer, eol);
> >     }
> These would be better written as a constructor for line.
Fixed.  (There were three, actually, plus similar code around line 4389)
> 
> >   nsAutoString  serverPathStr;
> >   nsCAutoString serverPath;
> >   aURL->GetPath(serverPath);
> >   if (!serverPath.IsEmpty())
> >   {
> >+    serverPathStr.Append(NS_ConvertUTF8toUTF16(serverPath));
> >+    serverPath.Truncate();
> serverPathStr is empty at this point, so you can CopyUTF8toUTF16 here.
Fixed
> 
> >+      NS_ConvertUTF16toUTF8 results(resultItem);
> >+      printf("\n----- Search result: '%s'\n\n", results.get());
> 
> >+      NS_ConvertUTF16toUTF8 hrefCStr(hrefStr);
> >+      printf("HREF: '%s'\n", hrefCStr.get());
> Should inline these.
Fixed. (And similiar code near line 2787)
> 
> >     nsAutoString  entityStr;
> >+    entityStr = Substring(nameStr, ampOffset, semiOffset - ampOffset + 1);
> This should be a constructor.
Fixed. (and similiar code near line 3091)
> 
> >     // cut out any CRs or LFs
> >-    while ((offset = nameStr.FindCharInSet("\n\r", 0)) >= 0)
> >+    while ((offset = nsString_FindCharInSet(nameStr, "\n\r", 0)) >= 0)
> >     {
> >       nameStr.Cut(offset, 1);
> >     }
> This should use StripChars.
> 
Fixed.


Thanks for the review :) (Now I really just feel like I suck :( )

Comment 30

[[e-mail]]

Comment on attachment 263375 [details] [diff] [review]
Address comments 26, 28

>-    PRInt32   queryOffset;
>-    if ((queryOffset = nsString_Find(queryStr, searchURL, PR_TRUE )) < 0)
>+    PRInt32   queryOffset = searchURL.Find(queryStr, CaseInsensitiveCompare);
>+    if (queryOffset < 0)
Nice :-)

>-      if (numericEncoding.EqualsASCII(encodingList[i].numericEncoding)) 
>+      if (numericEncoding.EqualsLiteral(encodingList[i].numericEncoding))
>       {
>-        stringEncoding.AssignASCII(encodingList[i].stringEncoding);
>+        stringEncoding.AssignLiteral(encodingList[i].stringEncoding);
>         return NS_OK;
>       }
So confusing... nsStringAPI doesn't have *ASCII but its *Literal works the same, which of course is different to the internal *Literal, which only works on real literals and not const char*s. Maybe it would be better to add (trivial) *ASCII to the nsStringAPI?

Comment 31

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 263375 [details] [diff] [review]
Address comments 26, 28

+	$(XPCOM_GLUE_LDOPTS) \
+	$(call EXPAND_LIBNAME_PATH,unicharutil_external_s,$(LIBXUL_DIST)/lib) \

is that order right? I would think that the other way round would make more sense

+/// @see nsReadableUtils.h
+PRBool RFindInReadable(const nsAString& aPattern,

:/ I'm not really a fan of c&p, but I guess you have little choice

-  *resultURL = ToNewCString(action);
+  *resultURL = ToNewCString(NS_LossyConvertUTF16toASCII(action));

since this is a URL, please use ToNewUTF8String instead (then you don't need a NS_*Convert* either)

(wanna add a nullcheck for the result here?)

Since you have this replacement loop in at least three places, have you considered moving it into a helper function?
-      if (numericEncoding.EqualsASCII(encodingList[i].numericEncoding)) 
+      if (numericEncoding.EqualsLiteral(encodingList[i].numericEncoding))

No, this isn't a literal. Please file a bug to add EqualsASCII to nsStringAPI.

(And AssignASCII)

+  rv = netUtil->EscapeString(filePath,
+                             nsINetUtil::ESCAPE_URL_PATH,
+                             uriCescaped);

Intriguing. That's not exactly the best way to do it, especially since GetNativePath doesn't really work for all local files. oh well.
(that was just a sidenote, the code was broken before)

+            PRInt32 contentLen=0;
+            contentLen = contentLengthValue.ToInteger(&rv);

I'd merge these two lines

(this code seriously parses the Content-Length header instead of just reading the channel's contentLength attribute?)

+            float val = (float)PR_strtod(priceItemC.BeginReading(), nsnull);

would be nice to keep the error checking...

+    if (!PL_strcmp(aTopic, "profile-before-change"))

Just use strcmp

+    else if (!PL_strcmp(aTopic, "profile-do-change"))

here too


sr=biesi, but don't check this in with EqualsLiteral/AssignLiteral

Comment 32

[:Mook]

Attachment: address comment 31

(In reply to comment #31)
> (From update of attachment 263375 [details] [diff] [review])
> +       $(XPCOM_GLUE_LDOPTS) \
> +       $(call EXPAND_LIBNAME_PATH,unicharutil_external_s,$(LIBXUL_DIST)/lib) \
> 
> is that order right? I would think that the other way round would make more
> sense
I thought I'd want to use glue as much as possible (i.e. "thing that doesn't break"); I'll ask around.
> 
> +/// @see nsReadableUtils.h
> +PRBool RFindInReadable(const nsAString& aPattern,
> 
> :/ I'm not really a fan of c&p, but I guess you have little choice
:/ I'm not a fan either; this sucks.
> 
> -  *resultURL = ToNewCString(action);
> +  *resultURL = ToNewCString(NS_LossyConvertUTF16toASCII(action));
> 
> since this is a URL, please use ToNewUTF8String instead (then you don't need a
> NS_*Convert* either)
Changed the IDL to return a wstring instead; I can still return null, but it's all Unicode and no conversion is involved.
> 
> (wanna add a nullcheck for the result here?)
Added.
> 
> Since you have this replacement loop in at least three places, have you
> considered moving it into a helper function?
I had considered it, but figured just three cases isn't worth it.

> -      if (numericEncoding.EqualsASCII(encodingList[i].numericEncoding)) 
> +      if (numericEncoding.EqualsLiteral(encodingList[i].numericEncoding))
> 
> No, this isn't a literal. Please file a bug to add EqualsASCII to nsStringAPI.
bug 383956.  Meanwhile, changed to do crappy conversions :(  Yes, I checked the EqualsASCII impl, it ignores any high bytes there too, so LossyConvert is just fine.
> 
> (And AssignASCII)
(that too)
> 
> +  rv = netUtil->EscapeString(filePath,
> +                             nsINetUtil::ESCAPE_URL_PATH,
> +                             uriCescaped);
> 
> Intriguing. That's not exactly the best way to do it, especially since
> GetNativePath doesn't really work for all local files. oh well.
> (that was just a sidenote, the code was broken before)
Yeah, I realized that it was broken for OS9, but I don't understand the code enough to try to fix it and still not break existing prefs.
> 
> +            PRInt32 contentLen=0;
> +            contentLen = contentLengthValue.ToInteger(&rv);
> 
> I'd merge these two lines
merged.
> 
> (this code seriously parses the Content-Length header instead of just reading
> the channel's contentLength attribute?)
(I am no longer surprised by anything this code tries to do, no matter how... strange it seems)
> 
> +            float val = (float)PR_strtod(priceItemC.BeginReading(), nsnull);
> 
> would be nice to keep the error checking...
You mean priceErr that was never actually checked, and was instead just dropped on the floor? ;)
> 
> +    if (!PL_strcmp(aTopic, "profile-before-change"))
> 
> Just use strcmp
done; I thought that might have been useful in avoiding the native libc, but I guess if you say so...
> 
> +    else if (!PL_strcmp(aTopic, "profile-do-change"))
> 
> here too
done
> 
> 
> sr=biesi, but don't check this in with EqualsLiteral/AssignLiteral
> 

From IRC, biesi asked if changing nsXPIDLString had any effects - the only one was sending the lang code (en-US) over in HTTP GET via a vmsprintf, and in that case "null" is no better than "".

updated patching, carrying forward Neil's r+, will leave sr+ off until I figure out the deal with unicharutil_external_s.

Comment 33

[:Mook]

Attachment: put unicharutil_external_s first

carrying over r/sr.  biesi convinced me over IRC that putting unicharutil_external_s first makes more sense; this also follows what browser/components/build/Makefile.in does, and that's written by people smarter than I am :)

Comment 34

[:Mook]

Attachment: again, now that I actually *saved* the file

I fail.  I need to actually save files before generating patches.

Comment 35

[Christian :Biesinger (don't email me, ping me on IRC)]

> Yeah, I realized that it was broken for OS9

I meant Windows FWIW :) (i.e. filenames with characters outside of the ANSI codepage)

Comment 36

[:Mook]

philor mentioned on IRC that there's link errors with undefined references.  Debugging now :(

Comment 37

[:Mook]

Attachment: fix for --enable-static

same as previous patch, the only difference being fixing a build error when seamonkey is built with --enable-static --disable-shared
(note that SM doesn't _actually_ build like that at the moment, see bug 383313 )

The only difference is xpfe/components/search/src/Makefile.in

Comment 38

[[e-mail]]

Comment on attachment 269566 [details] [diff] [review]
fix for --enable-static

...not that I actually understand those Makefile settings...

Comment 39

[Mark Banner (:standard8)]

Mook: I guess you're not working on this anymore, but can you just make clear what still needs to be done, as that last patch seems to be approved but not checked in?

Comment 40

[:Mook]

Sure; sorry, I should have given some explanation, sorry.

Last I checked, the patch doesn't build (nsINetUtil changes).  If that gets fixed, it should then work.  So it's just minor bustage that shouldn't take very long.  Unfortunately I don't have trees to build with at the moment, and was getting frustrated with trying to carry this forward.

Comment 41

[Mark Banner (:standard8)]

Attachment: Fix compilation bustage

Same as the fix for --enable-static but with the netUtil->UnescapeString fixed. I think adding flags as zero to the call should be fine here: http://mxr.mozilla.org/seamonkey/source/netwerk/base/public/nsINetUtil.idl#185

Comment 42

[[e-mail]]

Comment on attachment 278316 [details] [diff] [review]
Fix compilation bustage

rs=me assuming this is the only change:
rv = netUtil->UnescapeString(nativePathSrc, 0, nativePath);

Comment 43

[Mark Banner (:standard8)]

(In reply to comment #42)
> (From update of attachment 278316 [details] [diff] [review])
> rs=me assuming this is the only change:
> rv = netUtil->UnescapeString(nativePathSrc, 0, nativePath);
> 
Correct.

I've checked this in. Thanks to Mook for doing all the work on this.