Bug 195989 – [WebKit2] Introduce a public version of WKWebsitePolicies

• Component: WebKit2
• Status: RESOLVED FIXED

[Wenson Hsieh]

Work towards <rdar://problem/47228232>

[Wenson Hsieh]

Created attachment 365312 [details]
WIP

[Wenson Hsieh]

Created attachment 365314 [details]
WIP

[Wenson Hsieh]

Created attachment 365343 [details]
Work in progress

[EWS Watchlist]

Comment on attachment 365343 [details]
Work in progress

Attachment 365343 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11589360

New failing tests:
http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable.html

[EWS Watchlist]

Created attachment 365461 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[Wenson Hsieh]

Nope, changing course...

[Wenson Hsieh]

(In reply to Wenson Hsieh from comment #6)
> Nope, changing course...

🔙

[Wenson Hsieh]

Created attachment 365998 [details]
Patch

[Tim Horton]

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.

[Wenson Hsieh]

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!

[Tim Horton]

(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!

[Wenson Hsieh]

Created attachment 366238 [details]
Patch

[Tim Horton]

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!

[Wenson Hsieh]

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!

[Wenson Hsieh]

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.

[Wenson Hsieh]

Created attachment 366431 [details]
Take 2

[Wenson Hsieh]

Created attachment 366439 [details]
Fix a * on the wrong side

[WebKit Commit Bot]

Comment on attachment 366439 [details]
Fix a * on the wrong side

Clearing flags on attachment: 366439

Committed r243726: <https://trac.webkit.org/changeset/243726>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.