Bug 203974 - WKWebView is missing an equivalent to WebKit 1's API to set the media style
Summary: WKWebView is missing an equivalent to WebKit 1's API to set the media style
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: Safari 13
Hardware: All All
: P2 Blocker
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-11-07 12:42 PST by Ken Case
Modified: 2019-11-18 09:15 PST (History)
5 users (show)

See Also:


Attachments
Patch (16.37 KB, patch)
2019-11-12 13:48 PST, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (16.37 KB, patch)
2019-11-12 13:57 PST, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (16.37 KB, patch)
2019-11-12 14:56 PST, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (21.93 KB, patch)
2019-11-12 16:49 PST, Brady Eidson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ken Case 2019-11-07 12:42:48 PST
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.)
Comment 1 Brady Eidson 2019-11-08 17:29:38 PST
<rdar://problem/49862107>
Comment 2 Brady Eidson 2019-11-12 13:48:02 PST
Created attachment 383378 [details]
Patch
Comment 3 Brady Eidson 2019-11-12 13:57:46 PST
Created attachment 383381 [details]
Patch
Comment 4 Brady Eidson 2019-11-12 14:56:50 PST
Created attachment 383388 [details]
Patch
Comment 5 Alex Christensen 2019-11-12 15:12:55 PST
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.
Comment 6 Brady Eidson 2019-11-12 15:40:45 PST
(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.
Comment 7 Brady Eidson 2019-11-12 16:49:10 PST
Created attachment 383407 [details]
Patch
Comment 8 WebKit Commit Bot 2019-11-14 10:09:37 PST
Comment on attachment 383407 [details]
Patch

Clearing flags on attachment: 383407

Committed r252459: <https://trac.webkit.org/changeset/252459>
Comment 9 WebKit Commit Bot 2019-11-14 10:09:39 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Darin Adler 2019-11-14 13:08:57 PST
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.
Comment 11 Darin Adler 2019-11-15 09:53:53 PST
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.
Comment 12 Darin Adler 2019-11-18 09:15:01 PST
(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.