WebFrame sometimes incorrectly rule out PDF as a mime type that can be showed
Created attachment 337393 [details] Patch
Created attachment 337394 [details] WIP
Created attachment 337396 [details] WIP
Comment on attachment 337396 [details] WIP Attachment 337396 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/7231659 New failing tests: userscripts/user-script-plugin-document.html plugins/plugin-javascript-access.html plugins/access-after-page-destroyed-2.html plugins/embed-attributes-setting.html http/tests/security/contentSecurityPolicy/cross-origin-plugin-document-allowed-in-child-window.html fast/dom/collection-iterators.html plugins/plugin-document-back-forward.html http/tests/plugins/nounsupported-plugin.html http/tests/security/contentSecurityPolicy/same-origin-plugin-document-blocked-in-child-window.html plugins/iframe-plugin-bgcolor.html http/tests/security/contentSecurityPolicy/1.1/plugintypes-affects-child.html http/tests/plugins/supported-plugin-origin-specific-visibility.html imported/w3c/web-platform-tests/html/webappapis/system-state-and-capabilities/the-navigator-object/navigator-pluginarray.html plugins/access-after-page-destroyed.html http/tests/plugins/plugin-javascript-access.html fast/dom/Window/Plug-ins.html plugins/no-mime-with-valid-extension.html
Created attachment 337405 [details] Archive of layout-test-results from ews106 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Created attachment 337415 [details] Patch
Created attachment 337416 [details] Patch
Comment on attachment 337416 [details] Patch Attachment 337416 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/7234383 New failing tests: plugins/navigator-plugins-disabled.html http/tests/plugins/supported-plugin-origin-specific-visibility.html
Created attachment 337419 [details] Archive of layout-test-results from ews106 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Created attachment 337423 [details] Patch
Comment on attachment 337423 [details] Patch Attachment 337423 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/7237224 New failing tests: http/tests/plugins/supported-plugin-origin-specific-visibility.html
Created attachment 337424 [details] Archive of layout-test-results from ews107 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Created attachment 337428 [details] Patch
Created attachment 337432 [details] Patch
Comment on attachment 337432 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337432&action=review > Source/WebCore/plugins/PluginData.cpp:45 > + fprintf(stderr, "url %s\n", m_url.string().utf8().data()); Will remove this fprintf in a follow-up or at landing time.
Comment on attachment 337432 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337432&action=review Why no regression test? > Source/WebCore/page/Page.cpp:554 > + if (m_pluginData) { > + URL pageURL = mainFrame().document() ? mainFrame().document()->url() : URL { }; > + if (!protocolHostAndPortAreEqual(pageURL, m_pluginData->url())) > + m_pluginData = nullptr; > + } 1) Ugly that this check is here, but the extraction of the URL is done in the PluginData constructor. Instead there should be a PluginData function to tell if it’s OK to keep using a PluginData. That way the code can be shared and/or kept in sync with the constructor. So the code here would be more like this: if (!m_pluginData || !m_pluginData->canBeReused()) m_pluginData = PluginData::create(*this); Or we could find a cleaner way for the PluginData to "fix itself". Creating a new object doesn’t seem necessary or relevant to me. if (m_pluginData) m_pluginData->updateURL(); else m_pluginData = PluginData::create(*this); 2) It’s unclear to me why matching protocol, host, and port are sufficient, rather than matching the entire URL. 3) It is so inelegant to do this every time someone calls the pluginData function. Is there no better design? > Source/WebKit/WebProcess/Plugins/WebPluginInfoProvider.h:49 > - void getPluginInfo(WebCore::Page&, Vector<WebCore::PluginInfo>&, std::optional<Vector<WebCore::SupportedPluginName>>&) final; > - void getWebVisiblePluginInfo(WebCore::Page&, Vector<WebCore::PluginInfo>&) final; > + Vector<WebCore::PluginInfo> getPluginInfo(WebCore::Page&, std::optional<Vector<WebCore::SupportedPluginName>>&) final; > + Vector<WebCore::PluginInfo> getWebVisiblePluginInfo(WebCore::Page&, const WebCore::URL&) final; WebKit coding style says that the reason these function names have the word "get" in them is that they use out arguments. When changing them to return vectors, the word "get" should be removed from their names.
Thanks for the feedback. I think I'll split the refactoring and the modification change in two patches. We now have to pass the URL since the page URL is used to match plug-in rules defined by WebKit applications. The rules are matching URLS based on host and protocol information. (In reply to Darin Adler from comment #16) > Comment on attachment 337432 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=337432&action=review > > Why no regression test? > > > Source/WebCore/page/Page.cpp:554 > > + if (m_pluginData) { > > + URL pageURL = mainFrame().document() ? mainFrame().document()->url() : URL { }; > > + if (!protocolHostAndPortAreEqual(pageURL, m_pluginData->url())) > > + m_pluginData = nullptr; > > + } > > 1) Ugly that this check is here, but the extraction of the URL is done in > the PluginData constructor. Instead there should be a PluginData function to > tell if it’s OK to keep using a PluginData. That way the code can be shared > and/or kept in sync with the constructor. So the code here would be more > like this: > > if (!m_pluginData || !m_pluginData->canBeReused()) > m_pluginData = PluginData::create(*this); > > Or we could find a cleaner way for the PluginData to "fix itself". Creating > a new object doesn’t seem necessary or relevant to me. > > if (m_pluginData) > m_pluginData->updateURL(); > else > m_pluginData = PluginData::create(*this); > > 2) It’s unclear to me why matching protocol, host, and port are sufficient, > rather than matching the entire URL. > > 3) It is so inelegant to do this every time someone calls the pluginData > function. Is there no better design? > > > Source/WebKit/WebProcess/Plugins/WebPluginInfoProvider.h:49 > > - void getPluginInfo(WebCore::Page&, Vector<WebCore::PluginInfo>&, std::optional<Vector<WebCore::SupportedPluginName>>&) final; > > - void getWebVisiblePluginInfo(WebCore::Page&, Vector<WebCore::PluginInfo>&) final; > > + Vector<WebCore::PluginInfo> getPluginInfo(WebCore::Page&, std::optional<Vector<WebCore::SupportedPluginName>>&) final; > > + Vector<WebCore::PluginInfo> getWebVisiblePluginInfo(WebCore::Page&, const WebCore::URL&) final; > > WebKit coding style says that the reason these function names have the word > "get" in them is that they use out arguments. When changing them to return > vectors, the word "get" should be removed from their names.
Created attachment 337520 [details] Patch
Created attachment 338436 [details] Patch
Comment on attachment 338436 [details] Patch Patch does not apply on Tip of Tree? I cannot find where m_cachedVisiblePlugins is defined.
Comment on attachment 338436 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338436&action=review > Source/WebCore/plugins/PluginData.cpp:125 > + m_cachedVisiblePlugins.pageURL = url; I would suggest: m_cachedVisiblePlugins = { url, m_page.pluginInfoProvider().webVisiblePluginInfo(m_page, url) }; Also, why do we ignore potentially cached data if the domain matches?
Created attachment 338454 [details] Patch for landing
Created attachment 338455 [details] Patch for landing
<rdar://problem/38534312>
(In reply to Chris Dumez from comment #21) > Comment on attachment 338436 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=338436&action=review > > > Source/WebCore/plugins/PluginData.cpp:125 > > + m_cachedVisiblePlugins.pageURL = url; > > I would suggest: > m_cachedVisiblePlugins = { url, > m_page.pluginInfoProvider().webVisiblePluginInfo(m_page, url) }; > > Also, why do we ignore potentially cached data if the domain matches? Thanks for the review, updated both points.
Comment on attachment 338455 [details] Patch for landing Clearing flags on attachment: 338455 Committed r230853: <https://trac.webkit.org/changeset/230853>
All reviewed patches have been landed. Closing bug.