New issue
Advanced search Search tips

Issue 665064 link

Starred by 2 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug


Sign in to add a comment

mash: eliminate ChromeShellDelegate

Project Member Reported by jamescook@chromium.org, Nov 14 2016

Issue description

It's a mixed bag of functionality. It either needs a mustash version, or to be split into other delegates.

 
Blockedon: 651157
Labels: -Pri-3 Pri-2
Owner: jamescook@chromium.org
Status: Started (was: Untriaged)
I'll take a look at these.

Project Member

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

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

commit 459354b3e4963ea8e06594b9f08f5c62881a54b3
Author: James Cook <jamescook@chromium.org>
Date: Wed Sep 27 23:18:01 2017

cros: Eliminate ash::ShellDelegate::IsMultiProfilesEnabled

For go/mustash we need to eliminate in-process ash to chrome delegates.

Eliminate the concept from ash, in favor of checking whether multiple
users are signed in or whether more users can be added to the session.

Rename the chrome method to make it clearer that it's not just a
feature that can be enabled or disabled.

Eliminate dead code to support initializing the system tray menu for
ephemeral users. See  issue 312324  for details.

Change InactiveUserNotificationBlocker to only block notifications
when multiple users are in the session. This is mostly needed to
prevent notifications from being blocked in AshTestBase tests but
also more closely matches what we want in production.

Bug:  312324 , 665064
Test: ash_unittests, unit_tests, manually check system tray menu with ephemeral users and with multi-profile, manually test notification blocking for inactive users with http://jsfiddle.net/qjme9dmd/

Change-Id: Ib1c0bcd5da3f9e9c32641b29e95385e696cc415a
Reviewed-on: https://chromium-review.googlesource.com/685823
Commit-Queue: James Cook <jamescook@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504795}
[modify] https://crrev.com/459354b3e4963ea8e06594b9f08f5c62881a54b3/ash/accelerators/accelerator_controller.cc
[modify] https://crrev.com/459354b3e4963ea8e06594b9f08f5c62881a54b3/ash/message_center/message_center_controller.h
[modify] https://crrev.com/459354b3e4963ea8e06594b9f08f5c62881a54b3/ash/mus/shell_delegate_mus.cc
[modify] https://crrev.com/459354b3e4963ea8e06594b9f08f5c62881a54b3/ash/mus/shell_delegate_mus.h
[modify] https://crrev.com/459354b3e4963ea8e06594b9f08f5c62881a54b3/ash/session/test_session_controller_client.cc
[modify] https://crrev.com/459354b3e4963ea8e06594b9f08f5c62881a54b3/ash/session/test_session_controller_client.h
[modify] https://crrev.com/459354b3e4963ea8e06594b9f08f5c62881a54b3/ash/shell/shell_delegate_impl.cc
[modify] https://crrev.com/459354b3e4963ea8e06594b9f08f5c62881a54b3/ash/shell/shell_delegate_impl.h
[modify] https://crrev.com/459354b3e4963ea8e06594b9f08f5c62881a54b3/ash/shell_delegate.h
[modify] https://crrev.com/459354b3e4963ea8e06594b9f08f5c62881a54b3/ash/shell_test_api.cc
[modify] https://crrev.com/459354b3e4963ea8e06594b9f08f5c62881a54b3/ash/shell_test_api.h
[modify] https://crrev.com/459354b3e4963ea8e06594b9f08f5c62881a54b3/ash/system/user/tray_user_unittest.cc
[modify] https://crrev.com/459354b3e4963ea8e06594b9f08f5c62881a54b3/ash/system/user/user_view.cc
[modify] https://crrev.com/459354b3e4963ea8e06594b9f08f5c62881a54b3/ash/system/web_notification/inactive_user_notification_blocker.cc
[modify] https://crrev.com/459354b3e4963ea8e06594b9f08f5c62881a54b3/ash/system/web_notification/inactive_user_notification_blocker_unittest.cc
[modify] https://crrev.com/459354b3e4963ea8e06594b9f08f5c62881a54b3/ash/test_shell_delegate.cc
[modify] https://crrev.com/459354b3e4963ea8e06594b9f08f5c62881a54b3/ash/test_shell_delegate.h
[modify] https://crrev.com/459354b3e4963ea8e06594b9f08f5c62881a54b3/chrome/browser/ui/ash/chrome_shell_delegate.cc
[modify] https://crrev.com/459354b3e4963ea8e06594b9f08f5c62881a54b3/chrome/browser/ui/ash/chrome_shell_delegate.h
[modify] https://crrev.com/459354b3e4963ea8e06594b9f08f5c62881a54b3/chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc
[modify] https://crrev.com/459354b3e4963ea8e06594b9f08f5c62881a54b3/chrome/browser/ui/ash/multi_user/multi_user_window_manager.cc
[modify] https://crrev.com/459354b3e4963ea8e06594b9f08f5c62881a54b3/chrome/browser/ui/ash/session_controller_client.cc
[modify] https://crrev.com/459354b3e4963ea8e06594b9f08f5c62881a54b3/chrome/browser/ui/ash/session_controller_client.h

