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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 1085003: Implement chrome://plugins page that can disable plugins. (Closed)

Created:
10 years, 9 months ago by viettrungluu
Modified:
9 years, 6 months ago
CC:
chromium-reviews, jam+cc_chromium.org, brettw+cc_chromium.org, ben+cc_chromium.org, John Grabowski, jam, darin-cc_chromium.org, pam+watch_chromium.org, arv (Not doing code reviews), brettw-cc_chromium.org
Visibility:
Public.

Description

Implement chrome://plugins page that can disable plugins. BUG=736 TEST=Go to chrome://plugins/. Should be able to enable/disable plugins. Enabled/disabled plugins should persist between sessions. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=42412

Patch Set 1 #

Patch Set 2 : stooopid whitespaces #

Patch Set 3 : prototype prefs code #

Patch Set 4 : foobar #

Patch Set 5 : fix on Windows? #

Patch Set 6 : work in progress; plugins.html still needs more love #

Patch Set 7 : Cleaned up stuff. #

Patch Set 8 : oops #

Patch Set 9 : We don't need a plugins_section.png if this'll be merged with the Extensions page eventually. #

Patch Set 10 : foo #

Total comments: 34

Patch Set 11 : string type fix #

Patch Set 12 : added bug number #

Patch Set 13 : merged ToT #

Patch Set 14 : merge ToT again #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1080 lines, -25 lines) Patch
M chrome/app/generated_resources.grd View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +41 lines, -0 lines 0 comments Download
M chrome/browser/browser_main.cc View 3 4 5 6 7 8 9 10 11 12 13 3 chunks +3 lines, -3 lines 0 comments Download
M chrome/browser/browser_prefs.cc View 3 4 5 6 2 chunks +2 lines, -0 lines 0 comments Download
M chrome/browser/browser_resources.grd View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/dom_ui/dom_ui_factory.cc View 1 2 3 4 5 4 chunks +7 lines, -1 line 0 comments Download
A chrome/browser/dom_ui/plugins_ui.h View 1 2 1 chunk +24 lines, -0 lines 0 comments Download
A chrome/browser/dom_ui/plugins_ui.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +276 lines, -0 lines 0 comments Download
M chrome/browser/plugin_service.h View 3 chunks +6 lines, -1 line 0 comments Download
M chrome/browser/plugin_service.cc View 7 8 9 10 11 4 chunks +33 lines, -1 line 0 comments Download
M chrome/browser/renderer_host/resource_message_filter.cc View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
A chrome/browser/resources/plugins.html View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +558 lines, -0 lines 0 comments Download
M chrome/chrome_browser.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/common/pref_names.h View 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/pref_names.cc View 3 4 5 6 3 chunks +15 lines, -14 lines 0 comments Download
M chrome/common/url_constants.h View 3 chunks +3 lines, -1 line 0 comments Download
M chrome/common/url_constants.cc View 3 chunks +3 lines, -1 line 0 comments Download
M webkit/glue/plugins/plugin_lib_mac.mm View 2 chunks +2 lines, -0 lines 0 comments Download
M webkit/glue/plugins/plugin_lib_posix.cc View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M webkit/glue/plugins/plugin_list.h View 1 2 3 4 5 6 4 chunks +21 lines, -1 line 0 comments Download
M webkit/glue/plugins/plugin_list.cc View 1 2 3 4 5 6 7 8 9 5 chunks +77 lines, -1 line 0 comments Download
M webkit/glue/webplugininfo.h View 1 chunk +3 lines, -0 lines 0 comments Download

Messages

Total messages: 6 (0 generated)
viettrungluu
@arv: Could you please review the DOM UI parts? @jam: Could you please review the ...
10 years, 9 months ago (2010-03-23 17:20:12 UTC) #1
arv (Not doing code reviews)
I made some comments on the HTML. They are mostly nits. I assume you copied ...
10 years, 9 months ago (2010-03-23 18:03:12 UTC) #2
jam
Has this gone through UI review or did you sync up with chrome-ui-leads? The last ...
10 years, 9 months ago (2010-03-23 18:33:38 UTC) #3
jam
lgtm
10 years, 9 months ago (2010-03-23 19:22:45 UTC) #4
viettrungluu
Thanks all. @arv: All done (or punted on). Do you want to take another look? ...
10 years, 9 months ago (2010-03-23 22:42:36 UTC) #5
arv (Not doing code reviews)
10 years, 9 months ago (2010-03-24 00:11:38 UTC) #6 
LGTM

