Put extension enable logic into a ExtensionEnableFlow.

Put extension enable logic into a ExtensionEnableFlow.

Extract the code from AppLauncherHandler into a ExtensionEnableFlow so that it
can be reused by app list and launcher bar.

BUG=157996
TEST=No change. Wait until app list/launcher code changes land to test.

R=estade@chromium.org,benwells@chromium.org


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

[xiyuan, 2012-12-21 18:33:29]

Moving extension enable logic from AppLauncherHandler into a ExtensionEnableFlow
class so that we can share it with app list and launcher.

[sky, 2012-12-21 18:39:58]

I'm not a good reviewer for this stuff. Maybe estade and aa?

[xiyuan, 2012-12-21 18:43:31]

estade@, could you help with the review? Thanks.

[Evan Stade, 2012-12-21 22:45:26]

webui LGTM modulo my one comment, but please get someone on the extensions team
to look over the extensions changes.

https://codereview.chromium.org/11644077/diff/1/chrome/browser/ui/webui/ntp/a...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

https://codereview.chromium.org/11644077/diff/1/chrome/browser/ui/webui/ntp/a...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:830: extension_id_prompting_
= extension_id;
can you get rid of extension_id_prompting_ and just use
extension_enable_flow_->extension_id() as the single source of truth?

[xiyuan, 2012-12-22 00:35:43]

https://codereview.chromium.org/11644077/diff/1/chrome/browser/ui/webui/ntp/a...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

https://codereview.chromium.org/11644077/diff/1/chrome/browser/ui/webui/ntp/a...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:830: extension_id_prompting_
= extension_id;
On 2012/12/21 22:45:26, Evan Stade wrote:
> can you get rid of extension_id_prompting_ and just use
> extension_enable_flow_->extension_id() as the single source of truth?

extension_id_prompting_ used to guard both uninstall UI and enable UI.
extension_enable_flow_ is only used for enable UI. If we want to keep existing
behavior, i.e. only one uninstall UI or enable UI is shown to the user, then we
need to test extension_enable_flow_ in uninstall UI code. 

I would say, the code is cleaner with current extension_id_prompting_ test. What
do you think?

[xiyuan, 2012-12-22 00:38:36]

Ben, could you also review the CL?

The CL is mostly moving the existing extension enabling logic from
AppLauncherHandler into a separate ExtensionEnableFlow class that could be used
elsewhere (in app list and launcher, where I plan to use it).

Thanks.

[Evan Stade, 2013-01-02 21:55:38]

https://codereview.chromium.org/11644077/diff/1/chrome/browser/ui/webui/ntp/a...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

https://codereview.chromium.org/11644077/diff/1/chrome/browser/ui/webui/ntp/a...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:830: extension_id_prompting_
= extension_id;
On 2012/12/22 00:35:43, xiyuan wrote:
> On 2012/12/21 22:45:26, Evan Stade wrote:
> > can you get rid of extension_id_prompting_ and just use
> > extension_enable_flow_->extension_id() as the single source of truth?
> 
> extension_id_prompting_ used to guard both uninstall UI and enable UI.
> extension_enable_flow_ is only used for enable UI. If we want to keep existing
> behavior, i.e. only one uninstall UI or enable UI is shown to the user, then
we
> need to test extension_enable_flow_ in uninstall UI code. 
> 
> I would say, the code is cleaner with current extension_id_prompting_ test.
What
> do you think?

that's a reasonable explanation. Can you add "(either enable or uninstall)" to
the comment about extension_id_prompting_ in the header file?

[benwells, 2013-01-03 00:30:17]

lgtm, with one question. if we'll only ever have one enable prompt open at a
time i think it would be cleaner to remove the flow parameter and the related
DCHECKs from the ntp code.

https://codereview.chromium.org/11644077/diff/1/chrome/browser/ui/extensions/...
File chrome/browser/ui/extensions/extension_enable_flow_delegate.h (right):

https://codereview.chromium.org/11644077/diff/1/chrome/browser/ui/extensions/...
chrome/browser/ui/extensions/extension_enable_flow_delegate.h:21: virtual void
ExtensionEnableFlowAborted(ExtensionEnableFlow* flow,
Why pass the flow back? I can see this would enable multiple flows to happen
concurrently, is that something the other launcher / app list will use?

[xiyuan, 2013-01-07 18:13:11]

https://codereview.chromium.org/11644077/diff/1/chrome/browser/ui/extensions/...
File chrome/browser/ui/extensions/extension_enable_flow_delegate.h (right):

https://codereview.chromium.org/11644077/diff/1/chrome/browser/ui/extensions/...
chrome/browser/ui/extensions/extension_enable_flow_delegate.h:21: virtual void
ExtensionEnableFlowAborted(ExtensionEnableFlow* flow,
On 2013/01/03 00:30:17, benwells wrote:
> Why pass the flow back? I can see this would enable multiple flows to happen
> concurrently, is that something the other launcher / app list will use?

Agreed. We should not encourage concurrent usage of it.

https://codereview.chromium.org/11644077/diff/1/chrome/browser/ui/webui/ntp/a...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

https://codereview.chromium.org/11644077/diff/1/chrome/browser/ui/webui/ntp/a...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:830: extension_id_prompting_
= extension_id;
On 2013/01/02 21:55:38, Evan Stade wrote:
> On 2012/12/22 00:35:43, xiyuan wrote:
> > On 2012/12/21 22:45:26, Evan Stade wrote:
> > > can you get rid of extension_id_prompting_ and just use
> > > extension_enable_flow_->extension_id() as the single source of truth?
> > 
> > extension_id_prompting_ used to guard both uninstall UI and enable UI.
> > extension_enable_flow_ is only used for enable UI. If we want to keep
existing
> > behavior, i.e. only one uninstall UI or enable UI is shown to the user, then
> we
> > need to test extension_enable_flow_ in uninstall UI code. 
> > 
> > I would say, the code is cleaner with current extension_id_prompting_ test.
> What
> > do you think?
> 
> that's a reasonable explanation. Can you add "(either enable or uninstall)" to
> the comment about extension_id_prompting_ in the header file?

Done.

[xiyuan, 2013-01-07 21:12:02]

sky, I've got estade and benwells reviewed the CL.

Could you do an owner's stamp? Thanks.

[sky, 2013-01-07 22:01:15]

LGTM

[commit-bot: I haz the power, 2013-01-07 23:28:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xiyuan@chromium.org/11644077/13001

[commit-bot: I haz the power, 2013-01-08 03:19:31]

Change committed as 175483