Supports window teleports for notifications.

Supports window teleports for notifications.

MultiUserNotificationBlockerChromeOS now supports visiting windows.
Due to the dependency from MultiUserWindowManagerChromeOS, it would
be beter to be in c/b/ui/ash/multi_user rather than notification.

Because now this blocker is turned on only when multi-user feature
is enabled, the test for single-user mode has been removed.

BUG=321679
R=dewittj@chromium.org, skuhne@chromium.org, sky@chromium.org
TBR=bshe@chromium.org
TEST=unit_tests / manually

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=244058

[Mr4D (OOO till 08-26), 2014-01-08 17:29:35]

Please see my comments!

Let me know when you are in so that we can quickly discuss how I can help!

https://codereview.chromium.org/127423002/diff/40001/chrome/browser/ui/ash/mu...
File
chrome/browser/ui/ash/multi_user/multi_user_notification_blocker_chromeos.cc
(right):

https://codereview.chromium.org/127423002/diff/40001/chrome/browser/ui/ash/mu...
chrome/browser/ui/ash/multi_user/multi_user_notification_blocker_chromeos.cc:29:
// Browser/apps windows exist only on the default container.
Not quite sure if it is worth while the effort, but you might want to check
multi_user_manager->AreWindowsSharedAmongUsers() to see if there are any windows
shared before doing this, since visiting windows might not be used too often.

https://codereview.chromium.org/127423002/diff/40001/chrome/browser/ui/ash/mu...
chrome/browser/ui/ash/multi_user/multi_user_notification_blocker_chromeos.cc:33:
std::set<std::string> new_ids;
Looking at this I would guess that this should be done in the
MultiUserWindowManager itslef since there are all registered windows already in
a list and collecting these together should be much faster. Please create an
issue and I will do that today.

https://codereview.chromium.org/127423002/diff/40001/chrome/browser/ui/ash/mu...
chrome/browser/ui/ash/multi_user/multi_user_notification_blocker_chromeos.cc:37:
aura::Window* child = children[i];
IsWindowOnDesktopOfUser(<unowned_window>) will return true - so you will most
probably also collect empty owners in new_ids below. (Note that once an owner is
assigned it cannot be removed).

https://codereview.chromium.org/127423002/diff/40001/chrome/browser/ui/ash/mu...
File chrome/browser/ui/ash/multi_user/multi_user_notification_blocker_chromeos.h
(right):

https://codereview.chromium.org/127423002/diff/40001/chrome/browser/ui/ash/mu...
chrome/browser/ui/ash/multi_user/multi_user_notification_blocker_chromeos.h:27:
// Called by MultiUserWindowManager when the the active user has changed.
nit: .. when the active ..

https://codereview.chromium.org/127423002/diff/40001/chrome/browser/ui/ash/mu...
File chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.cc
(right):

https://codereview.chromium.org/127423002/diff/40001/chrome/browser/ui/ash/mu...
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.cc:254:
notification_blocker_->UpdateWindowOwners();
At the moment we have only this use case, but we might consider to add an
observer mechanism here. Please add a Todo(skuhne) here and below to replace
this accordingly.

https://codereview.chromium.org/127423002/diff/40001/chrome/browser/ui/ash/mu...
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.cc:304:
notification_blocker_->UpdateWindowOwners();
See above

https://codereview.chromium.org/127423002/diff/40001/chrome/browser/ui/ash/mu...
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.cc:434:
notification_blocker_->UpdateWindowOwners();
And again.

https://codereview.chromium.org/127423002/diff/40001/chrome/browser/ui/ash/mu...
File chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.h
(right):

https://codereview.chromium.org/127423002/diff/40001/chrome/browser/ui/ash/mu...
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.h:183: //
The blocker to controls the desktop notification visibility based on the
Nit: "The blocker which controls .." - or better - "The blocker to control ..."

[Jun Mukai, 2014-01-08 20:41:23]

https://codereview.chromium.org/127423002/diff/40001/chrome/browser/ui/ash/mu...
File
chrome/browser/ui/ash/multi_user/multi_user_notification_blocker_chromeos.cc
(right):

https://codereview.chromium.org/127423002/diff/40001/chrome/browser/ui/ash/mu...
chrome/browser/ui/ash/multi_user/multi_user_notification_blocker_chromeos.cc:29:
// Browser/apps windows exist only on the default container.
On 2014/01/08 17:29:35, Mr4D wrote:
> Not quite sure if it is worth while the effort, but you might want to check
> multi_user_manager->AreWindowsSharedAmongUsers() to see if there are any
windows
> shared before doing this, since visiting windows might not be used too often.

I added GetWindowOwners() to MultiUserWindowManager. In that situation calling
AreWindowsSharedAmongUsers() won't reduce the computation cost so much because
both are really similar loop.

https://codereview.chromium.org/127423002/diff/40001/chrome/browser/ui/ash/mu...
chrome/browser/ui/ash/multi_user/multi_user_notification_blocker_chromeos.cc:33:
std::set<std::string> new_ids;
On 2014/01/08 17:29:35, Mr4D wrote:
> Looking at this I would guess that this should be done in the
> MultiUserWindowManager itslef since there are all registered windows already
in
> a list and collecting these together should be much faster. Please create an
> issue and I will do that today.

Done.

