212272 - Switch some Substring users over to String(Begins|Ends)With

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[jag (Peter Annema)]

Comment 1

[jag (Peter Annema)]

Attachment: Switch 'em over

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 127424 [details] [diff] [review]
Switch 'em over

sr=me

Comment 3

[[e-mail]]

jag asked me on IRC if I'd mind reviewing this... so here you go ;)

>Index: content/base/src/nsFrameLoader.cpp
>       nsAutoString::const_iterator iter(start);
>       iter.advance(7);
> 
>-      const nsAString& valuePiece = Substring(start, iter);
>-
>-      if (valuePiece.Equals(NS_LITERAL_STRING("content")) &&
>+      if (Substring(start, iter).Equals(NS_LITERAL_STRING("content")) &&
>           (iter == end || *iter == '-')) {
>         isContent = PR_TRUE;
>       }

Can we use nsASingleFragmentCString::const_char_iterator here, and just say 
|nsASingleFragmentCString::const_char_iterator iter(start + 7)|? (We can use 
const_char_iterator also on #407 of that file). Also, wanna kill that |if| and 
just assign into isContent directly?

>Index: content/base/src/nsHTMLContentSerializer.cpp
>+    if (aTagName == nsHTMLAtoms::br && attrName == nsHTMLAtoms::type &&
>+        StringBeginsWith(valueStr, NS_LITERAL_STRING("_moz"))) {

Does declaring the NS_LITERAL_STRING temporary inside the loop, have any perf 
cost? (It used to be an NS_NAMED_LITERAL_STRING declared outside.)

>Index: content/html/document/src/nsHTMLContentSink.cpp
>   nsCaseInsensitiveStringComparator ignoreCase;
>-  if (Substring(name, 0, 9).Equals(NS_LITERAL_STRING("<!DOCTYPE"), 
ignoreCase)) {
>+  if (StringBeginsWith(name, NS_LITERAL_STRING("<!DOCTYPE"), ignoreCase)) {
>     name.Cut(0, 9);
>-  } else if (Substring(name, 0, 7).Equals(NS_LITERAL_STRING("DOCTYPE"), 
ignoreCase)) {
>+  } else if (StringBeginsWith(name, NS_LITERAL_STRING("DOCTYPE"), 
ignoreCase)) {

kill |ignoreCase| and put it inside the StringBeginsWith() call?

>Index: modules/plugin/base/src/nsPluginsDirUnix.cpp
>-    int len = filename.Length();
>-    nsCAutoString suffix (LOCAL_PLUGIN_DLL_SUFFIX);
>-    int slen = suffix.Length();
>-    if (len > slen && suffix.Equals(Substring(filename,len-slen,slen)))
>+    NS_NAMED_LITERAL_CSTRING(dllSuffix, LOCAL_PLUGIN_DLL_SUFFIX);
>+    if (filename.Length() > dllSuffix.Length() &&
>+        StringEndsWith(filename, dllSuffix))

we don't need the |filename.Length() > dllSuffix.Length()| check, right?

>-    suffix.Assign(LOCAL_PLUGIN_DLL_ALT_SUFFIX);
>-    slen = suffix.Length();
>-    if (len > slen && suffix.Equals(Substring(filename,len-slen,slen)))
>+    NS_NAMED_LITERAL_CSTRING(dllAltSuffix, LOCAL_PLUGIN_DLL_ALT_SUFFIX);
>+    if (filename.Length() > dllAltSuffix.Length() &&
>+        StringEndsWith(filename, dllAltSuffix))

same here?

>Index: rdf/base/src/rdfutil.cpp
>+    if (StringBeginsWith(uri, NS_LITERAL_STRING("urn:")) ||
>+        StringBeginsWith(uri, NS_LITERAL_STRING("chrome:")) ||
>+        StringBeginsWith(uri, NS_LITERAL_STRING("nc:"),
>+                         nsCaseInsensitiveStringComparator())) {
>         return PR_FALSE;
>      }
>      return PR_TRUE;

just |return !StringBeginsWith(...| ?

>Index: xpinstall/src/nsXPITriggerInfo.h
>+    PRBool  IsFileURL() { return StringBeginsWith(mURL, NS_LITERAL_STRING
("file:/")); }

totally unrelated drive-by nit: make this method |const|? :)

with that, r=dwitte. I'm wondering whether we should consider making the impls 
of StringBeginsWith/StringEndsWith inline wrappers around Substring, rather 
than out-of-line... perhaps dbaron has an opinion on this?

Comment 4

[jag (Peter Annema)]

>>Index: content/base/src/nsFrameLoader.cpp
>>       nsAutoString::const_iterator iter(start);
>>       iter.advance(7);
>>
>>-      const nsAString& valuePiece = Substring(start, iter);
>>-
>>-      if (valuePiece.Equals(NS_LITERAL_STRING("content")) &&
>>+      if (Substring(start, iter).Equals(NS_LITERAL_STRING("content")) &&
>>           (iter == end || *iter == '-')) {
>>         isContent = PR_TRUE;
>>       }
>
>Can we use nsASingleFragmentCString::const_char_iterator here

Sure. Actually, I'd prefer to go with nsAutoString::const_char_iterator to match
the knowledge that we're working on a nsAutoString here.

>>Index: content/base/src/nsHTMLContentSerializer.cpp
>>+    if (aTagName == nsHTMLAtoms::br && attrName == nsHTMLAtoms::type &&
>>+        StringBeginsWith(valueStr, NS_LITERAL_STRING("_moz"))) {
>
>Does declaring the NS_LITERAL_STRING temporary inside the loop, have any perf
>cost? (It used to be an NS_NAMED_LITERAL_STRING declared outside.)

I'll put it outside the loop again, nice catch.

>>Index: content/html/document/src/nsHTMLContentSink.cpp
>>   nsCaseInsensitiveStringComparator ignoreCase;
>>-  if (Substring(name, 0, 9).Equals(NS_LITERAL_STRING("<!DOCTYPE"),
>ignoreCase)) {
>>+  if (StringBeginsWith(name, NS_LITERAL_STRING("<!DOCTYPE"), ignoreCase)) {
>>     name.Cut(0, 9);                                        
>>-  } else if (Substring(name, 0, 7).Equals(NS_LITERAL_STRING("DOCTYPE"),
>ignoreCase)) {
>>+  } else if (StringBeginsWith(name, NS_LITERAL_STRING("DOCTYPE"),
>ignoreCase)) {
>
>kill |ignoreCase| and put it inside the StringBeginsWith() call?

Could do. I wonder if the compiler would generate more code if I did that.

>>Index: modules/plugin/base/src/nsPluginsDirUnix.cpp
>>-    int len = filename.Length();
>>-    nsCAutoString suffix (LOCAL_PLUGIN_DLL_SUFFIX);
>>-    int slen = suffix.Length();
>>-    if (len > slen && suffix.Equals(Substring(filename,len-slen,slen)))
>>+    NS_NAMED_LITERAL_CSTRING(dllSuffix, LOCAL_PLUGIN_DLL_SUFFIX);
>>+    if (filename.Length() > dllSuffix.Length() &&
>>+        StringEndsWith(filename, dllSuffix))
>
>we don't need the |filename.Length() > dllSuffix.Length()| check, right?

We actually do. It's slightly different from the check we do inside Substring
(filename.Length() >= dllSuffix.Length()), we want to make sure that the
filename is of the form "X.so", not just ".so". I'll add a comment for this.

>>Index: rdf/base/src/rdfutil.cpp
>>+    if (StringBeginsWith(uri, NS_LITERAL_STRING("urn:")) || 
>>+        StringBeginsWith(uri, NS_LITERAL_STRING("chrome:")) ||
>>+        StringBeginsWith(uri, NS_LITERAL_STRING("nc:"),
>>+                         nsCaseInsensitiveStringComparator())) {
>>         return PR_FALSE;
>>      }
>>      return PR_TRUE;
>
>just |return !StringBeginsWith(...| ?

Maybe, I thought about it but thought it'd make the code less grokable. I'll
just write it out and see.

>>Index: xpinstall/src/nsXPITriggerInfo.h
>>+    PRBool  IsFileURL() { return StringBeginsWith(mURL, NS_LITERAL_STRING
>("file:/")); }
>
>totally unrelated drive-by nit: make this method |const|? :)

I'll ask dbradley :-)

>>with that, r=dwitte. I'm wondering whether we should consider making the impls
>of StringBeginsWith/StringEndsWith inline wrappers around Substring, rather
>than out-of-line... perhaps dbaron has an opinion on this?

We could perhaps do that, but we might generate more code that way.

cc'ing dbaron for his input.

Comment 5

[jag (Peter Annema)]

Attachment: Recommended changes made, diff contains only those files

Comment 6

[jag (Peter Annema)]

Comment on attachment 127424 [details] [diff] [review]
Switch 'em over

Actually, there were a few cases where I incorrectly replaced:

  if (a.Length() > b.Length() &&
      Substring(a, 0, b.Length()) == b)

with

  if (StringBeginsWith(a, b))

losing the requirement that |a| not only starts with |b|, but also has at least
one more character following |b|. I've put those checks back in to keep the old
logic.

Comment 7

[jag (Peter Annema)]

Checked in.