Remove RestoreFocusedView in OnWindowActivated for DesktopNativeWidgetAura

Remove RestoreFocusedView in OnWindowActivated for DesktopNativeWidgetAura

RestoreFocusedView upon desktop native widget activation should be done in DesktopNativeWidgetAura::HandleActivationChanged, instead of OnWindowActivated, as in OnWindowActivated, GetWidget()->IsActive() is asynced for Linux. So we should remove the related code.

BUG=659080
TEST=manual build test sees bug fixed

Committed: https://crrev.com/c5e551b4da8b4bf7cc8fcfbdff3a052f19619bf3
Cr-Commit-Position: refs/heads/master@{#428157}

[Qiang(Joe) Xu, 2016-10-26 20:55:52]

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

[commit-bot: I haz the power, 2016-10-26 20:56:17]

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

[Qiang(Joe) Xu, 2016-10-26 20:59:20]

Description was changed from

==========
For DesktopNativeWidgetAura, RestoreFocusedView should not be done in
OnWindowActivated

BUG=659080
TEST=manual build test sees bug fixed
==========

to

==========
Remove RestoreFocusedView in OnWindowActivated for DesktopNativeWidgetAura

BUG=659080
TEST=manual build test sees bug fixed
==========

[Qiang(Joe) Xu, 2016-10-26 20:59:21]

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

[Qiang(Joe) Xu, 2016-10-26 21:38:40]

Description was changed from

==========
Remove RestoreFocusedView in OnWindowActivated for DesktopNativeWidgetAura

BUG=659080
TEST=manual build test sees bug fixed
==========

to

==========
Remove RestoreFocusedView in OnWindowActivated for DesktopNativeWidgetAura

RestoreFocusedView upon desktop native widget activation should be done in
DesktopNativeWidgetAura::HandleActivationChanged, instead of OnWindowActivated,
as in OnWindowActivated, GetWidget()->IsActive() is asynced for Linux. So we
should remove the related code.

BUG=659080
TEST=manual build test sees bug fixed
==========

[commit-bot: I haz the power, 2016-10-26 22:17:15]

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

[commit-bot: I haz the power, 2016-10-26 22:17:16]

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

[Qiang(Joe) Xu, 2016-10-26 23:10:10]

Hi Scott, could you please take a loot at this? tks

The trybot tests failure are marked flaky on crbug.com. So it should not be
caused by this CL.

[sky, 2016-10-26 23:47:59]

https://codereview.chromium.org/2451323002/diff/1/ui/views/widget/desktop_aur...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (left):

https://codereview.chromium.org/2451323002/diff/1/ui/views/widget/desktop_aur...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:1083:
DCHECK(content_window_ == gained_active || content_window_ == lost_active);
This DCHECK still seems useful. Is there a reason you are removing it?

https://codereview.chromium.org/2451323002/diff/1/ui/views/widget/desktop_aur...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:1086:
GetWidget()->GetFocusManager()->RestoreFocusedView();
I'm not understanding why you only want to do the Store() here and not the
Restore(). Shouldn't both be done in the same place?

Also, how do we end up calling to RestoreFocusedView if you remove the call
here?

[Qiang(Joe) Xu, 2016-10-27 17:28:17]

Hi Scott, there is an update, ptal.

https://codereview.chromium.org/2451323002/diff/1/ui/views/widget/desktop_aur...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (left):

https://codereview.chromium.org/2451323002/diff/1/ui/views/widget/desktop_aur...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:1083:
DCHECK(content_window_ == gained_active || content_window_ == lost_active);
On 2016/10/26 23:47:59, sky wrote:
> This DCHECK still seems useful. Is there a reason you are removing it?

Agree. I restored it.

https://codereview.chromium.org/2451323002/diff/1/ui/views/widget/desktop_aur...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:1086:
GetWidget()->GetFocusManager()->RestoreFocusedView();
On 2016/10/26 23:47:59, sky wrote:
> I'm not understanding why you only want to do the Store() here and not the
> Restore(). Shouldn't both be done in the same place?
> 
> Also, how do we end up calling to RestoreFocusedView if you remove the call
> here?

Restore on activation should depend on desktop native widget activation, which
is handled on:
https://cs.chromium.org/chromium/src/ui/views/widget/desktop_aura/desktop_nat...
Store on deactivation should depend on aura activation changes, as the child
widgets may not get the desktop native widget activation change.

[sky, 2016-10-27 19:34:06]

Thanks for the clarification. LGTM

[Qiang(Joe) Xu, 2016-10-27 20:36:12]

The CQ bit was checked by warx@chromium.org

[commit-bot: I haz the power, 2016-10-27 20:37:15]

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, 2016-10-27 22:12:37]

Description was changed from

==========
Remove RestoreFocusedView in OnWindowActivated for DesktopNativeWidgetAura

RestoreFocusedView upon desktop native widget activation should be done in
DesktopNativeWidgetAura::HandleActivationChanged, instead of OnWindowActivated,
as in OnWindowActivated, GetWidget()->IsActive() is asynced for Linux. So we
should remove the related code.

BUG=659080
TEST=manual build test sees bug fixed
==========

to

==========
Remove RestoreFocusedView in OnWindowActivated for DesktopNativeWidgetAura

RestoreFocusedView upon desktop native widget activation should be done in
DesktopNativeWidgetAura::HandleActivationChanged, instead of OnWindowActivated,
as in OnWindowActivated, GetWidget()->IsActive() is asynced for Linux. So we
should remove the related code.

BUG=659080
TEST=manual build test sees bug fixed
==========

[commit-bot: I haz the power, 2016-10-27 22:12:38]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-27 22:16:37]

Description was changed from

==========
Remove RestoreFocusedView in OnWindowActivated for DesktopNativeWidgetAura

RestoreFocusedView upon desktop native widget activation should be done in
DesktopNativeWidgetAura::HandleActivationChanged, instead of OnWindowActivated,
as in OnWindowActivated, GetWidget()->IsActive() is asynced for Linux. So we
should remove the related code.

BUG=659080
TEST=manual build test sees bug fixed
==========

to

==========
Remove RestoreFocusedView in OnWindowActivated for DesktopNativeWidgetAura

RestoreFocusedView upon desktop native widget activation should be done in
DesktopNativeWidgetAura::HandleActivationChanged, instead of OnWindowActivated,
as in OnWindowActivated, GetWidget()->IsActive() is asynced for Linux. So we
should remove the related code.

BUG=659080
TEST=manual build test sees bug fixed

Committed: https://crrev.com/c5e551b4da8b4bf7cc8fcfbdff3a052f19619bf3
Cr-Commit-Position: refs/heads/master@{#428157}
==========

[commit-bot: I haz the power, 2016-10-27 22:16:37]

Patchset 2 (id:??) landed as
https://crrev.com/c5e551b4da8b4bf7cc8fcfbdff3a052f19619bf3
Cr-Commit-Position: refs/heads/master@{#428157}