Moved app shortcut code out of ExtensionService.

Moved app shortcut code out of ExtensionService.

This code now lives in its own unit and is triggered by
NOTIFICATION_EXTENSION_INSTALLED. The code to disable shortcut creation
in browser tests has also been cleaned up.

BUG=112247
TEST=Manually tested shortcut creation


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=121138

[koz (OOO until 15th September), 2012-02-03 04:16:37]

lgtm

http://codereview.chromium.org/9310079/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.cc (left):

http://codereview.chromium.org/9310079/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:2257: // TODO(benwells): Remove
before launching platform apps.
It looks like this change preserves this behaviour, so should you move this TODO
into AppShortcutManager?

[benwells, 2012-02-03 04:37:38]

http://codereview.chromium.org/9310079/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.cc (left):

http://codereview.chromium.org/9310079/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:2257: // TODO(benwells): Remove
before launching platform apps.
On 2012/02/03 04:16:37, koz wrote:
> It looks like this change preserves this behaviour, so should you move this
TODO
> into AppShortcutManager?

My understanding is the feature will stay, possibly with some control over what
shortcuts get installed and / or a change to what shortcuts get installed by
default. So I took out the TODO :)

[Aaron Boodman, 2012-02-05 05:45:27]

http://codereview.chromium.org/9310079/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_browsertest.cc (right):

http://codereview.chromium.org/9310079/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_browsertest.cc:265:
service->app_shortcut_manager()->set_disable_shortcut_creation(true);
A better chokepoint might be ExtensionBrowserTest's constructor (and unset in
destructor for when browser tests are run in-process)?

[Aaron Boodman, 2012-02-05 05:45:43]

Thanks for fixing this btw!

[benwells, 2012-02-06 05:01:48]

http://codereview.chromium.org/9310079/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_browsertest.cc (right):

http://codereview.chromium.org/9310079/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_browsertest.cc:265:
service->app_shortcut_manager()->set_disable_shortcut_creation(true);
On 2012/02/05 05:45:27, Aaron Boodman wrote:
> A better chokepoint might be ExtensionBrowserTest's constructor (and unset in
> destructor for when browser tests are run in-process)?

When the test is being constructed there is no browser (or profile etc.)
available. When its getting destroyed the browser() is still around but trying
to use the profile causes crashes.

An alternative would be to add Setup / Teardown to EBT. I can investigate this
more, but the shortcut manager gets recreated for each test when run in-process;
maybe it is OK as it is.

[Aaron Boodman, 2012-02-06 06:46:53]

On Mon, Feb 6, 2012 at 12:01 AM,  <benwells@chromium.org> wrote:
>
>
http://codereview.chromium.org/9310079/diff/2001/chrome/browser/extensions/ex...
> File chrome/browser/extensions/extension_browsertest.cc (right):
>
>
http://codereview.chromium.org/9310079/diff/2001/chrome/browser/extensions/ex...
> chrome/browser/extensions/extension_browsertest.cc:265:
> service->app_shortcut_manager()->set_disable_shortcut_creation(true);
> On 2012/02/05 05:45:27, Aaron Boodman wrote:
>>
>> A better chokepoint might be ExtensionBrowserTest's constructor (and
>
> unset in
>>
>> destructor for when browser tests are run in-process)?
>
>
> When the test is being constructed there is no browser (or profile etc.)
> available. When its getting destroyed the browser() is still around but
> trying to use the profile causes crashes.
>
> An alternative would be to add Setup / Teardown to EBT. I can
> investigate this more, but the shortcut manager gets recreated for each
> test when run in-process; maybe it is OK as it is.

Ah, OK. I didn't realize that. It would still be nice to find one
location to call this method on that works for all cases. Is that
possible?

- a

[Mihai Parparita -not on Chrome, 2012-02-06 22:35:53]

http://codereview.chromium.org/9310079/diff/2001/chrome/browser/extensions/ap...
File chrome/browser/extensions/app_shortcut_manager.cc (right):

http://codereview.chromium.org/9310079/diff/2001/chrome/browser/extensions/ap...
chrome/browser/extensions/app_shortcut_manager.cc:40: DCHECK(type ==
chrome::NOTIFICATION_EXTENSION_INSTALLED);
We may want this for when an extension is updated too (it's possible for
extensions to change types, and Docs is looking to change their current hosted
app to a platform app).

