Use WindowActivated message instead of VisibilityChanged message to determine that Ash is the activ…

Use WindowActivated message instead of VisibilityChanged message to determine that Ash is the active desktop on Win8.

This is better than the VisibilityChanged event as it also handles the dual monitor case where Ash isn't hidden yet is no longer active (and Chrome desktop window can thus steal the "active desktop" state which was then not given back to the Ash desktop when re-focusing it. The WindowActivated event gets a CoreWindowActivationState_PointerActivated state for this specific case; which we treat the same as CoreWindowActivationState_CodeActivated for now.

Also remove the hooks for VisibilityChanged as they were only used for this purpose, i.e.:
https://code.google.com/p/chromium/codesearch#search/&q=OnRootWindowActivated&sq=package:chromium&type=cs
Called from: https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/env.cc&l=83
Called from: https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/root_window.cc&l=988
Called from: https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/remote_root_window_host_win.cc&l=463

BUG=177489
TEST=
The most reliable test I could find that would repro this is to:
1) Open Ash
2) Open Chrome on desktop on a 2nd monitor
3) Refocus Ash on first monitor
4) Signin with an account which will sync extensions (at least one of which opens a tab on install, e.g., https://chrome.google.com/webstore/detail/reload-all-tabs/midkcinmplflbiflboepnahkboeonkam).

Without this CL, this will result in the extension's tab opening on the desktop Chrome's window.

With this CL, the tab correctly opens in Ash as it was the last desktop focused.

Also make sure the regular use cases still work using the same test under:
A) Only Ash open
B) Ash open, but active desktop is native desktop.

Also confirmed using Sawbuck and some logging that the following is how OnWindowActivated() reports its state:
CoreWindowActivationState_CodeActivated == 0
 - First activation
 - Activated when hidden (either from Start Screen or Windows app switcher while viewing another app).

CoreWindowActivationState_Deactivated == 1
 - All deactivations (including focusing a desktop app on the 2nd monitor while Ash remains open on 1st monitor)

CoreWindowActivationState_PointerActivated
 - Ash is focused again after a window on another monitor had been focused while Ash stayed visible on its monitor.

R=cdn@chromium.org, grt@chromium.org, sky@chromium.org

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

[gab, 2013-10-11 21:21:13]

Carlos/Greg PTAL.

Thanks!
Gab

https://codereview.chromium.org/27058004/diff/1/win8/metro_driver/chrome_app_...
File win8/metro_driver/chrome_app_view_ash.cc (right):

https://codereview.chromium.org/27058004/diff/1/win8/metro_driver/chrome_app_...
win8/metro_driver/chrome_app_view_ash.cc:880: HRESULT
ChromeAppViewAsh::OnVisibilityChanged(
I decided to keep this implementation around just as OnWindowActivated() was
implemented before but not used. This can perhaps be useful in the future and
leaving it here is better documentation than whatever else I have seen regarding
Metro :)!

[cpu_(ooo_6.6-7.5), 2013-10-11 22:05:42]

https://codereview.chromium.org/27058004/diff/5001/win8/metro_driver/chrome_a...
File win8/metro_driver/chrome_app_view_ash.cc (left):

https://codereview.chromium.org/27058004/diff/5001/win8/metro_driver/chrome_a...
win8/metro_driver/chrome_app_view_ash.cc:888: ui_channel_->Send(new
MetroViewerHostMsg_VisibilityChanged(!!visible));
Lets put some text on ChromeAppViewAsh::OnVisibilityChanged stating that we
migrated to OnWindowActivated and that we used to send
MetroViewerHostMsg_VisibilityChanged, but we ran into problems blah blah.

https://codereview.chromium.org/27058004/diff/5001/win8/metro_driver/chrome_a...
File win8/metro_driver/chrome_app_view_ash.cc (right):

https://codereview.chromium.org/27058004/diff/5001/win8/metro_driver/chrome_a...
win8/metro_driver/chrome_app_view_ash.cc:904: }
so we don't need to know if we are deactivated? or non visible? I like not to
use GPU cycles if metro is not visible. It seems we want to pass a boolean to
the browser process.

