Remove more dead app list code.

Remove more dead app list code.

App list is only compiled on ChromeOS now, so a bunch of stuff is
unused. There's more to audit, but this CL deals with some of the more
mechanical stuff - #ifdefs, profile switching, and shortcut creation.

BUG=600915
Review-Url: https://codereview.chromium.org/2787693002
Cr-Commit-Position: refs/heads/master@{#461353}
Committed: https://chromium.googlesource.com/chromium/src/+/9e03960f75b3019372157db5c4fe5264dfc36616

[tapted, 2017-03-30 00:03:02]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 00:05:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 00:14:02]

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

[commit-bot: I haz the power, 2017-03-30 00:14:02]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tapted, 2017-03-30 04:48:10]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 04:48:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-03-31 00:23:10]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 00:24:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 02:04:32]

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

[commit-bot: I haz the power, 2017-03-31 02:04:34]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-03-31 03:09:11]

Patchset #1 (id:1) has been deleted

[tapted, 2017-03-31 03:09:23]

Patchset #1 (id:20001) has been deleted

[tapted, 2017-03-31 03:09:32]

Patchset #1 (id:40001) has been deleted

[tapted, 2017-03-31 03:09:41]

Patchset #1 (id:60001) has been deleted

[tapted, 2017-03-31 03:11:25]

Description was changed from

==========
Remove more dead app list code

BUG=600915
==========

to

==========
Remove more dead app list code.

App list is only compiled on ChromeOS now, so a bunch of stuff is
unused. There's more to audit, but this CL deals with some of the more
mechanical stuff - #ifdefs, profile switching, and shortcut creation.

BUG=600915
==========

[tapted, 2017-03-31 03:11:25]

tapted@chromium.org changed reviewers:
+ mgiuca@chromium.org

[tapted, 2017-03-31 03:13:46]

Hi Matt, please take a look.

[Matt Giuca, 2017-03-31 05:29:20]

Lgtm with one question.

Thanks for tackling this!

https://codereview.chromium.org/2787693002/diff/80001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/2787693002/diff/80001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_view_delegate.cc:134: // Note that the
destructor is not always called. Essential shutdown work must
Is this no longer true? (If the only example provided was "on Mac", maybe it
*is* always called now?)

[tapted, 2017-03-31 05:55:25]

https://codereview.chromium.org/2787693002/diff/80001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/2787693002/diff/80001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_view_delegate.cc:134: // Note that the
destructor is not always called. Essential shutdown work must
On 2017/03/31 05:29:20, Matt Giuca wrote:
> Is this no longer true? (If the only example provided was "on Mac", maybe it
> *is* always called now?)

So.. I *think* is is no longer true. Pretty sure that when we were playing with
it, AppListViewDelegate would get re-created every time the app list was shown
on ChromeOS. but I haven't kept track (and for performance, ChromeOS really
shouldn't be doing that). And if it is actually no longer true then I also need
to get rid of chrome::NOTIFICATION_APP_TERMINATING since there's no longer any
point listening for it.

Best fix for this I think is to have ChromeOS cache this lazily so that it isn't
recreated a whole bunch... which is why I left this here.

[tapted, 2017-03-31 10:58:02]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 10:58:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-03-31 10:59:22]

tapted@chromium.org changed reviewers:
+ jamescook@chromium.org

[tapted, 2017-03-31 10:59:23]

+jamescook for ash/shell OWNERS

https://codereview.chromium.org/2787693002/diff/80001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/2787693002/diff/80001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_view_delegate.cc:134: // Note that the
destructor is not always called. Essential shutdown work must
On 2017/03/31 05:55:25, tapted wrote:
> On 2017/03/31 05:29:20, Matt Giuca wrote:
> > Is this no longer true? (If the only example provided was "on Mac", maybe it
> > *is* always called now?)
> 
> So.. I *think* is is no longer true. Pretty sure that when we were playing
with
> it, AppListViewDelegate would get re-created every time the app list was shown
> on ChromeOS. but I haven't kept track (and for performance, ChromeOS really
> shouldn't be doing that). And if it is actually no longer true then I also
need
> to get rid of chrome::NOTIFICATION_APP_TERMINATING since there's no longer any
> point listening for it.
> 
> Best fix for this I think is to have ChromeOS cache this lazily so that it
isn't
> recreated a whole bunch... which is why I left this here.

Poked around - it looks like AppListServiceAsh is now a leaky singleton the way
Mac was.

changed comment: 
  // The destructor might not be called since the delegate is owned by a leaky
  // singleton. This matches the shutdown work done in Observe() in response to
  // chrome::NOTIFICATION_APP_TERMINATING, which may happen before this.

[commit-bot: I haz the power, 2017-03-31 12:01:43]

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

[commit-bot: I haz the power, 2017-03-31 12:01:45]

Dry run: This issue passed the CQ dry run.

[James Cook, 2017-03-31 19:20:07]

ash/shell LGTM

One question, though. You mention in the CL description "profile switching" not
being needed. Chrome OS does show different app lists when using multiprofile
("fast user switching"). Is the code being removed a different/separate
mechanism?

Thanks for cleaning all this up!

[tapted, 2017-04-02 23:19:42]

On 2017/03/31 19:20:07, James Cook wrote:
> ash/shell LGTM
> 
> One question, though. You mention in the CL description "profile switching"
not
> being needed. Chrome OS does show different app lists when using multiprofile
> ("fast user switching"). Is the code being removed a different/separate
> mechanism?

Yes - it's a different mechanism. On Windows/Linux/Mac the profile being used on
the app list itself could be switched via an app-list-specific hotdog menu that
appeared in the search box and showed a list of users associated with a profile
path. That menu also had `Help` and `Feedback` menu items which is why those
delegate methods are also no longer needed.

[tapted, 2017-04-02 23:20:10]

The CQ bit was checked by tapted@chromium.org

[tapted, 2017-04-02 23:20:11]

The patchset sent to the CQ was uploaded after l-g-t-m from mgiuca@chromium.org
Link to the patchset: https://codereview.chromium.org/2787693002/#ps100001
(title: "updated comment")

[commit-bot: I haz the power, 2017-04-02 23:20:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Matt Giuca, 2017-04-02 23:59:25]

slgtm (post comment change).

[commit-bot: I haz the power, 2017-04-03 00:23:11]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491175210795840,
"parent_rev": "02c25323de5c3f16bb47d5dc6fc6c7b9a939b784", "commit_rev":
"9e03960f75b3019372157db5c4fe5264dfc36616"}

[commit-bot: I haz the power, 2017-04-03 00:25:06]

Description was changed from

==========
Remove more dead app list code.

App list is only compiled on ChromeOS now, so a bunch of stuff is
unused. There's more to audit, but this CL deals with some of the more
mechanical stuff - #ifdefs, profile switching, and shortcut creation.

BUG=600915
==========

to

==========
Remove more dead app list code.

App list is only compiled on ChromeOS now, so a bunch of stuff is
unused. There's more to audit, but this CL deals with some of the more
mechanical stuff - #ifdefs, profile switching, and shortcut creation.

BUG=600915

Review-Url: https://codereview.chromium.org/2787693002
Cr-Commit-Position: refs/heads/master@{#461353}
Committed:
https://chromium.googlesource.com/chromium/src/+/9e03960f75b3019372157db5c4fe...
==========

[commit-bot: I haz the power, 2017-04-03 00:25:08]

Committed patchset #2 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/9e03960f75b3019372157db5c4fe...