mash: Remove DetachableBaseObserver dependency in SigninScreenHandler |
||||||||
Issue description
In SigninScreenHandler::SigninScreenHandler we currently have the code:
// TODO(tbarzic): This is needed for login UI - remove it when login switches
// to views implementation (or otherwise, make it work under mash).
if (features::IsAshInBrowserProcess())
detachable_base_observer_.Add(ash::Shell::Get()->detachable_base_handler());
We need to determine whether we can just remove the dependency (and possibly ash::DetachableBaseObserver and ash::DetachableBaseHandler as well?) now that we have switched to Views based login, or explore another mash solution.
,
Aug 7
Also: LoginDisplayHostCommon::LoginDisplayHostCommon disables Drag and Drop. That should also be unnecessary with Views based login?
,
Aug 7
Drag and Drop is actually already tracked in issue 854328
,
Aug 7
Removing sounds good to me
,
Aug 7
Yeah, we can remove dependency on datachable_base_handler from SigningScreenHander now that we don't use web ui login anymore. ash::DetachableBaseHandler and ash::DetachableBaseObserver are still needed - views based login implementation still uses this to display detachable base UI.
,
Aug 7
Great. If someone can do this cleanup in the 71 timeframe that would be much appreciated. If not, go ahead and assign this to me and I'll put up a CL for review. Thanks!
,
Aug 7
We'll have folks to help cleanup WebUI login code in 71 - once views based login launches successfully and we're certain that we won't need to revert.
,
Aug 13
,
Aug 14
,
Dec 3
Issue 678990 has been merged into this issue.
,
Dec 3
Reassigning to stevenjb@ since it's unlikely anyone working on login under rkc@ will get to it soon.
,
Dec 3
Sigh. I can take a look.
,
Dec 5
tbarzic@ - Is there any particular reason this was implemented in WebUI instead of as a system notification? It seems like that would be easier to maintain, and then we could move the observer and notification code to Ash. WDYT?
,
Dec 5
The mocks required the "warning" to be shown under the user pod on login/lock screen (so it's more noticeable than a system notification, which might be easily hidden or dismissed) - so it needed to be implemented for both views and webui based login screen. jdufault - do we still have to keep WebUI login screen around? If not, we can just safely remove this code from SigninScreenHandler. (Note that we do show a system notification if user attaches a keyboard inside an active session)
,
Dec 5
jdufault@ - Specifically, do we need to continue to maintain the login WebUI that shows this notification? If so, someone is going to need to add or modify a mojo API to handle this. This only needs to be fixed for MultiProcess Mash, which is a lot further out than SingleProcessMash, so I'm going to go ahead ad reassign this to tbarzic@ to fix at some point or resolve WontFix.
,
Dec 5
We don't need to maintain login webui for this - login webui is still used for OOBE but it will never display user pods/views.
,
Dec 5
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b0f265b71e6969a6762d82ac40cd2158f31920ff commit b0f265b71e6969a6762d82ac40cd2158f31920ff Author: Toni Barzic <tbarzic@chromium.org> Date: Wed Dec 05 18:21:58 2018 Remove dependency on ash::DetachableBaseHandler from chrome/ The dependency is required for displaying detachable base warnings in Web UI based login screen - given that this UI is not longer in use, the dependency (which breaks multiprocess mash) can now be removed. BUG= 871840 Change-Id: I12ac847526e308633aaab09d55aa20aa0edb6646 Reviewed-on: https://chromium-review.googlesource.com/c/1362485 Reviewed-by: Jacob Dufault <jdufault@chromium.org> Commit-Queue: Toni Baržić <tbarzic@chromium.org> Cr-Commit-Position: refs/heads/master@{#614029} [modify] https://crrev.com/b0f265b71e6969a6762d82ac40cd2158f31920ff/chrome/browser/ui/webui/chromeos/login/DEPS [modify] https://crrev.com/b0f265b71e6969a6762d82ac40cd2158f31920ff/chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc [modify] https://crrev.com/b0f265b71e6969a6762d82ac40cd2158f31920ff/chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h
,
Dec 5
|
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by steve...@chromium.org
, Aug 7