Content settings extension API: Implement ContentSetting.getResourceIdentifiers.

Content settings extension API: Implement ContentSetting.getResourceIdentifiers.

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

[Bernhard Bauer, 2011-06-14 15:38:18]

Please review:

Matt: Extension API
John: webkit/plugin changes
Tony: test_shell gyp change
Dominic: Rest :-)

[battre, 2011-06-14 16:31:01]

LGTM from my side.

http://codereview.chromium.org/7041005/diff/4018/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_content_settings_api.h (right):

http://codereview.chromium.org/7041005/diff/4018/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_content_settings_api.h:50: void
GetPluginsOnFileThread();
We now have a SyncIOThreadExtensionFunction and could add a
ASyncIOThreadExtensionFunction. Matt, WDYT? Should we skip the UI thread
completely?

http://codereview.chromium.org/7041005/diff/4018/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_content_settings_apitest.cc (right):

http://codereview.chromium.org/7041005/diff/4018/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_content_settings_apitest.cc:201:
webkit::npapi::WebPluginInfo(ASCIIToUTF16("Bar Plugin"),
nit: Why do you use a constant only for the path but not for the name? You do it
differently above.

http://codereview.chromium.org/7041005/diff/4018/chrome/common/extensions/api...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/7041005/diff/4018/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.json:5873: "description": "A list of
resource identifiers for this content type, or <var>undefined</var> if this
content type does not use resource identifiers",
nit: end with "."?

http://codereview.chromium.org/7041005/diff/4018/chrome/renderer/resources/ex...
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/7041005/diff/4018/chrome/renderer/resources/ex...
chrome/renderer/resources/extension_process_bindings.js:410:
this.parameters.getResourceIdentifiers;
nit: single line?

[jam, 2011-06-14 16:39:56]

http://codereview.chromium.org/7041005/diff/4018/webkit/plugins/npapi/stub_pl...
File webkit/plugins/npapi/stub_plugin_list.h (right):

http://codereview.chromium.org/7041005/diff/4018/webkit/plugins/npapi/stub_pl...
webkit/plugins/npapi/stub_plugin_list.h:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
nit: i think MockPluginList would be more consistent with some of the other
similar class, i.e. look at all the mock objects in chrome\renderer

[tony, 2011-06-14 17:46:56]

http://codereview.chromium.org/7041005/diff/4018/webkit/tools/test_shell/test...
File webkit/tools/test_shell/test_shell.gypi (right):

http://codereview.chromium.org/7041005/diff/4018/webkit/tools/test_shell/test...
webkit/tools/test_shell/test_shell.gypi:343:
'../../plugins/npapi/stub_plugin_list.h',
Can we just include these files in test_shell_tests? Also, maybe the files
should live in src/webkit/mocks with the other mocks.

[Matt Perry, 2011-06-14 19:19:34]

LGTM

http://codereview.chromium.org/7041005/diff/4018/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_content_settings_api.h (right):

http://codereview.chromium.org/7041005/diff/4018/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_content_settings_api.h:50: void
GetPluginsOnFileThread();
On 2011/06/14 16:31:01, battre wrote:
> We now have a SyncIOThreadExtensionFunction and could add a
> ASyncIOThreadExtensionFunction. Matt, WDYT? Should we skip the UI thread
> completely?

The IO thread is different from the FILE thread. IO handles IPC messages and
network resources.

We should stick with the standard extension function dispatching unless there's
a compelling performance reason to handle them on the IO thread.

http://codereview.chromium.org/7041005/diff/4018/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_content_settings_api.h:53: static void
SetPluginList(webkit::npapi::PluginList* plugin_list);
maybe add ForTesting to the name

[Bernhard Bauer, 2011-06-15 10:04:57]

http://codereview.chromium.org/7041005/diff/4018/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_content_settings_api.h (right):

http://codereview.chromium.org/7041005/diff/4018/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_content_settings_api.h:53: static void
SetPluginList(webkit::npapi::PluginList* plugin_list);
On 2011/06/14 19:19:34, Matt Perry wrote:
> maybe add ForTesting to the name

Done.

http://codereview.chromium.org/7041005/diff/4018/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_content_settings_apitest.cc (right):

http://codereview.chromium.org/7041005/diff/4018/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_content_settings_apitest.cc:201:
webkit::npapi::WebPluginInfo(ASCIIToUTF16("Bar Plugin"),
On 2011/06/14 16:31:01, battre wrote:
> nit: Why do you use a constant only for the path but not for the name? You do
it
> differently above.

I tried to clean it up a bit.

http://codereview.chromium.org/7041005/diff/4018/chrome/common/extensions/api...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/7041005/diff/4018/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.json:5873: "description": "A list of
resource identifiers for this content type, or <var>undefined</var> if this
content type does not use resource identifiers",
On 2011/06/14 16:31:01, battre wrote:
> nit: end with "."?

Done.

http://codereview.chromium.org/7041005/diff/4018/chrome/renderer/resources/ex...
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/7041005/diff/4018/chrome/renderer/resources/ex...
chrome/renderer/resources/extension_process_bindings.js:410:
this.parameters.getResourceIdentifiers;
On 2011/06/14 16:31:01, battre wrote:
> nit: single line?

Done.

http://codereview.chromium.org/7041005/diff/4018/webkit/tools/test_shell/test...
File webkit/tools/test_shell/test_shell.gypi (right):

http://codereview.chromium.org/7041005/diff/4018/webkit/tools/test_shell/test...
webkit/tools/test_shell/test_shell.gypi:343:
'../../plugins/npapi/stub_plugin_list.h',
On 2011/06/14 17:46:56, tony wrote:
> Can we just include these files in test_shell_tests? 

I need them in browser_tests as well, and I didn't want those to depend on all
of test_shell_tests.

> Also, maybe the files should live in src/webkit/mocks with the other mocks.

It looks like the classes in webkit/mocks are mocks for WebKit interfaces,
whereas mocks for webkit glue classes live next to the real classes (like in
webkit/appcache and webkit/quota).

[battre, 2011-06-15 12:53:39]

still LGTM

http://codereview.chromium.org/7041005/diff/16001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_content_settings_api.cc (right):

http://codereview.chromium.org/7041005/diff/16001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_content_settings_api.cc:32:
webkit::npapi::PluginList* g_plugin_list = NULL;
should this now be called g_plugin_list_for_testing? I don't have a strong
opinion.

http://codereview.chromium.org/7041005/diff/16001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_content_settings_apitest.cc (right):

http://codereview.chromium.org/7041005/diff/16001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_content_settings_apitest.cc:189:
"http://example.com/foo" },
nit: single line?

[tony, 2011-06-15 16:52:55]

LGTM

On 2011/06/15 10:04:57, Bernhard Bauer wrote:
> On 2011/06/14 17:46:56, tony wrote:
> > Can we just include these files in test_shell_tests? 
> 
> I need them in browser_tests as well, and I didn't want those to depend on all
> of test_shell_tests.

Sorry, I didn't see the dependency in browser_tests.gypi.  You're right, this is
probably the best way to do this.  I feel bad for the win devs that have lots of
little .vcproj files to open.

[commit-bot: I haz the power, 2011-06-15 18:41:58]

Change committed as 89226