Project Member

Comment 4 by bugdroid1@chromium.org, Sep 28 2017

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

commit 0616306add219e27537ed7176b36c0766370c7fd
Author: James Cook <jamescook@chromium.org>
Date: Thu Sep 28 23:29:19 2017

cros: Eliminate ash::ShellDelegate::IsIncognitoAllowed for mash

For go/mustash we have to remove in-process ash to chrome calls.

This was used to decide if ash should handle the Ctrl-Shift-N shortcut,
which might be disabled in guest sessions or via enterprise policy.
Ash now always consumes Ctrl-Shift-N (except in guest sessions) and
defers the decision about whether to open a window to the browser code.

Bug: 665064
Test: added to ash_unittests, browser_tests, manual test of Ctrl-Shift-N in guest sessions on device

Change-Id: If1ec062402eafc7785e483c7eee3809282a9767e
Reviewed-on: https://chromium-review.googlesource.com/690623
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#505210}
[modify] https://crrev.com/0616306add219e27537ed7176b36c0766370c7fd/ash/accelerators/accelerator_controller.cc
[modify] https://crrev.com/0616306add219e27537ed7176b36c0766370c7fd/ash/accelerators/accelerator_controller_unittest.cc
[modify] https://crrev.com/0616306add219e27537ed7176b36c0766370c7fd/ash/mus/shell_delegate_mus.cc
[modify] https://crrev.com/0616306add219e27537ed7176b36c0766370c7fd/ash/mus/shell_delegate_mus.h
[modify] https://crrev.com/0616306add219e27537ed7176b36c0766370c7fd/ash/shell/shell_delegate_impl.cc
[modify] https://crrev.com/0616306add219e27537ed7176b36c0766370c7fd/ash/shell/shell_delegate_impl.h
[modify] https://crrev.com/0616306add219e27537ed7176b36c0766370c7fd/ash/shell_delegate.h
[modify] https://crrev.com/0616306add219e27537ed7176b36c0766370c7fd/ash/test_shell_delegate.cc
[modify] https://crrev.com/0616306add219e27537ed7176b36c0766370c7fd/ash/test_shell_delegate.h
[modify] https://crrev.com/0616306add219e27537ed7176b36c0766370c7fd/chrome/browser/chromeos/accessibility/accessibility_manager.cc
[modify] https://crrev.com/0616306add219e27537ed7176b36c0766370c7fd/chrome/browser/chromeos/accessibility/accessibility_manager.h
[modify] https://crrev.com/0616306add219e27537ed7176b36c0766370c7fd/chrome/browser/ui/ash/chrome_new_window_client.cc
[modify] https://crrev.com/0616306add219e27537ed7176b36c0766370c7fd/chrome/browser/ui/ash/chrome_new_window_client.h
[modify] https://crrev.com/0616306add219e27537ed7176b36c0766370c7fd/chrome/browser/ui/ash/chrome_new_window_client_browsertest.cc
[modify] https://crrev.com/0616306add219e27537ed7176b36c0766370c7fd/chrome/browser/ui/ash/chrome_shell_delegate.cc
[modify] https://crrev.com/0616306add219e27537ed7176b36c0766370c7fd/chrome/browser/ui/ash/chrome_shell_delegate.h

