Bug 203974 – WKWebView is missing an equivalent to WebKit 1's API to set the media style

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Ken Case]

We're trying to migrate all of our apps from using WebView to WKWebView, and one of the pieces we're missing is an equivalent to the old WebKit's -[WebView setMediaStyle:]. It would be great if that API could be reintroduced!

(We use media styles in the CSS of some pages to render content differently based on whether you're viewing the page on the web or in an embedded context like our software update window.)

[Brady Eidson]

<rdar://problem/49862107>

[Brady Eidson]

Created attachment 383378 [details]
Patch

[Brady Eidson]

Created attachment 383381 [details]
Patch

[Brady Eidson]

Created attachment 383388 [details]
Patch

[Alex Christensen]

Comment on attachment 383388 [details]
Patch

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

> Source/WebKit/ChangeLog:11
> +        - Expose a read/write property on WKWebView.

I assume this needs to be changed at runtime so it can't be on the configuration.  Ok.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:325
> +@discussion The value of mediaType will override the normal value of the CSS media property.

I don't think this description makes it easy to understand how to use this.

> Source/WebKit/UIProcess/WebPageProxy.cpp:9502
> +    m_overriddenMediaType = mediaType;

WebKitLegacy's implementation does nothing if we're setting it to the same value.  Do we want such an optimization here?

> Source/WebKit/UIProcess/WebPageProxy.cpp:9503
> +    m_process->send(Messages::WebPage::SetOverriddenMediaType(mediaType), m_webPageID);

This appears to be the only way to set the override in the web process.  What if we do a cross-domain navigation?  I imagine we will probably want the same media type unless the API has been used to change it.  Please add the string to WebPageCreationParameters and add such a test.  Also one that sets the media type before the first navigation.

[Brady Eidson]

(In reply to Alex Christensen from comment #5)
> Comment on attachment 383388 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=383388&action=review
> 
> > Source/WebKit/ChangeLog:11
> > +        - Expose a read/write property on WKWebView.
> 
> I assume this needs to be changed at runtime so it can't be on the
> configuration.  Ok.
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:325
> > +@discussion The value of mediaType will override the normal value of the CSS media property.
> 
> I don't think this description makes it easy to understand how to use this.

If you are interested in this property, and therefore the related CSS concepts, the above should make sense.

It's pretty much cribbed directly from WebView.h mediaStyle - Which isn't to say we shouldn't improve it if there's an obvious path towards improving it!

I'm open to suggestions.
> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:9502
> > +    m_overriddenMediaType = mediaType;
> 
> WebKitLegacy's implementation does nothing if we're setting it to the same
> value.  Do we want such an optimization here?

WK1's implementation is busted in that it doesn't do the extra necessary work.

WK2 has normal IPC work (which is great to avoid) and the extra necessary work (for style recalc) - Both great to avoid.

> > Source/WebKit/UIProcess/WebPageProxy.cpp:9503
> > +    m_process->send(Messages::WebPage::SetOverriddenMediaType(mediaType), m_webPageID);
> 
> This appears to be the only way to set the override in the web process. 
> What if we do a cross-domain navigation?  I imagine we will probably want
> the same media type unless the API has been used to change it.  Please add
> the string to WebPageCreationParameters and add such a test.  Also one that
> sets the media type before the first navigation.

*DEFINITELY* didn't account for process swap. On it.

[Brady Eidson]

Created attachment 383407 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 383407 [details]
Patch

Clearing flags on attachment: 383407

Committed r252459: <https://trac.webkit.org/changeset/252459>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 383407 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:329
> +@property (nonatomic, null_resettable, copy) NSString *mediaType;

This says "null_resettable", which means you can set this property to nil, but it will never return nil.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:4784
> +    return _page->overriddenMediaType().isNull() ? nil : (NSString *)_page->overriddenMediaType();

But this method returns nil when it’s null, not "the normal value". So I think the property should be "nullable", not "null_resettable". And the documentation discussion may need to be rewritten to be slightly clearer.

[Darin Adler]

Comment on attachment 383407 [details]
Patch

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

>> Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:329
>> +@property (nonatomic, null_resettable, copy) NSString *mediaType;
> 
> This says "null_resettable", which means you can set this property to nil, but it will never return nil.

I did more research and this should definitely be "nullable" rather than "null_resettable". Was working a patch to fix it, but there’s something wrong with my Subversion login so someone else should probably do it.

>> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:4784
>> +    return _page->overriddenMediaType().isNull() ? nil : (NSString *)_page->overriddenMediaType();
> 
> But this method returns nil when it’s null, not "the normal value". So I think the property should be "nullable", not "null_resettable". And the documentation discussion may need to be rewritten to be slightly clearer.

Thought about it further, and I don’t think the discussion needs to change. Just the "nullable" thing.

[Darin Adler]

(In reply to Darin Adler from comment #11)
> I did more research and this should definitely be "nullable" rather than
> "null_resettable".

Bug 204297 has my patch for this.