Bug 208893 - Some common domains should always be App-bound domains
Summary: Some common domains should always be App-bound domains
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kate Cheney
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-03-10 16:31 PDT by Kate Cheney
Modified: 2020-03-25 09:30 PDT (History)
5 users (show)

See Also:


Attachments
Patch (2.30 KB, patch)
2020-03-10 17:36 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch (2.21 KB, patch)
2020-03-10 18:10 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kate Cheney 2020-03-10 16:31:43 PDT
<rdar://problem/60280943>
Comment 1 Kate Cheney 2020-03-10 17:36:25 PDT
Created attachment 393191 [details]
Patch
Comment 2 Brent Fulgham 2020-03-10 17:39:52 PDT
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 3 Chris Dumez 2020-03-10 17:42:03 PDT
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?
Comment 4 Brent Fulgham 2020-03-10 18:10:32 PDT
Created attachment 393196 [details]
Patch
Comment 5 Kate Cheney 2020-03-10 18:14:29 PDT
(In reply to Brent Fulgham from comment #4)
> Created attachment 393196 [details]
> Patch

Looks good,

Unofficial r=me
Comment 6 WebKit Commit Bot 2020-03-10 19:00:21 PDT
Comment on attachment 393196 [details]
Patch

Clearing flags on attachment: 393196

Committed r258247: <https://trac.webkit.org/changeset/258247>
Comment 7 WebKit Commit Bot 2020-03-10 19:00:22 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Chris Dumez 2020-03-11 08:04:54 PDT
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 9 Chris Dumez 2020-03-11 08:07:23 PDT
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.
Comment 10 Kate Cheney 2020-03-25 09:30:03 PDT
(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.