Start refractoring extension permissions into ExtensionPermissionSet.

Start refractoring extension permissions into ExtensionPermissionSet.

BUG=84507
TEST=*Extension*


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

[jstritar, 2011-06-09 21:12:57]

I've started to refractor extension permissions into their own set, to make it
easy to compare and modify them. My strategy was to get the
ExtensionPermissionSet working on its own, and then to gradually start taking
responsibility from Extension. I still have some integration and cleanup work to
do, but I thought it may be good to start getting another set of eyeballs on
this. Would one of you be up for doing a high level review? I'll be out of the
office Friday, and them am planning on doing a little more clean up and
integration next week.

This roughly describes the implementation plan:

https://docs.google.com/a/google.com/document/d/1UsHPgW6XJ8hfdHnR39SbpQ41OleT...

Thanks!
--Jon

http://codereview.chromium.org/7003098/diff/2001/chrome/common/extensions/ext...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/7003098/diff/2001/chrome/common/extensions/ext...
chrome/common/extensions/extension.cc:2503: bool Extension::HasApiPermission(
I kept many of these methods (and forward them to the permissions set) to reduce
the scope of the initial refractoring.

http://codereview.chromium.org/7003098/diff/2001/chrome/common/extensions/ext...
File chrome/common/extensions/extension_permission_set.cc (right):

http://codereview.chromium.org/7003098/diff/2001/chrome/common/extensions/ext...
chrome/common/extensions/extension_permission_set.cc:146:
ExtensionPermissionsInfo::ExtensionPermissionsInfo()
Any advice on how to make these initializations cleaner?

[Matt Perry, 2011-06-09 21:35:14]

I can take a look.

[Matt Perry, 2011-06-09 22:28:42]

http://codereview.chromium.org/7003098/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_preference_api.cc (right):

http://codereview.chromium.org/7003098/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_preference_api.cc:50:
ExtensionAPIPermission::ContentSettings()
this is against the style guide (only POD types in static initializers, no
classes allowed). you'll need to find some way to initialize this lazily.

http://codereview.chromium.org/7003098/diff/2001/chrome/common/extensions/ext...
File chrome/common/extensions/extension_permission_set.cc (right):

http://codereview.chromium.org/7003098/diff/2001/chrome/common/extensions/ext...
chrome/common/extensions/extension_permission_set.cc:147: : background_(this,
"background", true, false, 0,
If you take my suggestion and switch to using an enum, I would change this class
to instead keep:
  std::map<ThePermissionEnum, ExtensionApiPermission*> permissions_;

Then do:
  permissions_[kBookmark] = new ExtensionApiPermission("bookmarks", false,
false, IDS_BLA, Message::Bla);

Since there are commonalities between many of these, I'd make a couple helper
functions, like:
RegisterExtensionPermission(kBookmark, "bookmarks", IDS_bla, Message::Bla)
RegisterAppPermission(kClipboardRead, "clipboardRead", IDS_FOO, Message::Foo);
RegisterPrivatePermission(kChromeosInfoPrivate, ...);
and so on.

http://codereview.chromium.org/7003098/diff/2001/chrome/common/extensions/ext...
File chrome/common/extensions/extension_permission_set.h (right):

http://codereview.chromium.org/7003098/diff/2001/chrome/common/extensions/ext...
chrome/common/extensions/extension_permission_set.h:85: static const
ExtensionAPIPermission& Background();
Most consumers don't need to access the internal bits of the
ExtensionAPIPermission structure. Rather they just need a way to refer to the
"bookmarks" permission, or to distinguish between 2 permissions.

Given that, I think it'd be simpler to just return an opaque handle that
represents a permission. An enum would be best:
enum {
  kBackground,
  kBookmark,
  ....,
  // (works nicely for aliases too)
  kOldUnlimitedStorage = kUnlimitedStoage,
  kWindow = kTab
}

Then ExtensionAPIPermissionInfo singleton could have methods for looking up the
ExtensionAPIPermission from the ID.

[jstritar, 2011-06-09 23:00:20]

On 2011/06/09 22:28:42, Matt Perry wrote:
>
http://codereview.chromium.org/7003098/diff/2001/chrome/browser/extensions/ex...
> File chrome/browser/extensions/extension_preference_api.cc (right):
> 
>
http://codereview.chromium.org/7003098/diff/2001/chrome/browser/extensions/ex...
> chrome/browser/extensions/extension_preference_api.cc:50:
> ExtensionAPIPermission::ContentSettings()
> this is against the style guide (only POD types in static initializers, no
> classes allowed). you'll need to find some way to initialize this lazily.
> 

While the accessors are static, they are initialized as regular members of
ExtensionPermissionsInfo. I thought that is okay?

[Matt Perry, 2011-06-09 23:15:48]

On 2011/06/09 23:00:20, jstritar wrote:
> On 2011/06/09 22:28:42, Matt Perry wrote:
> >
>
http://codereview.chromium.org/7003098/diff/2001/chrome/browser/extensions/ex...
> > File chrome/browser/extensions/extension_preference_api.cc (right):
> > 
> >
>
http://codereview.chromium.org/7003098/diff/2001/chrome/browser/extensions/ex...
> > chrome/browser/extensions/extension_preference_api.cc:50:
> > ExtensionAPIPermission::ContentSettings()
> > this is against the style guide (only POD types in static initializers, no
> > classes allowed). you'll need to find some way to initialize this lazily.
> > 
> 
> While the accessors are static, they are initialized as regular members of
> ExtensionPermissionsInfo. I thought that is okay?

The problem is that you're initializing a member of PrefMappingEntry statically.
kPrefMapping here is initialized at program start, before main() runs, because
it's a global variable. This is bad for 2 reasons:
1. The call to ContentSettings() and the like instantiate
ExtensionPermissionsInfo, creating all the permission members.
2. You're initializing PrefMappingEntry's |permission| member here by copy,
which runs the copy constructor.

Anytime you run code during a static initializer, it's dangerous, because you
can't guarantee the order of execution.

Switching to enums instead of class instances for permissions would fix this
issue.

[jstritar, 2011-06-13 19:17:35]

On 2011/06/09 23:15:48, Matt Perry wrote:
> On 2011/06/09 23:00:20, jstritar wrote:
> > On 2011/06/09 22:28:42, Matt Perry wrote:
> > >
> >
>
http://codereview.chromium.org/7003098/diff/2001/chrome/browser/extensions/ex...
> > > File chrome/browser/extensions/extension_preference_api.cc (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/7003098/diff/2001/chrome/browser/extensions/ex...
> > > chrome/browser/extensions/extension_preference_api.cc:50:
> > > ExtensionAPIPermission::ContentSettings()
> > > this is against the style guide (only POD types in static initializers, no
> > > classes allowed). you'll need to find some way to initialize this lazily.
> > > 
> > 
> > While the accessors are static, they are initialized as regular members of
> > ExtensionPermissionsInfo. I thought that is okay?
> 
> The problem is that you're initializing a member of PrefMappingEntry
statically.
> kPrefMapping here is initialized at program start, before main() runs, because
> it's a global variable. This is bad for 2 reasons:
> 1. The call to ContentSettings() and the like instantiate
> ExtensionPermissionsInfo, creating all the permission members.
> 2. You're initializing PrefMappingEntry's |permission| member here by copy,
> which runs the copy constructor.
> 
> Anytime you run code during a static initializer, it's dangerous, because you
> can't guarantee the order of execution.
> 
> Switching to enums instead of class instances for permissions would fix this
> issue.

Ah, I see what you're referring to. I like your idea of just using enums a lot.
It simplifies a lot of this, especially given how the const refs would be used
(lookup and comparisons). Thanks for the advice.

[jstritar, 2011-06-17 21:39:51]

I uploaded new code that uses enums instead of the const refs on Wed....
definitely a good decision. 

I haven't sent it out for review yet because I decided to change the
ExtensionPermissionSet data model. The change helps out a lot, but is taking a
little while since it affects a bunch of parts. It'll be ready for another
review on Monday.

[jstritar, 2011-06-20 22:52:51]

I think this is ready for another round.

I made a pretty big change to the ExtensionPermissionSet data model to make this
less dependent on the extension's manifest (like plugins --> full access,
devtools_url --> full url access). The permission sets are now only defined by
three smaller sets:
 - named permissions: essentially api permissions, plus a few more described
below.
 - explicit host permissions: host permissions defined in the 'permissions'
field
 - scriptable host permissions: content script host permissions

(How do you like those names? Thinking about renaming ExtensionAPIPermission to
ExtensionNamedPermission)

Named permissions can imply permissions like "all access" and "all url access".
I added a couple new permissions kDevTools, kPlugin and kDefault. kDevTools and
kPlugin permissions are implicitly added when the plugin or devtools section is
present in the manifest. kDevTools implies full url access while kPlugin implies
full access. The default permission is really just a placeholder right now, but
will grant access to the default API modules like i18n, extension, i18n once I
implement ExtensionAPIPermission.GrantsAccessToFunction.

Thanks
--Jon

[Matt Perry, 2011-06-21 00:43:10]

http://codereview.chromium.org/7003098/diff/15001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/7003098/diff/15001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:102: // We explicity keep track of
these so that extensions can contain unknown
typo

http://codereview.chromium.org/7003098/diff/15001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:445: bool allow_file_access =
AllowFileAccess(extension_id);
should you clear result to be consistent with the old version?

http://codereview.chromium.org/7003098/diff/15001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:459: pattern.valid_schemes() &
~URLPattern::SCHEME_FILE);
indent +2

http://codereview.chromium.org/7003098/diff/15001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:688: // Migrate the API permission
list to use the enumeration values rather
I think this is dangerous. It requires us to commit to preserving the permission
enum list as it exists now. Why not just stick to the saving the strings, and
parsing them when we load the prefs?

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

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension.h:352: bool HasApiPermission(const
std::string& function_name) const;
I realize the code was already like this, but can you change this method to
HasAPIPermission? Unless you're planning on removing these, in which case,
please add a TODO comment.

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_permission_set.cc (right):

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.cc:130:
ExtensionPermissionMessage::Id id, string16 message) : id_(id),
Format like:
cons(
    foo)
    : a_(a), b_(b) {
}

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constructor_In...

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.cc:177: IdMap ids =
GetInstance()->id_map_;
use a ref here to avoid making a copy.

