|
|
Created:
10 years ago by pastarmovj Modified:
9 years, 7 months ago CC:
Jakob Kummerow, chromium-reviews, pam+watch_chromium.org, ben+cc_chromium.org, brettw-cc_chromium.org, darin-cc_chromium.org, Paweł Hajdan Jr., stuartmorgan+watch_chromium.org, davemoore+watch_chromium.org, jam, danno, arv (Not doing code reviews) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRefactor the plugin lists handling code.
Effects of this refactor:
1. The WebPluginInfo now keep information not only if a plugin is disabled but also the reason for that. It can either be user, policy or both. That way we can restore the right value after policies stop to control the feature.
2. Plugins can be correctly enabled and disabled either as a group or separately.
3. The code is cleaner and PluginGroup is not duplicating information from PluginList but stores all needed information and provides it through cleaner interface.
BUG=54681, 66505, 69374, 69148
TEST=Manual for the policy. DefaultPluginUITest.DefaultPluginLoadTest from ui_tests and Plugin* from test_shell_tests.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=72341
Patch Set 1 #Patch Set 2 : Style fixes. #Patch Set 3 : Removed superflous line. #Patch Set 4 : Fixed the wrong ordering of plugins in RebuildPluginList. #Patch Set 5 : Disabled a unit test that won't run until we get better PluginList mockability. #Patch Set 6 : MacOS support patched in. #
Total comments: 29
Patch Set 7 : Made plugins_ update continous instead of lazy. Also fixed reviewers and lint's comments. #Patch Set 8 : Rebased on the latest trunk 69124 #Patch Set 9 : Fixed PluginGroup::DisableOutdatedPlugins #
Total comments: 5
Patch Set 10 : Various fixes and initial windows version. #Patch Set 11 : Plugin reloading works completely now. Lint made happy as well. #
Total comments: 13
Patch Set 12 : Removed PluginList::plugins_ completely. #Patch Set 13 : Removed disabled_plugins_ and disabled_groups_ as well as PluginInfo and rebased on top of trunk. #Patch Set 14 : Made the compilers happy after lint fixes. #Patch Set 15 : Make windows compiler even happier. #
Total comments: 113
Patch Set 16 : More fixes and cleanups. #
Total comments: 21
Patch Set 17 : Cleaned up WebPluginInfo and rebased on fixed PluginGroup::InitFrom. #
Total comments: 36
Patch Set 18 : Removed priority from PluginGroup. Moved WebPluginInfoUtils::* to the respective users. #Patch Set 19 : Introduced prematurely disabled plugins vector and true enabled state enumeration. #Patch Set 20 : Fixed unit tests. #
Total comments: 11
Patch Set 21 : Empty groups creation on startup suppressed. Style cleanups. #Patch Set 22 : Mac compilation fix. #Patch Set 23 : Rebased. Cleaned up the public i-face of PluginList. #Patch Set 24 : Added empty group prunning. #
Total comments: 55
Patch Set 25 : Fixed CL according to comments. #Patch Set 26 : Addressed comments and made LoadPlugins threadsafe again. #Patch Set 27 : Make win and mac compilers happy. #
Total comments: 8
Patch Set 28 : More minor fixes. #Patch Set 29 : Making windows compiler happy #
Total comments: 13
Patch Set 30 : Forgotten to upload the mergeconflict resolutions (trybots happiness aplies to this upload). #
Total comments: 2
Patch Set 31 : Addressed Bernhard comments. Pending bonus expected ;) #
Total comments: 4
Patch Set 32 : Made *_to_disable_ sets and extracted some functionality from LoadPlugins for better testability. #
Total comments: 2
Patch Set 33 : Reversed the LoadPlugins separation as Bernhard suggested. Tuned the rest accordingly. #Patch Set 34 : Fixed tiny mistake I did when splitting LoadPlugins. #Patch Set 35 : Further simplified the LoadPluginsInternal (after discussing it with Bernhard). #Patch Set 36 : Whitespace fixes only. Trybot happiness still applies. #Messages
Total messages: 51 (0 generated)
Please have a look at that. It is by no means 100% done (at least not the mac,win platform specific code). But the general idea is complete and works for linux (manually tested).
http://codereview.chromium.org/5699005/diff/8002/webkit/glue/plugins/plugin_l... File webkit/glue/plugins/plugin_list.cc (right): http://codereview.chromium.org/5699005/diff/8002/webkit/glue/plugins/plugin_l... webkit/glue/plugins/plugin_list.cc:408: void PluginList::LoadPlugin(const FilePath& path) { Drive-by: Removing the |plugins| parameter may seem like a good idea, but it's not. As your CL stands right now, if multiple threads call LoadPlugins at the same time, we will possibly end up with duplicates in the plugin list. We don't use a lock around the whole LoadPlugins method for performance reasons, so we construct the new plugins array separately, and then only swap it out at the end (hence the extra |plugins| parameter). Jakob, for AddToPluginGroups we should probably add an additional parameter as well. Sorry I didn't catch that in the first CL.
here you go... http://codereview.chromium.org/5699005/diff/8002/chrome/browser/chromeos/gvie... File chrome/browser/chromeos/gview_request_interceptor_unittest.cc (right): http://codereview.chromium.org/5699005/diff/8002/chrome/browser/chromeos/gvie... chrome/browser/chromeos/gview_request_interceptor_unittest.cc:148: NPAPI::PluginList::Singleton()->DisablePlugin(pdf_path_, true); Why is this true? It seems to be unrelated to policy. http://codereview.chromium.org/5699005/diff/8002/chrome/browser/plugin_update... File chrome/browser/plugin_updater.cc (right): http://codereview.chromium.org/5699005/diff/8002/chrome/browser/plugin_update... chrome/browser/plugin_updater.cc:288: entry->second == NPAPI::PluginList::POLICY) Add methods to check this so the code looks more like enter->second->IsDisabledByPolicy. Then maybe you can make the enum private. http://codereview.chromium.org/5699005/diff/8002/chrome/browser/plugin_update... chrome/browser/plugin_updater.cc:300: !it->IsVulnerable()) { Might make code more readable to switch order of terms in || http://codereview.chromium.org/5699005/diff/8002/webkit/glue/plugins/plugin_l... File webkit/glue/plugins/plugin_list.cc (right): http://codereview.chromium.org/5699005/diff/8002/webkit/glue/plugins/plugin_l... webkit/glue/plugins/plugin_list.cc:375: if(0 == disabled_groups_.count(group_name)) use == 0 formatting (reverse order). compiler gives warning/error on inadvertent assignment. http://codereview.chromium.org/5699005/diff/8002/webkit/glue/plugins/plugin_l... webkit/glue/plugins/plugin_list.cc:381: RebuildPluginsList(); You also acquire the lock inside of RebuildPluginList. Do you really need to re-acquire it below? http://codereview.chromium.org/5699005/diff/8002/webkit/glue/plugins/plugin_l... webkit/glue/plugins/plugin_list.cc:402: new_plugins[*itprio] = &(*itp); Whoa. Taking the address of a dereferenced iterator makes my eyeballs itch, and has an odd "smell". Are you sure that the lifecycles of the WebPluginInfos in group_plugins outlive plugns_ always? If so, it needs to be clearly documented in the the header for the plugin_ member. I suspect there was a good reason for copying them in the old version. http://codereview.chromium.org/5699005/diff/8002/webkit/glue/plugins/plugin_l... File webkit/glue/plugins/plugin_list.h (right): http://codereview.chromium.org/5699005/diff/8002/webkit/glue/plugins/plugin_l... webkit/glue/plugins/plugin_list.h:76: enum DisabledElementReason { USER = 0, POLICY, POLICY_AND_USER }; How about PluginDisabledReason? http://codereview.chromium.org/5699005/diff/8002/webkit/glue/plugins/plugin_l... webkit/glue/plugins/plugin_list.h:78: typedef std::map<string16, DisabledElementReason> DisabledGroupsList; This is a map, so the list name is misleading. consider renaming to just DisabledPlugins and DisabledGroups. http://codereview.chromium.org/5699005/diff/8002/webkit/glue/plugins/plugin_l... webkit/glue/plugins/plugin_list.h:203: // will be disabled. The boolean flag should be set to true if the operation ... if the plugin is disabled by a policy and not a user. http://codereview.chromium.org/5699005/diff/8002/webkit/glue/plugins/plugin_l... webkit/glue/plugins/plugin_list.h:222: void GetDisabledGroups(DisabledGroupsList* disabled_groups); Do these copy? If so, why (probably should document in a comment). Alternatively (probably better) why not just return a const reference? http://codereview.chromium.org/5699005/diff/8002/webkit/glue/plugins/plugin_l... webkit/glue/plugins/plugin_list.h:244: // Populates the |plugins_| vector if is has been marked as dirty. "it" missing? http://codereview.chromium.org/5699005/diff/8002/webkit/glue/plugins/plugin_l... webkit/glue/plugins/plugin_list.h:314: std::vector<WebPluginInfo*> plugins_; I see that you don't explicitly manage the memory of the WebPluginInfos explicitly with this change (I see no STLDeleteContainerPointers in the destructor). If you are relying on the the objects staying alive due to some side-effect of ownership somewhere else, it needs to be clearly documented. http://codereview.chromium.org/5699005/diff/8002/webkit/glue/plugins/plugin_l... webkit/glue/plugins/plugin_list.h:315: bool plugins_vector_is_dirty_; Please document when this actually happens. http://codereview.chromium.org/5699005/diff/8002/webkit/glue/plugins/plugin_l... File webkit/glue/plugins/plugin_list_posix.cc (right): http://codereview.chromium.org/5699005/diff/8002/webkit/glue/plugins/plugin_l... webkit/glue/plugins/plugin_list_posix.cc:249: for (PluginGroup::PluginMap::iterator it = plugin_groups_.begin(); now that you have two iterators, you probably want to use names to disambiguate. e.g. current_curent instead of it and current_plugin_info instead of itp.
At last some working version for review. http://codereview.chromium.org/5699005/diff/8002/chrome/browser/chromeos/gvie... File chrome/browser/chromeos/gview_request_interceptor_unittest.cc (right): http://codereview.chromium.org/5699005/diff/8002/chrome/browser/chromeos/gvie... chrome/browser/chromeos/gview_request_interceptor_unittest.cc:148: NPAPI::PluginList::Singleton()->DisablePlugin(pdf_path_, true); On 2010/12/14 09:43:39, danno wrote: > Why is this true? It seems to be unrelated to policy. Done. It doesn't matter in that case really but yes maybe it's better to be false. http://codereview.chromium.org/5699005/diff/8002/chrome/browser/plugin_update... File chrome/browser/plugin_updater.cc (right): http://codereview.chromium.org/5699005/diff/8002/chrome/browser/plugin_update... chrome/browser/plugin_updater.cc:288: entry->second == NPAPI::PluginList::POLICY) On 2010/12/14 09:43:39, danno wrote: > Add methods to check this so the code looks more like > enter->second->IsDisabledByPolicy. Then maybe you can make the enum private. This doesn't seem to me to bring any more strucure to the code. Please correct me if I am wrong. (Maybe in a short offline chat). http://codereview.chromium.org/5699005/diff/8002/chrome/browser/plugin_update... chrome/browser/plugin_updater.cc:300: !it->IsVulnerable()) { On 2010/12/14 09:43:39, danno wrote: > Might make code more readable to switch order of terms in || Done. http://codereview.chromium.org/5699005/diff/8002/webkit/glue/plugins/plugin_l... File webkit/glue/plugins/plugin_list.cc (right): http://codereview.chromium.org/5699005/diff/8002/webkit/glue/plugins/plugin_l... webkit/glue/plugins/plugin_list.cc:375: if(0 == disabled_groups_.count(group_name)) On 2010/12/14 09:43:39, danno wrote: > use == 0 formatting (reverse order). compiler gives warning/error on inadvertent > assignment. Done. http://codereview.chromium.org/5699005/diff/8002/webkit/glue/plugins/plugin_l... webkit/glue/plugins/plugin_list.cc:381: RebuildPluginsList(); On 2010/12/14 09:43:39, danno wrote: > You also acquire the lock inside of RebuildPluginList. Do you really need to > re-acquire it below? I have to for the plugins_loaded_ flag. However this code has/will change anyway and RebuildPluginsList won't get called. http://codereview.chromium.org/5699005/diff/8002/webkit/glue/plugins/plugin_l... webkit/glue/plugins/plugin_list.cc:402: new_plugins[*itprio] = &(*itp); On 2010/12/14 09:43:39, danno wrote: > Whoa. Taking the address of a dereferenced iterator makes my eyeballs itch, and > has an odd "smell". Are you sure that the lifecycles of the WebPluginInfos in > group_plugins outlive plugns_ always? If so, it needs to be clearly documented > in the the header for the plugin_ member. I suspect there was a good reason for > copying them in the old version. Cool right ;) http://codereview.chromium.org/5699005/diff/8002/webkit/glue/plugins/plugin_l... webkit/glue/plugins/plugin_list.cc:408: void PluginList::LoadPlugin(const FilePath& path) { On 2010/12/14 04:21:49, Bernhard Bauer wrote: > Drive-by: Removing the |plugins| parameter may seem like a good idea, but it's > not. > > As your CL stands right now, if multiple threads call LoadPlugins at the same > time, we will possibly end up with duplicates in the plugin list. We don't use a > lock around the whole LoadPlugins method for performance reasons, so we > construct the new plugins array separately, and then only swap it out at the end > (hence the extra |plugins| parameter). > > Jakob, for AddToPluginGroups we should probably add an additional parameter as > well. Sorry I didn't catch that in the first CL. Done. http://codereview.chromium.org/5699005/diff/8002/webkit/glue/plugins/plugin_l... File webkit/glue/plugins/plugin_list.h (right): http://codereview.chromium.org/5699005/diff/8002/webkit/glue/plugins/plugin_l... webkit/glue/plugins/plugin_list.h:76: enum DisabledElementReason { USER = 0, POLICY, POLICY_AND_USER }; On 2010/12/14 09:43:39, danno wrote: > How about PluginDisabledReason? Done. http://codereview.chromium.org/5699005/diff/8002/webkit/glue/plugins/plugin_l... webkit/glue/plugins/plugin_list.h:78: typedef std::map<string16, DisabledElementReason> DisabledGroupsList; On 2010/12/14 09:43:39, danno wrote: > This is a map, so the list name is misleading. consider renaming to just > DisabledPlugins and DisabledGroups. Done. http://codereview.chromium.org/5699005/diff/8002/webkit/glue/plugins/plugin_l... webkit/glue/plugins/plugin_list.h:203: // will be disabled. The boolean flag should be set to true if the operation On 2010/12/14 09:43:39, danno wrote: > ... if the plugin is disabled by a policy and not a user. Done. http://codereview.chromium.org/5699005/diff/8002/webkit/glue/plugins/plugin_l... webkit/glue/plugins/plugin_list.h:222: void GetDisabledGroups(DisabledGroupsList* disabled_groups); On 2010/12/14 09:43:39, danno wrote: > Do these copy? If so, why (probably should document in a comment). Alternatively > (probably better) why not just return a const reference? Done. http://codereview.chromium.org/5699005/diff/8002/webkit/glue/plugins/plugin_l... webkit/glue/plugins/plugin_list.h:244: // Populates the |plugins_| vector if is has been marked as dirty. On 2010/12/14 09:43:39, danno wrote: > "it" missing? Done. s/is/it/ http://codereview.chromium.org/5699005/diff/8002/webkit/glue/plugins/plugin_l... webkit/glue/plugins/plugin_list.h:315: bool plugins_vector_is_dirty_; On 2010/12/14 09:43:39, danno wrote: > Please document when this actually happens. Obsoleted, http://codereview.chromium.org/5699005/diff/8002/webkit/glue/plugins/plugin_l... File webkit/glue/plugins/plugin_list_posix.cc (right): http://codereview.chromium.org/5699005/diff/8002/webkit/glue/plugins/plugin_l... webkit/glue/plugins/plugin_list_posix.cc:249: for (PluginGroup::PluginMap::iterator it = plugin_groups_.begin(); On 2010/12/14 09:43:39, danno wrote: > now that you have two iterators, you probably want to use names to disambiguate. > e.g. current_curent instead of it and current_plugin_info instead of itp. Done. I guess current_curent meant to be current_group? And I prefer to keep the inner one it for better readability.
http://codereview.chromium.org/5699005/diff/30002/webkit/glue/plugins/plugin_... File webkit/glue/plugins/plugin_list.cc (right): http://codereview.chromium.org/5699005/diff/30002/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list.cc:686: // callers of |AddToPluginGroups|. Nit: Leftover comment? http://codereview.chromium.org/5699005/diff/30002/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list.cc:699: return did_enable; // Early exit if plugin not in disabled list. Please update this comment. http://codereview.chromium.org/5699005/diff/30002/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list.cc:717: disabled_groups_.erase(entry); Nit: just disabled_groups_.erase(group->GetGroupName()); http://codereview.chromium.org/5699005/diff/30002/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list.cc:736: entry->second = POLICY_AND_USER; Nit: Braces around this line. http://codereview.chromium.org/5699005/diff/30002/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list.cc:737: return did_disable; // Early exit if plugin already in disabled list. Nit: Update comment. http://codereview.chromium.org/5699005/diff/37001/webkit/glue/plugins/plugin_... File webkit/glue/plugins/plugin_group.h (right): http://codereview.chromium.org/5699005/diff/37001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.h:106: WebPluginInfo** group_plugin_copy); Nit: Can you explain what |group_plugin_copy| is for? http://codereview.chromium.org/5699005/diff/37001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.h:108: // Returns a poitner to the plugin at the end of the |web_plugin_infos_| Nit: Pointer. Also, it's a reference. Also, make it const? http://codereview.chromium.org/5699005/diff/37001/webkit/glue/plugins/plugin_... File webkit/glue/plugins/plugin_list.cc (right): http://codereview.chromium.org/5699005/diff/37001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list.cc:380: plugins_ = new_plugins; What happens to the old pointers in |plugins_|? Do we leak them? What's the reason we're now using pointers, BTW?
http://codereview.chromium.org/5699005/diff/37001/webkit/glue/plugins/plugin_... File webkit/glue/plugins/plugin_list.cc (right): http://codereview.chromium.org/5699005/diff/37001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list.cc:317: std::vector<WebPluginInfo*> new_plugins; why is this change necessary? http://codereview.chromium.org/5699005/diff/37001/webkit/glue/plugins/plugin_... File webkit/glue/plugins/plugin_list_win.cc (right): http://codereview.chromium.org/5699005/diff/37001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list_win.cc:335: for (std::vector<WebPluginInfo*>::iterator it = plugins->begin(); I think the former code is more readable. please don't change code style when updating it to match personal taste, as that leads to a lot of hysteresis if everyone does it http://codereview.chromium.org/5699005/diff/37001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list_win.cc:397: for (std::vector<WebPluginInfo*>::iterator it = plugins->begin(); ditto http://codereview.chromium.org/5699005/diff/37001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list_win.cc:408: for (std::vector<WebPluginInfo*>::iterator it = plugins->begin(); ditto
http://codereview.chromium.org/5699005/diff/37001/webkit/glue/plugins/plugin_... File webkit/glue/plugins/plugin_group.cc (right): http://codereview.chromium.org/5699005/diff/37001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.cc:306: for (std::list<WebPluginInfo>::const_iterator it = web_plugin_infos_.begin(); ditto, using an iterator here makes the code unnecessarily more verbose http://codereview.chromium.org/5699005/diff/37001/webkit/glue/plugins/plugin_... File webkit/glue/plugins/plugin_group.h (right): http://codereview.chromium.org/5699005/diff/37001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.h:213: std::list<WebPluginInfo> web_plugin_infos_; I don't understand why this was changed into a list? I don't see items being removed from it, so why is a vector not good enough?
I only addressed the most important points about design decisions which need further discussion. I will consider the style points tomorrow. http://codereview.chromium.org/5699005/diff/37001/webkit/glue/plugins/plugin_... File webkit/glue/plugins/plugin_group.h (right): http://codereview.chromium.org/5699005/diff/37001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.h:213: std::list<WebPluginInfo> web_plugin_infos_; On 2010/12/15 19:48:55, John Abd-El-Malek wrote: > I don't understand why this was changed into a list? I don't see items being > removed from it, so why is a vector not good enough? Because pointers to elements of a vector might get invalidated if the vector is resized (which did happen all the time). So in order to make that adding new elements won't invalidate pointers to old ones we decided to make it a list. http://codereview.chromium.org/5699005/diff/37001/webkit/glue/plugins/plugin_... File webkit/glue/plugins/plugin_list.cc (right): http://codereview.chromium.org/5699005/diff/37001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list.cc:317: std::vector<WebPluginInfo*> new_plugins; On 2010/12/15 18:25:50, John Abd-El-Malek wrote: > why is this change necessary? Before we got a copy of the plugins in PluginsList::plugins_ and in the PluginGroup for that plugin. Therefore all code handling the state of plugin had to make sure the state of these was kept consistent as both are used in different contexts. However this tends to be not so trivial given the different contexts these happen to occur. In order to resolve this problem it is better to have only one instance of a WebPluginInfo per plugin. I haven't found any better way than this though to do it yet. Maybe using some sort of smart pointers can be a better solution but still doesn't solve the problem who owns that element. In general I see a long term solution in completely scraping the plugins_ member in favor of using directly the groups. However this is a major refactoring that is a bit outside the scope of that bugfix. I am open for other solutions here though. http://codereview.chromium.org/5699005/diff/37001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list.cc:380: plugins_ = new_plugins; On 2010/12/15 17:23:45, Bernhard Bauer wrote: > What happens to the old pointers in |plugins_|? Do we leak them? What's the > reason we're now using pointers, BTW? About the reason for pointers please look at the response to John's question above. As of the pointers being leaked - no, we don't leak then because the groups own the objects behind these pointers. And because the groups are being built and owned by the PluginList we can make sure the pointers never outlive the objects. All functions that give information about plugins in the system only make copies of these objects so they can't pass pointer that might get 0xDEADBEEF-ed while in use. http://codereview.chromium.org/5699005/diff/37001/webkit/glue/plugins/plugin_... File webkit/glue/plugins/plugin_list_win.cc (right): http://codereview.chromium.org/5699005/diff/37001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list_win.cc:335: for (std::vector<WebPluginInfo*>::iterator it = plugins->begin(); On 2010/12/15 18:25:50, John Abd-El-Malek wrote: > I think the former code is more readable. please don't change code style when > updating it to match personal taste, as that leads to a lot of hysteresis if > everyone does it Actually I only tried to bring in some consistency in the code base. We had in various files both for(int... and for(iterator... constructs. I am sorry if I did the wrong thing and if you say it's better to not change such things I won't do so, but I thought we want to have consistency of the code doing the same thing all over the place.
Based on the first two comments, it seems to me that we should wait and do this change cleanly, instead of making this already-complicated code even more complicated just to fix a bug that's not urgent. i.e. having a data structures from two classes point to elements in each other seems like a big warning sign to me. The PluginGroup changes were bolted on in probably not the cleanest way, and the resulting code makes it difficult to change PluginList as your change shows. I think the answer is what you alluded to below, i.e. refactoring this code and making it clean. In the meantime we shouldn't make it more complicated, since that'll make it harder to read and refactor. On Wed, Dec 15, 2010 at 1:56 PM, <pastarmovj@chromium.org> wrote: > I only addressed the most important points about design decisions which > need > further discussion. I will consider the style points tomorrow. > > > > > http://codereview.chromium.org/5699005/diff/37001/webkit/glue/plugins/plugin_... > File webkit/glue/plugins/plugin_group.h (right): > > > http://codereview.chromium.org/5699005/diff/37001/webkit/glue/plugins/plugin_... > webkit/glue/plugins/plugin_group.h:213: std::list<WebPluginInfo> > web_plugin_infos_; > On 2010/12/15 19:48:55, John Abd-El-Malek wrote: > >> I don't understand why this was changed into a list? I don't see >> > items being > >> removed from it, so why is a vector not good enough? >> > > Because pointers to elements of a vector might get invalidated if the > vector is resized (which did happen all the time). So in order to make > that adding new elements won't invalidate pointers to old ones we > decided to make it a list. > > > > http://codereview.chromium.org/5699005/diff/37001/webkit/glue/plugins/plugin_... > File webkit/glue/plugins/plugin_list.cc (right): > > > http://codereview.chromium.org/5699005/diff/37001/webkit/glue/plugins/plugin_... > webkit/glue/plugins/plugin_list.cc:317: std::vector<WebPluginInfo*> > new_plugins; > On 2010/12/15 18:25:50, John Abd-El-Malek wrote: > >> why is this change necessary? >> > > Before we got a copy of the plugins in PluginsList::plugins_ and in the > PluginGroup for that plugin. Therefore all code handling the state of > plugin had to make sure the state of these was kept consistent as both > are used in different contexts. However this tends to be not so trivial > given the different contexts these happen to occur. In order to resolve > this problem it is better to have only one instance of a WebPluginInfo > per plugin. I haven't found any better way than this though to do it > yet. Maybe using some sort of smart pointers can be a better solution > but still doesn't solve the problem who owns that element. In general I > see a long term solution in completely scraping the plugins_ member in > favor of using directly the groups. However this is a major refactoring > that is a bit outside the scope of that bugfix. > > I am open for other solutions here though. > > > http://codereview.chromium.org/5699005/diff/37001/webkit/glue/plugins/plugin_... > webkit/glue/plugins/plugin_list.cc:380: plugins_ = new_plugins; > > On 2010/12/15 17:23:45, Bernhard Bauer wrote: > >> What happens to the old pointers in |plugins_|? Do we leak them? >> > What's the > >> reason we're now using pointers, BTW? >> > > About the reason for pointers please look at the response to John's > question above. > > As of the pointers being leaked - no, we don't leak then because the > groups own the objects behind these pointers. And because the groups are > being built and owned by the PluginList we can make sure the pointers > never outlive the objects. All functions that give information about > plugins in the system only make copies of these objects so they can't > pass pointer that might get 0xDEADBEEF-ed while in use. > > > > http://codereview.chromium.org/5699005/diff/37001/webkit/glue/plugins/plugin_... > File webkit/glue/plugins/plugin_list_win.cc (right): > > > http://codereview.chromium.org/5699005/diff/37001/webkit/glue/plugins/plugin_... > webkit/glue/plugins/plugin_list_win.cc:335: for > (std::vector<WebPluginInfo*>::iterator it = plugins->begin(); > On 2010/12/15 18:25:50, John Abd-El-Malek wrote: > >> I think the former code is more readable. please don't change code >> > style when > >> updating it to match personal taste, as that leads to a lot of >> > hysteresis if > >> everyone does it >> > > Actually I only tried to bring in some consistency in the code base. We > had in various files both for(int... and for(iterator... constructs. I > am sorry if I did the wrong thing and if you say it's better to not > change such things I won't do so, but I thought we want to have > consistency of the code doing the same thing all over the place. > > > http://codereview.chromium.org/5699005/ >
I did the refactoring as we spoke with John yesterday. For my side I am very happy with the results - overall reduced heavily the LOC count without sacrificing anything. I think the priority is not needed anymore but I'd like to hear Bernhard on that point as well. therefore I'd like to hear at least both John's and Bernhard's LGTM on that before I land it.
I would really prefer not landing this until we get proper test coverage here, either by landing http://codereview.chromium.org/5783005/ or by adding unit tests here. Additional things you might want to test: * Enabling/disabling plugins before the list of plugins is loaded should have the same effect as enabling/disabling them afterwards * Updating a plugin (i.e. replacing a plugin with one with a different FilePath but which is still in the same PluginGroup) should keep the enabled/disabled state * A plug-in coming first in the list of plugins should always take precedence over a later one, even if we store the plugins in groups in a map. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... File webkit/glue/plugins/plugin_group.h (right): http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.h:98: // Retuns true if plugin has been added and false if it was already added. Nit: "Returns true if the plugin has been added and false if it was already contained in the group before". http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.h:109: void RefreshEnabledState(); Can we make this private? http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.h:124: bool HasPlugin(const FilePath& path) const; Nit: Maybe ContainsPlugin? http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... File webkit/glue/plugins/plugin_list.cc (right): http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list.cc:646: PluginGroup* PluginList::AddToPluginGroups( There is already a method AddToPluginGroups, and we don't allow overloading. Also, are you sure you're not going to get inconsistencies if you create a new WebPluginInfo with the other fields empty? http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list.cc:661: // No such group yet add one as a placeholder. Nit: Add punctuation please: "No such group yet; add one..." http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list.cc:673: // No such group yet add one as a placeholder. Same nit here. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... File webkit/glue/plugins/plugin_list.h (right): http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list.h:195: bool EnablePlugin(const FilePath& filename, const string16& name); Can you explain what |name| is for? Are both required? What if they don't match? http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list.h:201: // disabled by a policy and not a user. That comment doesn't seem right. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/webplug... File webkit/glue/plugins/webplugininfo.cc (right): http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/webplug... webkit/glue/plugins/webplugininfo.cc:83: if (IsManaged(reason)) return false; Nit: The coding style says it's okay, but personally I think I'd slightly prefer putting the return on a new line, because it draws more attention to the fact that the control flow is possibly changing here. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/webplug... webkit/glue/plugins/webplugininfo.cc:99: if (!allow_wildcard && mime_info.mime_type == "*") { Nit: Braces unnecessary. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/webplug... File webkit/glue/plugins/webplugininfo.h (right): http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/webplug... webkit/glue/plugins/webplugininfo.h:55: // Returns true if the plugin supports "mime-type". |mime_type| should be all Nit: Why the quotes? http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/webplug... webkit/glue/plugins/webplugininfo.h:61: // be set to the mime type if found. The mime type which corresponds to the Mini-nit: "MIME type" is usually written in caps, we just use lower case in variable names. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/webplug... webkit/glue/plugins/webplugininfo.h:85: int reason; If you add a new field to WebPluginInfo, you should also update chrome/common/render_messages.{h,cc} to serialize the fields, otherwise when you send a WebPluginInfo over IPC, hilarity will ensue. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/webplug... webkit/glue/plugins/webplugininfo.h:87: // Constants definig bit fields in the reason member. Nit: defining http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/webplug... webkit/glue/plugins/webplugininfo.h:91: // Priority of the plugin (obsolete?) At least it shouldn't be here. The priority is not an attribute of the plugin itself, it's just its index in the list of plugins.
thanks a lot for doing this refactoring :) some comments http://codereview.chromium.org/5699005/diff/66001/chrome/browser/plugin_updat... File chrome/browser/plugin_updater.cc (right): http://codereview.chromium.org/5699005/diff/66001/chrome/browser/plugin_updat... chrome/browser/plugin_updater.cc:284: for (std::vector<PluginGroup>::const_iterator it = groups.begin(); the old code using a size_t instead of an iterator to loop over is more readable. same for line 272 http://codereview.chromium.org/5699005/diff/66001/chrome/browser/plugin_updat... File chrome/browser/plugin_updater.h (right): http://codereview.chromium.org/5699005/diff/66001/chrome/browser/plugin_updat... chrome/browser/plugin_updater.h:37: const string16& name); can you add documentation to what "name" is? why is it needed now? http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... File webkit/glue/plugins/plugin_group.cc (right): http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.cc:91: enabled_ = false; curious why this doesn't get copied from other? http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.cc:98: std::vector<WebPluginInfo>::const_iterator it = please use size_t to iterate as before, it's much more readable http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.cc:233: for (std::vector<WebPluginInfo>::iterator it = web_plugin_infos_.begin(); nit: please don't switch this to an iterator http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.cc:305: for (std::vector<WebPluginInfo>::const_iterator it = nit: size_t please http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.cc:334: for (std::vector<WebPluginInfo>::const_iterator it = nit: size_t http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.cc:355: for (std::vector<WebPluginMimeType>::const_iterator type_it = nit: size_t http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.cc:423: for (std::vector<WebPluginInfo>::iterator it = web_plugin_infos_.begin(); nit: size_t http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.cc:450: for (std::vector<WebPluginInfo>::iterator it = web_plugin_infos_.begin(); nit: size_t http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.cc:467: for (std::vector<WebPluginInfo>::iterator it = web_plugin_infos_.begin(); nit: size_t http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... File webkit/glue/plugins/plugin_group.h (right): http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.h:9: #include <list> not needed? http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.h:99: bool AddPlugin(const WebPluginInfo& plugin, int priority); can you document what priority is? although i dont think it's really needed http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.h:101: bool EnablePlugin(const FilePath& filename); comments please http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.h:106: bool Enable(bool enable); nit: perhaps call this EnabledGroup to make it clear now that you have EnablePlugin http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... File webkit/glue/plugins/plugin_list.cc (right): http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list.cc:419: for (std::vector<WebPluginInfo>::const_iterator it = gr_plugins.begin(); size_t http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list.cc:449: for (std::vector<WebPluginInfo>::const_iterator it = plugins.begin(); size_t http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list.cc:472: for (std::vector<WebPluginInfo>::const_iterator it = plugins.begin(); size_t http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list.cc:492: for (std::vector<WebPluginInfo>::const_iterator it = plugins.begin(); size_t http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list.cc:558: for (std::vector<WebPluginInfo>::const_iterator it = plugins.begin(); size_t http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list.cc:654: bool PluginList::EnablePlugin(const FilePath& filename, const string16& name) { I still don't understand why name is needed :) http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... File webkit/glue/plugins/plugin_list.h (right): http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list.h:201: // disabled by a policy and not a user. nit: using boolean to specify whether it's user/policy makes it hard to read at the caller's site, and also limiting in the future in case we want to disable through other means (say because the plugin is out-of-date). please use an enum instead http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list.h:283: void LoadPluginsFromRegistry(std::set<FilePath>* visited_plugins, why switch these parameters' order? http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list.h:314: mutable Lock lock_; why make this mutable? if locking is required, seems the function don't need to be const? that would be a hint to the caller that some work can be required.. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... File webkit/glue/plugins/plugin_list_mac.mm (right): http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list_mac.mm:100: for (std::vector<WebPluginInfo>::iterator it = plugins->begin(); please leave as before http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... File webkit/glue/plugins/plugin_list_posix.cc (right): http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list_posix.cc:250: for (std::vector<WebPluginInfo>::iterator it = plugins->begin(); please leave as before http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... File webkit/glue/plugins/plugin_list_win.cc (right): http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list_win.cc:335: for (std::vector<WebPluginInfo>::iterator it = plugins->begin(); please leave as before http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list_win.cc:397: for (std::vector<WebPluginInfo>::iterator it = plugins->begin(); please leave as before http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list_win.cc:402: // possibly disable it first. why can't we remove it like this? why is this a TODO, i.e. why not fix this now? http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list_win.cc:408: for (std::vector<WebPluginInfo>::iterator it = plugins->begin(); please leave as before http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/webplug... File webkit/glue/plugins/webplugininfo.h (right): http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/webplug... webkit/glue/plugins/webplugininfo.h:30: struct WebPluginInfo { note that per the style guide, if this has methods, it should be a class instead of a struct. this will mean we'd have to add getters/setters and make the members private, and add underscores at the end. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/webplug... webkit/glue/plugins/webplugininfo.h:45: bool Enable(int reason); please use an enum instead of a reason, that way you force callers to use valid values. the enum values should be compatible with a bitfield, i.e. enum DisabledReason { NONE = 0, USER = 1 << 1, ADMINISTRATOR or GROUP_POLICY = 1 << 2, http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/webplug... webkit/glue/plugins/webplugininfo.h:89: static const int MANAGED; nit: the meaning of managed is a little cryptic. perhaps user and group policy, just to be very clear? http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/webplug... webkit/glue/plugins/webplugininfo.h:92: int priority; i think the order of plugins in a plugin group should be enough to denote the priority?
http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/webplug... File webkit/glue/plugins/webplugininfo.h (right): http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/webplug... webkit/glue/plugins/webplugininfo.h:92: int priority; On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > i think the order of plugins in a plugin group should be enough to denote the > priority? The order inside a group is probably not sufficient. The internal PDF viewer and Acrobat Reader for example are in different groups, but the PDF viewer as a bundled plug-in has higher priority.
Addressed the comments, fixed some bugs and introduced unit testing on the plugins_list (thanks to Jakob). I think the unit testing coverage now is quite good and combined with the rest of the integration tests we are pretty well tested. http://codereview.chromium.org/5699005/diff/66001/chrome/browser/plugin_updat... File chrome/browser/plugin_updater.cc (right): http://codereview.chromium.org/5699005/diff/66001/chrome/browser/plugin_updat... chrome/browser/plugin_updater.cc:284: for (std::vector<PluginGroup>::const_iterator it = groups.begin(); On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > the old code using a size_t instead of an iterator to loop over is more > readable. same for line 272 Done. http://codereview.chromium.org/5699005/diff/66001/chrome/browser/plugin_updat... File chrome/browser/plugin_updater.h (right): http://codereview.chromium.org/5699005/diff/66001/chrome/browser/plugin_updat... chrome/browser/plugin_updater.h:37: const string16& name); On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > can you add documentation to what "name" is? why is it needed now? Done. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... File webkit/glue/plugins/plugin_group.cc (right): http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.cc:91: enabled_ = false; On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > curious why this doesn't get copied from other? This whole function will be changed in CL 5783005 from Jakob anyhow. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.cc:98: std::vector<WebPluginInfo>::const_iterator it = On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > please use size_t to iterate as before, it's much more readable Same as above. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.cc:233: for (std::vector<WebPluginInfo>::iterator it = web_plugin_infos_.begin(); On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > nit: please don't switch this to an iterator Done. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.cc:305: for (std::vector<WebPluginInfo>::const_iterator it = On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > nit: size_t please Done. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.cc:334: for (std::vector<WebPluginInfo>::const_iterator it = On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > nit: size_t Done. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.cc:355: for (std::vector<WebPluginMimeType>::const_iterator type_it = On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > nit: size_t I would prefer to keep this one an iterator as well as the inner for or else we'll end up having web_plugin_infos_[i].mime_types[j].file_extensions[k] which is anything but understandable. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.cc:423: for (std::vector<WebPluginInfo>::iterator it = web_plugin_infos_.begin(); On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > nit: size_t Done. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.cc:450: for (std::vector<WebPluginInfo>::iterator it = web_plugin_infos_.begin(); On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > nit: size_t Done. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.cc:467: for (std::vector<WebPluginInfo>::iterator it = web_plugin_infos_.begin(); On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > nit: size_t Done. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... File webkit/glue/plugins/plugin_group.h (right): http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.h:9: #include <list> On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > not needed? Done. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.h:98: // Retuns true if plugin has been added and false if it was already added. On 2010/12/17 18:50:59, Bernhard Bauer wrote: > Nit: "Returns true if the plugin has been added and false if it was already > contained in the group before". Done. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.h:101: bool EnablePlugin(const FilePath& filename); On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > comments please Done. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.h:106: bool Enable(bool enable); On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > nit: perhaps call this EnabledGroup to make it clear now that you have > EnablePlugin Done. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.h:109: void RefreshEnabledState(); On 2010/12/17 18:50:59, Bernhard Bauer wrote: > Can we make this private? Done. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.h:124: bool HasPlugin(const FilePath& path) const; On 2010/12/17 18:50:59, Bernhard Bauer wrote: > Nit: Maybe ContainsPlugin? Done. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... File webkit/glue/plugins/plugin_list.cc (right): http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list.cc:419: for (std::vector<WebPluginInfo>::const_iterator it = gr_plugins.begin(); On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > size_t Done. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list.cc:449: for (std::vector<WebPluginInfo>::const_iterator it = plugins.begin(); On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > size_t Done. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list.cc:472: for (std::vector<WebPluginInfo>::const_iterator it = plugins.begin(); On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > size_t Done. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list.cc:492: for (std::vector<WebPluginInfo>::const_iterator it = plugins.begin(); On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > size_t Done. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list.cc:558: for (std::vector<WebPluginInfo>::const_iterator it = plugins.begin(); On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > size_t Done. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list.cc:646: PluginGroup* PluginList::AddToPluginGroups( On 2010/12/17 18:50:59, Bernhard Bauer wrote: > There is already a method AddToPluginGroups, and we don't allow overloading. > > Also, are you sure you're not going to get inconsistencies if you create a new > WebPluginInfo with the other fields empty? Renamed to AddPlaceholderToPluginGroup. I explicitly check if I am trying to add new plugin or update such a placeholder in the AddToPluginGroup function to prevent inconsistencies. This is still better than juggling with the disabld_plugins_ list we had before. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list.cc:654: bool PluginList::EnablePlugin(const FilePath& filename, const string16& name) { On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > I still don't understand why name is needed :) For us to be able to have a meaningful placeholder for a plugin we need a bit more than a filename. Especially because group names are built from plugin names. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list.cc:661: // No such group yet add one as a placeholder. On 2010/12/17 18:50:59, Bernhard Bauer wrote: > Nit: Add punctuation please: "No such group yet; add one..." Done. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list.cc:673: // No such group yet add one as a placeholder. On 2010/12/17 18:50:59, Bernhard Bauer wrote: > Same nit here. Done. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... File webkit/glue/plugins/plugin_list.h (right): http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list.h:195: bool EnablePlugin(const FilePath& filename, const string16& name); On 2010/12/17 18:50:59, Bernhard Bauer wrote: > Can you explain what |name| is for? Are both required? What if they don't match? Done. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list.h:201: // disabled by a policy and not a user. On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > nit: using boolean to specify whether it's user/policy makes it hard to read at > the caller's site, and also limiting in the future in case we want to disable > through other means (say because the plugin is out-of-date). please use an enum > instead I fixed the comment to make clear what the return flag means. It doesn't mean weather the plugin is policy enabled or disabled but only whether the change requested could be performed. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list.h:283: void LoadPluginsFromRegistry(std::set<FilePath>* visited_plugins, On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > why switch these parameters' order? Not by intention. Fixed. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list.h:314: mutable Lock lock_; On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > why make this mutable? if locking is required, seems the function don't need to > be const? that would be a hint to the caller that some work can be required.. I lock some getters to prevent writing happening while copying the output. The getters don't change anything and could well function in a const situation. In general I try to be as const friendly as possible but if you thing it's better to do it otherwise I will change it. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... File webkit/glue/plugins/plugin_list_mac.mm (right): http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list_mac.mm:100: for (std::vector<WebPluginInfo>::iterator it = plugins->begin(); On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > please leave as before Done. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... File webkit/glue/plugins/plugin_list_posix.cc (right): http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list_posix.cc:250: for (std::vector<WebPluginInfo>::iterator it = plugins->begin(); On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > please leave as before Done. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... File webkit/glue/plugins/plugin_list_win.cc (right): http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list_win.cc:335: for (std::vector<WebPluginInfo>::iterator it = plugins->begin(); On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > please leave as before Done. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list_win.cc:397: for (std::vector<WebPluginInfo>::iterator it = plugins->begin(); On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > please leave as before Done. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list_win.cc:402: // possibly disable it first. On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > why can't we remove it like this? why is this a TODO, i.e. why not fix this > now? Now we can again :) http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list_win.cc:408: for (std::vector<WebPluginInfo>::iterator it = plugins->begin(); On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > please leave as before Done. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/webplug... File webkit/glue/plugins/webplugininfo.cc (right): http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/webplug... webkit/glue/plugins/webplugininfo.cc:83: if (IsManaged(reason)) return false; On 2010/12/17 18:50:59, Bernhard Bauer wrote: > Nit: The coding style says it's okay, but personally I think I'd slightly prefer > putting the return on a new line, because it draws more attention to the fact > that the control flow is possibly changing here. Done. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/webplug... webkit/glue/plugins/webplugininfo.cc:99: if (!allow_wildcard && mime_info.mime_type == "*") { On 2010/12/17 18:50:59, Bernhard Bauer wrote: > Nit: Braces unnecessary. Done. This is legacy code just moved here but I tuned it too. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/webplug... File webkit/glue/plugins/webplugininfo.h (right): http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/webplug... webkit/glue/plugins/webplugininfo.h:30: struct WebPluginInfo { On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > note that per the style guide, if this has methods, it should be a class instead > of a struct. this will mean we'd have to add getters/setters and make the > members private, and add underscores at the end. I don't think that we should do this (at least not in this CL) - it is going to have huge impact on various files and make reviewing of this anyhow overgrown CL harder. I'd suggest if this CL is accepted without much further refactoring to file this as a bug and have it fixed as soon as possible. The few methods are really only there to provide simple means of working with the fields and to prevent code duplication. I could extract them as functions in a namespace and provide them with a parameter being the object to modify but this is not going to make the code more readable. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/webplug... webkit/glue/plugins/webplugininfo.h:45: bool Enable(int reason); On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > please use an enum instead of a reason, that way you force callers to use valid > values. the enum values should be compatible with a bitfield, i.e. > > enum DisabledReason { > NONE = 0, > USER = 1 << 1, > ADMINISTRATOR or GROUP_POLICY = 1 << 2, Done. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/webplug... webkit/glue/plugins/webplugininfo.h:55: // Returns true if the plugin supports "mime-type". |mime_type| should be all On 2010/12/17 18:50:59, Bernhard Bauer wrote: > Nit: Why the quotes? Done. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/webplug... webkit/glue/plugins/webplugininfo.h:61: // be set to the mime type if found. The mime type which corresponds to the On 2010/12/17 18:50:59, Bernhard Bauer wrote: > Mini-nit: "MIME type" is usually written in caps, we just use lower case in > variable names. Done. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/webplug... webkit/glue/plugins/webplugininfo.h:85: int reason; On 2010/12/17 18:50:59, Bernhard Bauer wrote: > If you add a new field to WebPluginInfo, you should also update > chrome/common/render_messages.{h,cc} to serialize the fields, otherwise when you > send a WebPluginInfo over IPC, hilarity will ensue. Done. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/webplug... webkit/glue/plugins/webplugininfo.h:87: // Constants definig bit fields in the reason member. On 2010/12/17 18:50:59, Bernhard Bauer wrote: > Nit: defining Removed anyhow http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/webplug... webkit/glue/plugins/webplugininfo.h:89: static const int MANAGED; On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > nit: the meaning of managed is a little cryptic. perhaps user and group policy, > just to be very clear? Managed is the name used in the policy world to identify a preference that is controlled by a greater power than the user. I prefer to keep the name managed so that it is consistent with the rest of the policy talk in chrome. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/webplug... webkit/glue/plugins/webplugininfo.h:92: int priority; On 2010/12/17 19:26:40, Bernhard Bauer wrote: > On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > > i think the order of plugins in a plugin group should be enough to denote the > > priority? > > The order inside a group is probably not sufficient. The internal PDF viewer and > Acrobat Reader for example are in different groups, but the PDF viewer as a > bundled plug-in has higher priority. We could only think of one synthetic case where order could be important - two separate groups both having internal and external plugins and all of them handling the same MIME types. This would lead to having the preference set to first group's internal plugin then first group's external one and then the internal of the other group which should be rather internal 1, internal 2, external 1... Moreover now I use a special value of this field - 0 to specify that the Plugin is only a placeholder and not a real active plugin. I would like to know if you think the situation I described above could arise and should be treated properly or if I should remove this field and introduce a boolean flag is_placeholder which will be enough for my needs.
http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... File webkit/glue/plugins/plugin_group.cc (right): http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.cc:91: enabled_ = false; On 2010/12/20 19:57:37, pastarmovj wrote: > On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > > curious why this doesn't get copied from other? > > This whole function will be changed in CL 5783005 from Jakob anyhow. It's fine that this function will change in the future, but our tree should always be shippable and anyone can sync to a specific revision. If I sync after you commit, this code will be confusing to me that it only copies some values but not others. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.cc:98: std::vector<WebPluginInfo>::const_iterator it = On 2010/12/20 19:57:37, pastarmovj wrote: > On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > > please use size_t to iterate as before, it's much more readable > > Same as above. ditto, I see no reason to change the existing code from a size_t to an iterator and just say it's changing in the future. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.cc:355: for (std::vector<WebPluginMimeType>::const_iterator type_it = On 2010/12/20 19:57:37, pastarmovj wrote: > On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > > nit: size_t > > I would prefer to keep this one an iterator as well as the inner for or else > we'll end up having web_plugin_infos_[i].mime_types[j].file_extensions[k] which > is anything but understandable. you can always create a local const reference, i.e. const WebPluginMimeType& mime_type = web_plugin_infos_[i].mime_types[j] it's nice to be consistent in a file for iterating over the same container http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... File webkit/glue/plugins/plugin_list.h (right): http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list.h:201: // disabled by a policy and not a user. On 2010/12/20 19:57:37, pastarmovj wrote: > On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > > nit: using boolean to specify whether it's user/policy makes it hard to read > at > > the caller's site, and also limiting in the future in case we want to disable > > through other means (say because the plugin is out-of-date). please use an > enum > > instead > > I fixed the comment to make clear what the return flag means. It doesn't mean > weather the plugin is policy enabled or disabled but only whether the change > requested could be performed. oops, i meant to add this comment on line 209. so is this function only used to disable plugins based on user action, and not on group policy? http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/webplug... File webkit/glue/plugins/webplugininfo.h (right): http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/webplug... webkit/glue/plugins/webplugininfo.h:30: struct WebPluginInfo { On 2010/12/20 19:57:37, pastarmovj wrote: > On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > > note that per the style guide, if this has methods, it should be a class > instead > > of a struct. this will mean we'd have to add getters/setters and make the > > members private, and add underscores at the end. > > I don't think that we should do this (at least not in this CL) - it is going to > have huge impact on various files and make reviewing of this anyhow overgrown CL > harder. I'd suggest if this CL is accepted without much further refactoring to > file this as a bug and have it fixed as soon as possible. The few methods are > really only there to provide simple means of working with the fields and to > prevent code duplication. I could extract them as functions in a namespace and > provide them with a parameter being the object to modify but this is not going > to make the code more readable. The issue is that the coding guidelines are pretty strict about structs only carrying data, and classes having functions to modify it etc. We shouldn't give up the consistency of the guidelines unless we have a very compelling reason, and I don't think it exists here. If you want to make it a class in a future change, then that change should make these functions be member functions. But until that is done, then it seems that the functions have to stay in PluginList/PluginGroup and take a WebPluginInfo to operate on. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/webplug... webkit/glue/plugins/webplugininfo.h:92: int priority; I'm really against having this concept of order because I think it unnecessarily complicates things. We didn't need this previously because the order of the plugin_list denoted the priority, since we always searched from the beginning. Since the plugin list is now generated from the plugin groups, it seems we should just be able to keep plugin groups in a vector and we don't need an ordering. An ordering is currently needed because the internal and external flash are in the same group. It seems that if we don't group them, and instead treat them separately like the internal PDF plugin and Adobe Reader are treated, then this problem goes away.
http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/webplug... File webkit/glue/plugins/webplugininfo.h (right): http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/webplug... webkit/glue/plugins/webplugininfo.h:92: int priority; On 2010/12/20 19:57:37, pastarmovj wrote: > On 2010/12/17 19:26:40, Bernhard Bauer wrote: > > On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > > > i think the order of plugins in a plugin group should be enough to denote > the > > > priority? > > > > The order inside a group is probably not sufficient. The internal PDF viewer > and > > Acrobat Reader for example are in different groups, but the PDF viewer as a > > bundled plug-in has higher priority. > > We could only think of one synthetic case where order could be important - two > separate groups both having internal and external plugins and all of them > handling the same MIME types. This would lead to having the preference set to > first group's internal plugin then first group's external one and then the > internal of the other group which should be rather internal 1, internal 2, > external 1... Moreover now I use a special value of this field - 0 to specify > that the Plugin is only a placeholder and not a real active plugin. I would like > to know if you think the situation I described above could arise and should be > treated properly or if I should remove this field and introduce a boolean flag > is_placeholder which will be enough for my needs. one more thing: if you add a is_placeholder, then the right place is not WebPluginInfo, which is simply a description of a plugin. That's a variable used in the internal workings of PluginGroup, so that class should store this information in another way, and not complicate WebPluginInfo just because of its own implementation
http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... File webkit/glue/plugins/plugin_group.cc (right): http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.cc:91: enabled_ = false; On 2010/12/20 20:56:59, John Abd-El-Malek wrote: > On 2010/12/20 19:57:37, pastarmovj wrote: > > On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > > > curious why this doesn't get copied from other? > > > > This whole function will be changed in CL 5783005 from Jakob anyhow. > > It's fine that this function will change in the future, but our tree should > always be shippable and anyone can sync to a specific revision. If I sync after > you commit, this code will be confusing to me that it only copies some values > but not others. Actually it is better to say that this function will change in a commit that will precede mine: http://codereview.chromium.org/5918003/diff/1/webkit/glue/plugins/plugin_grou... http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.cc:98: std::vector<WebPluginInfo>::const_iterator it = On 2010/12/20 20:56:59, John Abd-El-Malek wrote: > On 2010/12/20 19:57:37, pastarmovj wrote: > > On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > > > please use size_t to iterate as before, it's much more readable > > > > Same as above. > > ditto, I see no reason to change the existing code from a size_t to an iterator > and just say it's changing in the future. Same as above no loop in the code at all : http://codereview.chromium.org/5918003/diff/1/webkit/glue/plugins/plugin_grou... http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.cc:355: for (std::vector<WebPluginMimeType>::const_iterator type_it = On 2010/12/20 20:56:59, John Abd-El-Malek wrote: > On 2010/12/20 19:57:37, pastarmovj wrote: > > On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > > > nit: size_t > > > > I would prefer to keep this one an iterator as well as the inner for or else > > we'll end up having web_plugin_infos_[i].mime_types[j].file_extensions[k] > which > > is anything but understandable. > > you can always create a local const reference, i.e. > const WebPluginMimeType& mime_type = web_plugin_infos_[i].mime_types[j] > > it's nice to be consistent in a file for iterating over the same container Done. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... File webkit/glue/plugins/plugin_list.h (right): http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list.h:201: // disabled by a policy and not a user. On 2010/12/20 20:56:59, John Abd-El-Malek wrote: > On 2010/12/20 19:57:37, pastarmovj wrote: > > On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > > > nit: using boolean to specify whether it's user/policy makes it hard to read > > at > > > the caller's site, and also limiting in the future in case we want to > disable > > > through other means (say because the plugin is out-of-date). please use an > > enum > > > instead > > > > I fixed the comment to make clear what the return flag means. It doesn't mean > > weather the plugin is policy enabled or disabled but only whether the change > > requested could be performed. > > oops, i meant to add this comment on line 209. so is this function only used to > disable plugins based on user action, and not on group policy? In any case these functions doesn't differentiate between policy or user call externally. If there is a policy set it has precedence (the groups store information about what policies exist) over the user action. Normally the UI should prevent the user from changing the status of a plugin or group if it is policy controlled. But in case the user tries to manipulate that it will still be overridden inside these functions. A policy change can trigger calling any of these function or a user interaction can do as well. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/webplug... File webkit/glue/plugins/webplugininfo.h (right): http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/webplug... webkit/glue/plugins/webplugininfo.h:30: struct WebPluginInfo { On 2010/12/20 20:56:59, John Abd-El-Malek wrote: > On 2010/12/20 19:57:37, pastarmovj wrote: > > On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > > > note that per the style guide, if this has methods, it should be a class > > instead > > > of a struct. this will mean we'd have to add getters/setters and make the > > > members private, and add underscores at the end. > > > > I don't think that we should do this (at least not in this CL) - it is going > to > > have huge impact on various files and make reviewing of this anyhow overgrown > CL > > harder. I'd suggest if this CL is accepted without much further refactoring to > > file this as a bug and have it fixed as soon as possible. The few methods are > > really only there to provide simple means of working with the fields and to > > prevent code duplication. I could extract them as functions in a namespace and > > provide them with a parameter being the object to modify but this is not going > > to make the code more readable. > > The issue is that the coding guidelines are pretty strict about structs only > carrying data, and classes having functions to modify it etc. We shouldn't give > up the consistency of the guidelines unless we have a very compelling reason, > and I don't think it exists here. If you want to make it a class in a future > change, then that change should make these functions be member functions. But > until that is done, then it seems that the functions have to stay in > PluginList/PluginGroup and take a WebPluginInfo to operate on. Would you agree on having these as global functions in a namespace or as a set of static functions in a helper class say WebPluginInfoUtils ? http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/webplug... webkit/glue/plugins/webplugininfo.h:92: int priority; On 2010/12/20 20:56:59, John Abd-El-Malek wrote: > I'm really against having this concept of order because I think it unnecessarily > complicates things. We didn't need this previously because the order of the > plugin_list denoted the priority, since we always searched from the beginning. > Since the plugin list is now generated from the plugin groups, it seems we > should just be able to keep plugin groups in a vector and we don't need an > ordering. > > An ordering is currently needed because the internal and external flash are in > the same group. It seems that if we don't group them, and instead treat them > separately like the internal PDF plugin and Adobe Reader are treated, then this > problem goes away. The order the array of plugins that existed before in PluginList is preserved in the fact that groups are created sequentially in the same order. As for the internal external flash differentiation I couldn't see any code making use of that. If I search for uses of the priority it doesn't appear anywhere so even before my CL it has been superseded apparently. I will move the flag to the PluginGroup class and if no other comments arise remove the priority from here too.
http://codereview.chromium.org/5699005/diff/66001/chrome/browser/plugin_updat... File chrome/browser/plugin_updater.h (right): http://codereview.chromium.org/5699005/diff/66001/chrome/browser/plugin_updat... chrome/browser/plugin_updater.h:37: const string16& name); On 2010/12/20 19:57:37, pastarmovj wrote: > On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > > can you add documentation to what "name" is? why is it needed now? > > Done. Umm...? http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... File webkit/glue/plugins/plugin_group.cc (right): http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.cc:91: enabled_ = false; On 2010/12/20 21:47:22, pastarmovj wrote: > On 2010/12/20 20:56:59, John Abd-El-Malek wrote: > > On 2010/12/20 19:57:37, pastarmovj wrote: > > > On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > > > > curious why this doesn't get copied from other? > > > > > > This whole function will be changed in CL 5783005 from Jakob anyhow. > > > > It's fine that this function will change in the future, but our tree should > > always be shippable and anyone can sync to a specific revision. If I sync > after > > you commit, this code will be confusing to me that it only copies some values > > but not others. > > Actually it is better to say that this function will change in a commit that > will precede mine: > > http://codereview.chromium.org/5918003/diff/1/webkit/glue/plugins/plugin_grou... So... rebase it on top of the other CL? Asking someone to review a change which is not going to land is really confusing. You can ping me if you need help with the rebasing. http://codereview.chromium.org/5699005/diff/88001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.cc:420: void PluginGroup::DisableOutdatedPlugins() { Nit: space after if. http://codereview.chromium.org/5699005/diff/88001/webkit/glue/plugins/plugin_... File webkit/glue/plugins/plugin_group.h (right): http://codereview.chromium.org/5699005/diff/88001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.h:100: // inn the group before. Nit: "in". http://codereview.chromium.org/5699005/diff/88001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.h:110: // by policy or are already in the enabled state. You could group these comments ("the two following functions enable or disable ..."). http://codereview.chromium.org/5699005/diff/88001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.h:150: // Check if the group has no plugins or only non-existing pluigns Nit: plugins http://codereview.chromium.org/5699005/diff/88001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.h:151: // (with priority 0). This priority 0 thing is definitely a step in the wrong direction. Like John said, if we want to have placeholder groups, we can store that state in the group itself. http://codereview.chromium.org/5699005/diff/88001/webkit/glue/plugins/plugin_... File webkit/glue/plugins/plugin_list.h (right): http://codereview.chromium.org/5699005/diff/88001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list.h:194: // for a plugin that is not yet loaded. Especially true when setting plugins "Especially true"? As in, even more true than is usual? ;-) http://codereview.chromium.org/5699005/diff/88001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list.h:255: const string16& name); Nit: align with previous parameter pls. http://codereview.chromium.org/5699005/diff/88001/webkit/glue/plugins/plugin_... File webkit/glue/plugins/plugin_list_mac.mm (right): http://codereview.chromium.org/5699005/diff/88001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list_mac.mm:101: if (plugins->at(i).path.BaseName() == info.path.BaseName()) { Is there a particular reason you're using at here? In most other places, we just use [i] (and if you're worried about index mismatches, use DCHECK). http://codereview.chromium.org/5699005/diff/88001/webkit/glue/plugins/webplug... File webkit/glue/plugins/webplugininfo.h (right): http://codereview.chromium.org/5699005/diff/88001/webkit/glue/plugins/webplug... webkit/glue/plugins/webplugininfo.h:57: // Returns true if the plugin supports |mime-type|. |mime_type| should be all Mini-nit: |mime_type| with an underscore. http://codereview.chromium.org/5699005/diff/88001/webkit/glue/plugins/webplug... webkit/glue/plugins/webplugininfo.h:63: // be set to the MIME type if found. The mime type which corresponds to the And please for the second instance as well :)
Trybots are happy (restarted try jobs to make them visible from the CL page). As the last change name says - cleaned up the WebPluginList form priority flag and rebased on the cleaned up PluginGroup implementation. As well as split the priority field from the is placeholder flag as suggested by Bernhard. http://codereview.chromium.org/5699005/diff/88001/webkit/glue/plugins/plugin_... File webkit/glue/plugins/plugin_group.cc (right): http://codereview.chromium.org/5699005/diff/88001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.cc:420: if(web_plugin_infos_[i].priority != 0) On 2010/12/20 22:30:28, Bernhard Bauer wrote: > Nit: space after if. Done. http://codereview.chromium.org/5699005/diff/88001/webkit/glue/plugins/plugin_... File webkit/glue/plugins/plugin_group.h (right): http://codereview.chromium.org/5699005/diff/88001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.h:100: // inn the group before. On 2010/12/20 22:30:28, Bernhard Bauer wrote: > Nit: "in". Done. http://codereview.chromium.org/5699005/diff/88001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.h:110: // by policy or are already in the enabled state. On 2010/12/20 22:30:28, Bernhard Bauer wrote: > You could group these comments ("the two following functions enable or disable > ..."). Done. http://codereview.chromium.org/5699005/diff/88001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.h:150: // Check if the group has no plugins or only non-existing pluigns On 2010/12/20 22:30:28, Bernhard Bauer wrote: > Nit: plugins Done. http://codereview.chromium.org/5699005/diff/88001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.h:151: // (with priority 0). On 2010/12/20 22:30:28, Bernhard Bauer wrote: > This priority 0 thing is definitely a step in the wrong direction. Like John > said, if we want to have placeholder groups, we can store that state in the > group itself. I moved the priority back into the group class and introduced a placeholder flag separately from it so that we don't mix two meanings in the same variable. http://codereview.chromium.org/5699005/diff/88001/webkit/glue/plugins/plugin_... File webkit/glue/plugins/plugin_list.h (right): http://codereview.chromium.org/5699005/diff/88001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list.h:194: // for a plugin that is not yet loaded. Especially true when setting plugins On 2010/12/20 22:30:28, Bernhard Bauer wrote: > "Especially true"? As in, even more true than is usual? ;-) It was really really true in this case :) Fixed. http://codereview.chromium.org/5699005/diff/88001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list.h:255: const string16& name); On 2010/12/20 22:30:28, Bernhard Bauer wrote: > Nit: align with previous parameter pls. Done. http://codereview.chromium.org/5699005/diff/88001/webkit/glue/plugins/plugin_... File webkit/glue/plugins/plugin_list_mac.mm (right): http://codereview.chromium.org/5699005/diff/88001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_list_mac.mm:101: if (plugins->at(i).path.BaseName() == info.path.BaseName()) { On 2010/12/20 22:30:28, Bernhard Bauer wrote: > Is there a particular reason you're using at here? In most other places, we just > use [i] (and if you're worried about index mismatches, use DCHECK). plugins is a pointer to a vector here so to use [] op I'll have to write (*plugins)[i]. I have seen examples in the code where pointers to arrays have been used with at instead of dereferencing and using []. Should I change it here? http://codereview.chromium.org/5699005/diff/88001/webkit/glue/plugins/webplug... File webkit/glue/plugins/webplugininfo.h (right): http://codereview.chromium.org/5699005/diff/88001/webkit/glue/plugins/webplug... webkit/glue/plugins/webplugininfo.h:57: // Returns true if the plugin supports |mime-type|. |mime_type| should be all On 2010/12/20 22:30:28, Bernhard Bauer wrote: > Mini-nit: |mime_type| with an underscore. Done. http://codereview.chromium.org/5699005/diff/88001/webkit/glue/plugins/webplug... webkit/glue/plugins/webplugininfo.h:63: // be set to the MIME type if found. The mime type which corresponds to the On 2010/12/20 22:30:28, Bernhard Bauer wrote: > And please for the second instance as well :) Done.
http://codereview.chromium.org/5699005/diff/103001/chrome/browser/plugin_upda... File chrome/browser/plugin_updater.h (right): http://codereview.chromium.org/5699005/diff/103001/chrome/browser/plugin_upda... chrome/browser/plugin_updater.h:39: // setting the state of a plugin that has not been loaded yet (as is the case) Nit: extend parenthesis to include "with policy controlled plugins"; doesn't make much sense otherwise. http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_group.h (right): http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_group.h:151: // (with priority 0). Comment is outdated. http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_group.h:157: // Set plugin placeholder status Comment is wrong. Also, it doesn't really tell you anything you wouldn't know from the method name (if it was correct, that is). http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_group.h:235: std::vector<int> web_plugin_priority_; Nit: You should call it priorities. It's confusing otherwise. Also, we probably could get fully rid of them, but I'd say we should do that in a separate CL when we have proper test coverage in place so that we know we're not breaking anything. That okay with you, John? http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_group.h:239: std::vector<bool> is_plugin_placeholder_; I'm still not exactly sure what the invariants for this are. Is there ever going to be more than one placeholder? And what's the difference between a group without plugins and one with a placeholder? http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_list.cc (right): http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list.cc:364: // Clean up the plugins that has disappeared from the groups. Nit: have http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list.cc:367: std::vector<WebPluginInfo>& gr_plugins = group->second->GetPlugins(); Can you use a const reference here? If not, that is a sign that you're violating encapsulation; you're basically directly mucking around with a member variable of another object. http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/webpl... File webkit/plugins/npapi/webplugininfo.h (right): http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/webpl... webkit/plugins/npapi/webplugininfo.h:69: class WebPluginInfoUtils { Maybe move this to a separate class?
http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/webplug... File webkit/glue/plugins/webplugininfo.h (right): http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/webplug... webkit/glue/plugins/webplugininfo.h:30: struct WebPluginInfo { On 2010/12/20 21:47:22, pastarmovj wrote: > On 2010/12/20 20:56:59, John Abd-El-Malek wrote: > > On 2010/12/20 19:57:37, pastarmovj wrote: > > > On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > > > > note that per the style guide, if this has methods, it should be a class > > > instead > > > > of a struct. this will mean we'd have to add getters/setters and make the > > > > members private, and add underscores at the end. > > > > > > I don't think that we should do this (at least not in this CL) - it is going > > to > > > have huge impact on various files and make reviewing of this anyhow > overgrown > > CL > > > harder. I'd suggest if this CL is accepted without much further refactoring > to > > > file this as a bug and have it fixed as soon as possible. The few methods > are > > > really only there to provide simple means of working with the fields and to > > > prevent code duplication. I could extract them as functions in a namespace > and > > > provide them with a parameter being the object to modify but this is not > going > > > to make the code more readable. > > > > The issue is that the coding guidelines are pretty strict about structs only > > carrying data, and classes having functions to modify it etc. We shouldn't > give > > up the consistency of the guidelines unless we have a very compelling reason, > > and I don't think it exists here. If you want to make it a class in a future > > change, then that change should make these functions be member functions. But > > until that is done, then it seems that the functions have to stay in > > PluginList/PluginGroup and take a WebPluginInfo to operate on. > > Would you agree on having these as global functions in a namespace or as a set > of static functions in a helper class say WebPluginInfoUtils ? Is the only place that uses them in PluginGroup? If so then please keep the functions there. We have to many classes for plugins as is, so avoiding adding a new one is good. http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/webplug... webkit/glue/plugins/webplugininfo.h:92: int priority; On 2010/12/20 21:47:22, pastarmovj wrote: > On 2010/12/20 20:56:59, John Abd-El-Malek wrote: > > I'm really against having this concept of order because I think it > unnecessarily > > complicates things. We didn't need this previously because the order of the > > plugin_list denoted the priority, since we always searched from the beginning. > > > Since the plugin list is now generated from the plugin groups, it seems we > > should just be able to keep plugin groups in a vector and we don't need an > > ordering. > > > > An ordering is currently needed because the internal and external flash are in > > the same group. It seems that if we don't group them, and instead treat them > > separately like the internal PDF plugin and Adobe Reader are treated, then > this > > problem goes away. > > The order the array of plugins that existed before in PluginList is preserved in > the fact that groups are created sequentially in the same order. As for the > internal external flash differentiation I couldn't see any code making use of > that. If I search for uses of the priority it doesn't appear anywhere so even > before my CL it has been superseded apparently. > > I will move the flag to the PluginGroup class and if no other comments arise > remove the priority from here too. why is this needed in PluginGroup? I really don't see the need. If it's used to put our internal flash before the external one, then PluginGroup should be doing that with a one-off check when it creates the group http://codereview.chromium.org/5699005/diff/88001/webkit/glue/plugins/plugin_... File webkit/glue/plugins/plugin_group.h (right): http://codereview.chromium.org/5699005/diff/88001/webkit/glue/plugins/plugin_... webkit/glue/plugins/plugin_group.h:151: // (with priority 0). On 2010/12/21 16:53:02, pastarmovj wrote: > On 2010/12/20 22:30:28, Bernhard Bauer wrote: > > This priority 0 thing is definitely a step in the wrong direction. Like John > > said, if we want to have placeholder groups, we can store that state in the > > group itself. > > I moved the priority back into the group class and introduced a placeholder flag > separately from it so that we don't mix two meanings in the same variable. I still don't think we need a priority at all. Can you please explain why it's needed? The only example I've heard so far is because both internal and external flash are in the same group. There are two solutions to that: either put them in separate groups, and have the internal one first, or add the internal one before the external one in a group. that would reduce the need to have priority, which seems redundant since the same data can (and used to be) taken from the ordering of the plugin list. http://codereview.chromium.org/5699005/diff/103001/chrome/browser/chromeos/gv... File chrome/browser/chromeos/gview_request_interceptor_unittest.cc (right): http://codereview.chromium.org/5699005/diff/103001/chrome/browser/chromeos/gv... chrome/browser/chromeos/gview_request_interceptor_unittest.cc:71: info.product_name = UTF8ToWide("PDF Internal Plugin"); why is this needed here? http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_group.h (right): http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_group.h:130: std::vector<WebPluginInfo>& GetPlugins() { return web_plugin_infos_; } we shouldn't be returning a non-const reference to the internal data structure. http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_group.h:235: std::vector<int> web_plugin_priority_; i think we can do without this. http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_list.cc (right): http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list.cc:679: WebPluginInfo plugin_info; if I understand correctly, AddPlaceholderToPluginGroups is used to allow disabling a plugin based on its filename, before we have the list of groups ready, right? If that's the case, do we really need to call AddToPluginGroups? why can't we just keep a vector of disabled FilePaths, and check those when creating the real plugin groups? it seems that this would simplify things a little since we wouldn't need |name| or empty groups? http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/webpl... File webkit/plugins/npapi/webplugininfo.h (right): http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/webpl... webkit/plugins/npapi/webplugininfo.h:34: enum Reason { USER = 1 << 1, MANAGED = 1 << 2 }; nit: we usually put each enum in a separate line http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/webpl... webkit/plugins/npapi/webplugininfo.h:66: int reason; we don't need both an enabled boolean and a reason int. these can be combined into: int disabled_reason; enum DisabledReason { USER = 1 << 1, MANAGED = 1 << 2, } and then if disabled_reason is 0, we know it's enabled. http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/webpl... webkit/plugins/npapi/webplugininfo.h:69: class WebPluginInfoUtils { we already have too many plugin related classes, I really prefer we don't add more. especially in this case, a class isn't really needed since there are no member variables. what you need is just namespace. since this file is already in webkit::npapi namespace, you can leave the functions in this header outside of the class. even better though, I'm still not sure these classes need to be grouped here? SupportsExtension and SupportsType are only used in PluginList, so they can remain in that file. IsEnabled is just checking one member variable, so it can be done directly in the different callers. Enable and Disable are only used in PluginGroup, so they can remain there. These functions used to be in different files, so I think we should either leave them there if WebPluginInfo remains a struct, or they should only be moved to one place (inside WebPluginInfo) if it's a class. but moving them here, outside WebPluginInfo, even though each function is only needed in one place, makes for harder readability. http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/webpl... webkit/plugins/npapi/webplugininfo.h:85: const std::string& mime_type, nit: spacing is off http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/webpl... webkit/plugins/npapi/webplugininfo.h:93: const std::string& extension, nit: spacing
Final call for comments before XMas :). As I am leaving in two days for a week this is the last chance to bring this CL to a committable state before that! Please have a look at the comments I made and possibly aggree or disagree with them until tomorrow so that I can fix them and submit this for a final review before I either freeze the commit for ~10 days (...or forever if that be the choice - though the bug fixes present here are actually real problems and if solved by only patching the code will make it nastier than ever) or commit it. I checked the valgrind output too and I haven't seen anything new there - the only plugins connected problem is the one from the singleton object - which might need to be suspended as already discuessed by a previous commit touching this code. http://codereview.chromium.org/5699005/diff/103001/chrome/browser/chromeos/gv... File chrome/browser/chromeos/gview_request_interceptor_unittest.cc (right): http://codereview.chromium.org/5699005/diff/103001/chrome/browser/chromeos/gv... chrome/browser/chromeos/gview_request_interceptor_unittest.cc:71: info.product_name = UTF8ToWide("PDF Internal Plugin"); On 2010/12/21 19:57:42, John Abd-El-Malek wrote: > why is this needed here? I use the existence of a description as a marker for proper plugin and not a placeholder when adding them to the list. This is the only field that is really supported on all platforms in a clean manner (other than the filename which is not enough). And the name is needed so that the plugin won't land in a group called "" because group names are generated from plugin names. http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_group.h (right): http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_group.h:130: std::vector<WebPluginInfo>& GetPlugins() { return web_plugin_infos_; } On 2010/12/21 19:57:42, John Abd-El-Malek wrote: > we shouldn't be returning a non-const reference to the internal data structure. I probably don't need this version anymore. I will remove it if this is the case or replace it with a proper manipulator function for the aim I have. http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_group.h:239: std::vector<bool> is_plugin_placeholder_; On 2010/12/21 19:07:39, Bernhard Bauer wrote: > I'm still not exactly sure what the invariants for this are. Is there ever going > to be more than one placeholder? And what's the difference between a group > without plugins and one with a placeholder? In a group one can have more than one placeholder, but only one per distinctive plugin (different file). From the perspective of an user there is no difference between an empty group and one with only placeholders - both appear empty and are not returned from the PluginList::GetPluginGroups call. The concept is not much different than the disabledgroups/plugins lists we had before but implemented clean in terms only of existing obejcts and reducing the chance for these getting out of sync (as it happened before - plugins/groups couldn't get properly disabled/enabled separately or didn't update the groups status correctly). http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_list.cc (right): http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list.cc:367: std::vector<WebPluginInfo>& gr_plugins = group->second->GetPlugins(); On 2010/12/21 19:07:39, Bernhard Bauer wrote: > Can you use a const reference here? If not, that is a sign that you're violating > encapsulation; you're basically directly mucking around with a member variable > of another object. I can and will do it no problem. I only read from the plugins. http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list.cc:679: WebPluginInfo plugin_info; On 2010/12/21 19:57:42, John Abd-El-Malek wrote: > if I understand correctly, AddPlaceholderToPluginGroups is used to allow > disabling a plugin based on its filename, before we have the list of groups > ready, right? If that's the case, do we really need to call AddToPluginGroups? > why can't we just keep a vector of disabled FilePaths, and check those when > creating the real plugin groups? it seems that this would simplify things a > little since we wouldn't need |name| or empty groups? It is not so simple to keep this list of disabled plugins in sync every time they change. which happens from the UI/policy changes/(and soon file appearing and disappering). The code used to do that before and it failed because the connections were not that simple at all - a lot of possible cause-effects can occur that were too difficult to enumerate and change all too often. The implementation now hides this complexity at much lower price than the code needed before to keep such an array for plugins _and_ groups. http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/webpl... File webkit/plugins/npapi/webplugininfo.h (right): http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/webpl... webkit/plugins/npapi/webplugininfo.h:66: int reason; On 2010/12/21 19:57:42, John Abd-El-Malek wrote: > we don't need both an enabled boolean and a reason int. these can be combined > into: > > int disabled_reason; > > enum DisabledReason { > USER = 1 << 1, > MANAGED = 1 << 2, > } > > and then if disabled_reason is 0, we know it's enabled. Indeed we do. The reason why this enum is called "Reason" and not "DisabledReason" is that there is a reason for both being enabled and being disabled. For now we only have disabled by policy but this soon will extend to being enabled by policy - basically forcing a plugin being on regardless of user wish. In that case you will have enabled == true and reason == MANAGED. reason == 0 have no meaning and thus is not defined as a constant at all. Per default this fiend initializes to USER which was the state before - all plugins not explicitly managed are user controlled. http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/webpl... webkit/plugins/npapi/webplugininfo.h:69: class WebPluginInfoUtils { On 2010/12/21 19:07:39, Bernhard Bauer wrote: > Maybe move this to a separate class? Separate file you mean? http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/webpl... webkit/plugins/npapi/webplugininfo.h:69: class WebPluginInfoUtils { On 2010/12/21 19:57:42, John Abd-El-Malek wrote: > we already have too many plugin related classes, I really prefer we don't add > more. especially in this case, a class isn't really needed since there are no > member variables. what you need is just namespace. since this file is already > in webkit::npapi namespace, you can leave the functions in this header outside > of the class. > > even better though, I'm still not sure these classes need to be grouped here? > SupportsExtension and SupportsType are only used in PluginList, so they can > remain in that file. IsEnabled is just checking one member variable, so it can > be done directly in the different callers. Enable and Disable are only used in > PluginGroup, so they can remain there. > > These functions used to be in different files, so I think we should either leave > them there if WebPluginInfo remains a struct, or they should only be moved to > one place (inside WebPluginInfo) if it's a class. but moving them here, outside > WebPluginInfo, even though each function is only needed in one place, makes for > harder readability. Alright, I will move (possibly duplicate) any of these functions to where I need them and remove the whole class.
There are still a couple of unadressed nits. Also, please add unit tests at least for the bug this CL is supposed to fix, and the first two test cases in my previous comment, to make sure we don't regress there: * Enabling/disabling plugins before the list of plugins is loaded should have the same effect as enabling/disabling them afterwards * Updating a plugin (i.e. replacing a plugin with one with a different FilePath but which is still in the same PluginGroup) should keep the enabled/disabled state http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/webpl... File webkit/plugins/npapi/webplugininfo.h (right): http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/webpl... webkit/plugins/npapi/webplugininfo.h:69: class WebPluginInfoUtils { On 2010/12/21 20:31:19, pastarmovj wrote: > On 2010/12/21 19:07:39, Bernhard Bauer wrote: > > Maybe move this to a separate class? > > Separate file you mean? Uh, yes, but like John said, a namespace would probably be better. I just don't like having a class defined in a file which doesn't correspond to its name; it makes finding the definition for a class kinda hard.
@Bernhard : The whole suit of unit tests for PluginList that were in Jakob's commit 5783005 have been adopted in this CL. Most of the stuff is tested there. I will see tomorrow which of your proposed cases are not in there and can be added easily. I will try to simulate the policy re-enable situation as well. @John : Please, if you got a few minutes, have a look at my answers on your questions in my previous reply, so that I can get these done tomorrow.
On Tue, Dec 21, 2010 at 13:58, <pastarmovj@chromium.org> wrote: > @Bernhard : The whole suit of unit tests for PluginList that were in Jakob's > commit 5783005 have been adopted in this CL. Most of the stuff is tested > there. > I will see tomorrow which of your proposed cases are not in there and can be > added easily. I will try to simulate the policy re-enable situation as well. Please rebase your CL on 5783005 then (if it will likely land first), so that it's clear what's in what CL. > @John : Please, if you got a few minutes, have a look at my answers on your > questions in my previous reply, so that I can get these done tomorrow. > > http://codereview.chromium.org/5699005/ >
On Tue, Dec 21, 2010 at 12:31 PM, <pastarmovj@chromium.org> wrote: > Final call for comments before XMas :). > As I am leaving in two days for a week this is the last chance to bring > this CL > to a committable state before that! Please have a look at the comments I > made > and possibly aggree or disagree with them until tomorrow so that I can fix > them > and submit this for a final review before I either freeze the commit for > ~10 > days (...or forever if that be the choice - though the bug fixes present > here > are actually real problems and if solved by only patching the code will > make it > nastier than ever) or commit it. > Note that I think this refactoring is really great, and I'm happy that it's being done. I don't think anyone else will be touching this code for the next few weeks. You're very close, just a few more improvements I'd like to see here while cleaning this up. > I checked the valgrind output too and I haven't seen anything new there - > the > only plugins connected problem is the one from the singleton object - which > might need to be suspended as already discuessed by a previous commit > touching > this code. > > > > > http://codereview.chromium.org/5699005/diff/103001/chrome/browser/chromeos/gv... > File chrome/browser/chromeos/gview_request_interceptor_unittest.cc > (right): > > > http://codereview.chromium.org/5699005/diff/103001/chrome/browser/chromeos/gv... > chrome/browser/chromeos/gview_request_interceptor_unittest.cc:71: > info.product_name = UTF8ToWide("PDF Internal Plugin"); > On 2010/12/21 19:57:42, John Abd-El-Malek wrote: > >> why is this needed here? >> > > I use the existence of a description as a marker for proper plugin and > not a placeholder when adding them to the list. This seems another reason to store placeholders separately. Knowing that a plugin without description is a placeholder is sort of magic, and it is possible for plugins to not have a description. > This is the only field > that is really supported on all platforms in a clean manner (other than > the filename which is not enough). And the name is needed so that the > plugin won't land in a group called "" because group names are generated > from plugin names. > > > > http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... > File webkit/plugins/npapi/plugin_group.h (right): > > > http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... > webkit/plugins/npapi/plugin_group.h:130: std::vector<WebPluginInfo>& > GetPlugins() { return web_plugin_infos_; } > On 2010/12/21 19:57:42, John Abd-El-Malek wrote: > >> we shouldn't be returning a non-const reference to the internal data >> > structure. > > I probably don't need this version anymore. I will remove it if this is > the case or replace it with a proper manipulator function for the aim I > have. > > > > http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... > webkit/plugins/npapi/plugin_group.h:239: std::vector<bool> > is_plugin_placeholder_; > On 2010/12/21 19:07:39, Bernhard Bauer wrote: > >> I'm still not exactly sure what the invariants for this are. Is there >> > ever going > >> to be more than one placeholder? And what's the difference between a >> > group > >> without plugins and one with a placeholder? >> > > In a group one can have more than one placeholder, but only one per > distinctive plugin (different file). From the perspective of an user > there is no difference between an empty group and one with only > placeholders - both appear empty and are not returned from the > PluginList::GetPluginGroups call. The concept is not much different than > the disabledgroups/plugins lists we had before but implemented clean in > terms only of existing obejcts and reducing the chance for these getting > out of sync (as it happened before - plugins/groups couldn't get > properly disabled/enabled separately or didn't update the groups status > correctly). > > > http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... > File webkit/plugins/npapi/plugin_list.cc (right): > > > http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... > webkit/plugins/npapi/plugin_list.cc:367: std::vector<WebPluginInfo>& > gr_plugins = group->second->GetPlugins(); > On 2010/12/21 19:07:39, Bernhard Bauer wrote: > >> Can you use a const reference here? If not, that is a sign that you're >> > violating > >> encapsulation; you're basically directly mucking around with a member >> > variable > >> of another object. >> > > I can and will do it no problem. I only read from the plugins. > > > > http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... > webkit/plugins/npapi/plugin_list.cc:679: WebPluginInfo plugin_info; > On 2010/12/21 19:57:42, John Abd-El-Malek wrote: > >> if I understand correctly, AddPlaceholderToPluginGroups is used to >> > allow > >> disabling a plugin based on its filename, before we have the list of >> > groups > >> ready, right? If that's the case, do we really need to call >> > AddToPluginGroups? > >> why can't we just keep a vector of disabled FilePaths, and check those >> > when > >> creating the real plugin groups? it seems that this would simplify >> > things a > >> little since we wouldn't need |name| or empty groups? >> > > It is not so simple to keep this list of disabled plugins in sync every > time they change. which happens from the UI/policy changes/(and soon > file appearing and disappering). The code used to do that before and it > failed because the connections were not that simple at all - a lot of > possible cause-effects can occur that were too difficult to enumerate > and change all too often. The implementation now hides this complexity > at much lower price than the code needed before to keep such an array > for plugins _and_ groups. I totally agree that we only want one list of plugins. I'm not really following your reasons though. Can you confirm my guess as to why we need this? i.e. because we want to disable a plugin by its filename before we have loaded the list of plugins? If so, what I'm suggesting is that we just store this filename in a vector of FilePaths, instead of creating these quasi-fake PluginGroups. This would be temporarily stored until we build the groups. > > > > http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/webpl... > File webkit/plugins/npapi/webplugininfo.h (right): > > > http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/webpl... > webkit/plugins/npapi/webplugininfo.h:66: int reason; > On 2010/12/21 19:57:42, John Abd-El-Malek wrote: > >> we don't need both an enabled boolean and a reason int. these can be >> > combined > >> into: >> > > int disabled_reason; >> > > enum DisabledReason { >> USER = 1 << 1, >> MANAGED = 1 << 2, >> } >> > > and then if disabled_reason is 0, we know it's enabled. >> > > Indeed we do. The reason why this enum is called "Reason" and not > "DisabledReason" is that there is a reason for both being enabled and > being disabled. For now we only have disabled by policy but this soon > will extend to being enabled by policy - basically forcing a plugin > being on regardless of user wish. This is a little odd. What sort of scenario would we want an enterprise to force enable a plugin and not allow the user to disable it? Either way, we can still do this with enums and only keep one member variable for this. i.e. ENABLED_BY_POLICY = 1<< 1, DISABLED_BY_POLICY = 1<< 2, ENABLED_BY_USER = 1 << 3, DISABLED_BY_USER = 1 << 4, In that case you will have enabled == > true and reason == MANAGED. reason == 0 have no meaning and thus is not > defined as a constant at all. Per default this fiend initializes to USER > which was the state before - all plugins not explicitly managed are user > controlled. > > > > http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/webpl... > webkit/plugins/npapi/webplugininfo.h:69: class WebPluginInfoUtils { > On 2010/12/21 19:07:39, Bernhard Bauer wrote: > >> Maybe move this to a separate class? >> > > Separate file you mean? > > > > http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/webpl... > webkit/plugins/npapi/webplugininfo.h:69: class WebPluginInfoUtils { > On 2010/12/21 19:57:42, John Abd-El-Malek wrote: > >> we already have too many plugin related classes, I really prefer we >> > don't add > >> more. especially in this case, a class isn't really needed since >> > there are no > >> member variables. what you need is just namespace. since this file >> > is already > >> in webkit::npapi namespace, you can leave the functions in this header >> > outside > >> of the class. >> > > even better though, I'm still not sure these classes need to be >> > grouped here? > >> SupportsExtension and SupportsType are only used in PluginList, so >> > they can > >> remain in that file. IsEnabled is just checking one member variable, >> > so it can > >> be done directly in the different callers. Enable and Disable are >> > only used in > >> PluginGroup, so they can remain there. >> > > These functions used to be in different files, so I think we should >> > either leave > >> them there if WebPluginInfo remains a struct, or they should only be >> > moved to > >> one place (inside WebPluginInfo) if it's a class. but moving them >> > here, outside > >> WebPluginInfo, even though each function is only needed in one place, >> > makes for > >> harder readability. >> > > Alright, I will move (possibly duplicate) any of these functions to > where I need them and remove the whole class. Why duplicate? The only thing I saw that was used in more than one file was the IsEnabled, which seems like the struct's member variable should be checked directly. I don't see any functions that should be duplicated? > > > http://codereview.chromium.org/5699005/ >
also, I still believe we really should get rid of priority in this change, it shouldn't have gone in the first place and duplicates the order of plugins in plugingroup, which is what we always used as priority On Tue, Dec 21, 2010 at 2:38 PM, John Abd-El-Malek <jam@chromium.org> wrote: > > > On Tue, Dec 21, 2010 at 12:31 PM, <pastarmovj@chromium.org> wrote: > >> Final call for comments before XMas :). >> As I am leaving in two days for a week this is the last chance to bring >> this CL >> to a committable state before that! Please have a look at the comments I >> made >> and possibly aggree or disagree with them until tomorrow so that I can fix >> them >> and submit this for a final review before I either freeze the commit for >> ~10 >> days (...or forever if that be the choice - though the bug fixes present >> here >> are actually real problems and if solved by only patching the code will >> make it >> nastier than ever) or commit it. >> > > Note that I think this refactoring is really great, and I'm happy that it's > being done. I don't think anyone else will be touching this code for the > next few weeks. You're very close, just a few more improvements I'd like to > see here while cleaning this up. > > >> I checked the valgrind output too and I haven't seen anything new there - >> the >> only plugins connected problem is the one from the singleton object - >> which >> might need to be suspended as already discuessed by a previous commit >> touching >> this code. >> >> >> >> >> http://codereview.chromium.org/5699005/diff/103001/chrome/browser/chromeos/gv... >> File chrome/browser/chromeos/gview_request_interceptor_unittest.cc >> (right): >> >> >> http://codereview.chromium.org/5699005/diff/103001/chrome/browser/chromeos/gv... >> chrome/browser/chromeos/gview_request_interceptor_unittest.cc:71: >> info.product_name = UTF8ToWide("PDF Internal Plugin"); >> On 2010/12/21 19:57:42, John Abd-El-Malek wrote: >> >>> why is this needed here? >>> >> >> I use the existence of a description as a marker for proper plugin and >> not a placeholder when adding them to the list. > > > This seems another reason to store placeholders separately. Knowing that a > plugin without description is a placeholder is sort of magic, and it is > possible for plugins to not have a description. > > > >> This is the only field >> that is really supported on all platforms in a clean manner (other than >> the filename which is not enough). And the name is needed so that the >> plugin won't land in a group called "" because group names are generated >> from plugin names. >> >> >> >> http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... >> File webkit/plugins/npapi/plugin_group.h (right): >> >> >> http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... >> webkit/plugins/npapi/plugin_group.h:130: std::vector<WebPluginInfo>& >> GetPlugins() { return web_plugin_infos_; } >> On 2010/12/21 19:57:42, John Abd-El-Malek wrote: >> >>> we shouldn't be returning a non-const reference to the internal data >>> >> structure. >> >> I probably don't need this version anymore. I will remove it if this is >> the case or replace it with a proper manipulator function for the aim I >> have. >> >> >> >> http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... >> webkit/plugins/npapi/plugin_group.h:239: std::vector<bool> >> is_plugin_placeholder_; >> On 2010/12/21 19:07:39, Bernhard Bauer wrote: >> >>> I'm still not exactly sure what the invariants for this are. Is there >>> >> ever going >> >>> to be more than one placeholder? And what's the difference between a >>> >> group >> >>> without plugins and one with a placeholder? >>> >> >> In a group one can have more than one placeholder, but only one per >> distinctive plugin (different file). From the perspective of an user >> there is no difference between an empty group and one with only >> placeholders - both appear empty and are not returned from the >> PluginList::GetPluginGroups call. The concept is not much different than >> the disabledgroups/plugins lists we had before but implemented clean in >> terms only of existing obejcts and reducing the chance for these getting >> out of sync (as it happened before - plugins/groups couldn't get >> properly disabled/enabled separately or didn't update the groups status >> correctly). > > >> >> >> http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... >> File webkit/plugins/npapi/plugin_list.cc (right): >> >> >> http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... >> webkit/plugins/npapi/plugin_list.cc:367: std::vector<WebPluginInfo>& >> gr_plugins = group->second->GetPlugins(); >> On 2010/12/21 19:07:39, Bernhard Bauer wrote: >> >>> Can you use a const reference here? If not, that is a sign that you're >>> >> violating >> >>> encapsulation; you're basically directly mucking around with a member >>> >> variable >> >>> of another object. >>> >> >> I can and will do it no problem. I only read from the plugins. >> >> >> >> http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... >> webkit/plugins/npapi/plugin_list.cc:679: WebPluginInfo plugin_info; >> On 2010/12/21 19:57:42, John Abd-El-Malek wrote: >> >>> if I understand correctly, AddPlaceholderToPluginGroups is used to >>> >> allow >> >>> disabling a plugin based on its filename, before we have the list of >>> >> groups >> >>> ready, right? If that's the case, do we really need to call >>> >> AddToPluginGroups? >> >>> why can't we just keep a vector of disabled FilePaths, and check those >>> >> when >> >>> creating the real plugin groups? it seems that this would simplify >>> >> things a >> >>> little since we wouldn't need |name| or empty groups? >>> >> >> It is not so simple to keep this list of disabled plugins in sync every >> time they change. which happens from the UI/policy changes/(and soon >> file appearing and disappering). The code used to do that before and it >> failed because the connections were not that simple at all - a lot of >> possible cause-effects can occur that were too difficult to enumerate >> and change all too often. The implementation now hides this complexity >> at much lower price than the code needed before to keep such an array >> for plugins _and_ groups. > > > I totally agree that we only want one list of plugins. > > I'm not really following your reasons though. Can you confirm my guess as > to why we need this? i.e. because we want to disable a plugin by its > filename before we have loaded the list of plugins? If so, what I'm > suggesting is that we just store this filename in a vector of FilePaths, > instead of creating these quasi-fake PluginGroups. This would be > temporarily stored until we build the groups. > > >> >> >> >> http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/webpl... >> File webkit/plugins/npapi/webplugininfo.h (right): >> >> >> http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/webpl... >> webkit/plugins/npapi/webplugininfo.h:66: int reason; >> On 2010/12/21 19:57:42, John Abd-El-Malek wrote: >> >>> we don't need both an enabled boolean and a reason int. these can be >>> >> combined >> >>> into: >>> >> >> int disabled_reason; >>> >> >> enum DisabledReason { >>> USER = 1 << 1, >>> MANAGED = 1 << 2, >>> } >>> >> >> and then if disabled_reason is 0, we know it's enabled. >>> >> >> Indeed we do. The reason why this enum is called "Reason" and not >> "DisabledReason" is that there is a reason for both being enabled and >> being disabled. For now we only have disabled by policy but this soon >> will extend to being enabled by policy - basically forcing a plugin >> being on regardless of user wish. > > > This is a little odd. What sort of scenario would we want an enterprise to > force enable a plugin and not allow the user to disable it? > > Either way, we can still do this with enums and only keep one member > variable for this. i.e. > > ENABLED_BY_POLICY = 1<< 1, > DISABLED_BY_POLICY = 1<< 2, > ENABLED_BY_USER = 1 << 3, > DISABLED_BY_USER = 1 << 4, > > In that case you will have enabled == >> true and reason == MANAGED. reason == 0 have no meaning and thus is not >> defined as a constant at all. Per default this fiend initializes to USER >> which was the state before - all plugins not explicitly managed are user >> controlled. >> >> >> >> http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/webpl... >> webkit/plugins/npapi/webplugininfo.h:69: class WebPluginInfoUtils { >> On 2010/12/21 19:07:39, Bernhard Bauer wrote: >> >>> Maybe move this to a separate class? >>> >> >> Separate file you mean? >> >> >> >> http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/webpl... >> webkit/plugins/npapi/webplugininfo.h:69: class WebPluginInfoUtils { >> On 2010/12/21 19:57:42, John Abd-El-Malek wrote: >> >>> we already have too many plugin related classes, I really prefer we >>> >> don't add >> >>> more. especially in this case, a class isn't really needed since >>> >> there are no >> >>> member variables. what you need is just namespace. since this file >>> >> is already >> >>> in webkit::npapi namespace, you can leave the functions in this header >>> >> outside >> >>> of the class. >>> >> >> even better though, I'm still not sure these classes need to be >>> >> grouped here? >> >>> SupportsExtension and SupportsType are only used in PluginList, so >>> >> they can >> >>> remain in that file. IsEnabled is just checking one member variable, >>> >> so it can >> >>> be done directly in the different callers. Enable and Disable are >>> >> only used in >> >>> PluginGroup, so they can remain there. >>> >> >> These functions used to be in different files, so I think we should >>> >> either leave >> >>> them there if WebPluginInfo remains a struct, or they should only be >>> >> moved to >> >>> one place (inside WebPluginInfo) if it's a class. but moving them >>> >> here, outside >> >>> WebPluginInfo, even though each function is only needed in one place, >>> >> makes for >> >>> harder readability. >>> >> >> Alright, I will move (possibly duplicate) any of these functions to >> where I need them and remove the whole class. > > > Why duplicate? The only thing I saw that was used in more than one file > was the IsEnabled, which seems like the struct's member variable should be > checked directly. I don't see any functions that should be duplicated? > >> >> >> http://codereview.chromium.org/5699005/ >> > >
Some more comments here as answers of some earlier general design questions. I kept enough kontext in here to make sure it is understandable which topics are addressed in my answers. >> info.product_name = UTF8ToWide("PDF Internal Plugin"); >> >>> why is this needed here? >>> >> >> I use the existence of a description as a marker for proper plugin and >> not a placeholder when adding them to the list. > > >This seems another reason to store placeholders separately. Knowing that a >plugin without description is a placeholder is sort of magic, and it is >possible for plugins to not have a description. > > The problem with this list of placeholders/disabled/missing plugins is it's maintability. It will end up like it used to be with the disabled plugins list we had before. There are many functions that actually update this list - LoadPlugin, EnablePlugin/DisablePlugin, EnableGroup. While we were tuning the code to try to maintain these lists correctly it turned out to be rather complicated task and certainly not easy to follow. The placeholders on the other hand are in place mapping for these disabled plugins. They have only one place where they should be threated with care and this is PluginGroup::AddPlugin where we should see if we are upgrading existing placeholder to complete plugin description. The PluginList clients never see these as they are filtered from the output too. I am willing to write even more comments about this in the code if needed but I see it as a really clean solution to the problem. As of whether plugins have description or not - I extended the code to make sure plugins will have a description always - if there is no real one the base filename without extenstion is adopted for description. Before it was similar but less obvious - it used to take the full path of a plugin as groups name which didn't allow to group simmilar plugins with different versions in proper groups. >> It is not so simple to keep this list of disabled plugins in sync every >> time they change. which happens from the UI/policy changes/(and soon >> file appearing and disappering). The code used to do that before and it >> failed because the connections were not that simple at all - a lot of >> possible cause-effects can occur that were too difficult to enumerate >> and change all too often. The implementation now hides this complexity >> at much lower price than the code needed before to keep such an array >> for plugins _and_ groups. > > >I totally agree that we only want one list of plugins. > >I'm not really following your reasons though. Can you confirm my guess as >to why we need this? i.e. because we want to disable a plugin by its >filename before we have loaded the list of plugins? If so, what I'm >suggesting is that we just store this filename in a vector of FilePaths, >instead of creating these quasi-fake PluginGroups. This would be >temporarily stored until we build the groups. > > I mostly covered this in the previous section but one more note here : We should not only create it temporarily but also recreate it every time we reload the plugins. And we have to update it every time a request for Enable/Disable comes - which might be both for an existing or non-existing plugin. This is going to make the code of these functions really convoluted IMO. >> >> Indeed we do. The reason why this enum is called "Reason" and not >> "DisabledReason" is that there is a reason for both being enabled and >> being disabled. For now we only have disabled by policy but this soon >> will extend to being enabled by policy - basically forcing a plugin >> being on regardless of user wish. > > >This is a little odd. What sort of scenario would we want an enterprise to >force enable a plugin and not allow the user to disable it? > Recent talk of Linus and Adobe resulted in this proposal : "- We should make sure that businesses can construct policies which control which PDF reader is used in Chrome. Most of the files with special features that we don't understand are inside companies." Also described in this bug : http://code.google.com/p/chromium/issues/detail?id=67739 >Either way, we can still do this with enums and only keep one member >variable for this. i.e. > >ENABLED_BY_POLICY = 1<< 1, >DISABLED_BY_POLICY = 1<< 2, >ENABLED_BY_USER = 1 << 3, >DISABLED_BY_USER = 1 << 4, > Other than saving a few bytes per plugin I don't see this making the logic any more easier. This will be ok if we encapsulate the logic then in a few functions like IsEnabled/isManaged/enable/disable etc. I will propose to make this in another CL which will completely capsulate WebPluginInfo in a class with the right OOP concepts inplace. Maybe do this when the question with PDF policy described above gets scheduled for solving. > >Why duplicate? The only thing I saw that was used in more than one file was >the IsEnabled, which seems like the struct's member variable should be >checked directly. I don't see any functions that should be duplicated? > I see the problem with braking basic OOP principles by giving responsibility/control to the end user of the data instead of the object holding the data. http://codereview.chromium.org/5699005/diff/103001/chrome/browser/plugin_upda... File chrome/browser/plugin_updater.h (right): http://codereview.chromium.org/5699005/diff/103001/chrome/browser/plugin_upda... chrome/browser/plugin_updater.h:39: // setting the state of a plugin that has not been loaded yet (as is the case) On 2010/12/21 19:07:39, Bernhard Bauer wrote: > Nit: extend parenthesis to include "with policy controlled plugins"; doesn't > make much sense otherwise. Done. http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_group.h (right): http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_group.h:130: std::vector<WebPluginInfo>& GetPlugins() { return web_plugin_infos_; } On 2010/12/21 20:31:19, pastarmovj wrote: > On 2010/12/21 19:57:42, John Abd-El-Malek wrote: > > we shouldn't be returning a non-const reference to the internal data > structure. > > I probably don't need this version anymore. I will remove it if this is the case > or replace it with a proper manipulator function for the aim I have. Done. http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_group.h:151: // (with priority 0). On 2010/12/21 19:07:39, Bernhard Bauer wrote: > Comment is outdated. Done. http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_group.h:157: // Set plugin placeholder status On 2010/12/21 19:07:39, Bernhard Bauer wrote: > Comment is wrong. Also, it doesn't really tell you anything you wouldn't know > from the method name (if it was correct, that is). Done. http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_group.h:235: std::vector<int> web_plugin_priority_; On 2010/12/21 19:57:42, John Abd-El-Malek wrote: > i think we can do without this. I have removed it for now but I am not sure I have cleaned all the UI traces of that one. Should I look into some localized files for the IDS_PLUGIN_PRIORITY or is the generated_resources the only one that I had to clean? http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_list.cc (right): http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list.cc:364: // Clean up the plugins that has disappeared from the groups. On 2010/12/21 19:07:39, Bernhard Bauer wrote: > Nit: have Done. http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list.cc:367: std::vector<WebPluginInfo>& gr_plugins = group->second->GetPlugins(); On 2010/12/21 20:31:19, pastarmovj wrote: > On 2010/12/21 19:07:39, Bernhard Bauer wrote: > > Can you use a const reference here? If not, that is a sign that you're > violating > > encapsulation; you're basically directly mucking around with a member variable > > of another object. > > I can and will do it no problem. I only read from the plugins. Done. http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/webpl... File webkit/plugins/npapi/webplugininfo.h (right): http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/webpl... webkit/plugins/npapi/webplugininfo.h:69: class WebPluginInfoUtils { On 2010/12/21 21:36:11, Bernhard Bauer wrote: > On 2010/12/21 20:31:19, pastarmovj wrote: > > On 2010/12/21 19:07:39, Bernhard Bauer wrote: > > > Maybe move this to a separate class? > > > > Separate file you mean? > > Uh, yes, but like John said, a namespace would probably be better. I just don't > like having a class defined in a file which doesn't correspond to its name; it > makes finding the definition for a class kinda hard. Moved to the respective user classes. Though I find this couter-OOP. We are detaching data from manipulations on that data. Moving the responsibility to the users of the data. http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/webpl... webkit/plugins/npapi/webplugininfo.h:85: const std::string& mime_type, On 2010/12/21 19:57:42, John Abd-El-Malek wrote: > nit: spacing is off Done. http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/webpl... webkit/plugins/npapi/webplugininfo.h:93: const std::string& extension, On 2010/12/21 19:57:42, John Abd-El-Malek wrote: > nit: spacing Done.
On Thu, Dec 23, 2010 at 5:00 AM, <pastarmovj@chromium.org> wrote: > Some more comments here as answers of some earlier general design > questions. I > kept enough kontext in here to make sure it is understandable which topics > are > addressed in my answers. > > > info.product_name = UTF8ToWide("PDF Internal Plugin"); >>> >> > why is this needed here? >>>> >>> > > I use the existence of a description as a marker for proper plugin and >>> not a placeholder when adding them to the list. >>> >> > > This seems another reason to store placeholders separately. Knowing that >> a >> plugin without description is a placeholder is sort of magic, and it is >> possible for plugins to not have a description. >> > > > > The problem with this list of placeholders/disabled/missing plugins is it's > maintability. > > It will end up like it used to be with the disabled plugins list we had > before. > There are many functions that actually update this list - LoadPlugin, > EnablePlugin/DisablePlugin, EnableGroup. While we were tuning the code to > try to > maintain these lists correctly it turned out to be rather complicated task > and > certainly not easy to follow. > > The placeholders on the other hand are in place mapping for these disabled > plugins. They have only one place where they should be threated with care > and > this is PluginGroup::AddPlugin where we should see if we are upgrading > existing > placeholder to complete plugin description. The PluginList clients never > see > these as they are filtered from the output too. > I agree the previous situation was more messy. But I'm not suggesting we keep disabled plugins always in a separate list as before. I'm asking how is keeping placeholder plugins different from a new list of disabled plugins, that is only consulted in PluginGroup::AddPlugin. That way we don't need to have the concept of placeholder, need to always have description, (and I think empty groups)? > > I am willing to write even more comments about this in the code if needed > but I > see it as a really clean solution to the problem. > > As of whether plugins have description or not - I extended the code to make > sure > plugins will have a description always - if there is no real one the base > filename without extenstion is adopted for description. This bleeds into the UI though. If a plugin doesn't have a description, why should we now show it having one in about:plugins? > Before it was similar > but less obvious - it used to take the full path of a plugin as groups name > which didn't allow to group simmilar plugins with different versions in > proper > > groups. > > It is not so simple to keep this list of disabled plugins in sync every >>> time they change. which happens from the UI/policy changes/(and soon >>> file appearing and disappering). The code used to do that before and it >>> failed because the connections were not that simple at all - a lot of >>> possible cause-effects can occur that were too difficult to enumerate >>> and change all too often. The implementation now hides this complexity >>> at much lower price than the code needed before to keep such an array >>> for plugins _and_ groups. >>> >> > > I totally agree that we only want one list of plugins. >> > > I'm not really following your reasons though. Can you confirm my guess as >> to why we need this? i.e. because we want to disable a plugin by its >> filename before we have loaded the list of plugins? If so, what I'm >> suggesting is that we just store this filename in a vector of FilePaths, >> instead of creating these quasi-fake PluginGroups. This would be >> temporarily stored until we build the groups. >> > > > > I mostly covered this in the previous section but one more note here : > > We should not only create it temporarily but also recreate it every time we > reload the plugins. And we have to update it every time a request for > Enable/Disable comes - which might be both for an existing or non-existing > plugin. This is going to make the code of these functions really convoluted > IMO. > > > > Indeed we do. The reason why this enum is called "Reason" and not >>> "DisabledReason" is that there is a reason for both being enabled and >>> being disabled. For now we only have disabled by policy but this soon >>> will extend to being enabled by policy - basically forcing a plugin >>> being on regardless of user wish. >>> >> > > This is a little odd. What sort of scenario would we want an enterprise >> to >> force enable a plugin and not allow the user to disable it? >> > > > Recent talk of Linus and Adobe resulted in this proposal : "- We should > make > sure that businesses can construct policies which control which PDF reader > is > used in Chrome. This does not necessarily mean that we want to force users to use a specific plugin. i.e. an enterprise might want to disable internal PDF, and enable Reader by default. But what if a user wants to disable Reader as well. It seems odd that they can't. > Most of the files with special features that we don't understand > are inside companies." Also described in this bug : > > http://code.google.com/p/chromium/issues/detail?id=67739 > > > Either way, we can still do this with enums and only keep one member >> variable for this. i.e. >> > > ENABLED_BY_POLICY = 1<< 1, >> DISABLED_BY_POLICY = 1<< 2, >> ENABLED_BY_USER = 1 << 3, >> DISABLED_BY_USER = 1 << 4, >> > > > Other than saving a few bytes per plugin I don't see this making the logic > any > more easier. This will be ok if we encapsulate the logic then in a few > functions > like IsEnabled/isManaged/enable/disable etc. I will propose to make this in > another CL which will completely capsulate WebPluginInfo in a class with > the > right OOP concepts inplace. Maybe do this when the question with PDF policy > described above gets scheduled for solving. Correct me if I'm wrong, but I thought one of the goals for this change was to allow user choice to be remembered if a policy to disable a plugin is reverted. With a boolean enable/disable flag, and an int for reason, won't it be impossible to keep this information? i.e. if a user disabled plugin X, but then a group policy enabled it, and then the policy was reverted, we wouldn't remember that the user disabled that plugin before, right? > > > > Why duplicate? The only thing I saw that was used in more than one file >> was >> the IsEnabled, which seems like the struct's member variable should be >> checked directly. I don't see any functions that should be duplicated? >> > > > I see the problem with braking basic OOP principles by giving > responsibility/control to the end user of the data instead of the object > holding > the data. I think we all agree that it would be better if the functions were with the data. But to do that, and to follow our guidelines, then the members of the class need to be encapsulated, and we need getters/setters. You didn't want to do this in this change, to avoid touching even more files. If so, then we should keep these functions in their original location, and move them to the class when we convert it from a struct. But doing only half the work leaves the code in a less consistent way (i.e. class with functions, but people can poke at its internals). > > > > > http://codereview.chromium.org/5699005/diff/103001/chrome/browser/plugin_upda... > File chrome/browser/plugin_updater.h (right): > > > http://codereview.chromium.org/5699005/diff/103001/chrome/browser/plugin_upda... > chrome/browser/plugin_updater.h:39: // setting the state of a plugin > that has not been loaded yet (as is the case) > On 2010/12/21 19:07:39, Bernhard Bauer wrote: > >> Nit: extend parenthesis to include "with policy controlled plugins"; >> > doesn't > >> make much sense otherwise. >> > > Done. > > > > http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... > File webkit/plugins/npapi/plugin_group.h (right): > > > http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... > webkit/plugins/npapi/plugin_group.h:130: std::vector<WebPluginInfo>& > GetPlugins() { return web_plugin_infos_; } > On 2010/12/21 20:31:19, pastarmovj wrote: > >> On 2010/12/21 19:57:42, John Abd-El-Malek wrote: >> > we shouldn't be returning a non-const reference to the internal data >> structure. >> > > I probably don't need this version anymore. I will remove it if this >> > is the case > >> or replace it with a proper manipulator function for the aim I have. >> > > Done. > > > http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... > webkit/plugins/npapi/plugin_group.h:151: // (with priority 0). > > On 2010/12/21 19:07:39, Bernhard Bauer wrote: > >> Comment is outdated. >> > > Done. > > > > http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... > webkit/plugins/npapi/plugin_group.h:157: // Set plugin placeholder > status > On 2010/12/21 19:07:39, Bernhard Bauer wrote: > >> Comment is wrong. Also, it doesn't really tell you anything you >> > wouldn't know > >> from the method name (if it was correct, that is). >> > > Done. > > > > http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... > webkit/plugins/npapi/plugin_group.h:235: std::vector<int> > web_plugin_priority_; > On 2010/12/21 19:57:42, John Abd-El-Malek wrote: > >> i think we can do without this. >> > > I have removed it for now but I am not sure I have cleaned all the UI > traces of that one. Should I look into some localized files for the > IDS_PLUGIN_PRIORITY or is the generated_resources the only one that I > had to clean? I believe only the generated_resources one. The others get generated from it (you can check the history of changes to it to double check). > > > > http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... > File webkit/plugins/npapi/plugin_list.cc (right): > > > http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... > webkit/plugins/npapi/plugin_list.cc:364: // Clean up the plugins that > has disappeared from the groups. > On 2010/12/21 19:07:39, Bernhard Bauer wrote: > >> Nit: have >> > > Done. > > > > http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/plugi... > webkit/plugins/npapi/plugin_list.cc:367: std::vector<WebPluginInfo>& > gr_plugins = group->second->GetPlugins(); > On 2010/12/21 20:31:19, pastarmovj wrote: > >> On 2010/12/21 19:07:39, Bernhard Bauer wrote: >> > Can you use a const reference here? If not, that is a sign that >> > you're > >> violating >> > encapsulation; you're basically directly mucking around with a >> > member variable > >> > of another object. >> > > I can and will do it no problem. I only read from the plugins. >> > > Done. > > > > http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/webpl... > File webkit/plugins/npapi/webplugininfo.h (right): > > > http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/webpl... > webkit/plugins/npapi/webplugininfo.h:69: class WebPluginInfoUtils { > On 2010/12/21 21:36:11, Bernhard Bauer wrote: > >> On 2010/12/21 20:31:19, pastarmovj wrote: >> > On 2010/12/21 19:07:39, Bernhard Bauer wrote: >> > > Maybe move this to a separate class? >> > >> > Separate file you mean? >> > > Uh, yes, but like John said, a namespace would probably be better. I >> > just don't > >> like having a class defined in a file which doesn't correspond to its >> > name; it > >> makes finding the definition for a class kinda hard. >> > > Moved to the respective user classes. Though I find this couter-OOP. We > are detaching data from manipulations on that data. Moving the > responsibility to the users of the data. > > > > http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/webpl... > webkit/plugins/npapi/webplugininfo.h:85: const std::string& mime_type, > On 2010/12/21 19:57:42, John Abd-El-Malek wrote: > >> nit: spacing is off >> > > Done. > > > > http://codereview.chromium.org/5699005/diff/103001/webkit/plugins/npapi/webpl... > webkit/plugins/npapi/webplugininfo.h:93: const std::string& extension, > On 2010/12/21 19:57:42, John Abd-El-Malek wrote: > >> nit: spacing >> > > Done. > > > http://codereview.chromium.org/5699005/ >
To shorten the response I only kept a quote of my text (prepended with >), your response (no prefix) and my answer to it (starts with ***). The rest of the opened questions we can address over VC with you, Bernhard and me. > > > Recent talk of Linus and Adobe resulted in this proposal : "- We should > make > sure that businesses can construct policies which control which PDF reader > is > used in Chrome. This does not necessarily mean that we want to force users to use a specific plugin. i.e. an enterprise might want to disable internal PDF, and enable Reader by default. But what if a user wants to disable Reader as well. It seems odd that they can't. *** As far as I got the idea this is exactly what is planned - if the company wants to enforce specific plugin running they should be able to do so. The plugin will be running and the user shouldn't be able to disable it (say some sort of virus scanner or authentication service). ###################################################################### ###################################################################### ###################################################################### > > Other than saving a few bytes per plugin I don't see this making the logic > any > more easier. This will be ok if we encapsulate the logic then in a few > functions > like IsEnabled/isManaged/enable/disable etc. I will propose to make this in > another CL which will completely capsulate WebPluginInfo in a class with > the > right OOP concepts inplace. Maybe do this when the question with PDF policy > described above gets scheduled for solving. Correct me if I'm wrong, but I thought one of the goals for this change was to allow user choice to be remembered if a policy to disable a plugin is reverted. With a boolean enable/disable flag, and an int for reason, won't it be impossible to keep this information? i.e. if a user disabled plugin X, but then a group policy enabled it, and then the policy was reverted, we wouldn't remember that the user disabled that plugin before, right? *** We will because we allow for having multiple reasons for plugin being in a given state. So that if the user disabled a plugin and then a policy does this as well then both will be stored. This works because we have the default state being enabled by both user and policy - therefore if a plugin has no enforced state it is assumed to be enabled by both user and policy. I however start to cosider enumerating the states as you propose in a single enum because there is one state that is not compatible with the default asumption of enabledness - Forcibly enabled by policy and disabled by user. The plugin should stay enabled for as long as the policy is active and get disabled afterwards but this can't be saved as a combo of enabled flag + reason.
On Mon, Jan 3, 2011 at 5:44 AM, <pastarmovj@chromium.org> wrote: > To shorten the response I only kept a quote of my text (prepended with >), > your > response (no prefix) and my answer to it (starts with ***). The rest of the > opened questions we can address over VC with you, Bernhard and me. > > > > Recent talk of Linus and Adobe resulted in this proposal : "- We should >> make >> sure that businesses can construct policies which control which PDF reader >> is >> used in Chrome. >> > > > This does not necessarily mean that we want to force users to use a > specific > plugin. i.e. an enterprise might want to disable internal PDF, and enable > Reader by default. But what if a user wants to disable Reader as well. It > seems odd that they can't. > > *** As far as I got the idea this is exactly what is planned - if the > company > wants to enforce specific plugin running they should be able to do so. The > plugin will be running and the user shouldn't be able to disable it (say > some > sort of virus scanner or authentication service). > This is really theoretical IMO, developers have kind of stopped writing NPAPI plugins.. If a user wanted to disable a particular plugin, I think we should allow them. If we don't allow them on about:plugins, then writing an extension that does so is trivial. > ###################################################################### > ###################################################################### > ###################################################################### > > Other than saving a few bytes per plugin I don't see this making the logic >> any >> more easier. This will be ok if we encapsulate the logic then in a few >> functions >> like IsEnabled/isManaged/enable/disable etc. I will propose to make this >> in >> another CL which will completely capsulate WebPluginInfo in a class with >> the >> right OOP concepts inplace. Maybe do this when the question with PDF >> policy >> described above gets scheduled for solving. >> > > > Correct me if I'm wrong, but I thought one of the goals for this change was > to allow user choice to be remembered if a policy to disable a plugin is > reverted. With a boolean enable/disable flag, and an int for reason, won't > it be impossible to keep this information? i.e. if a user disabled plugin > X, but then a group policy enabled it, and then the policy was reverted, we > wouldn't remember that the user disabled that plugin before, right? > > *** We will because we allow for having multiple reasons for plugin being > in a > given state. So that if the user disabled a plugin and then a policy does > this > as well then both will be stored. This works because we have the default > state > being enabled by both user and policy - therefore if a plugin has no > enforced > state it is assumed to be enabled by both user and policy. I however start > to > cosider enumerating the states as you propose in a single enum because > there is > one state that is not compatible with the default asumption of enabledness > - > Forcibly enabled by policy and disabled by user. The plugin should stay > enabled > for as long as the policy is active and get disabled afterwards but this > can't > be saved as a combo of enabled flag + reason. > Yep that's the same scenario I described. > > > http://codereview.chromium.org/5699005/ >
Please take a look at the last changes. I hope they represent the result of our discussion. Most important changes are in plugin_list,plugin_groups and webpluginsinfo the rest is collateral damage :) If you are fine with these i will finalize the cl on monday (there is some small tuning for mac and win to be done).
just looked at the files you mentioned http://codereview.chromium.org/5699005/diff/121001/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_group.h (right): http://codereview.chromium.org/5699005/diff/121001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_group.h:17: #include "webkit/plugins/npapi/webplugininfo.h" nit: why is this needed? http://codereview.chromium.org/5699005/diff/121001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_group.h:155: // Check if the group has no plugins or only placeholder plugins. I thought we're not adding the concept of placeholder/empty groups? http://codereview.chromium.org/5699005/diff/121001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_group.h:193: static PluginGroup* CreateEmptyGroup(const string16& name); ditto http://codereview.chromium.org/5699005/diff/121001/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_list.h (right): http://codereview.chromium.org/5699005/diff/121001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list.h:335: /*mutable*/ Lock lock_; ? http://codereview.chromium.org/5699005/diff/121001/webkit/plugins/npapi/webpl... File webkit/plugins/npapi/webplugininfo.h (right): http://codereview.chromium.org/5699005/diff/121001/webkit/plugins/npapi/webpl... webkit/plugins/npapi/webplugininfo.h:86: // Whether the plugin is enabled. nit: need to update comment
Removed the creation of empty groups in the beginning too. I have one strange failure under windows which doesn't happen on my local PC. The test_shell_tests PluginTest.Visibility fails because it can't get HWND of the window of the plugin on the try servers. Does anyone have a clue how can I reproduce this bug on my own machine, or should I do some painfull LOG debugging on the trybots? http://codereview.chromium.org/5699005/diff/121001/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_group.h (right): http://codereview.chromium.org/5699005/diff/121001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_group.h:17: #include "webkit/plugins/npapi/webplugininfo.h" On 2011/01/15 00:51:34, John Abd-El-Malek wrote: > nit: why is this needed? I need the Definition of WebPluginInfo in several places like the web_plugins_infos vector. http://codereview.chromium.org/5699005/diff/121001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_group.h:155: // Check if the group has no plugins or only placeholder plugins. On 2011/01/15 00:51:34, John Abd-El-Malek wrote: > I thought we're not adding the concept of placeholder/empty groups? I forgot to update the comment there. There are no placeholders for plugins. Right now I still have kind of placeholders for groups which are just empty groups as there were empty groups before too and I just hijacked the concept there. I will try to get rid of these too. However the IsEmpty check can't be removed because a group can become empty later on and we have to be able to catch this state and possibly prone it. http://codereview.chromium.org/5699005/diff/121001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_group.h:193: static PluginGroup* CreateEmptyGroup(const string16& name); On 2011/01/15 00:51:34, John Abd-El-Malek wrote: > ditto Removed. http://codereview.chromium.org/5699005/diff/121001/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_list.h (right): http://codereview.chromium.org/5699005/diff/121001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list.h:335: /*mutable*/ Lock lock_; On 2011/01/15 00:51:34, John Abd-El-Malek wrote: > ? Removed now. I forgot to do so but I tested if it is not needed anymore to be mutable. http://codereview.chromium.org/5699005/diff/121001/webkit/plugins/npapi/webpl... File webkit/plugins/npapi/webplugininfo.h (right): http://codereview.chromium.org/5699005/diff/121001/webkit/plugins/npapi/webpl... webkit/plugins/npapi/webplugininfo.h:86: // Whether the plugin is enabled. On 2011/01/15 00:51:34, John Abd-El-Malek wrote: > nit: need to update comment True. Done.
http://codereview.chromium.org/5699005/diff/121001/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_group.h (right): http://codereview.chromium.org/5699005/diff/121001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_group.h:155: // Check if the group has no plugins or only placeholder plugins. On 2011/01/18 18:30:39, pastarmovj wrote: > On 2011/01/15 00:51:34, John Abd-El-Malek wrote: > > I thought we're not adding the concept of placeholder/empty groups? > > I forgot to update the comment there. There are no placeholders for plugins. > Right now I still have kind of placeholders for groups which are just empty > groups as there were empty groups before too and I just hijacked the concept > there. I will try to get rid of these too. ok, so should I just wait for that before doing another full review? > > However the IsEmpty check can't be removed because a group can become empty > later on and we have to be able to catch this state and possibly prone it. can we prune it right when it becomes empty?
There we go. Plugin groups pruning in place too. The funny windows crasher disappeared after rebasing my code on much later commit. Debugging code I had in place shows that NPAPI Test pluging gets loaded and functions too. I think it was some kind of flakiness as it appeared and disappeared on its own more or less. I think it is ready for a complete review now.
http://codereview.chromium.org/5699005/diff/142001/chrome/browser/automation/... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/5699005/diff/142001/chrome/browser/automation/... chrome/browser/automation/testing_automation_provider.cc:3025: string16 name; nit: here and below, looks like name is unused? http://codereview.chromium.org/5699005/diff/142001/chrome/browser/chromeos/gv... File chrome/browser/chromeos/gview_request_interceptor_unittest.cc (right): http://codereview.chromium.org/5699005/diff/142001/chrome/browser/chromeos/gv... chrome/browser/chromeos/gview_request_interceptor_unittest.cc:72: pdf_plugin.product_name = ASCIIToWide("PDF Plugin"); is this code still needed, i.e. adding a description when one doesn't exist to make it different from placeholder plugins? http://codereview.chromium.org/5699005/diff/142001/chrome/browser/plugin_serv... File chrome/browser/plugin_service.cc (right): http://codereview.chromium.org/5699005/diff/142001/chrome/browser/plugin_serv... chrome/browser/plugin_service.cc:434: webkit::npapi::PluginList::Singleton()->RefreshPlugins(); I think we want to avoid reloading the plugins from disk when the user enables/disables plugins, since it's an expensive operation. http://codereview.chromium.org/5699005/diff/142001/chrome/browser/plugin_upda... File chrome/browser/plugin_updater.cc (right): http://codereview.chromium.org/5699005/diff/142001/chrome/browser/plugin_upda... chrome/browser/plugin_updater.cc:192: if (!enabled) { nit: no need to add the brace brackets, to be consistent with the rest of the file http://codereview.chromium.org/5699005/diff/142001/chrome/browser/plugin_upda... File chrome/browser/plugin_updater.h (right): http://codereview.chromium.org/5699005/diff/142001/chrome/browser/plugin_upda... chrome/browser/plugin_updater.h:40: const FilePath::StringType& file_path); nit: i think these two parameters can fit on the same line, to be consistent with the rest of te file http://codereview.chromium.org/5699005/diff/142001/chrome/common/pepper_plugi... File chrome/common/pepper_plugin_registry.cc (right): http://codereview.chromium.org/5699005/diff/142001/chrome/common/pepper_plugi... chrome/common/pepper_plugin_registry.cc:124: // Empty description is not allowed so use the plugin base name if none is this is not the case anymore, right? http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_group.cc (right): http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_group.cc:201: if (!enabled_ || description_.empty()) { don't need to look for empty description anymore right? http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_group.cc:222: bool PluginGroup::AddPlugin(const WebPluginInfo& plugin, int priority) { priority isn't used anymore http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_group.cc:285: web_plugin_infos_.front().path.BaseName().RemoveExtension().value(); nit: any reason to change this? we avoid changing code in changes to match personal style, as that leads to a lot of hysteresis if everyone does it. http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_group.h (right): http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_group.h:120: // Enables/disables this group. This enables/disables all plugins in the nit: comment is copy and pasted from above http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_group.h:135: const std::vector<WebPluginInfo>& GetPlugins() const; simple getters should just be named in unix_hacker style, i.e. web_plugin_infos() { return web_plugin_infos_; } http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_group.h:137: bool ContainsPlugin(const FilePath& path) const; nit: small comment to match the rest of the header please http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_group.h:157: bool IsEmpty() const; this isn't needed anymore right? http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_group.h:168: typedef std::map<std::string, PluginGroup*> PluginMap; this typedef isn't used by PluginGroup, so it should be removed from here http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_list.cc (right): http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list.cc:325: next_priority_(1) { next_priority_ isn't needed amymore http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list.cc:418: if (group->IsEmpty()) { if plugin_groups_ was a vector, wouldn't need the temporary empty_groups and pruning since you can just erase it right away and do a continue. http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list.cc:469: AutoLock lock(lock_); this code is broken I think. unfortunately the way the groups stuff got bolted onto PluginList broke one of the characteristics of PluginList, which is that loading the plugins doesn't use the lock or update any member variables until everything is loaded off disk, and then, does it in atomically. Now, plugin_groups_ is changing during refreshing the plugins. Someone calling GetPlugins() now during plugin loading can get inconsistent results. What we need is for LoadPlugin to not modify any member variables, including plugin_groups_ (i.e. indirectly through AddToPluginGroups). It should treat groups like plugins, i.e. it should have another parameter that is a container holding PluginGroup, and it should only modify that. Then LoadPlugins() can swap the new plugin_groups_ in at the last minute, just like how it used to do with plugins_ http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_list.h (left): http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list.h:106: void RegisterInternalPlugin(const FilePath& path); why take this out? it was convenient for internal plugins. http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_list.h (right): http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list.h:191: string16 GetPluginGroupName(std::string identifier); nit: while you're at it, this should be const std::string& http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list.h:247: PluginGroup::PluginMap plugin_groups_; does this really need to be a map? we used to use a vector before because then ordering is a way of signifying who gets first dibs. this way we can put the plugin installer plugin (i.e. the one that catches all mime-types) at the end. we would also put internal plugins first, i.e. the pdf. I've looked at the usage of this, and almost all places just iterate over it. for the other (2) places, they could iterate manually. so i think given that performance is not an issue, we should use a vector as before. http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list.h:328: std::vector<FilePath> prematurely_disabled_plugins_; comment please http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_list_mac.mm (right): http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list_mac.mm:101: if (plugins->at(i).path.BaseName() == info.path.BaseName()) { there's no change here, so please just leave this file unmodified. changing code for personal style is something we avoid, as otherwise the code will continually change as people change it to suit their preferences http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_list_win.cc (right): http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list_win.cc:342: StringToLowerASCII(plugins->at(i).path.BaseName().ToWStringHack()); ditto for this file, it doesn't look like any changes are necessary here. http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/webpl... File webkit/plugins/npapi/webplugininfo.h (right): http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/webpl... webkit/plugins/npapi/webplugininfo.h:90: bool IsPluginEnabled(const WebPluginInfo& plugin); comment please
@John: I tried to address all your comments in this new iteration of the CL. Most of the stuff has been changed as you proposed. I left plugin_groups_ beings a map because of the way the key of this map is defined and used and that I don't see an easy way around it. Most uses as you mention are straightforward iterations but the rest is not really without consequences. Please if you got the time take a look at my comments/edits so that I can address any new ones tomorrow to win one more iteration on this CL as it will be great to get it done until M10. http://codereview.chromium.org/5699005/diff/142001/chrome/browser/automation/... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/5699005/diff/142001/chrome/browser/automation/... chrome/browser/automation/testing_automation_provider.cc:3025: string16 name; On 2011/01/19 20:22:09, John Abd-El-Malek wrote: > nit: here and below, looks like name is unused? Done. Undone small changes to following lines that were not relevant too. http://codereview.chromium.org/5699005/diff/142001/chrome/browser/chromeos/gv... File chrome/browser/chromeos/gview_request_interceptor_unittest.cc (right): http://codereview.chromium.org/5699005/diff/142001/chrome/browser/chromeos/gv... chrome/browser/chromeos/gview_request_interceptor_unittest.cc:72: pdf_plugin.product_name = ASCIIToWide("PDF Plugin"); On 2011/01/19 20:22:09, John Abd-El-Malek wrote: > is this code still needed, i.e. adding a description when one doesn't exist to > make it different from placeholder plugins? Actually yes. This plugin has description in it's original PluginList registrastion. All internal plugins get registered through the RegisterInternalPlugin function that has 5 params - path, name, desc, mime-types, entry points. Actually this one RegisterInternalPlugin function that takes only path is used only in this single unit test suite. That is also why I removed it from PluginLists. It was not used anywhere else. http://codereview.chromium.org/5699005/diff/142001/chrome/browser/plugin_serv... File chrome/browser/plugin_service.cc (right): http://codereview.chromium.org/5699005/diff/142001/chrome/browser/plugin_serv... chrome/browser/plugin_service.cc:434: webkit::npapi::PluginList::Singleton()->RefreshPlugins(); On 2011/01/19 20:22:09, John Abd-El-Malek wrote: > I think we want to avoid reloading the plugins from disk when the user > enables/disables plugins, since it's an expensive operation. If we don't do that here policy changes during the runtime won't take effect until next restart. I don't think that is so bad especially because we only mark plugins for refresh here we don't enforce it. And the UI anyway will refresh the plugins always after the user actually interacts with it. http://codereview.chromium.org/5699005/diff/142001/chrome/browser/plugin_upda... File chrome/browser/plugin_updater.cc (right): http://codereview.chromium.org/5699005/diff/142001/chrome/browser/plugin_upda... chrome/browser/plugin_updater.cc:192: if (!enabled) { On 2011/01/19 20:22:09, John Abd-El-Malek wrote: > nit: no need to add the brace brackets, to be consistent with the rest of the > file Done. http://codereview.chromium.org/5699005/diff/142001/chrome/browser/plugin_upda... File chrome/browser/plugin_updater.h (right): http://codereview.chromium.org/5699005/diff/142001/chrome/browser/plugin_upda... chrome/browser/plugin_updater.h:40: const FilePath::StringType& file_path); On 2011/01/19 20:22:09, John Abd-El-Malek wrote: > nit: i think these two parameters can fit on the same line, to be consistent > with the rest of te file Done. http://codereview.chromium.org/5699005/diff/142001/chrome/common/pepper_plugi... File chrome/common/pepper_plugin_registry.cc (right): http://codereview.chromium.org/5699005/diff/142001/chrome/common/pepper_plugi... chrome/common/pepper_plugin_registry.cc:124: // Empty description is not allowed so use the plugin base name if none is On 2011/01/19 20:22:09, John Abd-El-Malek wrote: > this is not the case anymore, right? Done. http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_group.cc (right): http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_group.cc:201: if (!enabled_ || description_.empty()) { On 2011/01/19 20:22:09, John Abd-El-Malek wrote: > don't need to look for empty description anymore right? Actually I do now. This is not part of the enforce plugins to have descriptions effort I had before. Here I actually had to introduce that as a counterpart to the same check in the else clause because before we added groups as being disabled and now I have them in both states depending on various criteria (user disabled plugin,disabled group or policy). It just makes sure the group that has no name gets one from it's first plugin no matter whether it is enabled or not and what the group's state is. No real change in the behavior here only capturing the possible group's enabled states. http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_group.cc:222: bool PluginGroup::AddPlugin(const WebPluginInfo& plugin, int priority) { On 2011/01/19 20:22:09, John Abd-El-Malek wrote: > priority isn't used anymore Done. http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_group.cc:285: web_plugin_infos_.front().path.BaseName().RemoveExtension().value(); On 2011/01/19 20:22:09, John Abd-El-Malek wrote: > nit: any reason to change this? we avoid changing code in changes to match > personal style, as that leads to a lot of hysteresis if everyone does it. Relict from the times this wasn't a vector. Reverted. http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_group.h (right): http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_group.h:120: // Enables/disables this group. This enables/disables all plugins in the On 2011/01/19 20:22:09, John Abd-El-Malek wrote: > nit: comment is copy and pasted from above Fixed. Sorry about that. http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_group.h:135: const std::vector<WebPluginInfo>& GetPlugins() const; On 2011/01/19 20:22:09, John Abd-El-Malek wrote: > simple getters should just be named in unix_hacker style, i.e. > web_plugin_infos() { return web_plugin_infos_; } Done. http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_group.h:137: bool ContainsPlugin(const FilePath& path) const; On 2011/01/19 20:22:09, John Abd-El-Malek wrote: > nit: small comment to match the rest of the header please Done. http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_group.h:157: bool IsEmpty() const; On 2011/01/19 20:22:09, John Abd-El-Malek wrote: > this isn't needed anymore right? It is needed only for actually checking which groups should be pruned. It is a shortcut to web_plugin_infos()->size() == 0. http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_group.h:168: typedef std::map<std::string, PluginGroup*> PluginMap; On 2011/01/19 20:22:09, John Abd-El-Malek wrote: > this typedef isn't used by PluginGroup, so it should be removed from here The problem is that we want to differentiate different groups based on their identifiers (which mostly coincide with names but in cases of collisions don't see. PluginGroup::AddToPluginGroups). Therefore we have this maps there. http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_list.cc (right): http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list.cc:325: next_priority_(1) { On 2011/01/19 20:22:09, John Abd-El-Malek wrote: > next_priority_ isn't needed amymore Done. http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list.cc:418: if (group->IsEmpty()) { On 2011/01/19 20:22:09, John Abd-El-Malek wrote: > if plugin_groups_ was a vector, wouldn't need the temporary empty_groups and > pruning since you can just erase it right away and do a continue. Ditto copied from plugin_group.cc question about the typedef: The problem is that we want to differentiate different groups based on their identifiers (which mostly coincide with names but in cases of collisions don't see. PluginGroup::AddToPluginGroups). Therefore we have this maps there. http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list.cc:469: AutoLock lock(lock_); On 2011/01/19 20:22:09, John Abd-El-Malek wrote: > this code is broken I think. unfortunately the way the groups stuff got bolted > onto PluginList broke one of the characteristics of PluginList, which is that > loading the plugins doesn't use the lock or update any member variables until > everything is loaded off disk, and then, does it in atomically. Now, > plugin_groups_ is changing during refreshing the plugins. Someone calling > GetPlugins() now during plugin loading can get inconsistent results. > > What we need is for LoadPlugin to not modify any member variables, including > plugin_groups_ (i.e. indirectly through AddToPluginGroups). It should treat > groups like plugins, i.e. it should have another parameter that is a container > holding PluginGroup, and it should only modify that. Then LoadPlugins() can > swap the new plugin_groups_ in at the last minute, just like how it used to do > with plugins_ I don't think it is that bad. Actually the way we handle this list now ensures a bit more consistency than before. Before until the load has finished we had the old list. Now every new plugin discovered is immediately visible to a new GetPlugins and all old are too. In this sense more "true" information is present. The end of LoadPlugins locks the access to the groups and prunes the missing plugins so that after it finishes the rest is only true information. http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_list.h (left): http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list.h:106: void RegisterInternalPlugin(const FilePath& path); On 2011/01/19 20:22:09, John Abd-El-Malek wrote: > why take this out? it was convenient for internal plugins. As already mentioned in my comment to http://codereview.chromium.org/5699005/diff/142001/chrome/browser/automation/... this function was only used in one unit test. (And after Evan's CL even there not anymore). It doesn't contribute to the interface it only can be source of confusion. All plugins use the full fledged 5 param version (and have to because they need to supply the entry points array). http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_list.h (right): http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list.h:191: string16 GetPluginGroupName(std::string identifier); On 2011/01/19 20:22:09, John Abd-El-Malek wrote: > nit: while you're at it, this should be const std::string& Done. http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list.h:247: PluginGroup::PluginMap plugin_groups_; On 2011/01/19 20:22:09, John Abd-El-Malek wrote: > does this really need to be a map? we used to use a vector before because then > ordering is a way of signifying who gets first dibs. this way we can put the > plugin installer plugin (i.e. the one that catches all mime-types) at the end. > we would also put internal plugins first, i.e. the pdf. > > I've looked at the usage of this, and almost all places just iterate over it. > for the other (2) places, they could iterate manually. so i think given that > performance is not an issue, we should use a vector as before. Copied from plugin_group.cc question about the typedef: The problem is that we want to differentiate different groups based on their identifiers (which mostly coincide with names but in cases of collisions don't see. PluginGroup::AddToPluginGroups). Therefore we have this maps there. http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list.h:328: std::vector<FilePath> prematurely_disabled_plugins_; On 2011/01/19 20:22:09, John Abd-El-Malek wrote: > comment please Done. http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_list_mac.mm (right): http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list_mac.mm:101: if (plugins->at(i).path.BaseName() == info.path.BaseName()) { On 2011/01/19 20:22:09, John Abd-El-Malek wrote: > there's no change here, so please just leave this file unmodified. changing > code for personal style is something we avoid, as otherwise the code will > continually change as people change it to suit their preferences Done. http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_list_win.cc (right): http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list_win.cc:342: StringToLowerASCII(plugins->at(i).path.BaseName().ToWStringHack()); On 2011/01/19 20:22:09, John Abd-El-Malek wrote: > ditto for this file, it doesn't look like any changes are necessary here. Done. http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/webpl... File webkit/plugins/npapi/webplugininfo.h (right): http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/webpl... webkit/plugins/npapi/webplugininfo.h:90: bool IsPluginEnabled(const WebPluginInfo& plugin); On 2011/01/19 20:22:09, John Abd-El-Malek wrote: > comment please Done.
http://codereview.chromium.org/5699005/diff/142001/chrome/browser/chromeos/gv... File chrome/browser/chromeos/gview_request_interceptor_unittest.cc (right): http://codereview.chromium.org/5699005/diff/142001/chrome/browser/chromeos/gv... chrome/browser/chromeos/gview_request_interceptor_unittest.cc:72: pdf_plugin.product_name = ASCIIToWide("PDF Plugin"); On 2011/01/19 23:39:17, pastarmovj wrote: > On 2011/01/19 20:22:09, John Abd-El-Malek wrote: > > is this code still needed, i.e. adding a description when one doesn't exist to > > make it different from placeholder plugins? > > Actually yes. This plugin has description in it's original PluginList > registrastion. All internal plugins get registered through the > RegisterInternalPlugin function that has 5 params - path, name, desc, > mime-types, entry points. Actually this one RegisterInternalPlugin function that > takes only path is used only in this single unit test suite. That is also why I > removed it from PluginLists. It was not used anywhere else. oops, I didn't notice that this is a test file, in that case, ignore my comment. I always favor making test code more verbose/ugly over doing the same to shipping code :) http://codereview.chromium.org/5699005/diff/142001/chrome/browser/plugin_serv... File chrome/browser/plugin_service.cc (right): http://codereview.chromium.org/5699005/diff/142001/chrome/browser/plugin_serv... chrome/browser/plugin_service.cc:434: webkit::npapi::PluginList::Singleton()->RefreshPlugins(); On 2011/01/19 23:39:17, pastarmovj wrote: > On 2011/01/19 20:22:09, John Abd-El-Malek wrote: > > I think we want to avoid reloading the plugins from disk when the user > > enables/disables plugins, since it's an expensive operation. > > If we don't do that here policy changes during the runtime won't take effect > until next restart. I don't think that is so bad especially because we only mark > plugins for refresh here we don't enforce it. > And the UI anyway will refresh the plugins always after the user actually > interacts with it. I see, I didn't know that the UI always marks it for refresh. It would be nice to avoid that, since no need to do all that unnecessary disk access. But since it should be rare, ok for now although I'd love to see that changed in the future :) http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_group.cc (right): http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_group.cc:201: if (!enabled_ || description_.empty()) { On 2011/01/19 23:39:17, pastarmovj wrote: > On 2011/01/19 20:22:09, John Abd-El-Malek wrote: > > don't need to look for empty description anymore right? > > Actually I do now. This is not part of the enforce plugins to have descriptions > effort I had before. Here I actually had to introduce that as a counterpart to > the same check in the else clause because before we added groups as being > disabled and now I have them in both states depending on various criteria (user > disabled plugin,disabled group or policy). It just makes sure the group that has > no name gets one from it's first plugin no matter whether it is enabled or not > and what the group's state is. No real change in the behavior here only > capturing the possible group's enabled states. Got it. Please add a comment as this is hard to understand when looking at the code. Can this logic be factored into two separate checks? i.e. when to take care of enabled_, and another of description? http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_group.h (right): http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_group.h:168: typedef std::map<std::string, PluginGroup*> PluginMap; On 2011/01/19 23:39:17, pastarmovj wrote: > On 2011/01/19 20:22:09, John Abd-El-Malek wrote: > > this typedef isn't used by PluginGroup, so it should be removed from here > > The problem is that we want to differentiate different groups based on their > identifiers (which mostly coincide with names but in cases of collisions don't > see. PluginGroup::AddToPluginGroups). Therefore we have this maps there. I don't understand what you mean. AddToPluginGroups is in PluginList, not PluginGroup. I don't see any code in PluginGroup using PluginMap typedef, so I'm saying the typedef doesn't belong in this file, it should be moved to PluginList's header. http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_list.cc (right): http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list.cc:418: if (group->IsEmpty()) { On 2011/01/19 23:39:17, pastarmovj wrote: > On 2011/01/19 20:22:09, John Abd-El-Malek wrote: > > if plugin_groups_ was a vector, wouldn't need the temporary empty_groups and > > pruning since you can just erase it right away and do a continue. > > Ditto copied from plugin_group.cc question about the typedef: > The problem is that we want to differentiate different groups based on their > identifiers (which mostly coincide with names but in cases of collisions don't > see. PluginGroup::AddToPluginGroups). Therefore we have this maps there. in case of collision (when exactly?), can't we just set the group's name to the new name? http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list.cc:469: AutoLock lock(lock_); On 2011/01/19 23:39:17, pastarmovj wrote: > On 2011/01/19 20:22:09, John Abd-El-Malek wrote: > > this code is broken I think. unfortunately the way the groups stuff got > bolted > > onto PluginList broke one of the characteristics of PluginList, which is that > > loading the plugins doesn't use the lock or update any member variables until > > everything is loaded off disk, and then, does it in atomically. Now, > > plugin_groups_ is changing during refreshing the plugins. Someone calling > > GetPlugins() now during plugin loading can get inconsistent results. > > > > What we need is for LoadPlugin to not modify any member variables, including > > plugin_groups_ (i.e. indirectly through AddToPluginGroups). It should treat > > groups like plugins, i.e. it should have another parameter that is a container > > holding PluginGroup, and it should only modify that. Then LoadPlugins() can > > swap the new plugin_groups_ in at the last minute, just like how it used to do > > with plugins_ > > I don't think it is that bad. Actually the way we handle this list now ensures a > bit more consistency than before. Before until the load has finished we had the > old list. Now every new plugin discovered is immediately visible to a new > GetPlugins and all old are too. In this sense more "true" information is > present. The end of LoadPlugins locks the access to the groups and prunes the > missing plugins so that after it finishes the rest is only true information. I'm not sure which consistency you're referring to? Now, if the plugin is in the process of being updated on disk, you might get both new and old versions together, no? I don't think we want to send a list of plugins that isn't fully finished updating yet to callers. There could be other weird scenarios. The old code didn't modify the state while updating to avoid any issues, and also to simplify the locking (i.e. it was sure that no locks would be held by functions it used). http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_list.h (right): http://codereview.chromium.org/5699005/diff/142001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list.h:247: PluginGroup::PluginMap plugin_groups_; On 2011/01/19 23:39:17, pastarmovj wrote: > On 2011/01/19 20:22:09, John Abd-El-Malek wrote: > > does this really need to be a map? we used to use a vector before because > then > > ordering is a way of signifying who gets first dibs. this way we can put the > > plugin installer plugin (i.e. the one that catches all mime-types) at the end. > > > we would also put internal plugins first, i.e. the pdf. > > > > I've looked at the usage of this, and almost all places just iterate over it. > > for the other (2) places, they could iterate manually. so i think given that > > performance is not an issue, we should use a vector as before. > > Copied from plugin_group.cc question about the typedef: > The problem is that we want to differentiate different groups based on their > identifiers (which mostly coincide with names but in cases of collisions don't > see. PluginGroup::AddToPluginGroups). Therefore we have this maps there. Sorry I don't follow here. if AddToPluginGroups finds that it wants to insert a group with an identifier that already exists (I'm actually not sure I understand when this situation can happen?), why shouldn't we just set the new group's identifier() to its GetLongIdentifier()?
I tried to make LoadPlugins thread safe as it used to be before the refactoring. Also addressed the other comments you had.
lgtm with these nits http://codereview.chromium.org/5699005/diff/160001/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_group.h (right): http://codereview.chromium.org/5699005/diff/160001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_group.h:131: std::string& identifier() { return identifier_; } our style guide disallows non-const references. for setting a member variable, there should be getter and setter, i.e. std::string identifier() const { return identifier_; } void set_identifier(const std::string& identifier) { return identifier_ = identifier; } http://codereview.chromium.org/5699005/diff/160001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_group.h:138: std::vector<WebPluginInfo>& web_info_plugins(); the non const version seems dangereous to have as public, since it might be used incorrectly. Since PluginList is the only one using it, and it's already a friend, this should be moved into private also, the style is to have simple getters put their implementation in the header, and their name must match the variable. i.e. const std::vector<WebPluginInfo>& web_plugin_infos() const { return web_plugin_infos_; } http://codereview.chromium.org/5699005/diff/160001/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_list.cc (right): http://codereview.chromium.org/5699005/diff/160001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list.cc:414: if (plugin_found >= 0) { nit: no need for brace brackets http://codereview.chromium.org/5699005/diff/160001/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_list.h (right): http://codereview.chromium.org/5699005/diff/160001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list.h:174: const PluginGroup* GetPluginGroup(const WebPluginInfo& web_plugin_info); this function is now only called from test code. is this big comment still needed? since it allows adding a group directly, which we don't need, can it be made protected/private, and make this class be a friend of PluginListTest? (the parent PluginListTest can have a helper for calling the function).
Addressed the last batch of comments. Last open point is http://codereview.chromium.org/5699005/diff/160001/webkit/plugins/npapi/plugi... please take a look at my question to your comment. Apart from this waiting for bot happiness. Reason for unhappiness until now was trivial - forgotten/wrongly merged "enabled = true" instead of "enabled = WebPluginInfo::USER_ENABLED_POLICY_UNMANAGED". Seems like end is near :) http://codereview.chromium.org/5699005/diff/160001/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_group.h (right): http://codereview.chromium.org/5699005/diff/160001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_group.h:131: std::string& identifier() { return identifier_; } On 2011/01/20 18:22:34, John Abd-El-Malek wrote: > our style guide disallows non-const references. for setting a member variable, > there should be getter and setter, i.e. > > std::string identifier() const { return identifier_; } > void set_identifier(const std::string& identifier) { return identifier_ = > identifier; } Done. http://codereview.chromium.org/5699005/diff/160001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_group.h:138: std::vector<WebPluginInfo>& web_info_plugins(); On 2011/01/20 18:22:34, John Abd-El-Malek wrote: > the non const version seems dangereous to have as public, since it might be used > incorrectly. Since PluginList is the only one using it, and it's already a > friend, this should be moved into private > > also, the style is to have simple getters put their implementation in the > header, and their name must match the variable. i.e. > > const std::vector<WebPluginInfo>& web_plugin_infos() const { > return web_plugin_infos_; > } Done. Sorry for the word swap there :( http://codereview.chromium.org/5699005/diff/160001/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_list.cc (right): http://codereview.chromium.org/5699005/diff/160001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list.cc:414: if (plugin_found >= 0) { On 2011/01/20 18:22:34, John Abd-El-Malek wrote: > nit: no need for brace brackets Done. http://codereview.chromium.org/5699005/diff/160001/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_list.h (right): http://codereview.chromium.org/5699005/diff/160001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list.h:174: const PluginGroup* GetPluginGroup(const WebPluginInfo& web_plugin_info); On 2011/01/20 18:22:34, John Abd-El-Malek wrote: > this function is now only called from test code. is this big comment still > needed? > > since it allows adding a group directly, which we don't need, can it be made > protected/private, and make this class be a friend of PluginListTest? (the > parent PluginListTest can have a helper for calling the function). I wish I can but it is also used from RenderView::CreatePlugin and could be used from other sources too. But the comment above is quite good at pointing the dangers of this function. I could split this function in two - one that does allow adding new groups and one that doesn't. I don't think this is really needed but if you think it is important to make this separation I will do so?
ah, I must have missed the other places the function is used in. lgtm. thanks for the patience, this is a great cleanup :) On Thu, Jan 20, 2011 at 2:59 PM, <pastarmovj@chromium.org> wrote: > Addressed the last batch of comments. Last open point is > > > http://codereview.chromium.org/5699005/diff/160001/webkit/plugins/npapi/plugi... > please take a look at my question to your comment. > > Apart from this waiting for bot happiness. Reason for unhappiness until now > was > trivial - forgotten/wrongly merged "enabled = true" instead of "enabled = > WebPluginInfo::USER_ENABLED_POLICY_UNMANAGED". > > Seems like end is near :) > > > > > http://codereview.chromium.org/5699005/diff/160001/webkit/plugins/npapi/plugi... > File webkit/plugins/npapi/plugin_group.h (right): > > > http://codereview.chromium.org/5699005/diff/160001/webkit/plugins/npapi/plugi... > webkit/plugins/npapi/plugin_group.h:131: std::string& identifier() { > return identifier_; } > On 2011/01/20 18:22:34, John Abd-El-Malek wrote: > >> our style guide disallows non-const references. for setting a member >> > variable, > >> there should be getter and setter, i.e. >> > > std::string identifier() const { return identifier_; } >> void set_identifier(const std::string& identifier) { return >> > identifier_ = > >> identifier; } >> > > Done. > > > > http://codereview.chromium.org/5699005/diff/160001/webkit/plugins/npapi/plugi... > webkit/plugins/npapi/plugin_group.h:138: std::vector<WebPluginInfo>& > web_info_plugins(); > On 2011/01/20 18:22:34, John Abd-El-Malek wrote: > >> the non const version seems dangereous to have as public, since it >> > might be used > >> incorrectly. Since PluginList is the only one using it, and it's >> > already a > >> friend, this should be moved into private >> > > also, the style is to have simple getters put their implementation in >> > the > >> header, and their name must match the variable. i.e. >> > > const std::vector<WebPluginInfo>& web_plugin_infos() const { >> return web_plugin_infos_; >> } >> > > Done. Sorry for the word swap there :( > > > > http://codereview.chromium.org/5699005/diff/160001/webkit/plugins/npapi/plugi... > File webkit/plugins/npapi/plugin_list.cc (right): > > > http://codereview.chromium.org/5699005/diff/160001/webkit/plugins/npapi/plugi... > webkit/plugins/npapi/plugin_list.cc:414: if (plugin_found >= 0) { > On 2011/01/20 18:22:34, John Abd-El-Malek wrote: > >> nit: no need for brace brackets >> > > Done. > > > > http://codereview.chromium.org/5699005/diff/160001/webkit/plugins/npapi/plugi... > File webkit/plugins/npapi/plugin_list.h (right): > > > http://codereview.chromium.org/5699005/diff/160001/webkit/plugins/npapi/plugi... > webkit/plugins/npapi/plugin_list.h:174: const PluginGroup* > GetPluginGroup(const WebPluginInfo& web_plugin_info); > On 2011/01/20 18:22:34, John Abd-El-Malek wrote: > >> this function is now only called from test code. is this big comment >> > still > >> needed? >> > > since it allows adding a group directly, which we don't need, can it >> > be made > >> protected/private, and make this class be a friend of PluginListTest? >> > (the > >> parent PluginListTest can have a helper for calling the function). >> > > I wish I can but it is also used from RenderView::CreatePlugin and could > be used from other sources too. But the comment above is quite good at > pointing the dangers of this function. > > I could split this function in two - one that does allow adding new > groups and one that doesn't. I don't think this is really needed but if > you think it is important to make this separation I will do so? > > > http://codereview.chromium.org/5699005/ >
I just uploaded the last version of this CL. The trybots are happy. The valgrind bot will be happy once the suppressions CL i suggested gets in. (no new suppression only fixed the prefix which was not fixed there s/NPAPI/webkit::npapi/). @John - no need to re-review no changes in this CL only fixed the nits and merged with latest trunk. @Bernhard - waiting for your LGTM and we are good to go.
Some minor stuff, mostly nits: http://codereview.chromium.org/5699005/diff/178001/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_list.cc (right): http://codereview.chromium.org/5699005/diff/178001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list.cc:746: // Non existing plugin is beig enabled. Ignore this request for now because Nit: being http://codereview.chromium.org/5699005/diff/178001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list.cc:747: // plugins are anyway born enabled. Nit: I wouldn't antropomorphize plugins so far as to say they're born, they're created ;-) http://codereview.chromium.org/5699005/diff/178001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list.cc:747: // plugins are anyway born enabled. What happens if I disable a plugin, then enable it again, all before they are actually loaded? Bonus points* if you add a test for it. *Bonus points are not redeemable. http://codereview.chromium.org/5699005/diff/178001/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_list.h (right): http://codereview.chromium.org/5699005/diff/178001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list.h:320: // The set of plugins that have been scheduled for disabled once they get Nit: disabling http://codereview.chromium.org/5699005/diff/178001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list.h:324: std::vector<FilePath> prematurely_disabled_plugins_; "prematurely disabled" sounds like it was wrong to disable them. Maybe |plugins_to_disable_|? http://codereview.chromium.org/5699005/diff/178001/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_list_unittest.cc (right): http://codereview.chromium.org/5699005/diff/178001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list_unittest.cc:38: LoadDummyPlugin(&plugins_to_load_[i]); Can't you just directly call AddToPluginGroups? http://codereview.chromium.org/5699005/diff/178001/webkit/plugins/npapi/webpl... File webkit/plugins/npapi/webplugininfo.h (right): http://codereview.chromium.org/5699005/diff/178001/webkit/plugins/npapi/webpl... webkit/plugins/npapi/webplugininfo.h:90: // Checks wheter a plugin is enabled either by the user or by policy. Nit: "whether". http://codereview.chromium.org/5699005/diff/180001/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_list.h (right): http://codereview.chromium.org/5699005/diff/180001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list.h:224: virtual void ProcessGroupAfterInitialize(PluginGroup* group) { } I think this is not being used anymore.
Addressed comments from Bernhard. Pending trybot happyness. http://codereview.chromium.org/5699005/diff/178001/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_list.cc (right): http://codereview.chromium.org/5699005/diff/178001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list.cc:746: // Non existing plugin is beig enabled. Ignore this request for now because On 2011/01/21 17:29:37, Bernhard Bauer wrote: > Nit: being Done. http://codereview.chromium.org/5699005/diff/178001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list.cc:747: // plugins are anyway born enabled. On 2011/01/21 17:29:37, Bernhard Bauer wrote: > What happens if I disable a plugin, then enable it again, all before they are > actually loaded? Bonus points* if you add a test for it. > > *Bonus points are not redeemable. Fixed that now both enable and disable properly update the list of plugins for disabling. http://codereview.chromium.org/5699005/diff/178001/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_list.h (right): http://codereview.chromium.org/5699005/diff/178001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list.h:320: // The set of plugins that have been scheduled for disabled once they get On 2011/01/21 17:29:37, Bernhard Bauer wrote: > Nit: disabling Done. http://codereview.chromium.org/5699005/diff/178001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list.h:324: std::vector<FilePath> prematurely_disabled_plugins_; On 2011/01/21 17:29:37, Bernhard Bauer wrote: > "prematurely disabled" sounds like it was wrong to disable them. Maybe > |plugins_to_disable_|? Done. http://codereview.chromium.org/5699005/diff/178001/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_list_unittest.cc (right): http://codereview.chromium.org/5699005/diff/178001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list_unittest.cc:38: LoadDummyPlugin(&plugins_to_load_[i]); On 2011/01/21 17:29:37, Bernhard Bauer wrote: > Can't you just directly call AddToPluginGroups? Done. http://codereview.chromium.org/5699005/diff/178001/webkit/plugins/npapi/webpl... File webkit/plugins/npapi/webplugininfo.h (right): http://codereview.chromium.org/5699005/diff/178001/webkit/plugins/npapi/webpl... webkit/plugins/npapi/webplugininfo.h:90: // Checks wheter a plugin is enabled either by the user or by policy. On 2011/01/21 17:29:37, Bernhard Bauer wrote: > Nit: "whether". Done. http://codereview.chromium.org/5699005/diff/180001/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_list.h (right): http://codereview.chromium.org/5699005/diff/180001/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list.h:224: virtual void ProcessGroupAfterInitialize(PluginGroup* group) { } On 2011/01/21 17:29:37, Bernhard Bauer wrote: > I think this is not being used anymore. True. Removed it.
http://codereview.chromium.org/5699005/diff/138012/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_list.cc (right): http://codereview.chromium.org/5699005/diff/138012/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list.cc:746: if (plugins_to_disable_[i] == filename) { If you make |plugins_to_disable_| a std::set<FilePath>, you can add/remove plugins from there more easily. Same for |groups_to_disable_|. http://codereview.chromium.org/5699005/diff/138012/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_list_unittest.cc (right): http://codereview.chromium.org/5699005/diff/138012/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list_unittest.cc:34: DisablePlugin(plugins_to_disable_[i]); Uh, why is this in here (and not in the real PluginList)? Doesn't that mean you're just testing the fake one here?
Fixed last two comments. Changes only in plugin_list.* and plugin_list_unittest.cc. http://codereview.chromium.org/5699005/diff/138012/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_list.cc (right): http://codereview.chromium.org/5699005/diff/138012/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list.cc:746: if (plugins_to_disable_[i] == filename) { On 2011/01/24 12:20:17, Bernhard Bauer wrote: > If you make |plugins_to_disable_| a std::set<FilePath>, you can add/remove > plugins from there more easily. Same for |groups_to_disable_|. Done. http://codereview.chromium.org/5699005/diff/138012/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_list_unittest.cc (right): http://codereview.chromium.org/5699005/diff/138012/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list_unittest.cc:34: DisablePlugin(plugins_to_disable_[i]); On 2011/01/24 12:20:17, Bernhard Bauer wrote: > Uh, why is this in here (and not in the real PluginList)? Doesn't that mean > you're just testing the fake one here? Partially yes. I extracted the functionality from the original LoadPlugins method and call it instead.
http://codereview.chromium.org/5699005/diff/120022/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_list_unittest.cc (right): http://codereview.chromium.org/5699005/diff/120022/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list_unittest.cc:33: UpdatePluginsEnabledFlags(&plugin_groups_); What I meant was actually extracting the part that accesses the disk into a LoadPluginsInternal (or something) method, and then only overriding that one here ;-) That way you can also make |plugins_loaded_| private again.
Addressed the last comment. Changes again only to plugin_list.*, plugin_gropup.h and plugin_list_unittest.cc. http://codereview.chromium.org/5699005/diff/120022/webkit/plugins/npapi/plugi... File webkit/plugins/npapi/plugin_list_unittest.cc (right): http://codereview.chromium.org/5699005/diff/120022/webkit/plugins/npapi/plugi... webkit/plugins/npapi/plugin_list_unittest.cc:33: UpdatePluginsEnabledFlags(&plugin_groups_); On 2011/01/24 13:12:21, Bernhard Bauer wrote: > What I meant was actually extracting the part that accesses the disk into a > LoadPluginsInternal (or something) method, and then only overriding that one > here ;-) > > That way you can also make |plugins_loaded_| private again. Done.
LGTM. Thanks! On Monday, January 24, 2011, <pastarmovj@chromium.org> wrote: > Addressed the last comment. Changes again only to plugin_list.*, plugin_gropup.h > and plugin_list_unittest.cc. > > > http://codereview.chromium.org/5699005/diff/120022/webkit/plugins/npapi/plugi... > File webkit/plugins/npapi/plugin_list_unittest.cc (right): > > http://codereview.chromium.org/5699005/diff/120022/webkit/plugins/npapi/plugi... > webkit/plugins/npapi/plugin_list_unittest.cc:33: > UpdatePluginsEnabledFlags(&plugin_groups_); > On 2011/01/24 13:12:21, Bernhard Bauer wrote: > > What I meant was actually extracting the part that accesses the disk > > into a > > LoadPluginsInternal (or something) method, and then only overriding > > that one > > here ;-) > > > > That way you can also make |plugins_loaded_| private again. > > > Done. > > http://codereview.chromium.org/5699005/ > |