https://codereview.chromium.org/127423002/diff/40001/chrome/browser/ui/ash/mu...
chrome/browser/ui/ash/multi_user/multi_user_notification_blocker_chromeos.cc:37:
aura::Window* child = children[i];
On 2014/01/08 17:29:35, Mr4D wrote:
> IsWindowOnDesktopOfUser(<unowned_window>) will return true - so you will most
> probably also collect empty owners in new_ids below. (Note that once an owner
is
> assigned it cannot be removed).

this logic itself was removed.

https://codereview.chromium.org/127423002/diff/40001/chrome/browser/ui/ash/mu...
File chrome/browser/ui/ash/multi_user/multi_user_notification_blocker_chromeos.h
(right):

https://codereview.chromium.org/127423002/diff/40001/chrome/browser/ui/ash/mu...
chrome/browser/ui/ash/multi_user/multi_user_notification_blocker_chromeos.h:27:
// Called by MultiUserWindowManager when the the active user has changed.
On 2014/01/08 17:29:35, Mr4D wrote:
> nit: .. when the active ..

Done.

https://codereview.chromium.org/127423002/diff/40001/chrome/browser/ui/ash/mu...
File chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.cc
(right):

https://codereview.chromium.org/127423002/diff/40001/chrome/browser/ui/ash/mu...
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.cc:254:
notification_blocker_->UpdateWindowOwners();
On 2014/01/08 17:29:35, Mr4D wrote:
> At the moment we have only this use case, but we might consider to add an
> observer mechanism here. Please add a Todo(skuhne) here and below to replace
> this accordingly.

Done.

https://codereview.chromium.org/127423002/diff/40001/chrome/browser/ui/ash/mu...
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.cc:304:
notification_blocker_->UpdateWindowOwners();
On 2014/01/08 17:29:35, Mr4D wrote:
> See above

Done.

https://codereview.chromium.org/127423002/diff/40001/chrome/browser/ui/ash/mu...
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.cc:434:
notification_blocker_->UpdateWindowOwners();
On 2014/01/08 17:29:35, Mr4D wrote:
> And again.

Done.

https://codereview.chromium.org/127423002/diff/40001/chrome/browser/ui/ash/mu...
File chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.h
(right):

https://codereview.chromium.org/127423002/diff/40001/chrome/browser/ui/ash/mu...
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.h:183: //
The blocker to controls the desktop notification visibility based on the
On 2014/01/08 17:29:35, Mr4D wrote:
> Nit: "The blocker which controls .." - or better - "The blocker to control
..."

Done.

[Mr4D (OOO till 08-26), 2014-01-08 20:53:53]

lgtm after addressing the few nits.

https://codereview.chromium.org/127423002/diff/180001/chrome/browser/ui/ash/m...
File chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.h
(right):

https://codereview.chromium.org/127423002/diff/180001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.h:64:
virtual void GetWindowOwners(std::set<std::string>* user_ids) OVERRIDE;
This should possibly be GetOwnersOfVisibleWindows.

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

https://codereview.chromium.org/127423002/diff/180001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc:67:
// Returns a test-friendly string format of GetWindowOwners().
GetOwnersOfVisibleWindows?

https://codereview.chromium.org/127423002/diff/180001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc:316:
EXPECT_EQ("H[A], S[A,B], S[B], S[B], H[C]", GetStatus());
Thanks for adding these tests!

[Jun Mukai, 2014-01-08 21:20:17]

https://codereview.chromium.org/127423002/diff/180001/chrome/browser/ui/ash/m...
File chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.h
(right):

https://codereview.chromium.org/127423002/diff/180001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.h:64:
virtual void GetWindowOwners(std::set<std::string>* user_ids) OVERRIDE;
On 2014/01/08 20:53:53, Mr4D wrote:
> This should possibly be GetOwnersOfVisibleWindows.

Done.

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

https://codereview.chromium.org/127423002/diff/180001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc:67:
// Returns a test-friendly string format of GetWindowOwners().
On 2014/01/08 20:53:53, Mr4D wrote:
> GetOwnersOfVisibleWindows?

Done.

[dewittj, 2014-01-09 00:43:50]

lgtm

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

https://codereview.chromium.org/127423002/diff/250001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc:68:
std::string GetOwnersOfVisibleWindows();
Can you rename this to GetOwnersOfVisibleWindowsAsString() or something so it's
clearer that you're calling the test method?

[Jun Mukai, 2014-01-09 19:37:47]

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

https://codereview.chromium.org/127423002/diff/250001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc:68:
std::string GetOwnersOfVisibleWindows();
On 2014/01/09 00:43:51, dewittj wrote:
> Can you rename this to GetOwnersOfVisibleWindowsAsString() or something so
it's
> clearer that you're calling the test method?

Done.

[Jun Mukai, 2014-01-09 19:39:03]

sky, please take a look.
I have to edit chrome/browser/ui/browser_navigator_browsertest_chromeos.cc
because it has TestMultiUserWindowManager...

[sky, 2014-01-09 21:26:35]

LGTM

[Jun Mukai, 2014-01-09 22:40:02]

Patchset6 fails in WallpaperPrivateApiMultiUserUnittest due to the destruction
order.
Let me commit with TBR=bshe since this change is trivial.

[commit-bot: I haz the power, 2014-01-09 22:47:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mukai@chromium.org/127423002/360001

[commit-bot: I haz the power, 2014-01-10 01:23:11]

Retried try job too often on linux_aura for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2014-01-10 01:50:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mukai@chromium.org/127423002/360001

[commit-bot: I haz the power, 2014-01-10 02:55:39]

Change committed as 244058