Summary: | WKWebView is missing an equivalent to WebKit 1's API to set the media style | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ken Case <kc> | ||||||||||
Component: | WebKit2 | Assignee: | Brady Eidson <beidson> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Blocker | CC: | achristensen, beidson, commit-queue, darin, thorton | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | Safari 13 | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=204297 | ||||||||||||
Attachments: |
|
Description
Ken Case
2019-11-07 12:42:48 PST
Created attachment 383378 [details]
Patch
Created attachment 383381 [details]
Patch
Created attachment 383388 [details]
Patch
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. (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. Created attachment 383407 [details]
Patch
Comment on attachment 383407 [details] Patch Clearing flags on attachment: 383407 Committed r252459: <https://trac.webkit.org/changeset/252459> All reviewed patches have been landed. Closing bug. 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 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. (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. |