Bug 127739 – Add a method for schemes to be registered as supporting cache partitioning

• Component: Page Loading
• Status: RESOLVED FIXED

[Vicki Pfau]

Currently, only HTTP and HTTPS support cache partitioning. We should be able to register additional schemes as supporting cache partitioning at run time.

<rdar://problem/15255247>

[Vicki Pfau]

Created attachment 222388 [details]
Patch

[Darin Adler]

Comment on attachment 222388 [details]
Patch

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

> Source/WebCore/page/SecurityOrigin.cpp:448
> +    if (m_protocol == "http" || m_protocol == "https")
> +        return host();

Would be nice if this used an isHTTPFamily helper function.

> Source/WebCore/platform/SchemeRegistry.cpp:168
> +static URLSchemesMap& CachePartitioningSchemes()

Function name should start with a lowercase letter.

> Source/WebCore/platform/SchemeRegistry.cpp:170
> +    DEFINE_STATIC_LOCAL(URLSchemesMap, schemes, ());

In new code, the way to write this is:

    static NeverDestroyed<URLSchemesMap> schemes;

There’s no need to use the DEFINE_STATIC_LOCAL macro.

> Source/WebCore/platform/SchemeRegistry.h:98
> +    static void registerURLSchemeAsCachePartitioned(const String& scheme);
> +    static bool schemeShouldPartitionCache(const String& scheme);

Why does one of these have “URL” in its name but not the other?

> Source/WebKit2/WebProcess/WebProcess.cpp:332
> +    for (size_t i = 0; i < parameters.urlSchemesRegisteredAsCachePartitioned.size(); ++i)
> +        registerURLSchemeAsCORSEnabled(parameters.urlSchemesRegisteredAsCachePartitioned[i]);

Should use a C++11 for loop here:

    for (auto& scheme : parameters.urlSchemesRegisteredAsCachePartitioned)
        registerURLSchemeAsCORSEnabled(scheme);

[Vicki Pfau]

Created attachment 222509 [details]
Patch

[Vicki Pfau]

Uploaded a new patch just to make sure I've addressed Darin's nitpicks, and would like a WK2 sign-off from an owner.

[Darin Adler]

Comment on attachment 222509 [details]
Patch

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

> Source/WebCore/page/SecurityOrigin.h:210
> +    // This method checks that the scheme for this origin is an HTTP-family
> +    // scheme, e.g. HTTP and HTTPS.
> +    bool isHTTPFamily() const { return m_protocol == "http" || m_protocol == "https"; }

The mix in terminology here between protocol and scheme is unnecessarily confusing.

Why is this function public instead of private?

[Vicki Pfau]

(In reply to comment #5)
> (From update of attachment 222509 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=222509&action=review
> 
> > Source/WebCore/page/SecurityOrigin.h:210
> > +    // This method checks that the scheme for this origin is an HTTP-family
> > +    // scheme, e.g. HTTP and HTTPS.
> > +    bool isHTTPFamily() const { return m_protocol == "http" || m_protocol == "https"; }
> 
> The mix in terminology here between protocol and scheme is unnecessarily confusing.

I'll change this to protocol before committing it.

> Why is this function public instead of private?

Is there any reason it should be private? It might be useful in the future for code that isn't in SecurityOrigin. Or it might not be. It doesn't seem like code that makes more sense to be private to me.

[Vicki Pfau]

Committed r163121: <http://trac.webkit.org/changeset/163121>