ash: fix regression where ctrl+n put new window on wrong desktop

ash: fix regression where ctrl+n put new window on wrong desktop

In multi-user mode on chromeos, we check whether a new window was
created by a user gesture by looking at the state of the MessageLoop and
the EventDispatcher. This doesn't work in an asynchronous world.

This patch replaces this check by manually annotating windows which were
created by user gesture.

BUG=690160
TBR=stevenjb@chromium.org

Review-Url: https://codereview.chromium.org/2685333005
Cr-Commit-Position: refs/heads/master@{#452222}
Committed: https://chromium.googlesource.com/chromium/src/+/7b01d69adba07ea22bc68412c023f9d27fa397e0

[Elliot Glaysher, 2017-02-10 20:05:37]

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

[commit-bot: I haz the power, 2017-02-10 20:06:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-10 21:04:28]

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

[commit-bot: I haz the power, 2017-02-10 21:04:29]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Elliot Glaysher, 2017-02-10 21:09:17]

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

[commit-bot: I haz the power, 2017-02-10 21:09:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-10 22:24:04]

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

[commit-bot: I haz the power, 2017-02-10 22:24:05]

Dry run: This issue passed the CQ dry run.

[Elliot Glaysher, 2017-02-10 22:56:24]

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

[commit-bot: I haz the power, 2017-02-10 22:56:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Elliot Glaysher, 2017-02-10 23:12:08]

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

[commit-bot: I haz the power, 2017-02-10 23:12:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Elliot Glaysher, 2017-02-10 23:45:26]

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

[commit-bot: I haz the power, 2017-02-10 23:45:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-11 00:50:19]

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

[commit-bot: I haz the power, 2017-02-11 00:50:20]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Elliot Glaysher, 2017-02-13 19:05:24]

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

[commit-bot: I haz the power, 2017-02-13 19:06:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Elliot Glaysher, 2017-02-13 20:20:34]

Description was changed from

==========
ash: fix regression where ctrl+n put new window on wrong desktop

In multi-user mode on chromeos, we check whether a new window was
created by a user gesture by looking at the state of the MessageLoop and
the EventDispatcher. This doesn't work in an asynchronous world.

This patch replaces this check by manually annotating windows which were
created by user gesture.

BUG=
==========

to

==========
ash: fix regression where ctrl+n put new window on wrong desktop

In multi-user mode on chromeos, we check whether a new window was
created by a user gesture by looking at the state of the MessageLoop and
the EventDispatcher. This doesn't work in an asynchronous world.

This patch replaces this check by manually annotating windows which were
created by user gesture.

BUG=690160
==========

[commit-bot: I haz the power, 2017-02-13 20:25:37]

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

[commit-bot: I haz the power, 2017-02-13 20:25:38]

Dry run: This issue passed the CQ dry run.

[Elliot Glaysher, 2017-02-13 20:49:15]

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

[Elliot Glaysher, 2017-02-13 20:49:16]

The current code in multi_user_window_manager_chromeos.cc tries to intuit
whether it is currently processing a user gesture by looking at the message loop
and the event dispatcher. This is already fragile in presence of asynchronous
PostTasks, but now that most ui events are cross-process asynchronous, it
doesn't work.

This patch attempts to solve this by annotating all aura windows with a
kCreatedByUserGesture property. Right now, it defaults to true. The only callers
which set it either way are BrowserWindows.

This fixes the immediate regression: ctrl+n now puts the new window on the
correct desktop (and I tested dragging windows out of a window and using the New
Window menu item, too).

However, since we're now not doing something general, I must be forgetting
something.

[Mr4D (OOO till 08-26), 2017-02-13 23:57:39]

Only a few nits.

https://codereview.chromium.org/2685333005/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/srt_fetcher_win.cc (right):

https://codereview.chromium.org/2685333005/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/srt_fetcher_win.cc:532: params.user_gesture =
false;
A default of true is odd. But ok

https://codereview.chromium.org/2685333005/diff/100001/chrome/browser/ui/brow...
File chrome/browser/ui/browser.h (right):

https://codereview.chromium.org/2685333005/diff/100001/chrome/browser/ui/brow...
chrome/browser/ui/browser.h:189: // Whether this browser was created by a user
gesture.
Please add a comment towards why this is&needed - not a repetition of the name.

