Bug 184421 – Make PluginData cache its web visible plugins

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

Make PluginData cache its web visible plugins

[youenn fablet]

Created attachment 337519 [details]
Patch

[Darin Adler]

Comment on attachment 337519 [details]
Patch

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

> Source/WebCore/plugins/PluginData.cpp:49
> +    if (m_visiblePlugins.isEmpty())
> +        m_visiblePlugins = m_page.pluginInfoProvider().webVisiblePluginInfo(m_page, m_pageURL);

I think we should be using std::optional and nullopt instead of treating an empty vector as a special case, because there is a chance there are *no* visible plug-ins and we don’t want to keep calling the pluginInfoProvider over and over again in that case just because the vector is empty.

[youenn fablet]

Created attachment 337549 [details]
Patch

[EWS Watchlist]

Comment on attachment 337549 [details]
Patch

Attachment 337549 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7259604

New failing tests:
imported/w3c/web-platform-tests/workers/name-property.html

[EWS Watchlist]

Created attachment 337560 [details]
Archive of layout-test-results from ews115 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Chris Dumez]

Comment on attachment 337549 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Make PluginData cache its web visible plugins

Why? The changelog does not explain why we're doing this and there is no associated radar for me to get information from either.

[youenn fablet]

(In reply to Chris Dumez from comment #6)
> Comment on attachment 337549 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=337549&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        Make PluginData cache its web visible plugins
> 
> Why? The changelog does not explain why we're doing this and there is no
> associated radar for me to get information from either.

This is a refactoring when working on a pdf plugin issue.
We often call several times webVisiblePlugins() which does create the same Vector over and over. While no longer strictly needed to fix the pdf plugin issue (see latest patch in bug 184369), it seems useful to do it anyway.

[Daniel Bates]

Comment on attachment 337549 [details]
Patch

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

> Source/WebCore/plugins/PluginData.cpp:43
> +    if (!documentURL.isNull() && !protocolHostAndPortAreEqual(m_pageURL, documentURL)) {

Do we need to worry about file URLs?

[youenn fablet]

> New failing tests:
> imported/w3c/web-platform-tests/workers/name-property.html

Test failure is unrelated.

> > Source/WebCore/plugins/PluginData.cpp:43
> > +    if (!documentURL.isNull() && !protocolHostAndPortAreEqual(m_pageURL, documentURL)) {
> 
> Do we need to worry about file URLs?

The patch is not optimal with file URLs but I am not sure this is worth optimizing for these.

[Daniel Bates]

(In reply to youenn fablet from comment #9)
> > New failing tests:
> > imported/w3c/web-platform-tests/workers/name-property.html
> 
> Test failure is unrelated.
> 
> > > Source/WebCore/plugins/PluginData.cpp:43
> > > +    if (!documentURL.isNull() && !protocolHostAndPortAreEqual(m_pageURL, documentURL)) {
> > 
> > Do we need to worry about file URLs?
> 
> The patch is not optimal with file URLs but I am not sure this is worth
> optimizing for these.

Can you please elaborate?

[Daniel Bates]

(In reply to Daniel Bates from comment #8)
> Comment on attachment 337549 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=337549&action=review
> 
> > Source/WebCore/plugins/PluginData.cpp:43
> > +    if (!documentURL.isNull() && !protocolHostAndPortAreEqual(m_pageURL, documentURL)) {
> 
> Do we need to worry about file URLs?

Just to clarify, I am asking: do we need to ensure correctness of this code when documentURL and/or m_pageURL is a file URL?

[youenn fablet]

All file based urls will get the same list of plugins. This code is fine as is.

[youenn fablet]

(In reply to youenn fablet from comment #12)
> All file based urls will get the same list of plugins. This code is fine as
> is.

We could optimize this by not recomputing the list over and over. But I am not sure this optimization meets the bar

[Chris Dumez]

Comment on attachment 337549 [details]
Patch

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

r=me with comment.

> Source/WebCore/plugins/PluginData.h:131
> +    mutable URL m_pageURL;

The naming here is a bit too generic. We should convey that this relates to the m_visiblePlugins cache member below. I would suggest:
- m_cachedVisiblePluginsURL
- m_cachedVisiblePlugins

Or similar.

Alternatively, we could merge them into a single data member:
mutable std::optional<std::pair<URL, Vector<PluginInfo>>> m_cachedVisiblePlugins;

[youenn fablet]

Created attachment 338432 [details]
Patch for landing

[youenn fablet]

(In reply to youenn fablet from comment #15)
> Created attachment 338432 [details]
> Patch for landing

Thanks for the review.
I went going with m_cachedVisiblePlugins.
I used a struct instead of a pair.

[WebKit Commit Bot]

Comment on attachment 338432 [details]
Patch for landing

Clearing flags on attachment: 338432

Committed r230843: <https://trac.webkit.org/changeset/230843>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/39601482>