Status: Available (was: Started)
Components: -Internals>MUS Internals>Services>WindowService
Blockedon: 678949
Cc: steve...@chromium.org
I am currently moving display_prefs_ and display_configuration_observer_ to ash::Shell ( issue 678949 ).


Status: Assigned (was: Available)
Cc: jamescook@chromium.org
Owner: steve...@chromium.org
Status: Started (was: Assigned)
Oops, meant to assign this to myself, at least while I am actively working on removing parts of this.

Components: -Internals>Services>WindowService Internals>Services>Ash
Labels: -Proj-Mustash-Mash
Labels: M-68
Two cleanup CLs are in review:
https://chromium-review.googlesource.com/c/chromium/src/+/1070771
https://chromium-review.googlesource.com/c/chromium/src/+/1070854

TODO:
Move remaining code in ChromeShellDelegate::Observe to ChromeBrowserMainExtraPartsAsh so that the logic is implemented for Mash.

With those changes, the following delegate implementations will remain:
* CreateAccessibilityDelegate / ash::AccessibilityDelegate - issue 756054
* CanShowWindowForUser - Calls into ShellContentState - issue 672277
* policy::DisplayRotationDefaultHandler() -  issue 705713 
* OpenUrlFromArc, OpenKeyboardShortcutHelpPage -  issue 846128 
* CreateKeyboardUI -  issue 843332 
* GetNetworkingConfigDelegate -  issue 651157 
* CreateScreenshotDelegate - issue 557397


Project Member

Comment 13 by bugdroid1@chromium.org, May 24 2018

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

commit badc309cd5ccc92d4c4a9674eea6b97b69f07f3d
Author: Steven Bennetts <stevenjb@chromium.org>
Date: Thu May 24 21:26:01 2018

ChromeShellDelegate: Cleanup: move/remove some methods

This CL:
* Removes IsRunningInForcedAppMode from ChromeShellDelegate
  (ash.mojom.SessionInfo.is_running_in_app_mode is used instead).
* Moves enabling of MagnifierKeyScroller and SpokenFeedbackToggler
  to ash::Shell.
* Eliminates unused PreShutdown method.
* Replaces call back into ash::Shell (ugh!) to show settings with
  direct call.

Bug: 665064

Change-Id: I31e68366be2f6efab3845be6d00ebc3e967c99c7
Reviewed-on: https://chromium-review.googlesource.com/1070771
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561635}
[modify] https://crrev.com/badc309cd5ccc92d4c4a9674eea6b97b69f07f3d/ash/accelerators/accelerator_controller.cc
[modify] https://crrev.com/badc309cd5ccc92d4c4a9674eea6b97b69f07f3d/ash/session/session_controller.cc
[modify] https://crrev.com/badc309cd5ccc92d4c4a9674eea6b97b69f07f3d/ash/session/session_observer.h
[modify] https://crrev.com/badc309cd5ccc92d4c4a9674eea6b97b69f07f3d/ash/shell.cc
[modify] https://crrev.com/badc309cd5ccc92d4c4a9674eea6b97b69f07f3d/ash/shell.h
[modify] https://crrev.com/badc309cd5ccc92d4c4a9674eea6b97b69f07f3d/ash/shell/shell_delegate_impl.cc
[modify] https://crrev.com/badc309cd5ccc92d4c4a9674eea6b97b69f07f3d/ash/shell/shell_delegate_impl.h
[modify] https://crrev.com/badc309cd5ccc92d4c4a9674eea6b97b69f07f3d/ash/shell_delegate.h
[modify] https://crrev.com/badc309cd5ccc92d4c4a9674eea6b97b69f07f3d/ash/shell_delegate_mus.cc
[modify] https://crrev.com/badc309cd5ccc92d4c4a9674eea6b97b69f07f3d/ash/shell_delegate_mus.h
[modify] https://crrev.com/badc309cd5ccc92d4c4a9674eea6b97b69f07f3d/ash/test_shell_delegate.cc
[modify] https://crrev.com/badc309cd5ccc92d4c4a9674eea6b97b69f07f3d/ash/test_shell_delegate.h
[modify] https://crrev.com/badc309cd5ccc92d4c4a9674eea6b97b69f07f3d/chrome/browser/app_mode/app_mode_utils.h
[modify] https://crrev.com/badc309cd5ccc92d4c4a9674eea6b97b69f07f3d/chrome/browser/chromeos/accessibility/DEPS
[modify] https://crrev.com/badc309cd5ccc92d4c4a9674eea6b97b69f07f3d/chrome/browser/ui/ash/chrome_shell_delegate.cc
[modify] https://crrev.com/badc309cd5ccc92d4c4a9674eea6b97b69f07f3d/chrome/browser/ui/ash/chrome_shell_delegate.h