https://codereview.chromium.org/2685333005/diff/100001/chrome/browser/ui/brow...
chrome/browser/ui/browser.h:190: bool user_gesture;
The name "user_gesture" suggests that there must have been touch used. That is
rather confusing.

How about "user_action" - or "user_activation"?

https://codereview.chromium.org/2685333005/diff/100001/ui/aura/client/aura_co...
File ui/aura/client/aura_constants.cc (right):

https://codereview.chromium.org/2685333005/diff/100001/ui/aura/client/aura_co...
ui/aura/client/aura_constants.cc:38: DEFINE_UI_CLASS_PROPERTY_KEY(bool,
kCreatedByUserGesture, false);
If the default is true - shouldn't this be true as well?

[joenotcharles, 2017-02-14 00:09:15]

joenotcharles@google.com changed reviewers:
+ joenotcharles@google.com

[joenotcharles, 2017-02-14 00:09:16]

https://codereview.chromium.org/2685333005/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/srt_fetcher_win.cc (right):

https://codereview.chromium.org/2685333005/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/srt_fetcher_win.cc:532: params.user_gesture =
false;
On 2017/02/13 23:57:39, Mr4D wrote:
> A default of true is odd. But ok

I feel like most people who call CreateParms won't know that the user_gesture
field exists and won't bother to set it. If the default isn't always the right
thing to do for callers who don't know to set it (and I don't think it is) then
it should be always be passed as a parameter to CreateParams so that every
caller has to think about what to set it to.

[joenotcharles, 2017-02-14 00:10:39]

https://codereview.chromium.org/2685333005/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/srt_fetcher_win.cc (right):

https://codereview.chromium.org/2685333005/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/srt_fetcher_win.cc:532: params.user_gesture =
false;
On 2017/02/14 00:09:16, joenotcharles wrote:
> On 2017/02/13 23:57:39, Mr4D wrote:
> > A default of true is odd. But ok
> 
> I feel like most people who call CreateParms won't know that the user_gesture
> field exists and won't bother to set it. If the default isn't always the right
> thing to do for callers who don't know to set it (and I don't think it is)
then
> it should be always be passed as a parameter to CreateParams so that every
> caller has to think about what to set it to.

(That said, I'm skeptical that plumbing this through everywhere is a good idea,
but I don't have a better suggestion.)

[Mr4D (OOO till 08-26), 2017-02-14 00:35:57]

Yepp - me neither. I was looking into this some years back and back then it
wasn't obvious either. So - patching it through via options sounds like a plan.

[Elliot Glaysher, 2017-02-14 20:27:11]

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

[commit-bot: I haz the power, 2017-02-14 20:27:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-14 20:46:35]

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

[commit-bot: I haz the power, 2017-02-14 20:46:36]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Elliot Glaysher, 2017-02-14 20:51:33]

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

[commit-bot: I haz the power, 2017-02-14 20:52:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-14 21:11:36]

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

[commit-bot: I haz the power, 2017-02-14 21:11:37]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Elliot Glaysher, 2017-02-14 21:28:20]

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

[commit-bot: I haz the power, 2017-02-14 21:29:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Elliot Glaysher, 2017-02-14 21:45:11]

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

[commit-bot: I haz the power, 2017-02-14 21:46:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-14 22:08:41]

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

[commit-bot: I haz the power, 2017-02-14 22:08:42]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Elliot Glaysher, 2017-02-14 22:11:42]

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

[commit-bot: I haz the power, 2017-02-14 22:12:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Elliot Glaysher, 2017-02-14 22:30:22]

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

[commit-bot: I haz the power, 2017-02-14 22:30:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Elliot Glaysher, 2017-02-14 23:00:46]

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

[commit-bot: I haz the power, 2017-02-14 23:01:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Elliot Glaysher, 2017-02-14 23:32:28]

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

[commit-bot: I haz the power, 2017-02-14 23:33:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-15 00:16:05]

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

[commit-bot: I haz the power, 2017-02-15 00:16:06]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Elliot Glaysher, 2017-02-15 00:25:41]

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

