Bug 211033 – Remove implicit URL->String conversion operators

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Remove implicit URL->String conversion operators

[Alex Christensen]

Created attachment 397595 [details]
Patch

[Alex Christensen]

Created attachment 397597 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Alex Christensen]

Created attachment 397600 [details]
Patch

[Alex Christensen]

Created attachment 397602 [details]
Patch

[Darin Adler]

Comment on attachment 397600 [details]
Patch

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

> Source/WebCore/html/DOMURL.cpp:61
> +    return create(url, base.href().string());

This is not good, and easy to fix. Resolving against a base requires a parsed URL; this re-parses for no reason. Why not rearrange the code so that doesn’t happen? Could do it by having another overload of the create function or just write this function out like the one just below it.

> Source/WebCore/html/HTMLPlugInElement.cpp:367
> +        type = mimeTypeFromDataURL(url.string());

All the callers of the mimeTypeFromDataURL function have a URL. Maybe we should just change it to take const URL& instead of StringView instead of changing all of them.

> Source/WebCore/html/URLUtils.h:89
> -    if (WTF::protocolIsJavaScript(href()))
> +    if (href().protocolIsJavaScript())
>          return "javascript:"_s;

Seems like we don’t need this at all. If URL::protocol properly returns "javascript", then the code below will work. If it doesn’t then it seems likely protocolIsJavaScript will give us false negatives.

I think that either this code and should be removed, or it needs to keep using the protocolIsJavaScript function that works on a string. See also my comments about the future in bug 211007.

This conflicts with my changes in bug 211025; no big deal, but I would like you to look at that one at some point.

> Source/WebCore/html/URLUtils.h:111
> -    setHref(url);
> +    setHref(url.string());

This wouldn’t be needed after my changes in bug 211025; hope you get a chance to check that out.

> Source/WebCore/html/URLUtils.h:125
> -    setHref(url);
> +    setHref(url.string());

Ditto.

> Source/WebCore/page/Location.cpp:139
> +    return setLocation(activeWindow, firstWindow, url.string());

Seems unfortunate to parse, convert to a string, and then reparse. If this is guaranteed to be a complete URL and not a relative URL piece, then we could make a setLocation overload that takes a URL and use it here.

> Source/WebCore/page/Location.cpp:149
> +    return setLocation(activeWindow, firstWindow, url.string());

Ditto.

> Source/WebCore/page/Location.cpp:159
> +    return setLocation(activeWindow, firstWindow, url.string());

Ditto.

> Source/WebCore/page/Location.cpp:179
> +    return setLocation(activeWindow, firstWindow, url.string());

Ditto.

> Source/WebCore/page/Location.cpp:189
> +    return setLocation(activeWindow, firstWindow, url.string());

Ditto.

> Source/WebCore/page/Location.cpp:209
> +    return setLocation(activeWindow, firstWindow, url.string());

Ditto.

> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:345
> +    NSURL *targetURL = [NSURL URLWithString:_positionInformation->url.string()];

Should consider whether we can change this to:

    NSURL *targetURL = _positionInformation->url;

It would change the semantics of how the URL is created to use the NSURL * conversion in the URL class, but maybe that’s good.

> Source/WebKit/UIProcess/ios/WKGeolocationProviderIOS.mm:211
> +            WebKit::decidePolicyForGeolocationRequestFromOrigin(request.frameInfo.securityOrigin.securityOrigin().ptr(), request.frameInfo.request.url().string(), policyListener.get(), request.view.get());

I think we should change decidePolicyForGeolocationRequestFromOrigin to take a const URL& instead of a String.

[Darin Adler]

Comment on attachment 397602 [details]
Patch

New changes look good; all my comments should apply to this version as well as the one I reviewed.

[Alex Christensen]

(In reply to Darin Adler from comment #6)
> > Source/WebCore/html/HTMLPlugInElement.cpp:367
> > +        type = mimeTypeFromDataURL(url.string());
> 
> All the callers of the mimeTypeFromDataURL function have a URL. Maybe we
> should just change it to take const URL& instead of StringView instead of
> changing all of them.

HTMLPlugInImageElement has only a String when it calls mimeTypeFromDataURL.  I'm going to keep it taking a StringView for now.

> > Source/WebCore/page/Location.cpp:139
> > +    return setLocation(activeWindow, firstWindow, url.string());
> 
> Seems unfortunate to parse, convert to a string, and then reparse. If this
> is guaranteed to be a complete URL and not a relative URL piece, then we
> could make a setLocation overload that takes a URL and use it here.

This is a String because it is given to Document::completeURL, which uses the string as a relative URL.

[Alex Christensen]

Created attachment 399856 [details]
Patch

[Darin Adler]

Comment on attachment 399856 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKPDFView.mm:532
> +        (NSString *)kUTTypeUTF8PlainText : (NSString *)_positionInformation.url.string(),

Looking at this makes me realize that we should add a createNSString function on WTF::String so we can make a case like this work without autorelease and without a type cast. I’d like to deprecate the operator NSString * on WTF::String and use createNSString instead.

[Alex Christensen]

Created attachment 399866 [details]
Patch

[Alex Christensen]

Created attachment 399875 [details]
Patch

[Alex Christensen]

Created attachment 399890 [details]
Patch

[Darin Adler]

Comment on attachment 399890 [details]
Patch

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

> Source/WebKitLegacy/win/Plugins/PluginStream.cpp:146
> -    if (protocolIsJavaScript(responseURL))
> +    if (responseURL.protocolIsJavaScript())

Is this change safe? I thought you told me that invalid URLs would return false from WebCore::URL::protocolIsJavaScript, but would return true from WebCore::protocolIsJavaScript.

> Source/WebKitLegacy/win/Plugins/PluginView.cpp:106
> -    if (!protocolIsJavaScript(url))
> +    if (!url.protocolIsJavaScript())

Ditto.

[Alex Christensen]

Yes, if you look at URL::protocolIsJavaScript you see it is no change in behavior.  Detecting javascript URLs needs special behavior and that function takes it into account, and URL::protocolIs asserts that the protocol is not "javascript" to enforce this.

[EWS]

Committed r261968: <https://trac.webkit.org/changeset/261968>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399890 [details].

[Radar WebKit Bug Importer]

<rdar://problem/63467643>