Summary: | Add a method for schemes to be registered as supporting cache partitioning | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Vicki Pfau <jeffrey+webkit> | ||||||
Component: | Page Loading | Assignee: | Vicki Pfau <jeffrey+webkit> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | mjs, sam | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Vicki Pfau
2014-01-27 17:22:59 PST
Created attachment 222388 [details]
Patch
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); Created attachment 222509 [details]
Patch
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 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? (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. Committed r163121: <http://trac.webkit.org/changeset/163121> |