Add "sidebar" section to extension manifest:...

Add "sidebar" section to extension manifest:

  "sidebar": {
    "default_url": "",
    "default_title": "",
    "default_icon": ""
  }

Allow extension to access experimental.sidebar API only in presence of this section.

BUG=51084
TEST=unit_tests, ExtensionManifestTest.Sidebar


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

[Paweł Hajdan Jr., 2011-01-19 08:49:16]

Drive-by with a minor test comment, no need to wait for another review by me.

http://codereview.chromium.org/6321006/diff/1/chrome/browser/sidebar/sidebar_...
File chrome/browser/sidebar/sidebar_browsertest.cc (right):

http://codereview.chromium.org/6321006/diff/1/chrome/browser/sidebar/sidebar_...
chrome/browser/sidebar/sidebar_browsertest.cc:43:
PathService::Get(chrome::DIR_TEST_DATA, &extension_path);
Check return value of this.

[Aleksey Shlyapnikov, 2011-01-19 19:23:46]

http://codereview.chromium.org/6321006/diff/1/chrome/browser/sidebar/sidebar_...
File chrome/browser/sidebar/sidebar_browsertest.cc (right):

http://codereview.chromium.org/6321006/diff/1/chrome/browser/sidebar/sidebar_...
chrome/browser/sidebar/sidebar_browsertest.cc:43:
PathService::Get(chrome::DIR_TEST_DATA, &extension_path);
On 2011/01/19 08:49:16, Paweł Hajdan Jr. wrote:
> Check return value of this.

Done.

[Aaron Boodman, 2011-01-20 00:36:10]

http://codereview.chromium.org/6321006/diff/19001/chrome/browser/sidebar/side...
File chrome/browser/sidebar/sidebar_container.cc (right):

