Minimized windows should not be activated by another user in multiprofile scenario.

Fix the following two cases in multiprofile scenario:

1) Window A belongs to user1 and is shown for user1. Minimizes window A.
Switch to user2, window A can not be activated by user2.

2) Window A belongs to user1 and is shown for user2. Then windowA can be activated
by user2 but cannot be activated by user1.

Also add unit tests for these two cases.

BUG=471858
TEST=manually tested

Committed: https://crrev.com/13beb501975bbc13f07cd8a1915a8a51310cecec
Cr-Commit-Position: refs/heads/master@{#325259}

[xdai1, 2015-04-06 23:15:18]

Patchset #1 (id:1) has been deleted

[xdai1, 2015-04-06 23:25:09]

xdai@chromium.org changed reviewers:
+ oshima@chromium.org

[xdai1, 2015-04-06 23:25:10]

Oshima, could you help to review the CL please? Thanks!

[oshima, 2015-04-07 16:55:36]

The CQ bit was checked by oshima@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-04-07 16:55:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1053013007/20001

[commit-bot: I haz the power, 2015-04-07 17:05:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-07 17:06:00]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[oshima, 2015-04-07 19:53:45]

https://codereview.chromium.org/1053013007/diff/20001/ash/session/session_sta...
File ash/session/session_state_delegate.h (right):

https://codereview.chromium.org/1053013007/diff/20001/ash/session/session_sta...
ash/session/session_state_delegate.h:86: virtual const std::string&
GetUserPresentingWindow(aura::Window* window) = 0;
Is it hard to return BrowserContext instead of std::string?

https://codereview.chromium.org/1053013007/diff/20001/ash/wm/ash_focus_rules.cc
File ash/wm/ash_focus_rules.cc (right):

https://codereview.chromium.org/1053013007/diff/20001/ash/wm/ash_focus_rules....
ash/wm/ash_focus_rules.cc:98: user_manager ? user_manager->GetActiveUser() :
NULL;
Can you use GetActiveBrowserContext and  GetUserInfo instead?

https://codereview.chromium.org/1053013007/diff/20001/chrome/browser/ui/ash/m...
File
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/1053013007/diff/20001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc:47:
~TestSessionStateDelegateChromeOS() override {}
you no longer need empty dtor.

https://codereview.chromium.org/1053013007/diff/20001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/session_state_delegate_chromeos.cc (left):

https://codereview.chromium.org/1053013007/diff/20001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:70: return
chromeos::ProfileHelper::Get()->GetProfileByUserUnsafe(user);
why this change?

[xdai1, 2015-04-09 16:39:48]

Patchset #2 (id:40001) has been deleted

[xdai1, 2015-04-09 17:22:40]

Oshima, please help to take another look, thanks!

https://codereview.chromium.org/1053013007/diff/20001/ash/session/session_sta...
File ash/session/session_state_delegate.h (right):

https://codereview.chromium.org/1053013007/diff/20001/ash/session/session_sta...
ash/session/session_state_delegate.h:86: virtual const std::string&
GetUserPresentingWindow(aura::Window* window) = 0;
On 2015/04/07 19:53:44, oshima wrote:
> Is it hard to return BrowserContext instead of std::string?

Done.

https://codereview.chromium.org/1053013007/diff/20001/ash/wm/ash_focus_rules.cc
File ash/wm/ash_focus_rules.cc (right):

https://codereview.chromium.org/1053013007/diff/20001/ash/wm/ash_focus_rules....
ash/wm/ash_focus_rules.cc:98: user_manager ? user_manager->GetActiveUser() :
NULL;
On 2015/04/07 19:53:44, oshima wrote:
> Can you use GetActiveBrowserContext and  GetUserInfo instead?

Done.

https://codereview.chromium.org/1053013007/diff/20001/chrome/browser/ui/ash/m...
File
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/1053013007/diff/20001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc:47:
~TestSessionStateDelegateChromeOS() override {}
On 2015/04/07 19:53:44, oshima wrote:
> you no longer need empty dtor.

Done.

https://codereview.chromium.org/1053013007/diff/20001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/session_state_delegate_chromeos.cc (left):

https://codereview.chromium.org/1053013007/diff/20001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:70: return
chromeos::ProfileHelper::Get()->GetProfileByUserUnsafe(user);
On 2015/04/07 19:53:44, oshima wrote:
> why this change?

In Login screen, we might fail this check since there is no active user at that
moment.

[oshima, 2015-04-09 17:50:48]

https://codereview.chromium.org/1053013007/diff/20001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/session_state_delegate_chromeos.cc (left):

https://codereview.chromium.org/1053013007/diff/20001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:70: return
chromeos::ProfileHelper::Get()->GetProfileByUserUnsafe(user);
On 2015/04/09 17:22:40, xdai1 wrote:
> On 2015/04/07 19:53:44, oshima wrote:
> > why this change?
> 
> In Login screen, we might fail this check since there is no active user at
that
> moment.

Why this wasn't an issue before? Do you think this is because of
crbug.com/475318?

https://codereview.chromium.org/1053013007/diff/60001/ash/test/test_session_s...
File ash/test/test_session_state_delegate.cc (right):

https://codereview.chromium.org/1053013007/diff/60001/ash/test/test_session_s...
ash/test/test_session_state_delegate.cc:105: }
please keep the same order as in the header.

