Enable permission warnings from ManifestHandlers.

Enable permission warnings from ManifestHandlers.

BUG=293755, 247857
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=235332

[rpaquay, 2013-10-29 21:05:50]

yoz@: This is WIP for addressing permissions messages from manifest handlers.
What do you think?

A next step is to figure out the extensionprefs for diff'ing permissions when an
extension is updated.

Another one is to figure out optional permissions.

Am I missing something else?

[Yoyo Zhou, 2013-10-29 21:12:46]

On 2013/10/29 21:05:50, rpaquay wrote:
> yoz@: This is WIP for addressing permissions messages from manifest handlers.
> What do you think?

Looks fine so far, but...

> A next step is to figure out the extensionprefs for diff'ing permissions when
an
> extension is updated.

This is the most important one. To do this, you need to determine what to store
in the prefs for "granted permissions", i.e. the permissions the user has
accepted.

> Another one is to figure out optional permissions.

This is optional (no pun intended), unless you need it for sockets or for other
use cases.

[rpaquay, 2013-10-29 22:01:10]

> > A next step is to figure out the extensionprefs for diff'ing permissions
when
> an
> > extension is updated.
> 
> This is the most important one. To do this, you need to determine what to
store
> in the prefs for "granted permissions", i.e. the permissions the user has
> accepted.

Ok... Looks like one option is adding another member variable (e.g.
manifest_handlers_permissions_) to "PermissionSet", then update the code to take
that new member variable into account. manifest_handlers would be instance of of
new class similar to "APIPermissionSet". What do you think?

[Yoyo Zhou, 2013-10-30 01:01:01]

On 2013/10/29 22:01:10, rpaquay wrote:
> > > A next step is to figure out the extensionprefs for diff'ing permissions
> when
> > an
> > > extension is updated.
> > 
> > This is the most important one. To do this, you need to determine what to
> store
> > in the prefs for "granted permissions", i.e. the permissions the user has
> > accepted.
> 
> Ok... Looks like one option is adding another member variable (e.g.
> manifest_handlers_permissions_) to "PermissionSet", then update the code to
take
> that new member variable into account. manifest_handlers would be instance of
of
> new class similar to "APIPermissionSet". What do you think?

Yes, it seems like adding a new field to PermissionSet will work best. I'd call
it manifest_permissions_ or manifest_warnings_. We could have a new class
similar to APIPermissionSet to enumerate these, but most ManifestHandlers don't
have warnings so it will be have different defaults.

[rpaquay, 2013-11-05 21:50:25]

PTAL. It is still work in progress, but the CL is getting big, and I would like
to make sure I am on the right track.

[Yoyo Zhou, 2013-11-06 04:02:36]

I think this looks mostly sane. I would also want to see the changes to
ChromePermissionMessageProvider, which is the main avenue by which permission
warnings are consumed.

https://codereview.chromium.org/51433002/diff/100001/chrome/common/extensions...
File chrome/common/extensions/api/sockets/sockets_handler.cc (right):

https://codereview.chromium.org/51433002/diff/100001/chrome/common/extensions...
chrome/common/extensions/api/sockets/sockets_handler.cc:186: // TODO(rpaquay):
This function and callees is (almost) a copy/paste
Have you thought about how extensions that currently have socket permissions are
going to be handled when they update or Chrome updates? Will this be
backward-compatible at all?

https://codereview.chromium.org/51433002/diff/100001/extensions/common/permis...
File extensions/common/permissions/api_permission_set.cc (right):

https://codereview.chromium.org/51433002/diff/100001/extensions/common/permis...
extensions/common/permissions/api_permission_set.cc:463: // permission should be
a string or a single key dict.
Is this still true?

https://codereview.chromium.org/51433002/diff/100001/extensions/common/permis...
File extensions/common/permissions/api_permission_set.h (right):

