Bug 25318 – Add a method to Chromium's port to reset the plugin cache

• Status: RESOLVED
• Resolution: FIXED
• Component: Plug-ins

[John Abd-El-Malek]

To allow plugins to be installed while Chromium is running, I need to add a method to PluginDataChromium.cpp to flush the plugin list cache.

[John Abd-El-Malek]

Created attachment 29668 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 29668 [details]
Patch

> +void resetChromiumPluginCache() {

Brace goes on new line. But this is tiny -- I'll fix up and land.

[Dimitri Glazkov (Google)]

Landed as http://trac.webkit.org/changeset/42739.

[Darin Fisher (:fishd, Google)]

Comment on attachment 29668 [details]
Patch

> Index: WebCore/plugins/chromium/PluginDataChromium.cpp

> +void resetChromiumPluginCache() {
> +    pluginCache.reset(false);
> +}
> +

Shouldn't there be a header file change as well?

[Darin Fisher (:fishd, Google)]

> Shouldn't there be a header file change as well?

Actually, I think this should be a static method on PluginData.  It seems like you want a method that is like PluginData::refresh but works slightly differently?

[John Abd-El-Malek]

I was forward declaring it and using it from render_thread.cc.

The reason I didn't add it to a header (i.e. PluginData.h) is that I thought it seemed specific to Chromium's implementation, since the cache is only in PluginDataChromium.cc.  Not sure if other platforms do similar caching.

For background, here's the Chromium change: http://codereview.chromium.org/88020

[Darin Fisher (:fishd, Google)]

> I was forward declaring it and using it from render_thread.cc.

I see... that won't work in the future when we optionally build WebKit as a DLL.  We will need a WebKit API corresponding to this, and I think it would be a lot cleaner to have something on PluginData as a static method corresponding to that.  (I think it leads to maintenance problems if we have undeclared global functions being used by various parts of the product, so we should anyways avoid it.)

It seems like all implementations of PluginData should be conceptually similar, such that a function like this would be appropriate.  Otherwise, maybe we have built our cache at the wrong place?  At the very least, we can put a #if PLATFORM(CHROMIUM) specific method on PluginData.

[John Abd-El-Malek]

I see your point about adding it to the WebKit API, I can do that.

I don't think this should be added to PluginData, as it's not a general method that would be used by other ports.  It's only in Chromium's case that we double cache the list (once in PluginData, and another in the browser process), but that's a result of our multi-process architecture and I don't think other ports need it.  I checked them and didn't find any of them doing caching in their PluginData implementation.

Also, I think the current location is a good place to cache, since moving our plugin list from outside WebKit code to WebKit code entails a lot of string conversion (over 500 on my machine).

[John Abd-El-Malek]

thinking about it more, I agree that it could be in PluginData but with an ifdef CHROMIUM.  I'll add a patch to this bug.

[John Abd-El-Malek]

Created attachment 29686 [details]
move resetChromiumPluginCache() to PluginData

[Sam Weinig]

Comment on attachment 29686 [details]
move resetChromiumPluginCache() to PluginData

Why not just call the function resetPluginCache.  If other platforms use a pluginCache in the future and need to reset it, wouldn't they use the same function?

[John Abd-El-Malek]

Created attachment 29687 [details]
renamed to resetPluginCache

[Sam Weinig]

Comment on attachment 29687 [details]
renamed to resetPluginCache

> +        * plugins/PluginData.h:
> +        * plugins/chromium/PluginDataChromium.cpp:
> +        (WebCore::PluginData::resetChromiumPluginCache):
This is no longer the name of the function.

Otherwise, it seems fine.  r=me

[John Abd-El-Malek]

Created attachment 29690 [details]
updated comment

[John Abd-El-Malek]

Created attachment 29691 [details]
removed comment about no test

[John Abd-El-Malek]

While testing this out, I found a problem on youtube.  Looks like it's caused by the fact that the Page* objects remains the same during refresh.  I think I have a better fix than this.  I'll test and upload soon.

[John Abd-El-Malek]

Created attachment 29713 [details]
revert previous change

OK after some more investigation, I realized that the cache purge needs to happen to all the Page* instances.  Page::refreshPlugins does that, although it also causes a call to the browser process (in Chromium's case) from each renderer process.  I've come up with a change that's all in Chromium's source code, so this change isn't needed anymore.

[John Abd-El-Malek]

ping?  can anyone submit my patch to revert this?  It's not needed anymore.

Thanks.

[John Abd-El-Malek]

Thanks Dimitri, when you get a chance, can you commit this for me?

[Eric Seidel (no email)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/plugins/chromium/PluginDataChromium.cpp
Committed r43004