[commit-bot: I haz the power, 2017-02-15 00:26:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-15 01:04:57]

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

[commit-bot: I haz the power, 2017-02-15 01:04:58]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Elliot Glaysher, 2017-02-15 01:07:54]

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

[commit-bot: I haz the power, 2017-02-15 01:09:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-15 01:59:29]

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

[commit-bot: I haz the power, 2017-02-15 01:59:30]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Elliot Glaysher, 2017-02-15 18:19:02]

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

[commit-bot: I haz the power, 2017-02-15 18:20:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-15 18:45:33]

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

[commit-bot: I haz the power, 2017-02-15 18:45:34]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Elliot Glaysher, 2017-02-15 18:50:44]

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

[commit-bot: I haz the power, 2017-02-15 18:51:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Elliot Glaysher, 2017-02-15 19:11:37]

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

[commit-bot: I haz the power, 2017-02-15 19:12:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-15 19:49:34]

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

[commit-bot: I haz the power, 2017-02-15 19:49:35]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Elliot Glaysher, 2017-02-15 20:16:19]

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

[commit-bot: I haz the power, 2017-02-15 20:17:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-15 22:07:52]

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

[commit-bot: I haz the power, 2017-02-15 22:07:54]

Dry run: This issue passed the CQ dry run.

[Elliot Glaysher, 2017-02-15 22:15:02]

I've gone through and added a user_gesture boolean to Browser::CreateParams
constructor.

https://codereview.chromium.org/2685333005/diff/100001/chrome/browser/ui/brow...
File chrome/browser/ui/browser.h (right):

https://codereview.chromium.org/2685333005/diff/100001/chrome/browser/ui/brow...
chrome/browser/ui/browser.h:189: // Whether this browser was created by a user
gesture.
On 2017/02/13 23:57:39, Mr4D wrote:
> Please add a comment towards why this is&needed - not a repetition of the
name.

Done.

https://codereview.chromium.org/2685333005/diff/100001/chrome/browser/ui/brow...
chrome/browser/ui/browser.h:190: bool user_gesture;
On 2017/02/13 23:57:39, Mr4D wrote:
> The name "user_gesture" suggests that there must have been touch used. That is
> rather confusing.
> 
> How about "user_action" - or "user_activation"?

I disagree. The name "user_gesture" predates touch support entirely, and has
always referred to an event caused by user input, whether keyboard or mouse (or
when we added touch support, touch).

$ git grep user_gesture | wc -l
1078

https://codereview.chromium.org/2685333005/diff/100001/ui/aura/client/aura_co...
File ui/aura/client/aura_constants.cc (right):

https://codereview.chromium.org/2685333005/diff/100001/ui/aura/client/aura_co...
ui/aura/client/aura_constants.cc:38: DEFINE_UI_CLASS_PROPERTY_KEY(bool,
kCreatedByUserGesture, false);
On 2017/02/13 23:57:39, Mr4D wrote:
> If the default is true - shouldn't this be true as well?

Sorry, my bad. It's the default Browser window is true, not all windows.

Attempting to set all windows to true breaks several of the multiwindow unit
tests.

[Elliot Glaysher, 2017-02-21 18:39:44]

ping

[Mr4D (OOO till 08-26), 2017-02-21 20:04:26]

Woahhhh! Speaking of exploding CL...

lgtm. You might consider to add a unit test for the other case, but please do
after you have landed this monster.

https://codereview.chromium.org/2685333005/diff/380001/chrome/browser/ui/ash/...
File
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/2685333005/diff/380001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc:1580:
EXPECT_EQ(nullptr, ChromeNewWindowClient::GetActiveBrowser());
We might want to add a test here which checks that a non user activation will
keep the window on the inactive desktop?

[Elliot Glaysher, 2017-02-21 21:19:46]

erg@chromium.org changed reviewers:
+ sky@chromium.org

[Elliot Glaysher, 2017-02-21 21:19:47]

+sky for toplevel owners stamp for changing basically every file which touches
creates a Browser. Most files in this patch are just adding true or false to the
new parameter to Browser::CreateParams().

[sky, 2017-02-21 21:42:08]

LGTM

