title says it all. in bug 100214, we finally got rid of xpcom depending on unicharutil.. The good news is that it's only string/obsolete that has this evil dependency. the new string API is safe. The plan of action is to remove case-insensitive compares from the nsString API. (nsCString is fine)

ugh, not getting to this in 0.9.6

to make my life easier, I'm going to attempt to remove the *WithConversion routines.. that should allow us to eliminate Compare1to2 and friends, and force people to be explicit about what charset they're converting from/to.

this isn't happening in 0.9.7 :(

*sigh* 0.9.8 was way to short. moving all my bugs to 0.9.9

ok, this patch is not going in, but I'm putting it here so I can share it amongst my machines & test it on all platforms. Basically what I'm doing is removing the "PRBool aIgnoreCase" argument from ::FindChar2. The problem was that nsString::FindChar/RFindChar both have default arguments, so if I just remove the "aIgnoreCase" argument, then some callers will just pass in "PR_FALSE" to the aIndex parameter. And so, to find all the callers, I had to rename nsString::FindChar and nsString::RFindChar (adding a "Z" at the end). What I learned is that every caller of nsString::FindChar can be replaced with nsAString::FindChar, so ultimately I'll just be removing nsString::FindChar altogether. Unfortunately there is no nsAString::RFindChar, so I'll have to just update the callers... Ultimately I'll rename them back, and submit a new patch for this bug.

Or, if you are willing to, you could add nsA{C}String::RFindChar.

let's try that again, this with the string/ diffs!

*sigh* Next time I'll wait until "cvs diff" is done before uploading the patch

ok, this patch 1) removes nsString::FindChar() altogether 2) fixes ::FindChar2 so it no longer supports case-sensitivity 3) fixes all consumers of nsString::FindChar and nsString::RFindChar to use the right parameters (and now consumers of nsString::FindChar are using the nsAString::FindChar instead) At the very least, this makes it easier for someone to add RFindChar to nsAString() Can I get a review? I'm still running tests/etc of course.. but assuming all is well with this patch...

close. The entire nsIPref interface is deprecated. use nsIPrefService instead.

ignore that last comment, that goes in another bug

adding dougt for a review (trying to spread the string love here :))

Comment on attachment 66829 [details] [diff] [review] Final FindChar patch some callers of FindChar pass true and other false as ignore-case flag. Is it okay just to ignore this flag? If so, and jag is okay with the api changes, r=dougt

the PR_TRUE flag is only for case-sensitivity...but the great part about the consumers of this patch is that everyone who passed in PR_TRUE was only searching for a symbol... doh! If anything, this means this will be just a tiny tiny bit faster because we won't be trying to up-case '='!

Comment on attachment 66829 [details] [diff] [review] Final FindChar patch sr=jag

*sigh* - as jag pointed out, I missed a few. it's amazing what grep will find for you. reviews?

ok, now I've removed the aIgnoreCase from all external and internal FindChar() routines. There's simply no need for it, as I've demonstrated on a complete sweep of the tree....reviews? :)

Comment on attachment 67344 [details] [diff] [review] better patch, with nsCString diffs sr=jag What about the commercial tree?

commercial tree is cool - there are only 3 consumers of FindChar() and they all only pass in one parameter.

Comment on attachment 67344 [details] [diff] [review] better patch, with nsCString diffs r=shaver (alec's going to slip some whitespace into FindChar too -- it's cheap).

everyone is probably wondering what is up with this.. basically I made massive changes in my tree, and now my build wont start. (random assertions, then a crash) I've been stuck in this state for over a week working on other bugs.. I'm going to try to break my patch down into 2-3 testable chunks, and see if I can narrow down the problem.

So, I'm using a divide and conquer approach to figure out why my build is screwed. This patch fixes all the Equals*() consumers.. and it seems to work. So I'll get this one in first. Can I get an sr=jag, r=dbaron?

Notes on the patch: - really took a stick to nsDocShell - the string use in that file was killing me! We made something like 5-6 copies of a url every time we loaded it. - there's a little overlap with bug 63025, but don't worry I'll land that bug first so we don't have any bustage

