Bug 195989

Summary: [WebKit2] Introduce a public version of WKWebsitePolicies
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: WebKit2Assignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, bdakin, cdumez, commit-queue, ews-watchlist, ggaren, rniwa, thorton, webkit-bug-importer
Priority: P2 Keywords: DoNotImportToRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 196284    
Attachments:
Description Flags
WIP
none
WIP
none
Work in progress
ews-watchlist: commit-queue-
Archive of layout-test-results from ews104 for mac-highsierra-wk2
none
Patch
none
Patch
none
Take 2
none
Fix a * on the wrong side none

Description Wenson Hsieh 2019-03-19 22:04:07 PDT 
Work towards <rdar://problem/47228232>
Comment 1 Wenson Hsieh 2019-03-19 22:20:55 PDT Comment hidden (obsolete)
Comment 2 Wenson Hsieh 2019-03-19 22:33:12 PDT Comment hidden (obsolete)
Comment 3 Wenson Hsieh 2019-03-20 07:24:12 PDT Comment hidden (obsolete)
Comment 4 EWS Watchlist 2019-03-20 18:56:18 PDT Comment hidden (obsolete)
Comment 5 EWS Watchlist 2019-03-20 18:56:19 PDT Comment hidden (obsolete)
Comment 6 Wenson Hsieh 2019-03-20 20:44:37 PDT 
Nope, changing course...
Comment 7 Wenson Hsieh 2019-03-21 17:59:38 PDT 
(In reply to Wenson Hsieh from comment #6)
> Nope, changing course...

🔙
Comment 8 Wenson Hsieh 2019-03-26 14:15:22 PDT 
Created attachment 365998 [details]
Patch
Comment 9 Tim Horton 2019-03-27 11:30:04 PDT 
Comment on attachment 365998 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebpagePreferences.mm:191
> +    NSMutableDictionary *dictionary = [[[NSMutableDictionary alloc] initWithCapacity:fields.size()] autorelease];

RetainPtr?

> Source/WebKit/UIProcess/API/Cocoa/WKWebpagePreferences.mm:193
> +        [dictionary setObject:field.value() forKey:field.name()];

Or subscript operator?

> Source/WebKit/UIProcess/API/Cocoa/WKWebpagePreferences.mm:252
> +    return [NSString stringWithFormat:@"<%@: %p; contentBlockersEnabled = %d>", NSStringFromClass(self.class), self, self.contentBlockersEnabled];

Don't do this. It should only print API properties, and either all or none.
Comment 10 Wenson Hsieh 2019-03-28 19:32:39 PDT 
Comment on attachment 365998 [details]
Patch

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

>> Source/WebKit/UIProcess/API/Cocoa/WKWebpagePreferences.mm:191
>> +    NSMutableDictionary *dictionary = [[[NSMutableDictionary alloc] initWithCapacity:fields.size()] autorelease];
> 
> RetainPtr?

👍🏻

>> Source/WebKit/UIProcess/API/Cocoa/WKWebpagePreferences.mm:193
>> +        [dictionary setObject:field.value() forKey:field.name()];
> 
> Or subscript operator?

Subscript operator gets a tiny bit annoying if I go with RetainPtr above :/ In other places, I think we might prefer just using -setObject:forKey: since subscripting would look like:

dictionary.get()[(NSString *)field.name()] = field.value();

>> Source/WebKit/UIProcess/API/Cocoa/WKWebpagePreferences.mm:252
>> +    return [NSString stringWithFormat:@"<%@: %p; contentBlockersEnabled = %d>", NSStringFromClass(self.class), self, self.contentBlockersEnabled];
> 
> Don't do this. It should only print API properties, and either all or none.

Good call!
Comment 11 Tim Horton 2019-03-28 19:37:48 PDT 
(In reply to Wenson Hsieh from comment #10)
> Comment on attachment 365998 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=365998&action=review
> 
> >> Source/WebKit/UIProcess/API/Cocoa/WKWebpagePreferences.mm:191
> >> +    NSMutableDictionary *dictionary = [[[NSMutableDictionary alloc] initWithCapacity:fields.size()] autorelease];
> > 
> > RetainPtr?
> 
> 👍🏻
> 
> >> Source/WebKit/UIProcess/API/Cocoa/WKWebpagePreferences.mm:193
> >> +        [dictionary setObject:field.value() forKey:field.name()];
> > 
> > Or subscript operator?
> 
> Subscript operator gets a tiny bit annoying if I go with RetainPtr above :/
> In other places, I think we might prefer just using -setObject:forKey: since
> subscripting would look like:
> 
> dictionary.get()[(NSString *)field.name()] = field.value();

Ah, yes, these are definitely mutually exclusive suggestions :)

> >> Source/WebKit/UIProcess/API/Cocoa/WKWebpagePreferences.mm:252
> >> +    return [NSString stringWithFormat:@"<%@: %p; contentBlockersEnabled = %d>", NSStringFromClass(self.class), self, self.contentBlockersEnabled];
> > 
> > Don't do this. It should only print API properties, and either all or none.
> 
> Good call!
Comment 12 Wenson Hsieh 2019-03-28 20:32:47 PDT 
Created attachment 366238 [details]
Patch
Comment 13 Tim Horton 2019-04-01 14:52:19 PDT 
Comment on attachment 366238 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:5133
> +    auto data = [websitePolicies webPagePreferences]->_websitePolicies->data();

Dots?

> Source/WebKit/UIProcess/API/Cocoa/WKWebpagePreferences.h:30
> + @helps Contains properties used to determine web page preferences.

What's @helps for? I never know anything about this doc comment format.

> Source/WebKit/UIProcess/API/Cocoa/WKWebpagePreferencesPrivate.h:69
> +@property (nonatomic) BOOL contentBlockersEnabled;
> +@property (nonatomic) _WKWebsiteAutoplayQuirk allowedAutoplayQuirks;
> +@property (nonatomic) _WKWebsiteAutoplayPolicy autoplayPolicy;
> +@property (nonatomic, copy) NSDictionary<NSString *, NSString *> *customHeaderFields;
> +@property (nonatomic) _WKWebsitePopUpPolicy popUpPolicy;
> +@property (nonatomic, strong) WKWebsiteDataStore *websiteDataStore;
> +@property (nonatomic, copy) NSString *customUserAgent;
> +@property (nonatomic, copy) NSString *customJavaScriptUserAgentAsSiteSpecificQuirks;
> +@property (nonatomic, copy) NSString *customNavigatorPlatform;
> +@property (nonatomic) _WKWebsiteDeviceOrientationAndMotionAccessPolicy deviceOrientationAndMotionAccessPolicy;

These all need to gain underscores now that they're on a public class, right?

> Source/WebKit/UIProcess/API/Cocoa/_WKWebsitePolicies.mm:43
> +- (WKWebpagePreferences *)webPagePreferences

Capitalization consistency!
Comment 14 Wenson Hsieh 2019-04-01 15:13:44 PDT 
Comment on attachment 366238 [details]
Patch

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

>> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:5133
>> +    auto data = [websitePolicies webPagePreferences]->_websitePolicies->data();
> 
> Dots?

More dots!

>> Source/WebKit/UIProcess/API/Cocoa/WKWebpagePreferences.h:30
>> + @helps Contains properties used to determine web page preferences.
> 
> What's @helps for? I never know anything about this doc comment format.

Oh, interesting. I took this from WKWebViewConfiguration's public header. As far as I can tell though, WKWebViewConfiguration.h is the only place in the entire public Cocoa API surface (macOS and iOS) that uses @helps, so it might be a mistake. I changed this to @discussion instead, which is much more commonly used.

>> Source/WebKit/UIProcess/API/Cocoa/WKWebpagePreferencesPrivate.h:69
>> +@property (nonatomic) _WKWebsiteDeviceOrientationAndMotionAccessPolicy deviceOrientationAndMotionAccessPolicy;
> 
> These all need to gain underscores now that they're on a public class, right?

They rather should, but I was hoping to do that later, to maintain bincompat with Safari.

I could also just add the underscore-prefixed versions here in this patch, alongside these non-underscore-prefixed versions. I'll go ahead and do that, so we can get Safari on the underscore-prefixed versions sooner, and then finally remove these existing ones.

>> Source/WebKit/UIProcess/API/Cocoa/_WKWebsitePolicies.mm:43
>> +- (WKWebpagePreferences *)webPagePreferences
> 
> Capitalization consistency!

Fixed!
Comment 15 Wenson Hsieh 2019-04-01 15:15:58 PDT 
Comment on attachment 366238 [details]
Patch

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

>>> Source/WebKit/UIProcess/API/Cocoa/WKWebpagePreferencesPrivate.h:69
>>> +@property (nonatomic) _WKWebsiteDeviceOrientationAndMotionAccessPolicy deviceOrientationAndMotionAccessPolicy;
>> 
>> These all need to gain underscores now that they're on a public class, right?
> 
> They rather should, but I was hoping to do that later, to maintain bincompat with Safari.
> 
> I could also just add the underscore-prefixed versions here in this patch, alongside these non-underscore-prefixed versions. I'll go ahead and do that, so we can get Safari on the underscore-prefixed versions sooner, and then finally remove these existing ones.

Nope, never mind, this is on the new WKWebpagePreferences so this should totally be _-prefixed from the get-go.
Comment 16 Wenson Hsieh 2019-04-01 15:36:39 PDT 
Created attachment 366431 [details]
Take 2
Comment 17 Wenson Hsieh 2019-04-01 16:22:08 PDT 
Created attachment 366439 [details]
Fix a * on the wrong side
Comment 18 WebKit Commit Bot 2019-04-01 17:01:10 PDT 
Comment on attachment 366439 [details]
Fix a * on the wrong side

Clearing flags on attachment: 366439

Committed r243726: <https://trac.webkit.org/changeset/243726>
Comment 19 WebKit Commit Bot 2019-04-01 17:01:12 PDT 
All reviewed patches have been landed.  Closing bug.