Bug 72342 – Refactor SecurityOrigin::create to be easier to understand

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

Refactor SecurityOrigin::create to be easier to understand

[Adam Barth]

Created attachment 115076 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 115076 [details]
Patch

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 ""?

[Adam Barth]

Comment on attachment 115076 [details]
Patch

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

>> Source/WebCore/page/SecurityOrigin.cpp:109
>> +    if (SchemeRegistry::shouldTreatURLSchemeAsNoAccess(protocol))
>> +        return true;
> 
> Why do we lower the protocol here?  Why isn't SchemeRegistry case agnostic?

It's not case agnostic because it uses a hash map lookup.  It could lower the scheme itself, but that would be slow because it often requires a malloc.

>> 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 ""?

We can't use String::empty because this class is used on multiple threads.  I'm not sure whether we can get away with null strings.

Currently, this constructor matches the other constructor, which is very explicit about converting null strings to empty strings.  I'm inclined to experiment with changing these to null in a future patch.

[Adam Barth]

Created attachment 115690 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 115690 [details]
Patch

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

OK.

> Source/WebCore/page/SecurityOrigin.cpp:196
> +        if (url.protocolIs("file")) {
> +            // Unfortunately, we can't represent all unique origins exactly
> +            // the same way because we need to produce a quirky database
> +            // identifier for file URLs due to persistent storage in some
> +            // embedders of WebKit.
> +            origin->m_needsDatabaseIdentifierQuirkForFiles = true;
> +        }

Can't we set this only for origins that matter to the database world?

[Adam Barth]

Committed r100691: <http://trac.webkit.org/changeset/100691>