https://codereview.chromium.org/1053013007/diff/60001/ash/wm/ash_focus_rules.cc
File ash/wm/ash_focus_rules.cc (right):

https://codereview.chromium.org/1053013007/diff/60001/ash/wm/ash_focus_rules....
ash/wm/ash_focus_rules.cc:104: return false;
you need {} in this case.

[xdai1, 2015-04-09 22:45:25]

Oshima, please take another look at this CL, thanks!

https://codereview.chromium.org/1053013007/diff/20001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/session_state_delegate_chromeos.cc (left):

https://codereview.chromium.org/1053013007/diff/20001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:70: return
chromeos::ProfileHelper::Get()->GetProfileByUserUnsafe(user);
On 2015/04/09 17:50:48, oshima wrote:
> On 2015/04/09 17:22:40, xdai1 wrote:
> > On 2015/04/07 19:53:44, oshima wrote:
> > > why this change?
> > 
> > In Login screen, we might fail this check since there is no active user at
> that
> > moment.
> 
> Why this wasn't an issue before? Do you think this is because of
> crbug.com/475318?

Because in this fix we will call this GetBrowserContextForWindow() function in
AshFocusRules::IsWindowConsideredVisibleForActivation() function, which will be
used to check if the window can be activatable even in the login screen. But
without this fix, we don't need to worry about this since we always call
GetBrowserContextForWindow() after we have an active user.

It has nothing to do with crbug.com/475318.

https://codereview.chromium.org/1053013007/diff/60001/ash/test/test_session_s...
File ash/test/test_session_state_delegate.cc (right):

https://codereview.chromium.org/1053013007/diff/60001/ash/test/test_session_s...
ash/test/test_session_state_delegate.cc:105: }
On 2015/04/09 17:50:48, oshima wrote:
> please keep the same order as in the header.

It was in the same order:)

https://codereview.chromium.org/1053013007/diff/60001/ash/wm/ash_focus_rules.cc
File ash/wm/ash_focus_rules.cc (right):

https://codereview.chromium.org/1053013007/diff/60001/ash/wm/ash_focus_rules....
ash/wm/ash_focus_rules.cc:104: return false;
On 2015/04/09 17:50:48, oshima wrote:
> you need {} in this case. 

Done.

[oshima, 2015-04-10 20:39:49]

https://codereview.chromium.org/1053013007/diff/20001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/session_state_delegate_chromeos.cc (left):

https://codereview.chromium.org/1053013007/diff/20001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:70: return
chromeos::ProfileHelper::Get()->GetProfileByUserUnsafe(user);
On 2015/04/09 22:45:25, xdai1 wrote:
> On 2015/04/09 17:50:48, oshima wrote:
> > On 2015/04/09 17:22:40, xdai1 wrote:
> > > On 2015/04/07 19:53:44, oshima wrote:
> > > > why this change?
> > > 
> > > In Login screen, we might fail this check since there is no active user at
> > that
> > > moment.
> > 
> > Why this wasn't an issue before? Do you think this is because of
> > crbug.com/475318?
> 
> Because in this fix we will call this GetBrowserContextForWindow() function in
> AshFocusRules::IsWindowConsideredVisibleForActivation() function, which will
be
> used to check if the window can be activatable even in the login screen. But
> without this fix, we don't need to worry about this since we always call
> GetBrowserContextForWindow() after we have an active user.
> 
> It has nothing to do with crbug.com/475318. 