though if you make this a member function, you won't have to make a temporary.

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.cc:184: NameMap names =
GetInstance()->name_map_;
ditto

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.cc:225: id_map_.clear();
this will happen in the map's destructor automatically.

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.cc:409: URLPatternSet
explicit_hosts) : apis_(apis),
formatting

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.cc:419: URLPatternSet
scriptable_hosts) : apis_(apis),
ditto

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.cc:441: 
nit: kill extra newline

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.cc:514: if (apis().size() == 1
&& apis().count(ExtensionAPIPermission::kDefault))
What's the purpose of the Default permission? How is it different from having no
permissions?

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.cc:621: // Things can't get
worse native code access.
worse than

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.cc:700: void
ExtensionPermissionSet::InitFromExtension(const Extension* extension) {
I'm confused. This only initializes some permissions. I suppose the rest are
covered by apis_ and explicit_hosts_? Not sure what a better name would be.
InitImplicitExtensionPermissions?

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.cc:751: new_api_count++;
could just return true from here rather than counting

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_permission_set.h (right):

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.h:33: enum Id {
nit: rename to ID

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.h:56: const
std::vector<std::string> hosts);
pass by ref: const std::vector<std::string>&

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.h:59:
ExtensionPermissionMessage(Id id, string16 message);
pass string16 by const ref

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.h:60: 
nit: add a destructor, and define it in the .cc. (eglaysher found this resulted
in faster compile times)

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.h:89: kInvalid,
nit: can you make these 2 negative?

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.h:125: typedef std::set<Id>
IdSet;
IDSet

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.h:195: static
ExtensionAPIPermission* GetById(ExtensionAPIPermission::Id id);
GetByID

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.h:195: static
ExtensionAPIPermission* GetById(ExtensionAPIPermission::Id id);
Why are all the public methods static? Consumers can call GetInstance(), so
these should just be regular instance methods.

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.h:253: typedef
std::map<ExtensionAPIPermission::Id, ExtensionAPIPermission*> IdMap;
IDMap

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.h:278: // of the newly created
permission set will be infered from the |extension|
inferred

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.h:281:
ExtensionAPIPermissionSet apis,
pass these args by const ref

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.h:291: // TODO(jstritar):
Implement this...
do you have a use case planned for these methods?

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.h:329: bool IsDefault() const;
I think IsEmpty would convey this more clearly.

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.h:339: bool
HasExplicitAccessToOrigin(const GURL& origin) const;
Looks like this actually matches an URL, not an Origin.

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.h:349: bool
HasEffectiveAccessToHost(const GURL& origin) const;
should be HasEffectAccessToURL - doesn't just match origin or host.

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_permission_set_unittest.cc (right):

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set_unittest.cc:89:
EXPECT_EQ(ExtensionAPIPermission::kTab,
could also loop through all enum values for a thorough test - would be less code
I think.

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set_unittest.cc:186:
EXPECT_EQ((size_t)9, count);
use 9u

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ur...
File chrome/common/extensions/url_pattern_set.cc (right):

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern_set.cc:28: if (!duplicate)
is there a strong reason to strip duplicates? if not, I'd say just let them
happen.

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ur...
File chrome/common/extensions/url_pattern_set.h (right):

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern_set.h:18: // Passes ownership to caller.
ownership of what?

[jstritar, 2011-06-21 23:12:16]

http://codereview.chromium.org/7003098/diff/15001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/7003098/diff/15001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:102: // We explicity keep track of
these so that extensions can contain unknown
On 2011/06/21 00:43:10, Matt Perry wrote:
> typo

Done.

http://codereview.chromium.org/7003098/diff/15001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:445: bool allow_file_access =
AllowFileAccess(extension_id);
On 2011/06/21 00:43:10, Matt Perry wrote:
> should you clear result to be consistent with the old version?

Yes, thanks. Done.

http://codereview.chromium.org/7003098/diff/15001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:459: pattern.valid_schemes() &
~URLPattern::SCHEME_FILE);
On 2011/06/21 00:43:10, Matt Perry wrote:
> indent +2

Done.

http://codereview.chromium.org/7003098/diff/15001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:688: // Migrate the API permission
list to use the enumeration values rather
On 2011/06/21 00:43:10, Matt Perry wrote:
> I think this is dangerous. It requires us to commit to preserving the
permission
> enum list as it exists now. Why not just stick to the saving the strings, and
> parsing them when we load the prefs?

Good point. We're already committed to preserving the names, so we should stick
with that. I was thinking we could save some space, but it's not worth it.
Done.

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

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension.h:352: bool HasApiPermission(const
std::string& function_name) const;
On 2011/06/21 00:43:10, Matt Perry wrote:
> I realize the code was already like this, but can you change this method to
> HasAPIPermission? Unless you're planning on removing these, in which case,
> please add a TODO comment.

I'm thinking about removing it in a subsequent CL, but I renamed the method for
now.
Done.

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_permission_set.cc (right):

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.cc:130:
ExtensionPermissionMessage::Id id, string16 message) : id_(id),
On 2011/06/21 00:43:10, Matt Perry wrote:
> Format like:
> cons(
>     foo)
>     : a_(a), b_(b) {
> }
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constructor_In...

