Add sample extension that allows setting plugin-specific content settings.

Add sample extension that allows setting plugin-specific content settings.

BUG=64155
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=111352

[Bernhard Bauer, 2011-10-25 16:53:00]

Hi Mike, 

if you have some time on your hands (you don't need sleep, right?), could you
review this extension?

The interesting parts are in the top-level js/ and css/ directory, the code in
domui/ and and options/ is from other places in chrome/browser/resources.

Ben, James: FYI; Comments welcome.

[Mike West, 2011-10-25 19:52:06]

First batch of comments in. I'll sift through the other few thousand lines
tomorrow morning. :)

For a first pass, I think pulling in all of the DOMUI code is fine. It's faster
to develop with, and it's valuable to get a demo out for developers to play
with. That said, it's _much_ more complex than it probably needs to be for the
functionality you're actually building. Maybe we can spend some time later in
the quarter to rip some of the code out and to simplify. :)

http://codereview.chromium.org/8396001/diff/5001/chrome/common/extensions/doc...
File
chrome/common/extensions/docs/examples/extensions/plugin_settings/js/main.js
(right):

http://codereview.chromium.org/8396001/diff/5001/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/extensions/plugin_settings/js/main.js:10:
return;
This means that if the extension ever errors out, the user won't be able to fix
it (as the plugin list is never initialized, etc.). Is that intentional?

It would probably be a good idea to fiddle with this such that the error message
is displayed, but the options page is still functional, which might be as simple
as dropping the `return`.

http://codereview.chromium.org/8396001/diff/5001/chrome/common/extensions/doc...
File
chrome/common/extensions/docs/examples/extensions/plugin_settings/js/plugin_settings.js
(right):

http://codereview.chromium.org/8396001/diff/5001/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/extensions/plugin_settings/js/plugin_settings.js:25:
* @type {string}
`@type {?string}` since it's apparently nullable.

http://codereview.chromium.org/8396001/diff/5001/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/extensions/plugin_settings/js/plugin_settings.js:46:
* @param {function()} callback Called when the content settings have been
`@param {function}`

http://codereview.chromium.org/8396001/diff/5001/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/extensions/plugin_settings/js/plugin_settings.js:47:
*     recreated.
(or upon error)

