Created attachment 269774 [details] Patch

Comment on attachment 269774 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=269774&action=review Name should be startsWithLettersIgnoringASCIICase, plural. > Source/WTF/wtf/text/StringCommon.h:562 > +template<typename StringClass> bool equalLettersIgnoringASCIICaseCommonWithLength(const StringClass& string, const char* lowercaseLetters, unsigned length) I am not sure this is a good name for this helper. This checks if a prefix of string is equal assuming we already know length is <= string.length(), so it’s not a general purpose function so needs a more specific name. > Source/WTF/wtf/text/StringCommon.h:582 > + return equalLettersIgnoringASCIICaseCommonWithLength(string, lowercaseLetters, length); Because the function this calls is not marked inline, this is likely adding a level function call overhead to every call of equalLettersIgnoringASCIICase. We don’t want to do that! Need to mark the function this is calling inline. > Source/WTF/wtf/text/StringImpl.h:1200 > +template<unsigned length> inline bool startWithLettersIgnoringASCIICase(const StringImpl& string, const char (&lowercaseLetters)[length]) > +{ > + if (length == 1) > + return true; > + if (string.length() < (length - 1)) > + return false; > + return equalLettersIgnoringASCIICaseCommonWithLength(string, lowercaseLetters, length - 1); > +} I think this should work the same way as equalLettersIgnoringASCIICaseCommon; don’t actually use the length, chose code size over speed (for now). Call a version that just takes a pointer and does strlen.

Created attachment 269863 [details] Updated according review

Created attachment 269876 [details] Fully aligning with equalLetters implementation

