Change FocusSynchronizer to maintain active focus client and window.

Change FocusSynchronizer to maintain active focus client and window. 

Deleted all active focus client and active focus client root 
maintaining code from aura::Env and updated FocusSynchronizer to be
the place that keeps track of them. All clients that used to be the 
EnvObserver to get notified for active focus client and active focus
client root changes are now changed to be the observer for 
FocusSynchronizer to get notified.

Moved kFocusClientKey definition from FocusClient to aura_constants. 

BUG=699324
TEST=covered by tests (aura_unittests and views_mus_unittests)

Review-Url: https://codereview.chromium.org/2714763002
Cr-Commit-Position: refs/heads/master@{#460963}
Committed: https://chromium.googlesource.com/chromium/src/+/86a7cee9063b92b30353bfa0e32c3bfb24625593

[riajiang, 2017-03-08 01:01:17]

Patchset #2 (id:20001) has been deleted

[riajiang, 2017-03-08 01:07:34]

Patchset #2 (id:40001) has been deleted

[riajiang, 2017-03-08 01:07:48]

Patchset #1 (id:1) has been deleted

[riajiang, 2017-03-08 01:16:16]

Description was changed from

==========
WIP

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

to

==========
Change FocusSynchronizer to maintain active focus client and window. 

Deleted all active focus client and window maintaining code from 
aura::Env and updated FocusSynchronizer to be the place that keeps 
track of active focus client and focused window. All clients that 
used to be the EnvObserver to get notified for active focus client 
and focused window changes are now changed to be the observer for 
FocusSynchronizer to get notified.

Moved kFocusClientKey definition from FocusClient to aura_constants. 

BUG=699324
TEST=covered by tests (aura_unittests and views_mus_unittests)
==========

[riajiang, 2017-03-08 01:17:03]

riajiang@chromium.org changed reviewers:
+ sadrul@chromium.org, sky@chromium.org

[riajiang, 2017-03-08 01:18:31]

Hi sadrul@ and sky@, PTAL, thanks!

[riajiang, 2017-03-08 01:35:22]

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

[commit-bot: I haz the power, 2017-03-08 01:36:21]

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-03-08 02:20:50]

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

[commit-bot: I haz the power, 2017-03-08 02:20:51]

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

[sadrul, 2017-03-09 02:01:23]

https://codereview.chromium.org/2714763002/diff/80001/ui/views/mus/desktop_wi...
File ui/views/mus/desktop_window_tree_host_mus.cc (right):

https://codereview.chromium.org/2714763002/diff/80001/ui/views/mus/desktop_wi...
ui/views/mus/desktop_window_tree_host_mus.cc:770: if (window == this->window())
{
Could it not do this by just being a FocusChangeObserver (and adding itself to
the FocusClient)? i.e. do we need to introduce the FocusSynchronizerObserver?

[riajiang, 2017-03-14 21:58:07]

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

[commit-bot: I haz the power, 2017-03-14 21:59:01]

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-03-14 23:54:54]

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

[commit-bot: I haz the power, 2017-03-14 23:54:56]

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_...)

[riajiang, 2017-03-15 20:35:04]

Patchset #4 (id:120001) has been deleted

[riajiang, 2017-03-15 20:35:12]

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

[commit-bot: I haz the power, 2017-03-15 20:35:50]

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-03-15 21:41:49]

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

[commit-bot: I haz the power, 2017-03-15 21:41:50]

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_...)

[riajiang, 2017-03-23 19:00:38]

Description was changed from

==========
Change FocusSynchronizer to maintain active focus client and window. 

Deleted all active focus client and window maintaining code from 
aura::Env and updated FocusSynchronizer to be the place that keeps 
track of active focus client and focused window. All clients that 
used to be the EnvObserver to get notified for active focus client 
and focused window changes are now changed to be the observer for 
FocusSynchronizer to get notified.

Moved kFocusClientKey definition from FocusClient to aura_constants. 

BUG=699324
TEST=covered by tests (aura_unittests and views_mus_unittests)
==========

to

==========
Change FocusSynchronizer to maintain active focus client and window. 

Deleted all active focus client and active focus client root 
maintaining code from aura::Env and updated FocusSynchronizer to be
the place that keeps track of them. All clients that used to be the 
EnvObserver to get notified for active focus client and active focus
client root changes are now changed to be the observer for 
FocusSynchronizer to get notified.

