Bug 171396 – Add WebCore::protocolIsJavaScript(StringView)

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Daniel Bates]

As suggested by Sam Weinig in bug 170925, comment 2, we should add an overload of WebCore::protocolIsJavaScript() that takes a StringView to avoid the need to perform a type conversion from StringView to String.

[Daniel Bates]

Created attachment 308455 [details]
Patch

[Alex Christensen]

Comment on attachment 308455 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308455&action=review

r=me
I think we should get rid of all instances of protocolIs that take Strings instead of URLs.  They're probably a marginal performance benefit, but we should really parse URLs rather than maintain a separate code path that looks enough like the URLParser.

> Source/WebCore/platform/URL.h:315
> +bool protocolIsJavaScript(StringView url);

Did you want to use this somewhere?  This patch only changes URL.h and URL.cpp.

[Daniel Bates]

(In reply to Alex Christensen from comment #2)
> [...]
> I think we should get rid of all instances of protocolIs that take Strings
> instead of URLs.  They're probably a marginal performance benefit, but we
> should really parse URLs rather than maintain a separate code path that
> looks enough like the URLParser.
> 

We should investigate this. We should do this in the separate bug. 

> > Source/WebCore/platform/URL.h:315
> > +bool protocolIsJavaScript(StringView url);
> 
> Did you want to use this somewhere?  This patch only changes URL.h and
> URL.cpp.

As I mentioned in the ChangeLog entry, I plan to use this function in the patch for bug #170925.

[Daniel Bates]

Comment on attachment 308455 [details]
Patch

Clearing flags on attachment: 308455

Committed r215949: <http://trac.webkit.org/changeset/215949>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

(In reply to Alex Christensen from comment #2)
> I think we should get rid of all instances of protocolIs that take Strings
> instead of URLs. They're probably a marginal performance benefit, but we
> should really parse URLs rather than maintain a separate code path that
> looks enough like the URLParser.

I am not sure I agree.

If the performance benefit is tiny, then that’s OK.

Parsing an entire URL to do a properly-case-insensitive prefix check seems like massive overkill to me, so we should be sure a performance benefit is not there before dropping. I think there is at *least* an order of magnitude difference in the costs of the two operations.