Handle _NET_ACTIVE_WINDOW or FocusIn & FocusOut, not both.

Handle _NET_ACTIVE_WINDOW or FocusIn & FocusOut, not both.

When WM supports _NET_ACTIVE_WINDOW notification, we get OnActiveWindowChanged() from two places:
 - X11DesktopHandler::ProcessXEvent (FocusIn)
 - X11DesktopHandler::DispatchEvent (PropertyNotify)

One is enough, two can lead to confusion. FocusIn/Out should not be handled if WM supports active window.

BUG=None
TEST=None

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

[joleksy, 2014-09-09 08:51:59]

joleksy@opera.com changed reviewers:
+ erg@chromium.org

[joleksy, 2014-09-09 08:52:19]

@reviewer: does it look ok to you?

[Elliot Glaysher, 2014-09-09 17:26:04]

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

[Elliot Glaysher, 2014-09-09 17:26:05]

pkotwicz: you were the last person to touch focus here. Does this look correct
to you?

[pkotwicz, 2014-09-09 17:41:08]

This looks good to me.

jolesky@ can you file a bug as to what does not work due to the "confusion"?

[joleksy, 2014-09-10 06:48:10]

On 2014/09/09 17:41:08, pkotwicz wrote:
> This looks good to me.
> 
> jolesky@ can you file a bug as to what does not work due to the "confusion"?

The problem is that I could not reproduce the issue on Chrome, that is why I did
not file a bug.

What happened with Opera with two app windows opened and switching focus:
1. Window 1 receives focus, it sets itself as active by sending
_NET_ACTIVE_WINDOW 
2. Window 2 receives focus (before Window 1 gets notification back from WM!) and
sets itself as active by sending _NET_ACTIVE_WINDOW 
3. Window 1 receives activation notification and WM automatically sets focus to
Window 1, which in response sends _NET_ACTIVE_WINDOW 
4. Window 2 receives activation notification and WM automatically sets focus to
Window 2, which in response sends _NET_ACTIVE_WINDOW 
5. Window 1 receives activation notification and WM automatically sets focus to
Window 1, which in response sends _NET_ACTIVE_WINDOW 
...and so on

This is most probably a nasty timing issue. I can open a theoretical bug for
Chrome;-)

[pkotwicz, 2014-09-10 15:40:57]

I think your fix is correct.

One more clarification question. I must be missing something. In which case does
receiving FocusIn cause us to call into X11DesktopHandler::ActivateWindow() (I
think that you are saying that we do this in step #3)

[joleksy, 2014-09-11 06:41:58]

On 2014/09/10 15:40:57, pkotwicz wrote:
> I think your fix is correct.
> 
> One more clarification question. I must be missing something. In which case
does
> receiving FocusIn cause us to call into X11DesktopHandler::ActivateWindow() (I
> think that you are saying that we do this in step #3)

Well, I tried to reproduce the problem and it seems that it is gone. Looks to me
like to code has changed (the patch is actually quite old) and it is no longer
possible for ActivateWindow() to be called after receiving FocusIn.

Still, it seems to me that the change is logical.

[pkotwicz, 2014-09-11 14:27:49]

LGTM Thanks for the CL!

[joleksy, 2014-09-11 14:35:30]

The CQ bit was checked by joleksy@opera.com

[commit-bot: I haz the power, 2014-09-11 14:36:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/549713003/1

[commit-bot: I haz the power, 2014-09-11 14:43:14]

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

[commit-bot: I haz the power, 2014-09-11 14:43:15]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[joleksy, 2014-09-11 14:47:12]

On 2014/09/11 14:43:15, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

It says:

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    ui/views/widget/desktop_aura/x11_desktop_handler.cc

[pkotwicz, 2014-09-11 15:17:38]

Yes, you still need to get approval from erg@

[Elliot Glaysher, 2014-09-11 17:28:31]

lgtm

[joleksy, 2014-09-12 06:22:08]

The CQ bit was checked by joleksy@opera.com

[commit-bot: I haz the power, 2014-09-12 06:23:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/549713003/1

[commit-bot: I haz the power, 2014-09-12 06:37:15]

Committed patchset #1 (id:1) as 3e4271951257a60af71427f6381b2a19304b9e09

[commit-bot: I haz the power, 2014-09-12 06:39:14]

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