Support socket endpoint permissions for AppsV2 Socket API.

Support socket endpoint permissions for AppsV2 Socket API.

BUG=130222
TEST=unittest & browser_test

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

[Peng, 2012-07-26 19:02:13]

Hi Mike, Could please review this CL? Thanks.

[miket_OOO, 2012-07-31 18:18:36]

I don't see unit tests for the new APIPermissionSet class.

The CL description no longer appears to match the changes.

http://codereview.chromium.org/10692160/diff/22019/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/10692160/diff/22019/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:495: tmp = NULL;
Does permission_list->Get() change the Value yet also return failure? If so,
it's a weird function. If not, the tmp = NULL is a no-op.

http://codereview.chromium.org/10692160/diff/22019/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:498: detail->FromValue(tmp);
Does this work if tmp is NULL? Should we even be adding a null detail?

http://codereview.chromium.org/10692160/diff/22019/chrome/common/extensions/e...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/10692160/diff/22019/chrome/common/extensions/e...
chrome/common/extensions/extension.cc:3373: value = NULL;
Same comment as earlier. It's weird that a failing method would require the
caller to clean up the mess that it knows it created.

http://codereview.chromium.org/10692160/diff/22019/chrome/common/extensions/p...
File chrome/common/extensions/permissions/api_permission.cc (right):

http://codereview.chromium.org/10692160/diff/22019/chrome/common/extensions/p...
chrome/common/extensions/permissions/api_permission.cc:50: CHECK(permission() ==
detail->permission());
Is this right? Do we really want to crash?

Same with the rest of these set methods -- is this the right behavior?

http://codereview.chromium.org/10692160/diff/22019/chrome/common/extensions/p...
chrome/common/extensions/permissions/api_permission.cc:115: const
DetailConstructor &detail_constructor)
whitespace

http://codereview.chromium.org/10692160/diff/22019/chrome/common/extensions/p...
File chrome/common/extensions/permissions/api_permission.h (right):

http://codereview.chromium.org/10692160/diff/22019/chrome/common/extensions/p...
chrome/common/extensions/permissions/api_permission.h:20: 
Vertical whitespace inside forward-declaration namespaces is uncommon. Please
remove.

http://codereview.chromium.org/10692160/diff/22019/chrome/common/extensions/p...
chrome/common/extensions/permissions/api_permission.h:195: :
permission_(permission) { }
As long as you have a constructor body, might as well DCHECK the param.

http://codereview.chromium.org/10692160/diff/22019/chrome/common/extensions/p...
chrome/common/extensions/permissions/api_permission.h:216: virtual bool
Contains(const APIPermissionDetail*detail) const = 0;
space after *

http://codereview.chromium.org/10692160/diff/22019/chrome/common/extensions/p...
chrome/common/extensions/permissions/api_permission.h:230: // Returns a new
created differences detail objectbetween this and
typo. Also, this description is not fluent. "Returns a new created differences
detail object" is confusing.

http://codereview.chromium.org/10692160/diff/22019/chrome/common/extensions/p...
chrome/common/extensions/permissions/api_permission.h:231: // |detail| (this -
|detal|).
typo

http://codereview.chromium.org/10692160/diff/22019/chrome/common/extensions/p...
chrome/common/extensions/permissions/api_permission.h:235: // Returns a new
created union detail of this and |detail|.
same (et seq.)

http://codereview.chromium.org/10692160/diff/22019/chrome/common/extensions/p...
chrome/common/extensions/permissions/api_permission.h:251: virtual void
Log(std::string *l) const = 0;
As a general readability rule, avoid lowercase L as a variable name, because
it's nearly indistinguishable from the number 1.

http://codereview.chromium.org/10692160/diff/22019/chrome/common/extensions/p...
File chrome/common/extensions/permissions/api_permission_set.h (right):

http://codereview.chromium.org/10692160/diff/22019/chrome/common/extensions/p...
chrome/common/extensions/permissions/api_permission_set.h:28: const_iterator&
operator++() {
the vertical whitespace here is inconsistent. If you're grouping constructors,
then fix the operator++. If you're separating each method, then do that.

[Peng, 2012-07-31 19:32:28]

http://codereview.chromium.org/10692160/diff/22019/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/10692160/diff/22019/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:495: tmp = NULL;
On 2012/07/31 18:18:37, miket wrote:
> Does permission_list->Get() change the Value yet also return failure? If so,
> it's a weird function. If not, the tmp = NULL is a no-op.

Done. I checked ListValue::Get() the tmp will be unchanged, if it returns false.

http://codereview.chromium.org/10692160/diff/22019/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:498: detail->FromValue(tmp);
On 2012/07/31 18:18:37, miket wrote:
> Does this work if tmp is NULL? Should we even be adding a null detail?

Done.

http://codereview.chromium.org/10692160/diff/22019/chrome/common/extensions/e...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/10692160/diff/22019/chrome/common/extensions/e...
chrome/common/extensions/extension.cc:3373: value = NULL;
On 2012/07/31 18:18:37, miket wrote:
> Same comment as earlier. It's weird that a failing method would require the
> caller to clean up the mess that it knows it created.

Done.

http://codereview.chromium.org/10692160/diff/22019/chrome/common/extensions/p...
File chrome/common/extensions/permissions/api_permission.cc (right):

http://codereview.chromium.org/10692160/diff/22019/chrome/common/extensions/p...
chrome/common/extensions/permissions/api_permission.cc:50: CHECK(permission() ==
detail->permission());
On 2012/07/31 18:18:37, miket wrote:
> Is this right? Do we really want to crash?
> 
> Same with the rest of these set methods -- is this the right behavior?

Diff, Union, Intersect can only be used on same API details and caller should
guarantee it.

http://codereview.chromium.org/10692160/diff/22019/chrome/common/extensions/p...
chrome/common/extensions/permissions/api_permission.cc:115: const
DetailConstructor &detail_constructor)
On 2012/07/31 18:18:37, miket wrote:
> whitespace

Done.

http://codereview.chromium.org/10692160/diff/22019/chrome/common/extensions/p...
File chrome/common/extensions/permissions/api_permission.h (right):

http://codereview.chromium.org/10692160/diff/22019/chrome/common/extensions/p...
chrome/common/extensions/permissions/api_permission.h:20: 
On 2012/07/31 18:18:37, miket wrote:
> Vertical whitespace inside forward-declaration namespaces is uncommon. Please
> remove.

Done.

http://codereview.chromium.org/10692160/diff/22019/chrome/common/extensions/p...
chrome/common/extensions/permissions/api_permission.h:195: :
permission_(permission) { }
On 2012/07/31 18:18:37, miket wrote:
> As long as you have a constructor body, might as well DCHECK the param.

Done.

http://codereview.chromium.org/10692160/diff/22019/chrome/common/extensions/p...
chrome/common/extensions/permissions/api_permission.h:216: virtual bool
Contains(const APIPermissionDetail*detail) const = 0;
On 2012/07/31 18:18:37, miket wrote:
> space after *

Done.

http://codereview.chromium.org/10692160/diff/22019/chrome/common/extensions/p...
chrome/common/extensions/permissions/api_permission.h:230: // Returns a new
created differences detail objectbetween this and
On 2012/07/31 18:18:37, miket wrote:
> typo. Also, this description is not fluent. "Returns a new created differences
> detail object" is confusing.