http://codereview.chromium.org/8396001/diff/5001/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/extensions/plugin_settings/js/plugin_settings.js:63:
for (var i = 0; i < length; i++) {
Where is `length` defined? Maybe copy/paste from #197?

http://codereview.chromium.org/8396001/diff/5001/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/extensions/plugin_settings/js/plugin_settings.js:69:
chrome.contentSettings.plugins.set({
I think you might be able to replace this block with a call/callback to
`setInternal_` to remove some duplication.

http://codereview.chromium.org/8396001/diff/5001/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/extensions/plugin_settings/js/plugin_settings.js:78:
window.localStorage.removeItem(key);
Might be worth noting in the function's docblock that localStorage entries which
error off will be removed entirely.

http://codereview.chromium.org/8396001/diff/5001/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/extensions/plugin_settings/js/plugin_settings.js:167:
// If setting the new rule failed, restore the old rule.
I like this. Nice thought.

http://codereview.chromium.org/8396001/diff/5001/chrome/common/extensions/doc...
File
chrome/common/extensions/docs/examples/extensions/plugin_settings/manifest.json
(right):

http://codereview.chromium.org/8396001/diff/5001/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/extensions/plugin_settings/manifest.json:2:
"name" : "Per-plugin content settings",
Might as well localize the name and description, since you've already got a `
_locale` directory with an English message file, and `default_locale` set.

http://codereview.chromium.org/8396001/diff/5001/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/extensions/plugin_settings/manifest.json:14:
"content_security_policy": "default-src 'self' ",
You used `content_security_policy`! Hooray you!

I suspect you're neither using Flash at the moment, nor planning to.
`default-src 'none'; script-src 'self'; style-src 'self'` might be more
appropriate. It would, for example, prevent EVIL fonts from being downloaded.
Like Comic Sans.

[Bernhard Bauer, 2011-10-26 13:55:18]

http://codereview.chromium.org/8396001/diff/5001/chrome/common/extensions/doc...
File
chrome/common/extensions/docs/examples/extensions/plugin_settings/js/main.js
(right):

http://codereview.chromium.org/8396001/diff/5001/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/extensions/plugin_settings/js/main.js:10:
return;
On 2011/10/25 19:52:06, Mike West (chromium) wrote:
> This means that if the extension ever errors out, the user won't be able to
fix
> it (as the plugin list is never initialized, etc.). Is that intentional?

I'm not sure there would be much to fix in that case ;-) All the options page
displays is a list of plug-ins with rules for them, so if we don't get a list of
plug-ins, we wouldn't have anything to show anyway.

> It would probably be a good idea to fiddle with this such that the error
message
> is displayed, but the options page is still functional, which might be as
simple
> as dropping the `return`.

http://codereview.chromium.org/8396001/diff/5001/chrome/common/extensions/doc...
File
chrome/common/extensions/docs/examples/extensions/plugin_settings/js/plugin_settings.js
(right):

http://codereview.chromium.org/8396001/diff/5001/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/extensions/plugin_settings/js/plugin_settings.js:25:
* @type {string}
On 2011/10/25 19:52:06, Mike West (chromium) wrote:
> `@type {?string}` since it's apparently nullable.

Hm, once it's been initialized, it's not nullable anymore. I think the
initialization to null is just to have something to add JSDoc comments to?

http://codereview.chromium.org/8396001/diff/5001/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/extensions/plugin_settings/js/plugin_settings.js:46:
* @param {function()} callback Called when the content settings have been
On 2011/10/25 19:52:06, Mike West (chromium) wrote:
> `@param {function}`

Is there another way to document that the callback takes no arguments? Or do we
not do that?

http://codereview.chromium.org/8396001/diff/5001/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/extensions/plugin_settings/js/plugin_settings.js:47:
*     recreated.
On 2011/10/25 19:52:06, Mike West (chromium) wrote:
> (or upon error)

Done.

http://codereview.chromium.org/8396001/diff/5001/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/extensions/plugin_settings/js/plugin_settings.js:63:
for (var i = 0; i < length; i++) {
On 2011/10/25 19:52:06, Mike West (chromium) wrote:
> Where is `length` defined? Maybe copy/paste from #197?

Oops. Fixed.

http://codereview.chromium.org/8396001/diff/5001/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/extensions/plugin_settings/js/plugin_settings.js:69:
chrome.contentSettings.plugins.set({
On 2011/10/25 19:52:06, Mike West (chromium) wrote:
> I think you might be able to replace this block with a call/callback to
> `setInternal_` to remove some duplication.

Hm, but setInternal also updates local storage, which we don't want to do here.

http://codereview.chromium.org/8396001/diff/5001/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/extensions/plugin_settings/js/plugin_settings.js:78:
window.localStorage.removeItem(key);
On 2011/10/25 19:52:06, Mike West (chromium) wrote:
> Might be worth noting in the function's docblock that localStorage entries
which
> error off will be removed entirely.

Done.

http://codereview.chromium.org/8396001/diff/5001/chrome/common/extensions/doc...
File
chrome/common/extensions/docs/examples/extensions/plugin_settings/manifest.json
(right):

http://codereview.chromium.org/8396001/diff/5001/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/extensions/plugin_settings/manifest.json:2:
"name" : "Per-plugin content settings",
On 2011/10/25 19:52:06, Mike West (chromium) wrote:
> Might as well localize the name and description, since you've already got a `
> _locale` directory with an English message file, and `default_locale` set.

Done.

http://codereview.chromium.org/8396001/diff/5001/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/extensions/plugin_settings/manifest.json:14:
"content_security_policy": "default-src 'self' ",
On 2011/10/25 19:52:06, Mike West (chromium) wrote:
> You used `content_security_policy`! Hooray you!
> 
> I suspect you're neither using Flash at the moment, nor planning to.
> `default-src 'none'; script-src 'self'; style-src 'self'` might be more
> appropriate. It would, for example, prevent EVIL fonts from being downloaded.
> Like Comic Sans.

Done, but I also added img-src 'self' for the "X" to delete rules.

[Bernhard Bauer, 2011-11-23 12:41:59]

Hi Mike, 

as GDD is over now and I'm sure you have *so much* free time on your hands,
could you take another look?

Thanks!

[Mike West, 2011-11-23 12:56:46]

Changes LGTM, with one stupid nit. :)

http://codereview.chromium.org/8396001/diff/8001/chrome/common/extensions/doc...
File
chrome/common/extensions/docs/examples/extensions/plugin_settings/_locales/en/messages.json
(right):

http://codereview.chromium.org/8396001/diff/8001/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/extensions/plugin_settings/_locales/en/messages.json:6:
"message": "Customize your content setting for different plug-ins."
Nit: settingS.

[Bernhard Bauer, 2011-11-23 15:17:12]

http://codereview.chromium.org/8396001/diff/8001/chrome/common/extensions/doc...
File
chrome/common/extensions/docs/examples/extensions/plugin_settings/_locales/en/messages.json
(right):

http://codereview.chromium.org/8396001/diff/8001/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/extensions/plugin_settings/_locales/en/messages.json:6:
"message": "Customize your content setting for different plug-ins."
On 2011/11/23 12:56:46, Mike West (chromium) wrote:
> Nit: settingS.

Done.