[cpu_(ooo_6.6-7.5), 2013-10-11 22:06:42]

adding sky.

[gab, 2013-10-11 22:16:30]

https://codereview.chromium.org/27058004/diff/5001/win8/metro_driver/chrome_a...
File win8/metro_driver/chrome_app_view_ash.cc (left):

https://codereview.chromium.org/27058004/diff/5001/win8/metro_driver/chrome_a...
win8/metro_driver/chrome_app_view_ash.cc:888: ui_channel_->Send(new
MetroViewerHostMsg_VisibilityChanged(!!visible));
On 2013/10/11 22:05:42, cpu wrote:
> Lets put some text on ChromeAppViewAsh::OnVisibilityChanged stating that we
> migrated to OnWindowActivated and that we used to send
> MetroViewerHostMsg_VisibilityChanged, but we ran into problems blah blah.
> 

Sure, I didn't feel a comment was needed as I think its fairly explicit to a
reader that now sees both both OnVisibilityChanged() and OnWindowActivated()
which one is used for what.

But if you feel this is needed, I'll add one.

https://codereview.chromium.org/27058004/diff/5001/win8/metro_driver/chrome_a...
File win8/metro_driver/chrome_app_view_ash.cc (right):

https://codereview.chromium.org/27058004/diff/5001/win8/metro_driver/chrome_a...
win8/metro_driver/chrome_app_view_ash.cc:904: }
On 2013/10/11 22:05:42, cpu wrote:
> so we don't need to know if we are deactivated? or non visible? I like not to
> use GPU cycles if metro is not visible. It seems we want to pass a boolean to
> the browser process.
> 

A deactivation message (i.e. state == CoreWindowActivationState_Deactivated) is
also sent when Ash remains visible on another monitor, yet it's not the active
window.

I think you'd want to implement that in OnVisibilityChanged() if anything; that
really tells you when Ash is no longer visible.

[sky, 2013-10-11 22:21:53]

LGTM

[gab, 2013-10-15 03:12:44]

On 2013/10/11 22:21:53, sky wrote:
> LGTM

Thanks, grt/cpu?

[grt (UTC plus 2), 2013-10-15 17:23:26]

looks good. please delete some more code while you're at it. :-)

https://codereview.chromium.org/27058004/diff/16001/win8/metro_driver/chrome_...
File win8/metro_driver/chrome_app_view_ash.cc (right):

