[Extensions] Add a developerPrivate.updateExtensionConfiguration function

[Extensions] Add a developerPrivate.updateExtensionConfiguration function

Add a chrome.developerPrivate.updateExtensionConfiguration function, which allows
for setting particular extension traits.  Coalesce the existing allow file
access and allow incognito access into this, and add others.  Update the
chrome://extensions page to use the new function.

BUG=461039
(get past histogram presubmit warning)
NOTRY=true

Committed: https://crrev.com/cca2195ed0ab793a55b323f7d67f2d38c2d6e13e
Cr-Commit-Position: refs/heads/master@{#321381}

[Devlin, 2015-03-16 21:34:20]

Patchset #1 (id:1) has been deleted

[Devlin, 2015-03-16 21:35:15]

rdevlin.cronin@chromium.org changed reviewers:
+ kalman@chromium.org

[Devlin, 2015-03-16 21:35:16]

https://codereview.chromium.org/997183005/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/developer_private.idl (right):

https://codereview.chromium.org/997183005/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/developer_private.idl:82: enum PermissionType {
This is not my favorite name, because it's a bit of a stretch for error
collection and show action button (though both can be argued to be
"permissions").  Open to suggestions.

[not at google - send to devlin, 2015-03-16 21:48:09]

https://codereview.chromium.org/997183005/diff/20001/chrome/browser/extension...
File
chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc
(right):

https://codereview.chromium.org/997183005/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc:146:
args.Append(parameters);
consider using DictionaryBuilder

https://codereview.chromium.org/997183005/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/developer_private.idl (right):

https://codereview.chromium.org/997183005/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/developer_private.idl:82: enum PermissionType {
On 2015/03/16 21:35:16, Devlin wrote:
> This is not my favorite name, because it's a bit of a stretch for error
> collection and show action button (though both can be argued to be
> "permissions").  Open to suggestions.

I might as well reply to this comment for my API meta comment.

I would design this API differently. My issues with it so far:
- "permission" is an overloaded term. "configuration" is more natural to me.
- it doesn't allow setting more than one thing at once, and similarly,
- the pairing of type-with-new-value is awkward.

Proposal:

dictionary ExtensionConfigurationUpdate {
  boolean? fileAccess;
  boolean? incognitoAccess;
  boolean? errorCollection;
  boolean? runOnAllUrls;
  boolean? showActionButton;
};

...

static void updateExtensionConfiguration(
    ExtensionConfigurationUpdate update, VoidCallback callback);

...

JavaScript would look like:

chrome.developerPrivate.updateExtensionConfiguration({
  fileAccess: true,
  incognitoAccess: true
});

...

Yes - this introduces a tri-state boolean. Sort of. It's pretty standard though
- you omit a parameter and it isn't set. You provide it and it sets it to true
or false.

[Devlin, 2015-03-16 22:41:37]

https://codereview.chromium.org/997183005/diff/20001/chrome/browser/extension...
File
chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc
(right):

https://codereview.chromium.org/997183005/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc:146:
args.Append(parameters);
On 2015/03/16 21:48:08, kalman wrote:
> consider using DictionaryBuilder 

I did consider it, but this is shorter when we want to cache the DictionaryValue
pointer to modify it on line 163 :)

https://codereview.chromium.org/997183005/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/developer_private.idl (right):

https://codereview.chromium.org/997183005/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/developer_private.idl:82: enum PermissionType {
On 2015/03/16 21:48:08, kalman wrote:
> On 2015/03/16 21:35:16, Devlin wrote:
> > This is not my favorite name, because it's a bit of a stretch for error
> > collection and show action button (though both can be argued to be
> > "permissions").  Open to suggestions.
> 
> I might as well reply to this comment for my API meta comment.
> 
> I would design this API differently. My issues with it so far:
> - "permission" is an overloaded term. "configuration" is more natural to me.
> - it doesn't allow setting more than one thing at once, and similarly,
> - the pairing of type-with-new-value is awkward.
> 
> Proposal:
> 
> dictionary ExtensionConfigurationUpdate {
>   boolean? fileAccess;
>   boolean? incognitoAccess;
>   boolean? errorCollection;
>   boolean? runOnAllUrls;
>   boolean? showActionButton;
> };
> 
> ...
> 
> static void updateExtensionConfiguration(
>     ExtensionConfigurationUpdate update, VoidCallback callback);
> 
> ...
> 
> JavaScript would look like:
> 
> chrome.developerPrivate.updateExtensionConfiguration({
>   fileAccess: true,
>   incognitoAccess: true
> });
> 
> ...
> 
> Yes - this introduces a tri-state boolean. Sort of. It's pretty standard
though
> - you omit a parameter and it isn't set. You provide it and it sets it to true
> or false.

I like it. I was thinking about having the ability to set multiple before, but
didn't originally lean to having a tri-state boolean, and didn't want the bulk
of having n number of pairs (because, in practice, we only really update one
field at a time).  But having tristates here is actually quite nice.  Good call.

Still don't love the name (ExtensionConfiguration doesn't quite fit, either -
it's more like "run options" or something) but it beats "permission" ;)

[not at google - send to devlin, 2015-03-17 14:31:31]

lgtm

https://codereview.chromium.org/997183005/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(right):