Comment on attachment 69482 [details] [diff] [review] First part of consumer cleanup >Index: intl/uconv/src/nsCharsetAliasImp.cpp >=================================================================== >RCS file: /cvsroot/mozilla/intl/uconv/src/nsCharsetAliasImp.cpp,v >retrieving revision 1.29 >diff -u -r1.29 nsCharsetAliasImp.cpp >--- intl/uconv/src/nsCharsetAliasImp.cpp 10 Feb 2002 15:49:18 -0000 1.29 >+++ intl/uconv/src/nsCharsetAliasImp.cpp 14 Feb 2002 16:03:53 -0000 >@@ -144,7 +144,7 @@ > if(NS_SUCCEEDED(res)) { > res = this->GetPreferred(aCharset2, name2); > if(NS_SUCCEEDED(res)) { >- *oResult = (name1.EqualsIgnoreCase(name2)) ? PR_TRUE : PR_FALSE; >+ *oResult = (name1.Equals(name2,nsCaseInsensitiveStringComparator())) ? PR_TRUE : PR_FALSE; Nit: space after comma. >Index: intl/locale/src/windows/nsDateTimeFormatWin.cpp >=================================================================== >RCS file: /cvsroot/mozilla/intl/locale/src/windows/nsDateTimeFormatWin.cpp,v >retrieving revision 1.38 >diff -u -r1.38 nsDateTimeFormatWin.cpp >--- intl/locale/src/windows/nsDateTimeFormatWin.cpp 5 Feb 2002 01:40:19 -0000 1.38 >+++ intl/locale/src/windows/nsDateTimeFormatWin.cpp 14 Feb 2002 16:03:53 -0000 >@@ -62,14 +63,14 @@ > > // use cached info if match with stored locale > if (NULL == locale) { >- if (mLocale.Length() && mLocale.EqualsIgnoreCase(mAppLocale)) { >+ if (mLocale.Length() && mLocale.Equals(mAppLocale, nsCaseInsensitiveStringComparator())) { !mLocale.IsEmpty(). This check can be left off if we know they're not both going to be empty at the same time. > return NS_OK; > } > } > else { > res = locale->GetCategory(aCategory.get(), &aLocaleUnichar); > if (NS_SUCCEEDED(res) && NULL != aLocaleUnichar) { >- if (mLocale.Length() && mLocale.EqualsIgnoreCase(nsString(aLocaleUnichar))) { >+ if (mLocale.Length() && mLocale.Equals(nsDependentString(aLocaleUnichar), nsCaseInsensitiveStringComparator())) { Idem dito. >Index: intl/chardet/src/nsMetaCharsetObserver.cpp >=================================================================== >RCS file: /cvsroot/mozilla/intl/chardet/src/nsMetaCharsetObserver.cpp,v >retrieving revision 1.61 >diff -u -r1.61 nsMetaCharsetObserver.cpp >--- intl/chardet/src/nsMetaCharsetObserver.cpp 10 Feb 2002 15:49:16 -0000 1.61 >+++ intl/chardet/src/nsMetaCharsetObserver.cpp 14 Feb 2002 16:03:54 -0000 >@@ -287,11 +287,10 @@ > > if (!newCharset.IsEmpty()) > { >- nsAutoString charsetString(charset); nsDependentString charsetString(charset) And you can save a string length check below. >- if(! newCharset.EqualsIgnoreCase(charsetString)) >+ if(! newCharset.Equals(nsDependentString(charset), nsCaseInsensitiveStringComparator())) > { > PRBool same = PR_FALSE; >- nsresult res2 = mAlias->Equals( newCharset, charsetString , &same); >+ nsresult res2 = mAlias->Equals( newCharset, nsDependentString(charset) , &same); >Index: gfx/src/windows/nsFontMetricsWin.cpp >=================================================================== >RCS file: /cvsroot/mozilla/gfx/src/windows/nsFontMetricsWin.cpp,v >retrieving revision 3.157 >diff -u -r3.157 nsFontMetricsWin.cpp >--- gfx/src/windows/nsFontMetricsWin.cpp 12 Feb 2002 04:51:40 -0000 3.157 >+++ gfx/src/windows/nsFontMetricsWin.cpp 14 Feb 2002 16:03:57 -0000 >@@ -2935,7 +2935,11 @@ > nsFontMetricsWin::LoadGenericFont(HDC aDC, PRUint32 aChar, nsString* aName) > { > for (int i = mLoadedFonts.Count()-1; i >= 0; --i) { >- if (aName->EqualsIgnoreCase(((nsFontWin*)mLoadedFonts[i])->mName)) { >+ // woah, this seems bad >+ const nsACString& fontName = >+ nsDependentCString(((nsFontWin*)mLoadedFonts[i])->mName); >+ if (fontName.Equals(NS_LossyConvertUCS2toASCII(*aName), nsCaseInsensitiveCStringComparator())) { >+ How about: if (aName->Equals(NS_ConvertASCIItoUCS2(((nsFontWin*)mLoadedFonts[i])->mName), nsCaseInsensitiveCStringComparator())) { >@@ -4686,7 +4690,10 @@ > nsFontMetricsWinA::LoadGenericFont(HDC aDC, PRUnichar aChar, nsString* aName) > { > for (int i = mLoadedFonts.Count()-1; i >= 0; --i) { >- if (aName->EqualsIgnoreCase(((nsFontWinA*)mLoadedFonts[i])->mName)) { >+ const nsACString& fontName = >+ nsDependentCString(((nsFontWinA*)mLoadedFonts[i])->mName); >+ >+ if (fontName.Equals(NS_LossyConvertUCS2toASCII(*aName), nsCaseInsensitiveCStringComparator())) { > return nsnull; > } > } And the same... Though one wonders why |aName| is a |nsString|. >Index: gfx/src/windows/nsDeviceContextSpecWin.cpp >=================================================================== >RCS file: /cvsroot/mozilla/gfx/src/windows/nsDeviceContextSpecWin.cpp,v >retrieving revision 3.17 >diff -u -r3.17 nsDeviceContextSpecWin.cpp >--- gfx/src/windows/nsDeviceContextSpecWin.cpp 13 Feb 2002 13:58:31 -0000 3.17 >+++ gfx/src/windows/nsDeviceContextSpecWin.cpp 14 Feb 2002 16:03:58 -0000 >@@ -1062,7 +1062,7 @@ > if (NS_SUCCEEDED(GetLocalizedBundle(PRINTDLG_PROPERTIES, getter_AddRefs(strBundle)))) { > nsAutoString doExtendStr; > if (NS_SUCCEEDED(GetLocalizedString(strBundle, "extend", doExtendStr))) { >- doExtend = doExtendStr.EqualsIgnoreCase("true"); >+ doExtend = doExtendStr.Equals(NS_LITERAL_STRING("true")); We'll need to make sure our PRINTDLG_PROPERTIES always use lowercase "true" then. >Index: gfx/src/windows/nsNativeThemeWin.cpp >=================================================================== >RCS file: /cvsroot/mozilla/gfx/src/windows/nsNativeThemeWin.cpp,v >retrieving revision 3.12 >diff -u -r3.12 nsNativeThemeWin.cpp >--- gfx/src/windows/nsNativeThemeWin.cpp 13 Feb 2002 00:15:44 -0000 3.12 >+++ gfx/src/windows/nsNativeThemeWin.cpp 14 Feb 2002 16:03:58 -0000 >@@ -333,7 +326,7 @@ > if (res == NS_CONTENT_ATTR_NO_VALUE || > (res != NS_CONTENT_ATTR_NOT_THERE && attr.IsEmpty())) > return PR_TRUE; // This handles the HTML case (an attr with no value is like a true val) >- return attr.EqualsIgnoreCase("true"); // This handles the XUL case. >+ return attr.Equals(NS_LITERAL_STRING("true")); // This handles the XUL case. I would very much favor case sensitive attribute values, but I think you should make this a case insensitive check for now. > } > > PRBool nsNativeThemeWin::IsDisabled(nsIFrame* aFrame) >@@ -358,7 +351,7 @@ > return PR_TRUE; // XXXdwh Can the HTML form control's checked property differ > // from the checked attribute? If so, will need an IsContentofType > // HTML followed by a QI to nsIDOMHTMLInputElement to grab the prop. >- return checked.EqualsIgnoreCase("true"); // This handles the XUL case. >+ return checked.Equals(NS_LITERAL_STRING("true")); // This handles the XUL case. Same here. > PRBool nsNativeThemeWin::IsSelected(nsIFrame* aFrame) >Index: widget/src/windows/nsFilePicker.cpp >=================================================================== >RCS file: /cvsroot/mozilla/widget/src/windows/nsFilePicker.cpp,v >retrieving revision 1.46 >diff -u -r1.46 nsFilePicker.cpp >--- widget/src/windows/nsFilePicker.cpp 12 Feb 2002 03:52:43 -0000 1.46 >+++ widget/src/windows/nsFilePicker.cpp 14 Feb 2002 16:04:02 -0000 >@@ -180,9 +179,11 @@ > if ( extIndex >= 0) { > nsAutoString ext; > mDefault.Right(ext, mDefault.Length() - extIndex); >- // Should we test for ".cgi", ".asp", ".jsp" and other "generated" html pages? >- if ( ext.EqualsIgnoreCase(".htm") || >- ext.EqualsIgnoreCase(".html") || >+ // Should we test for ".cgi", ".asp", ".jsp" and other >+ // "generated" html pages? >+ >+ if ( ext.EqualsIgnoreCase(".htm") || >+ ext.EqualsIgnoreCase(".html") || > ext.EqualsIgnoreCase(".shtml") ) { > // This is supposed to append ".htm" if user doesn't supply an extension > //XXX Actually, behavior is sort of weird: You missed some. >=================================================================== >RCS file: /cvsroot/mozilla/content/xul/document/src/nsXULDocument.cpp,v >retrieving revision 1.479 >diff -u -r1.479 nsXULDocument.cpp >--- content/xul/document/src/nsXULDocument.cpp 14 Feb 2002 00:40:55 -0000 1.479 >+++ content/xul/document/src/nsXULDocument.cpp 14 Feb 2002 16:04:09 -0000 >@@ -2272,7 +2272,6 @@ > nsIContent * end = nsnull; > nsIContent * body = nsnull; > >- nsAutoString bodyStr; bodyStr.Assign(NS_LITERAL_STRING("BODY")); > PRInt32 i, n; > mRootContent->ChildCount(n); > for (i=0;i<n;i++) { >@@ -2280,7 +2279,12 @@ > mRootContent->ChildAt(i, child); > nsCOMPtr<nsIAtom> atom; > child->GetTag(*getter_AddRefs(atom)); >- if (bodyStr.EqualsIgnoreCase(atom)) { >+ >+ nsAutoString atomValue; >+ atom->ToString(atomValue); >+ >+ ToUpperCase(atomValue); >+ if (NS_LITERAL_STRING("BODY").Equals(atomValue)) { > body = child; > break; > } Erh ... Uh huh. Bug 125627. >Index: docshell/base/nsDocShell.cpp I'll look at this later tonight. >Index: mailnews/import/src/nsImportMail.cpp >=================================================================== >RCS file: /cvsroot/mozilla/mailnews/import/src/nsImportMail.cpp,v >retrieving revision 1.40 >diff -u -r1.40 nsImportMail.cpp >--- mailnews/import/src/nsImportMail.cpp 31 Jan 2002 01:28:27 -0000 1.40 >+++ mailnews/import/src/nsImportMail.cpp 14 Feb 2002 16:04:33 -0000 >@@ -1139,7 +1140,7 @@ > nsXPIDLString prettyName; > rv = server->GetPrettyName( getter_Copies( prettyName)); > if (NS_SUCCEEDED( rv)) { >- if (!newName.CompareWithConversion( nsAutoString(prettyName), PR_TRUE)) >+ if (newName.Equals( nsDependentString(prettyName), nsCaseInsensitiveStringComparator())) nsXPIDLString is a nsAString, no need to wrap it. >Index: mailnews/absync/src/nsAbSync.cpp >=================================================================== >RCS file: /cvsroot/mozilla/mailnews/absync/src/nsAbSync.cpp,v >retrieving revision 1.72 >diff -u -r1.72 nsAbSync.cpp >--- mailnews/absync/src/nsAbSync.cpp 10 Feb 2002 15:49:24 -0000 1.72 >+++ mailnews/absync/src/nsAbSync.cpp 14 Feb 2002 16:04:35 -0000 >@@ -2795,10 +2795,9 @@ > } > > PRInt32 >-nsAbSync::GetTypeOfPhoneNumber(nsString tagName) >+nsAbSync::GetTypeOfPhoneNumber(nsAString& tagName) > { >- nsString compValue = tagName; >- compValue.Append(NS_LITERAL_STRING("_type")); >+ const nsAString& compValue = tagName + NS_LITERAL_STRING("_type"); eek! NS_LITERAL_STRING is a temporary which will potentially go away after that line, leaving a dangling pointer in the nsDependentConcatenation.

ok, addressed jag's concerns The reason I used nsCaseInsensitiveCStringComparator instead of nsCaseInsensitiveStringComparator is because the former is cheaper than the latter. .. and usually we're comparing against ascii strings (like font names, etc) but I'll switch them as you requested :)

Comment on attachment 69677 [details] [diff] [review] First part of consumer cleanup, v2 Then again, having a NS_LossyConvertUCS2toASCII in there might compel someone to go make |aName| be nsACString. Prefer ++iter over iter++ since the former doesn't need to create a temporary copy of itself. Anyway, looked at nsDocShell.cpp, changes look okay. Assuming all comments were addressed (modulo the one above), sr=jag. One step at a time, huh.

all suggested changes have been addressed. Just need a reviewer now....

Comment on attachment 69677 [details] [diff] [review] First part of consumer cleanup, v2 alecf will make a few minor changes (mark some const, drop a ?:) but this looks good. r=timeless

This change actually broke anchor support in html mail messages. Please see http://bugzilla.mozilla.org/show_bug.cgi?id=126981

I put a patch in that bug. You accidentally reversed the logic in nsDocShell::ScrollToAnchor. Old code: if (sCurrentLeft.CompareWithConversion(sNewLeft, PR_FALSE, -1) != 0) return NS_OK; your new Code: if (Substring(currentLeftStart, currentLeftEnd).Equals(sNewLeft)) return NS_OK; This causes the breakage in anchor support.

ok, I've done enough on this bug for 0.9.9, I guess the rest will wait for 1.0

ok, This patch is for platform testing... I'm applying it to my linux/mac builds now. jag, I'd definitely appreciate you taking a look at the changes to string/public and string/src. You can ignore the FindEx() changes, that was just a syntax change I put in so I could catch all the Find() occurances via the compiler. before I check in, they will return to being Find()

Comment on attachment 73772 [details] [diff] [review] Find/Compare/Equals patch for testing >Index: intl/unicharutil/util/nsUnicharUtils.cpp >=================================================================== >RCS file: /cvsroot/mozilla/intl/unicharutil/util/nsUnicharUtils.cpp,v >retrieving revision 1.16 >diff -u -r1.16 nsUnicharUtils.cpp >--- intl/unicharutil/util/nsUnicharUtils.cpp 23 Dec 2001 02:56:41 -0000 1.16 >+++ intl/unicharutil/util/nsUnicharUtils.cpp 12 Mar 2002 23:23:33 -0000 >@@ -232,6 +232,10 @@ > int > nsCaseInsensitiveStringComparator::operator()( const PRUnichar* lhs, const PRUnichar* rhs, PRUint32 aLength ) const > { >+ // see if they're an exact match first >+ if (nsCRT::strncmp(lhs, rhs, aLength) == 0) >+ return 0; >+ Is this really a win? >Index: string/obsolete/bufferRoutines.h >=================================================================== >RCS file: /cvsroot/mozilla/string/obsolete/bufferRoutines.h,v >retrieving revision 1.15 >diff -u -r1.15 bufferRoutines.h >--- string/obsolete/bufferRoutines.h 14 Feb 2002 06:22:53 -0000 1.15 >+++ string/obsolete/bufferRoutines.h 12 Mar 2002 23:23:35 -0000 >@@ -694,16 +694,8 @@ > * @return -1,0,1 depending on <,==,> > */ > PRInt32 Compare2To2(const PRUnichar* aStr1,const PRUnichar* aStr2,PRUint32 aCount,PRBool aIgnoreCase); Need to update this to match definition below. >-PRInt32 Compare2To2(const PRUnichar* aStr1,const PRUnichar* aStr2,PRUint32 aCount,PRBool aIgnoreCase){ >- PRInt32 result=0; >-#ifndef XPCOM_STANDALONE >- if(aIgnoreCase && NS_SUCCEEDED(NS_InitCaseConversion())) >- gCaseConv->CaseInsensitiveCompare(aStr1, aStr2, aCount, &result); >- else result=nsCRT::strncmp(aStr1,aStr2,aCount); >-#else >- NS_ERROR("call not supported in XPCOM_STANDALONE"); >-#endif >- return result; >+PRInt32 Compare2To2(const PRUnichar* aStr1,const PRUnichar* aStr2,PRUint32 aCount){ >+ return nsCRT::strncmp(aStr1,aStr2,aCount); > } > > >Index: layout/xul/base/src/nsTextBoxFrame.cpp >=================================================================== >RCS file: /cvsroot/mozilla/layout/xul/base/src/nsTextBoxFrame.cpp,v >retrieving revision 1.44 >diff -u -r1.44 nsTextBoxFrame.cpp >--- layout/xul/base/src/nsTextBoxFrame.cpp 21 Feb 2002 13:39:39 -0000 1.44 >+++ layout/xul/base/src/nsTextBoxFrame.cpp 12 Mar 2002 23:23:50 -0000 >@@ -741,17 +741,25 @@ > if (!mAccessKeyInfo) > mAccessKeyInfo = new nsAccessKeyInfo(); > >- if (!AlwaysAppendAccessKey()) { >- // not appending access key - do case-sensitive search first >- mAccessKeyInfo->mAccesskeyIndex = mCroppedTitle.Find(mAccessKey, PR_FALSE); >- if (mAccessKeyInfo->mAccesskeyIndex == kNotFound) { >- // didn't find it - perform a case-insensitive search >- mAccessKeyInfo->mAccesskeyIndex = mCroppedTitle.Find(mAccessKey, PR_TRUE); >- } >- } else { >- // use case-insensitive, reverse find for appended access keys >- mAccessKeyInfo->mAccesskeyIndex = mCroppedTitle.RFind(mAccessKey, PR_TRUE); >- } >+ nsAString::const_iterator start, end; >+ >+ mCroppedTitle.BeginReading(start); >+ mCroppedTitle.EndReading(end); >+ >+ // remember the beginning of the string >+ nsAString::const_iterator originalStart = start; >+ >+ PRBool found; >+ if (!AlwaysAppendAccessKey()) >+ found = FindInReadable(mAccessKey, start, end, >+ nsCaseInsensitiveStringComparator()); >+ else >+ found = RFindInReadable(mAccessKey, start, end, >+ nsCaseInsensitiveStringComparator()); >+ if (found) >+ mAccessKeyInfo->mAccesskeyIndex = Distance(originalStart, start); >+ else >+ mAccessKeyInfo->mAccesskeyIndex = kNotFound; > } > } > } You need to do the case sensitive search first. This to make "Save Page _A_s" work (with your code it'll be "S_a_ve Page As"). >Index: xpfe/components/search/src/nsInternetSearchService.cpp >=================================================================== >RCS file: /cvsroot/mozilla/xpfe/components/search/src/nsInternetSearchService.cpp,v >retrieving revision 1.167 >diff -u -r1.167 nsInternetSearchService.cpp >--- xpfe/components/search/src/nsInternetSearchService.cpp 6 Mar 2002 07:48:50 -0000 1.167 >+++ xpfe/components/search/src/nsInternetSearchService.cpp 12 Mar 2002 23:23:59 -0000 >@@ -126,7 +127,38 @@ > > int PR_CALLBACK searchModePrefCallback(const char *pref, void *aClosure); > >+// helper routine because we need to rewrite this to use string >+// iterators.. this replaces the old nsString::Find > >+static PRInt32 nsString_Find(nsAString& aPattern, >+ nsAString& aSource, >+ PRBool aIgnoreCase = PR_FALSE, >+ PRInt32 aOffset = 0, PRInt32 aCount = -1) Nit: indentation >@@ -917,6 +949,7 @@ > { > nsresult rv = NS_OK; > >+ printf("Loading engines..\n"); > if (mEngineListBuilt == PR_FALSE) > { > mEngineListBuilt = PR_TRUE; printf. >@@ -2483,6 +2516,7 @@ > { > nsCOMPtr<nsIRDFResource> trueEngine; > rv = resolveSearchCategoryEngineURI(engine, getter_AddRefs(trueEngine)); >+ NS_ENSURE_SUCCESS(rv, rv); NS_ENSURE_SUCCESS returns on failure. Just make it an assertion (if you need that), see the next line: > if (NS_FAILED(rv) || (rv == NS_RDF_NO_VALUE)) return(rv); > if (!trueEngine) return(NS_RDF_NO_VALUE); > >@@ -2490,7 +2524,9 @@ > } > > nsCOMPtr<nsIRDFLiteral> dataLit; >- if (NS_FAILED(rv = FindData(engine, getter_AddRefs(dataLit))) || >+ rv = FindData(engine, getter_AddRefs(dataLit)); >+ NS_ENSURE_SUCCESS(rv, rv); Same here: >+ if (NS_FAILED(rv) || > (rv == NS_RDF_NO_VALUE)) return(rv); > if (!dataLit) return(NS_ERROR_UNEXPECTED); > And here: >@@ -2542,9 +2578,18 @@ > } > > nsAutoString action, input, method, userVar; >- if (NS_FAILED(rv = GetData(dataUni, "search", 0, "action", action))) return(rv); >- if (NS_FAILED(rv = GetData(dataUni, "search", 0, "method", method))) return(rv); >- if (NS_FAILED(rv = GetInputs(dataUni, userVar, text, input))) return(rv); >+ if (NS_FAILED(rv = GetData(dataUni, "search", 0, "action", action))) { >+ NS_ENSURE_SUCCESS(rv, rv); >+ return(rv); >+ } >+ if (NS_FAILED(rv = GetData(dataUni, "search", 0, "method", method))) { >+ NS_ENSURE_SUCCESS(rv, rv); >+ return(rv); >+ } >+ if (NS_FAILED(rv = GetInputs(dataUni, userVar, text, input))) { >+ NS_ENSURE_SUCCESS(rv, rv); >+ return(rv); >+ } > if (input.Length() < 1) return(NS_ERROR_UNEXPECTED); > > // we can only handle HTTP GET >Index: editor/libeditor/text/nsPlaintextEditor.cpp >=================================================================== >RCS file: /cvsroot/mozilla/editor/libeditor/text/nsPlaintextEditor.cpp,v >retrieving revision 1.30 >diff -u -r1.30 nsPlaintextEditor.cpp >--- editor/libeditor/text/nsPlaintextEditor.cpp 7 Mar 2002 14:34:35 -0000 1.30 >+++ editor/libeditor/text/nsPlaintextEditor.cpp 12 Mar 2002 23:24:07 -0000 >@@ -285,20 +285,21 @@ > metaElement = do_QueryInterface(metaNode); > if (!metaElement) continue; > >- nsString currentValue; >+ nsAutoString currentValue; > if (NS_FAILED(metaElement->GetAttribute(NS_LITERAL_STRING("http-equiv"), currentValue))) continue; > >- if (kNotFound != currentValue.Find("content-type", PR_TRUE)) { >+ if (FindInReadable(NS_LITERAL_STRING("content-type"), >+ currentValue, >+ nsCaseInsensitiveStringComparator())) { > NS_NAMED_LITERAL_STRING(content, "content"); > if (NS_FAILED(metaElement->GetAttribute(content, currentValue))) continue; > >- NS_NAMED_LITERAL_STRING(charset, "charset="); >- PRInt32 offset = currentValue.Find(charset.get(), PR_TRUE); >- if (kNotFound != offset) { >- currentValue.Left(newMetaString, offset); // copy current value before "charset=" (e.g. text/html) >- newMetaString.Append(charset); >- newMetaString.Append(characterSet); >- result = nsEditor::SetAttribute(metaElement, content, newMetaString); >+ NS_NAMED_LITERAL_STRING(charsetEquals, "charset="); >+ if (FindInReadable(charsetEquals, currentValue, >+ nsCaseInsensitiveStringComparator())) { >+ >+ result = nsEditor::SetAttribute(metaElement, content, >+ charsetEquals + characterSet); > if (NS_SUCCEEDED(result)) > newMetaCharset = PR_FALSE; > break; Given a content attribute of "foopy, charset=bla", the old code would turn this into "foopy, charset=newbla". Your code doesn't copy the "foopy, " part. >Index: mailnews/addrbook/src/nsAbDirectoryQuery.cpp >=================================================================== >RCS file: /cvsroot/mozilla/mailnews/addrbook/src/nsAbDirectoryQuery.cpp,v >retrieving revision 1.8 >diff -u -r1.8 nsAbDirectoryQuery.cpp >--- mailnews/addrbook/src/nsAbDirectoryQuery.cpp 6 Feb 2002 22:50:16 -0000 1.8 >+++ mailnews/addrbook/src/nsAbDirectoryQuery.cpp 12 Mar 2002 23:24:08 -0000 >@@ -618,28 +619,38 @@ > case nsIAbBooleanConditionTypes::BeginsWith: >- *matchFound = value.Find (matchValue.get(), PR_TRUE) == 0; >+ { >+ if (value.Length() < matchValue.Length()) { >+ *matchFound = PR_FALSE; >+ break; >+ } >+ *matchFound = >+ matchValue.Equals(Substring(value, 0, >+ matchValue.Length()), >+ nsCaseInsensitiveStringComparator()); >+ } The second matchValue (in the Substring) should be value.

Excellent, thanks for the review jag. The only thing is, at the very end of your review, "matchValue.Length()" should indeed be exactly that - the test is whether or not "value" begins with "matchValue" - so I check if "matchValue" is equal to the prefix of "value" (and earlier, I've tested their lengths, to make sure that the Length is ok)

You are correct. For some reason I read this as: >+ matchValue.Equals(Substring(matchValue, 0, >+ matchValue.Length()), >+ nsCaseInsensitiveStringComparator()); Don't ask me why :-)

this is a supplement to the above patch, I needed this patch so unix could build. this locale stuff REALLY wants to be cleaned up to use nsCString's for locale, but it looks like I'll have to change about a half-dozen interfaces to get there, so I did what I could in this patch. waiting on my mac build now.

Comment on attachment 73865 [details] [diff] [review] Find/Compare/Equals strawman v2 sr=jag, but remove that optimization from the case insensitive wide string comparator until we've measured whether it really is.

Is this bug related to bug 125958? The fix for this bug was checked in at the end of January and bug 125958 appears from around that time.

mac required two minor tweaks.

ok, this is the final patch, which changes "FindEx" back to "Find" and includes all patches for windows, mac and unix. Can I get a final review? Nothing else has changed from the last 3 "Find/Compare/Equals" patches besides: 1) the change from FindEx back to Find. 2) the removal of that nsCRT::strcmp(), by request of jag can I get a final set of reviews?

(ignore that last one, I hit "Submit" before the diff finished) ok, this is the final patch, which changes "FindEx" back to "Find" and includes all patches for windows, mac and unix. Can I get a final review? Nothing else has changed from the last 3 "Find/Compare/Equals" patches besides: 1) the change from FindEx back to Find. 2) the removal of that nsCRT::strcmp(), by request of jag can I get a final set of reviews?

