Bug 210074 - Create a way to signal if the WKAppBoundDomains list is empty
Summary: Create a way to signal if the WKAppBoundDomains list is empty
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-04-06 14:25 PDT by Kate Cheney
Modified: 2020-04-06 17:11 PDT (History)
3 users (show)

See Also:


Attachments
Patch (11.01 KB, patch)
2020-04-06 14:40 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch (13.99 KB, patch)
2020-04-06 15:55 PDT, Kate Cheney
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-04-06 14:25:52 PDT
We want different behavior for In-App Browser Privacy if WKAppBoundDomains is empty. We should signal this when reading from the plist.
Comment 1 Radar WebKit Bug Importer 2020-04-06 14:26:29 PDT
<rdar://problem/61359228>
Comment 2 Kate Cheney 2020-04-06 14:40:47 PDT
Created attachment 395619 [details]
Patch
Comment 3 Chris Dumez 2020-04-06 14:44:39 PDT
Comment on attachment 395619 [details]
Patch

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

> Source/WebKit/Shared/NavigatingToAppBoundDomain.h:30
> +enum class NavigatingToAppBoundDomain : uint8_t { Yes, No, AppBoundDomainsEmpty };

What does AppBoundDomainsEmpty mean here? Why is it useful to differentiate? This seems like a weird name in this enum. Maybe it should be something like "Maybe" or "Unknown", which would fit better with "No", "Yes".
Comment 4 Kate Cheney 2020-04-06 14:46:53 PDT
(In reply to Chris Dumez from comment #3)
> Comment on attachment 395619 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=395619&action=review
> 
> > Source/WebKit/Shared/NavigatingToAppBoundDomain.h:30
> > +enum class NavigatingToAppBoundDomain : uint8_t { Yes, No, AppBoundDomainsEmpty };
> 
> What does AppBoundDomainsEmpty mean here? Why is it useful to differentiate?
> This seems like a weird name in this enum. Maybe it should be something like
> "Maybe" or "Unknown", which would fit better with "No", "Yes".

It means the WKAppBoundDomains plist entry was empty. I could change it to something like 'Unknown' but I feel like that is less clear than just signaling that the list was empty when asking if something is an app-bound domain.
Comment 5 Chris Dumez 2020-04-06 15:19:53 PDT
(In reply to katherine_cheney from comment #4)
> (In reply to Chris Dumez from comment #3)
> > Comment on attachment 395619 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=395619&action=review
> > 
> > > Source/WebKit/Shared/NavigatingToAppBoundDomain.h:30
> > > +enum class NavigatingToAppBoundDomain : uint8_t { Yes, No, AppBoundDomainsEmpty };
> > 
> > What does AppBoundDomainsEmpty mean here? Why is it useful to differentiate?
> > This seems like a weird name in this enum. Maybe it should be something like
> > "Maybe" or "Unknown", which would fit better with "No", "Yes".
> 
> It means the WKAppBoundDomains plist entry was empty. I could change it to
> something like 'Unknown' but I feel like that is less clear than just
> signaling that the list was empty when asking if something is an app-bound
> domain.

Why is it important to differentiate "No" and "Empty" ?
Comment 6 Chris Dumez 2020-04-06 15:20:41 PDT
(In reply to Chris Dumez from comment #5)
> (In reply to katherine_cheney from comment #4)
> > (In reply to Chris Dumez from comment #3)
> > > Comment on attachment 395619 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=395619&action=review
> > > 
> > > > Source/WebKit/Shared/NavigatingToAppBoundDomain.h:30
> > > > +enum class NavigatingToAppBoundDomain : uint8_t { Yes, No, AppBoundDomainsEmpty };
> > > 
> > > What does AppBoundDomainsEmpty mean here? Why is it useful to differentiate?
> > > This seems like a weird name in this enum. Maybe it should be something like
> > > "Maybe" or "Unknown", which would fit better with "No", "Yes".
> > 
> > It means the WKAppBoundDomains plist entry was empty. I could change it to
> > something like 'Unknown' but I feel like that is less clear than just
> > signaling that the list was empty when asking if something is an app-bound
> > domain.
> 
> Why is it important to differentiate "No" and "Empty" ?

Another alternative would also be Optional<NavigatingToAppBoundDomain>
Comment 7 Kate Cheney 2020-04-06 15:23:56 PDT
(In reply to Chris Dumez from comment #5)
> (In reply to katherine_cheney from comment #4)
> > (In reply to Chris Dumez from comment #3)
> > > Comment on attachment 395619 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=395619&action=review
> > > 
> > > > Source/WebKit/Shared/NavigatingToAppBoundDomain.h:30
> > > > +enum class NavigatingToAppBoundDomain : uint8_t { Yes, No, AppBoundDomainsEmpty };
> > > 
> > > What does AppBoundDomainsEmpty mean here? Why is it useful to differentiate?
> > > This seems like a weird name in this enum. Maybe it should be something like
> > > "Maybe" or "Unknown", which would fit better with "No", "Yes".
> > 
> > It means the WKAppBoundDomains plist entry was empty. I could change it to
> > something like 'Unknown' but I feel like that is less clear than just
> > signaling that the list was empty when asking if something is an app-bound
> > domain.
> 
> Why is it important to differentiate "No" and "Empty" ?

For simplicity, I split this patch out from a bigger one. The behavior of In-App Browser privacy should be different if the list has no entries. Optional<NavigatingToAppBoundDomain> would probably work.
Comment 8 Kate Cheney 2020-04-06 15:55:12 PDT
Created attachment 395626 [details]
Patch
Comment 9 Brent Fulgham 2020-04-06 16:30:35 PDT
Comment on attachment 395626 [details]
Patch

r=me
Comment 10 EWS 2020-04-06 17:11:48 PDT
Committed r259615: <https://trac.webkit.org/changeset/259615>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395626 [details].