Apply new WM shadows to app list.

Apply new WM shadows to app list.

Also clean up/streamline AppListView.
1. Remove code that supported arrows + view anchoring
2. Remove code that supported !CrOS
3. Remove dead code for making bubble borders accept events
4. Remove AppListViewObserver which is dead and who knows what it was used for.
5. Stop building //ui/app_list on all aura (just build on CrOS)

BUG=608852
TBR=derat@chromium.org

Committed: https://crrev.com/835d550911b618ebf077fbe1221a779842ef56ad
Cr-Commit-Position: refs/heads/master@{#441394}

[Evan Stade, 2016-12-20 00:16:41]

estade@chromium.org changed reviewers:
+ mgiuca@chromium.org, msw@chromium.org

[Evan Stade, 2016-12-20 00:16:43]

+mgiuca for *app_list*
+msw for ui/views/bubble

[Evan Stade, 2016-12-20 00:16:51]

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

[commit-bot: I haz the power, 2016-12-20 00:17:45]

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, 2016-12-20 00:46:01]

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

[commit-bot: I haz the power, 2016-12-20 00:46:03]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[msw, 2016-12-20 15:58:10]

Nice cleanup; lgtm with a nit and a q.

https://codereview.chromium.org/2588103004/diff/1/ui/app_list/views/app_list_...
File ui/app_list/views/app_list_view.cc (right):

