Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6163003:
Added automatic update for plugins based on watching file changes. (Closed)
|
Created:
9 years, 11 months ago by pastarmovj Modified:
9 years, 7 months ago CC:
chromium-reviews, darin-cc_chromium.org, stuartmorgan+watch_chromium.org, jam Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdded automatic update for plugins based on watching file changes.
BUG=48383
TEST=Add or remove some files to a default plugin location and check in about:plugins whether the change got detected. Also part of integration tests.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=71783
Patch Set 1 #Patch Set 2 : Fixed a line >80chars long. #
Total comments: 12
Patch Set 3 : Fixed nits. #Patch Set 4 : Removed PurgePluginListCache from the file watchere as it lead to crashes when plugins were in use. #
Total comments: 5
Patch Set 5 : Disabled file watcher for MacOS too many false positives. #Patch Set 6 : Limited the file watcher to Linux only. #
Total comments: 1
Patch Set 7 : Fixed two typos in comments. #Patch Set 8 : Removed unused header. #
Messages
Total messages: 19 (0 generated)
This is the old CL that I wrote before my trip to MTV but now I could test it whether it does its job on all three platforms properly. It does so as far as the currently existing bugs in the plugin code allow it. That means plugins don't get properly removed from the UI and the groups when the files that implement them disappear. I haven't observed any spurious callbacks happen when no file activity is present. On all three platforms the list of paths watched is not all that big and quite consequent. This is the link to the old CL that was create with my @google.com account back then. http://codereview.chromium.org/4459002/show
http://codereview.chromium.org/6163003/diff/2001/chrome/browser/plugin_servic... File chrome/browser/plugin_service.cc (right): http://codereview.chromium.org/6163003/diff/2001/chrome/browser/plugin_servic... chrome/browser/plugin_service.cc:77: BrowserThread::UI, FROM_HERE, Is there are reason you're calling this method on the UI thread? http://codereview.chromium.org/6163003/diff/2001/chrome/browser/plugin_servic... chrome/browser/plugin_service.cc:186: VLOG(1) << "Watching for changes in : " << plugin_dirs_list[i].value(); Nit: no space before the colon. http://codereview.chromium.org/6163003/diff/2001/chrome/browser/plugin_servic... chrome/browser/plugin_service.cc:481: DCHECK(watcher->Watch(path, delegate)); Isn't this whole line a no-op in a release build? I think you want to do bool result = ...; DCHECK(result); http://codereview.chromium.org/6163003/diff/2001/chrome/browser/plugin_service.h File chrome/browser/plugin_service.h (right): http://codereview.chromium.org/6163003/diff/2001/chrome/browser/plugin_servic... chrome/browser/plugin_service.h:192: std::vector<FilePathWatcher*> file_watchers_; I think you could use a scoped_vector for automatic element deletion.
lgtm http://codereview.chromium.org/6163003/diff/2001/chrome/browser/plugin_servic... File chrome/browser/plugin_service.cc (right): http://codereview.chromium.org/6163003/diff/2001/chrome/browser/plugin_servic... chrome/browser/plugin_service.cc:77: BrowserThread::UI, FROM_HERE, On 2011/01/10 13:27:20, Bernhard Bauer wrote: > Is there are reason you're calling this method on the UI thread? yep RenderProcessHost lives on the UI thread http://codereview.chromium.org/6163003/diff/2001/chrome/browser/plugin_servic... chrome/browser/plugin_service.cc:180: std::vector<FilePath> plugin_dirs_list; nit: plugin_dirs_ is enough, especially since list in this case is slightly confusing since it's a vector and not a list
On Mon, Jan 10, 2011 at 18:18, <jam@chromium.org> wrote: > lgtm > > > http://codereview.chromium.org/6163003/diff/2001/chrome/browser/plugin_servic... > File chrome/browser/plugin_service.cc (right): > > http://codereview.chromium.org/6163003/diff/2001/chrome/browser/plugin_servic... > chrome/browser/plugin_service.cc:77: BrowserThread::UI, FROM_HERE, > On 2011/01/10 13:27:20, Bernhard Bauer wrote: >> >> Is there are reason you're calling this method on the UI thread? > > yep RenderProcessHost lives on the UI thread OK. I saw a couple of methods that said "called on the UI thread" in their comment, but not all of them did, and there was no comment at the top. > > http://codereview.chromium.org/6163003/diff/2001/chrome/browser/plugin_servic... > chrome/browser/plugin_service.cc:180: std::vector<FilePath> > plugin_dirs_list; > nit: plugin_dirs_ is enough, especially since list in this case is > slightly confusing since it's a vector and not a list > > http://codereview.chromium.org/6163003/ >
Fixed the nits. I think the failed trybots are not because of me but I will investigate the mac one tomorrow because they are still in the plugins field. http://codereview.chromium.org/6163003/diff/2001/chrome/browser/plugin_servic... File chrome/browser/plugin_service.cc (right): http://codereview.chromium.org/6163003/diff/2001/chrome/browser/plugin_servic... chrome/browser/plugin_service.cc:77: BrowserThread::UI, FROM_HERE, On 2011/01/10 13:27:20, Bernhard Bauer wrote: > Is there are reason you're calling this method on the UI thread? It was a comment from John back than "since this is being called on the FILE thread, PluginDirWatcherDelegate will get called back on that thread. It can't call RenderProcessHost::iterator since RenderProcessHost is not thread-safe, and neither is its iterator. You will need to post a task to the UI thread to do this." http://codereview.chromium.org/6163003/diff/2001/chrome/browser/plugin_servic... chrome/browser/plugin_service.cc:77: BrowserThread::UI, FROM_HERE, On 2011/01/10 17:18:10, John Abd-El-Malek wrote: > On 2011/01/10 13:27:20, Bernhard Bauer wrote: > > Is there are reason you're calling this method on the UI thread? > > yep RenderProcessHost lives on the UI thread Sorry I didn't post my answer earlier, when I had it written already - that would have spent you the need to answer as well :) http://codereview.chromium.org/6163003/diff/2001/chrome/browser/plugin_servic... chrome/browser/plugin_service.cc:180: std::vector<FilePath> plugin_dirs_list; On 2011/01/10 17:18:10, John Abd-El-Malek wrote: > nit: plugin_dirs_ is enough, especially since list in this case is slightly > confusing since it's a vector and not a list That's true, thanks for the note :) Done. http://codereview.chromium.org/6163003/diff/2001/chrome/browser/plugin_servic... chrome/browser/plugin_service.cc:186: VLOG(1) << "Watching for changes in : " << plugin_dirs_list[i].value(); On 2011/01/10 13:27:20, Bernhard Bauer wrote: > Nit: no space before the colon. Done. http://codereview.chromium.org/6163003/diff/2001/chrome/browser/plugin_servic... chrome/browser/plugin_service.cc:481: DCHECK(watcher->Watch(path, delegate)); On 2011/01/10 13:27:20, Bernhard Bauer wrote: > Isn't this whole line a no-op in a release build? I think you want to do > > bool result = ...; > DCHECK(result); True. Sorry about that. http://codereview.chromium.org/6163003/diff/2001/chrome/browser/plugin_service.h File chrome/browser/plugin_service.h (right): http://codereview.chromium.org/6163003/diff/2001/chrome/browser/plugin_servic... chrome/browser/plugin_service.h:192: std::vector<FilePathWatcher*> file_watchers_; On 2011/01/10 13:27:20, Bernhard Bauer wrote: > I think you could use a scoped_vector for automatic element deletion. Done.
LGTM (pending trybot happiness or asserting it's not you ;-) )
Please have a look at the comments in http://codereview.chromium.org/6163003/diff/12001/chrome/browser/plugin_servi... - the change is trivial but the consequences are far reaching. Any other changes to the files are only result of the rebase to newer trunk. http://codereview.chromium.org/6163003/diff/12001/chrome/browser/plugin_servi... File chrome/browser/plugin_service.cc (right): http://codereview.chromium.org/6163003/diff/12001/chrome/browser/plugin_servi... chrome/browser/plugin_service.cc:75: webkit::npapi::PluginList::Singleton()->RefreshPlugins(); Problem here was that PurgePluginCache can be executed between an Async call to a plugin and before it's been processed. This happens in the PluginThreadAsyncCall UI test. The problem turned up in mac because MacOS produces some directory change notifications even when there are none (the frequency is not so high but it turns out to be a access-time change notification - so it happens more or less when there is plugin activity and we might want to disable this for mac altogether). However the problem is not limited to a specific platform and should be tackled in general. http://codereview.chromium.org/6163003/diff/12001/chrome/browser/plugin_servi... chrome/browser/plugin_service.cc:365: PurgePluginListCache(true); I can imagine this will lead to the same problem (only that it doesn't happen all too often) http://codereview.chromium.org/6163003/diff/12001/chrome/browser/plugin_servi... chrome/browser/plugin_service.cc:395: PurgePluginListCache(false); Dito. http://codereview.chromium.org/6163003/diff/12001/chrome/browser/plugin_servi... chrome/browser/plugin_service.cc:416: PurgePluginListCache(false); Dito. http://codereview.chromium.org/6163003/diff/12001/chrome/browser/plugin_servi... chrome/browser/plugin_service.cc:429: PurgePluginListCache(false); Dito. Here we need additional RefreshPlugins call to make plugin policy changes work immediately after a change. (proposed in the http://codereview.chromium.org/6157008/ CL)
On Wed, Jan 12, 2011 at 8:51 AM, <pastarmovj@chromium.org> wrote: > Please have a look at the comments in > > http://codereview.chromium.org/6163003/diff/12001/chrome/browser/plugin_servi... > - the change is trivial but the consequences are far reaching. > > Any other changes to the files are only result of the rebase to newer > trunk. > > > > http://codereview.chromium.org/6163003/diff/12001/chrome/browser/plugin_servi... > > File chrome/browser/plugin_service.cc (right): > > > http://codereview.chromium.org/6163003/diff/12001/chrome/browser/plugin_servi... > chrome/browser/plugin_service.cc:75: > > webkit::npapi::PluginList::Singleton()->RefreshPlugins(); > Problem here was that PurgePluginCache can be executed between an Async > call to a plugin and before it's been processed. This happens in the > PluginThreadAsyncCall UI test. The problem turned up in mac because > MacOS produces some directory change notifications even when there are > none (the frequency is not so high but it turns out to be a access-time > change notification - so it happens more or less when there is plugin > activity and we might want to disable this for mac altogether). However > the problem is not limited to a specific platform and should be tackled > in general. > If I'm understanding this correctly, Mac sends directory change notifications each time we load a plugin? If so, how is this not limited to a specific platform if this doesn't happen on Win/Linux? > > > http://codereview.chromium.org/6163003/diff/12001/chrome/browser/plugin_servi... > chrome/browser/plugin_service.cc:365: PurgePluginListCache(true); > I can imagine this will lead to the same problem (only that it doesn't > happen all too often) > > > http://codereview.chromium.org/6163003/diff/12001/chrome/browser/plugin_servi... > chrome/browser/plugin_service.cc:395: PurgePluginListCache(false); > Dito. > > > http://codereview.chromium.org/6163003/diff/12001/chrome/browser/plugin_servi... > chrome/browser/plugin_service.cc:416: PurgePluginListCache(false); > Dito. > > > http://codereview.chromium.org/6163003/diff/12001/chrome/browser/plugin_servi... > chrome/browser/plugin_service.cc:429: PurgePluginListCache(false); > Dito. Here we need additional RefreshPlugins call to make plugin policy > changes work immediately after a change. (proposed in the > http://codereview.chromium.org/6157008/ CL) > > > http://codereview.chromium.org/6163003/ >
On Wed, Jan 12, 2011 at 18:14, John Abd-El-Malek <jam@chromium.org> wrote: > > > On Wed, Jan 12, 2011 at 8:51 AM, <pastarmovj@chromium.org> wrote: >> >> Please have a look at the comments in >> >> http://codereview.chromium.org/6163003/diff/12001/chrome/browser/plugin_servi... >> - the change is trivial but the consequences are far reaching. >> >> Any other changes to the files are only result of the rebase to newer >> trunk. >> >> >> >> http://codereview.chromium.org/6163003/diff/12001/chrome/browser/plugin_servi... >> File chrome/browser/plugin_service.cc (right): >> >> >> http://codereview.chromium.org/6163003/diff/12001/chrome/browser/plugin_servi... >> chrome/browser/plugin_service.cc:75: >> webkit::npapi::PluginList::Singleton()->RefreshPlugins(); >> Problem here was that PurgePluginCache can be executed between an Async >> call to a plugin and before it's been processed. This happens in the >> PluginThreadAsyncCall UI test. The problem turned up in mac because >> MacOS produces some directory change notifications even when there are >> none (the frequency is not so high but it turns out to be a access-time >> change notification - so it happens more or less when there is plugin >> activity and we might want to disable this for mac altogether). However >> the problem is not limited to a specific platform and should be tackled >> in general. > > If I'm understanding this correctly, Mac sends directory change > notifications each time we load a plugin? If so, how is this not limited to > a specific platform if this doesn't happen on Win/Linux? AFAICT, it's a bug in Apple's FSEvents (which FileWatcher on Mac is based on), where it produces false positives. >> >> >> http://codereview.chromium.org/6163003/diff/12001/chrome/browser/plugin_servi... >> chrome/browser/plugin_service.cc:365: PurgePluginListCache(true); >> I can imagine this will lead to the same problem (only that it doesn't >> happen all too often) >> >> >> http://codereview.chromium.org/6163003/diff/12001/chrome/browser/plugin_servi... >> chrome/browser/plugin_service.cc:395: PurgePluginListCache(false); >> Dito. >> >> >> http://codereview.chromium.org/6163003/diff/12001/chrome/browser/plugin_servi... >> chrome/browser/plugin_service.cc:416: PurgePluginListCache(false); >> Dito. >> >> >> http://codereview.chromium.org/6163003/diff/12001/chrome/browser/plugin_servi... >> chrome/browser/plugin_service.cc:429: PurgePluginListCache(false); >> Dito. Here we need additional RefreshPlugins call to make plugin policy >> changes work immediately after a change. (proposed in the >> http://codereview.chromium.org/6157008/ CL) >> >> http://codereview.chromium.org/6163003/ > >
On Wed, Jan 12, 2011 at 9:17 AM, Bernhard Bauer <bauerb@chromium.org> wrote: > On Wed, Jan 12, 2011 at 18:14, John Abd-El-Malek <jam@chromium.org> wrote: > > > > > > On Wed, Jan 12, 2011 at 8:51 AM, <pastarmovj@chromium.org> wrote: > >> > >> Please have a look at the comments in > >> > >> > http://codereview.chromium.org/6163003/diff/12001/chrome/browser/plugin_servi... > >> - the change is trivial but the consequences are far reaching. > >> > >> Any other changes to the files are only result of the rebase to newer > >> trunk. > >> > >> > >> > >> > http://codereview.chromium.org/6163003/diff/12001/chrome/browser/plugin_servi... > >> File chrome/browser/plugin_service.cc (right): > >> > >> > >> > http://codereview.chromium.org/6163003/diff/12001/chrome/browser/plugin_servi... > >> chrome/browser/plugin_service.cc:75: > >> webkit::npapi::PluginList::Singleton()->RefreshPlugins(); > >> Problem here was that PurgePluginCache can be executed between an Async > >> call to a plugin and before it's been processed. This happens in the > >> PluginThreadAsyncCall UI test. The problem turned up in mac because > >> MacOS produces some directory change notifications even when there are > >> none (the frequency is not so high but it turns out to be a access-time > >> change notification - so it happens more or less when there is plugin > >> activity and we might want to disable this for mac altogether). However > >> the problem is not limited to a specific platform and should be tackled > >> in general. > > > > If I'm understanding this correctly, Mac sends directory change > > notifications each time we load a plugin? If so, how is this not limited > to > > a specific platform if this doesn't happen on Win/Linux? > > AFAICT, it's a bug in Apple's FSEvents (which FileWatcher on Mac is > based on), where it produces false positives. > Right, this was one of the issues that I raised when watching for file notifications for plugins was brought up. It seems like this makes it a no-go for Mac, since the cost compared to the benefit is high (i.e. plugins are used a lot more than they are updated, and if each time they're loaded we have to refresh the list, that'll be a lot of disk access). On Windows, I'm curious to know which plugins this would help us catch (i.e. where they don't write to the registry which we watch, but just install to Program Files\Mozilla\Plugins). >> > >> > >> > http://codereview.chromium.org/6163003/diff/12001/chrome/browser/plugin_servi... > >> chrome/browser/plugin_service.cc:365: PurgePluginListCache(true); > >> I can imagine this will lead to the same problem (only that it doesn't > >> happen all too often) > >> > >> > >> > http://codereview.chromium.org/6163003/diff/12001/chrome/browser/plugin_servi... > >> chrome/browser/plugin_service.cc:395: PurgePluginListCache(false); > >> Dito. > >> > >> > >> > http://codereview.chromium.org/6163003/diff/12001/chrome/browser/plugin_servi... > >> chrome/browser/plugin_service.cc:416: PurgePluginListCache(false); > >> Dito. > >> > >> > >> > http://codereview.chromium.org/6163003/diff/12001/chrome/browser/plugin_servi... > >> chrome/browser/plugin_service.cc:429: PurgePluginListCache(false); > >> Dito. Here we need additional RefreshPlugins call to make plugin policy > >> changes work immediately after a change. (proposed in the > >> http://codereview.chromium.org/6157008/ CL) > >> > >> http://codereview.chromium.org/6163003/ > > > > >
Disabled the plugin update for mac because I couldn't find a way to switch of the updates for non-changing updates and these happen at least a few times every minute. The MacOS api for that doesn't provide enough info to filter these. A solution later could be to manually track the directories and filter non-change notifications in the FilePathWatcher. For windows the list of plugins that seem to not use the registry but blindly copy to the default Netscape locations is not so small if this information is mostly correct : http://plugindoc.mozdev.org/windows-all.html . Actually some of them might not install if no Mozilla browser is present but there is still the Fake 4.x patch for these. Waiting for try bot happiness too.
On 2011/01/13 11:06:52, pastarmovj wrote: > Disabled the plugin update for mac because I couldn't find a way to switch of > the updates for non-changing updates and these happen at least a few times every > minute. The MacOS api for that doesn't provide enough info to filter these. A > solution later could be to manually track the directories and filter non-change > notifications in the FilePathWatcher. > > For windows the list of plugins that seem to not use the registry but blindly > copy to the default Netscape locations is not so small if this information is > mostly correct : http://plugindoc.mozdev.org/windows-all.html . Actually some of > them might not install if no Mozilla browser is present but there is still the > Fake 4.x patch for these. > > Waiting for try bot happiness too. We only care about the top plugins, i.e. most of the list above probably only have < a few % install rate at most, so I don't think the overhead of watching directories is worth it for them. i.e. the price is that we might refresh the plugin list more than once if we get notifications from both registry and disk, since they'll come at different times. The plugins we care about are Flash/Reader/QuickTime/SilverLight/WMP/Java/Real. We have UMA stats on which plugins are used (see Jim's thread to chrome-team titled "Ever wonder what plugins are being used by Chrome users"). So I'm curious if the disk watching helps catch any of these on Windows, where the registry watching doesn't.
All these you mentioned should be completely supported through the registry on windows. Given that fact do you think we should limit this feature to Linux only? MacOS file watching is crappy, windows is mostly supported by registry watchers so linux is the only platfom where the enumeration filesystem is the only way to get new plugins and file watchers function properly there. ChromeOS I guess is not interesting either as users can't install plugins themselves. On Thu, Jan 13, 2011 at 8:44 PM, <jam@chromium.org> wrote: > On 2011/01/13 11:06:52, pastarmovj wrote: > >> Disabled the plugin update for mac because I couldn't find a way to switch >> of >> the updates for non-changing updates and these happen at least a few times >> > every > >> minute. The MacOS api for that doesn't provide enough info to filter >> these. A >> solution later could be to manually track the directories and filter >> > non-change > >> notifications in the FilePathWatcher. >> > > For windows the list of plugins that seem to not use the registry but >> blindly >> copy to the default Netscape locations is not so small if this information >> is >> mostly correct : http://plugindoc.mozdev.org/windows-all.html . Actually >> some >> > of > >> them might not install if no Mozilla browser is present but there is still >> the >> Fake 4.x patch for these. >> > > Waiting for try bot happiness too. >> > > We only care about the top plugins, i.e. most of the list above probably > only > have < a few % install rate at most, so I don't think the overhead of > watching > directories is worth it for them. i.e. the price is that we might refresh > the > plugin list more than once if we get notifications from both registry and > disk, > since they'll come at different times. > > The plugins we care about are > Flash/Reader/QuickTime/SilverLight/WMP/Java/Real. > We have UMA stats on which plugins are used (see Jim's thread to > chrome-team > titled "Ever wonder what plugins are being used by Chrome users"). So I'm > curious if the disk watching helps catch any of these on Windows, where the > registry watching doesn't. > > > http://codereview.chromium.org/6163003/ >
Yep, Linux only for now seems to be where we get the benefit, so we should do it just there. On Thu, Jan 13, 2011 at 1:34 PM, Julian Pastarmov <pastarmovj@google.com>wrote: > All these you mentioned should be completely supported through the registry > on windows. Given that fact do you think we should limit this feature to > Linux only? MacOS file watching is crappy, windows is mostly supported by > registry watchers so linux is the only platfom where the enumeration > filesystem is the only way to get new plugins and file watchers function > properly there. ChromeOS I guess is not interesting either as users can't > install plugins themselves. > > > On Thu, Jan 13, 2011 at 8:44 PM, <jam@chromium.org> wrote: > >> On 2011/01/13 11:06:52, pastarmovj wrote: >> >>> Disabled the plugin update for mac because I couldn't find a way to >>> switch of >>> the updates for non-changing updates and these happen at least a few >>> times >>> >> every >> >>> minute. The MacOS api for that doesn't provide enough info to filter >>> these. A >>> solution later could be to manually track the directories and filter >>> >> non-change >> >>> notifications in the FilePathWatcher. >>> >> >> For windows the list of plugins that seem to not use the registry but >>> blindly >>> copy to the default Netscape locations is not so small if this >>> information is >>> mostly correct : http://plugindoc.mozdev.org/windows-all.html . Actually >>> some >>> >> of >> >>> them might not install if no Mozilla browser is present but there is >>> still the >>> Fake 4.x patch for these. >>> >> >> Waiting for try bot happiness too. >>> >> >> We only care about the top plugins, i.e. most of the list above probably >> only >> have < a few % install rate at most, so I don't think the overhead of >> watching >> directories is worth it for them. i.e. the price is that we might refresh >> the >> plugin list more than once if we get notifications from both registry and >> disk, >> since they'll come at different times. >> >> The plugins we care about are >> Flash/Reader/QuickTime/SilverLight/WMP/Java/Real. >> We have UMA stats on which plugins are used (see Jim's thread to >> chrome-team >> titled "Ever wonder what plugins are being used by Chrome users"). So I'm >> curious if the disk watching helps catch any of these on Windows, where >> the >> registry watching doesn't. >> >> >> http://codereview.chromium.org/6163003/ >> > >
Sounds good. Will fix it tomorrow :) On Jan 13, 2011 10:48 PM, "John Abd-El-Malek" <jam@chromium.org> wrote: > Yep, Linux only for now seems to be where we get the benefit, so we should > do it just there. > > On Thu, Jan 13, 2011 at 1:34 PM, Julian Pastarmov <pastarmovj@google.com >wrote: > >> All these you mentioned should be completely supported through the registry >> on windows. Given that fact do you think we should limit this feature to >> Linux only? MacOS file watching is crappy, windows is mostly supported by >> registry watchers so linux is the only platfom where the enumeration >> filesystem is the only way to get new plugins and file watchers function >> properly there. ChromeOS I guess is not interesting either as users can't >> install plugins themselves. >> >> >> On Thu, Jan 13, 2011 at 8:44 PM, <jam@chromium.org> wrote: >> >>> On 2011/01/13 11:06:52, pastarmovj wrote: >>> >>>> Disabled the plugin update for mac because I couldn't find a way to >>>> switch of >>>> the updates for non-changing updates and these happen at least a few >>>> times >>>> >>> every >>> >>>> minute. The MacOS api for that doesn't provide enough info to filter >>>> these. A >>>> solution later could be to manually track the directories and filter >>>> >>> non-change >>> >>>> notifications in the FilePathWatcher. >>>> >>> >>> For windows the list of plugins that seem to not use the registry but >>>> blindly >>>> copy to the default Netscape locations is not so small if this >>>> information is >>>> mostly correct : http://plugindoc.mozdev.org/windows-all.html . Actually >>>> some >>>> >>> of >>> >>>> them might not install if no Mozilla browser is present but there is >>>> still the >>>> Fake 4.x patch for these. >>>> >>> >>> Waiting for try bot happiness too. >>>> >>> >>> We only care about the top plugins, i.e. most of the list above probably >>> only >>> have < a few % install rate at most, so I don't think the overhead of >>> watching >>> directories is worth it for them. i.e. the price is that we might refresh >>> the >>> plugin list more than once if we get notifications from both registry and >>> disk, >>> since they'll come at different times. >>> >>> The plugins we care about are >>> Flash/Reader/QuickTime/SilverLight/WMP/Java/Real. >>> We have UMA stats on which plugins are used (see Jim's thread to >>> chrome-team >>> titled "Ever wonder what plugins are being used by Chrome users"). So I'm >>> curious if the disk watching helps catch any of these on Windows, where >>> the >>> registry watching doesn't. >>> >>> >>> http://codereview.chromium.org/6163003/ >>> >> >>
On 2011/01/13 11:06:52, pastarmovj wrote: > Disabled the plugin update for mac because I couldn't find a way to switch of > the updates for non-changing updates and these happen at least a few times every > minute. The MacOS api for that doesn't provide enough info to filter these. Can you file a bug about this? If FilePathWatcher doesn't work right on the Mac, that's a problem. It's certainly not the case that OS X doesn't provide APIs that allow for getting reasonable change notifications.
On Thu, Jan 13, 2011 at 23:11, <stuartmorgan@chromium.org> wrote: > On 2011/01/13 11:06:52, pastarmovj wrote: >> >> Disabled the plugin update for mac because I couldn't find a way to switch >> of >> the updates for non-changing updates and these happen at least a few times > > every >> >> minute. The MacOS api for that doesn't provide enough info to filter >> these. > > Can you file a bug about this? If FilePathWatcher doesn't work right on the > Mac, > that's a problem. It's certainly not the case that OS X doesn't provide APIs > that allow for getting reasonable change notifications. And please cc mnissler, as he's been working on it. > http://codereview.chromium.org/6163003/ >
Please review. Limited the reloading to Linux only as discussed. FYI: The strange access notifications on MacOS were caused mainly because of Sophos going through the hard drive but as this reason can't be excluded on many machines it might be better to keep this off on MacOS for now.
lgtm http://codereview.chromium.org/6163003/diff/32001/chrome/browser/plugin_servi... File chrome/browser/plugin_service.cc (right): http://codereview.chromium.org/6163003/diff/32001/chrome/browser/plugin_servi... chrome/browser/plugin_service.cc:176: // important pluigns register themselves in the registry so no need to do that. nit: plugins |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
