Bug 127739 - Add a method for schemes to be registered as supporting cache partitioning
Summary: Add a method for schemes to be registered as supporting cache partitioning
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Vicki Pfau
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-01-27 17:22 PST by Vicki Pfau
Modified: 2014-01-30 14:58 PST (History)
2 users (show)

See Also:


Attachments
Patch (15.52 KB, patch)
2014-01-27 17:36 PST, Vicki Pfau
no flags Details | Formatted Diff | Diff
Patch (16.29 KB, patch)
2014-01-28 15:31 PST, Vicki Pfau
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vicki Pfau 2014-01-27 17:22:59 PST
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>
Comment 1 Vicki Pfau 2014-01-27 17:36:54 PST
Created attachment 222388 [details]
Patch
Comment 2 Darin Adler 2014-01-28 09:56:11 PST
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);
Comment 3 Vicki Pfau 2014-01-28 15:31:38 PST
Created attachment 222509 [details]
Patch
Comment 4 Vicki Pfau 2014-01-28 15:32:23 PST
Uploaded a new patch just to make sure I've addressed Darin's nitpicks, and would like a WK2 sign-off from an owner.
Comment 5 Darin Adler 2014-01-29 10:04:24 PST
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?
Comment 6 Vicki Pfau 2014-01-29 13:50:28 PST
(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.
Comment 7 Vicki Pfau 2014-01-30 14:58:11 PST
Committed r163121: <http://trac.webkit.org/changeset/163121>