https://codereview.chromium.org/2685333005/diff/380001/chrome/browser/ui/brow...
File chrome/browser/ui/browser.h (right):

https://codereview.chromium.org/2685333005/diff/380001/chrome/browser/ui/brow...
chrome/browser/ui/browser.h:190: // Whether this browser was created by a user
generated event. We track
When I think of generated I think of
https://chromium.googlesource.com/chromium/src/+/master/ui/events/event_const...
or
https://chromium.googlesource.com/chromium/src/+/master/ui/events/event_const...
. I think you want gesture here, right, e.g. a 'user gesture'?

[Elliot Glaysher, 2017-02-21 21:53:50]

https://codereview.chromium.org/2685333005/diff/380001/chrome/browser/ui/brow...
File chrome/browser/ui/browser.h (right):

https://codereview.chromium.org/2685333005/diff/380001/chrome/browser/ui/brow...
chrome/browser/ui/browser.h:190: // Whether this browser was created by a user
generated event. We track
On 2017/02/21 21:42:07, sky wrote:
> When I think of generated I think of
>
https://chromium.googlesource.com/chromium/src/+/master/ui/events/event_const...
> or
>
https://chromium.googlesource.com/chromium/src/+/master/ui/events/event_const...
> . I think you want gesture here, right, e.g. a 'user gesture'?

Reverted phrasing to what I originally had.

[Elliot Glaysher, 2017-02-21 21:53:59]

The CQ bit was checked by erg@chromium.org

[Elliot Glaysher, 2017-02-21 21:53:59]

The patchset sent to the CQ was uploaded after l-g-t-m from skuhne@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2685333005/#ps400001
(title: "fixes")

[commit-bot: I haz the power, 2017-02-21 21:55:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 00:00:44]

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

[commit-bot: I haz the power, 2017-02-22 00:00:46]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[Elliot Glaysher, 2017-02-22 00:15:02]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2017-02-22 00:16:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Elliot Glaysher, 2017-02-22 01:00:58]

Description was changed from

==========
ash: fix regression where ctrl+n put new window on wrong desktop

In multi-user mode on chromeos, we check whether a new window was
created by a user gesture by looking at the state of the MessageLoop and
the EventDispatcher. This doesn't work in an asynchronous world.

This patch replaces this check by manually annotating windows which were
created by user gesture.

BUG=690160
==========

to

==========
ash: fix regression where ctrl+n put new window on wrong desktop

In multi-user mode on chromeos, we check whether a new window was
created by a user gesture by looking at the state of the MessageLoop and
the EventDispatcher. This doesn't work in an asynchronous world.

This patch replaces this check by manually annotating windows which were
created by user gesture.

BUG=690160
TBR=stevenjb@chromium.org
==========

[Elliot Glaysher, 2017-02-22 01:00:59]

erg@chromium.org changed reviewers:
+ stevenjb@chromium.org

[Elliot Glaysher, 2017-02-22 01:01:45]

TBR stevenjb for minor interface change.

[Elliot Glaysher, 2017-02-22 01:02:05]

The CQ bit was unchecked by erg@chromium.org

[Elliot Glaysher, 2017-02-22 01:02:15]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2017-02-22 01:05:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 02:19:43]

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

[commit-bot: I haz the power, 2017-02-22 02:19:46]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)

[stevenjb, 2017-02-22 16:30:00]

RS LGTM

[Elliot Glaysher, 2017-02-22 18:23:38]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2017-02-22 18:24:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 20:13:28]

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

[commit-bot: I haz the power, 2017-02-22 20:13:30]

Failed to apply patch for chrome/browser/ui/browser_browsertest.cc:
While running git apply --index -p1;
  error: patch failed: chrome/browser/ui/browser_browsertest.cc:1653
  error: chrome/browser/ui/browser_browsertest.cc: patch does not apply