Thanks... Done.

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.cc:177: IdMap ids =
GetInstance()->id_map_;
On 2011/06/21 00:43:10, Matt Perry wrote:
> use a ref here to avoid making a copy.
> 
> though if you make this a member function, you won't have to make a temporary.

This is now a member function. Done.

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.cc:184: NameMap names =
GetInstance()->name_map_;
On 2011/06/21 00:43:10, Matt Perry wrote:
> ditto
same here... Done.

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.cc:225: id_map_.clear();
On 2011/06/21 00:43:10, Matt Perry wrote:
> this will happen in the map's destructor automatically.

Done.

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.cc:409: URLPatternSet
explicit_hosts) : apis_(apis),
On 2011/06/21 00:43:10, Matt Perry wrote:
> formatting

Done.

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.cc:419: URLPatternSet
scriptable_hosts) : apis_(apis),
On 2011/06/21 00:43:10, Matt Perry wrote:
> ditto

Done.

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.cc:441: 
On 2011/06/21 00:43:10, Matt Perry wrote:
> nit: kill extra newline

Done.

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.cc:514: if (apis().size() == 1
&& apis().count(ExtensionAPIPermission::kDefault))
On 2011/06/21 00:43:10, Matt Perry wrote:
> What's the purpose of the Default permission? How is it different from having
no
> permissions?