Done.

http://codereview.chromium.org/10692160/diff/22019/chrome/common/extensions/p...
chrome/common/extensions/permissions/api_permission.h:231: // |detail| (this -
|detal|).
On 2012/07/31 18:18:37, miket wrote:
> typo

Done.

http://codereview.chromium.org/10692160/diff/22019/chrome/common/extensions/p...
chrome/common/extensions/permissions/api_permission.h:235: // Returns a new
created union detail of this and |detail|.
On 2012/07/31 18:18:37, miket wrote:
> same (et seq.)

Done.

http://codereview.chromium.org/10692160/diff/22019/chrome/common/extensions/p...
chrome/common/extensions/permissions/api_permission.h:251: virtual void
Log(std::string *l) const = 0;
On 2012/07/31 18:18:37, miket wrote:
> As a general readability rule, avoid lowercase L as a variable name, because
> it's nearly indistinguishable from the number 1.

Done.

http://codereview.chromium.org/10692160/diff/22019/chrome/common/extensions/p...
File chrome/common/extensions/permissions/api_permission_set.h (right):

http://codereview.chromium.org/10692160/diff/22019/chrome/common/extensions/p...
chrome/common/extensions/permissions/api_permission_set.h:28: const_iterator&
operator++() {
On 2012/07/31 18:18:37, miket wrote:
> the vertical whitespace here is inconsistent. If you're grouping constructors,
> then fix the operator++. If you're separating each method, then do that.

Done.

[Peng, 2012-07-31 19:36:26]

On 2012/07/31 18:18:36, miket wrote:
> I don't see unit tests for the new APIPermissionSet class.

I think in chrome/common/extensions/permissions/permission_set_unittest.cc, test
cases PermissionsTest.Create{Difference,Union, Intersection} cover major
functions of APIPermissionSet class. Do you think we still need separated unit
test?

[miket_OOO, 2012-07-31 23:23:45]

On 2012/07/31 19:36:26, Peng wrote:
> On 2012/07/31 18:18:36, miket wrote:
> > I don't see unit tests for the new APIPermissionSet class.
> 
> I think in chrome/common/extensions/permissions/permission_set_unittest.cc,
test
> cases PermissionsTest.Create{Difference,Union, Intersection} cover major
> functions of APIPermissionSet class. Do you think we still need separated unit
> test?

When we're adding new functionality, it's good to see tests that exercise the
new functionality. As-is, when I look at the *test.cc files in the current CL as
of Patch #2, all I see are syntactic changes. This strongly suggests that the CL
adds *no* new functionality, which raises the question why we're doing it at all
(and note that the changelist description gives no explanation).

The CL is the chance to show off the new functionality. We have new, detailed
permissions? Great! Let's see sample usage in the form of new unit tests. Let's
show what the new classes can and can't do. Let's see a toy version of the
actual use case that drove us to do this in the first place. In concrete terms,
why not go ahead and implement a small, simple version of the socket API
permissions (call them something else imaginary like
TinCanSocialNetworkRelationships, whatever you like) and confirm that this CL
actually addresses the need that you expect to have? Personally, I find that in
the process of writing real, substantial tests for my code, I discover that my
first API design is inconvenient (sometimes even terrible), and I end up
revising it significantly before another person gets to even review it. Think of
the tests as your first customer of your code. You wouldn't call a product done
without asking the customer's opinion, would you?

If you think it really is the case that this CL adds no new functionality and is
merely a refactoring of a poor design or prior implementation, then state that
in the CL description. As is, the CL claims to be adding substantial new
functionality but no tests to prove that new functionality works, so it's hard
for a reviewer to approve it -- especially a reviewer who doesn't have the same
vision as you do of how you intend to use these new capabilities.

[Peng, 2012-08-01 15:28:41]

On 2012/07/31 23:23:45, miket wrote:
> On 2012/07/31 19:36:26, Peng wrote:
> > On 2012/07/31 18:18:36, miket wrote:
> > > I don't see unit tests for the new APIPermissionSet class.
> > 
> > I think in chrome/common/extensions/permissions/permission_set_unittest.cc,
> test
> > cases PermissionsTest.Create{Difference,Union, Intersection} cover major
> > functions of APIPermissionSet class. Do you think we still need separated
unit
> > test?
> 
> When we're adding new functionality, it's good to see tests that exercise the
> new functionality. As-is, when I look at the *test.cc files in the current CL
as
> of Patch #2, all I see are syntactic changes. This strongly suggests that the
CL
> adds *no* new functionality, which raises the question why we're doing it at
all
> (and note that the changelist description gives no explanation).
> 
> The CL is the chance to show off the new functionality. We have new, detailed
> permissions? Great! Let's see sample usage in the form of new unit tests.
Let's
> show what the new classes can and can't do. Let's see a toy version of the
> actual use case that drove us to do this in the first place. In concrete
terms,
> why not go ahead and implement a small, simple version of the socket API
> permissions (call them something else imaginary like
> TinCanSocialNetworkRelationships, whatever you like) and confirm that this CL
> actually addresses the need that you expect to have? Personally, I find that
in
> the process of writing real, substantial tests for my code, I discover that my
> first API design is inconvenient (sometimes even terrible), and I end up
> revising it significantly before another person gets to even review it. Think
of
> the tests as your first customer of your code. You wouldn't call a product
done
> without asking the customer's opinion, would you?
> 
> If you think it really is the case that this CL adds no new functionality and
is
> merely a refactoring of a poor design or prior implementation, then state that
> in the CL description. As is, the CL claims to be adding substantial new
> functionality but no tests to prove that new functionality works, so it's hard
> for a reviewer to approve it -- especially a reviewer who doesn't have the
same
> vision as you do of how you intend to use these new capabilities.

Sounds good. I think there are two ideas:

1) create a fake permission detail for testing.
2) modify the old socket permission CL base on this design, and add unit_test
and browser_test with it. And review two CLs together.

If option 1 needs too much extra efforts, probably I will go option 2). Is it
OK?

[miket_OOO, 2012-08-01 16:15:57]

Whichever makes more sense to you.

[Peng, 2012-08-03 18:43:25]

On 2012/08/01 16:15:57, miket wrote:
> Whichever makes more sense to you.

Hi Mike, I integrated the socket endpoint permissions with this CL and added
some test cases. Please check it. Thanks.

[miket_OOO, 2012-08-06 21:04:06]

This is going to need another look, and it would be really helpful if each class
had its own unit tests rather than tunneling through the unit tests of the
classes that are composed of them.

http://codereview.chromium.org/10692160/diff/18017/chrome/browser/extensions/...
File chrome/browser/extensions/api/permissions/permissions_api.cc (right):

http://codereview.chromium.org/10692160/diff/18017/chrome/browser/extensions/...
chrome/browser/extensions/api/permissions/permissions_api.cc:201:
GetExtension()->GetType()).size() == 0;
Can you check to see whether you can use IsEmpty() or empty() instead of size()
== 0? It might not make a difference in runtime, but it is much more readable,
and in the case of some STL containers it permits a faster test (rather than
producing an exact count, it early-outs if there are more than zero members).

http://codereview.chromium.org/10692160/diff/18017/chrome/browser/extensions/...
File chrome/browser/extensions/api/socket/socket_api.cc (right):

