Summary: | Add a method to Chromium's port to reset the plugin cache | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | John Abd-El-Malek <jam> | ||||||||||||||
Component: | Plug-ins | Assignee: | John Abd-El-Malek <jam> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | dglazkov | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | PC | ||||||||||||||||
OS: | All | ||||||||||||||||
Attachments: |
|
Description
John Abd-El-Malek
2009-04-21 16:26:36 PDT
Created attachment 29668 [details]
Patch
Comment on attachment 29668 [details] Patch > +void resetChromiumPluginCache() { Brace goes on new line. But this is tiny -- I'll fix up and land. Landed as http://trac.webkit.org/changeset/42739. 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? > 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?
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 > 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.
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). 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. Created attachment 29686 [details]
move resetChromiumPluginCache() to PluginData
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?
Created attachment 29687 [details]
renamed to resetPluginCache
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 Created attachment 29690 [details]
updated comment
Created attachment 29691 [details]
removed comment about no test
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. 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.
ping? can anyone submit my patch to revert this? It's not needed anymore. Thanks. Thanks Dimitri, when you get a chance, can you commit this for me? Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog M WebCore/plugins/chromium/PluginDataChromium.cpp Committed r43004 |