remove *WithConversion in (most of) xpfe/components

NEW
Unassigned

Status

()

13 years ago
6 years ago

People

(Reporter: Biesinger, Unassigned)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment)

this doesn't do bookmarks yet (and not xremote either, but that's a single instance)
Created attachment 200407 [details] [diff] [review]
patch
Attachment #200407 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #200407 - Flags: review?(jag)
Comment on attachment 200407 [details] [diff] [review]
patch

>+    aClassName = mClassName;
>+    mClassName.Assign(aClassName);
Nit: inconsistent. Although I don't understand why you didn't just make mClassName an nsCString.

>+  nsAutoString checked;
>+  checked.AssignASCII(aValue ? "true" : "false");
>   res = mRDFService->GetLiteral(checked.get(), getter_AddRefs(checkedLiteral));
Not sure how that compares to res = mRDFService->GetLiteral(aValue ? NS_LITERAL_STRING("true").get() : NS_LITERAL_STRING("false").get(), getter_AddRefs(checkedLiteral));
Or maybe use if (aValue) checked.AssignLiteral("true"); else checked.AssignLiteral("false");

Internet Search Service is a law unto itself :-( Will review later...

>+      NS_NAMED_LITERAL_STRING(defaultMatch, "default-match");
>+      NS_NAMED_LITERAL_STRING(localAbook, "local-abook");
>+      rv = newItem->SetClassName(pDefaultMatch ? defaultMatch :
>+                                 localAbook);
My preferences is for unnamed literals in the ?:
Comment on attachment 200407 [details] [diff] [review]
patch

> 	char			*mRelatedLinksURL;
>-        static nsString         *mRLServerURL;
>+        static nsCString         *mRLServerURL;
> 
Even allowing for the tab, it doesn't line up any more...
Comment on attachment 200407 [details] [diff] [review]
patch

> NS_IMETHODIMP
> InternetSearchContext::AppendBytes(const char *buffer, PRInt32 numBytes)
> {
>-	mBuffer.AppendWithConversion(buffer, numBytes);
>+  AppendASCIItoUTF16(Substring(buffer, buffer + numBytes), mBuffer);
> 	return(NS_OK);
> }
While many of your changes are detabbed this makes the file look odd.
In this simple case, I'd suggest detabbing (and debracketing) the return.
In other cases, I'd prefer tabs to be left in.

> 	if ((isSearchCategoryURI(source)) && (categoryDataSource))
> 	{
> 		const char	*uri = nsnull;
> 		source->GetValueConst(&uri);
> 		if (!uri)	return(NS_ERROR_UNEXPECTED);
>-		nsAutoString	catURI;
>-		catURI.AssignWithConversion(uri);
> 
> 		nsCOMPtr<nsIRDFResource>	category;
>-		nsCAutoString			caturiC;
>-		caturiC.AssignWithConversion(catURI);
>-		if (NS_FAILED(rv = gRDFService->GetResource(caturiC,
>+		if (NS_FAILED(rv = gRDFService->GetResource(nsDependentCString(uri),
> 			getter_AddRefs(category))))
> 			return(rv);
> 
> 		rv = categoryDataSource->GetTarget(category, property, tv, target);
> 		return(rv);
> 	}
As discussted on IRC this is roughly equivalent to return categoryDataSource->GetTarget(source, property, rv, target); (and similarly for GetTargets below).

> 		const	char			*catResURI = nsnull;
> 		aCategoryRes->GetValueConst(&catResURI);
> 		if (!catResURI)		break;
> 		nsAutoString		categoryStr;
>-		categoryStr.AssignWithConversion(kURINC_SearchCategoryPrefix);
>-		categoryStr.AppendWithConversion(catResURI);
>+		categoryStr.AssignLiteral(kURINC_SearchCategoryPrefix);
>+    AppendUTF8toUTF16(catResURI, categoryStr);
> 
> 		nsCOMPtr<nsIRDFResource>	searchCategoryRes;
> 		if (NS_FAILED(rv = gRDFService->GetUnicodeResource(categoryStr,
> 			getter_AddRefs(searchCategoryRes))))	break;
We can do better here by calling GetResource which takes an AUTF8String.

>-char *
>+PRUnichar *
> InternetSearchDataSource::getSearchURI(nsIRDFResource *src)
> {
>-	char	*uri = nsnull;
>+	PRUnichar	*uri = nsnull;
> 
> 	if (src)
> 	{
> 		nsresult		rv;
> 		nsCOMPtr<nsIRDFNode>	srcNode;
> 		if (NS_SUCCEEDED(rv = mInner->GetTarget(src, kNC_URL, PR_TRUE, getter_AddRefs(srcNode))))
>@@ -1954,14 +1947,13 @@ InternetSearchDataSource::getSearchURI(n
> 			if (urlLiteral)
> 			{
> 				const PRUnichar	*uriUni = nsnull;
> 				urlLiteral->GetValueConst(&uriUni);
> 				if (uriUni)
> 				{
>-					nsAutoString	uriString(uriUni);
>-					uri = ToNewUTF8String(uriString);
>+					uri = ToNewUnicode(nsDependentString(uriUni));
> 				}
> 			}
> 		}
> 	}
> 	return(uri);
> }
Can we improve on this by returning the const PRUnichar*?
Actually one of the callers would prefer to have the urlLiteral!

>+				rv = bookmarks->AddBookmarkImmediately(nsDependentString(uri).get(),
D'oh!

>-	char	*uri = getSearchURI(aResource);
>+	PRUnichar	*uri = getSearchURI(aResource);
> 	if (!uri)	return(NS_ERROR_UNEXPECTED);
> 	nsAutoString	url;
>-	url.AssignWithConversion(uri);
>-	Recycle(uri);
>+	url.Adopt(uri);
> 
> 	nsresult			rv;
> 	nsCOMPtr<nsIRDFLiteral>	urlLiteral;
> 	if (NS_FAILED(rv = gRDFService->GetLiteral(url.get(), getter_AddRefs(urlLiteral)))
If you are unable to adopt(!) the comment before last, at least please use an nsAdoptingString.

> 			nsCOMPtr<nsIRDFResource>	catRes;
> 			if (catURI)
> 			{
> 				nsAutoString	catList;
> 				catList.AssignASCII(kURINC_SearchCategoryPrefix);
>-				catList.AppendWithConversion(catURI);
>+        AppendUTF8toUTF16(catURI, catList);
> 				gRDFService->GetUnicodeResource(catList, getter_AddRefs(catRes));
> 			}
Needs to use GetResource again.

>-				sourceContents.AssignWithConversion(contents, contentsLen);
>+				// XXX i18n?
>+				CopyASCIItoUTF16(Substring(contents, contents + contentsLen),
>+				                 sourceContents);
This needs to be an nsCString, see DecodeData (line 3477!) but that's beyond the scope of this bug.

> nsresult
> InternetSearchDataSource::GetURL(nsIRDFResource *source, nsIRDFLiteral** aResult)
> {
>         const char	*uri = nsnull;
> 	source->GetValueConst( &uri );
>-	nsAutoString	url;
>-	url.AssignWithConversion(uri);
>+	NS_ConvertUTF8toUTF16 url(uri);
> 	nsIRDFLiteral	*literal;
> 	gRDFService->GetLiteral(url.get(), &literal);
>         *aResult = literal;
>         return NS_OK;
> }
Bleh. This should be a three-line function...

> 			if(NS_FAILED(rv))
> 			{
> 				decoder->Reset();
>-				unsigned char	smallBuf[2];
>-				smallBuf[0] = 0xFF;
>-				smallBuf[1] = 0xFD;
>-				context->AppendBytes( (const char *)&smallBuf, 2L);
>+        PRUnichar replacementChar = 0xFFFD;
>+				context->AppendUnicodeBytes( &replacementChar, 1L);
Is this an endian fix too, or just correctness?

>-						updateURL.AssignWithConversion(updateUni);
>+            CopyUTF16toUTF8(updateUni, updateURL);
It's sooo tempting to push all these conversions into NS_NewURI ;-)
Attachment #200407 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview-
> Although I don't understand why you didn't just make
>mClassName an nsCString.

Since class names can be unicode, using a wide string seemed better to me.

>Not sure how that compares to res = mRDFService->GetLiteral(aValue ?
>NS_LITERAL_STRING("true").get() : NS_LITERAL_STRING("false").get(),
>getter_AddRefs(checkedLiteral));

Some compilers have problems with that. I forgot which ones, though... might've only been VACPP on OS/2.

>Or maybe use if (aValue) checked.AssignLiteral("true"); else
>checked.AssignLiteral("false");

hrm, I think what I wrote is easier to read...

> My preferences is for unnamed literals in the ?:

see above

> Even allowing for the tab, it doesn't line up any more...

oh. in my editor, it never lined up with anything.
Comment on attachment 200407 [details] [diff] [review]
patch

Just trying to "revive" this stalled bug/patch.
Attachment #200407 - Flags: review?(jag-mozilla) → review?(neil)
Comment on attachment 200407 [details] [diff] [review]
patch

Umm... did you not see the sr- on the patch?

(In reply to Christian Biesinger from comment #5)
> > Although I don't understand why you didn't just make
> >mClassName an nsCString.
> Since class names can be unicode, using a wide string seemed better to me.
Except there is only one caller, and it passes literals.
Attachment #200407 - Flags: review?(neil)
Assignee: cbiesinger → nobody
You need to log in before you can comment on or make changes to this bug.