[webkit-reviews] review denied: [Bug 72342] Refactor SecurityOrigin::create to be easier to understand : [Attachment 115076] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Adam Barth <abarth at webkit.org>'s
request for review:
Bug 72342: Refactor SecurityOrigin::create to be easier to understand
https://bugs.webkit.org/show_bug.cgi?id=72342

Attachment 115076: Patch
https://bugs.webkit.org/attachment.cgi?id=115076&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=115076&action=review


I'd like to see one more round.

> Source/WebCore/page/SecurityOrigin.cpp:84
> +    return KURL(ParsedURLString, decodeURLEscapeSequences(url.path()));

Why does this work?  Can you add a FIXME about what this should be?

> Source/WebCore/page/SecurityOrigin.cpp:104
> +    // For edge case URLs that were probably misparsed, make sure that the
origin is unique.
> +    if (schemeRequiresAuthority(effectiveURL) &&
effectiveURL.host().isEmpty())
> +	   return true;

Why do we do this?  Should this be a FIXME to remove this?  This smells bad...

> Source/WebCore/page/SecurityOrigin.cpp:109
> +    String protocol = effectiveURL.protocol().lower();
> +
> +    if (SchemeRegistry::shouldTreatURLSchemeAsNoAccess(protocol))
> +	   return true;

Why do we lower the protocol here?  Why isn't SchemeRegistry case agnostic?

> Source/WebCore/page/SecurityOrigin.cpp:117
> +	   // FIXME: Should we be using shouldUseInnerURL here?
> +	   // We originally did that check to avoid treating blob and
filesystem
> +	   // urls as unique when it is created from local file urls.

This FIXME is unclear.	Can you explain better what you're trying to say?

> Source/WebCore/page/SecurityOrigin.cpp:151
> +    : m_protocol("")
> +    , m_host("")
> +    , m_domain("")

Why should this be empty strings instead of null strings?  And what about using
String::empty instead of ""?