I'm planning adding a method, GrantsAccessToFunction(std::string funtion_name),
to ExtensionAPIPermission that specifies what functions and modules a given
permission grants access to. We can get rid of all the k*ModuleName and
k*FunctionNames constants if we have a default permission that returns true for
those (e.g., things that require no permissions like tabs.create, tabs.remove,
etc). This should be cleaner since there wouldn't be any special cased rules--
checking access is simply iterating through the API set.

Since this is not implemented and just getting in the way, I removed all the
default permissions code but left in the enum value.

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.cc:621: // Things can't get
worse native code access.
On 2011/06/21 00:43:10, Matt Perry wrote:
> worse than

Done.

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.cc:700: void
ExtensionPermissionSet::InitFromExtension(const Extension* extension) {
On 2011/06/21 00:43:10, Matt Perry wrote:
> I'm confused. This only initializes some permissions. I suppose the rest are
> covered by apis_ and explicit_hosts_? Not sure what a better name would be.
> InitImplicitExtensionPermissions?

Yes, this initializes implicit permissions and extracts the scriptable host
permissions from the extension's UserScriptList. I handle the apis_ and
explicit_hosts_ separately since they are only stored in the set. I think
InitImplicitExtensionPermissions makes better sense, so I renamed it to that.

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.cc:751: new_api_count++;
On 2011/06/21 00:43:10, Matt Perry wrote:
> could just return true from here rather than counting

Oh, good point. Done.

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_permission_set.h (right):

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.h:33: enum Id {
On 2011/06/21 00:43:10, Matt Perry wrote:
> nit: rename to ID

Done.

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.h:56: const
std::vector<std::string> hosts);
On 2011/06/21 00:43:10, Matt Perry wrote:
> pass by ref: const std::vector<std::string>&

Done.

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.h:59:
ExtensionPermissionMessage(Id id, string16 message);
On 2011/06/21 00:43:10, Matt Perry wrote:
> pass string16 by const ref

Done.

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.h:60: 
On 2011/06/21 00:43:10, Matt Perry wrote:
> nit: add a destructor, and define it in the .cc. (eglaysher found this
resulted
> in faster compile times)

Done.

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.h:89: kInvalid,
On 2011/06/21 00:43:10, Matt Perry wrote:
> nit: can you make these 2 negative?

Done.

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.h:125: typedef std::set<Id>
IdSet;
On 2011/06/21 00:43:10, Matt Perry wrote:
> IDSet

Done.

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.h:195: static
ExtensionAPIPermission* GetById(ExtensionAPIPermission::Id id);
On 2011/06/21 00:43:10, Matt Perry wrote:
> GetByID

Done.

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.h:195: static
ExtensionAPIPermission* GetById(ExtensionAPIPermission::Id id);
On 2011/06/21 00:43:10, Matt Perry wrote:
> Why are all the public methods static? Consumers can call GetInstance(), so
> these should just be regular instance methods.

I originally thought they'd be easier to access like this since every consumer
would have to call GetInstance (and probably span 2 lines). After looking at
where I've used these methods, it's not necessary. I've converted them to
regular instance methods.
Done.

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.h:253: typedef
std::map<ExtensionAPIPermission::Id, ExtensionAPIPermission*> IdMap;
On 2011/06/21 00:43:10, Matt Perry wrote:
> IDMap

Done.

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.h:278: // of the newly created
permission set will be infered from the |extension|
On 2011/06/21 00:43:10, Matt Perry wrote:
> inferred

Done.

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.h:281:
ExtensionAPIPermissionSet apis,
On 2011/06/21 00:43:10, Matt Perry wrote:
> pass these args by const ref

Done.

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.h:291: // TODO(jstritar):
Implement this...
On 2011/06/21 00:43:10, Matt Perry wrote:
> do you have a use case planned for these methods?

Yes, they'll be used by the optional permissions API. I removed them for now,
since they're just cluttering the code.

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.h:329: bool IsDefault() const;
On 2011/06/21 00:43:10, Matt Perry wrote:
> I think IsEmpty would convey this more clearly.

Done.

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.h:339: bool
HasExplicitAccessToOrigin(const GURL& origin) const;
On 2011/06/21 00:43:10, Matt Perry wrote:
> Looks like this actually matches an URL, not an Origin.

The host permission paths are set to "/*" within Extension::InitFromValue, so
this does really just check origin access.

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.h:349: bool
HasEffectiveAccessToHost(const GURL& origin) const;
On 2011/06/21 00:43:10, Matt Perry wrote:
> should be HasEffectAccessToURL - doesn't just match origin or host.

Yeah, good point, since the user script URLPatterns will have paths in them
still. I'll change this to URL for now.

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_permission_set_unittest.cc (right):

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set_unittest.cc:89:
EXPECT_EQ(ExtensionAPIPermission::kTab,
On 2011/06/21 00:43:10, Matt Perry wrote:
> could also loop through all enum values for a thorough test - would be less
code
> I think.

Done.

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set_unittest.cc:186:
EXPECT_EQ((size_t)9, count);
On 2011/06/21 00:43:10, Matt Perry wrote:
> use 9u

Done.

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ur...
File chrome/common/extensions/url_pattern_set.cc (right):

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern_set.cc:28: if (!duplicate)
On 2011/06/21 00:43:10, Matt Perry wrote:
> is there a strong reason to strip duplicates? if not, I'd say just let them
> happen.

It doesn't really matter, except that they'll accumulate in the prefs. I got rid
of the duplicate checks here, and modified
ExtensionPrefs::SetExtensionPrefURLPatternSet to not store duplicates.
Done.

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ur...
File chrome/common/extensions/url_pattern_set.h (right):

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern_set.h:18: // Passes ownership to caller.
On 2011/06/21 00:43:10, Matt Perry wrote:
> ownership of what?

Oops, I had originally been returning a pointer to a new instance.

[Matt Perry, 2011-06-22 00:01:22]

LGTM

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_permission_set.h (right):

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.h:339: bool
HasExplicitAccessToOrigin(const GURL& origin) const;
On 2011/06/21 23:12:16, jstritar wrote:
> On 2011/06/21 00:43:10, Matt Perry wrote:
> > Looks like this actually matches an URL, not an Origin.
> 
> The host permission paths are set to "/*" within Extension::InitFromValue, so
> this does really just check origin access.

But this class does nothing to enforce that the argument passed in is only an
origin, or that explicit_hosts contains only origins, does it?

http://codereview.chromium.org/7003098/diff/24002/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.h:86: // to kEnumBoundary.
This is no longer a requirement, right?

[jstritar, 2011-06-22 15:44:47]

Thanks for the review!

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_permission_set.h (right):

http://codereview.chromium.org/7003098/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.h:339: bool
HasExplicitAccessToOrigin(const GURL& origin) const;
On 2011/06/22 00:01:22, Matt Perry wrote:
>
> But this class does nothing to enforce that the argument passed in is only an
> origin, or that explicit_hosts contains only origins, does it?

Yeah, good point. I now remove all the explicit host paths in the
ExtensionPermissionSet constructors.

http://codereview.chromium.org/7003098/diff/24002/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.h:86: // to kEnumBoundary.
On 2011/06/22 00:01:22, Matt Perry wrote:
> This is no longer a requirement, right?

Oh, yes. Removed.

[commit-bot: I haz the power, 2011-06-23 19:02:57]

Change committed as 90244