Project Member

Comment 14 by bugdroid1@chromium.org, May 25 2018

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

commit 7fa507bfbc1a21784e3e10d5e044fbb0e7061296
Author: Steven Bennetts <stevenjb@chromium.org>
Date: Fri May 25 15:31:55 2018

ChromeShellDelegate: Cleanup: move initial focus code to ash

This change makes mru_window_tracker.cc a SessionObserver and
moves the initial focus logic there.

Bug: 665064
Change-Id: I5c125e0bbe62b9d2c8d809d519d3ccfeade76f8f
Reviewed-on: https://chromium-review.googlesource.com/1070854
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561886}
[modify] https://crrev.com/7fa507bfbc1a21784e3e10d5e044fbb0e7061296/ash/wm/mru_window_tracker.cc
[modify] https://crrev.com/7fa507bfbc1a21784e3e10d5e044fbb0e7061296/ash/wm/mru_window_tracker.h
[modify] https://crrev.com/7fa507bfbc1a21784e3e10d5e044fbb0e7061296/chrome/browser/ui/ash/DEPS
[modify] https://crrev.com/7fa507bfbc1a21784e3e10d5e044fbb0e7061296/chrome/browser/ui/ash/chrome_shell_delegate.cc

Project Member

Comment 15 by bugdroid1@chromium.org, May 25 2018

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

commit 7c3a331ed75e2a7db08dbbcfc105467c6d030759
Author: Steven Bennetts <stevenjb@chromium.org>
Date: Fri May 25 16:24:37 2018

Move NotificationObserver code out of ChromeShellDelegate

There is no particular reason for this code to be part of the delegate.
This allows the code to run in Mash and Mus modes.

Bug: 665064
Change-Id: I0c9a7b1872f82b3487aa51412294b5baeea33b9b
Reviewed-on: https://chromium-review.googlesource.com/1071873
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561903}
[modify] https://crrev.com/7c3a331ed75e2a7db08dbbcfc105467c6d030759/chrome/browser/ui/ash/chrome_browser_main_extra_parts_ash.cc
[modify] https://crrev.com/7c3a331ed75e2a7db08dbbcfc105467c6d030759/chrome/browser/ui/ash/chrome_browser_main_extra_parts_ash.h
[modify] https://crrev.com/7c3a331ed75e2a7db08dbbcfc105467c6d030759/chrome/browser/ui/ash/chrome_shell_delegate.cc
[modify] https://crrev.com/7c3a331ed75e2a7db08dbbcfc105467c6d030759/chrome/browser/ui/ash/chrome_shell_delegate.h

Blockedon: 705713 672277 756054 843332 557397 846128
Status: Assigned (was: Started)
Setting 'Blocked on' for remaining TODO items:

* CreateAccessibilityDelegate / ash::AccessibilityDelegate - issue 756054
* CanShowWindowForUser - Calls into ShellContentState - issue 672277
* policy::DisplayRotationDefaultHandler() -  issue 705713 
* OpenUrlFromArc, OpenKeyboardShortcutHelpPage -  issue 846128 
* CreateKeyboardUI -  issue 843332 
* GetNetworkingConfigDelegate -  issue 651157 
* CreateScreenshotDelegate - issue 557397

Labels: -M-68
Labels: Proj-Mustash
Labels: -Proj-Mustash Proj-Mash-MultiProcess
Summary: mash: eliminate ChromeShellDelegate (was: mash: ShellDelegate implementation for mus)
Renaming this because a) we've deprecated 'mus', and b) this kind of already got repurposed for eliminating the need for ChromeShellDelegate.

Sign in to add a comment