http://codereview.chromium.org/9310079/diff/2001/chrome/browser/extensions/ap...
File chrome/browser/extensions/app_shortcut_manager.h (right):

http://codereview.chromium.org/9310079/diff/2001/chrome/browser/extensions/ap...
chrome/browser/extensions/app_shortcut_manager.h:36: void
set_disable_shortcut_creation(bool val) {
This should have a for_testing suffix so that it's not callable from non-test
code.

[benwells, 2012-02-07 06:57:01]

http://codereview.chromium.org/9310079/diff/2001/chrome/browser/extensions/ap...
File chrome/browser/extensions/app_shortcut_manager.cc (right):

http://codereview.chromium.org/9310079/diff/2001/chrome/browser/extensions/ap...
chrome/browser/extensions/app_shortcut_manager.cc:40: DCHECK(type ==
chrome::NOTIFICATION_EXTENSION_INSTALLED);
On 2012/02/06 22:35:53, Mihai Parparita wrote:
> We may want this for when an extension is updated too (it's possible for
> extensions to change types, and Docs is looking to change their current hosted
> app to a platform app).

That makes sense. We'd only want to create the shortcuts when the extension
first becomes a platform app. Maybe I can look at this in a separate change? A
similar thing we need to do is remove shortcuts at uninstall.

http://codereview.chromium.org/9310079/diff/2001/chrome/browser/extensions/ap...
File chrome/browser/extensions/app_shortcut_manager.h (right):

http://codereview.chromium.org/9310079/diff/2001/chrome/browser/extensions/ap...
chrome/browser/extensions/app_shortcut_manager.h:36: void
set_disable_shortcut_creation(bool val) {
On 2012/02/06 22:35:53, Mihai Parparita wrote:
> This should have a for_testing suffix so that it's not callable from non-test
> code.

Neat, I didn't know that suffix did that!

Done for the renamed static function.

http://codereview.chromium.org/9310079/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_browsertest.cc (right):

http://codereview.chromium.org/9310079/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_browsertest.cc:265:
service->app_shortcut_manager()->set_disable_shortcut_creation(true);
On 2012/02/06 05:01:48, benwells wrote:
> On 2012/02/05 05:45:27, Aaron Boodman wrote:
> > A better chokepoint might be ExtensionBrowserTest's constructor (and unset
in
> > destructor for when browser tests are run in-process)?
> 
> When the test is being constructed there is no browser (or profile etc.)
> available. When its getting destroyed the browser() is still around but trying
> to use the profile causes crashes.
> 
> An alternative would be to add Setup / Teardown to EBT. I can investigate this
> more, but the shortcut manager gets recreated for each test when run
in-process;
> maybe it is OK as it is.

I've changed how the disabling works, its now done in single place (and feels
better).

[Aaron Boodman, 2012-02-08 00:40:14]

excellent. lgtm.

[Mihai Parparita -not on Chrome, 2012-02-08 02:18:32]

LGTM

[commit-bot: I haz the power, 2012-02-08 02:19:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/9310079/11001

[commit-bot: I haz the power, 2012-02-08 03:06:04]

Try job failure for 9310079-11001 (retry) on linux_clang for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2012-02-08 05:00:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/9310079/22002

[commit-bot: I haz the power, 2012-02-08 07:22:35]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is win_rel, revision is 120909, job name
was 9310079-22002 (retry) (previous was lost) (previous was lost) (previous was
lost).

[commit-bot: I haz the power, 2012-02-08 07:47:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/9310079/22002

[commit-bot: I haz the power, 2012-02-08 08:52:09]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is linux_rel, revision is 120952, job name
was 9310079-22002 (previous was lost) (previous was lost) (previous was lost)
(previous was lost).

[commit-bot: I haz the power, 2012-02-08 16:45:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/9310079/22002

[commit-bot: I haz the power, 2012-02-08 17:18:24]

Try job failure for 9310079-22002 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-02-08 22:17:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/9310079/22002

[commit-bot: I haz the power, 2012-02-09 01:49:39]

Change committed as 121138