https://codereview.chromium.org/2588103004/diff/1/ui/app_list/views/app_list_...
ui/app_list/views/app_list_view.cc:214:
window->SetEventTargeter(std::unique_ptr<ui::EventTargeter>(
nit: MakeUnique

https://codereview.chromium.org/2588103004/diff/1/ui/app_list/views/app_list_...
ui/app_list/views/app_list_view.cc:217: const int kOverlayCornerRadius =
q: Do we still need AppListOverlayView?

[Evan Stade, 2016-12-20 17:51:29]

Description was changed from

==========
Apply new WM shadows to app list.

Also clean up/streamline AppListView.
1. Remove code that supported arrows + view anchoring
2. Remove code that supported !CrOS
3. Remove dead code for making bubble borders accept events

BUG=608852
==========

to

==========
Apply new WM shadows to app list.

Also clean up/streamline AppListView.
1. Remove code that supported arrows + view anchoring
2. Remove code that supported !CrOS
3. Remove dead code for making bubble borders accept events
4. Remove AppListViewObserver which is dead and who knows what it was used for.
5. Stop building //ui/app_list on all aura (just build on CrOS)

BUG=608852
==========

[Evan Stade, 2016-12-20 17:51:58]

new changes: see CL desc bullet points #4 & #5

https://codereview.chromium.org/2588103004/diff/1/ui/app_list/views/app_list_...
File ui/app_list/views/app_list_view.cc (right):

https://codereview.chromium.org/2588103004/diff/1/ui/app_list/views/app_list_...
ui/app_list/views/app_list_view.cc:214:
window->SetEventTargeter(std::unique_ptr<ui::EventTargeter>(
On 2016/12/20 15:58:10, msw wrote:
> nit: MakeUnique

Done.

https://codereview.chromium.org/2588103004/diff/1/ui/app_list/views/app_list_...
ui/app_list/views/app_list_view.cc:217: const int kOverlayCornerRadius =
On 2016/12/20 15:58:10, msw wrote:
> q: Do we still need AppListOverlayView?

This patch doesn't affect whether we need that or not, but it's a good question
nonetheless. Here's what I found: the overlay is still being used; it provides a
white shield on top of the app list view when the app info dialog (a modal
child) is showing on top of it. That was added here[1]. Back then, there was a
10px border around the app info dialog, but that didn't last long[2] --- now
it's full-bleed, so the overlay_view_ and the rest of the app list is completely
hidden. When you close the app info dialog, you get to see the overlay fade out
for 125ms. So, basically worthless.

However, it's also used when showing the uninstall dialog, which looks like
this[3] so it's still useful in that case.

[1] https://codereview.chromium.org/276833002
[2] https://codereview.chromium.org/306023011
[3] https://screenshot.googleplex.com/edDL9Qmt0nS.png

[Evan Stade, 2016-12-20 17:53:14]

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

[commit-bot: I haz the power, 2016-12-20 17:53:33]

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, 2016-12-20 18:06:15]

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

[commit-bot: I haz the power, 2016-12-20 18:06:16]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[msw, 2016-12-20 19:16:10]

https://codereview.chromium.org/2588103004/diff/1/ui/app_list/views/app_list_...
File ui/app_list/views/app_list_view.cc (right):

https://codereview.chromium.org/2588103004/diff/1/ui/app_list/views/app_list_...
ui/app_list/views/app_list_view.cc:214:
window->SetEventTargeter(std::unique_ptr<ui::EventTargeter>(
On 2016/12/20 17:51:57, Evan Stade wrote:
> On 2016/12/20 15:58:10, msw wrote:
> > nit: MakeUnique
> 
> Done.

You changed line 433, please update this one [too].

https://codereview.chromium.org/2588103004/diff/40001/ui/app_list/demo/app_li...
File ui/app_list/demo/app_list_demo_views.cc (right):

https://codereview.chromium.org/2588103004/diff/40001/ui/app_list/demo/app_li...
ui/app_list/demo/app_list_demo_views.cc:61: container = window_context;
nit: merge decl and init

https://codereview.chromium.org/2588103004/diff/40001/ui/app_list/views/app_l...
File ui/app_list/views/app_list_view.cc (right):

https://codereview.chromium.org/2588103004/diff/40001/ui/app_list/views/app_l...
ui/app_list/views/app_list_view.cc:296: const float kOverlayFadeInMilliseconds =
1250;
Revert this

https://codereview.chromium.org/2588103004/diff/40001/ui/app_list/views/app_l...
File ui/app_list/views/app_list_view_observer.h (left):

https://codereview.chromium.org/2588103004/diff/40001/ui/app_list/views/app_l...
ui/app_list/views/app_list_view_observer.h:14: class AppListViewObserver {
You'll need to remove use in
chrome/browser/ui/views/app_list/linux/app_list_service_linux.h

[Evan Stade, 2016-12-21 16:58:52]

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

[Evan Stade, 2016-12-21 17:00:57]

-mgiuca, +tapted

I haven't addressed Mike's comments yet, but I think you can still review (and
make sure I'm not misunderstanding or unintentionally breaking something).

https://codereview.chromium.org/2588103004/diff/1/ui/app_list/views/app_list_...
File ui/app_list/views/app_list_view.cc (right):

https://codereview.chromium.org/2588103004/diff/1/ui/app_list/views/app_list_...
ui/app_list/views/app_list_view.cc:214:
window->SetEventTargeter(std::unique_ptr<ui::EventTargeter>(
On 2016/12/20 19:16:10, msw wrote:
> On 2016/12/20 17:51:57, Evan Stade wrote:
> > On 2016/12/20 15:58:10, msw wrote:
> > > nit: MakeUnique
> > 
> > Done.
> 
> You changed line 433, please update this one [too].

oops

[tapted, 2016-12-21 22:39:58]

app_list lgtm with the following

https://codereview.chromium.org/2588103004/diff/40001/ui/app_list/BUILD.gn
File ui/app_list/BUILD.gn (right):

https://codereview.chromium.org/2588103004/diff/40001/ui/app_list/BUILD.gn#ne...
ui/app_list/BUILD.gn:218: if (is_win) {
This is_win bit can be removed too

https://codereview.chromium.org/2588103004/diff/40001/ui/app_list/views/app_l...
File ui/app_list/views/app_list_view.cc (left):

https://codereview.chromium.org/2588103004/diff/40001/ui/app_list/views/app_l...
ui/app_list/views/app_list_view.cc:622: // Do not called inherited function as
the bubble delegate auto close
So long as you've checked that clicking outside the app list on ChromeOS still
dismisses it properly. I think this is fine - CrOS wasn't hooked up as an
observer, and calling the parent OnWidgetActivationChanged should be OK since
AppListView calls set_deactivate_on_close(false) during initialization.  (But I
don't actually recall where Ash hooks in to dismiss the app list when clicking
outside it..)

https://codereview.chromium.org/2588103004/diff/40001/ui/app_list/views/app_l...
File ui/app_list/views/app_list_view.cc (right):

https://codereview.chromium.org/2588103004/diff/40001/ui/app_list/views/app_l...
ui/app_list/views/app_list_view.cc:214:
window->SetEventTargeter(std::unique_ptr<ui::EventTargeter>(
nit: MakeUnique?

https://codereview.chromium.org/2588103004/diff/40001/ui/app_list/views/app_l...
File ui/app_list/views/app_list_view_observer.h (left):

https://codereview.chromium.org/2588103004/diff/40001/ui/app_list/views/app_l...
ui/app_list/views/app_list_view_observer.h:14: class AppListViewObserver {
On 2016/12/20 19:16:10, msw wrote:
> You'll need to remove use in
> chrome/browser/ui/views/app_list/linux/app_list_service_linux.h

https://codereview.chromium.org/2582743002/ is in the CQ to remove this \o/

https://codereview.chromium.org/2588103004/diff/40001/ui/views/bubble/bubble_...
File ui/views/bubble/bubble_dialog_delegate.h (right):

https://codereview.chromium.org/2588103004/diff/40001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_dialog_delegate.h:196: bool border_accepts_events_;
remove? (or initialize..)

[Evan Stade, 2016-12-29 17:23:13]

https://codereview.chromium.org/2588103004/diff/40001/ui/app_list/BUILD.gn
File ui/app_list/BUILD.gn (right):

https://codereview.chromium.org/2588103004/diff/40001/ui/app_list/BUILD.gn#ne...
ui/app_list/BUILD.gn:218: if (is_win) {
On 2016/12/21 22:39:58, tapted (OOO until 2017-01-03) wrote:
> This is_win bit can be removed too

Done.

https://codereview.chromium.org/2588103004/diff/40001/ui/app_list/demo/app_li...
File ui/app_list/demo/app_list_demo_views.cc (right):

https://codereview.chromium.org/2588103004/diff/40001/ui/app_list/demo/app_li...
ui/app_list/demo/app_list_demo_views.cc:61: container = window_context;
On 2016/12/20 19:16:10, msw wrote:
> nit: merge decl and init

Done.

https://codereview.chromium.org/2588103004/diff/40001/ui/app_list/views/app_l...
File ui/app_list/views/app_list_view.cc (left):

https://codereview.chromium.org/2588103004/diff/40001/ui/app_list/views/app_l...
ui/app_list/views/app_list_view.cc:622: // Do not called inherited function as
the bubble delegate auto close
On 2016/12/21 22:39:58, tapted (OOO until 2017-01-03) wrote:
> So long as you've checked that clicking outside the app list on ChromeOS still
> dismisses it properly. I think this is fine - CrOS wasn't hooked up as an
> observer, and calling the parent OnWidgetActivationChanged should be OK since
> AppListView calls set_deactivate_on_close(false) during initialization.  (But
I
> don't actually recall where Ash hooks in to dismiss the app list when clicking
> outside it..)

it does still close. Not the easiest thing to track down because stack traces
didn't prove very useful but it's this:

https://cs.chromium.org/chromium/src/ash/app_list/app_list_presenter_delegate...

https://codereview.chromium.org/2588103004/diff/40001/ui/app_list/views/app_l...
File ui/app_list/views/app_list_view.cc (right):

https://codereview.chromium.org/2588103004/diff/40001/ui/app_list/views/app_l...
ui/app_list/views/app_list_view.cc:214:
window->SetEventTargeter(std::unique_ptr<ui::EventTargeter>(
On 2016/12/21 22:39:58, tapted (OOO until 2017-01-03) wrote:
> nit: MakeUnique?

Done.

https://codereview.chromium.org/2588103004/diff/40001/ui/app_list/views/app_l...
ui/app_list/views/app_list_view.cc:296: const float kOverlayFadeInMilliseconds =
1250;
On 2016/12/20 19:16:10, msw wrote:
> Revert this

Done.

https://codereview.chromium.org/2588103004/diff/40001/ui/app_list/views/app_l...
File ui/app_list/views/app_list_view_observer.h (left):

https://codereview.chromium.org/2588103004/diff/40001/ui/app_list/views/app_l...
ui/app_list/views/app_list_view_observer.h:14: class AppListViewObserver {
On 2016/12/21 22:39:58, tapted (OOO until 2017-01-03) wrote:
> On 2016/12/20 19:16:10, msw wrote:
> > You'll need to remove use in
> > chrome/browser/ui/views/app_list/linux/app_list_service_linux.h
> 
> https://codereview.chromium.org/2582743002/ is in the CQ to remove this \o/

Trent to the rescue!

https://codereview.chromium.org/2588103004/diff/40001/ui/views/bubble/bubble_...
File ui/views/bubble/bubble_dialog_delegate.h (right):

https://codereview.chromium.org/2588103004/diff/40001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_dialog_delegate.h:196: bool border_accepts_events_;
On 2016/12/21 22:39:58, tapted (OOO until 2017-01-03) wrote:
> remove? (or initialize..)

Done.

[Evan Stade, 2016-12-29 17:23:33]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-12-29 17:23:33]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org,
tapted@chromium.org
Link to the patchset: https://codereview.chromium.org/2588103004/#ps60001
(title: "remove header")

[commit-bot: I haz the power, 2016-12-29 17:23:47]

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, 2016-12-29 17:30:16]

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

[commit-bot: I haz the power, 2016-12-29 17:30:17]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Evan Stade, 2016-12-29 21:20:32]

estade@chromium.org changed reviewers:
+ derat@chromium.org, dpranke@chromium.org

[Evan Stade, 2016-12-29 21:20:33]

+derat for ash/ (TBR)
+dpranke for //BUILD.gn

[Evan Stade, 2016-12-29 21:27:31]

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

[commit-bot: I haz the power, 2016-12-29 21:27:47]

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, 2016-12-29 22:31:51]

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

[commit-bot: I haz the power, 2016-12-29 22:31:52]

Dry run: This issue passed the CQ dry run.

[Dirk Pranke, 2017-01-04 02:54:42]

//BUILD.gn lgtm.

[Evan Stade, 2017-01-04 15:28:27]

Description was changed from

==========
Apply new WM shadows to app list.

Also clean up/streamline AppListView.
1. Remove code that supported arrows + view anchoring
2. Remove code that supported !CrOS
3. Remove dead code for making bubble borders accept events
4. Remove AppListViewObserver which is dead and who knows what it was used for.
5. Stop building //ui/app_list on all aura (just build on CrOS)

BUG=608852
==========

to

==========
Apply new WM shadows to app list.

Also clean up/streamline AppListView.
1. Remove code that supported arrows + view anchoring
2. Remove code that supported !CrOS
3. Remove dead code for making bubble borders accept events
4. Remove AppListViewObserver which is dead and who knows what it was used for.
5. Stop building //ui/app_list on all aura (just build on CrOS)

BUG=608852
TBR=derat@chromium.org
==========

[Evan Stade, 2017-01-04 15:28:33]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2017-01-04 15:28:34]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org,
msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2588103004/#ps80001
(title: "remove references on testing bots")

[commit-bot: I haz the power, 2017-01-04 15:29:07]

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-01-04 16:44:10]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1483543713595640,
"parent_rev": "ac344ab1e83c9bdd65697990f0105a98bad403ae", "commit_rev":
"42e74a0edb43fe93b6eea6ab6edfdcc97c988159"}

[commit-bot: I haz the power, 2017-01-04 16:44:39]

Description was changed from

==========
Apply new WM shadows to app list.

Also clean up/streamline AppListView.
1. Remove code that supported arrows + view anchoring
2. Remove code that supported !CrOS
3. Remove dead code for making bubble borders accept events
4. Remove AppListViewObserver which is dead and who knows what it was used for.
5. Stop building //ui/app_list on all aura (just build on CrOS)

BUG=608852
TBR=derat@chromium.org
==========

to

==========
Apply new WM shadows to app list.

Also clean up/streamline AppListView.
1. Remove code that supported arrows + view anchoring
2. Remove code that supported !CrOS
3. Remove dead code for making bubble borders accept events
4. Remove AppListViewObserver which is dead and who knows what it was used for.
5. Stop building //ui/app_list on all aura (just build on CrOS)

BUG=608852
TBR=derat@chromium.org

Review-Url: https://codereview.chromium.org/2588103004
==========

[commit-bot: I haz the power, 2017-01-04 16:44:41]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2017-01-04 16:48:00]

Description was changed from

==========
Apply new WM shadows to app list.

Also clean up/streamline AppListView.
1. Remove code that supported arrows + view anchoring
2. Remove code that supported !CrOS
3. Remove dead code for making bubble borders accept events
4. Remove AppListViewObserver which is dead and who knows what it was used for.
5. Stop building //ui/app_list on all aura (just build on CrOS)

BUG=608852
TBR=derat@chromium.org

Review-Url: https://codereview.chromium.org/2588103004
==========

to

==========
Apply new WM shadows to app list.

Also clean up/streamline AppListView.
1. Remove code that supported arrows + view anchoring
2. Remove code that supported !CrOS
3. Remove dead code for making bubble borders accept events
4. Remove AppListViewObserver which is dead and who knows what it was used for.
5. Stop building //ui/app_list on all aura (just build on CrOS)

BUG=608852
TBR=derat@chromium.org

Committed: https://crrev.com/835d550911b618ebf077fbe1221a779842ef56ad
Cr-Commit-Position: refs/heads/master@{#441394}
==========

[commit-bot: I haz the power, 2017-01-04 16:48:01]

Patchset 5 (id:??) landed as
https://crrev.com/835d550911b618ebf077fbe1221a779842ef56ad
Cr-Commit-Position: refs/heads/master@{#441394}

[Daniel Erat, 2017-01-06 21:54:04]

lgtm for ash