https://codereview.chromium.org/27058004/diff/16001/win8/metro_driver/chrome_...
win8/metro_driver/chrome_app_view_ash.cc:880: HRESULT
ChromeAppViewAsh::OnVisibilityChanged(
is there any reason not to remove this and visibility_changed_token_?

[gab, 2013-10-15 17:53:25]

https://codereview.chromium.org/27058004/diff/16001/win8/metro_driver/chrome_...
File win8/metro_driver/chrome_app_view_ash.cc (right):

https://codereview.chromium.org/27058004/diff/16001/win8/metro_driver/chrome_...
win8/metro_driver/chrome_app_view_ash.cc:880: HRESULT
ChromeAppViewAsh::OnVisibilityChanged(
On 2013/10/15 17:23:26, grt wrote:
> is there any reason not to remove this and visibility_changed_token_?

As I mentioned on the first patch set (should I state this explicitly in the CL
description):

I decided to keep this implementation around just as OnWindowActivated() was
implemented before but not used. This can perhaps be useful in the future and
leaving it here is better documentation than whatever else I have seen regarding
Metro :)!

Let me know if you still think strongly that this should go away.

[grt (UTC plus 2), 2013-10-15 19:13:19]

https://codereview.chromium.org/27058004/diff/16001/win8/metro_driver/chrome_...
File win8/metro_driver/chrome_app_view_ash.cc (right):

https://codereview.chromium.org/27058004/diff/16001/win8/metro_driver/chrome_...
win8/metro_driver/chrome_app_view_ash.cc:880: HRESULT
ChromeAppViewAsh::OnVisibilityChanged(
On 2013/10/15 17:53:26, gab wrote:
> On 2013/10/15 17:23:26, grt wrote:
> > is there any reason not to remove this and visibility_changed_token_?
> 
> As I mentioned on the first patch set (should I state this explicitly in the
CL
> description):
> 
> I decided to keep this implementation around just as OnWindowActivated() was
> implemented before but not used. This can perhaps be useful in the future and
> leaving it here is better documentation than whatever else I have seen
regarding
> Metro :)!
> 
> Let me know if you still think strongly that this should go away.

I feel strongly that we should not keep dead code. The OnWindowActivated
implementation did work when it was added by ananta in r196835. It looks to me
like sky forgot to remove it in r224521 when he removed the work it did.

[gab, 2013-10-15 19:18:40]

https://codereview.chromium.org/27058004/diff/16001/win8/metro_driver/chrome_...
File win8/metro_driver/chrome_app_view_ash.cc (right):

https://codereview.chromium.org/27058004/diff/16001/win8/metro_driver/chrome_...
win8/metro_driver/chrome_app_view_ash.cc:880: HRESULT
ChromeAppViewAsh::OnVisibilityChanged(
On 2013/10/15 19:13:19, grt wrote:
> On 2013/10/15 17:53:26, gab wrote:
> > On 2013/10/15 17:23:26, grt wrote:
> > > is there any reason not to remove this and visibility_changed_token_?
> > 
> > As I mentioned on the first patch set (should I state this explicitly in the
> CL
> > description):
> > 
> > I decided to keep this implementation around just as OnWindowActivated() was
> > implemented before but not used. This can perhaps be useful in the future
and
> > leaving it here is better documentation than whatever else I have seen
> regarding
> > Metro :)!
> > 
> > Let me know if you still think strongly that this should go away.
> 
> I feel strongly that we should not keep dead code. The OnWindowActivated
> implementation did work when it was added by ananta in r196835. It looks to me
> like sky forgot to remove it in r224521 when he removed the work it did.

Okay, done.

[grt (UTC plus 2), 2013-10-16 13:25:25]

lgtm

[gab, 2013-10-16 13:36:02]

On 2013/10/16 13:25:25, grt wrote:
> lgtm

Thanks, Carlos I assume it's good with you given sky's review?

CQ'ing.

[commit-bot: I haz the power, 2013-10-16 13:36:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/27058004/28001

[commit-bot: I haz the power, 2013-10-16 13:48:11]

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

[gab, 2013-10-16 14:57:27]

TBR: cdn for side-effect on ui/metro_viewer/metro_viewer_messages.h

[commit-bot: I haz the power, 2013-10-16 14:58:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/27058004/28001

[Cris Neckar, 2013-10-16 20:45:04]

IPC changes LGTM

I realize this is a trivial change but please don't TBR IPC audits.

[gab, 2013-10-16 20:46:27]

On 2013/10/16 20:45:04, Cris Neckar wrote:
> IPC changes LGTM
> 
> I realize this is a trivial change but please don't TBR IPC audits.

Ok my bad, will make sure to not do so in the future.

Thanks!
Gab

[commit-bot: I haz the power, 2013-10-16 20:47:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/27058004/28001

[commit-bot: I haz the power, 2013-10-16 21:46:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/27058004/28001

[commit-bot: I haz the power, 2013-10-17 02:31:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/27058004/28001

[commit-bot: I haz the power, 2013-10-17 06:31:26]

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

[commit-bot: I haz the power, 2013-10-17 19:37:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/27058004/28001

[commit-bot: I haz the power, 2013-10-18 01:33:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/27058004/28001

[commit-bot: I haz the power, 2013-10-18 02:15:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/27058004/28001

[commit-bot: I haz the power, 2013-10-18 15:07:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/27058004/150001

[gab, 2013-10-18 15:08:20]

Committed patchset #5 manually as r229420 (presubmit successful).