Bug 25318 - Add a method to Chromium's port to reset the plugin cache
Summary: Add a method to Chromium's port to reset the plugin cache
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: John Abd-El-Malek
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-04-21 16:26 PDT by John Abd-El-Malek
Modified: 2009-04-29 13:21 PDT (History)
1 user (show)

See Also:


Attachments
Patch (1.09 KB, patch)
2009-04-21 16:31 PDT, John Abd-El-Malek
dglazkov: review+
Details | Formatted Diff | Diff
move resetChromiumPluginCache() to PluginData (1.85 KB, patch)
2009-04-22 11:50 PDT, John Abd-El-Malek
sam: review-
Details | Formatted Diff | Diff
renamed to resetPluginCache (1.82 KB, patch)
2009-04-22 13:04 PDT, John Abd-El-Malek
sam: review+
Details | Formatted Diff | Diff
updated comment (1.76 KB, patch)
2009-04-22 15:39 PDT, John Abd-El-Malek
no flags Details | Formatted Diff | Diff
removed comment about no test (1.71 KB, patch)
2009-04-22 15:40 PDT, John Abd-El-Malek
no flags Details | Formatted Diff | Diff
revert previous change (1.02 KB, patch)
2009-04-23 12:59 PDT, John Abd-El-Malek
dglazkov: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Abd-El-Malek 2009-04-21 16:26:36 PDT
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.
Comment 1 John Abd-El-Malek 2009-04-21 16:31:35 PDT
Created attachment 29668 [details]
Patch
Comment 2 Dimitri Glazkov (Google) 2009-04-21 21:17:22 PDT
Comment on attachment 29668 [details]
Patch

> +void resetChromiumPluginCache() {

Brace goes on new line. But this is tiny -- I'll fix up and land.
Comment 3 Dimitri Glazkov (Google) 2009-04-21 21:20:29 PDT
Landed as http://trac.webkit.org/changeset/42739.
Comment 4 Darin Fisher (:fishd, Google) 2009-04-21 23:25:28 PDT
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?
Comment 5 Darin Fisher (:fishd, Google) 2009-04-21 23:27:17 PDT
> 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?
Comment 6 John Abd-El-Malek 2009-04-21 23:30:16 PDT
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
Comment 7 Darin Fisher (:fishd, Google) 2009-04-21 23:58:23 PDT
> 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.
Comment 8 John Abd-El-Malek 2009-04-22 11:22:41 PDT
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).
Comment 9 John Abd-El-Malek 2009-04-22 11:32:55 PDT
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.
Comment 10 John Abd-El-Malek 2009-04-22 11:50:47 PDT
Created attachment 29686 [details]
move resetChromiumPluginCache() to PluginData
Comment 11 Sam Weinig 2009-04-22 12:52:48 PDT
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?
Comment 12 John Abd-El-Malek 2009-04-22 13:04:48 PDT
Created attachment 29687 [details]
renamed to resetPluginCache
Comment 13 Sam Weinig 2009-04-22 13:36:52 PDT
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
Comment 14 John Abd-El-Malek 2009-04-22 15:39:35 PDT
Created attachment 29690 [details]
updated comment
Comment 15 John Abd-El-Malek 2009-04-22 15:40:48 PDT
Created attachment 29691 [details]
removed comment about no test
Comment 16 John Abd-El-Malek 2009-04-22 17:32:33 PDT
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.
Comment 17 John Abd-El-Malek 2009-04-23 12:59:08 PDT
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.
Comment 18 John Abd-El-Malek 2009-04-28 14:35:13 PDT
ping?  can anyone submit my patch to revert this?  It's not needed anymore.

Thanks.
Comment 19 John Abd-El-Malek 2009-04-28 14:49:46 PDT
Thanks Dimitri, when you get a chance, can you commit this for me?
Comment 20 Eric Seidel (no email) 2009-04-29 13:21:58 PDT
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/plugins/chromium/PluginDataChromium.cpp
Committed r43004