http://codereview.chromium.org/10692160/diff/18017/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket_api.cc:36: const char
kNoPermission[] = "Caller does not have permission";
Can you pick a constant name that's consistent with the others in this group?
It'll make it easier for future developers searching for error constants.

http://codereview.chromium.org/10692160/diff/18017/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/10692160/diff/18017/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:510: DLOG(WARNING) << "Get detail
from permission_list failed.";
I haven't looked at DLOG, but beware that depending on how it's implemented it
might cause a compile error in release mode because of the else without braces.

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/e...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/e...
chrome/common/extensions/extension.cc:3496: base::AutoLock
auto_lock(runtime_data_lock_);
It looks like this follows the existing pattern for permissions in this file,
but beware that new AutoLocks are generally a bad review smell.

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/e...
File chrome/common/extensions/extension_unittest.cc (right):

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/e...
chrome/common/extensions/extension_unittest.cc:460: TEST(ExtensionTest,
SocketPermissions) {
These are good because they're tests, but you can still do much more specific
unit tests at the individual class level. E.g., for the empty.json test, you can
test much more -- wildcards, invalid input, input that looks malicious
(NULL/empty, :, ******, \\\\\\\\\, \0\0\0\0\0\0\0\0, \"*8*0:, illegal Unicode,
nulls in the middle of strings, negative port numbers, 33-bit port numbers,
dotted quads with illegal characters in the middle, etc.) that would be too
cumbersome to test at this level. You don't need to create an extension, even an
empty one, to test all those cases.

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/p...
File chrome/common/extensions/permissions/permission_set_unittest.cc (right):

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/p...
chrome/common/extensions/permissions/permission_set_unittest.cc:569:
value->Append(Value::CreateStringValue("udp-send-to::8888"));
These tests are all good, just make sure that they really need to be part of
this class, and not part of a simpler class.

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/p...
File chrome/common/extensions/permissions/socket_permission.h (right):

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission.h:77: private:
You should #include <set>, as well as other stuff this file directly uses
(RefCounted, IPC::Message, etc.). As it is, it only happens to build because the
included files already include those files.

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/p...
File chrome/common/extensions/permissions/socket_permission_data.cc (right):

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.cc:19: host_("*"),
Consider making these special characters (*, :, ., etc.) into string constants.

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.cc:63: if
(host.length() < host_.length() + 2)
Can you add a comment about the + 2? It's not at all obvious what it means.

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.cc:67:
host_.length(), host_) != 0)
This doesn't appear to be properly indented.

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.cc:79: type_ = NONE;
Ideally the reset functionality would be in a separate, testable private method.

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.cc:86:
base::SplitString(permission, ':', &tokens);
Please have a look at base/string_tokenizer.h and see whether that's a better
fit than SplitString.

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.cc:90: 
Does this crash if tokens is empty at this point?

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.cc:91: if (tokens[0]
== "tcp-connect")
These should be constants in a common file.

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.cc:109:
base::SplitString(host_, '.', &host_components);
Same question. I'm not sure how you can guarantee that host_components is not
empty at this point.

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.cc:125: port_ =
atoi(tokens[2].c_str());
Please look at base/string_number_conversions.h. Given atoi's poor record with
error-handling, I'd advise using Chrome wrappers wherever possible.

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.cc:133: if
(!spec_.empty())
Are you sure a stringstream is necessary here? It looks like fairly simple
string concatenation that a std::string ought to handle just fine.

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.cc:137: switch
(type_) {
Consider two separate methods that map type to string and vice-versa. They'll be
easier to test, and it'll be more obvious what needs to change if someone else
comes in needing to alter the permission scheme.

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/p...
File chrome/common/extensions/permissions/socket_permission_data.h (right):

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.h:11: // A pattern
that can be used to match socket permission.
This seems to be a very unit-testable class, but there isn't a dedicated test
for it. Can you write one? Remember that browser tests are relatively slow and
hard to debug, while unit tests are fast and bring in few unrelated parts of the
system.

http://codereview.chromium.org/10692160/diff/18017/chrome/test/data/extension...
File chrome/test/data/extensions/socket_permissions/socket1.json (right):

http://codereview.chromium.org/10692160/diff/18017/chrome/test/data/extension...
chrome/test/data/extensions/socket_permissions/socket1.json:5: "app" : {
Why add the inconsistent whitespace?

[Peng, 2012-08-07 21:31:54]

I fixed issues and added some unit tests. Please take a look. Thanks.

http://codereview.chromium.org/10692160/diff/18017/chrome/browser/extensions/...
File chrome/browser/extensions/api/permissions/permissions_api.cc (right):

http://codereview.chromium.org/10692160/diff/18017/chrome/browser/extensions/...
chrome/browser/extensions/api/permissions/permissions_api.cc:201:
GetExtension()->GetType()).size() == 0;
On 2012/08/06 21:04:06, miket wrote:
> Can you check to see whether you can use IsEmpty() or empty() instead of
size()
> == 0? It might not make a difference in runtime, but it is much more readable,
> and in the case of some STL containers it permits a faster test (rather than
> producing an exact count, it early-outs if there are more than zero members).

Done.

http://codereview.chromium.org/10692160/diff/18017/chrome/browser/extensions/...
File chrome/browser/extensions/api/socket/socket_api.cc (right):

http://codereview.chromium.org/10692160/diff/18017/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket_api.cc:36: const char
kNoPermission[] = "Caller does not have permission";
On 2012/08/06 21:04:06, miket wrote:
> Can you pick a constant name that's consistent with the others in this group?
> It'll make it easier for future developers searching for error constants.

Done. I replaced it with kPermissionError. If you have better name, please
suggest me.

http://codereview.chromium.org/10692160/diff/18017/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/10692160/diff/18017/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:510: DLOG(WARNING) << "Get detail
from permission_list failed.";
On 2012/08/06 21:04:06, miket wrote:
> I haven't looked at DLOG, but beware that depending on how it's implemented it
> might cause a compile error in release mode because of the else without
braces.

Tested it in release build. Seems OK.

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/e...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/e...
chrome/common/extensions/extension.cc:3496: base::AutoLock
auto_lock(runtime_data_lock_);
On 2012/08/06 21:04:06, miket wrote:
> It looks like this follows the existing pattern for permissions in this file,
> but beware that new AutoLocks are generally a bad review smell.

I guess this AutoLock protects runtime_data_ which could be modified by
permission extension API or other components. I am not sure those modifying are
from the same thread. So I think it will be safer to follow other functions.

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/e...
File chrome/common/extensions/extension_unittest.cc (right):

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/e...
chrome/common/extensions/extension_unittest.cc:460: TEST(ExtensionTest,
SocketPermissions) {
On 2012/08/06 21:04:06, miket wrote:
> These are good because they're tests, but you can still do much more specific
> unit tests at the individual class level. E.g., for the empty.json test, you
can
> test much more -- wildcards, invalid input, input that looks malicious
> (NULL/empty, :, ******, \\\\\\\\\, \0\0\0\0\0\0\0\0, \"*8*0:, illegal Unicode,
> nulls in the middle of strings, negative port numbers, 33-bit port numbers,
> dotted quads with illegal characters in the middle, etc.) that would be too
> cumbersome to test at this level. You don't need to create an extension, even
an
> empty one, to test all those cases.

I added some unitttest for it. Done

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/p...
File chrome/common/extensions/permissions/permission_set_unittest.cc (right):

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/p...
chrome/common/extensions/permissions/permission_set_unittest.cc:569:
value->Append(Value::CreateStringValue("udp-send-to::8888"));
On 2012/08/06 21:04:06, miket wrote:
> These tests are all good, just make sure that they really need to be part of
> this class, and not part of a simpler class.

Done. I also added some unittests for APIPermissionSet.

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/p...
File chrome/common/extensions/permissions/socket_permission.h (right):

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission.h:77: private:
On 2012/08/06 21:04:06, miket wrote:
> You should #include <set>, as well as other stuff this file directly uses
> (RefCounted, IPC::Message, etc.). As it is, it only happens to build because
the
> included files already include those files.

Done.

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/p...
File chrome/common/extensions/permissions/socket_permission_data.cc (right):

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.cc:19: host_("*"),
On 2012/08/06 21:04:06, miket wrote:
> Consider making these special characters (*, :, ., etc.) into string
constants.

Done.

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.cc:63: if
(host.length() < host_.length() + 2)
On 2012/08/06 21:04:06, miket wrote:
> Can you add a comment about the + 2? It's not at all obvious what it means.

Done.

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.cc:67:
host_.length(), host_) != 0)
On 2012/08/06 21:04:06, miket wrote:
> This doesn't appear to be properly indented.