Moved kFocusClientKey definition from FocusClient to aura_constants. 

BUG=699324
TEST=covered by tests (aura_unittests and views_mus_unittests)
==========

[riajiang, 2017-03-23 19:46:00]

Patchset #5 (id:160001) has been deleted

[riajiang, 2017-03-23 19:50:44]

As discussed with sadrul@ offline, keeping FocusSynchronizerObserver for
active_focus_client and active_focus_client_root changes for
DesktopWindowTreeHostMus and WmHelperMus. FocusChangeObserver would get notified
when the actual focused window gets changed. Could you take another look?
Thanks!

[riajiang, 2017-03-23 19:50:54]

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

[commit-bot: I haz the power, 2017-03-23 19:51:26]

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-03-23 21:05:35]

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

[commit-bot: I haz the power, 2017-03-23 21:05:36]

Dry run: This issue passed the CQ dry run.

[sky, 2017-03-23 23:02:43]

On 2017/03/23 19:50:44, riajiang wrote:
> As discussed with sadrul@ offline, keeping FocusSynchronizerObserver for
> active_focus_client and active_focus_client_root changes for
> DesktopWindowTreeHostMus and WmHelperMus. FocusChangeObserver would get
notified
> when the actual focused window gets changed. Could you take another look?
> Thanks!

I'm assuming Sadrul is reviewing all this and you don't need me. If that isn't
the case, just say so.

[sky, 2017-03-23 23:02:56]

Also, you will need an exo reviewer

[sadrul, 2017-03-28 15:03:23]

https://codereview.chromium.org/2714763002/diff/180001/ui/aura/mus/focus_sync...
File ui/aura/mus/focus_synchronizer.cc (right):

https://codereview.chromium.org/2714763002/diff/180001/ui/aura/mus/focus_sync...
ui/aura/mus/focus_synchronizer.cc:68:
active_focus_client_root_->AddObserver(this);
Wrap this inside an if (active_focus_client_root_ != focus_client_root)

https://codereview.chromium.org/2714763002/diff/180001/ui/aura/mus/focus_sync...
File ui/aura/mus/focus_synchronizer.h (right):

https://codereview.chromium.org/2714763002/diff/180001/ui/aura/mus/focus_sync...
ui/aura/mus/focus_synchronizer.h:41: WindowMus* focused_window() { return
focused_window_; }
Doesn't look like this is actually ever used by anyone.

https://codereview.chromium.org/2714763002/diff/180001/ui/aura/mus/focus_sync...
ui/aura/mus/focus_synchronizer.h:57: // a FocusClient.
Why would a null |focus_client_root| imply all windows share a FocusClient? That
seems weird. I would expect it to be null when there is no known active window
(e.g. some other client has the active window), but in that case, the
|focus_client| should also be null, right?

https://codereview.chromium.org/2714763002/diff/180001/ui/aura/mus/focus_sync...
File ui/aura/mus/focus_synchronizer_observer.h (right):

https://codereview.chromium.org/2714763002/diff/180001/ui/aura/mus/focus_sync...
ui/aura/mus/focus_synchronizer_observer.h:29: virtual
~FocusSynchronizerObserver() {}
I think this can just be public

[riajiang, 2017-03-28 22:07:27]

https://codereview.chromium.org/2714763002/diff/180001/ui/aura/mus/focus_sync...
File ui/aura/mus/focus_synchronizer.cc (right):

https://codereview.chromium.org/2714763002/diff/180001/ui/aura/mus/focus_sync...
ui/aura/mus/focus_synchronizer.cc:68:
active_focus_client_root_->AddObserver(this);
On 2017/03/28 15:03:23, sadrul wrote:
> Wrap this inside an if (active_focus_client_root_ != focus_client_root)

I'm not sure if I misunderstood - there's an early return at the beginning if
(active_focus_client_root_ == focus_client_root)? Do you mean at a different
place?

https://codereview.chromium.org/2714763002/diff/180001/ui/aura/mus/focus_sync...
File ui/aura/mus/focus_synchronizer.h (right):