https://codereview.chromium.org/997183005/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:525: if
(update.file_access) {
I like it.

https://codereview.chromium.org/997183005/diff/40001/chrome/common/extensions...
File chrome/common/extensions/api/developer_private.idl (right):

https://codereview.chromium.org/997183005/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/developer_private.idl:416: // Modifies an
extension's current configuration.
"Any properties omitted from |update| will not be changed."

[Devlin, 2015-03-17 16:00:29]

Patchset #3 (id:60001) has been deleted

[Devlin, 2015-03-18 18:38:02]

https://codereview.chromium.org/997183005/diff/40001/chrome/common/extensions...
File chrome/common/extensions/api/developer_private.idl (right):

https://codereview.chromium.org/997183005/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/developer_private.idl:416: // Modifies an
extension's current configuration.
On 2015/03/17 14:31:31, kalman wrote:
> "Any properties omitted from |update| will not be changed."

Done.

[Devlin, 2015-03-18 18:39:01]

rdevlin.cronin@chromium.org changed reviewers:
+ dbeam@chromium.org, mpearson@chromium.org

[Devlin, 2015-03-18 18:39:02]

+Dan for closure compiler, Mark for histograms.

[Mark P, 2015-03-18 18:46:32]

histograms.xml lgtm

[Dan Beam, 2015-03-18 19:12:49]

lgtm

https://codereview.chromium.org/997183005/diff/80001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/developer_private_custom_bindings.js
(right):

https://codereview.chromium.org/997183005/diff/80001/chrome/renderer/resource...
chrome/renderer/resources/extensions/developer_private_custom_bindings.js:31: {
extensionId: id, fileAccess: allow }, callback);
nit: {extensionId: id, fileAccess: allow} ({\s -> {, \s} -> })

https://codereview.chromium.org/997183005/diff/80001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/developer/test/basics.js (right):

https://codereview.chromium.org/997183005/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/developer/test/basics.js:39:
chrome.test.assertTrue(info != null);
^ shouldn't you do this before accessing info.id?

https://codereview.chromium.org/997183005/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/developer/test/basics.js:63:
getExtensionInfoCallback();
nit: Promises?

[Devlin, 2015-03-18 22:20:28]

https://codereview.chromium.org/997183005/diff/80001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/developer_private_custom_bindings.js
(right):

https://codereview.chromium.org/997183005/diff/80001/chrome/renderer/resource...
chrome/renderer/resources/extensions/developer_private_custom_bindings.js:31: {
extensionId: id, fileAccess: allow }, callback);
On 2015/03/18 19:12:48, Dan Beam wrote:
> nit: {extensionId: id, fileAccess: allow} ({\s -> {, \s} -> })

Done.

https://codereview.chromium.org/997183005/diff/80001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/developer/test/basics.js (right):

https://codereview.chromium.org/997183005/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/developer/test/basics.js:39:
chrome.test.assertTrue(info != null);
On 2015/03/18 19:12:49, Dan Beam wrote:
> ^ shouldn't you do this before accessing info.id?

Yes. Done.

https://codereview.chromium.org/997183005/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/developer/test/basics.js:63:
getExtensionInfoCallback();
On 2015/03/18 19:12:49, Dan Beam wrote:
> nit: Promises?

I've tried rewriting it a few ways, and, frankly, none of them are any prettier.
 Trying to mix both callbacks (which are a requisite of extension APIs and the
test api) with promises just results in a confusing mess.

Of course, my promise experience is also a fairly limited, so open to any
suggestions on how this could work. :)

[Dan Beam, 2015-03-18 22:38:13]

lgtm

https://codereview.chromium.org/997183005/diff/80001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/developer/test/basics.js (right):

https://codereview.chromium.org/997183005/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/developer/test/basics.js:63:
getExtensionInfoCallback();
On 2015/03/18 22:20:28, Devlin wrote:
> On 2015/03/18 19:12:49, Dan Beam wrote:
> > nit: Promises?
> 
> I've tried rewriting it a few ways, and, frankly, none of them are any
prettier.
>  Trying to mix both callbacks (which are a requisite of extension APIs and the
> test api) with promises just results in a confusing mess.
> 
> Of course, my promise experience is also a fairly limited, so open to any
> suggestions on how this could work. :)

don't worry about it

[Devlin, 2015-03-19 00:07:18]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2015-03-19 00:07:19]

The patchset sent to the CQ was uploaded after l-g-t-m from kalman@chromium.org,
mpearson@chromium.org
Link to the patchset: https://codereview.chromium.org/997183005/#ps100001
(title: " ")

[Devlin, 2015-03-19 00:07:40]

The CQ bit was unchecked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2015-03-19 00:07:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/997183005/100001

[Devlin, 2015-03-19 00:08:34]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2015-03-19 00:09:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/997183005/100001

[commit-bot: I haz the power, 2015-03-19 00:25:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-19 00:25:34]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Devlin, 2015-03-19 16:49:30]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2015-03-19 16:49:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/997183005/100001

[commit-bot: I haz the power, 2015-03-19 16:56:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-19 16:56:29]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Devlin, 2015-03-19 16:59:42]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2015-03-19 16:59:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/997183005/100001

[commit-bot: I haz the power, 2015-03-19 17:00:34]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2015-03-19 17:01:19]

Patchset 4 (id:??) landed as
https://crrev.com/cca2195ed0ab793a55b323f7d67f2d38c2d6e13e
Cr-Commit-Position: refs/heads/master@{#321381}