chrome/browser/chromeos/login/session/chrome_session_manager.cc - Issue 2734933004: ash: Use SessionController instead of SessionStateDelegate

Unified Diff: chrome/browser/chromeos/login/session/chrome_session_manager.cc

Issue 2734933004: ash: Use SessionController instead of SessionStateDelegate (Closed)

Patch Set: rebase Created 3 years, 9 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

« chrome/browser/chromeos/login/lock/screen_locker_browsertest.cc ('K') | « chrome/browser/chromeos/login/lock/screen_locker_browsertest.cc ('k') | chrome/browser/chromeos/login/session/user_session_manager.cc » ('j') | chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager_browsertest.cc » ('J')
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

Index: chrome/browser/chromeos/login/session/chrome_session_manager.cc

diff --git a/chrome/browser/chromeos/login/session/chrome_session_manager.cc b/chrome/browser/chromeos/login/session/chrome_session_manager.cc

index a08df7b0e095d043421daec63919c80346e5e1d7..815022d5b728e90ed1fa2e8b71fe0c80ff54d353 100644

--- a/chrome/browser/chromeos/login/session/chrome_session_manager.cc

+++ b/chrome/browser/chromeos/login/session/chrome_session_manager.cc

@@ -26,6 +26,7 @@

#include "chrome/browser/profiles/profile.h"

#include "chrome/browser/signin/signin_manager_factory.h"

#include "chrome/browser/ui/ash/ash_util.h"

+#include "chrome/browser/ui/ash/session_controller_client.h"

#include "chrome/common/chrome_switches.h"

#include "chrome/common/pref_names.h"

#include "chromeos/audio/cras_audio_handler.h"

@@ -134,6 +135,11 @@ void StartRestoreAfterCrashSession(Profile* user_profile,

// restore Sync.

UserSessionManager::GetInstance()->RestoreAuthenticationSession(

user_profile);

+ } else {

+ // Ensure that initial session state reaches ash during test. Otherwise,

+ // browser window would be created without focus because ash thinks the

+ // user session is blocked.

+ SessionControllerClient::FlushForTesting();

James Cook 2017/03/17 17:14:36 Hrm. Do we have to do this? I remember we added t

xiyuan 2017/03/17 22:52:03 It is not just one focus test. There are about 18

James Cook 2017/03/18 18:40:06 Could we do something like have ash look for the -

xiyuan 2017/03/19 08:48:33 Yep, sounds good.

Yep, sounds good.

Not expert, but think your concern is valid that this would make test code and production code runs very differently. FlushForTest would run nested loop until it hears back from the other side.

Ken Rockot(use gerrit already) 2017/03/20 15:59:26 Sounds like my answer doesn't matter anymore, but

On 2017/03/19 at 08:48:33, xiyuan wrote: > On 2017/03/18 18:40:06, James Cook wrote: > > On 2017/03/17 22:52:03, xiyuan wrote: > > > On 2017/03/17 17:14:36, James Cook wrote: > > > > Hrm. Do we have to do this? > > > > > > > > I remember we added this line before when I was working on asynchronous > > shelf > > > > creation. It seems like this makes the tests much different than production. > > > > > > > > Frankly I think it might be better to delete/disable that one focus test > > > instead > > > > of changing all tests to have a synchronous flush during startup. > > > > > > > > What do you think? > > > > > > It is not just one focus test. There are about 18 browser tests + 17 > > > chromevox_tests [1]. Esp. the failed chromevox ones, they have something like > > > this.awaitDocumentFocused(...) to run their test code. Without this, browser > > > window is not properly focused on creation. > > > > > > The problem is the race between browser window creation and session state in > > > SessionController. If SessionController does not have ACTIVE state, ash would > > > think the session is blocked and the focus rule will not activate any window. > > > > > > Another possible work around is to make SessionController default to ACTIVE > > > instead of UNKNOWN. This way, the browser window will get focus on creation. > > > WDYT? > > > > > > [1]: > > > > > https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium... > > > [2]: > > > > > https://cs.chromium.org/chromium/src/chrome/browser/resources/chromeos/chrome... > > > > Could we do something like have ash look for the --login-user command line flag > > and in that case default the session to ACTIVE? > > > > Yep, sounds good. > > > Also, I would be concerned about this if the *only* messages it flushes are the > > ones on the session controller interface. Then I would have more confidence > > about test coverage in other cases. But I don't know if that's how it works. > > rockot, in practice does binding.FlushForTest() end up flushing all IPCs? > > Not expert, but think your concern is valid that this would make test code and production code runs very differently. FlushForTest would run nested loop until it hears back from the other side.

Sounds like my answer doesn't matter anymore, but just in case: FlushForTesting simply pings a single interface and runs the message loop on the calling thread until it gets a ping response. So yes, other messages may or may not be dispatched on that interface or other interfaces in the interim. The intent of the API is not to allow tests to force synchronous behavior in certain situations, but only to allow tests to verify that e.g. a certain message or event has -not- been received as a result of some particular testing condition.

}