[webkit-reviews] review granted: [Bug 205157] Ignore URL host for schemes that are not using host information : [Attachment 386390] Removing host inside Document class

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted youenn fablet <youennf at gmail.com>'s
request for review:
Bug 205157: Ignore URL host for schemes that are not using host information
https://bugs.webkit.org/show_bug.cgi?id=205157

Attachment 386390: Removing host inside Document class

https://bugs.webkit.org/attachment.cgi?id=386390&action=review




--- Comment #7 from Darin Adler <darin at apple.com> ---
Comment on attachment 386390
  --> https://bugs.webkit.org/attachment.cgi?id=386390
Removing host inside Document class

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

> Source/WebCore/dom/Document.cpp:3233
> +    if (SecurityOrigin::shouldIgnoreHost(m_url) && !m_url.host().isEmpty())
> +	   m_url.setHost({ });

I think this should do something more like setHostAndPort instead of setHost. I
don’t think it makes sense to leave a port number behind. But setHostAndPort
doesn’t seem to handle the empty string correctly. So might need to change it
in the URL class or even add a new removeHostAndPort function. If we did add a
new removeHostAndPort function, we could optimize it to quickly do nothing when
it’s already empty, and then we wouldn’t need the host().isEmpty() check here.

> Source/WebCore/page/DOMWindow.h:179
> +    WEBCORE_EXPORT Location& location();

We also have WEBCORE_TESTSUPPORT_EXPORT. Not sure if we are using it
consistently.

> Source/WebCore/page/Location.h:55
> +    WEBCORE_EXPORT String host() const;

Ditto.

> Source/WebCore/page/SecurityOrigin.cpp:61
> +    return url.protocolIs("data") || url.protocolIs("about") ||
url.protocolIs("file");

How did you determine this set of protocols?

Could use protocolIsAbout() and protocolIsData() rather than writing those two
strings out.

Could consider using isLocalFile() for the "file" one, but not sure if that’s
the correct abstraction.

I don’t understand how we determined this list. What about "blob"? What about
"javascript"? How are they different from "data"?