New issue
Advanced search Search tips

Issue 766759 link

Starred by 3 users
Status: Fixed
Owner:
jamescook@chromium.org
Closed: Sep 12
Cc:
x...@chromium.org
sammiequon@chromium.org
jamescook@chromium.org
est...@chromium.org
osh...@chromium.org
mustash-bugs@google.com
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug

Blocking:
issue 678705



Sign in to add a comment

mash: Fix chrome includes of ash/wm/tablet_mode/ headers

Project Member Reported by jamescook@chromium.org, Sep 19 2017 
Chrome needs to keep track of whether tablet mode is enabled/disabled instead of calling into ash each time it needs that data.

I think a good way to do this would be:
1. Introduce a class TabletModeClient in chrome
2. Have TabletModeController in ash sends changes in tablet mode state to TabletModeClient in chrome over mojo
3. Let chrome code add C++ observers to TabletModeClient directly, rather than using mojo observers
4. Get rid of ash::mojom::TabletModeObserver

This allows all the chrome observers to be synchronously notified once when tablet mode state changes, rather than having multiple mojo message sends.

Does this sound OK?

Comment 1 by jamescook@chromium.org, Sep 19 2017

Cc: e...@chromium.org est...@chromium.org
+erg and estade, as this will affect c/b/ui/ash/multi_user and c/b/ui/views/tabs

I'm going to take a quick look at how much work it would be to do this.

Comment 2 by jamescook@chromium.org, Sep 19 2017

Cc: x...@chromium.org
Owner: jamescook@chromium.org
Status: Started (was: Untriaged)
Chatted briefly with oshima/erg/xdai - I'll start this now.
Project Member

Comment 3 by bugdroid1@chromium.org, Sep 21 2017 

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

commit bf962f3bb0d17e01b36f76e03d75d1ae8644c906
Author: James Cook <jamescook@chromium.org>
Date: Thu Sep 21 19:48:44 2017

cros: Add TabletModeClient to hold tablet mode state in chrome

For go/mustash chrome browser code cannot call into ash. This CL
eliminates several places where chrome code calls into
ash::TabletModeController.

TabletModeClient also allows all chrome code to observe tablet mode
state changes simultaneously, rather than having each chrome class
process an async mojo observer message.

Convert SigninScreenHandler and TabDragController to the new approach.
Fix SigninScreen teardown in tests so we can ensure that the
TabletModeClient observer list is empty at shutdown.

Bug:  766759 
Test: browser_tests, unit_tests
Change-Id: I76edd44e83012722973d463073bfec3d395a8243
Reviewed-on: https://chromium-review.googlesource.com/675596
Commit-Queue: James Cook <jamescook@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#503537}
[modify] https://crrev.com/bf962f3bb0d17e01b36f76e03d75d1ae8644c906/ash/mojo_interface_factory.cc
[modify] https://crrev.com/bf962f3bb0d17e01b36f76e03d75d1ae8644c906/ash/mus/manifest.json
[modify] https://crrev.com/bf962f3bb0d17e01b36f76e03d75d1ae8644c906/ash/mus/standalone/manifest.json
[modify] https://crrev.com/bf962f3bb0d17e01b36f76e03d75d1ae8644c906/ash/public/interfaces/tablet_mode.mojom
[modify] https://crrev.com/bf962f3bb0d17e01b36f76e03d75d1ae8644c906/ash/wm/tablet_mode/tablet_mode_controller.cc
[modify] https://crrev.com/bf962f3bb0d17e01b36f76e03d75d1ae8644c906/ash/wm/tablet_mode/tablet_mode_controller.h
[modify] https://crrev.com/bf962f3bb0d17e01b36f76e03d75d1ae8644c906/ash/wm/tablet_mode/tablet_mode_observer.h
[modify] https://crrev.com/bf962f3bb0d17e01b36f76e03d75d1ae8644c906/chrome/browser/chromeos/chrome_browser_main_chromeos.cc
[modify] https://crrev.com/bf962f3bb0d17e01b36f76e03d75d1ae8644c906/chrome/browser/chromeos/chrome_browser_main_chromeos.h
[modify] https://crrev.com/bf962f3bb0d17e01b36f76e03d75d1ae8644c906/chrome/browser/chromeos/shutdown_policy_browsertest.cc
[modify] https://crrev.com/bf962f3bb0d17e01b36f76e03d75d1ae8644c906/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/bf962f3bb0d17e01b36f76e03d75d1ae8644c906/chrome/browser/ui/ash/launcher/launcher_context_menu.cc
[modify] https://crrev.com/bf962f3bb0d17e01b36f76e03d75d1ae8644c906/chrome/browser/ui/ash/session_controller_client.h
[add] https://crrev.com/bf962f3bb0d17e01b36f76e03d75d1ae8644c906/chrome/browser/ui/ash/tablet_mode_client.cc
[add] https://crrev.com/bf962f3bb0d17e01b36f76e03d75d1ae8644c906/chrome/browser/ui/ash/tablet_mode_client.h
[add] https://crrev.com/bf962f3bb0d17e01b36f76e03d75d1ae8644c906/chrome/browser/ui/ash/tablet_mode_client_observer.h
[add] https://crrev.com/bf962f3bb0d17e01b36f76e03d75d1ae8644c906/chrome/browser/ui/ash/tablet_mode_client_unittest.cc
[modify] https://crrev.com/bf962f3bb0d17e01b36f76e03d75d1ae8644c906/chrome/browser/ui/views/tabs/tab_drag_controller.cc
[modify] https://crrev.com/bf962f3bb0d17e01b36f76e03d75d1ae8644c906/chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc
[modify] https://crrev.com/bf962f3bb0d17e01b36f76e03d75d1ae8644c906/chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h
[modify] https://crrev.com/bf962f3bb0d17e01b36f76e03d75d1ae8644c906/chrome/test/BUILD.gn