Comment on attachment 74534 [details] [diff] [review] Find/Compare/Equals absolutely final patch nit: your touching nsCRT::strlen, ya wanna change it to strlen()? CompareEx? Are you happy with that name? More soon.

Comment on attachment 74534 [details] [diff] [review] Find/Compare/Equals absolutely final patch r=dougt but i defer to jag for string expertise.

Comment on attachment 74534 [details] [diff] [review] Find/Compare/Equals absolutely final patch Noting r=dougt, adding sr=jag. Fix the CompareEx (especially since it might compile for you but not on platforms which don't support explicit) It looks like the const PRUnichar* CompareEx is only used internally (by the nsXPIDLString one).

after talking with Jag, I've removed all the CompareEx routines altogether, since nobody was using them. Doesn't affect the rest of the patch, just removes dead code.

Comment on attachment 74534 [details] [diff] [review] Find/Compare/Equals absolutely final patch a=scc

ok, that patch has landed.. this is the final, final patch which actually removes the final dependency now here's the thing. what I'm doing is assuming that if someone is doing a 2-to-1 comparison, then they already understand the implications of what they are doing. Before the patch, we called into gCaseConv to do the conversion, but we have to remember that one ofthe characters that we're downcasing, started life as a straight "char" - which means when we lowercase it, it will fall into the ASCII range as well.. ..and since we're comparing it against a PRUnichar we're basicaly deciding that that PRUnichar must fall into the ASCII range in order to be equal, and so the PRUnichar is expected to be in the ASCII range. I've put a NS_WARNING in there. My guess is that if we ever see this NS_WARNING, that it will be on non-Latin1 web pages. Can I get a review/super-review?

Comment on attachment 74994 [details] [diff] [review] Fix up bufferRoutines The current implementation of nsCRT::ToUpper and nsCRT::ToLower only works for ASCII, not all of ISO-8859-1. So, really, you should have a warning that the 1-byte string is ASCII. What does a conversion from PRUnichar to char do? Does it just remove the high bits? Then it's possible for this algorithm to lead to incorrect comparison when I think it really could be done correctly -- just convert the |char| to |PRUnichar|, and give an NS_WARNING that the char is less than 128. (I think such use is valid -- after all, we often have known strings, e.g., HTML tags, that are within ASCII, that we want to compare to user input that could contain high characters.)

such a use is valid, but there's no API that takes a PRUnichar string. But I see the danger, where we could hit false-positives. I'll spin a new patch

ok, I dropped the kIsoLatin1toUCS2 conversion because it was just a 256-byte identity array (i.e. mapped every character onto itself) - a big lie, basically. What's happening now is that we don't try to do the case conversion if the given byte is out of range, and we throw the warning if someone is trying to do a case-insensitive comparison on strings that are out of range. This should be valid because: 1) Case conversion will only transform characters in the range (<128) into other characters within the range. 2) If the two characters are NOT equal, and either one is out of range, then no case conversion will map the two characters to the same character. i.e. - if both are out of range, no case conversion will occur - if one is out of range, and one is in range, then the out-of-range character won't change, and the in-range character will map to another character in-range - if both are in-range, then we're fine (they'll both map to characters in-range)

argh, attached the wrong version of the patch. the same arguments still apply.

adding dbaron for review of the latest patch.

I'm worried about the direct cast from char to PRUnichar there. On systems where char is signed it'll first be expanded to a (signed) int16 (with preservation of sign), and then cast to uint16. That means any char >= 128 will lead to a rather large number, while the same value in the PRUnichar* will stay the same, which will mess up your equality comparison.

Comment on attachment 75063 [details] [diff] [review] Fix up bufferRoutines v2.1 >+ // careful! we're down-casting PRUnichar to char here >+ // this is pretty safe because we'd be comparing it with a >+ // char anyway This comment seems wrong now. > PRUnichar c1 = *s1++; >- PRUnichar c2 = kIsoLatin1ToUCS2_2[*(const unsigned char*)s2++]; >+ PRUnichar c2 = PRUnichar(*s2++); As jag said, this probably needs to be a 2-step cast. (I wouldn't have caught that.) > if (c1 != c2) { >+#ifdef NS_DEBUG >+ if (aIgnoreCase && (c1>=128 || c2>=128)) >+ NS_WARNING("got a non-ASCII string, but we can't do an accurage case conversion!"); >+#endif I think we should only warn about the range of c2, not c1. >+ // can't do case conversion on characters out of our range >+ if (aIgnoreCase && c1<128 && c2<128) { >+ >+ c1 = nsCRT::ToLower(char(c1)); >+ c2 = nsCRT::ToLower(char(c2)); Maybe you should have new char varibles here to avoid the conversion back to PRUnichar?

ok, comment fixed, and 2-step cast done. As for the range, I think this is a valid check - because its possible someone will in fact send us a character that is a valid 8-bit char, say Ä, and nsCRT::ToLower won't handle this.. so we need to warn people if they're trying to do a case-insensitive comparison. I'm not too concerned about the 2nd pairs of casts - at this point we're converting from a PRUnichar that we know is < 128, I don't think its worth keeping around more temporary variables just for the case where we're doing an case-insensitive comparison.

> As for the range, I think this is a valid check - because its possible someone > will in fact send us a character that is a valid 8-bit char, say Ä, and > nsCRT::ToLower won't handle this.. so we need to warn people if they're trying > to do a case-insensitive comparison. But if you check the 1-byte string's character for being less than 128, then it will never compare as equal to a character in the 2-byte string that's not. The reason I think there should be a warning for only the one byte string is that often these comparison functions are used (I think) to compare a constant string in code (1-byte) to some user input (2-byte). The < 128 checks for both around the ToLower calls still make sense.

> The > reason I think there should be a warning for only the one byte string is that > often these comparison functions are used (I think) to compare a constant string > in code (1-byte) to some user input (2-byte). ok, point taken...you're right in that it will probably cause lots of unnecessary warnings. I'll remove the warning for the 2-byte string. with that, do I get a review? :)

here's the final patch, hopefully :)

Comment on attachment 75187 [details] [diff] [review] Fix up bufferRoutines v2.3 >+ PRUnichar c2 = PRUnichar(*(unsigned char*)s2++); That's going to mess up on one endianness, isn't it? I think you should cast after dereferencing.

*sigh* yeah, probably. It's exactly what the old code did, but I don't care either way. how is THIS one?

Comment on attachment 75282 [details] [diff] [review] Fix up bufferRoutines v2.4 sr=jag

Comment on attachment 75282 [details] [diff] [review] Fix up bufferRoutines v2.4 Oh, s2 is a char*. So the old code was ok. But it's fine this way too. r=dbaron.

Comment on attachment 75282 [details] [diff] [review] Fix up bufferRoutines v2.4 a=asa (on behalf of drivers) for checkin to the 1.0 trunk

yay! patch is in. this bug is DEAD!