Done.

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.cc:79: type_ = NONE;
On 2012/08/06 21:04:06, miket wrote:
> Ideally the reset functionality would be in a separate, testable private
method.

Done. I changed it to *this = SocketPermissionData(), Is it OK? (otherwise it
has to have duplicate in reset and constructor, and we have to make sure they
are doing same thing, when we modify the initial values.)

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.cc:86:
base::SplitString(permission, ':', &tokens);
On 2012/08/06 21:04:06, miket wrote:
> Please have a look at base/string_tokenizer.h and see whether that's a better
> fit than SplitString.
Seems StringTokenizer can not handle "aaa::cc" well. It will return aaa, cc
instead aaa, , cc. So probably using SplitString is simpler.

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.cc:90: 
On 2012/08/06 21:04:06, miket wrote:
> Does this crash if tokens is empty at this point?

Done.

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.cc:91: if (tokens[0]
== "tcp-connect")
On 2012/08/06 21:04:06, miket wrote:
> These should be constants in a common file.

Done.

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.cc:109:
base::SplitString(host_, '.', &host_components);
On 2012/08/06 21:04:06, miket wrote:
> Same question. I'm not sure how you can guarantee that host_components is not
> empty at this point.

Done. host_ is checked at 107 line. And it is from tokens[1] which should
trimmed by SplitString() function. So it has to have some non space chars.
Anyway, I will add a DCHECK() here. And I will add some unittest for it.

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.cc:125: port_ =
atoi(tokens[2].c_str());
On 2012/08/06 21:04:06, miket wrote:
> Please look at base/string_number_conversions.h. Given atoi's poor record with
> error-handling, I'd advise using Chrome wrappers wherever possible.

Done.

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.cc:133: if
(!spec_.empty())
On 2012/08/06 21:04:06, miket wrote:
> Are you sure a stringstream is necessary here? It looks like fairly simple
> string concatenation that a std::string ought to handle just fine.

Done.

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.cc:137: switch
(type_) {
On 2012/08/06 21:04:06, miket wrote:
> Consider two separate methods that map type to string and vice-versa. They'll
be
> easier to test, and it'll be more obvious what needs to change if someone else
> comes in needing to alter the permission scheme.

Done.

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/p...
File chrome/common/extensions/permissions/socket_permission_data.h (right):

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.h:11: // A pattern
that can be used to match socket permission.
On 2012/08/06 21:04:06, miket wrote:
> This seems to be a very unit-testable class, but there isn't a dedicated test
> for it. Can you write one? Remember that browser tests are relatively slow and
> hard to debug, while unit tests are fast and bring in few unrelated parts of
the
> system.

Done

http://codereview.chromium.org/10692160/diff/18017/chrome/test/data/extension...
File chrome/test/data/extensions/socket_permissions/socket1.json (right):

http://codereview.chromium.org/10692160/diff/18017/chrome/test/data/extension...
chrome/test/data/extensions/socket_permissions/socket1.json:5: "app" : {
On 2012/08/06 21:04:06, miket wrote:
> Why add the inconsistent whitespace?

Done. Guess it is because of c-n-p.

[miket_OOO, 2012-08-07 23:02:46]

We're getting very close. I would approve except I'd like to hear your thoughts
about simplifying the permission grammar.

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/p...
File chrome/common/extensions/permissions/socket_permission_data.cc (right):

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.cc:19: host_("*"),
On 2012/08/07 21:31:55, Peng wrote:
> On 2012/08/06 21:04:06, miket wrote:
> > Consider making these special characters (*, :, ., etc.) into string
> constants.
> 
> Done.

I don't see that this is done.

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.cc:79: type_ = NONE;
> Done. I changed it to *this = SocketPermissionData(), Is it OK? (otherwise it
> has to have duplicate in reset and constructor, and we have to make sure they
> are doing same thing, when we modify the initial values.)

Hmmm. This looks weird. Why can't you call reset() in the constructor?

http://codereview.chromium.org/10692160/diff/22023/chrome/common/extensions/p...
File chrome/common/extensions/permissions/api_permission_set_unittest.cc
(right):

http://codereview.chromium.org/10692160/diff/22023/chrome/common/extensions/p...
chrome/common/extensions/permissions/api_permission_set_unittest.cc:111: //
Insert a new detail socket permisssion which will replace the old one.
spelling

http://codereview.chromium.org/10692160/diff/22023/chrome/common/extensions/p...
File chrome/common/extensions/permissions/socket_permission_data.cc (right):

http://codereview.chromium.org/10692160/diff/22023/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.cc:103: // host
should equal to one or more chars + "." + host_.
either "be equal to" or just "equal."

http://codereview.chromium.org/10692160/diff/22023/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.cc:159: port_ < 0 ||
port_ > 65535)
Is port == 0 really a valid port? I'm concerned that on some system it might be
interpreted as something dangerous (wildcard, etc.).

http://codereview.chromium.org/10692160/diff/22023/chrome/common/extensions/p...
File chrome/common/extensions/permissions/socket_permission_unittest.cc (right):

http://codereview.chromium.org/10692160/diff/22023/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_unittest.cc:145:
EXPECT_EQ(Parse("tcp-connect : :  99 "), "tcp-connect:*:99");
Consider that if you accept these kinds of weird strings, you're making a de
facto promise to _always_ support them, and you might be prevented in the future
from doing a refactoring or redesign that rejects them. If you want to be
extremely strict and reject constructs including arbitrary whitespace, or to
make the explicit wildcard mandatory rather than implied, then now is the time
to enforce that.

There's this expression from Larry Wall: "Be strict in what you emit and liberal
in what you accept." I agree entirely with that expression except for the
liberal part. Be strict in what you accept. Then everyone feeding inputs to your
code will be forced to get it right. You will have a smaller, simpler grammar,
making future testing easier, reducing the cost of static analysis, and
simplifying code paths.

http://codereview.chromium.org/10692160/diff/22023/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_unittest.cc:276: }
Great! This is nice because it's a programmatic expression of how this class is
supposed to be used.

[Peng, 2012-08-08 15:40:07]

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/p...
File chrome/common/extensions/permissions/socket_permission_data.cc (right):

http://codereview.chromium.org/10692160/diff/18017/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.cc:19: host_("*"),
On 2012/08/07 23:02:47, miket wrote:
> On 2012/08/07 21:31:55, Peng wrote:
> > On 2012/08/06 21:04:06, miket wrote:
> > > Consider making these special characters (*, :, ., etc.) into string
> > constants.
> > 
> > Done.
> 
> I don't see that this is done.

Done.

http://codereview.chromium.org/10692160/diff/22023/chrome/common/extensions/p...
File chrome/common/extensions/permissions/api_permission_set_unittest.cc
(right):

http://codereview.chromium.org/10692160/diff/22023/chrome/common/extensions/p...
chrome/common/extensions/permissions/api_permission_set_unittest.cc:111: //
Insert a new detail socket permisssion which will replace the old one.
On 2012/08/07 23:02:47, miket wrote:
> spelling

Done.

http://codereview.chromium.org/10692160/diff/22023/chrome/common/extensions/p...
File chrome/common/extensions/permissions/socket_permission_data.cc (right):

http://codereview.chromium.org/10692160/diff/22023/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.cc:103: // host
should equal to one or more chars + "." + host_.
On 2012/08/07 23:02:47, miket wrote:
> either "be equal to" or just "equal."

Done.

http://codereview.chromium.org/10692160/diff/22023/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.cc:159: port_ < 0 ||
port_ > 65535)
On 2012/08/07 23:02:47, miket wrote:
> Is port == 0 really a valid port? I'm concerned that on some system it might
be
> interpreted as something dangerous (wildcard, etc.).

