Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
9 years, 11 months ago by Evan Martin Modified:
9 years, 7 months ago Reviewers:
pastarmovj, jam, evanm CC:
chromium-reviews, darin-cc_chromium.org, stuartmorgan+watch_chromium.org, jam, davemoore+watch_chromium.org, Paweł Hajdan Jr. Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptionplugins: drop PluginVersionInfo for internal plugins
The PluginVersionInfo structure just mirrored the Windows file
metadata format. Since we want to create a WebPluginInfo anyway,
just store a WebPluginInfo as the internal plugin info.
TEST=refactoring change, should be covered by existing tests
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=71841
Patch Set 1 #Patch Set 2 : more comments #
Total comments: 3
Patch Set 3 : fixed #Patch Set 4 : fixes #
Created: 9 years, 11 months ago
Messages
Total messages: 8 (0 generated)
Here is my cleanup change for PluginVersionInfo. I'm still not sure I understand where the "enabled" field is supposed to be initialized. Can you point me to a reviewer who understands that as well?
lgtm pastarmovj is refactoring the enabled stuff in a big plugin code refactoring as we speak, he's probably the one you want to talk to
I am provisional commiter so I can't really LGTM but I am posting three small comments - one is clarification of your question and one is a place in the code that should be fixed or else plugins with undefined enabled state will appear. The last is a nit I think might improve quality of a comment. http://codereview.chromium.org/6162008/diff/3001/webkit/plugins/npapi/plugin_... File webkit/plugins/npapi/plugin_lib_win.cc (right): http://codereview.chromium.org/6162008/diff/3001/webkit/plugins/npapi/plugin_... webkit/plugins/npapi/plugin_lib_win.cc:40: info->enabled = true; The way you set this flag here is correct. The enabled flag should be per default set to true as you had it in PluginList::CreateWebPluginInfo. This is because the code assumes plugins are enabled until the user don't disable them or a policy enforces their disabled state (which happens in the PluginsList::LoadPlugins. This is anyway going to be tweaked by a refactoring plugin I am working on right now but the change shouldn't concern this code. http://codereview.chromium.org/6162008/diff/3001/webkit/plugins/npapi/plugin_... File webkit/plugins/npapi/plugin_list.cc (right): http://codereview.chromium.org/6162008/diff/3001/webkit/plugins/npapi/plugin_... webkit/plugins/npapi/plugin_list.cc:236: plugin.info.desc = ASCIIToUTF16(description); You should be setting the enabled flag to true here as well or else it will be in a random state. http://codereview.chromium.org/6162008/diff/3001/webkit/plugins/npapi/plugin_... File webkit/plugins/npapi/plugin_list.h (right): http://codereview.chromium.org/6162008/diff/3001/webkit/plugins/npapi/plugin_... webkit/plugins/npapi/plugin_list.h:83: // be loaded using PluginList::LoadPlugin(). Maybe fix this comment because it used to cover both versions and now you have comments for each.
It seems good to me now. BTW I have the same change - removing the PluginList::RegisterInternalPlugin(const FilePath&) function and I basically solved the resulting problems the same way as you did so one of us might need to merge into the other's changes but the merge should really be straightforward. But I think this is the right change to do anyways. On 2011/01/13 13:36:08, pastarmovj wrote: > I am provisional commiter so I can't really LGTM but I am posting three small > comments - one is clarification of your question and one is a place in the code > that should be fixed or else plugins with undefined enabled state will appear. > The last is a nit I think might improve quality of a comment. > > http://codereview.chromium.org/6162008/diff/3001/webkit/plugins/npapi/plugin_... > File webkit/plugins/npapi/plugin_lib_win.cc (right): > > http://codereview.chromium.org/6162008/diff/3001/webkit/plugins/npapi/plugin_... > webkit/plugins/npapi/plugin_lib_win.cc:40: info->enabled = true; > The way you set this flag here is correct. > > The enabled flag should be per default set to true as you had it in > PluginList::CreateWebPluginInfo. This is because the code assumes plugins are > enabled until the user don't disable them or a policy enforces their disabled > state (which happens in the PluginsList::LoadPlugins. > > This is anyway going to be tweaked by a refactoring plugin I am working on right > now but the change shouldn't concern this code. > > http://codereview.chromium.org/6162008/diff/3001/webkit/plugins/npapi/plugin_... > File webkit/plugins/npapi/plugin_list.cc (right): > > http://codereview.chromium.org/6162008/diff/3001/webkit/plugins/npapi/plugin_... > webkit/plugins/npapi/plugin_list.cc:236: plugin.info.desc = > ASCIIToUTF16(description); > You should be setting the enabled flag to true here as well or else it will be > in a random state. > > http://codereview.chromium.org/6162008/diff/3001/webkit/plugins/npapi/plugin_... > File webkit/plugins/npapi/plugin_list.h (right): > > http://codereview.chromium.org/6162008/diff/3001/webkit/plugins/npapi/plugin_... > webkit/plugins/npapi/plugin_list.h:83: // be loaded using > PluginList::LoadPlugin(). > Maybe fix this comment because it used to cover both versions and now you have > comments for each.
Should I just hold off and let you do your change? I don't care too much and it seems you know more about this than me. My interest in modifying this code is removing the wstrings, so if you have also removed them then I will just leave it. On Wed, Jan 19, 2011 at 2:47 AM, <pastarmovj@chromium.org> wrote: > It seems good to me now. > > BTW I have the same change - removing the > PluginList::RegisterInternalPlugin(const FilePath&) function and I basically > solved the resulting problems the same way as you did so one of us might > need to > merge into the other's changes but the merge should really be > straightforward. > But I think this is the right change to do anyways. > > On 2011/01/13 13:36:08, pastarmovj wrote: >> >> I am provisional commiter so I can't really LGTM but I am posting three >> small >> comments - one is clarification of your question and one is a place in the > > code >> >> that should be fixed or else plugins with undefined enabled state will >> appear. >> The last is a nit I think might improve quality of a comment. > > > http://codereview.chromium.org/6162008/diff/3001/webkit/plugins/npapi/plugin_... >> >> File webkit/plugins/npapi/plugin_lib_win.cc (right): > > > http://codereview.chromium.org/6162008/diff/3001/webkit/plugins/npapi/plugin_... >> >> webkit/plugins/npapi/plugin_lib_win.cc:40: info->enabled = true; >> The way you set this flag here is correct. > >> The enabled flag should be per default set to true as you had it in >> PluginList::CreateWebPluginInfo. This is because the code assumes plugins >> are >> enabled until the user don't disable them or a policy enforces their >> disabled >> state (which happens in the PluginsList::LoadPlugins. > >> This is anyway going to be tweaked by a refactoring plugin I am working on > > right >> >> now but the change shouldn't concern this code. > > > http://codereview.chromium.org/6162008/diff/3001/webkit/plugins/npapi/plugin_... >> >> File webkit/plugins/npapi/plugin_list.cc (right): > > > http://codereview.chromium.org/6162008/diff/3001/webkit/plugins/npapi/plugin_... >> >> webkit/plugins/npapi/plugin_list.cc:236: plugin.info.desc = >> ASCIIToUTF16(description); >> You should be setting the enabled flag to true here as well or else it >> will be >> in a random state. > > > http://codereview.chromium.org/6162008/diff/3001/webkit/plugins/npapi/plugin_... >> >> File webkit/plugins/npapi/plugin_list.h (right): > > > http://codereview.chromium.org/6162008/diff/3001/webkit/plugins/npapi/plugin_... >> >> webkit/plugins/npapi/plugin_list.h:83: // be loaded using >> PluginList::LoadPlugin(). >> Maybe fix this comment because it used to cover both versions and now you >> have >> comments for each. > > > > http://codereview.chromium.org/6162008/ >
I think Evan's change is still needed, because we want to get rid of PluginVersionInfo, which Julian's change still keeps. On Wed, Jan 19, 2011 at 9:49 AM, Evan Martin <evan@chromium.org> wrote: > Should I just hold off and let you do your change? I don't care too > much and it seems you know more about this than me. My interest in > modifying this code is removing the wstrings, so if you have also > removed them then I will just leave it. > > On Wed, Jan 19, 2011 at 2:47 AM, <pastarmovj@chromium.org> wrote: > > It seems good to me now. > > > > BTW I have the same change - removing the > > PluginList::RegisterInternalPlugin(const FilePath&) function and I > basically > > solved the resulting problems the same way as you did so one of us might > > need to > > merge into the other's changes but the merge should really be > > straightforward. > > But I think this is the right change to do anyways. > > > > On 2011/01/13 13:36:08, pastarmovj wrote: > >> > >> I am provisional commiter so I can't really LGTM but I am posting three > >> small > >> comments - one is clarification of your question and one is a place in > the > > > > code > >> > >> that should be fixed or else plugins with undefined enabled state will > >> appear. > >> The last is a nit I think might improve quality of a comment. > > > > > > > http://codereview.chromium.org/6162008/diff/3001/webkit/plugins/npapi/plugin_... > >> > >> File webkit/plugins/npapi/plugin_lib_win.cc (right): > > > > > > > http://codereview.chromium.org/6162008/diff/3001/webkit/plugins/npapi/plugin_... > >> > >> webkit/plugins/npapi/plugin_lib_win.cc:40: info->enabled = true; > >> The way you set this flag here is correct. > > > >> The enabled flag should be per default set to true as you had it in > >> PluginList::CreateWebPluginInfo. This is because the code assumes > plugins > >> are > >> enabled until the user don't disable them or a policy enforces their > >> disabled > >> state (which happens in the PluginsList::LoadPlugins. > > > >> This is anyway going to be tweaked by a refactoring plugin I am working > on > > > > right > >> > >> now but the change shouldn't concern this code. > > > > > > > http://codereview.chromium.org/6162008/diff/3001/webkit/plugins/npapi/plugin_... > >> > >> File webkit/plugins/npapi/plugin_list.cc (right): > > > > > > > http://codereview.chromium.org/6162008/diff/3001/webkit/plugins/npapi/plugin_... > >> > >> webkit/plugins/npapi/plugin_list.cc:236: plugin.info.desc = > >> ASCIIToUTF16(description); > >> You should be setting the enabled flag to true here as well or else it > >> will be > >> in a random state. > > > > > > > http://codereview.chromium.org/6162008/diff/3001/webkit/plugins/npapi/plugin_... > >> > >> File webkit/plugins/npapi/plugin_list.h (right): > > > > > > > http://codereview.chromium.org/6162008/diff/3001/webkit/plugins/npapi/plugin_... > >> > >> webkit/plugins/npapi/plugin_list.h:83: // be loaded using > >> PluginList::LoadPlugin(). > >> Maybe fix this comment because it used to cover both versions and now > you > >> have > >> comments for each. > > > > > > > > http://codereview.chromium.org/6162008/ > > >
I think Julian replied to me instead of all, so adding everyone On Wed, Jan 19, 2011 at 11:30 AM, Julian Pastarmov <pastarmovj@google.com>wrote: > I agree don't hesitate to commit whenever lgtm-ed. I like this cl too it is > a move in the right direction. > On Jan 19, 2011 8:25 PM, "John Abd-El-Malek" <jam@chromium.org> wrote: > > I think Evan's change is still needed, because we want to get rid > > of PluginVersionInfo, which Julian's change still keeps. > > > > On Wed, Jan 19, 2011 at 9:49 AM, Evan Martin <evan@chromium.org> wrote: > > > >> Should I just hold off and let you do your change? I don't care too > >> much and it seems you know more about this than me. My interest in > >> modifying this code is removing the wstrings, so if you have also > >> removed them then I will just leave it. > >> > >> On Wed, Jan 19, 2011 at 2:47 AM, <pastarmovj@chromium.org> wrote: > >> > It seems good to me now. > >> > > >> > BTW I have the same change - removing the > >> > PluginList::RegisterInternalPlugin(const FilePath&) function and I > >> basically > >> > solved the resulting problems the same way as you did so one of us > might > >> > need to > >> > merge into the other's changes but the merge should really be > >> > straightforward. > >> > But I think this is the right change to do anyways. > >> > > >> > On 2011/01/13 13:36:08, pastarmovj wrote: > >> >> > >> >> I am provisional commiter so I can't really LGTM but I am posting > three > >> >> small > >> >> comments - one is clarification of your question and one is a place > in > >> the > >> > > >> > code > >> >> > >> >> that should be fixed or else plugins with undefined enabled state > will > >> >> appear. > >> >> The last is a nit I think might improve quality of a comment. > >> > > >> > > >> > > >> > http://codereview.chromium.org/6162008/diff/3001/webkit/plugins/npapi/plugin_... > >> >> > >> >> File webkit/plugins/npapi/plugin_lib_win.cc (right): > >> > > >> > > >> > > >> > http://codereview.chromium.org/6162008/diff/3001/webkit/plugins/npapi/plugin_... > >> >> > >> >> webkit/plugins/npapi/plugin_lib_win.cc:40: info->enabled = true; > >> >> The way you set this flag here is correct. > >> > > >> >> The enabled flag should be per default set to true as you had it in > >> >> PluginList::CreateWebPluginInfo. This is because the code assumes > >> plugins > >> >> are > >> >> enabled until the user don't disable them or a policy enforces their > >> >> disabled > >> >> state (which happens in the PluginsList::LoadPlugins. > >> > > >> >> This is anyway going to be tweaked by a refactoring plugin I am > working > >> on > >> > > >> > right > >> >> > >> >> now but the change shouldn't concern this code. > >> > > >> > > >> > > >> > http://codereview.chromium.org/6162008/diff/3001/webkit/plugins/npapi/plugin_... > >> >> > >> >> File webkit/plugins/npapi/plugin_list.cc (right): > >> > > >> > > >> > > >> > http://codereview.chromium.org/6162008/diff/3001/webkit/plugins/npapi/plugin_... > >> >> > >> >> webkit/plugins/npapi/plugin_list.cc:236: plugin.info.desc = > >> >> ASCIIToUTF16(description); > >> >> You should be setting the enabled flag to true here as well or else > it > >> >> will be > >> >> in a random state. > >> > > >> > > >> > > >> > http://codereview.chromium.org/6162008/diff/3001/webkit/plugins/npapi/plugin_... > >> >> > >> >> File webkit/plugins/npapi/plugin_list.h (right): > >> > > >> > > >> > > >> > http://codereview.chromium.org/6162008/diff/3001/webkit/plugins/npapi/plugin_... > >> >> > >> >> webkit/plugins/npapi/plugin_list.h:83: // be loaded using > >> >> PluginList::LoadPlugin(). > >> >> Maybe fix this comment because it used to cover both versions and now > >> you > >> >> have > >> >> comments for each. > >> > > >> > > >> > > >> > http://codereview.chromium.org/6162008/ > >> > > >> >
Just waiting for green trybots... On Wed, Jan 19, 2011 at 12:05 PM, John Abd-El-Malek <jam@chromium.org> wrote: > I think Julian replied to me instead of all, so adding everyone > > On Wed, Jan 19, 2011 at 11:30 AM, Julian Pastarmov <pastarmovj@google.com> > wrote: >> >> I agree don't hesitate to commit whenever lgtm-ed. I like this cl too it >> is a move in the right direction. >> >> On Jan 19, 2011 8:25 PM, "John Abd-El-Malek" <jam@chromium.org> wrote: >> > I think Evan's change is still needed, because we want to get rid >> > of PluginVersionInfo, which Julian's change still keeps. >> > >> > On Wed, Jan 19, 2011 at 9:49 AM, Evan Martin <evan@chromium.org> wrote: >> > >> >> Should I just hold off and let you do your change? I don't care too >> >> much and it seems you know more about this than me. My interest in >> >> modifying this code is removing the wstrings, so if you have also >> >> removed them then I will just leave it. >> >> >> >> On Wed, Jan 19, 2011 at 2:47 AM, <pastarmovj@chromium.org> wrote: >> >> > It seems good to me now. >> >> > >> >> > BTW I have the same change - removing the >> >> > PluginList::RegisterInternalPlugin(const FilePath&) function and I >> >> basically >> >> > solved the resulting problems the same way as you did so one of us >> >> > might >> >> > need to >> >> > merge into the other's changes but the merge should really be >> >> > straightforward. >> >> > But I think this is the right change to do anyways. >> >> > >> >> > On 2011/01/13 13:36:08, pastarmovj wrote: >> >> >> >> >> >> I am provisional commiter so I can't really LGTM but I am posting >> >> >> three >> >> >> small >> >> >> comments - one is clarification of your question and one is a place >> >> >> in >> >> the >> >> > >> >> > code >> >> >> >> >> >> that should be fixed or else plugins with undefined enabled state >> >> >> will >> >> >> appear. >> >> >> The last is a nit I think might improve quality of a comment. >> >> > >> >> > >> >> > >> >> >> >> http://codereview.chromium.org/6162008/diff/3001/webkit/plugins/npapi/plugin_... >> >> >> >> >> >> File webkit/plugins/npapi/plugin_lib_win.cc (right): >> >> > >> >> > >> >> > >> >> >> >> http://codereview.chromium.org/6162008/diff/3001/webkit/plugins/npapi/plugin_... >> >> >> >> >> >> webkit/plugins/npapi/plugin_lib_win.cc:40: info->enabled = true; >> >> >> The way you set this flag here is correct. >> >> > >> >> >> The enabled flag should be per default set to true as you had it in >> >> >> PluginList::CreateWebPluginInfo. This is because the code assumes >> >> plugins >> >> >> are >> >> >> enabled until the user don't disable them or a policy enforces their >> >> >> disabled >> >> >> state (which happens in the PluginsList::LoadPlugins. >> >> > >> >> >> This is anyway going to be tweaked by a refactoring plugin I am >> >> >> working >> >> on >> >> > >> >> > right >> >> >> >> >> >> now but the change shouldn't concern this code. >> >> > >> >> > >> >> > >> >> >> >> http://codereview.chromium.org/6162008/diff/3001/webkit/plugins/npapi/plugin_... >> >> >> >> >> >> File webkit/plugins/npapi/plugin_list.cc (right): >> >> > >> >> > >> >> > >> >> >> >> http://codereview.chromium.org/6162008/diff/3001/webkit/plugins/npapi/plugin_... >> >> >> >> >> >> webkit/plugins/npapi/plugin_list.cc:236: plugin.info.desc = >> >> >> ASCIIToUTF16(description); >> >> >> You should be setting the enabled flag to true here as well or else >> >> >> it >> >> >> will be >> >> >> in a random state. >> >> > >> >> > >> >> > >> >> >> >> http://codereview.chromium.org/6162008/diff/3001/webkit/plugins/npapi/plugin_... >> >> >> >> >> >> File webkit/plugins/npapi/plugin_list.h (right): >> >> > >> >> > >> >> > >> >> >> >> http://codereview.chromium.org/6162008/diff/3001/webkit/plugins/npapi/plugin_... >> >> >> >> >> >> webkit/plugins/npapi/plugin_list.h:83: // be loaded using >> >> >> PluginList::LoadPlugin(). >> >> >> Maybe fix this comment because it used to cover both versions and >> >> >> now >> >> you >> >> >> have >> >> >> comments for each. >> >> > >> >> > >> >> > >> >> > http://codereview.chromium.org/6162008/ >> >> > >> >> > > |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