what kind of "user_id" would you get if it's in login screen?

https://codereview.chromium.org/1053013007/diff/60001/ash/test/test_session_s...
File ash/test/test_session_state_delegate.cc (right):

https://codereview.chromium.org/1053013007/diff/60001/ash/test/test_session_s...
ash/test/test_session_state_delegate.cc:105: }
On 2015/04/09 22:45:25, xdai1 wrote:
> On 2015/04/09 17:50:48, oshima wrote:
> > please keep the same order as in the header.
> 
> It was in the same order:)

oops sorry.

[xdai1, 2015-04-10 22:56:08]

Oshima, please take another look at the CL, thanks!

https://codereview.chromium.org/1053013007/diff/100001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/session_state_delegate_chromeos.cc (right):

https://codereview.chromium.org/1053013007/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:69: :
multi_user_util::GetProfileFromUserID(user_id);
I think it might be more cleaner to get profile this way? The user_id will be
empty string if the window is special (for example, the system tray bubble
window, login window, etc)

[oshima, 2015-04-10 23:44:36]

https://codereview.chromium.org/1053013007/diff/100001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/session_state_delegate_chromeos.cc (right):

https://codereview.chromium.org/1053013007/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:69: :
multi_user_util::GetProfileFromUserID(user_id);
On 2015/04/10 22:56:07, xdai1 wrote:
> I think it might be more cleaner to get profile this way? The user_id will be
> empty string if the window is special (for example, the system tray bubble
> window, login window, etc)

lgtm

[xdai1, 2015-04-13 23:16:15]

Patchset #5 (id:120001) has been deleted

[xdai1, 2015-04-14 18:11:23]

xdai@chromium.org changed reviewers:
+ skuhne@chromium.org

[xdai1, 2015-04-14 18:11:23]

Stefan, could you help to review these files please? Thanks!

chrome/browser/ui/ash/chrome_shell_delegate.h
chrome/browser/ui/ash/chrome_shell_delegate.cc
chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc

[Mr4D (OOO till 08-26), 2015-04-14 20:06:56]

A few nits. But nearly there!

https://codereview.chromium.org/1053013007/diff/160001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc (right):

https://codereview.chromium.org/1053013007/diff/160001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc:45: void
InitAfterSessionStart(bool is_first_run) {
You can remove this.

https://codereview.chromium.org/1053013007/diff/160001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc:268: case
chrome::NOTIFICATION_SESSION_STARTED:
Do not call this function in the first place when the variable is false.

You might want to rename it to "InitAfterFirstSessionStart" to make the fact
clearer that this is only needed to do once per system start.

Additionally you might want to add a comment like:

This should only be call once upon system start.

https://codereview.chromium.org/1053013007/diff/160001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc:269:
InitAfterSessionStart(is_first_run_);
You don't even need is_first_run_ by using:

      if (user_manager::UserManager::Get()->GetLoggedInUsers().size() < 2) {
        InitAfterFirstSessionStart();
      }

[xdai1, 2015-04-14 20:40:36]

xdai@chromium.org changed reviewers:
+ benwells@chromium.org

[xdai1, 2015-04-14 20:40:37]

Thanks for the review Stefan! Please take another look, thanks! 

benwells@, could you take a look at chrome/browser/apps/app_browsertest.cc file?
Thanks!

[Mr4D (OOO till 08-26), 2015-04-14 20:42:20]

LGTM!

[benwells, 2015-04-15 01:16:00]

lgtm

[xdai1, 2015-04-15 16:24:15]

The CQ bit was checked by xdai@chromium.org

[xdai1, 2015-04-15 16:24:15]

The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/1053013007/#ps180001
(title: "Address skuhne@'s comments.")

[commit-bot: I haz the power, 2015-04-15 16:24:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1053013007/180001

[commit-bot: I haz the power, 2015-04-15 16:28:56]

Committed patchset #7 (id:180001)

[commit-bot: I haz the power, 2015-04-15 16:29:34]

Patchset 7 (id:??) landed as
https://crrev.com/13beb501975bbc13f07cd8a1915a8a51310cecec
Cr-Commit-Position: refs/heads/master@{#325259}