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

Issue 5699005: Policy: Re-enabled plugin still disabled (Closed)

Created:
10 years ago by pastarmovj
Modified:
9 years, 7 months ago
Reviewers:
Bernhard Bauer, jam
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)
Visibility:
Public.

Description

Refactor 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. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+930 lines, -486 lines) Patch
M chrome/app/generated_resources.grd View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 1 chunk +0 lines, -3 lines 0 comments Download
M chrome/browser/automation/testing_automation_provider.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 4 chunks +7 lines, -3 lines 0 comments Download
M chrome/browser/chromeos/gview_request_interceptor.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/chromeos/gview_request_interceptor_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 2 chunks +3 lines, -2 lines 0 comments Download
M chrome/browser/dom_ui/plugins_ui.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 2 chunks +1 line, -3 lines 0 comments Download
M chrome/browser/plugin_data_remover.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/plugin_exceptions_table_model_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 2 chunks +4 lines, -2 lines 0 comments Download
M chrome/browser/plugin_service.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 3 chunks +3 lines, -2 lines 0 comments Download
M chrome/browser/plugin_updater.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/plugin_updater.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 4 chunks +26 lines, -16 lines 0 comments Download
M chrome/browser/renderer_host/buffered_resource_handler.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/resources/plugins.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +0 lines, -5 lines 0 comments Download
M chrome/browser/ui/cocoa/table_model_array_controller_unittest.mm View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +6 lines, -3 lines 0 comments Download
M chrome/renderer/render_view.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 2 chunks +2 lines, -2 lines 0 comments Download
M webkit/plugins/npapi/plugin_group.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 8 chunks +56 lines, -14 lines 0 comments Download
M webkit/plugins/npapi/plugin_group.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 8 chunks +189 lines, -73 lines 0 comments Download
M webkit/plugins/npapi/plugin_group_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 6 chunks +13 lines, -28 lines 0 comments Download
M webkit/plugins/npapi/plugin_lib_mac.mm View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 2 chunks +2 lines, -2 lines 0 comments Download
M webkit/plugins/npapi/plugin_lib_posix.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +1 line, -1 line 0 comments Download
M webkit/plugins/npapi/plugin_lib_win.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 1 chunk +1 line, -1 line 0 comments Download
M webkit/plugins/npapi/plugin_list.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 10 chunks +54 lines, -48 lines 0 comments Download
M webkit/plugins/npapi/plugin_list.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 15 chunks +238 lines, -224 lines 0 comments Download
M webkit/plugins/npapi/plugin_list_mac.mm View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 1 chunk +10 lines, -6 lines 0 comments Download
M webkit/plugins/npapi/plugin_list_posix.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 3 chunks +16 lines, -12 lines 0 comments Download
A webkit/plugins/npapi/plugin_list_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 1 chunk +211 lines, -0 lines 0 comments Download
M webkit/plugins/npapi/plugin_list_win.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 6 chunks +37 lines, -25 lines 0 comments Download
M webkit/plugins/npapi/webplugininfo.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 2 chunks +31 lines, -2 lines 0 comments Download
M webkit/plugins/npapi/webplugininfo.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 3 chunks +11 lines, -3 lines 0 comments Download
M webkit/support/webkit_support.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +1 line, -1 line 0 comments Download
M webkit/tools/test_shell/test_shell.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 1 chunk +1 line, -0 lines 0 comments Download
M webkit/tools/test_shell/test_webview_delegate.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 51 (0 generated)
pastarmovj
Please have a look at that. It is by no means 100% done (at least ...
10 years ago (2010-12-10 16:27:51 UTC) #1
Bernhard Bauer
http://codereview.chromium.org/5699005/diff/8002/webkit/glue/plugins/plugin_list.cc File webkit/glue/plugins/plugin_list.cc (right): http://codereview.chromium.org/5699005/diff/8002/webkit/glue/plugins/plugin_list.cc#newcode408 webkit/glue/plugins/plugin_list.cc:408: void PluginList::LoadPlugin(const FilePath& path) { Drive-by: Removing the |plugins| ...
10 years ago (2010-12-14 04:21:48 UTC) #2
danno
here you go... http://codereview.chromium.org/5699005/diff/8002/chrome/browser/chromeos/gview_request_interceptor_unittest.cc File chrome/browser/chromeos/gview_request_interceptor_unittest.cc (right): http://codereview.chromium.org/5699005/diff/8002/chrome/browser/chromeos/gview_request_interceptor_unittest.cc#newcode148 chrome/browser/chromeos/gview_request_interceptor_unittest.cc:148: NPAPI::PluginList::Singleton()->DisablePlugin(pdf_path_, true); Why is this true? ...
10 years ago (2010-12-14 09:43:39 UTC) #3
pastarmovj
At last some working version for review. http://codereview.chromium.org/5699005/diff/8002/chrome/browser/chromeos/gview_request_interceptor_unittest.cc File chrome/browser/chromeos/gview_request_interceptor_unittest.cc (right): http://codereview.chromium.org/5699005/diff/8002/chrome/browser/chromeos/gview_request_interceptor_unittest.cc#newcode148 chrome/browser/chromeos/gview_request_interceptor_unittest.cc:148: NPAPI::PluginList::Singleton()->DisablePlugin(pdf_path_, true); ...
10 years ago (2010-12-15 14:44:51 UTC) #4
Bernhard Bauer
http://codereview.chromium.org/5699005/diff/30002/webkit/glue/plugins/plugin_list.cc File webkit/glue/plugins/plugin_list.cc (right): http://codereview.chromium.org/5699005/diff/30002/webkit/glue/plugins/plugin_list.cc#newcode686 webkit/glue/plugins/plugin_list.cc:686: // callers of |AddToPluginGroups|. Nit: Leftover comment? http://codereview.chromium.org/5699005/diff/30002/webkit/glue/plugins/plugin_list.cc#newcode699 webkit/glue/plugins/plugin_list.cc:699: ...
10 years ago (2010-12-15 17:23:45 UTC) #5
jam
http://codereview.chromium.org/5699005/diff/37001/webkit/glue/plugins/plugin_list.cc File webkit/glue/plugins/plugin_list.cc (right): http://codereview.chromium.org/5699005/diff/37001/webkit/glue/plugins/plugin_list.cc#newcode317 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_list_win.cc File ...
10 years ago (2010-12-15 18:25:50 UTC) #6
jam
http://codereview.chromium.org/5699005/diff/37001/webkit/glue/plugins/plugin_group.cc File webkit/glue/plugins/plugin_group.cc (right): http://codereview.chromium.org/5699005/diff/37001/webkit/glue/plugins/plugin_group.cc#newcode306 webkit/glue/plugins/plugin_group.cc:306: for (std::list<WebPluginInfo>::const_iterator it = web_plugin_infos_.begin(); ditto, using an iterator ...
10 years ago (2010-12-15 19:48:54 UTC) #7
pastarmovj
I only addressed the most important points about design decisions which need further discussion. I ...
10 years ago (2010-12-15 21:56:03 UTC) #8
jam
Based on the first two comments, it seems to me that we should wait and ...
10 years ago (2010-12-15 22:17:57 UTC) #9
pastarmovj
I did the refactoring as we spoke with John yesterday. For my side I am ...
10 years ago (2010-12-17 17:45:48 UTC) #10
Bernhard Bauer
I would really prefer not landing this until we get proper test coverage here, either ...
10 years ago (2010-12-17 18:50:59 UTC) #11
jam
thanks a lot for doing this refactoring :) some comments http://codereview.chromium.org/5699005/diff/66001/chrome/browser/plugin_updater.cc File chrome/browser/plugin_updater.cc (right): http://codereview.chromium.org/5699005/diff/66001/chrome/browser/plugin_updater.cc#newcode284 ...
10 years ago (2010-12-17 19:14:45 UTC) #12
Bernhard Bauer
http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/webplugininfo.h File webkit/glue/plugins/webplugininfo.h (right): http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/webplugininfo.h#newcode92 webkit/glue/plugins/webplugininfo.h:92: int priority; On 2010/12/17 19:14:45, John Abd-El-Malek wrote: > ...
10 years ago (2010-12-17 19:26:39 UTC) #13
pastarmovj
Addressed the comments, fixed some bugs and introduced unit testing on the plugins_list (thanks to ...
10 years ago (2010-12-20 19:57:36 UTC) #14
jam
http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_group.cc File webkit/glue/plugins/plugin_group.cc (right): http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_group.cc#newcode91 webkit/glue/plugins/plugin_group.cc:91: enabled_ = false; On 2010/12/20 19:57:37, pastarmovj wrote: > ...
10 years ago (2010-12-20 20:56:58 UTC) #15
jam
http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/webplugininfo.h File webkit/glue/plugins/webplugininfo.h (right): http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/webplugininfo.h#newcode92 webkit/glue/plugins/webplugininfo.h:92: int priority; On 2010/12/20 19:57:37, pastarmovj wrote: > On ...
10 years ago (2010-12-20 21:01:42 UTC) #16
pastarmovj
http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_group.cc File webkit/glue/plugins/plugin_group.cc (right): http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/plugin_group.cc#newcode91 webkit/glue/plugins/plugin_group.cc:91: enabled_ = false; On 2010/12/20 20:56:59, John Abd-El-Malek wrote: ...
10 years ago (2010-12-20 21:47:21 UTC) #17
Bernhard Bauer
http://codereview.chromium.org/5699005/diff/66001/chrome/browser/plugin_updater.h File chrome/browser/plugin_updater.h (right): http://codereview.chromium.org/5699005/diff/66001/chrome/browser/plugin_updater.h#newcode37 chrome/browser/plugin_updater.h:37: const string16& name); On 2010/12/20 19:57:37, pastarmovj wrote: > ...
10 years ago (2010-12-20 22:30:28 UTC) #18
pastarmovj
Trybots are happy (restarted try jobs to make them visible from the CL page). As ...
10 years ago (2010-12-21 16:53:01 UTC) #19
Bernhard Bauer
http://codereview.chromium.org/5699005/diff/103001/chrome/browser/plugin_updater.h File chrome/browser/plugin_updater.h (right): http://codereview.chromium.org/5699005/diff/103001/chrome/browser/plugin_updater.h#newcode39 chrome/browser/plugin_updater.h:39: // setting the state of a plugin that has ...
10 years ago (2010-12-21 19:07:39 UTC) #20
jam
http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/webplugininfo.h File webkit/glue/plugins/webplugininfo.h (right): http://codereview.chromium.org/5699005/diff/66001/webkit/glue/plugins/webplugininfo.h#newcode30 webkit/glue/plugins/webplugininfo.h:30: struct WebPluginInfo { On 2010/12/20 21:47:22, pastarmovj wrote: > ...
10 years ago (2010-12-21 19:57:41 UTC) #21
pastarmovj
Final call for comments before XMas :). As I am leaving in two days for ...
10 years ago (2010-12-21 20:31:19 UTC) #22
Bernhard Bauer
There are still a couple of unadressed nits. Also, please add unit tests at least ...
10 years ago (2010-12-21 21:36:11 UTC) #23
pastarmovj
@Bernhard : The whole suit of unit tests for PluginList that were in Jakob's commit ...
10 years ago (2010-12-21 21:58:50 UTC) #24
Bernhard Bauer
On Tue, Dec 21, 2010 at 13:58, <pastarmovj@chromium.org> wrote: > @Bernhard : The whole suit ...
10 years ago (2010-12-21 22:22:52 UTC) #25
jam
On Tue, Dec 21, 2010 at 12:31 PM, <pastarmovj@chromium.org> wrote: > Final call for comments ...
10 years ago (2010-12-21 22:38:37 UTC) #26
jam
also, I still believe we really should get rid of priority in this change, it ...
10 years ago (2010-12-21 22:40:11 UTC) #27
pastarmovj
Some more comments here as answers of some earlier general design questions. I kept enough ...
9 years, 12 months ago (2010-12-23 13:00:18 UTC) #28
jam
On Thu, Dec 23, 2010 at 5:00 AM, <pastarmovj@chromium.org> wrote: > Some more comments here ...
9 years, 12 months ago (2010-12-26 22:13:34 UTC) #29
pastarmovj
To shorten the response I only kept a quote of my text (prepended with >), ...
9 years, 11 months ago (2011-01-03 13:44:26 UTC) #30
jam
On Mon, Jan 3, 2011 at 5:44 AM, <pastarmovj@chromium.org> wrote: > To shorten the response ...
9 years, 11 months ago (2011-01-03 18:34:25 UTC) #31
pastarmovj
Please take a look at the last changes. I hope they represent the result of ...
9 years, 11 months ago (2011-01-14 17:32:45 UTC) #32
jam
just looked at the files you mentioned http://codereview.chromium.org/5699005/diff/121001/webkit/plugins/npapi/plugin_group.h File webkit/plugins/npapi/plugin_group.h (right): http://codereview.chromium.org/5699005/diff/121001/webkit/plugins/npapi/plugin_group.h#newcode17 webkit/plugins/npapi/plugin_group.h:17: #include "webkit/plugins/npapi/webplugininfo.h" ...
9 years, 11 months ago (2011-01-15 00:51:34 UTC) #33
pastarmovj
Removed the creation of empty groups in the beginning too. I have one strange failure ...
9 years, 11 months ago (2011-01-18 18:30:39 UTC) #34
jam
http://codereview.chromium.org/5699005/diff/121001/webkit/plugins/npapi/plugin_group.h File webkit/plugins/npapi/plugin_group.h (right): http://codereview.chromium.org/5699005/diff/121001/webkit/plugins/npapi/plugin_group.h#newcode155 webkit/plugins/npapi/plugin_group.h:155: // Check if the group has no plugins or ...
9 years, 11 months ago (2011-01-18 20:26:00 UTC) #35
pastarmovj
There we go. Plugin groups pruning in place too. The funny windows crasher disappeared after ...
9 years, 11 months ago (2011-01-19 17:02:12 UTC) #36
jam
http://codereview.chromium.org/5699005/diff/142001/chrome/browser/automation/testing_automation_provider.cc File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/5699005/diff/142001/chrome/browser/automation/testing_automation_provider.cc#newcode3025 chrome/browser/automation/testing_automation_provider.cc:3025: string16 name; nit: here and below, looks like name ...
9 years, 11 months ago (2011-01-19 20:22:09 UTC) #37
pastarmovj
@John: I tried to address all your comments in this new iteration of the CL. ...
9 years, 11 months ago (2011-01-19 23:39:17 UTC) #38
jam
http://codereview.chromium.org/5699005/diff/142001/chrome/browser/chromeos/gview_request_interceptor_unittest.cc File chrome/browser/chromeos/gview_request_interceptor_unittest.cc (right): http://codereview.chromium.org/5699005/diff/142001/chrome/browser/chromeos/gview_request_interceptor_unittest.cc#newcode72 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: ...
9 years, 11 months ago (2011-01-20 00:24:24 UTC) #39
pastarmovj
I tried to make LoadPlugins thread safe as it used to be before the refactoring. ...
9 years, 11 months ago (2011-01-20 17:15:05 UTC) #40
jam
lgtm with these nits http://codereview.chromium.org/5699005/diff/160001/webkit/plugins/npapi/plugin_group.h File webkit/plugins/npapi/plugin_group.h (right): http://codereview.chromium.org/5699005/diff/160001/webkit/plugins/npapi/plugin_group.h#newcode131 webkit/plugins/npapi/plugin_group.h:131: std::string& identifier() { return identifier_; ...
9 years, 11 months ago (2011-01-20 18:22:33 UTC) #41
pastarmovj
Addressed the last batch of comments. Last open point is http://codereview.chromium.org/5699005/diff/160001/webkit/plugins/npapi/plugin_list.h#newcode174 please take a look ...
9 years, 11 months ago (2011-01-20 22:59:48 UTC) #42
jam
ah, I must have missed the other places the function is used in. lgtm. thanks ...
9 years, 11 months ago (2011-01-20 23:28:40 UTC) #43
pastarmovj
I just uploaded the last version of this CL. The trybots are happy. The valgrind ...
9 years, 11 months ago (2011-01-21 17:03:11 UTC) #44
Bernhard Bauer
Some minor stuff, mostly nits: http://codereview.chromium.org/5699005/diff/178001/webkit/plugins/npapi/plugin_list.cc File webkit/plugins/npapi/plugin_list.cc (right): http://codereview.chromium.org/5699005/diff/178001/webkit/plugins/npapi/plugin_list.cc#newcode746 webkit/plugins/npapi/plugin_list.cc:746: // Non existing plugin ...
9 years, 11 months ago (2011-01-21 17:29:37 UTC) #45
pastarmovj
Addressed comments from Bernhard. Pending trybot happyness. http://codereview.chromium.org/5699005/diff/178001/webkit/plugins/npapi/plugin_list.cc File webkit/plugins/npapi/plugin_list.cc (right): http://codereview.chromium.org/5699005/diff/178001/webkit/plugins/npapi/plugin_list.cc#newcode746 webkit/plugins/npapi/plugin_list.cc:746: // Non ...
9 years, 11 months ago (2011-01-24 10:47:30 UTC) #46
Bernhard Bauer
http://codereview.chromium.org/5699005/diff/138012/webkit/plugins/npapi/plugin_list.cc File webkit/plugins/npapi/plugin_list.cc (right): http://codereview.chromium.org/5699005/diff/138012/webkit/plugins/npapi/plugin_list.cc#newcode746 webkit/plugins/npapi/plugin_list.cc:746: if (plugins_to_disable_[i] == filename) { If you make |plugins_to_disable_| ...
9 years, 11 months ago (2011-01-24 12:20:17 UTC) #47
pastarmovj
Fixed last two comments. Changes only in plugin_list.* and plugin_list_unittest.cc. http://codereview.chromium.org/5699005/diff/138012/webkit/plugins/npapi/plugin_list.cc File webkit/plugins/npapi/plugin_list.cc (right): http://codereview.chromium.org/5699005/diff/138012/webkit/plugins/npapi/plugin_list.cc#newcode746 ...
9 years, 11 months ago (2011-01-24 13:02:10 UTC) #48
Bernhard Bauer
http://codereview.chromium.org/5699005/diff/120022/webkit/plugins/npapi/plugin_list_unittest.cc File webkit/plugins/npapi/plugin_list_unittest.cc (right): http://codereview.chromium.org/5699005/diff/120022/webkit/plugins/npapi/plugin_list_unittest.cc#newcode33 webkit/plugins/npapi/plugin_list_unittest.cc:33: UpdatePluginsEnabledFlags(&plugin_groups_); What I meant was actually extracting the part ...
9 years, 11 months ago (2011-01-24 13:12:20 UTC) #49
pastarmovj
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/plugin_list_unittest.cc File webkit/plugins/npapi/plugin_list_unittest.cc ...
9 years, 11 months ago (2011-01-24 14:47:45 UTC) #50
Bernhard Bauer
9 years, 11 months ago (2011-01-24 15:06:45 UTC) #51
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/
>

Powered by Google App Engine
This is Rietveld 408576698