https://codereview.chromium.org/2714763002/diff/180001/ui/aura/mus/focus_sync...
ui/aura/mus/focus_synchronizer.h:41: WindowMus* focused_window() { return
focused_window_; }
On 2017/03/28 15:03:23, sadrul wrote:
> Doesn't look like this is actually ever used by anyone.

It is used by WindowTreeClient::OnWindowMusDestroyed
(https://cs.chromium.org/chromium/src/ui/aura/mus/window_tree_client.cc?gsn=Fo...).

https://codereview.chromium.org/2714763002/diff/180001/ui/aura/mus/focus_sync...
ui/aura/mus/focus_synchronizer.h:57: // a FocusClient.
On 2017/03/28 15:03:23, sadrul wrote:
> Why would a null |focus_client_root| imply all windows share a FocusClient?
That
> seems weird. I would expect it to be null when there is no known active window
> (e.g. some other client has the active window), but in that case, the
> |focus_client| should also be null, right?

I copied this comment from aura::Env::SetActiveFocusClient and not sure if I
understood the last sentence completely. |focus_client_root| is the window
associated with |focus_client| so I think it should never be null if
|focus_client| is not null? I think it means the current focused window may be
null? Deleted the last sentence.

https://codereview.chromium.org/2714763002/diff/180001/ui/aura/mus/focus_sync...
File ui/aura/mus/focus_synchronizer_observer.h (right):

https://codereview.chromium.org/2714763002/diff/180001/ui/aura/mus/focus_sync...
ui/aura/mus/focus_synchronizer_observer.h:29: virtual
~FocusSynchronizerObserver() {}
On 2017/03/28 15:03:23, sadrul wrote:
> I think this can just be public

But the current pattern of other observer classes is putting dtor in protected?

[sadrul, 2017-03-29 16:13:19]

https://codereview.chromium.org/2714763002/diff/180001/ui/aura/mus/focus_sync...
File ui/aura/mus/focus_synchronizer.cc (right):

https://codereview.chromium.org/2714763002/diff/180001/ui/aura/mus/focus_sync...
ui/aura/mus/focus_synchronizer.cc:68:
active_focus_client_root_->AddObserver(this);
On 2017/03/28 22:07:27, riajiang wrote:
> On 2017/03/28 15:03:23, sadrul wrote:
> > Wrap this inside an if (active_focus_client_root_ != focus_client_root)
> 
> I'm not sure if I misunderstood - there's an early return at the beginning if
> (active_focus_client_root_ == focus_client_root)? Do you mean at a different
> place?

It checks both the client and the root. Although I guess if the windows are the
same, then the clients should also be the same?

Maybe update the if above to instead be something like:

  if (focus_client_root == active_focus_client_root_) {
    DCHECK_EQ(focus_client, active_focus_client_);
    return;
  }

https://codereview.chromium.org/2714763002/diff/180001/ui/aura/mus/focus_sync...
File ui/aura/mus/focus_synchronizer_observer.h (right):

https://codereview.chromium.org/2714763002/diff/180001/ui/aura/mus/focus_sync...
ui/aura/mus/focus_synchronizer_observer.h:29: virtual
~FocusSynchronizerObserver() {}
On 2017/03/28 22:07:27, riajiang wrote:
> On 2017/03/28 15:03:23, sadrul wrote:
> > I think this can just be public
> 
> But the current pattern of other observer classes is putting dtor in
protected?

Yes. I can't think of a good reason for doing this though. You can keep it as it
is, or move it to public. Upto you.

[riajiang, 2017-03-29 17:27:35]

https://codereview.chromium.org/2714763002/diff/180001/ui/aura/mus/focus_sync...
File ui/aura/mus/focus_synchronizer.cc (right):

https://codereview.chromium.org/2714763002/diff/180001/ui/aura/mus/focus_sync...
ui/aura/mus/focus_synchronizer.cc:68:
active_focus_client_root_->AddObserver(this);
On 2017/03/29 16:13:19, sadrul wrote:
> On 2017/03/28 22:07:27, riajiang wrote:
> > On 2017/03/28 15:03:23, sadrul wrote:
> > > Wrap this inside an if (active_focus_client_root_ != focus_client_root)
> > 
> > I'm not sure if I misunderstood - there's an early return at the beginning
if
> > (active_focus_client_root_ == focus_client_root)? Do you mean at a different
> > place?
> 
> It checks both the client and the root. Although I guess if the windows are
the
> same, then the clients should also be the same?
> 
> Maybe update the if above to instead be something like:
> 
>   if (focus_client_root == active_focus_client_root_) {
>     DCHECK_EQ(focus_client, active_focus_client_);
>     return;
>   }

Yea. Done.

https://codereview.chromium.org/2714763002/diff/180001/ui/aura/mus/focus_sync...
File ui/aura/mus/focus_synchronizer_observer.h (right):

https://codereview.chromium.org/2714763002/diff/180001/ui/aura/mus/focus_sync...
ui/aura/mus/focus_synchronizer_observer.h:29: virtual
~FocusSynchronizerObserver() {}
On 2017/03/29 16:13:19, sadrul wrote:
> On 2017/03/28 22:07:27, riajiang wrote:
> > On 2017/03/28 15:03:23, sadrul wrote:
> > > I think this can just be public
> > 
> > But the current pattern of other observer classes is putting dtor in
> protected?
> 
> Yes. I can't think of a good reason for doing this though. You can keep it as
it
> is, or move it to public. Upto you.

I think I'll keep it here to be consistent for now.

[sadrul, 2017-03-29 19:52:11]

lgtm

[riajiang, 2017-03-29 19:54:57]

riajiang@chromium.org changed reviewers:
+ reveman@chromium.org

[riajiang, 2017-03-29 19:54:57]

+reveman@, could you take a look at components/exo? Thanks!

[reveman, 2017-03-29 20:40:56]

components/exo lgtm

[riajiang, 2017-03-29 21:07:55]

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

[commit-bot: I haz the power, 2017-03-29 21:08:36]

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-03-29 21:41:17]

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

