Replace "external_install" boolean parameter with explicit enumeration in ExtensionUninstall method.

Replace "external_install" boolean parameter with explicit enumeration in ExtensionUninstall method.

The intent is to be able to address issues found in https://code.google.com/p/chromium/issues/detail?id=84556#c39

* This change should be a strict refactoring, and there should be no change of behavior wrt to extension uninstall behavior.

* Identify a few specific reasons for extension uninstall and create enum values for them.

* Run "git cl format"


BUG=84556
TBR=sky@chromium.org for trivial changes under chrome/browser, except chrome/browser/extensions (already lgtm'ed).

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

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

[rpaquay, 2014-05-14 22:35:53]

kalman@chromium.org: Please review changes in 
chrome/browser/apps/*
chrome/browser/extensions/*

benwells@chromium.org: Please review changes in 
chrome/browser/ui/app_list/*
chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc

[not at google - send to devlin, 2014-05-14 22:47:41]

I won't have time to do this today, too many reviews still pending. Devlin could
you have a look?

[Devlin, 2014-05-14 23:14:19]

lgtm

https://codereview.chromium.org/284103002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/management/management_api.cc (right):

https://codereview.chromium.org/284103002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/management/management_api.cc:584: extension_id_,
ExtensionService::kUninstallReasonDefault, NULL);
I think ManagementAPI should more likely be user initiated, in most cases.  The
API will always pop up a "Do you wish to uninstall?" dialog, unless it's
uninstalling the extension which called it.  Of course, we don't necessarily
know what causes the prompt to be displayed, so a UninstallReasonAPI could also
make sense.  In either case, we probably want to separate from Default here.

https://codereview.chromium.org/284103002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_disabled_ui.cc (right):

https://codereview.chromium.org/284103002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_disabled_ui.cc:311:
ExtensionService::kUninstallReasonUserInitiated,
Looking at this more, maybe we want to differentiate between "UserInitiated" and
"UserConfirmed".  This isn't initiated by the user, but the user is aware of it.

https://codereview.chromium.org/284103002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/284103002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:685: (reason ==
kUninstallReasonOrphanedSharedModule);
Shame this is still here... ah well.

https://codereview.chromium.org/284103002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.h (right):

https://codereview.chromium.org/284103002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_service.h:132: kUninstallReasonDefault,
For better or worse, Extensions code overwhelmingly uses SCREAMING_STYLE for
enums.

https://codereview.chromium.org/284103002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_service.h:138: kUninstallReasonSynch,
nit: "Sync" is more common spelling in the code.

https://codereview.chromium.org/284103002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/external_install_ui.cc (right):

https://codereview.chromium.org/284103002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/external_install_ui.cc:346: extension_id_,
ExtensionService::kUninstallReasonDefault, NULL);
I don't think that external_install_ui and extension_disabled_ui should have
different reasons.  They are very similar in nature.

[Devlin, 2014-05-14 23:14:48]

On 2014/05/14 23:14:19, D Cronin wrote:
> lgtm
> 
>
https://codereview.chromium.org/284103002/diff/20001/chrome/browser/extension...
> File chrome/browser/extensions/api/management/management_api.cc (right):
> 
>
https://codereview.chromium.org/284103002/diff/20001/chrome/browser/extension...
> chrome/browser/extensions/api/management/management_api.cc:584: extension_id_,
> ExtensionService::kUninstallReasonDefault, NULL);
> I think ManagementAPI should more likely be user initiated, in most cases. 
The
> API will always pop up a "Do you wish to uninstall?" dialog, unless it's
> uninstalling the extension which called it.  Of course, we don't necessarily
> know what causes the prompt to be displayed, so a UninstallReasonAPI could
also
> make sense.  In either case, we probably want to separate from Default here.
> 
>
https://codereview.chromium.org/284103002/diff/20001/chrome/browser/extension...
> File chrome/browser/extensions/extension_disabled_ui.cc (right):
> 
>
https://codereview.chromium.org/284103002/diff/20001/chrome/browser/extension...
> chrome/browser/extensions/extension_disabled_ui.cc:311:
> ExtensionService::kUninstallReasonUserInitiated,
> Looking at this more, maybe we want to differentiate between "UserInitiated"
and
> "UserConfirmed".  This isn't initiated by the user, but the user is aware of
it.
> 
>
https://codereview.chromium.org/284103002/diff/20001/chrome/browser/extension...
> File chrome/browser/extensions/extension_service.cc (right):
> 
>
https://codereview.chromium.org/284103002/diff/20001/chrome/browser/extension...
> chrome/browser/extensions/extension_service.cc:685: (reason ==
> kUninstallReasonOrphanedSharedModule);
> Shame this is still here... ah well.
> 
>
https://codereview.chromium.org/284103002/diff/20001/chrome/browser/extension...
> File chrome/browser/extensions/extension_service.h (right):
> 
>
https://codereview.chromium.org/284103002/diff/20001/chrome/browser/extension...
> chrome/browser/extensions/extension_service.h:132: kUninstallReasonDefault,
> For better or worse, Extensions code overwhelmingly uses SCREAMING_STYLE for
> enums.
> 
>
https://codereview.chromium.org/284103002/diff/20001/chrome/browser/extension...
> chrome/browser/extensions/extension_service.h:138: kUninstallReasonSynch,
> nit: "Sync" is more common spelling in the code.
> 
>
https://codereview.chromium.org/284103002/diff/20001/chrome/browser/extension...
> File chrome/browser/extensions/external_install_ui.cc (right):
> 
>
https://codereview.chromium.org/284103002/diff/20001/chrome/browser/extension...
> chrome/browser/extensions/external_install_ui.cc:346: extension_id_,
> ExtensionService::kUninstallReasonDefault, NULL);
> I don't think that external_install_ui and extension_disabled_ui should have
> different reasons.  They are very similar in nature.

Gah, didn't mean to lg that.  Please hold off on committing.

[commit-bot: I haz the power, 2014-05-14 23:16:26]

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

[not at google - send to devlin, 2014-05-14 23:17:02]

drive-by

https://codereview.chromium.org/284103002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/management/management_api.cc (right):

https://codereview.chromium.org/284103002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/management/management_api.cc:584: extension_id_,
ExtensionService::kUninstallReasonDefault, NULL);
On 2014/05/14 23:14:20, D Cronin wrote:
> I think ManagementAPI should more likely be user initiated, in most cases. 
The
> API will always pop up a "Do you wish to uninstall?" dialog, unless it's
> uninstalling the extension which called it.  Of course, we don't necessarily
> know what causes the prompt to be displayed, so a UninstallReasonAPI could
also
> make sense.  In either case, we probably want to separate from Default here.

If somebody writes an extension management extension then uninstalls from that
should be treated like uninstalls from chrome://extensions (consider also that
the developer tool uses the management API).

I.e. if it went through the uninstall UI, it should be considered initiated by
the user (unlike an extension uninstalling itself).

[Devlin, 2014-05-14 23:18:06]

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

[rpaquay, 2014-06-30 23:16:18]

PTAL.

Retrying this change after a few weeks working on other stuff. As a reminder the
eventual goal here is to update extension uninstall events to pass a reason to
their listener so they can take more precise action. In particular, the
"uninstall_url" of an extension will be opened only if the extension is
installed by the user initiated gesture.

https://codereview.chromium.org/284103002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/management/management_api.cc (right):

https://codereview.chromium.org/284103002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/management/management_api.cc:584: extension_id_,
ExtensionService::kUninstallReasonDefault, NULL);
On 2014/05/14 23:17:02, kalman wrote:
> On 2014/05/14 23:14:20, D Cronin wrote:
> > I think ManagementAPI should more likely be user initiated, in most cases. 
> The
> > API will always pop up a "Do you wish to uninstall?" dialog, unless it's
> > uninstalling the extension which called it.  Of course, we don't necessarily
> > know what causes the prompt to be displayed, so a UninstallReasonAPI could
> also
> > make sense.  In either case, we probably want to separate from Default here.
> 
> If somebody writes an extension management extension then uninstalls from that
> should be treated like uninstalls from chrome://extensions (consider also that
> the developer tool uses the management API).
> 
> I.e. if it went through the uninstall UI, it should be considered initiated by
> the user (unlike an extension uninstalling itself).

Done.

https://codereview.chromium.org/284103002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_disabled_ui.cc (right):

https://codereview.chromium.org/284103002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_disabled_ui.cc:311:
ExtensionService::kUninstallReasonUserInitiated,
On 2014/05/14 23:14:20, Devlin wrote:
> Looking at this more, maybe we want to differentiate between "UserInitiated"
and
> "UserConfirmed".  This isn't initiated by the user, but the user is aware of
it.

I created a new enum entry for this case.

https://codereview.chromium.org/284103002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.h (right):

https://codereview.chromium.org/284103002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_service.h:132: kUninstallReasonDefault,
On 2014/05/14 23:14:20, Devlin wrote:
> For better or worse, Extensions code overwhelmingly uses SCREAMING_STYLE for
> enums.

Done.

https://codereview.chromium.org/284103002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_service.h:138: kUninstallReasonSynch,
On 2014/05/14 23:14:20, D Cronin wrote:
> nit: "Sync" is more common spelling in the code.

Done.

https://codereview.chromium.org/284103002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/external_install_ui.cc (right):

https://codereview.chromium.org/284103002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/external_install_ui.cc:346: extension_id_,
ExtensionService::kUninstallReasonDefault, NULL);
On 2014/05/14 23:14:20, Devlin wrote:
> I don't think that external_install_ui and extension_disabled_ui should have
> different reasons.  They are very similar in nature.

Done.

[Devlin, 2014-07-01 17:39:10]

https://codereview.chromium.org/284103002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service.h (right):

https://codereview.chromium.org/284103002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.h:159: UNINSTALL_REASON_DEFAULT,    
        // Usually from testing code
Please rename this to UNINSTALL_REASON_FOR_TESTING.  Otherwise, I fear that
everyone adding a new "random" uninstall will just use "default".

https://codereview.chromium.org/284103002/diff/120001/chrome/browser/search/h...
File chrome/browser/search/hotword_service.cc (right):

https://codereview.chromium.org/284103002/diff/120001/chrome/browser/search/h...
chrome/browser/search/hotword_service.cc:364:
ExtensionService::UNINSTALL_REASON_DEFAULT,
This one's tricky.  I'd love to get rid of DEFAULT and rename it to TESTING, but
this is also used for updating itself.  Hmm... I wonder if we should have a
UNINSTALL_REASON_INTERNAL_MANAGEMENT for this stuff.  It would be used for
Hotword, Bookmarks, and maybe the Drive one.  Unfortunately, I'm not entirely
sure that's a much more specific name than "DEFAULT".  WDYT?

https://codereview.chromium.org/284103002/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/bookmark_app_bubble_view.cc (right):

https://codereview.chromium.org/284103002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/bookmark_app_bubble_view.cc:210:
extension_id_, ExtensionService::UNINSTALL_REASON_DEFAULT, NULL);
See comment in hotword.

[rpaquay, 2014-07-01 22:56:53]

https://codereview.chromium.org/284103002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service.h (right):

https://codereview.chromium.org/284103002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.h:159: UNINSTALL_REASON_DEFAULT,    
        // Usually from testing code
On 2014/07/01 17:39:10, Devlin wrote:
> Please rename this to UNINSTALL_REASON_FOR_TESTING.  Otherwise, I fear that
> everyone adding a new "random" uninstall will just use "default".

Done.

https://codereview.chromium.org/284103002/diff/120001/chrome/browser/search/h...
File chrome/browser/search/hotword_service.cc (right):

https://codereview.chromium.org/284103002/diff/120001/chrome/browser/search/h...
chrome/browser/search/hotword_service.cc:364:
ExtensionService::UNINSTALL_REASON_DEFAULT,
On 2014/07/01 17:39:10, Devlin wrote:
> This one's tricky.  I'd love to get rid of DEFAULT and rename it to TESTING,
but
> this is also used for updating itself.  Hmm... I wonder if we should have a
> UNINSTALL_REASON_INTERNAL_MANAGEMENT for this stuff.  It would be used for
> Hotword, Bookmarks, and maybe the Drive one.  Unfortunately, I'm not entirely
> sure that's a much more specific name than "DEFAULT".  WDYT?

I like the "internal management" idea. I added a comment next to the new entry.

https://codereview.chromium.org/284103002/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/bookmark_app_bubble_view.cc (right):

https://codereview.chromium.org/284103002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/bookmark_app_bubble_view.cc:210:
extension_id_, ExtensionService::UNINSTALL_REASON_DEFAULT, NULL);
On 2014/07/01 17:39:10, Devlin wrote:
> See comment in hotword.

Done.

[Devlin, 2014-07-01 23:02:02]

lgtm

[rpaquay, 2014-07-02 21:55:36]

The CQ bit was checked by rpaquay@chromium.org

[commit-bot: I haz the power, 2014-07-02 21:58:25]

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

[commit-bot: I haz the power, 2014-07-03 02:51:26]

Change committed as 281130

[Nikita (slow), 2014-07-03 09:15:24]

A revert of this CL has been created in
https://codereview.chromium.org/364183002/ by nkostylev@chromium.org.

The reason for reverting is: Speculative revert,
PlatformAppBrowserTest.Messaging test is broken: http://goo.gl/nYCK6a

PlatformAppBrowserTest.Messaging (run #1):
[ RUN      ] PlatformAppBrowserTest.Messaging
[708:3584:0702/221627:ERROR:gpu_info_collector_win.cc(103)] Can't retrieve a
valid WinSAT assessment.
[708:3584:0702/221628:WARNING:chrome_browser_main_win.cc(325)] Command line too
long for RegisterApplicationRestart
[4064:3824:0702/221631:ERROR:singleton_hwnd.cc(43)] Cannot create windows on
non-UI thread!
[640:3716:0702/221636:ERROR:ipc_channel_win.cc(444)] pipe error: 232.

[Nikita (slow), 2014-07-03 09:18:34]

A revert of this CL has been created in
https://codereview.chromium.org/369093002/ by nkostylev@chromium.org.

The reason for reverting is: The reason for reverting is: Speculative revert,
PlatformAppBrowserTest.Messaging test is broken: http://goo.gl/nYCK6a

PlatformAppBrowserTest.Messaging (run #1):
[ RUN      ] PlatformAppBrowserTest.Messaging
[708:3584:0702/221627:ERROR:gpu_info_collector_win.cc(103)] Can't retrieve a
valid WinSAT assessment.
[708:3584:0702/221628:WARNING:chrome_browser_main_win.cc(325)] Command line too
long for RegisterApplicationRestart
[4064:3824:0702/221631:ERROR:singleton_hwnd.cc(43)] Cannot create windows on
non-UI thread!
[640:3716:0702/221636:ERROR:ipc_channel_win.cc(444)] pipe error: 232..

[Nikita (slow), 2014-07-03 09:19:52]

A revert of this CL has been created in
https://codereview.chromium.org/363223004/ by nkostylev@chromium.org.

The reason for reverting is: Speculative revert,
PlatformAppBrowserTest.Messaging test is broken: http://goo.gl/nYCK6a .

[Nikita (slow), 2014-07-03 09:25:31]

Committed revert (via drover) in https://codereview.chromium.org/369863002/

[rpaquay, 2014-07-07 20:34:59]

The CQ bit was checked by rpaquay@chromium.org

[commit-bot: I haz the power, 2014-07-07 20:36:05]

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

[commit-bot: I haz the power, 2014-07-08 02:19:10]

Change committed as 281631