https://codereview.chromium.org/51433002/diff/100001/extensions/common/permis...
extensions/common/permissions/api_permission_set.h:208: class
ManifestPermissionSet {
Would it make more sense to have the generic at this level, i.e.

Set<APIPermission>
Set<ManifestPermission>

(we'd need a better name for the container - PermissionSet is unfortunately
already taken)

You could make ParseFromJSON, etc. template specializations.

[rpaquay, 2013-11-06 16:16:16]

Thanks for the review, I will keep moving in that direction then.

https://codereview.chromium.org/51433002/diff/100001/chrome/common/extensions...
File chrome/common/extensions/api/sockets/sockets_handler.cc (right):

https://codereview.chromium.org/51433002/diff/100001/chrome/common/extensions...
chrome/common/extensions/api/sockets/sockets_handler.cc:186: // TODO(rpaquay):
This function and callees is (almost) a copy/paste
On 2013/11/06 04:02:36, Yoyo Zhou wrote:
> Have you thought about how extensions that currently have socket permissions
are
> going to be handled when they update or Chrome updates? Will this be
> backward-compatible at all? 

Good question. I guess unless we do extra work, there will be additional warning
messages on update.

https://codereview.chromium.org/51433002/diff/100001/extensions/common/permis...
File extensions/common/permissions/api_permission_set.cc (right):

https://codereview.chromium.org/51433002/diff/100001/extensions/common/permis...
extensions/common/permissions/api_permission_set.cc:463: // permission should be
a string or a single key dict.
On 2013/11/06 04:02:36, Yoyo Zhou wrote:
> Is this still true?

I am not 100% sure yet, but i think it is. Either the manifest permission has no
details, and we store just the name, or it has additional detail, and we store a
single key dict { 'name': <arbitrary data> }

https://codereview.chromium.org/51433002/diff/100001/extensions/common/permis...
File extensions/common/permissions/api_permission_set.h (right):

https://codereview.chromium.org/51433002/diff/100001/extensions/common/permis...
extensions/common/permissions/api_permission_set.h:208: class
ManifestPermissionSet {
On 2013/11/06 04:02:36, Yoyo Zhou wrote:
> Would it make more sense to have the generic at this level, i.e.
> 
> Set<APIPermission>
> Set<ManifestPermission>
> 
> (we'd need a better name for the container - PermissionSet is unfortunately
> already taken)
> 
> You could make ParseFromJSON, etc. template specializations.

I *think* it might make sense. At the very least, we could have a common base
class (template) and expose the specific behavior (if there is any) in
sub-classes.

[rpaquay, 2013-11-07 19:33:45]

PTAL. I think I am done from a functionality point of view. I need to cleanup
the code and add unit tests. What do you think?

[rpaquay, 2013-11-07 23:55:07]

(Patch set #4 rebased with move of manifest_handler to /extensions/common)

[Yoyo Zhou, 2013-11-09 01:15:29]

https://chromiumcodereview.appspot.com/51433002/diff/210001/chrome/browser/ex...
File chrome/browser/extensions/api/permissions/permissions_api_helpers.cc
(right):

https://chromiumcodereview.appspot.com/51433002/diff/210001/chrome/browser/ex...
chrome/browser/extensions/api/permissions/permissions_api_helpers.cc:60: // to
apps/extensions.
via the permissions API?

https://chromiumcodereview.appspot.com/51433002/diff/210001/chrome/browser/ex...
File chrome/browser/extensions/extension_prefs.cc (right):

https://chromiumcodereview.appspot.com/51433002/diff/210001/chrome/browser/ex...
chrome/browser/extensions/extension_prefs.cc:547: // Retrieve the Manifest Keys
permissions. Please refer
nit: add "to"

https://chromiumcodereview.appspot.com/51433002/diff/210001/chrome/common/ext...
File chrome/common/extensions/api/sockets/sockets_handler.cc (right):

https://chromiumcodereview.appspot.com/51433002/diff/210001/chrome/common/ext...
chrome/common/extensions/api/sockets/sockets_handler.cc:35: class
SocketsManifestPermission : public ManifestPermission {
Do you anticipate something like SetDisjunctionPermission being able to
generalize this? Certainly we have other complex permissions that may want to be
specified by manifest keys.

https://chromiumcodereview.appspot.com/51433002/diff/210001/chrome/common/ext...
chrome/common/extensions/api/sockets/sockets_handler.cc:245:
scoped_ptr<SocketsManifestData> data_;
This looks like unusual ownership. ManifestDatas are generally owned by the
Extension.

https://chromiumcodereview.appspot.com/51433002/diff/210001/extensions/common...
File extensions/common/permissions/api_permission_set.h (right):

https://chromiumcodereview.appspot.com/51433002/diff/210001/extensions/common...
extensions/common/permissions/api_permission_set.h:174: class ManifestPermission
{
This has grown large enough to need its own file, separate from
api_permission_set.h

https://chromiumcodereview.appspot.com/51433002/diff/210001/extensions/common...
extensions/common/permissions/api_permission_set.h:229: class
ManifestPermissionSet {
It looks like there is really a lot of duplicated code here. Can we templatize a
set of permissions (API or Manifest), as I mentioned in the last review?

[rpaquay, 2013-11-11 18:37:35]

Comments inline. Let me know if it is ok to commit like this and address
comments in a follow up CL, or if I need to address things in this CL. Thanks!

https://codereview.chromium.org/51433002/diff/210001/chrome/browser/extension...
File chrome/browser/extensions/api/permissions/permissions_api_helpers.cc
(right):

https://codereview.chromium.org/51433002/diff/210001/chrome/browser/extension...
chrome/browser/extensions/api/permissions/permissions_api_helpers.cc:60: // to
apps/extensions.
On 2013/11/09 01:15:30, Yoyo Zhou wrote:
> via the permissions API?
Correct. I think we'd need to revamp the permissions API to support this
scenario. For example, the current spec does not even mention anything about
complex permissions, it just says "returns an array of strings" (see
http://developer.chrome.com/apps/permissions.html#type-Permissions).

https://codereview.chromium.org/51433002/diff/210001/chrome/common/extensions...
File chrome/common/extensions/api/sockets/sockets_handler.cc (right):

https://codereview.chromium.org/51433002/diff/210001/chrome/common/extensions...
chrome/common/extensions/api/sockets/sockets_handler.cc:35: class
SocketsManifestPermission : public ManifestPermission {
On 2013/11/09 01:15:30, Yoyo Zhou wrote:
> Do you anticipate something like SetDisjunctionPermission being able to
> generalize this? Certainly we have other complex permissions that may want to
be
> specified by manifest keys.

I would say probably at some point. I don't know of any other current work to
create manifest permission though.

https://codereview.chromium.org/51433002/diff/210001/chrome/common/extensions...
chrome/common/extensions/api/sockets/sockets_handler.cc:245:
scoped_ptr<SocketsManifestData> data_;
On 2013/11/09 01:15:30, Yoyo Zhou wrote:
> This looks like unusual ownership. ManifestDatas are generally owned by the
> Extension.

The "initial" value is a clone of the extension manifest data, but after that
(e.g. union, diff, etc.) it is indeed not related to the extension manifest
data. What would you suggest? Another intermediate class (I am running out of
names, because there are so many SocketXxxData, Info, Entry etc. at this point
:)?

https://codereview.chromium.org/51433002/diff/210001/extensions/common/permis...
File extensions/common/permissions/api_permission_set.h (right):

https://codereview.chromium.org/51433002/diff/210001/extensions/common/permis...
extensions/common/permissions/api_permission_set.h:174: class ManifestPermission
{
On 2013/11/09 01:15:30, Yoyo Zhou wrote:
> This has grown large enough to need its own file, separate from
> api_permission_set.h

Agreed, I have been working on this in a follow up CL (along with unit tests and
set unification).

https://codereview.chromium.org/51433002/diff/210001/extensions/common/permis...
extensions/common/permissions/api_permission_set.h:229: class
ManifestPermissionSet {
On 2013/11/09 01:15:30, Yoyo Zhou wrote:
> It looks like there is really a lot of duplicated code here. Can we templatize
a
> set of permissions (API or Manifest), as I mentioned in the last review?

Agreed. I have been working on this (along with unit tests) in a follow up CL.

[Yoyo Zhou, 2013-11-12 02:39:28]

mostly nits, but the last comment especially - I think we should put things in
the appropriate files now rather than later.

https://codereview.chromium.org/51433002/diff/210001/chrome/browser/extension...
File chrome/browser/extensions/extension_prefs.cc (right):

https://codereview.chromium.org/51433002/diff/210001/chrome/browser/extension...
chrome/browser/extensions/extension_prefs.cc:575: // Set the API permissions.
Or manifest permissions.

https://codereview.chromium.org/51433002/diff/210001/chrome/browser/extension...
chrome/browser/extensions/extension_prefs.cc:584: static ListValue*
CreatePermisionList(const T& permissions) {
typo: Permission

https://codereview.chromium.org/51433002/diff/210001/chrome/common/extensions...
File chrome/common/extensions/api/sockets/sockets_handler.cc (right):

https://codereview.chromium.org/51433002/diff/210001/chrome/common/extensions...
chrome/common/extensions/api/sockets/sockets_handler.cc:35: class
SocketsManifestPermission : public ManifestPermission {
On 2013/11/11 18:37:35, rpaquay wrote:
> On 2013/11/09 01:15:30, Yoyo Zhou wrote:
> > Do you anticipate something like SetDisjunctionPermission being able to
> > generalize this? Certainly we have other complex permissions that may want
to
> be
> > specified by manifest keys.
> 
> I would say probably at some point. I don't know of any other current work to
> create manifest permission though.

Okay.

https://codereview.chromium.org/51433002/diff/210001/chrome/common/extensions...
File chrome/common/extensions/api/sockets/sockets_handler.h (right):

https://codereview.chromium.org/51433002/diff/210001/chrome/common/extensions...
chrome/common/extensions/api/sockets/sockets_handler.h:37: kNone = 0x0000,
nit: we usually write (1<<0), (1<<1), etc. for these kind of bitmasks.

https://codereview.chromium.org/51433002/diff/210001/chrome/common/extensions...
chrome/common/extensions/api/sockets/sockets_handler.h:77: bool is_udp() const {
return (kinds_ & kUdpPermission) != 0; }
Is "is" or "has" better for this if it's a bitmask?

https://codereview.chromium.org/51433002/diff/210001/chrome/common/extensions...
File chrome/common/extensions/chrome_manifest_permission_set.cc (right):

https://codereview.chromium.org/51433002/diff/210001/chrome/common/extensions...
chrome/common/extensions/chrome_manifest_permission_set.cc:66: // TODO(rpaquay):
Update name of local variables to reflect we are dealing
Does this still need to be done?

https://codereview.chromium.org/51433002/diff/210001/extensions/common/manife...
File extensions/common/manifest_handler.h (right):

https://codereview.chromium.org/51433002/diff/210001/extensions/common/manife...
extensions/common/manifest_handler.h:69: virtual ManifestPermission*
CreatePermission();
These new functions need documentation.

https://codereview.chromium.org/51433002/diff/210001/extensions/common/permis...
File extensions/common/permissions/api_permission_set.h (right):

https://codereview.chromium.org/51433002/diff/210001/extensions/common/permis...
extensions/common/permissions/api_permission_set.h:174: class ManifestPermission
{
On 2013/11/11 18:37:35, rpaquay wrote:
> On 2013/11/09 01:15:30, Yoyo Zhou wrote:
> > This has grown large enough to need its own file, separate from
> > api_permission_set.h
> 
> Agreed, I have been working on this in a follow up CL (along with unit tests
and
> set unification).

I think this is important enough that it should be done in this CL.

[rpaquay, 2013-11-12 21:40:31]

I think I addressed all code review feedback comments in Patchset #5. I also
added unit tests for ManifestPermissionSet. Let me know if there is anything
else.

https://codereview.chromium.org/51433002/diff/210001/chrome/browser/extension...
File chrome/browser/extensions/extension_prefs.cc (right):

https://codereview.chromium.org/51433002/diff/210001/chrome/browser/extension...
chrome/browser/extensions/extension_prefs.cc:547: // Retrieve the Manifest Keys
permissions. Please refer
On 2013/11/09 01:15:30, Yoyo Zhou wrote:
> nit: add "to"

Done.

https://codereview.chromium.org/51433002/diff/210001/chrome/browser/extension...
chrome/browser/extensions/extension_prefs.cc:575: // Set the API permissions.
On 2013/11/12 02:39:29, Yoyo Zhou wrote:
> Or manifest permissions.

Done.

https://codereview.chromium.org/51433002/diff/210001/chrome/browser/extension...
chrome/browser/extensions/extension_prefs.cc:584: static ListValue*
CreatePermisionList(const T& permissions) {
On 2013/11/12 02:39:29, Yoyo Zhou wrote:
> typo: Permission

Done.

https://codereview.chromium.org/51433002/diff/210001/chrome/common/extensions...
File chrome/common/extensions/api/sockets/sockets_handler.h (right):

https://codereview.chromium.org/51433002/diff/210001/chrome/common/extensions...
chrome/common/extensions/api/sockets/sockets_handler.h:37: kNone = 0x0000,
On 2013/11/12 02:39:29, Yoyo Zhou wrote:
> nit: we usually write (1<<0), (1<<1), etc. for these kind of bitmasks.

Done.

https://codereview.chromium.org/51433002/diff/210001/chrome/common/extensions...
chrome/common/extensions/api/sockets/sockets_handler.h:77: bool is_udp() const {
return (kinds_ & kUdpPermission) != 0; }
On 2013/11/12 02:39:29, Yoyo Zhou wrote:
> Is "is" or "has" better for this if it's a bitmask?

Done.

https://codereview.chromium.org/51433002/diff/210001/chrome/common/extensions...
File chrome/common/extensions/chrome_manifest_permission_set.cc (right):

https://codereview.chromium.org/51433002/diff/210001/chrome/common/extensions...
chrome/common/extensions/chrome_manifest_permission_set.cc:66: // TODO(rpaquay):
Update name of local variables to reflect we are dealing
On 2013/11/12 02:39:29, Yoyo Zhou wrote:
> Does this still need to be done?
Nope. Removed comment.

https://codereview.chromium.org/51433002/diff/210001/extensions/common/manife...
File extensions/common/manifest_handler.h (right):

https://codereview.chromium.org/51433002/diff/210001/extensions/common/manife...
extensions/common/manifest_handler.h:69: virtual ManifestPermission*
CreatePermission();
On 2013/11/12 02:39:29, Yoyo Zhou wrote:
> These new functions need documentation.

Done.

https://codereview.chromium.org/51433002/diff/210001/extensions/common/permis...
File extensions/common/permissions/api_permission_set.h (right):

https://codereview.chromium.org/51433002/diff/210001/extensions/common/permis...
extensions/common/permissions/api_permission_set.h:174: class ManifestPermission
{
On 2013/11/12 02:39:29, Yoyo Zhou wrote:
> On 2013/11/11 18:37:35, rpaquay wrote:
> > On 2013/11/09 01:15:30, Yoyo Zhou wrote:
> > > This has grown large enough to need its own file, separate from
> > > api_permission_set.h
> > 
> > Agreed, I have been working on this in a follow up CL (along with unit tests
> and
> > set unification).
> 
> I think this is important enough that it should be done in this CL.

Done.

https://codereview.chromium.org/51433002/diff/210001/extensions/common/permis...
extensions/common/permissions/api_permission_set.h:229: class
ManifestPermissionSet {
On 2013/11/11 18:37:35, rpaquay wrote:
> On 2013/11/09 01:15:30, Yoyo Zhou wrote:
> > It looks like there is really a lot of duplicated code here. Can we
templatize
> a
> > set of permissions (API or Manifest), as I mentioned in the last review?
> 
> Agreed. I have been working on this (along with unit tests) in a follow up CL.

The latest patchset (#5) addresses this to a large extent. The sub-classes are
still there, but they are very small now.

[Yoyo Zhou, 2013-11-13 02:57:06]

https://chromiumcodereview.appspot.com/51433002/diff/210001/chrome/common/ext...
File chrome/common/extensions/api/sockets/sockets_handler.cc (right):

https://chromiumcodereview.appspot.com/51433002/diff/210001/chrome/common/ext...
chrome/common/extensions/api/sockets/sockets_handler.cc:245:
scoped_ptr<SocketsManifestData> data_;
On 2013/11/11 18:37:35, rpaquay wrote:
> On 2013/11/09 01:15:30, Yoyo Zhou wrote:
> > This looks like unusual ownership. ManifestDatas are generally owned by the
> > Extension.
> 
> The "initial" value is a clone of the extension manifest data, but after that
> (e.g. union, diff, etc.) it is indeed not related to the extension manifest
> data. What would you suggest? Another intermediate class (I am running out of
> names, because there are so many SocketXxxData, Info, Entry etc. at this point
> :)?

(SocketPermissionData already exists, but it seems like a logical name. But see
below.)

Here's how I think of it. Extension owns a PermissionsData, which holds the
PermissionSets which contain Permissions. Extension also owns various
ManifestDatas. Rather than having the ManifestPermission own a ManifestData,
it's better to have it the other way around - the ManifestData is a wrapper
around the ManifestPermission.

https://chromiumcodereview.appspot.com/51433002/diff/670001/chrome/common/ext...
File chrome/common/extensions/api/sockets/sockets_handler.h (right):

https://chromiumcodereview.appspot.com/51433002/diff/670001/chrome/common/ext...
chrome/common/extensions/api/sockets/sockets_handler.h:60: // Stores this into a
new created |value|.
nit: |value| would be used to name an argument. Say Value instead.

https://chromiumcodereview.appspot.com/51433002/diff/670001/extensions/common...
File extensions/common/permissions/manifest_permission_set_unittest.cc (right):

https://chromiumcodereview.appspot.com/51433002/diff/670001/extensions/common...
extensions/common/permissions/manifest_permission_set_unittest.cc:130:
ManifestPermissionSet permssions1;
typo: permissions
(throughout this file)

[rpaquay, 2013-11-13 21:28:55]

https://codereview.chromium.org/51433002/diff/210001/chrome/common/extensions...
File chrome/common/extensions/api/sockets/sockets_handler.cc (right):

https://codereview.chromium.org/51433002/diff/210001/chrome/common/extensions...
chrome/common/extensions/api/sockets/sockets_handler.cc:245:
scoped_ptr<SocketsManifestData> data_;
On 2013/11/13 02:57:07, Yoyo Zhou wrote:
> On 2013/11/11 18:37:35, rpaquay wrote:
> > On 2013/11/09 01:15:30, Yoyo Zhou wrote:
> > > This looks like unusual ownership. ManifestDatas are generally owned by
the
> > > Extension.
> > 
> > The "initial" value is a clone of the extension manifest data, but after
that
> > (e.g. union, diff, etc.) it is indeed not related to the extension manifest
> > data. What would you suggest? Another intermediate class (I am running out
of
> > names, because there are so many SocketXxxData, Info, Entry etc. at this
point
> > :)?
> 
> (SocketPermissionData already exists, but it seems like a logical name. But
see
> below.)
> 
> Here's how I think of it. Extension owns a PermissionsData, which holds the
> PermissionSets which contain Permissions. Extension also owns various
> ManifestDatas. Rather than having the ManifestPermission own a ManifestData,
> it's better to have it the other way around - the ManifestData is a wrapper
> around the ManifestPermission.

This makes sense. Patchset #6 swaps ownership between SocketsManifestPermission
and SocketsManifestData, as well as split each class (and
SocketsManifestHandler) into their own source files.

https://codereview.chromium.org/51433002/diff/670001/chrome/common/extensions...
File chrome/common/extensions/api/sockets/sockets_handler.h (right):

https://codereview.chromium.org/51433002/diff/670001/chrome/common/extensions...
chrome/common/extensions/api/sockets/sockets_handler.h:60: // Stores this into a
new created |value|.
On 2013/11/13 02:57:07, Yoyo Zhou wrote:
> nit: |value| would be used to name an argument. Say Value instead.

Done.

https://codereview.chromium.org/51433002/diff/670001/extensions/common/permis...
File extensions/common/permissions/manifest_permission_set_unittest.cc (right):

https://codereview.chromium.org/51433002/diff/670001/extensions/common/permis...
extensions/common/permissions/manifest_permission_set_unittest.cc:130:
ManifestPermissionSet permssions1;
On 2013/11/13 02:57:07, Yoyo Zhou wrote:
> typo: permissions
> (throughout this file)

Done.

[Yoyo Zhou, 2013-11-14 00:26:54]

LGTM

https://chromiumcodereview.appspot.com/51433002/diff/1010001/chrome/common/ex...
File chrome/common/extensions/api/sockets/sockets_manifest_handler.cc (right):

https://chromiumcodereview.appspot.com/51433002/diff/1010001/chrome/common/ex...
chrome/common/extensions/api/sockets/sockets_manifest_handler.cc:41: if (data) {
nit: don't need braces here

https://chromiumcodereview.appspot.com/51433002/diff/1010001/chrome/common/ex...
File chrome/common/extensions/api/sockets/sockets_manifest_permission.cc
(right):

https://chromiumcodereview.appspot.com/51433002/diff/1010001/chrome/common/ex...
chrome/common/extensions/api/sockets/sockets_manifest_permission.cc:38:
std::vector<InstallWarning>* install_warnings,
You don't use this at all.

https://chromiumcodereview.appspot.com/51433002/diff/1010001/chrome/common/ex...
chrome/common/extensions/api/sockets/sockets_manifest_permission.cc:113: // from
|extensions::SocketPermission|.
nit: no |

[rpaquay, 2013-11-14 00:32:52]

pkotwicz@chromium.org: Please review (trivial) changes in 
chrome/browser/themes/theme_syncable_service_unittest.cc

cdn@chromium.org: Please review changes in 
chrome/common/extensions/extension_messages.h

[pkotwicz, 2013-11-14 00:54:32]

chrome/browser/themes LGTM

[Cris Neckar, 2013-11-14 02:14:54]

On 2013/11/14 00:54:32, pkotwicz wrote:
> chrome/browser/themes LGTM

IPC changes LGTM

[commit-bot: I haz the power, 2013-11-14 03:57:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rpaquay@chromium.org/51433002/1310001

[commit-bot: I haz the power, 2013-11-14 04:19:46]

Retried try job too often on mac_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-11-14 16:18:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rpaquay@chromium.org/51433002/760003

[commit-bot: I haz the power, 2013-11-14 16:44:51]

Retried try job too often on linux_rel for step(s) base_unittests,
browser_tests, interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-11-14 16:58:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rpaquay@chromium.org/51433002/1640001

[commit-bot: I haz the power, 2013-11-14 17:22:15]

Retried try job too often on mac_rel for step(s) app_list_unittests,
cacheinvalidation_unittests, cc_unittests, check_deps, chromedriver2_unittests,
components_unittests, content_browsertests, content_unittests, crypto_unittests,
google_apis_unittests, gpu_unittests, ipc_tests, jingle_unittests,
media_unittests, message_center_unittests, nacl_integration, ppapi_unittests,
printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests,
sync_unit_tests, telemetry_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-11-14 20:21:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rpaquay@chromium.org/51433002/1830001

[commit-bot: I haz the power, 2013-11-14 20:47:24]

Retried try job too often on mac_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-11-14 21:20:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rpaquay@chromium.org/51433002/2030001

[commit-bot: I haz the power, 2013-11-14 21:48:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rpaquay@chromium.org/51433002/2030001

[commit-bot: I haz the power, 2013-11-15 01:17:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rpaquay@chromium.org/51433002/2030001

[commit-bot: I haz the power, 2013-11-15 03:49:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rpaquay@chromium.org/51433002/2030001

[commit-bot: I haz the power, 2013-11-15 07:25:10]

Retried try job too often on win_rel for step(s) nacl_integration
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-15 15:28:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rpaquay@chromium.org/51433002/2030001

[commit-bot: I haz the power, 2013-11-15 15:55:30]

Change committed as 235332