[commit-bot: I haz the power, 2017-03-29 21:41:18]

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_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[riajiang, 2017-03-30 17:57:30]

Patchset #8 (id:240001) has been deleted

[riajiang, 2017-03-30 17:57:37]

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

[commit-bot: I haz the power, 2017-03-30 17:58:05]

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-03-30 19:26:08]

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

[commit-bot: I haz the power, 2017-03-30 19:26:09]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[riajiang, 2017-03-31 00:41:01]

The CQ bit was checked by riajiang@chromium.org

[riajiang, 2017-03-31 00:41:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
reveman@chromium.org, sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2714763002/#ps260001
(title: "test")

[commit-bot: I haz the power, 2017-03-31 00:41:55]

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-03-31 00:56:30]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1490920861787110,
"parent_rev": "9756fe7ccbfda2cc417659366fcbf36c9b837745", "commit_rev":
"86a7cee9063b92b30353bfa0e32c3bfb24625593"}

[commit-bot: I haz the power, 2017-03-31 00:57:12]

Description was changed from

==========
Change FocusSynchronizer to maintain active focus client and window. 

Deleted all active focus client and active focus client root 
maintaining code from aura::Env and updated FocusSynchronizer to be
the place that keeps track of them. All clients that used to be the 
EnvObserver to get notified for active focus client and active focus
client root changes are now changed to be the observer for 
FocusSynchronizer to get notified.

Moved kFocusClientKey definition from FocusClient to aura_constants. 

BUG=699324
TEST=covered by tests (aura_unittests and views_mus_unittests)
==========

to

==========
Change FocusSynchronizer to maintain active focus client and window. 

Deleted all active focus client and active focus client root 
maintaining code from aura::Env and updated FocusSynchronizer to be
the place that keeps track of them. All clients that used to be the 
EnvObserver to get notified for active focus client and active focus
client root changes are now changed to be the observer for 
FocusSynchronizer to get notified.

Moved kFocusClientKey definition from FocusClient to aura_constants. 

BUG=699324
TEST=covered by tests (aura_unittests and views_mus_unittests)

Review-Url: https://codereview.chromium.org/2714763002
Cr-Commit-Position: refs/heads/master@{#460963}
Committed:
https://chromium.googlesource.com/chromium/src/+/86a7cee9063b92b30353bfa0e32c...
==========

[commit-bot: I haz the power, 2017-03-31 00:57:14]

Committed patchset #8 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/86a7cee9063b92b30353bfa0e32c...