On Mar 23, 2010 3:42 PM, <viettrungluu@chromium.org> wrote:

Thanks all.

@arv: All done (or punted on). Do you want to take another look?




http://codereview.chromium.org/1085003/diff/20001/21006
File chrome/browser/dom_ui/plugins_ui.cc ...

On 2010/03/23 18:03:12, arv wrote:
>
> Can you put the bug number here?

Done.



http://codereview.chromium.org/1085003/diff/20001/21006#newcode146
chrome/browser/dom_ui/plugins_u...

On 2010/03/23 18:03:12, arv wrote:
>
> Can you return a ListValue instead?

I could just return plugins_list (below), but the thought is that the
return value should eventually (soon) contain some more global data
about the plugins state, e.g., if they're enabled/disabled globally from
the Content Settings dialog.



http://codereview.chromium.org/1085003/diff/20001/21006#newcode170
chrome/browser/dom_ui/plugins_u...

On 2010/03/23 18:03:12, arv wrote:
>
> Can you do GetBoolean here and pass a boolean instead?

Correct me if I'm wrong, but chrome.send() really wants to only send an
array of strings.



http://codereview.chromium.org/1085003/diff/20001/21006#newcode176
chrome/browser/dom_ui/plugins_u...
On 2010/03/23 18:03:12, arv wrote:

> It is fairly what?
>

No idea. Comment changed (and bug number added).



http://codereview.chromium.org/1085003/diff/20001/21008
File chrome/browser/plugin_service.cc (rig...

On 2010/03/23 18:03:12, arv wrote:
>
> The .h file said this needs to be called on the main thread. ...
Done.



http://codereview.chromium.org/1085003/diff/20001/21011
File chrome/browser/resources/plugins.html...

On 2010/03/23 18:03:12, arv wrote:
>
> There should be an empty line between each rule

Oops. Fixed.



http://codereview.chromium.org/1085003/diff/20001/21011#newcode251
chrome/browser/resources/plugin...

On 2010/03/23 18:03:12, arv wrote:
>
> Prefer single quotes over double quotes in JS.

Done.



http://codereview.chromium.org/1085003/diff/20001/21011#newcode310
chrome/browser/resources/plugin...

On 2010/03/23 18:03:12, arv wrote:
>
> or


> var body = document.body;

Done.



http://codereview.chromium.org/1085003/diff/20001/21011#newcode313
chrome/browser/resources/plugin...
On 2010/03/23 18:03:12, arv wrote:

> Use single quotes
>

Done.



http://codereview.chromium.org/1085003/diff/20001/21011#newcode371
chrome/browser/resources/plugin...
On 2010/03/23 18:03:12, arv wrote:

> document.body
>

Done.



http://codereview.chromium.org/1085003/diff/20001/21011#newcode408
chrome/browser/resources/plugin...

On 2010/03/23 18:03:12, arv wrote:
>
> move onload to JS


> window.onload = requestPluginsData;

Done.



http://codereview.chromium.org/1085003/diff/20001/21011#newcode417
chrome/browser/resources/plugin...

On 2010/03/23 18:03:12, arv wrote:
>
> Can you remove the style attribute and add that info as a rul...
I did some of this; I'll punt on the rest and leave it to be taken care
en masse with our other DOM UI pages (possibly by feldstein).



http://codereview.chromium.org/1085003/diff/20001/21011#newcode429
chrome/browser/resources/plugin...
On 2010/03/23 18:03:12, arv wrote:

> invalid attribute padding
>

Done.



http://codereview.chromium.org/1085003/diff/20001/21011#newcode434
chrome/browser/resources/plugin...
On 2010/03/23 18:03:12, arv wrote:

> HTML should not have />
>

Done.



http://codereview.chromium.org/1085003/diff/20001/21011#newcode475
chrome/browser/resources/plugin...

On 2010/03/23 18:03:12, arv wrote:
>
> Does the description have HTML in it?

Yes. :-(



http://codereview.chromium.org/1085003/diff/20001/21011#newcode517
chrome/browser/resources/plugin...

On 2010/03/23 18:03:12, arv wrote:
>
> This is bad accessibility. We should use a styled button inst...
I'll punt on this too.



http://codereview.chromium.org/1085003

Powered by Google App Engine
This is Rietveld 408576698