WPT: html/webappapis/system-state-and-capabilities/the-navigator-object/navigator-pluginarray.html fails due to lack of caching of Plugin objects Test: http://wpt.live/html/webappapis/system-state-and-capabilities/the-navigator-object/navigator-pluginarray.html Spec: https://html.spec.whatwg.org/multipage/system-state.html#navigatorplugins
Seems like a good opportunity to overhaul the DOM plugin stuff to simplify / match the spec.
Created attachment 401867 [details] Patch
Created attachment 401868 [details] Patch
Created attachment 401869 [details] Patch
Created attachment 401871 [details] Patch
Created attachment 401872 [details] Patch
Comment on attachment 401872 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=401872&action=review > Source/WebCore/ChangeLog:23 > + [WORK IN PROGRESS] How is "review?" consistent with this? > Source/WebCore/loader/SubframeLoader.cpp:178 > + for (auto& mimeType : page.pluginData().webVisibleMimeTypes()) { > + for (auto& mimeTypeExtension : mimeType.extensions) { I totally would have used the names "type" and "extension". Big believer in single words and shorter names taking advantage of narrow scope. > Source/WebCore/page/Navigator.cpp:218 > + if (publiclyVisiblePlugins.size() > 0) { Don’t need this if statement. Code below is harmless if size is 0. > Source/WebCore/page/Navigator.cpp:221 > + auto domPlugin = DOMPlugin::create(*this, plugin); I would name this "wrapper". > Source/WebCore/page/Navigator.cpp:222 > + for (auto& domMimeType : domPlugin->mimeTypes()) I would name this "type". But also, can you use appendVector or something else like that? > Source/WebCore/page/Navigator.cpp:227 > + if (additionalWebVisibilePlugins.size() > 0) { Ditto. > Source/WebCore/page/Navigator.cpp:230 > + auto domPlugin = DOMPlugin::create(*this, plugin); Ditto. > Source/WebCore/page/Navigator.cpp:231 > + for (auto& domMimeType : domPlugin->mimeTypes()) Ditto. > Source/WebCore/page/Navigator.h:71 > + void initializePluginAndMIMETypeArraysIfNeeded(); Don’t really think we need "if needed" in this function name. > Source/WebCore/plugins/DOMMimeType.cpp:60 > + return m_enabledPlugin.get(); Surprised that we need "get()". > Source/WebCore/plugins/DOMMimeTypeArray.cpp:54 > + for (auto& mimeType : m_mimeTypes) { I’d use the name "type". > Source/WebCore/plugins/DOMMimeTypeArray.cpp:65 > + for (auto& mimeType : m_mimeTypes) Ditto. > Source/WebCore/plugins/DOMMimeTypeArray.h:37 > + static Ref<DOMMimeTypeArray> create(Navigator& navigator, Vector<Ref<DOMMimeType>>&& mimeTypes = { }) { return adoptRef(*new DOMMimeTypeArray(navigator, WTFMove(mimeTypes))); } I think I would use "types". But also, these kinds of create functions don’t need to be inlined in the header. If the constructor gets inlined in the create function that’s better than the create function getting inlined at the call site, so I suggest putting these in .cpp files. > Source/WebCore/plugins/DOMMimeTypeArray.h:48 > + explicit DOMMimeTypeArray(Navigator&, Vector<Ref<DOMMimeType>>&& mimeTypes); Don’t think we need the name "mimeTypes" here. > Source/WebCore/plugins/DOMPlugin.cpp:41 > + m_mimeTypes.reserveInitialCapacity(m_pluginInfo.mimes.size()); > + for (auto& mimeType : m_pluginInfo.mimes) > + m_mimeTypes.uncheckedAppend(DOMMimeType::create(navigator, mimeType, *this)); > + > + std::sort(m_mimeTypes.begin(), m_mimeTypes.end(), [](const Ref<DOMMimeType>& a, const Ref<DOMMimeType>& b) { > + return codePointCompareLessThan(a->type(), b->type()); > + }); Would slightly prefer this be in a block or function and use construction rather than initialization. > Source/WebCore/plugins/DOMPlugin.cpp:75 > + for (auto& mimeType : m_mimeTypes) { Consider "type". > Source/WebCore/plugins/DOMPlugin.cpp:86 > + for (auto& mimeType : m_mimeTypes) Consider "type". > Source/WebCore/plugins/DOMPlugin.h:36 > + static Ref<DOMPlugin> create(Navigator& navigator, const PluginInfo& pluginInfo) { return adoptRef(*new DOMPlugin(navigator, pluginInfo)); } Same thought about "create" function inlining and about naming the argument just "info". > Source/WebCore/plugins/DOMPluginArray.cpp:35 > + , m_publiclyVisiblePlugins(publiclyVisiblePlugins) Missing WTFMove. > Source/WebCore/plugins/DOMPluginArray.cpp:36 > + , m_additionalWebVisibilePlugins(additionalWebVisibilePlugins) Ditto. > Source/WebCore/plugins/DOMPluginArray.h:38 > + static Ref<DOMPluginArray> create(Navigator& navigator, Vector<Ref<DOMPlugin>>&& publiclyVisiblePlugins = { }, Vector<Ref<DOMPlugin>>&& additionalWebVisibilePlugins = { }) { return adoptRef(*new DOMPluginArray(navigator, WTFMove(publiclyVisiblePlugins), WTFMove(additionalWebVisibilePlugins))); } Same thought about not putting "create" in headers. > Source/WebCore/plugins/PluginData.cpp:116 > + for (auto& mime : plugin.mimes) Maybe "type" instead of "mime"? > Source/WebCore/plugins/PluginData.cpp:124 > + for (auto plugin : plugins) { Maybe auto& instead of auto? > Source/WebCore/plugins/PluginData.cpp:125 > for (auto& mime : plugin.mimes) { Ditto. > Source/WebCore/plugins/PluginData.cpp:149 > + if (!m_cachedVisiblePlugins.pluginList) > + return false; > + return supportsMimeTypeForPlugins(mimeType, allowedPluginTypes, *m_cachedVisiblePlugins.pluginList); Maybe: return list && ... Instead? Or I guess could leave it like this so we don’t end up with a long line or a local variable. > Source/WebCore/plugins/PluginData.cpp:155 > + for (auto& mime : plugin.mimes) { Consider the name "type". > Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1660 > + for (auto& mimeType : pluginData.webVisibleMimeTypes()) { Consider just "type"?
(In reply to Darin Adler from comment #7) > Comment on attachment 401872 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=401872&action=review > > > Source/WebCore/ChangeLog:23 > > + [WORK IN PROGRESS] > > How is "review?" consistent with this? :(. Oh no, it wasn't meant for review quite yet, but I forgot to use webkit-patch post --no-review as I iterated on fixing non-macOS build failures. :(. That said, it's pretty much done, so I think it's perfectly acceptable to review. I just want to improve the ChangeLogs and this should also fix some tests, so I want update the results. I will also address all the feedback, thanks for taking the time! > > > Source/WebCore/loader/SubframeLoader.cpp:178 > > + for (auto& mimeType : page.pluginData().webVisibleMimeTypes()) { > > + for (auto& mimeTypeExtension : mimeType.extensions) { > > I totally would have used the names "type" and "extension". Big believer in > single words and shorter names taking advantage of narrow scope. Done. > > > Source/WebCore/page/Navigator.cpp:218 > > + if (publiclyVisiblePlugins.size() > 0) { > > Don’t need this if statement. Code below is harmless if size is 0. Fixed. > > > Source/WebCore/page/Navigator.cpp:221 > > + auto domPlugin = DOMPlugin::create(*this, plugin); > > I would name this "wrapper". Done. > > > Source/WebCore/page/Navigator.cpp:222 > > + for (auto& domMimeType : domPlugin->mimeTypes()) > > I would name this "type". But also, can you use appendVector or something > else like that? Yup. Used appendVector. Forgot about that. > > > Source/WebCore/page/Navigator.cpp:227 > > + if (additionalWebVisibilePlugins.size() > 0) { > > Ditto. Fixed. > > > Source/WebCore/page/Navigator.cpp:230 > > + auto domPlugin = DOMPlugin::create(*this, plugin); > > Ditto. Fixed. > > > Source/WebCore/page/Navigator.cpp:231 > > + for (auto& domMimeType : domPlugin->mimeTypes()) > > Ditto. Fixed. > > > Source/WebCore/page/Navigator.h:71 > > + void initializePluginAndMIMETypeArraysIfNeeded(); > > Don’t really think we need "if needed" in this function name. Removed. > > > Source/WebCore/plugins/DOMMimeType.cpp:60 > > + return m_enabledPlugin.get(); > > Surprised that we need "get()". Not sure we do. Will check. > > > Source/WebCore/plugins/DOMMimeTypeArray.cpp:54 > > + for (auto& mimeType : m_mimeTypes) { > > I’d use the name "type". Fixed. > > > Source/WebCore/plugins/DOMMimeTypeArray.cpp:65 > > + for (auto& mimeType : m_mimeTypes) > > Ditto. Fixed. > > > Source/WebCore/plugins/DOMMimeTypeArray.h:37 > > + static Ref<DOMMimeTypeArray> create(Navigator& navigator, Vector<Ref<DOMMimeType>>&& mimeTypes = { }) { return adoptRef(*new DOMMimeTypeArray(navigator, WTFMove(mimeTypes))); } > > I think I would use "types". But also, these kinds of create functions don’t > need to be inlined in the header. If the constructor gets inlined in the > create function that’s better than the create function getting inlined at > the call site, so I suggest putting these in .cpp files. Good call. > > > Source/WebCore/plugins/DOMMimeTypeArray.h:48 > > + explicit DOMMimeTypeArray(Navigator&, Vector<Ref<DOMMimeType>>&& mimeTypes); > > Don’t think we need the name "mimeTypes" here. Fixed. > > > Source/WebCore/plugins/DOMPlugin.cpp:41 > > + m_mimeTypes.reserveInitialCapacity(m_pluginInfo.mimes.size()); > > + for (auto& mimeType : m_pluginInfo.mimes) > > + m_mimeTypes.uncheckedAppend(DOMMimeType::create(navigator, mimeType, *this)); > > + > > + std::sort(m_mimeTypes.begin(), m_mimeTypes.end(), [](const Ref<DOMMimeType>& a, const Ref<DOMMimeType>& b) { > > + return codePointCompareLessThan(a->type(), b->type()); > > + }); > > Would slightly prefer this be in a block or function and use construction > rather than initialization. Fixed. > > > Source/WebCore/plugins/DOMPlugin.cpp:75 > > + for (auto& mimeType : m_mimeTypes) { > > Consider "type". Fixed. > > > Source/WebCore/plugins/DOMPlugin.cpp:86 > > + for (auto& mimeType : m_mimeTypes) > > Consider "type". Fixed > > > Source/WebCore/plugins/DOMPlugin.h:36 > > + static Ref<DOMPlugin> create(Navigator& navigator, const PluginInfo& pluginInfo) { return adoptRef(*new DOMPlugin(navigator, pluginInfo)); } > > Same thought about "create" function inlining and about naming the argument > just "info". Fixed. > > > Source/WebCore/plugins/DOMPluginArray.cpp:35 > > + , m_publiclyVisiblePlugins(publiclyVisiblePlugins) > > Missing WTFMove. Fixed. > > > Source/WebCore/plugins/DOMPluginArray.cpp:36 > > + , m_additionalWebVisibilePlugins(additionalWebVisibilePlugins) > > Ditto. Fixed. > > > Source/WebCore/plugins/DOMPluginArray.h:38 > > + static Ref<DOMPluginArray> create(Navigator& navigator, Vector<Ref<DOMPlugin>>&& publiclyVisiblePlugins = { }, Vector<Ref<DOMPlugin>>&& additionalWebVisibilePlugins = { }) { return adoptRef(*new DOMPluginArray(navigator, WTFMove(publiclyVisiblePlugins), WTFMove(additionalWebVisibilePlugins))); } > > Same thought about not putting "create" in headers. Fixed. > > > Source/WebCore/plugins/PluginData.cpp:116 > > + for (auto& mime : plugin.mimes) > > Maybe "type" instead of "mime"? Fixed. > > > Source/WebCore/plugins/PluginData.cpp:124 > > + for (auto plugin : plugins) { > > Maybe auto& instead of auto? Fixed. > > > Source/WebCore/plugins/PluginData.cpp:125 > > for (auto& mime : plugin.mimes) { > > Ditto. Fixed. > > > Source/WebCore/plugins/PluginData.cpp:149 > > + if (!m_cachedVisiblePlugins.pluginList) > > + return false; > > + return supportsMimeTypeForPlugins(mimeType, allowedPluginTypes, *m_cachedVisiblePlugins.pluginList); > > Maybe: > > return list && ... > > Instead? Or I guess could leave it like this so we don’t end up with a long > line or a local variable. > > > Source/WebCore/plugins/PluginData.cpp:155 > > + for (auto& mime : plugin.mimes) { > > Consider the name "type". Fixed. > > > Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1660 > > + for (auto& mimeType : pluginData.webVisibleMimeTypes()) { > > Consider just "type"? Fixed.
(In reply to Sam Weinig from comment #8) > (In reply to Darin Adler from comment #7) > > Comment on attachment 401872 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=401872&action=review > > > > > Source/WebCore/plugins/DOMMimeType.cpp:60 > > > + return m_enabledPlugin.get(); > > > > Surprised that we need "get()". > > Not sure we do. Will check. > Yeah, it's needed unfortunately. m_enabledPlugin is a WeakPtr<DOMPlugin> and apparently there is no implicit conversion from WeakPtr to RefPtr. Seems like it would probably be fine add.
Created attachment 401874 [details] Patch
Comment on attachment 401874 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=401874&action=review > Source/WebCore/page/Navigator.cpp:215 > + Vector<Ref<DOMPlugin>> additionalWebVisibileDOMPlugins; typo: visibile > Source/WebCore/page/Navigator.cpp:232 > + std::sort(publiclyVisibleDOMPlugins.begin(), publiclyVisibleDOMPlugins.end(), [](const Ref<DOMPlugin>& a, const Ref<DOMPlugin>& b) { Should double check that there is some reason these names can never be duplicated. if they could, we would probably want to use a stable sort, or compare something else as a secondary key. > Source/WebCore/page/Navigator.cpp:236 > + std::sort(webVisibileDOMMimeTypes.begin(), webVisibileDOMMimeTypes.end(), [](const Ref<DOMMimeType>& a, const Ref<DOMMimeType>& b) { Should double check that there is some reason these type strings can never be duplicated. if they could, we would probably want to use a stable sort, or compare something else as a secondary key. > Source/WebCore/page/Navigator.cpp:241 > + // named property look up, sort their order is not exposed. "sort their order" -> "so their order" > Source/WebCore/plugins/DOMPlugin.cpp:43 > + std::sort(types.begin(), types.end(), [](const Ref<DOMMimeType>& a, const Ref<DOMMimeType>& b) { Should double check that there is some reason these type strings can never be duplicated. if they could, we would probably want to use a stable sort, or compare something else as a secondary key.
Comment on attachment 401874 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=401874&action=review >> Source/WebCore/page/Navigator.cpp:215 >> + Vector<Ref<DOMPlugin>> additionalWebVisibileDOMPlugins; > > typo: visibile Oh man, I did that one a bunch :(. Visi-bile sounds really awful. >> Source/WebCore/page/Navigator.cpp:232 >> + std::sort(publiclyVisibleDOMPlugins.begin(), publiclyVisibleDOMPlugins.end(), [](const Ref<DOMPlugin>& a, const Ref<DOMPlugin>& b) { > > Should double check that there is some reason these names can never be duplicated. if they could, we would probably want to use a stable sort, or compare something else as a secondary key. Hm, that's an interesting point. The old code did essentially the same sort (see the old PluginData::publiclyVisiblePlugins()), but that doesn't mean it was safe to do there either. Looking through the WebKit code that creates the plugin data, it looks to me like there isn't anything that ensures that plugins don't have the same name, but we do guarantee that the bundleIdentifiers are unique, so I will use that as the secondary key. std::sort(publiclyVisibleDOMPlugins.begin(), publiclyVisibleDOMPlugins.end(), [](const Ref<DOMPlugin>& a, const Ref<DOMPlugin>& b) { if (auto nameComparison = codePointCompare(a->info().name, b->info().name)) return nameComparison < 0; return codePointCompareLessThan(a->info().bundleIdentifier, b->info().bundleIdentifier); }); For the mime types, they are guaranteed to be unique for an individual plugin, but I can't see anything that would ensure that across plugins. So, for the sort in DOMPlugin's makeMimeTypes(), we don't need a secondary key, but for the one Navigator::initializePluginAndMIMETypeArrays() we do. Fortunately, we can use the associated plugin's bundle identifier for that as well. std::sort(webVisibleDOMMimeTypes.begin(), webVisibleDOMMimeTypes.end(), [](const Ref<DOMMimeType>& a, const Ref<DOMMimeType>& b) { if (auto typeComparison = codePointCompare(a->type(), b->type())) return typeComparison < 0; return codePointCompareLessThan(a->enabledPlugin()->info().bundleIdentifier, b->enabledPlugin()->info().bundleIdentifier); });
Created attachment 401877 [details] Patch
Committed r263017: <https://trac.webkit.org/changeset/263017> All reviewed patches have been landed. Closing bug and clearing flags on attachment 401877 [details].
<rdar://problem/64347684>
<rdar://problem/64347685>