[webkit-reviews] review denied: [Bug 50182] Introduce the notion of a "display-isolated" URL scheme for use by Chrome-internal URLs : [Attachment 75060] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Adam Barth <abarth at webkit.org>'s
request for review:
Bug 50182: Introduce the notion of a "display-isolated" URL scheme for use by
Chrome-internal URLs
https://bugs.webkit.org/show_bug.cgi?id=50182

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=75060&action=review

I have enough questions about this that I’m going to say review- for now, but
it may just be my misunderstanding rather than defects in the patch.

> WebCore/page/SecurityOrigin.cpp:-313
> -    if (!SchemeRegistry::shouldTreatURLAsLocal(url.string()))
> -	   return true;

Since you’re changing this policy, how do we maintain compatibility with
existing Mac OS X applications using the public API, +[WebKit
registerURLSchemeAsLocal:], and expecting this behavior?

> WebCore/platform/SchemeRegistry.cpp:-95
> -    if (scheme == "file")
> -	   return;
> -#if PLATFORM(MAC)
> -    if (scheme == "applewebdata")
> -	   return;
> -#endif

Why did you remove this?

> WebCore/platform/SchemeRegistry.cpp:-134
> -    // This avoids an allocation of another String and the HashSet
contains()
> -    // call for the file: and http: schemes.
> -    if (scheme.length() == 4) {
> -	   const UChar* s = scheme.characters();
> -	   if (s[0] == 'h' && s[1] == 't' && s[2] == 't' && s[3] == 'p')
> -	       return false;
> -	   if (s[0] == 'f' && s[1] == 'i' && s[2] == 'l' && s[3] == 'e')
> -	       return true;
> -    }

What’s the rationale for removing this fast path?

> WebCore/platform/SchemeRegistry.cpp:116
> -    if (scheme.isEmpty())
> -	   return false;
> -
> -    return WebCore::localURLSchemes().contains(scheme);
> +    return localURLSchemes().contains(scheme);

Why isn’t the isEmpty change needed any more? Does function still work for both
null strings and empty strings? If not, did you audit the callers or do
something else?

> WebKit/chromium/public/WebSecurityPolicy.h:53
> +    // Registers a URL scheme to be treated as display-isolated.  This means


Double space after period here. Are you a conscientious objector?

> WebKit/chromium/public/WebSecurityPolicy.h:54
> +    // that pages cannot dispaly these URLs unless they from the same
scheme.

Typo: “dispaly”
Typo: “they from”