Done.

http://codereview.chromium.org/10692160/diff/22023/chrome/common/extensions/p...
File chrome/common/extensions/permissions/socket_permission_unittest.cc (right):

http://codereview.chromium.org/10692160/diff/22023/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_unittest.cc:145:
EXPECT_EQ(Parse("tcp-connect : :  99 "), "tcp-connect:*:99");
On 2012/08/07 23:02:47, miket wrote:
> Consider that if you accept these kinds of weird strings, you're making a de
> facto promise to _always_ support them, and you might be prevented in the
future
> from doing a refactoring or redesign that rejects them. If you want to be
> extremely strict and reject constructs including arbitrary whitespace, or to
> make the explicit wildcard mandatory rather than implied, then now is the time
> to enforce that.
> 
> There's this expression from Larry Wall: "Be strict in what you emit and
liberal
> in what you accept." I agree entirely with that expression except for the
> liberal part. Be strict in what you accept. Then everyone feeding inputs to
your
> code will be forced to get it right. You will have a smaller, simpler grammar,
> making future testing easier, reducing the cost of static analysis, and
> simplifying code paths.

A good advice. Done.

[Mihai Parparita -not on Chrome, 2012-08-10 00:16:16]

The permission detail format (in both the prefs and the manifest) seems to be 
["<permission_name>", <permission_detail_object>]

Is there a reason why you went with that, instead of:
{"<permission_name>": <permission_detail_object>}

http://codereview.chromium.org/10692160/diff/19098/chrome/browser/extensions/...
File chrome/browser/extensions/api/socket/socket.h (right):

http://codereview.chromium.org/10692160/diff/19098/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket.h:39: TYPE_NONE = 0,
I don't see this used anywhere. It seems preferable to remove it, instead of
somehow having unhandled cases.

http://codereview.chromium.org/10692160/diff/19098/chrome/browser/extensions/...
File chrome/browser/extensions/api/socket/socket_api.cc (right):

http://codereview.chromium.org/10692160/diff/19098/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket_api.cc:177:
SocketPermissionData::OperationType type;
Can you call this variable operation_type? Otherwise the name is too generic,
and risks getting confused with the socket type.

http://codereview.chromium.org/10692160/diff/19098/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket_api.cc:186: type =
SocketPermissionData::NONE;
NOTREACHED() may be more appropriate here (I think we always want to map to a
operation type).

http://codereview.chromium.org/10692160/diff/19098/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket_api.cc:246:
SocketPermissionData::UDP_BIND, address_, port_);
TCP sockets also implement the Bind method. Can you either check at the start of
the function that the socket type is UDP, or add support for a TCP_BIND
operation?