Comment 4 by jamescook@chromium.org, Sep 21 2017

Cc: jamescook@chromium.org
Owner: ----
Status: Untriaged (was: Started)
This fixes many of them. The rest are in areas that have other significant dependencies on synchronous access to ash state. For example:

* Chrome window frames are calling directly into ash frame painting code
* Extension API for display info is calling directly into ash display management code
* ARC app launcher code calls into ash's ScreenOrientationController

These will need to be fixed on a per-feature basis to be sure we don't introduce races in classic ash between the existing sync code and the async tablet mode code.
Project Member

Comment 5 by bugdroid1@chromium.org, Oct 6 2017 

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

commit 5e241a0fe221dccc8fa60e3a1e017553eb062af1
Author: Elliot Glaysher <erg@chromium.org>
Date: Fri Oct 06 19:39:04 2017

Change BrowserNonClientFrameViewAsh to not use TabletModeObserver.

This switches BrowserNonClientFrameViewAsh to observe tablet mode
changes through the chrome side TabletModeClient and to fix tests so
that they deal with the asynchronicity now.

Bug:  766759 
Change-Id: Iee66e57afddfb2cf97a476328fec1cb2a375b916
Reviewed-on: https://chromium-review.googlesource.com/701641
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Commit-Queue: Elliot Glaysher <erg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#507147}
[modify] https://crrev.com/5e241a0fe221dccc8fa60e3a1e017553eb062af1/chrome/browser/ui/ash/tablet_mode_client.cc
[modify] https://crrev.com/5e241a0fe221dccc8fa60e3a1e017553eb062af1/chrome/browser/ui/ash/tablet_mode_client.h
[modify] https://crrev.com/5e241a0fe221dccc8fa60e3a1e017553eb062af1/chrome/browser/ui/views/apps/chrome_native_app_window_views_aura_ash.cc
[modify] https://crrev.com/5e241a0fe221dccc8fa60e3a1e017553eb062af1/chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc
[modify] https://crrev.com/5e241a0fe221dccc8fa60e3a1e017553eb062af1/chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.h
[modify] https://crrev.com/5e241a0fe221dccc8fa60e3a1e017553eb062af1/chrome/browser/ui/views/frame/browser_non_client_frame_view_ash_browsertest.cc
[add] https://crrev.com/5e241a0fe221dccc8fa60e3a1e017553eb062af1/chrome/browser/ui/views/frame/immersive_mode_controller_ash_browsertest.cc
[modify] https://crrev.com/5e241a0fe221dccc8fa60e3a1e017553eb062af1/chrome/browser/ui/views/frame/immersive_mode_controller_ash_unittest.cc
[modify] https://crrev.com/5e241a0fe221dccc8fa60e3a1e017553eb062af1/chrome/test/BUILD.gn
Project Member

Comment 6 by bugdroid1@chromium.org, Oct 9 2017 

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

commit 585e6c2fe4d7b9d8da5ea340847d3d18324f6e1d
Author: Elliot Glaysher <erg@chromium.org>
Date: Mon Oct 09 17:50:42 2017

Removes the dependency on tablet_mode_controller() from immersive mode.

ImmersiveModeControllerAsh no longer hard depends on ash's
tablet_mode_controller(), instead using chrome's TabletModeClient.

This moves another unit test to the browser tests due to the dependency
on a mojo component.