(In reply to comment #2) > Comment on attachment 269774 [details] > Patch Thanks for the review. I fully aligned with equalLettersIgnoringASCIICase for consistency. I changed the new routine to hasPrefixWithLettersIgnoringASCIICaseCommon

Comment on attachment 269876 [details] Fully aligning with equalLetters implementation Excellent. We will want to follow this patch up by using this everywhere that the member startsWith(x, false) is currently used with a constant string that includes the legal characters. WebCore/css/CSSParser.cpp: if (!string.startsWith("translate", false)) WebCore/css/CSSFontFaceSrcValue.cpp: if (!m_resource.startsWith("data:", false) && m_resource.endsWith(".eot", false)) WebCore/css/CSSParser.cpp: if (!string.startsWith("translate", false)) WebCore/dom/DOMImplementation.cpp: return feature.startsWith("http://www.w3.org/tr/svg11/feature#", false) WebCore/dom/DOMImplementation.cpp: if (feature.startsWith("http://www.w3.org/TR/SVG", false) WebCore/dom/DOMImplementation.cpp: || feature.startsWith("org.w3c.dom.svg", false) WebCore/dom/DOMImplementation.cpp: || feature.startsWith("org.w3c.svg", false)) { WebCore/html/HTMLObjectElement.cpp: if (equalLettersIgnoringASCIICase(metaElement.name(), "generator") && metaElement.content().startsWith("Mac OS X Server Web Services Server", false)) WebCore/html/HTMLObjectElement.cpp: if (MIMETypeRegistry::isJavaAppletMIMEType(serviceType()) && fastGetAttribute(classidAttr).startsWith("java:", false)) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("+//Silmaril//dtd html Pro v0r11 19970101//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//AdvaSoft Ltd//DTD HTML 3.0 asWedit + extensions//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//AS//DTD HTML 3.0 asWedit + extensions//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//IETF//DTD HTML 2.0 Level 1//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//IETF//DTD HTML 2.0 Level 2//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//IETF//DTD HTML 2.0 Strict Level 1//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//IETF//DTD HTML 2.0 Strict Level 2//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//IETF//DTD HTML 2.0 Strict//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//IETF//DTD HTML 2.0//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//IETF//DTD HTML 2.1E//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//IETF//DTD HTML 3.0//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//IETF//DTD HTML 3.2 Final//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//IETF//DTD HTML 3.2//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//IETF//DTD HTML 3//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//IETF//DTD HTML Level 0//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//IETF//DTD HTML Level 1//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//IETF//DTD HTML Level 2//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//IETF//DTD HTML Level 3//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//IETF//DTD HTML Strict Level 0//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//IETF//DTD HTML Strict Level 1//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//IETF//DTD HTML Strict Level 2//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//IETF//DTD HTML Strict Level 3//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//IETF//DTD HTML Strict//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//IETF//DTD HTML//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//Metrius//DTD Metrius Presentational//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//Microsoft//DTD Internet Explorer 2.0 HTML Strict//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//Microsoft//DTD Internet Explorer 2.0 HTML//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//Microsoft//DTD Internet Explorer 2.0 Tables//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//Microsoft//DTD Internet Explorer 3.0 HTML Strict//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//Microsoft//DTD Internet Explorer 3.0 HTML//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//Microsoft//DTD Internet Explorer 3.0 Tables//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//Netscape Comm. Corp.//DTD HTML//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//Netscape Comm. Corp.//DTD Strict HTML//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//O'Reilly and Associates//DTD HTML 2.0//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//O'Reilly and Associates//DTD HTML Extended 1.0//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//O'Reilly and Associates//DTD HTML Extended Relaxed 1.0//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//SoftQuad Software//DTD HoTMetaL PRO 6.0::19990601::extensions to HTML 4.0//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//SoftQuad//DTD HoTMetaL PRO 4.0::19971010::extensions to HTML 4.0//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//Spyglass//DTD HTML 2.0 Extended//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//SQ//DTD HTML 2.0 HoTMetaL + extensions//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//Sun Microsystems Corp.//DTD HotJava HTML//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//Sun Microsystems Corp.//DTD HotJava Strict HTML//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//W3C//DTD HTML 3 1995-03-24//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//W3C//DTD HTML 3.2 Draft//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//W3C//DTD HTML 3.2 Final//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//W3C//DTD HTML 3.2//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//W3C//DTD HTML 3.2S Draft//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//W3C//DTD HTML 4.0 Frameset//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//W3C//DTD HTML 4.0 Transitional//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//W3C//DTD HTML Experimental 19960712//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//W3C//DTD HTML Experimental 970421//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//W3C//DTD W3 HTML//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//W3O//DTD W3 HTML 3.0//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//WebTechs//DTD Mozilla HTML 2.0//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//WebTechs//DTD Mozilla HTML//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || (systemId.isEmpty() && publicId.startsWith("-//W3C//DTD HTML 4.01 Frameset//", false)) WebCore/html/parser/HTMLConstructionSite.cpp: || (systemId.isEmpty() && publicId.startsWith("-//W3C//DTD HTML 4.01 Transitional//", false))) { WebCore/html/parser/HTMLConstructionSite.cpp: if (publicId.startsWith("-//W3C//DTD XHTML 1.0 Frameset//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || publicId.startsWith("-//W3C//DTD XHTML 1.0 Transitional//", false) WebCore/html/parser/HTMLConstructionSite.cpp: || (!systemId.isEmpty() && publicId.startsWith("-//W3C//DTD HTML 4.01 Frameset//", false)) WebCore/html/parser/HTMLConstructionSite.cpp: || (!systemId.isEmpty() && publicId.startsWith("-//W3C//DTD HTML 4.01 Transitional//", false))) { WebCore/page/SecurityOrigin.cpp: if (!urlString.startsWith("feed", false)) WebCore/page/SecurityOrigin.cpp: return urlString.startsWith("feed://", false) WebCore/page/SecurityOrigin.cpp: || urlString.startsWith("feed:http:", false) || urlString.startsWith("feed:https:", false) WebCore/page/SecurityOrigin.cpp: || urlString.startsWith("feeds:http:", false) || urlString.startsWith("feeds:https:", false) WebCore/page/SecurityOrigin.cpp: || urlString.startsWith("feedsearch:http:", false) || urlString.startsWith("feedsearch:https:", false); WebCore/platform/MIMETypeRegistry.cpp: return mimeType.startsWith("application/x-java-applet", false) WebCore/platform/MIMETypeRegistry.cpp: || mimeType.startsWith("application/x-java-bean", false) WebCore/platform/MIMETypeRegistry.cpp: || mimeType.startsWith("application/x-java-vm", false); WebCore/platform/MIMETypeRegistry.cpp: if (mimeType.startsWith("text/", false)) WebCore/platform/graphics/MediaPlayer.cpp: && (parameters.type.startsWith("video/webm", false) || parameters.type.startsWith("video/x-flv", false))) WebCore/platform/network/curl/ResourceHandleManager.cpp: if (key.startsWith("x-", /* caseSensitive */ false)) WebCore/platform/network/curl/ResourceHandleManager.cpp: } else if (header.startsWith("HTTP", false)) { WebCore/xml/XMLHttpRequest.cpp: if (name.startsWith("proxy-", false)) WebCore/xml/XMLHttpRequest.cpp: if (name.startsWith("sec-", false)) WebCore/xml/parser/XMLDocumentParserLibxml2.cpp: if (urlString.startsWith("file:///", false) && urlString.endsWith("/etc/catalog", false)) WebCore/xml/parser/XMLDocumentParserLibxml2.cpp: if (urlString.startsWith("http://www.w3.org/TR/xhtml", false)) WebCore/xml/parser/XMLDocumentParserLibxml2.cpp: if (urlString.startsWith("http://www.w3.org/Graphics/SVG", false)) WebKit/win/Plugins/PluginDatabaseWin.cpp: if ((!filename.startsWith("np", false) || !filename.endsWith("dll", false)) && WebKit2/NetworkProcess/cache/NetworkCache.cpp: if (mimeType.startsWith("video/", /*caseSensitive*/ false)) WebKit2/NetworkProcess/cache/NetworkCache.cpp: if (mimeType.startsWith("audio/", /*caseSensitive*/ false)) Our goal will be to eliminate the boolean case sensitivity argument to startsWith entirely, soon.

Please note a couple points about this for future consideration: 1) Using non-member functions for string operations is forward looking; it lets us overload the same operations for non-class types such as "const char*" or "pointers plus lengths both as separate argument". So over time we should start moving these to non-member functions if they can be done efficiently without any special private access. 2) We normally overload for the following types: StringView, StringImpl&, StringImpl*, const String&, const char* (for all-ASCII strings). In some cases we also overload for ASCII literals, char (&)[length], and this is one of those. We can also overload for const CString& if it’s handy. In some cases we may also need to overload for const AtomicString&, AtomicStringImpl&, AtomicStringImpl*, and but this is usually not necessary. There are additional issues only for string operations that sometimes produce a new string and sometimes just return the original string. And additional issues for string operations that return offsets within a string. And additional issues for string operations that can operate on a substring (such as a find operation that takes a start character offset for where to begin searching). For some of these we may even chose to support the operation only on StringView rather than overloading.

Comment on attachment 269876 [details] Fully aligning with equalLetters implementation Clearing flags on attachment: 269876 Committed r198019: <http://trac.webkit.org/changeset/198019>

All reviewed patches have been landed. Closing bug.