http://codereview.chromium.org/10692160/diff/19098/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket_api.cc:416:
SocketPermission::CheckParam param(SocketPermissionData::UDP_SEND_TO,
Same thing here.

http://codereview.chromium.org/10692160/diff/19098/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket_api.cc:426: } while (false);
I found the break/while(false) pattern hard to read (it's effectively a goto).
Can you use the same early return flow as in
SocketConnectFunction::AsyncWorkStart(), even if it's a bit more repetitive?

http://codereview.chromium.org/10692160/diff/19098/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket_api.cc:526: info.socket_type =
kUnknown;
Also use NOTREACHED here.

http://codereview.chromium.org/10692160/diff/19098/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/10692160/diff/19098/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:489: // Retrieve the API
permissions.
Can you add a comment explaining how permissions and their details are stored
(since there are two possible formats now)? Should also reference it from
SetExtensionPrefPermissionSet.

http://codereview.chromium.org/10692160/diff/19098/chrome/common/extensions/p...
File chrome/common/extensions/permissions/socket_permission_data.cc (right):

http://codereview.chromium.org/10692160/diff/19098/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.cc:26: }  //
namespace
Seems like you can merge these two adjacent unnamed namespaces.

http://codereview.chromium.org/10692160/diff/19098/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.cc:120: if
(lhost.compare(lhost.length() - host_.length(),
You don't seem to check that there's a period before the matched portion, so
fooexample.com would be matched by *.example.com, even if that wasn't the
intent.

http://codereview.chromium.org/10692160/diff/19098/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.cc:151: 
Extra newline.

http://codereview.chromium.org/10692160/diff/19098/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.cc:158: // The first
component can optionally be '*' to match all subdomains.
Wildcards should not be allowed for IP addresses (URL patterns used for host
permissions don't allow it either:
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/common/extensions/u...)

http://codereview.chromium.org/10692160/diff/19098/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.cc:203: if (port_ ==
0)
Can you make the 0 magic value into a ANY_PORT constant?

http://codereview.chromium.org/10692160/diff/19098/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.cc:213: host_
.clear();
Extra space after _

http://codereview.chromium.org/10692160/diff/19098/chrome/common/extensions/p...
File chrome/common/extensions/permissions/socket_permission_data.h (right):

http://codereview.chromium.org/10692160/diff/19098/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.h:31: bool
operator<(const SocketPermissionData& rhs) const;
The C++ style guide says " In particular, do not overload operator== or
operator< just so that your class can be used as a key in an STL container;
instead, you should create equality and comparison functor types when declaring
the container." (see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Operat...)

[Peng, 2012-08-13 16:26:10]

http://codereview.chromium.org/10692160/diff/19098/chrome/browser/extensions/...
File chrome/browser/extensions/api/socket/socket.h (right):

http://codereview.chromium.org/10692160/diff/19098/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket.h:39: TYPE_NONE = 0,
On 2012/08/10 00:16:16, Mihai Parparita wrote:
> I don't see this used anywhere. It seems preferable to remove it, instead of
> somehow having unhandled cases.

Done.

http://codereview.chromium.org/10692160/diff/19098/chrome/browser/extensions/...
File chrome/browser/extensions/api/socket/socket_api.cc (right):

http://codereview.chromium.org/10692160/diff/19098/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket_api.cc:177:
SocketPermissionData::OperationType type;
On 2012/08/10 00:16:16, Mihai Parparita wrote:
> Can you call this variable operation_type? Otherwise the name is too generic,
> and risks getting confused with the socket type.

Done.

http://codereview.chromium.org/10692160/diff/19098/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket_api.cc:186: type =
SocketPermissionData::NONE;
On 2012/08/10 00:16:16, Mihai Parparita wrote:
> NOTREACHED() may be more appropriate here (I think we always want to map to a
> operation type).

Done.

http://codereview.chromium.org/10692160/diff/19098/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket_api.cc:246:
SocketPermissionData::UDP_BIND, address_, port_);
On 2012/08/10 00:16:16, Mihai Parparita wrote:
> TCP sockets also implement the Bind method. Can you either check at the start
of
> the function that the socket type is UDP, or add support for a TCP_BIND
> operation?

Done.

http://codereview.chromium.org/10692160/diff/19098/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket_api.cc:416:
SocketPermission::CheckParam param(SocketPermissionData::UDP_SEND_TO,
On 2012/08/10 00:16:16, Mihai Parparita wrote:
> Same thing here.

Done.

http://codereview.chromium.org/10692160/diff/19098/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket_api.cc:426: } while (false);
On 2012/08/10 00:16:16, Mihai Parparita wrote:
> I found the break/while(false) pattern hard to read (it's effectively a goto).
> Can you use the same early return flow as in
> SocketConnectFunction::AsyncWorkStart(), even if it's a bit more repetitive?

Done.

http://codereview.chromium.org/10692160/diff/19098/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket_api.cc:426: } while (false);
On 2012/08/10 00:16:16, Mihai Parparita wrote:
> I found the break/while(false) pattern hard to read (it's effectively a goto).
> Can you use the same early return flow as in
> SocketConnectFunction::AsyncWorkStart(), even if it's a bit more repetitive?

Done.

http://codereview.chromium.org/10692160/diff/19098/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket_api.cc:526: info.socket_type =
kUnknown;
On 2012/08/10 00:16:16, Mihai Parparita wrote:
> Also use NOTREACHED here.

Done.

http://codereview.chromium.org/10692160/diff/19098/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/10692160/diff/19098/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:489: // Retrieve the API
permissions.
On 2012/08/10 00:16:16, Mihai Parparita wrote:
> Can you add a comment explaining how permissions and their details are stored
> (since there are two possible formats now)? Should also reference it from
> SetExtensionPrefPermissionSet.

Done.

http://codereview.chromium.org/10692160/diff/19098/chrome/common/extensions/p...
File chrome/common/extensions/permissions/socket_permission_data.cc (right):

http://codereview.chromium.org/10692160/diff/19098/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.cc:26: }  //
namespace
On 2012/08/10 00:16:16, Mihai Parparita wrote:
> Seems like you can merge these two adjacent unnamed namespaces.

Done.

http://codereview.chromium.org/10692160/diff/19098/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.cc:120: if
(lhost.compare(lhost.length() - host_.length(),
On 2012/08/10 00:16:16, Mihai Parparita wrote:
> You don't seem to check that there's a period before the matched portion, so
> http://fooexample.com would be matched by *.example.com, even if that wasn't
the
> intent.

Done.

http://codereview.chromium.org/10692160/diff/19098/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.cc:151: 
On 2012/08/10 00:16:16, Mihai Parparita wrote:
> Extra newline.

Done.

http://codereview.chromium.org/10692160/diff/19098/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.cc:158: // The first
component can optionally be '*' to match all subdomains.
On 2012/08/10 00:16:16, Mihai Parparita wrote:
> Wildcards should not be allowed for IP addresses (URL patterns used for host
> permissions don't allow it either:
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/common/extensions/u...)

Done.

http://codereview.chromium.org/10692160/diff/19098/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.cc:203: if (port_ ==
0)
On 2012/08/10 00:16:16, Mihai Parparita wrote:
> Can you make the 0 magic value into a ANY_PORT constant?

Done.

http://codereview.chromium.org/10692160/diff/19098/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.cc:213: host_
.clear();
On 2012/08/10 00:16:16, Mihai Parparita wrote:
> Extra space after _

Done.

http://codereview.chromium.org/10692160/diff/19098/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.cc:213: host_
.clear();
On 2012/08/10 00:16:16, Mihai Parparita wrote:
> Extra space after _

Done.

http://codereview.chromium.org/10692160/diff/19098/chrome/common/extensions/p...
File chrome/common/extensions/permissions/socket_permission_data.h (right):

http://codereview.chromium.org/10692160/diff/19098/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.h:31: bool
operator<(const SocketPermissionData& rhs) const;
On 2012/08/10 00:16:16, Mihai Parparita wrote:
> The C++ style guide says " In particular, do not overload operator== or
> operator< just so that your class can be used as a key in an STL container;
> instead, you should create equality and comparison functor types when
declaring
> the container." (see
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Operat...)

Done. Have to keep them. They are also used by std::set_differences and
std::set_includes. I added some comment here.

[Peng, 2012-08-13 16:31:36]

On 2012/08/10 00:16:16, Mihai Parparita wrote:
> The permission detail format (in both the prefs and the manifest) seems to be 
> ["<permission_name>", <permission_detail_object>]
> 
> Is there a reason why you went with that, instead of:
> {"<permission_name>": <permission_detail_object>}

I think a single key dictionary is little weird. It may confuse apps developers.
It is very likely, developers will write a permission section like:
[ "p1", "p2",
  {"permission_name1" : value,
   "permission_name2" : value,
   ...
  }
]
instead of 
[ "p1", "p2",
  { "permission_name1": value },
  { "permission_name2": value },
  ...
]

So I suggest using 
[ p1, p2 , [p3, [..] or {..} or any_other_format ], p4 ].
> 
>
http://codereview.chromium.org/10692160/diff/19098/chrome/browser/extensions/...
> File chrome/browser/extensions/api/socket/socket.h (right):
> 
>
http://codereview.chromium.org/10692160/diff/19098/chrome/browser/extensions/...
> chrome/browser/extensions/api/socket/socket.h:39: TYPE_NONE = 0,
> I don't see this used anywhere. It seems preferable to remove it, instead of
> somehow having unhandled cases.
> 
>
http://codereview.chromium.org/10692160/diff/19098/chrome/browser/extensions/...
> File chrome/browser/extensions/api/socket/socket_api.cc (right):
> 
>
http://codereview.chromium.org/10692160/diff/19098/chrome/browser/extensions/...
> chrome/browser/extensions/api/socket/socket_api.cc:177:
> SocketPermissionData::OperationType type;
> Can you call this variable operation_type? Otherwise the name is too generic,
> and risks getting confused with the socket type.
> 
>
http://codereview.chromium.org/10692160/diff/19098/chrome/browser/extensions/...
> chrome/browser/extensions/api/socket/socket_api.cc:186: type =
> SocketPermissionData::NONE;
> NOTREACHED() may be more appropriate here (I think we always want to map to a
> operation type).
> 
>
http://codereview.chromium.org/10692160/diff/19098/chrome/browser/extensions/...
> chrome/browser/extensions/api/socket/socket_api.cc:246:
> SocketPermissionData::UDP_BIND, address_, port_);
> TCP sockets also implement the Bind method. Can you either check at the start
of
> the function that the socket type is UDP, or add support for a TCP_BIND
> operation?
> 
>
http://codereview.chromium.org/10692160/diff/19098/chrome/browser/extensions/...
> chrome/browser/extensions/api/socket/socket_api.cc:416:
> SocketPermission::CheckParam param(SocketPermissionData::UDP_SEND_TO,
> Same thing here.
> 
>
http://codereview.chromium.org/10692160/diff/19098/chrome/browser/extensions/...
> chrome/browser/extensions/api/socket/socket_api.cc:426: } while (false);
> I found the break/while(false) pattern hard to read (it's effectively a goto).
> Can you use the same early return flow as in
> SocketConnectFunction::AsyncWorkStart(), even if it's a bit more repetitive?
> 
>
http://codereview.chromium.org/10692160/diff/19098/chrome/browser/extensions/...
> chrome/browser/extensions/api/socket/socket_api.cc:526: info.socket_type =
> kUnknown;
> Also use NOTREACHED here.
> 
>
http://codereview.chromium.org/10692160/diff/19098/chrome/browser/extensions/...
> File chrome/browser/extensions/extension_prefs.cc (right):
> 
>
http://codereview.chromium.org/10692160/diff/19098/chrome/browser/extensions/...
> chrome/browser/extensions/extension_prefs.cc:489: // Retrieve the API
> permissions.
> Can you add a comment explaining how permissions and their details are stored
> (since there are two possible formats now)? Should also reference it from
> SetExtensionPrefPermissionSet.
> 
>
http://codereview.chromium.org/10692160/diff/19098/chrome/common/extensions/p...
> File chrome/common/extensions/permissions/socket_permission_data.cc (right):
> 
>
http://codereview.chromium.org/10692160/diff/19098/chrome/common/extensions/p...
> chrome/common/extensions/permissions/socket_permission_data.cc:26: }  //
> namespace
> Seems like you can merge these two adjacent unnamed namespaces.
> 
>
http://codereview.chromium.org/10692160/diff/19098/chrome/common/extensions/p...
> chrome/common/extensions/permissions/socket_permission_data.cc:120: if
> (lhost.compare(lhost.length() - host_.length(),
> You don't seem to check that there's a period before the matched portion, so
> http://fooexample.com would be matched by *.example.com, even if that wasn't
the
> intent.
> 
>
http://codereview.chromium.org/10692160/diff/19098/chrome/common/extensions/p...
> chrome/common/extensions/permissions/socket_permission_data.cc:151: 
> Extra newline.
> 
>
http://codereview.chromium.org/10692160/diff/19098/chrome/common/extensions/p...
> chrome/common/extensions/permissions/socket_permission_data.cc:158: // The
first
> component can optionally be '*' to match all subdomains.
> Wildcards should not be allowed for IP addresses (URL patterns used for host
> permissions don't allow it either:
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/common/extensions/u...)
> 
>
http://codereview.chromium.org/10692160/diff/19098/chrome/common/extensions/p...
> chrome/common/extensions/permissions/socket_permission_data.cc:203: if (port_
==
> 0)
> Can you make the 0 magic value into a ANY_PORT constant?
> 
>
http://codereview.chromium.org/10692160/diff/19098/chrome/common/extensions/p...
> chrome/common/extensions/permissions/socket_permission_data.cc:213: host_
> .clear();
> Extra space after _
> 
>
http://codereview.chromium.org/10692160/diff/19098/chrome/common/extensions/p...
> File chrome/common/extensions/permissions/socket_permission_data.h (right):
> 
>
http://codereview.chromium.org/10692160/diff/19098/chrome/common/extensions/p...
> chrome/common/extensions/permissions/socket_permission_data.h:31: bool
> operator<(const SocketPermissionData& rhs) const;
> The C++ style guide says " In particular, do not overload operator== or
> operator< just so that your class can be used as a key in an STL container;
> instead, you should create equality and comparison functor types when
declaring
> the container." (see
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Operat...)

[Aaron Boodman, 2012-08-13 22:33:27]

On 2012/08/13 16:31:36, Peng wrote:
> On 2012/08/10 00:16:16, Mihai Parparita wrote:
> > The permission detail format (in both the prefs and the manifest) seems to
be 
> > ["<permission_name>", <permission_detail_object>]
> > 
> > Is there a reason why you went with that, instead of:
> > {"<permission_name>": <permission_detail_object>}
> 
> I think a single key dictionary is little weird. It may confuse apps
developers.
> It is very likely, developers will write a permission section like:
> [ "p1", "p2",
>   {"permission_name1" : value,
>    "permission_name2" : value,
>    ...
>   }
> ]

We can easily validate that and throw an error, though. And I don't really see a
multi-level array as any less confusing.

[Mihai Parparita -not on Chrome, 2012-08-14 00:29:56]

My main reason for preferring a single-key dictionary is that we take
advantage of the built-in structure of the dictionary: the key is the
permission name, and the value is its details. Whereas with a heterogenous
array of length 2, it smells like a hack, std::pair-style.

Mihai

On Mon, Aug 13, 2012 at 3:33 PM, <aa@chromium.org> wrote:

> On 2012/08/13 16:31:36, Peng wrote:
>
>> On 2012/08/10 00:16:16, Mihai Parparita wrote:
>> > The permission detail format (in both the prefs and the manifest) seems
>> to
>>
> be
>
>> > ["<permission_name>", <permission_detail_object>]
>> >
>> > Is there a reason why you went with that, instead of:
>> > {"<permission_name>": <permission_detail_object>}
>>
>
>  I think a single key dictionary is little weird. It may confuse apps
>>
> developers.
>
>> It is very likely, developers will write a permission section like:
>> [ "p1", "p2",
>>    {"permission_name1" : value,
>>     "permission_name2" : value,
>>     ...
>>    }
>> ]
>>
>
> We can easily validate that and throw an error, though. And I don't really
> see a
> multi-level array as any less confusing.
>
>
http://codereview.chromium.**org/10692160/<http://codereview.chromium.org/106...
>

[Mihai Parparita -not on Chrome, 2012-08-14 00:53:09]

Just some small additional nits.

http://codereview.chromium.org/10692160/diff/27043/chrome/browser/extensions/...
File chrome/browser/extensions/api/socket/socket.h (right):

http://codereview.chromium.org/10692160/diff/27043/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket.h:77: virtual SocketType
socket_type() const = 0;
I believe that per the style guide, hacker_style naming should only be used for
simple getter/setter methods that can be inlined, and virtual methods are by
definition not that simple.

http://codereview.chromium.org/10692160/diff/27043/chrome/common/extensions/p...
File chrome/common/extensions/permissions/socket_permission_data.cc (right):

http://codereview.chromium.org/10692160/diff/27043/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.cc:67: }
It's convention to add a "//  namespace" closing comment here.

http://codereview.chromium.org/10692160/diff/27043/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.cc:116: // Do not
wildcard part of IP address.
This doesn't seem to be covered by tests.

[Peng, 2012-08-14 17:43:31]

http://codereview.chromium.org/10692160/diff/27043/chrome/browser/extensions/...
File chrome/browser/extensions/api/socket/socket.h (right):

http://codereview.chromium.org/10692160/diff/27043/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket.h:77: virtual SocketType
socket_type() const = 0;
On 2012/08/14 00:53:09, Mihai Parparita wrote:
> I believe that per the style guide, hacker_style naming should only be used
for
> simple getter/setter methods that can be inlined, and virtual methods are by
> definition not that simple.

Done.

http://codereview.chromium.org/10692160/diff/27043/chrome/common/extensions/p...
File chrome/common/extensions/permissions/socket_permission_data.cc (right):

http://codereview.chromium.org/10692160/diff/27043/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.cc:67: }
On 2012/08/14 00:53:09, Mihai Parparita wrote:
> It's convention to add a "//  namespace" closing comment here.

Done.

http://codereview.chromium.org/10692160/diff/27043/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.cc:116: // Do not
wildcard part of IP address.
On 2012/08/14 00:53:09, Mihai Parparita wrote:
> This doesn't seem to be covered by tests.
Done. Added a test case for it.

[Peng, 2012-08-14 17:44:12]

I updated the CL to use the single ket dict. Please check it. Thanks.

On 2012/08/14 00:29:56, Mihai Parparita wrote:
> My main reason for preferring a single-key dictionary is that we take
> advantage of the built-in structure of the dictionary: the key is the
> permission name, and the value is its details. Whereas with a heterogenous
> array of length 2, it smells like a hack, std::pair-style.
> 
> Mihai
> 
> On Mon, Aug 13, 2012 at 3:33 PM, <mailto:aa@chromium.org> wrote:
> 
> > On 2012/08/13 16:31:36, Peng wrote:
> >
> >> On 2012/08/10 00:16:16, Mihai Parparita wrote:
> >> > The permission detail format (in both the prefs and the manifest) seems
> >> to
> >>
> > be
> >
> >> > ["<permission_name>", <permission_detail_object>]
> >> >
> >> > Is there a reason why you went with that, instead of:
> >> > {"<permission_name>": <permission_detail_object>}
> >>
> >
> >  I think a single key dictionary is little weird. It may confuse apps
> >>
> > developers.
> >
> >> It is very likely, developers will write a permission section like:
> >> [ "p1", "p2",
> >>    {"permission_name1" : value,
> >>     "permission_name2" : value,
> >>     ...
> >>    }
> >> ]
> >>
> >
> > We can easily validate that and throw an error, though. And I don't really
> > see a
> > multi-level array as any less confusing.
> >
> >
>
http://codereview.chromium.**org/10692160/%3Chttp://codereview.chromium.org/1...>
> >

[Mihai Parparita -not on Chrome, 2012-08-15 00:01:12]

LGTM

[Mihai Parparita -not on Chrome, 2012-08-15 00:02:26]

BTW, the tryserver failures look legitimate.

Mihai

On Tue, Aug 14, 2012 at 5:01 PM, <mihaip@chromium.org> wrote:

> LGTM
>
>
http://codereview.chromium.**org/10692160/<http://codereview.chromium.org/106...
>

[commit-bot: I haz the power, 2012-08-15 00:16:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/penghuang@chromium.org/10692160/23016

[commit-bot: I haz the power, 2012-08-15 00:17:05]

Failed to apply patch for
chrome/common/extensions/permissions/api_permission.cc:
While running patch -p1 --forward --force;
patching file chrome/common/extensions/permissions/api_permission.cc
Hunk #5 FAILED at 240.
Hunk #6 succeeded at 267 (offset 1 line).
Hunk #7 succeeded at 276 (offset 1 line).
1 out of 7 hunks FAILED -- saving rejects to file
chrome/common/extensions/permissions/api_permission.cc.rej

[commit-bot: I haz the power, 2012-08-15 00:23:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/penghuang@chromium.org/10692160/36072

[commit-bot: I haz the power, 2012-08-15 00:23:31]

Presubmit check for 10692160-36072 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Warnings **
Found line ending with white spaces in:

***************
chrome/common/extensions/permissions/socket_permission_unittest.cc, line 190
***************

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 3.3s to calculate.

[Peng, 2012-08-15 00:30:35]

Hi Ben,  Could you please review changes in chrome/test/data/extensions/* and
chrome/chrome_*.gypi? Thanks.

[Mihai Parparita -not on Chrome, 2012-08-15 00:33:47]

On Tue, Aug 14, 2012 at 5:30 PM, <penghuang@chromium.org> wrote:

> Hi Ben,  Could you please review changes in chrome/test/data/extensions/*
>

BTW, the test data directory doesn't need an OWNERS approval, anyone can
commit there:
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/test/data/OWNERS?view=...

Mihai

[Peng, 2012-08-15 02:59:46]

Hi Ben,

Could you please do an owner review for changes in chrome/chrome_*.gypi? Please
ignore my previous comment (chrome/test/data/extensions/* does not need owner
review). I am sorry about the misstake. Thanks.



On 2012/08/15 00:33:47, Mihai Parparita wrote:
> On Tue, Aug 14, 2012 at 5:30 PM, <mailto:penghuang@chromium.org> wrote:
> 
> > Hi Ben,  Could you please review changes in chrome/test/data/extensions/*
> >
> 
> BTW, the test data directory doesn't need an OWNERS approval, anyone can
> commit there:
>
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/test/data/OWNERS?view=...
> 
> Mihai

[Ben Goodger (Google), 2012-08-15 15:46:21]

gyp/data changes lgtm

[commit-bot: I haz the power, 2012-08-15 16:00:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/penghuang@chromium.org/10692160/16169

[commit-bot: I haz the power, 2012-08-15 16:53:51]

Try job failure for 10692160-16169 (retry) on mac_rel for step "unit_tests".
It's a second try, previously, step "unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-08-15 18:52:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/penghuang@chromium.org/10692160/34086

[commit-bot: I haz the power, 2012-08-15 19:37:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/penghuang@chromium.org/10692160/37002

[commit-bot: I haz the power, 2012-08-15 20:07:05]

Try job failure for 10692160-37002 on linux_chromeos for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-08-15 20:15:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/penghuang@chromium.org/10692160/25016

[commit-bot: I haz the power, 2012-08-15 22:26:36]

Try job failure for 10692160-25016 (retry) on mac_rel for step "unit_tests".
It's a second try, previously, step "unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-08-15 22:51:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/penghuang@chromium.org/10692160/25016

[commit-bot: I haz the power, 2012-08-16 00:09:25]

Try job failure for 10692160-25016 (retry) on mac_rel for step "unit_tests".
It's a second try, previously, step "unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-08-16 16:17:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/penghuang@chromium.org/10692160/23023

[commit-bot: I haz the power, 2012-08-16 18:34:20]

Try job failure for 10692160-23023 (retry) on win_rel for step
"content_browsertests".
It's a second try, previously, steps "interactive_ui_tests,
content_browsertests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-08-16 18:45:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/penghuang@chromium.org/10692160/23023

[commit-bot: I haz the power, 2012-08-16 20:22:57]

Change committed as 151956