Open
Bug 313333
Opened 19 years ago
Updated 2 years ago
remove *WithConversion in (most of) xpfe/components
Categories
(Core :: XUL, defect)
Core
XUL
Tracking
()
NEW
People
(Reporter: Biesinger, Unassigned)
References
()
Details
Attachments
(1 file)
38.68 KB,
patch
|
neil
:
superreview-
|
Details | Diff | Splinter Review |
this doesn't do bookmarks yet (and not xremote either, but that's a single instance)
Reporter | ||
Comment 1•19 years ago
|
||
Attachment #200407 -
Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #200407 -
Flags: review?(jag)
Comment 2•19 years ago
|
||
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 3•19 years ago
|
||
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 4•19 years ago
|
||
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-
Reporter | ||
Comment 5•19 years ago
|
||
> 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.
Updated•12 years ago
|
Comment 6•12 years ago
|
||
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 7•12 years ago
|
||
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)
Reporter | ||
Updated•12 years ago
|
Assignee: cbiesinger → nobody
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•