Bug:  766759 
Change-Id: I419d2ba63fd2d93ff74d63cd417df4e3c4551955
Reviewed-on: https://chromium-review.googlesource.com/706201
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Elliot Glaysher <erg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#507403}
[modify] https://crrev.com/585e6c2fe4d7b9d8da5ea340847d3d18324f6e1d/chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc
[modify] https://crrev.com/585e6c2fe4d7b9d8da5ea340847d3d18324f6e1d/chrome/browser/ui/views/frame/immersive_mode_controller_ash_browsertest.cc
[modify] https://crrev.com/585e6c2fe4d7b9d8da5ea340847d3d18324f6e1d/chrome/browser/ui/views/frame/immersive_mode_controller_ash_unittest.cc
Project Member

Comment 7 by bugdroid1@chromium.org, Oct 11 2017 

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

commit 5d31dfe72d7931f9d5b98dfdd3fd238294163001
Author: Elliot Glaysher <erg@chromium.org>
Date: Wed Oct 11 00:22:45 2017

Remove ash link in ArcAppWindowLauncherController.

This replaces direct usage of ash::Shell->tablet_mode_controller() with
access to the chrome side TabletModeClient, along with small fix ups of
unit tests.

Bug:  766759 
Change-Id: I1a165ba9b477b33354efc72af0ec28beb1da1485
Reviewed-on: https://chromium-review.googlesource.com/707761
Commit-Queue: Elliot Glaysher <erg@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#507837}
[modify] https://crrev.com/5d31dfe72d7931f9d5b98dfdd3fd238294163001/chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc
[modify] https://crrev.com/5d31dfe72d7931f9d5b98dfdd3fd238294163001/chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.h
[modify] https://crrev.com/5d31dfe72d7931f9d5b98dfdd3fd238294163001/chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc
[modify] https://crrev.com/5d31dfe72d7931f9d5b98dfdd3fd238294163001/chrome/browser/ui/ash/launcher/launcher_context_menu_unittest.cc
Project Member

Comment 8 by bugdroid1@chromium.org, Oct 17 2017 

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

commit 50bb2afdd2b5a0926dc1f66e01d5e1f7a8e2e922
Author: Elliot Glaysher <erg@chromium.org>
Date: Tue Oct 17 23:28:07 2017

Remove direct dependency on TabletModeController in DisplayInfoProvider.

Bug:  766759 
Change-Id: Idbe2e37cdef51476c6af9b56952daa2851d4d2b2
Reviewed-on: https://chromium-review.googlesource.com/723200
Reviewed-by: James Cook <jamescook@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Commit-Queue: Elliot Glaysher <erg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#509589}
[modify] https://crrev.com/50bb2afdd2b5a0926dc1f66e01d5e1f7a8e2e922/chrome/browser/extensions/display_info_provider_chromeos.cc
[modify] https://crrev.com/50bb2afdd2b5a0926dc1f66e01d5e1f7a8e2e922/chrome/browser/extensions/display_info_provider_chromeos_unittest.cc
Project Member

Comment 9 by bugdroid1@chromium.org, Oct 25 2017 

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

commit 247b7633aa0178350f2f5b76da58b536c5edb081
Author: Elliot Glaysher <erg@chromium.org>
Date: Wed Oct 25 03:02:09 2017

Adds a generic window management interface to mus.

This patch replaces the last direct usage in chrome code of
//ash/wm/tablet_mode_controller.cc by adding a generic interface to mus
for window manager specific actions.

