Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(582)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 6162008: plugins: drop PluginVersionInfo for internal plugins (Closed)

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.
Visibility:
Public.

Description

plugins: 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+105 lines, -100 lines) Patch
M chrome/browser/chromeos/gview_request_interceptor_unittest.cc View 1 2 3 1 chunk +4 lines, -1 line 0 comments Download
M chrome/browser/plugin_service.cc View 1 2 1 chunk +18 lines, -13 lines 0 comments Download
M webkit/glue/plugins/plugin_list.h View 1 chunk +0 lines, -1 line 0 comments Download
M webkit/plugins/npapi/plugin_lib_win.cc View 1 2 3 1 chunk +19 lines, -10 lines 0 comments Download
M webkit/plugins/npapi/plugin_list.h View 1 2 4 chunks +23 lines, -30 lines 0 comments Download
M webkit/plugins/npapi/plugin_list.cc View 1 2 6 chunks +41 lines, -45 lines 0 comments Download

Messages

Total messages: 8 (0 generated)
Evan Martin
Here is my cleanup change for PluginVersionInfo. I'm still not sure I understand where the ...
9 years, 11 months ago (2011-01-12 20:24:36 UTC) #1
jam
lgtm pastarmovj is refactoring the enabled stuff in a big plugin code refactoring as we ...
9 years, 11 months ago (2011-01-12 21:21:28 UTC) #2
pastarmovj
I am provisional commiter so I can't really LGTM but I am posting three small ...
9 years, 11 months ago (2011-01-13 13:36:08 UTC) #3
pastarmovj
It seems good to me now. BTW I have the same change - removing the ...
9 years, 11 months ago (2011-01-19 10:47:49 UTC) #4
Evan Martin
Should I just hold off and let you do your change? I don't care too ...
9 years, 11 months ago (2011-01-19 17:49:30 UTC) #5
jam
I think Evan's change is still needed, because we want to get rid of PluginVersionInfo, ...
9 years, 11 months ago (2011-01-19 19:25:33 UTC) #6
jam
I think Julian replied to me instead of all, so adding everyone On Wed, Jan ...
9 years, 11 months ago (2011-01-19 20:05:11 UTC) #7
evanm
9 years, 11 months ago (2011-01-19 20:25:06 UTC) #8 
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/
>> >> >
>> >>
>
>

Powered by Google App Engine
This is Rietveld 408576698