New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 696028 link

Starred by 1 user
Status: Archived
Owner:
msw@chromium.org
Closed: Aug 2017
Cc:
█ mfomitchev@chromium.org
mustash-bugs@google.com
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug

Blocking:
issue 693114



Sign in to add a comment

TestAppListViewPresenterImpl causes number of test failures in mash

Project Member Reported by sky@chromium.org, Feb 24 2017 
Maybe they should use ExampleAppListPresenter? This needs to be investigated.
Project Member

Comment 1 by bugdroid1@chromium.org, Feb 27 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/56ce72b787bcd211e206ec4d49bb06f07590ca3f

commit 56ce72b787bcd211e206ec4d49bb06f07590ca3f
Author: sky <sky@chromium.org>
Date: Mon Feb 27 19:07:06 2017

chromeos: makes more tests run in both mash and ash

BUG= 693114 , 581462 ,647438, 695628 , 695629 , 695632 ,695640, 695686 , 695751 ,695758, 695887 , 696006 , 696028 
TEST=test only changes
R=msw@chromium.org

Review-Url: https://codereview.chromium.org/2718763003
Cr-Commit-Position: refs/heads/master@{#453280}

[modify] https://crrev.com/56ce72b787bcd211e206ec4d49bb06f07590ca3f/ash/BUILD.gn
[modify] https://crrev.com/56ce72b787bcd211e206ec4d49bb06f07590ca3f/ash/metrics/desktop_task_switch_metric_recorder_unittest.cc
[modify] https://crrev.com/56ce72b787bcd211e206ec4d49bb06f07590ca3f/ash/metrics/user_metrics_recorder_unittest.cc
[modify] https://crrev.com/56ce72b787bcd211e206ec4d49bb06f07590ca3f/ash/root_window_controller_unittest.cc
[modify] https://crrev.com/56ce72b787bcd211e206ec4d49bb06f07590ca3f/ash/screen_util_unittest.cc
[modify] https://crrev.com/56ce72b787bcd211e206ec4d49bb06f07590ca3f/ash/shelf/shelf_layout_manager_unittest.cc
[modify] https://crrev.com/56ce72b787bcd211e206ec4d49bb06f07590ca3f/ash/shelf/shelf_view_unittest.cc
[modify] https://crrev.com/56ce72b787bcd211e206ec4d49bb06f07590ca3f/ash/shell_unittest.cc
[modify] https://crrev.com/56ce72b787bcd211e206ec4d49bb06f07590ca3f/ash/sticky_keys/sticky_keys_overlay_unittest.cc
[modify] https://crrev.com/56ce72b787bcd211e206ec4d49bb06f07590ca3f/ash/system/chromeos/power/tablet_power_button_controller_unittest.cc
Project Member

Comment 2 by bugdroid1@chromium.org, Feb 27 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/56ce72b787bcd211e206ec4d49bb06f07590ca3f

commit 56ce72b787bcd211e206ec4d49bb06f07590ca3f
Author: sky <sky@chromium.org>
Date: Mon Feb 27 19:07:06 2017

chromeos: makes more tests run in both mash and ash

BUG= 693114 , 581462 ,647438, 695628 , 695629 , 695632 ,695640, 695686 , 695751 ,695758, 695887 , 696006 , 696028 
TEST=test only changes
R=msw@chromium.org

Review-Url: https://codereview.chromium.org/2718763003
Cr-Commit-Position: refs/heads/master@{#453280}

[modify] https://crrev.com/56ce72b787bcd211e206ec4d49bb06f07590ca3f/ash/BUILD.gn
[modify] https://crrev.com/56ce72b787bcd211e206ec4d49bb06f07590ca3f/ash/metrics/desktop_task_switch_metric_recorder_unittest.cc
[modify] https://crrev.com/56ce72b787bcd211e206ec4d49bb06f07590ca3f/ash/metrics/user_metrics_recorder_unittest.cc
[modify] https://crrev.com/56ce72b787bcd211e206ec4d49bb06f07590ca3f/ash/root_window_controller_unittest.cc
[modify] https://crrev.com/56ce72b787bcd211e206ec4d49bb06f07590ca3f/ash/screen_util_unittest.cc
[modify] https://crrev.com/56ce72b787bcd211e206ec4d49bb06f07590ca3f/ash/shelf/shelf_layout_manager_unittest.cc
[modify] https://crrev.com/56ce72b787bcd211e206ec4d49bb06f07590ca3f/ash/shelf/shelf_view_unittest.cc
[modify] https://crrev.com/56ce72b787bcd211e206ec4d49bb06f07590ca3f/ash/shell_unittest.cc
[modify] https://crrev.com/56ce72b787bcd211e206ec4d49bb06f07590ca3f/ash/sticky_keys/sticky_keys_overlay_unittest.cc
[modify] https://crrev.com/56ce72b787bcd211e206ec4d49bb06f07590ca3f/ash/system/chromeos/power/tablet_power_button_controller_unittest.cc
Project Member

Comment 3 by bugdroid1@chromium.org, Feb 27 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/56ce72b787bcd211e206ec4d49bb06f07590ca3f

commit 56ce72b787bcd211e206ec4d49bb06f07590ca3f
Author: sky <sky@chromium.org>
Date: Mon Feb 27 19:07:06 2017

chromeos: makes more tests run in both mash and ash

BUG= 693114 , 581462 ,647438, 695628 , 695629 , 695632 ,695640, 695686 , 695751 ,695758, 695887 , 696006 , 696028 
TEST=test only changes
R=msw@chromium.org

Review-Url: https://codereview.chromium.org/2718763003
Cr-Commit-Position: refs/heads/master@{#453280}

[modify] https://crrev.com/56ce72b787bcd211e206ec4d49bb06f07590ca3f/ash/BUILD.gn
[modify] https://crrev.com/56ce72b787bcd211e206ec4d49bb06f07590ca3f/ash/metrics/desktop_task_switch_metric_recorder_unittest.cc
[modify] https://crrev.com/56ce72b787bcd211e206ec4d49bb06f07590ca3f/ash/metrics/user_metrics_recorder_unittest.cc
[modify] https://crrev.com/56ce72b787bcd211e206ec4d49bb06f07590ca3f/ash/root_window_controller_unittest.cc
[modify] https://crrev.com/56ce72b787bcd211e206ec4d49bb06f07590ca3f/ash/screen_util_unittest.cc
[modify] https://crrev.com/56ce72b787bcd211e206ec4d49bb06f07590ca3f/ash/shelf/shelf_layout_manager_unittest.cc
[modify] https://crrev.com/56ce72b787bcd211e206ec4d49bb06f07590ca3f/ash/shelf/shelf_view_unittest.cc
[modify] https://crrev.com/56ce72b787bcd211e206ec4d49bb06f07590ca3f/ash/shell_unittest.cc
[modify] https://crrev.com/56ce72b787bcd211e206ec4d49bb06f07590ca3f/ash/sticky_keys/sticky_keys_overlay_unittest.cc
[modify] https://crrev.com/56ce72b787bcd211e206ec4d49bb06f07590ca3f/ash/system/chromeos/power/tablet_power_button_controller_unittest.cc

Comment 4 by sky@chromium.org, Mar 6 2017

Cc: msw@chromium.org
Owner: mfomitchev@chromium.org
Status: Assigned (was: Untriaged)
Summary: TestAppListViewPresenterImpl causes number of test failures in mash (was: Handful of ShelfLayoutManagerTest tests fail in mash because of use of applistpresenter)
I'm renaming this to reflect it effects a number of tests.

Comment 5 by sky@chromium.org, Mar 7 2017

Blocking: 693114
Project Member

Comment 6 by bugdroid1@chromium.org, Mar 7 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ea37af717f1569dd93282e09b6fb49ff22b771f8

commit ea37af717f1569dd93282e09b6fb49ff22b771f8
Author: sky <sky@chromium.org>
Date: Tue Mar 07 22:59:41 2017

Last sets of tests that can move to common_unittests

This way they run in both mash_unittests and ash_unittests.
Interesting things of note:
. Had to remove DCHECKs for GetRootWindowForDisplayId() as some tests
  call with invalid.
. Had to rename class in mus named WindowManagerTest as it conflicts
  with test with same name in ash.

BUG= 622486 , 631103 , 637853 , 648733 ,695556, 696028 ,698091, 698129 , 698878 , 698887 ,698888,698892,698894, 698895 , 698914 ,699172,699175, 693114 
TEST=test changes
R=msw@chromium.org

Review-Url: https://codereview.chromium.org/2739703003
Cr-Commit-Position: refs/heads/master@{#455272}

[modify] https://crrev.com/ea37af717f1569dd93282e09b6fb49ff22b771f8/ash/BUILD.gn
[modify] https://crrev.com/ea37af717f1569dd93282e09b6fb49ff22b771f8/ash/mus/bridge/wm_shell_mus.cc
[modify] https://crrev.com/ea37af717f1569dd93282e09b6fb49ff22b771f8/ash/mus/window_manager_unittest.cc
[modify] https://crrev.com/ea37af717f1569dd93282e09b6fb49ff22b771f8/ash/test/ash_test_base.cc
[modify] https://crrev.com/ea37af717f1569dd93282e09b6fb49ff22b771f8/ash/wm/overview/window_selector_unittest.cc
[modify] https://crrev.com/ea37af717f1569dd93282e09b6fb49ff22b771f8/ash/wm/panels/panel_layout_manager_unittest.cc
[modify] https://crrev.com/ea37af717f1569dd93282e09b6fb49ff22b771f8/ash/wm/panels/panel_window_resizer_unittest.cc
[modify] https://crrev.com/ea37af717f1569dd93282e09b6fb49ff22b771f8/ash/wm/system_modal_container_layout_manager_unittest.cc
[modify] https://crrev.com/ea37af717f1569dd93282e09b6fb49ff22b771f8/ash/wm/window_cycle_controller_unittest.cc
[modify] https://crrev.com/ea37af717f1569dd93282e09b6fb49ff22b771f8/ash/wm/window_manager_unittest.cc
[modify] https://crrev.com/ea37af717f1569dd93282e09b6fb49ff22b771f8/ash/wm/window_modality_controller_unittest.cc
[modify] https://crrev.com/ea37af717f1569dd93282e09b6fb49ff22b771f8/ash/wm/window_state_unittest.cc
[modify] https://crrev.com/ea37af717f1569dd93282e09b6fb49ff22b771f8/ash/wm/window_util_unittest.cc
[modify] https://crrev.com/ea37af717f1569dd93282e09b6fb49ff22b771f8/ash/wm/workspace/workspace_event_handler_unittest.cc
[modify] https://crrev.com/ea37af717f1569dd93282e09b6fb49ff22b771f8/ash/wm/workspace/workspace_window_resizer_unittest.cc
Project Member

Comment 7 by bugdroid1@chromium.org, Aug 22 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b3a073579dffb23de8ebe1f181361b7f2a3d1ba7

commit b3a073579dffb23de8ebe1f181361b7f2a3d1ba7
Author: Mike Wasserman <msw@chromium.org>
Date: Tue Aug 22 23:25:14 2017

mash: Fix the app list button's ink drop deactivation.

Always use AppListDelegateImpl for Shell AppList notifications.
(avoid AppListPresenterDelegate[Mus] duplication/disparity)

Remove the delegate removal TODO, may relate to crbug.com/733662
(the delegate may be necessary until ui/app_list moves into ash)

Fix root window checks (they're null for invalid/old display ids).

Rename TestAppListPresenterImpl, connect with the ash app list.
Add helpers to spin a run loop on app list show/dismiss (via mojo).

Enable numerous tests on Mash that rely on the presenter, etc.
Avoid mock app list notifications when possible in tests.

Bug:  730887 ,  696028 ,  695751 
Change-Id: I19a3f8adc7ca66a7ad81fb254198c96fb4428ae3
Reviewed-on: https://chromium-review.googlesource.com/624515
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Commit-Queue: Michael Wasserman <msw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#496502}
[modify] https://crrev.com/b3a073579dffb23de8ebe1f181361b7f2a3d1ba7/ash/BUILD.gn
[modify] https://crrev.com/b3a073579dffb23de8ebe1f181361b7f2a3d1ba7/ash/app_list/app_list_delegate_impl.cc
[modify] https://crrev.com/b3a073579dffb23de8ebe1f181361b7f2a3d1ba7/ash/app_list/app_list_presenter_delegate.cc
[modify] https://crrev.com/b3a073579dffb23de8ebe1f181361b7f2a3d1ba7/ash/app_list/app_list_presenter_delegate_unittest.cc
[add] https://crrev.com/b3a073579dffb23de8ebe1f181361b7f2a3d1ba7/ash/app_list/test_app_list_presenter_impl.cc
[add] https://crrev.com/b3a073579dffb23de8ebe1f181361b7f2a3d1ba7/ash/app_list/test_app_list_presenter_impl.h
[delete] https://crrev.com/a48fd5adbbe586213bbca80cb10cc83aaa705e70/ash/app_list/test_app_list_view_presenter_impl.cc
[delete] https://crrev.com/a48fd5adbbe586213bbca80cb10cc83aaa705e70/ash/app_list/test_app_list_view_presenter_impl.h
[modify] https://crrev.com/b3a073579dffb23de8ebe1f181361b7f2a3d1ba7/ash/shelf/app_list_button.cc
[modify] https://crrev.com/b3a073579dffb23de8ebe1f181361b7f2a3d1ba7/ash/shelf/shelf_layout_manager.cc
[modify] https://crrev.com/b3a073579dffb23de8ebe1f181361b7f2a3d1ba7/ash/shelf/shelf_layout_manager_unittest.cc
[modify] https://crrev.com/b3a073579dffb23de8ebe1f181361b7f2a3d1ba7/ash/shelf/shelf_view_unittest.cc
[modify] https://crrev.com/b3a073579dffb23de8ebe1f181361b7f2a3d1ba7/ash/shell.cc
[modify] https://crrev.com/b3a073579dffb23de8ebe1f181361b7f2a3d1ba7/ash/wm/overview/window_selector_unittest.cc
[modify] https://crrev.com/b3a073579dffb23de8ebe1f181361b7f2a3d1ba7/ash/wm/window_cycle_controller_unittest.cc
[modify] https://crrev.com/b3a073579dffb23de8ebe1f181361b7f2a3d1ba7/ash/wm/workspace/workspace_layout_manager_unittest.cc
[modify] https://crrev.com/b3a073579dffb23de8ebe1f181361b7f2a3d1ba7/ui/app_list/presenter/app_list_presenter_impl.cc
[modify] https://crrev.com/b3a073579dffb23de8ebe1f181361b7f2a3d1ba7/ui/app_list/presenter/app_list_presenter_impl.h

Comment 8 by msw@chromium.org, Aug 22 2017

Cc: -msw@chromium.org mfomitchev@chromium.org
Owner: msw@chromium.org
Status: Fixed (was: Assigned)
My CL enabled the tests by tweaking/using TestAppList[View]PresenterImpl.
The tests could probably be refined, but lots of cleanup is gated on:
1) Issue 733662: the ui/app_list code should probably move to ash/*
2) (no issue?) Ash should present the app list, not Chrome...
Anyway, I guess this issue is fixed.

Comment 9 by dchan@chromium.org, Jan 22 2018

Status: Archived (was: Fixed)
►

Sign in to add a comment