Bug 208893

Created attachment 393191 [details]
Patch

Comment on attachment 393191 [details]
Patch

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

> Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:483
> +        || startsWithLettersIgnoringASCIICase(domain.string(), "data:"));

I don't think this will work, because RegistrableDomains are just things like "apple.com" or "webkit.org". I don't think we retain the protocol in the class.

Comment on attachment 393191 [details]
Patch

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

>> Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:483
>> +        || startsWithLettersIgnoringASCIICase(domain.string(), "data:"));
> 
> I don't think this will work, because RegistrableDomains are just things like "apple.com" or "webkit.org". I don't think we retain the protocol in the class.

Correct. Can this be tested somehow?

Created attachment 393196 [details]
Patch

(In reply to Brent Fulgham from comment #4)
> Created attachment 393196 [details]
> Patch

Looks good,

Unofficial r=me

Comment on attachment 393196 [details]
Patch

Clearing flags on attachment: 393196

Committed r258247: <https://trac.webkit.org/changeset/258247>

All reviewed patches have been landed.  Closing bug.

Comment on attachment 393196 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:3098
> +    return requestURL.protocolIsAbout() || requestURL.protocolIsData() || requestURL.protocolIsBlob() || requestURL.isLocalFile();

Would !requestURL.protocolIsInHTTPFamily() have worked? Or do you expect there are other non HTTP-family protocols that should not be treated as AppBound? I am asking because !requestURL.protocolIsInHTTPFamily() would be more concise and more efficient.
Technically, I think registrable domains only make sense for HTTP-Family protocol anyway.

> Source/WebKit/UIProcess/WebPageProxy.cpp:3105
> +        if (isNavigatingToAppBoundDomain == NavigatingToAppBoundDomain::No && shouldBeTreatedAsAppBound(requestURL))

This does not seems ideally to me. Am I right that for those special protocols you are still going to extract the registrable domain and check if it is part of app bounds domains, and then ignore the result of that work and simply set that value to false?
I think a much better way would be to call setIsNavigatingToAppBoundDomain(NavigatingToAppBoundDomain::No) right away for non-http URLs instead of doing the whole registrable check logic unnecessarily.

Comment on attachment 393196 [details]
Patch

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

> Source/WebKit/ChangeLog:3
> +        Some common domains should always be App-bound domains

Also, the title talks about domains but this has nothing to do with domains, it is really about non-http protocols.

> Source/WebKit/ChangeLog:9
> +        Some domains, like about:blank and pages loaded from files should

Same, this sentence does not make sense.

(In reply to Chris Dumez from comment #8)
> Comment on attachment 393196 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=393196&action=review
> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:3098
> > +    return requestURL.protocolIsAbout() || requestURL.protocolIsData() || requestURL.protocolIsBlob() || requestURL.isLocalFile();
> 
> Would !requestURL.protocolIsInHTTPFamily() have worked? Or do you expect
> there are other non HTTP-family protocols that should not be treated as
> AppBound? I am asking because !requestURL.protocolIsInHTTPFamily() would be
> more concise and more efficient.
> Technically, I think registrable domains only make sense for HTTP-Family
> protocol anyway.
> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:3105
> > +        if (isNavigatingToAppBoundDomain == NavigatingToAppBoundDomain::No && shouldBeTreatedAsAppBound(requestURL))
> 
> This does not seems ideally to me. Am I right that for those special
> protocols you are still going to extract the registrable domain and check if
> it is part of app bounds domains, and then ignore the result of that work
> and simply set that value to false?
> I think a much better way would be to call
> setIsNavigatingToAppBoundDomain(NavigatingToAppBoundDomain::No) right away
> for non-http URLs instead of doing the whole registrable check logic
> unnecessarily.


Late but agree this should be changed. Tracking this in rdar://problem/60875376.