Bug:  766759 
Change-Id: Ifd38c6b94d27e6cdcd7f130a44802e020c824610
Reviewed-on: https://chromium-review.googlesource.com/726971
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: Elliot Glaysher <erg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#511336}
[modify] https://crrev.com/247b7633aa0178350f2f5b76da58b536c5edb081/ash/mus/window_manager.cc
[modify] https://crrev.com/247b7633aa0178350f2f5b76da58b536c5edb081/ash/mus/window_manager.h
[modify] https://crrev.com/247b7633aa0178350f2f5b76da58b536c5edb081/ash/public/interfaces/BUILD.gn
[add] https://crrev.com/247b7633aa0178350f2f5b76da58b536c5edb081/ash/public/interfaces/window_actions.mojom
[modify] https://crrev.com/247b7633aa0178350f2f5b76da58b536c5edb081/chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.cc
[modify] https://crrev.com/247b7633aa0178350f2f5b76da58b536c5edb081/mash/simple_wm/simple_wm.cc
[modify] https://crrev.com/247b7633aa0178350f2f5b76da58b536c5edb081/mash/simple_wm/simple_wm.h
[modify] https://crrev.com/247b7633aa0178350f2f5b76da58b536c5edb081/services/ui/public/interfaces/window_manager.mojom
[modify] https://crrev.com/247b7633aa0178350f2f5b76da58b536c5edb081/services/ui/public/interfaces/window_tree.mojom
[modify] https://crrev.com/247b7633aa0178350f2f5b76da58b536c5edb081/services/ui/ws/access_policy.h
[modify] https://crrev.com/247b7633aa0178350f2f5b76da58b536c5edb081/services/ui/ws/default_access_policy.cc
[modify] https://crrev.com/247b7633aa0178350f2f5b76da58b536c5edb081/services/ui/ws/default_access_policy.h
[modify] https://crrev.com/247b7633aa0178350f2f5b76da58b536c5edb081/services/ui/ws/test_utils.cc
[modify] https://crrev.com/247b7633aa0178350f2f5b76da58b536c5edb081/services/ui/ws/test_utils.h
[modify] https://crrev.com/247b7633aa0178350f2f5b76da58b536c5edb081/services/ui/ws/window_manager_access_policy.cc
[modify] https://crrev.com/247b7633aa0178350f2f5b76da58b536c5edb081/services/ui/ws/window_manager_access_policy.h
[modify] https://crrev.com/247b7633aa0178350f2f5b76da58b536c5edb081/services/ui/ws/window_tree.cc
[modify] https://crrev.com/247b7633aa0178350f2f5b76da58b536c5edb081/services/ui/ws/window_tree.h
[modify] https://crrev.com/247b7633aa0178350f2f5b76da58b536c5edb081/services/ui/ws/window_tree_client_unittest.cc
[modify] https://crrev.com/247b7633aa0178350f2f5b76da58b536c5edb081/services/ui/ws/window_tree_unittest.cc
[modify] https://crrev.com/247b7633aa0178350f2f5b76da58b536c5edb081/ui/aura/mus/window_manager_delegate.cc
[modify] https://crrev.com/247b7633aa0178350f2f5b76da58b536c5edb081/ui/aura/mus/window_manager_delegate.h
[modify] https://crrev.com/247b7633aa0178350f2f5b76da58b536c5edb081/ui/aura/mus/window_tree_client.cc
[modify] https://crrev.com/247b7633aa0178350f2f5b76da58b536c5edb081/ui/aura/mus/window_tree_client.h
[modify] https://crrev.com/247b7633aa0178350f2f5b76da58b536c5edb081/ui/aura/mus/window_tree_host_mus.cc
[modify] https://crrev.com/247b7633aa0178350f2f5b76da58b536c5edb081/ui/aura/mus/window_tree_host_mus.h
[modify] https://crrev.com/247b7633aa0178350f2f5b76da58b536c5edb081/ui/aura/mus/window_tree_host_mus_delegate.h
[modify] https://crrev.com/247b7633aa0178350f2f5b76da58b536c5edb081/ui/aura/mus/window_tree_host_mus_unittest.cc
[modify] https://crrev.com/247b7633aa0178350f2f5b76da58b536c5edb081/ui/aura/test/mus/test_window_tree.cc
[modify] https://crrev.com/247b7633aa0178350f2f5b76da58b536c5edb081/ui/aura/test/mus/test_window_tree.h
Project Member

Comment 10 by bugdroid1@chromium.org, Nov 27 2017 

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

commit 18997342523ef8d729d4c257d4e2c455e8337d1c
Author: Elliot Glaysher <erg@chromium.org>
Date: Mon Nov 27 23:40:19 2017

Remove outdated mash checks.

ChromeNativeAppWindowViewsAuraAsh used to directly depend on the
TabletModeController instead of the mojom client. This usage needed to
be protected under mash. Now that we use TabletModeClient, remove this
check so we update more state correctly.

Bug:  766759 
Change-Id: Ib4ace7c6bc9f1a39cbf08db7a340c58a48dca6df
Reviewed-on: https://chromium-review.googlesource.com/764867
Reviewed-by: Michael Wasserman <msw@chromium.org>
Commit-Queue: Elliot Glaysher <erg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#519451}
[modify] https://crrev.com/18997342523ef8d729d4c257d4e2c455e8337d1c/chrome/browser/ui/views/apps/chrome_native_app_window_views_aura_ash.cc

Comment 11 by lafo...@chromium.org, Feb 26 2018

Components: -Internals>MUS Internals>Services>WindowService

Comment 12 by e...@chromium.org, Mar 9 2018

Cc: -e...@chromium.org
Un-cc-ing me from all bugs on my final day.

Comment 13 by jamescook@chromium.org, Apr 19 2018

Components: -Internals>Services>WindowService Internals>Services>Ash
Labels: -Proj-Mustash-Mash

Comment 14 by sky@chromium.org, Sep 12

Owner: jamescook@chromium.org
Status: Assigned (was: Untriaged)
James, I think this is done, do I have that right? AFAICT the only includes of this now that aren't mash-ok are in tests.

Comment 15 by jamescook@chromium.org, Sep 12

Labels: Proj-Mash-MultiProcess
Status: Fixed (was: Assigned)
Yes, I think this is done.
►

Sign in to add a comment