http://codereview.chromium.org/6321006/diff/19001/chrome/browser/sidebar/side...
chrome/browser/sidebar/sidebar_container.cc:61:
gfx::Size(Extension::kSidebarIconMaxSize,
Is this constant used anywhere else besides here? Maybe it makes more sense in
this file.

http://codereview.chromium.org/6321006/diff/19001/chrome/browser/sidebar/side...
File chrome/browser/sidebar/sidebar_container.h (right):

http://codereview.chromium.org/6321006/diff/19001/chrome/browser/sidebar/side...
chrome/browser/sidebar/sidebar_container.h:149: bool
navigate_to_default_url_on_expand_;
Please add comments for these new fields.

http://codereview.chromium.org/6321006/diff/19001/chrome/browser/sidebar/side...
chrome/browser/sidebar/sidebar_container.h:152: // The object that is waiting
for the sidebar images loading to complete
More accurate comment would be:

Helper to load icons from extension asynchronously

http://codereview.chromium.org/6321006/diff/19001/chrome/common/extensions/ex...
File chrome/common/extensions/extension.h (right):

http://codereview.chromium.org/6321006/diff/19001/chrome/common/extensions/ex...
chrome/common/extensions/extension.h:541: // Helper method to load a
ExtensionSidebarDefaults from the sidebar manifest
s/load a/load an/

http://codereview.chromium.org/6321006/diff/19001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_sidebar_defaults.h (right):

http://codereview.chromium.org/6321006/diff/19001/chrome/common/extensions/ex...
chrome/common/extensions/extension_sidebar_defaults.h:21:
~ExtensionSidebarDefaults() {}
Nit: You don't need these, they are implied by the compiler.

http://codereview.chromium.org/6321006/diff/19001/chrome/common/extensions/ex...
chrome/common/extensions/extension_sidebar_defaults.h:33: std::string
default_icon_path() const {
Return const ref here?

http://codereview.chromium.org/6321006/diff/19001/chrome/common/extensions/ex...
chrome/common/extensions/extension_sidebar_defaults.h:41: GURL default_url()
const {
const ref here too

http://codereview.chromium.org/6321006/diff/19001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_sidebar_utils.cc (right):

http://codereview.chromium.org/6321006/diff/19001/chrome/common/extensions/ex...
chrome/common/extensions/extension_sidebar_utils.cc:20: if
(extension->HasHostPermission(url))
This is overloading the meaning of host permissions. The current meaning is that
an extension has access to do authenticated cross-origin XHR to that host. I
think it's a bad idea to use the same permission to mean two different things.

The simplest thing to do for v1 would be to only allow extension pages to be
displayed in sidebars. That is consistent with how the rest of the system works.
If developers want to override this they can always embed an iframe that
references their remote page.

That would mean changing the URL fields in the manifest to be paths, more like
the rest of the system.

http://codereview.chromium.org/6321006/diff/19001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_sidebar_utils.h (right):

http://codereview.chromium.org/6321006/diff/19001/chrome/common/extensions/ex...
chrome/common/extensions/extension_sidebar_utils.h:17: std::string
GetExtensionIdByContentId(const std::string& content_id);
Can you catch me up to date on what 'content id' is meant to be? Is this for in
the future, when multiple panels can be displayed in a sidebar?

[Aleksey Shlyapnikov, 2011-01-20 02:41:43]

http://codereview.chromium.org/6321006/diff/19001/chrome/browser/sidebar/side...
File chrome/browser/sidebar/sidebar_container.cc (right):

http://codereview.chromium.org/6321006/diff/19001/chrome/browser/sidebar/side...
chrome/browser/sidebar/sidebar_container.cc:61:
gfx::Size(Extension::kSidebarIconMaxSize,
Not in this CL, but sidebar tabs UI needs to know this size too.

On 2011/01/20 00:36:11, Aaron Boodman wrote:
> Is this constant used anywhere else besides here? Maybe it makes more sense in
> this file.

http://codereview.chromium.org/6321006/diff/19001/chrome/browser/sidebar/side...
File chrome/browser/sidebar/sidebar_container.h (right):

http://codereview.chromium.org/6321006/diff/19001/chrome/browser/sidebar/side...
chrome/browser/sidebar/sidebar_container.h:149: bool
navigate_to_default_url_on_expand_;
On 2011/01/20 00:36:11, Aaron Boodman wrote:
> Please add comments for these new fields.

Done.

http://codereview.chromium.org/6321006/diff/19001/chrome/browser/sidebar/side...
chrome/browser/sidebar/sidebar_container.h:152: // The object that is waiting
for the sidebar images loading to complete
On 2011/01/20 00:36:11, Aaron Boodman wrote:
> More accurate comment would be:
> 
> Helper to load icons from extension asynchronously

Done.

http://codereview.chromium.org/6321006/diff/19001/chrome/common/extensions/ex...
File chrome/common/extensions/extension.h (right):

http://codereview.chromium.org/6321006/diff/19001/chrome/common/extensions/ex...
chrome/common/extensions/extension.h:541: // Helper method to load a
ExtensionSidebarDefaults from the sidebar manifest
On 2011/01/20 00:36:11, Aaron Boodman wrote:
> s/load a/load an/

Done.

http://codereview.chromium.org/6321006/diff/19001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_sidebar_defaults.h (right):

http://codereview.chromium.org/6321006/diff/19001/chrome/common/extensions/ex...
chrome/common/extensions/extension_sidebar_defaults.h:21:
~ExtensionSidebarDefaults() {}
On 2011/01/20 00:36:11, Aaron Boodman wrote:
> Nit: You don't need these, they are implied by the compiler.

Done.

http://codereview.chromium.org/6321006/diff/19001/chrome/common/extensions/ex...
chrome/common/extensions/extension_sidebar_defaults.h:33: std::string
default_icon_path() const {
On 2011/01/20 00:36:11, Aaron Boodman wrote:
> Return const ref here?

Done.

http://codereview.chromium.org/6321006/diff/19001/chrome/common/extensions/ex...
chrome/common/extensions/extension_sidebar_defaults.h:41: GURL default_url()
const {
On 2011/01/20 00:36:11, Aaron Boodman wrote:
> const ref here too

Done.

http://codereview.chromium.org/6321006/diff/19001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_sidebar_utils.cc (right):

http://codereview.chromium.org/6321006/diff/19001/chrome/common/extensions/ex...
chrome/common/extensions/extension_sidebar_utils.cc:20: if
(extension->HasHostPermission(url))
As chromium.org says, "Many extension capabilities, such as cross-origin
XMLHttpRequests, programmatically injected content scripts, and the cookies API
require host permissions", and it's even even to check access rights to
chrome://favicon/ url (if
(extension->HasHostPermission(GURL(chrome::kChromeUIFavIconURL))) ...).
I assumed that sidebar is one of "many extension capabilities", that's why
permissions are used there.
Ultimately I think you're correct and we should not overload concepts, but I
would really like to be sure that permissions are misused before I start
renaming a bunch of variables and functions.


On 2011/01/20 00:36:11, Aaron Boodman wrote:
> This is overloading the meaning of host permissions. The current meaning is
that
> an extension has access to do authenticated cross-origin XHR to that host. I
> think it's a bad idea to use the same permission to mean two different things.
> 
> The simplest thing to do for v1 would be to only allow extension pages to be
> displayed in sidebars. That is consistent with how the rest of the system
works.
> If developers want to override this they can always embed an iframe that
> references their remote page.
> 
> That would mean changing the URL fields in the manifest to be paths, more like
> the rest of the system.

http://codereview.chromium.org/6321006/diff/19001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_sidebar_utils.h (right):

http://codereview.chromium.org/6321006/diff/19001/chrome/common/extensions/ex...
chrome/common/extensions/extension_sidebar_utils.h:17: std::string
GetExtensionIdByContentId(const std::string& content_id);
The initial idea was to let one extension open more than one sidebar and
content_id was essentially a way for extension to identify a particular sidebar.
This idea did not make into sidebar v1, but I kept the concept and the name
around to keep the code generic.

On 2011/01/20 00:36:11, Aaron Boodman wrote:
> Can you catch me up to date on what 'content id' is meant to be? Is this for
in
> the future, when multiple panels can be displayed in a sidebar?

[Aaron Boodman, 2011-01-20 17:53:16]

http://codereview.chromium.org/6321006/diff/19001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_sidebar_utils.cc (right):

http://codereview.chromium.org/6321006/diff/19001/chrome/common/extensions/ex...
chrome/common/extensions/extension_sidebar_utils.cc:20: if
(extension->HasHostPermission(url))
On 2011/01/20 02:41:43, Aleksey Shlyapnikov wrote:
> As http://chromium.org says, "Many extension capabilities, such as
cross-origin
> XMLHttpRequests, programmatically injected content scripts, and the cookies
API
> require host permissions"

Yup. All of these are basically the same permission -- they all grant what we
call 'authenticated access' to the data on the site. That is, the extension can
act on the site on behalf of the user via all these APIs; that is why they all
have the same permission.

> and it's even even to check access rights to chrome://favicon/ url (if
> (extension->HasHostPermission(GURL(chrome::kChromeUIFavIconURL))) ...).

This one was a huge mistake that has caused us endless heartache. I would still
like to undo it. This is how we learned that it was a terrible idea to overload
what the hosts permissions are giving access to.

> I assumed that sidebar is one of "many extension capabilities", that's why
> permissions are used there.
> Ultimately I think you're correct and we should not overload concepts, but I
> would really like to be sure that permissions are misused before I start
> renaming a bunch of variables and functions.

Also, if you agree that we should not support remote URLs for the contents of
sidebars (just like we don't for all other surfaces in extensions), then this is
mostly a delete of code. You don't need an extra permission at all, and much of
sidebar_utils can be removed, I think.

[Aaron Boodman, 2011-01-20 18:46:01]

lgtm