Support for extensions permissions. This CL only contains, parsing, initializ...

Support for extensions permissions. This CL only contains, parsing, initializing values into Extension, outputting values to the extension_ui, and appropriate unit test changes



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

[rafaelw, 2009-03-21 18:12:52]

First part of extensions xhr. Check it out.

[rafaelw, 2009-03-21 18:16:30]

Here's part 1

[Aaron Boodman, 2009-03-21 22:43:43]

Nice!

http://codereview.chromium.org/42480/diff/1/9
File chrome/browser/extensions/extension.cc (right):

http://codereview.chromium.org/42480/diff/1/9#newcode79
Line 79: const char* Extension::kInvalidPermissionsError =
Alphabetize these too please.

http://codereview.chromium.org/42480/diff/1/9#newcode82
Line 82: "Invalid value for 'permissions'. There must be at least one "
I don't think we should require at least one permission. We should probably
remove the requirement for content scripts, too. It doesn't really make sense to
have a permission key with no items, but people might remove an entry to test or
something.

http://codereview.chromium.org/42480/diff/1/9#newcode87
Line 87: "Invalid scheme for 'permissions[*]'. Only 'http' and 'https' are "
I like how you've made this common error separate from a generic 'invalid'
message.

http://codereview.chromium.org/42480/diff/1/9#newcode108
Line 108: permissions_() {
You don't need to initialize members that have default constructor; they get
initialized automatically.

http://codereview.chromium.org/42480/diff/1/9#newcode492
Line 492: if (pattern.scheme() != "http" && pattern.scheme() != "https") {
There are constants for "http" and "https" in url_constants.h.

http://codereview.chromium.org/42480/diff/1/10
File chrome/browser/extensions/extension.h (right):

http://codereview.chromium.org/42480/diff/1/10#newcode10
Line 10: #include <map>
Is this used? It seems unnecessary.

http://codereview.chromium.org/42480/diff/1/10#newcode49
Line 49: static const wchar_t* kPermissionsKey;
Can you re-alphabetize this list as long as you're in here?

http://codereview.chromium.org/42480/diff/1/7
File chrome/browser/extensions/extension_unittest.cc (right):

http://codereview.chromium.org/42480/diff/1/7#newcode189
Line 189: EXPECT_TRUE(MatchPattern(error,
Extension::kInvalidPermissionCountError));
Please add tests for the other two failures: permission item is not a string,
and permission item is a string, but not a valid URL pattern.

http://codereview.chromium.org/42480/diff/1/6
File chrome/browser/extensions/extensions_ui.cc (right):

http://codereview.chromium.org/42480/diff/1/6#newcode156
Line 156: permission != permissions.end(); ++permission) {
Indent this line one more space.

[rafaelw, 2009-03-22 20:09:55]

http://codereview.chromium.org/42480/diff/1/9
File chrome/browser/extensions/extension.cc (right):

http://codereview.chromium.org/42480/diff/1/9#newcode79
Line 79: const char* Extension::kInvalidPermissionsError =
On 2009/03/21 22:43:43, Aaron Boodman wrote:
> Alphabetize these too please.

Sorry. They were when they were "XHR...". Done.

http://codereview.chromium.org/42480/diff/1/9#newcode82
Line 82: "Invalid value for 'permissions'. There must be at least one "
On 2009/03/21 22:43:43, Aaron Boodman wrote:
> I don't think we should require at least one permission. We should probably
> remove the requirement for content scripts, too. It doesn't really make sense
to
> have a permission key with no items, but people might remove an entry to test
or
> something.

Done. Now issues warning, but doesn't fail the extension loading.

http://codereview.chromium.org/42480/diff/1/9#newcode87
Line 87: "Invalid scheme for 'permissions[*]'. Only 'http' and 'https' are "
On 2009/03/21 22:43:43, Aaron Boodman wrote:
> I like how you've made this common error separate from a generic 'invalid'
> message.

Awesome.

http://codereview.chromium.org/42480/diff/1/9#newcode108
Line 108: permissions_() {
On 2009/03/21 22:43:43, Aaron Boodman wrote:
> You don't need to initialize members that have default constructor; they get
> initialized automatically.

Done.

http://codereview.chromium.org/42480/diff/1/9#newcode492
Line 492: if (pattern.scheme() != "http" && pattern.scheme() != "https") {
On 2009/03/21 22:43:43, Aaron Boodman wrote:
> There are constants for "http" and "https" in url_constants.h.

Done.

http://codereview.chromium.org/42480/diff/1/10
File chrome/browser/extensions/extension.h (right):

http://codereview.chromium.org/42480/diff/1/10#newcode10
Line 10: #include <map>
On 2009/03/21 22:43:43, Aaron Boodman wrote:
> Is this used? It seems unnecessary.

It is used in this file, but not by me. GLint told me to fix it ("declare what
you use"). And I do what I'm told.

Say the word and it's gone.

http://codereview.chromium.org/42480/diff/1/10#newcode49
Line 49: static const wchar_t* kPermissionsKey;
On 2009/03/21 22:43:43, Aaron Boodman wrote:
> Can you re-alphabetize this list as long as you're in here?

Done.

http://codereview.chromium.org/42480/diff/1/7
File chrome/browser/extensions/extension_unittest.cc (right):

http://codereview.chromium.org/42480/diff/1/7#newcode189
Line 189: EXPECT_TRUE(MatchPattern(error,
Extension::kInvalidPermissionCountError));
On 2009/03/21 22:43:43, Aaron Boodman wrote:
> Please add tests for the other two failures: permission item is not a string,
> and permission item is a string, but not a valid URL pattern.

Done.

http://codereview.chromium.org/42480/diff/1/6
File chrome/browser/extensions/extensions_ui.cc (right):

http://codereview.chromium.org/42480/diff/1/6#newcode156
Line 156: permission != permissions.end(); ++permission) {
On 2009/03/21 22:43:43, Aaron Boodman wrote:
> Indent this line one more space.

Done.

[Aaron Boodman, 2009-03-23 02:29:17]

LGTM!