Patch:       chrome/browser/ui/browser_browsertest.cc
Index: chrome/browser/ui/browser_browsertest.cc
diff --git a/chrome/browser/ui/browser_browsertest.cc
b/chrome/browser/ui/browser_browsertest.cc
index
d35e8938bb307b618adace55fe91a652723aab53..ef65157753b687ae286be592ee5c2189ba3dbe74
100644
--- a/chrome/browser/ui/browser_browsertest.cc
+++ b/chrome/browser/ui/browser_browsertest.cc
@@ -1636,7 +1636,7 @@ IN_PROC_BROWSER_TEST_F(BrowserTest, OpenAppWindowLikeNtp)
{
 IN_PROC_BROWSER_TEST_F(BrowserTest, StartMaximized) {
   Browser::Type types[] = { Browser::TYPE_TABBED, Browser::TYPE_POPUP };
   for (size_t i = 0; i < arraysize(types); ++i) {
-    Browser::CreateParams params(types[i], browser()->profile());
+    Browser::CreateParams params(types[i], browser()->profile(), true);
     params.initial_show_state = ui::SHOW_STATE_MAXIMIZED;
     AddBlankTabAndShow(new Browser(params));
   }
@@ -1653,7 +1653,7 @@ IN_PROC_BROWSER_TEST_F(BrowserTest, StartMaximized) {
 IN_PROC_BROWSER_TEST_F(BrowserTest, MAYBE_StartMinimized) {
   Browser::Type types[] = { Browser::TYPE_TABBED, Browser::TYPE_POPUP };
   for (size_t i = 0; i < arraysize(types); ++i) {
-    Browser::CreateParams params(types[i], browser()->profile());
+    Browser::CreateParams params(types[i], browser()->profile(), true);
     params.initial_show_state = ui::SHOW_STATE_MINIMIZED;
     AddBlankTabAndShow(new Browser(params));
   }
@@ -1718,8 +1718,8 @@ IN_PROC_BROWSER_TEST_F(BrowserTest,
DisableMenuItemsWhenIncognitoIsForced) {
   EXPECT_TRUE(command_updater->IsCommandEnabled(IDC_NEW_INCOGNITO_WINDOW));
 
   // Create a new browser.
-  Browser* new_browser = new Browser(
-      Browser::CreateParams(browser()->profile()->GetOffTheRecordProfile()));
+  Browser* new_browser = new Browser(Browser::CreateParams(
+      browser()->profile()->GetOffTheRecordProfile(), true));
   CommandUpdater* new_command_updater =
       new_browser->command_controller()->command_updater();
   // It should have Bookmarks & Settings commands disabled by default.
@@ -1752,7 +1752,7 @@ IN_PROC_BROWSER_TEST_F(BrowserTest,
 
   // Create a new browser.
   Browser* new_browser =
-      new Browser(Browser::CreateParams(browser()->profile()));
+      new Browser(Browser::CreateParams(browser()->profile(), true));
   CommandUpdater* new_command_updater =
       new_browser->command_controller()->command_updater();
  
EXPECT_FALSE(new_command_updater->IsCommandEnabled(IDC_NEW_INCOGNITO_WINDOW));
@@ -1785,7 +1785,7 @@ IN_PROC_BROWSER_TEST_F(BrowserTest,
   // Create a popup (non-main-UI-type) browser. Settings command as well
   // as Extensions should be disabled.
   Browser* popup_browser = new Browser(
-      Browser::CreateParams(Browser::TYPE_POPUP, browser()->profile()));
+      Browser::CreateParams(Browser::TYPE_POPUP, browser()->profile(), true));
   CommandUpdater* popup_command_updater =
       popup_browser->command_controller()->command_updater();
   EXPECT_FALSE(popup_command_updater->IsCommandEnabled(IDC_MANAGE_EXTENSIONS));
@@ -1801,7 +1801,7 @@ IN_PROC_BROWSER_TEST_F(BrowserTest,
                        DisableOptionsAndImportMenuItemsConsistently) {
   // Create a popup browser.
   Browser* popup_browser = new Browser(
-      Browser::CreateParams(Browser::TYPE_POPUP, browser()->profile()));
+      Browser::CreateParams(Browser::TYPE_POPUP, browser()->profile(), true));
   CommandUpdater* command_updater =
       popup_browser->command_controller()->command_updater();
   // OPTIONS and IMPORT_SETTINGS are disabled for a non-normal UI.
@@ -2866,7 +2866,8 @@ IN_PROC_BROWSER_TEST_F(BrowserTest, TestPopupBounds) {
     // Creates an untrusted popup window and asserts that the eventual height
is
     // padded with the toolbar and title bar height (initial height is content
     // height).
-    Browser::CreateParams params(Browser::TYPE_POPUP, browser()->profile());
+    Browser::CreateParams params(Browser::TYPE_POPUP, browser()->profile(),
+                                 true);
     params.initial_bounds = gfx::Rect(0, 0, 100, 122);
     Browser* browser = new Browser(params);
     gfx::Rect bounds = browser->window()->GetBounds();
@@ -2883,7 +2884,8 @@ IN_PROC_BROWSER_TEST_F(BrowserTest, TestPopupBounds) {
   {
     // Creates a trusted popup window and asserts that the eventual height
     // doesn't change (initial height is window height).
-    Browser::CreateParams params(Browser::TYPE_POPUP, browser()->profile());
+    Browser::CreateParams params(Browser::TYPE_POPUP, browser()->profile(),
+                                 true);
     params.initial_bounds = gfx::Rect(0, 0, 100, 122);
     params.trusted_source = true;
     Browser* browser = new Browser(params);
@@ -2900,7 +2902,8 @@ IN_PROC_BROWSER_TEST_F(BrowserTest, TestPopupBounds) {
     // Creates an untrusted app window and asserts that the eventual height
     // doesn't change.
     Browser::CreateParams params = Browser::CreateParams::CreateForApp(
-        "app-name", false, gfx::Rect(0, 0, 100, 122), browser()->profile());
+        "app-name", false, gfx::Rect(0, 0, 100, 122), browser()->profile(),
+        true);
     Browser* browser = new Browser(params);
     gfx::Rect bounds = browser->window()->GetBounds();
 
@@ -2915,7 +2918,8 @@ IN_PROC_BROWSER_TEST_F(BrowserTest, TestPopupBounds) {
     // Creates a trusted app window and asserts that the eventual height
     // doesn't change.
     Browser::CreateParams params = Browser::CreateParams::CreateForApp(
-        "app-name", true, gfx::Rect(0, 0, 100, 122), browser()->profile());
+        "app-name", true, gfx::Rect(0, 0, 100, 122), browser()->profile(),
+        true);
     Browser* browser = new Browser(params);
     gfx::Rect bounds = browser->window()->GetBounds();

[Elliot Glaysher, 2017-02-22 20:41:19]

The CQ bit was checked by erg@chromium.org

[Elliot Glaysher, 2017-02-22 20:41:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, skuhne@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2685333005/#ps420001
(title: "Rebase to ToT")

[commit-bot: I haz the power, 2017-02-22 20:42:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 21:57:39]

CQ is committing da patch.

Bot data: {"patchset_id": 420001, "attempt_start_ts": 1487796079155880,
"parent_rev": "ffa03d80bf40825e68b51bc270472028ebfb760f", "commit_rev":
"7b01d69adba07ea22bc68412c023f9d27fa397e0"}

[commit-bot: I haz the power, 2017-02-22 21:58:17]

Description was changed from

==========
ash: fix regression where ctrl+n put new window on wrong desktop

In multi-user mode on chromeos, we check whether a new window was
created by a user gesture by looking at the state of the MessageLoop and
the EventDispatcher. This doesn't work in an asynchronous world.

This patch replaces this check by manually annotating windows which were
created by user gesture.

BUG=690160
TBR=stevenjb@chromium.org
==========

to

==========
ash: fix regression where ctrl+n put new window on wrong desktop

In multi-user mode on chromeos, we check whether a new window was
created by a user gesture by looking at the state of the MessageLoop and
the EventDispatcher. This doesn't work in an asynchronous world.

This patch replaces this check by manually annotating windows which were
created by user gesture.

BUG=690160
TBR=stevenjb@chromium.org

Review-Url: https://codereview.chromium.org/2685333005
Cr-Commit-Position: refs/heads/master@{#452222}
Committed:
https://chromium.googlesource.com/chromium/src/+/7b01d69adba07ea22bc68412c023...
==========

[commit-bot: I haz the power, 2017-02-22 21:58:19]

Committed patchset #22 (id:420001) as
https://chromium.googlesource.com/chromium/src/+/7b01d69adba07ea22bc68412c023...