Require user confirmation for chrome.management.uninstall except when uninstalling self.

Require user confirmation for chrome.management.uninstall except when uninstalling self.

chrome.management.uninstall is changed as follows unless the extension is trying to
uninstall itself:
- The confirmation dialog is shown by default.
- A user gesture is required.

BUG=178319
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245457

[meacer, 2014-01-16 00:49:46]

kalman: Can you please take a look? I have another CL for changing the dialog
text (crrev.com/139843003, not ready yet).

[not at google - send to devlin, 2014-01-16 01:13:41]

https://codereview.chromium.org/137793011/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/management/management_api.cc (right):

https://codereview.chromium.org/137793011/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/management/management_api.cc:612: if (extension_
&& params->id == extension_->id()) {
If you're worried about |extension_| being valid then you should have it as an
EXTENSION_FUNCTION_VALIDATE(extension_), because if this API managed to get
through something odd is going on.

https://codereview.chromium.org/137793011/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/management/management_api.cc:615:
show_confirm_dialog = *params->options->show_confirm_dialog;
nit: can you do the above 3 lines in a single expression?

https://codereview.chromium.org/137793011/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/management/management_api_constants.cc
(right):

https://codereview.chromium.org/137793011/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/management/management_api_constants.cc:19:
"chrome.management.uninstall should be called on a user gesture.";
s/should be called on/requires/

https://codereview.chromium.org/137793011/diff/1/chrome/test/data/extensions/...
File
chrome/test/data/extensions/api_test/management/management_policy/prohibited.js
(right):

https://codereview.chromium.org/137793011/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/management/management_policy/prohibited.js:75:
});
(making this comment in an arbitrary place)

can we add a test somewhere that calls chrome.management.uninstall without a
user gesture and makes sure it fails?

[meacer, 2014-01-16 01:59:19]

https://codereview.chromium.org/137793011/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/management/management_api.cc (right):

https://codereview.chromium.org/137793011/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/management/management_api.cc:612: if (extension_
&& params->id == extension_->id()) {
On 2014/01/16 01:13:41, kalman wrote:
> If you're worried about |extension_| being valid then you should have it as an
> EXTENSION_FUNCTION_VALIDATE(extension_), because if this API managed to get
> through something odd is going on.

Done.

https://codereview.chromium.org/137793011/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/management/management_api.cc:615:
show_confirm_dialog = *params->options->show_confirm_dialog;
On 2014/01/16 01:13:41, kalman wrote:
> nit: can you do the above 3 lines in a single expression?

Can't think of a better way that won't be confusing. This explains
showConfirmDialog behavior is opposite for normal and self uninstalls and can
only be overriden by self uninstalls.

https://codereview.chromium.org/137793011/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/management/management_api_constants.cc
(right):

https://codereview.chromium.org/137793011/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/management/management_api_constants.cc:19:
"chrome.management.uninstall should be called on a user gesture.";
On 2014/01/16 01:13:41, kalman wrote:
> s/should be called on/requires/

Done.

https://codereview.chromium.org/137793011/diff/1/chrome/test/data/extensions/...
File
chrome/test/data/extensions/api_test/management/management_policy/prohibited.js
(right):

https://codereview.chromium.org/137793011/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/management/management_policy/prohibited.js:75:
});
On 2014/01/16 01:13:41, kalman wrote:
> (making this comment in an arbitrary place)
> 
> can we add a test somewhere that calls chrome.management.uninstall without a
> user gesture and makes sure it fails?

Added uninstallEnabledAppWithoutUserGesture to management/test/uninstall.js

[not at google - send to devlin, 2014-01-16 05:07:38]

lgtm

https://codereview.chromium.org/137793011/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/management/management_api.cc (right):

https://codereview.chromium.org/137793011/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/management/management_api.cc:615:
show_confirm_dialog = *params->options->show_confirm_dialog;
On 2014/01/16 01:59:19, Mustafa Emre Acer wrote:
> On 2014/01/16 01:13:41, kalman wrote:
> > nit: can you do the above 3 lines in a single expression?
> 
> Can't think of a better way that won't be confusing. This explains
> showConfirmDialog behavior is opposite for normal and self uninstalls and can
> only be overriden by self uninstalls.

It could be like (inside the is-self check)

show_confirm_dialog = params->options.get() &&
                      params->options->show_confirm_dialog.get() &&
                      *params->options->show_confirm_dialog;

right? though eh, it was just a nit. The conditions here fit into my brain now
that the extension_ check has been moved out.

https://codereview.chromium.org/137793011/diff/190001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/runtime/uninstall_url/test.js (right):

https://codereview.chromium.org/137793011/diff/190001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/runtime/uninstall_url/test.js:18:
chrome.test.runWithUserGesture(function() {
pass(function() { ...

https://codereview.chromium.org/137793011/diff/190001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/webstore_private/common.js (right):

https://codereview.chromium.org/137793011/diff/190001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/webstore_private/common.js:93:
chrome.test.runWithUserGesture(function() {
also need a callbackPass around this one

[meacer, 2014-01-16 18:59:38]

https://codereview.chromium.org/137793011/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/management/management_api.cc (right):

https://codereview.chromium.org/137793011/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/management/management_api.cc:615:
show_confirm_dialog = *params->options->show_confirm_dialog;
On 2014/01/16 05:07:39, kalman wrote:
> On 2014/01/16 01:59:19, Mustafa Emre Acer wrote:
> > On 2014/01/16 01:13:41, kalman wrote:
> > > nit: can you do the above 3 lines in a single expression?
> > 
> > Can't think of a better way that won't be confusing. This explains
> > showConfirmDialog behavior is opposite for normal and self uninstalls and
can
> > only be overriden by self uninstalls.
> 
> It could be like (inside the is-self check)
> 
> show_confirm_dialog = params->options.get() &&
>                       params->options->show_confirm_dialog.get() &&
>                       *params->options->show_confirm_dialog;
> 
> right? though eh, it was just a nit. The conditions here fit into my brain now
> that the extension_ check has been moved out.

Makes sense.

https://codereview.chromium.org/137793011/diff/190001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/runtime/uninstall_url/test.js (right):

https://codereview.chromium.org/137793011/diff/190001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/runtime/uninstall_url/test.js:18:
chrome.test.runWithUserGesture(function() {
On 2014/01/16 05:07:39, kalman wrote:
> pass(function() { ...

Done.

https://codereview.chromium.org/137793011/diff/190001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/webstore_private/common.js (right):

https://codereview.chromium.org/137793011/diff/190001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/webstore_private/common.js:93:
chrome.test.runWithUserGesture(function() {
On 2014/01/16 05:07:39, kalman wrote:
> also need a callbackPass around this one

Done.

[commit-bot: I haz the power, 2014-01-16 19:00:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/meacer@chromium.org/137793011/300001

[commit-bot: I haz the power, 2014-01-16 20:37:49]

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

[commit-bot: I haz the power, 2014-01-16 22:58:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/meacer@chromium.org/137793011/560001

[commit-bot: I haz the power, 2014-01-17 07:06:55]

Change committed as 245457