keypress should make sure the window is focused

RESOLVED INVALID

Status

RESOLVED INVALID
9 years ago
8 years ago

People

(Reporter: asuth, Assigned: asuth)

Tracking

Trunk
All
Linux

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

9 years ago
Created attachment 428618 [details] [diff] [review]
v1 wrap keypress, force focus

This wraps controller.keypress so that we spin an event loop until we get the focus.  Or failing that, we timeout.

In conjunction with using Xvnc this makes a lot of angry tests on 1.9.3 happy for me.

This patch fixing things probably assumes that:
1) The window manager is not fighting our request to focus a window.
2) The user is not moving their mouse around fighting the unit test.

I believe this will likely also fix an intermittent 1.9.2 issue I see.

I'm going to push the patch and see what it does on an NPOTB and only-the-buildbots-know-what-fixes-the-buildbots basis.  I'll back it out if things get more orange, but failing that, I think this is an appropriate candidate for post-commit review.
Attachment #428618 - Flags: review?(bugzilla)
(Assignee)

Comment 1

9 years ago
Dang.  comm-central went (as reported by the tinderbox UI) from 13 to 3.  But comm-1.9.2 went from 0 to 8 because it claims it can't actually focus the window sometimes.  I'm going to try and make them do another run for another data point before backing out.
(Assignee)

Comment 2

9 years ago
Comment on attachment 428618 [details] [diff] [review]
v1 wrap keypress, force focus

(r=sid0 over irc, although I am going to need to back out unless I can figure out why 1.9.2 is unhappy)

<sid0> asuth: r+
<sid0> asuth: though I would have preferred a waitForEval loop, since that's what we've been using everywhere
<asuth> sid0: yeah, my goal is not to have a thousand custom event loop spinners, but I wanted to try and event-driven idiom
<asuth> since that's arguably much more efficient
Attachment #428618 - Flags: review?(bugzilla) → review+
(Assignee)

Comment 3

9 years ago
initial landing:
http://hg.mozilla.org/comm-central/rev/b93800b7ee50

whitespace change to trigger another test build:
http://hg.mozilla.org/comm-central/rev/5adcb0569c2a

additional debug:
http://hg.mozilla.org/comm-central/rev/91ec97a51e5f

1.9.2 unhappiness with extra debug log; unfortunately, the default string reps don't include pointers so we can only tell null from non-null:
http://tinderbox.mozilla.org/showlog.cgi?log=ThunderbirdTest/1266996426.1266997355.25296.gz

The two characteristic examples of that:
Failed to focus window! Initially active window: [object ChromeWindow] currently active window: [object ChromeWindow]
Failed to focus window! Initially active window: null currently active window: null



backout:
http://hg.mozilla.org/comm-central/rev/542db8f9ca0a


I'm not really clear what's up.  Possibilites that occur:

- Window manager problems; it's refusing our requests or something else is interfering.  This would explain the activeWindow == null case.

- The comparison is failing because our equivalence test is too strict.  Which is to say, perhaps we're trying to tell it an inner window instead of an outer window or we're comparing different QI's or something.  I'm not clear on the XPConnect/JS semantics for things like this.
(Assignee)

Comment 4

9 years ago
Probably a good next step for investigation would be to make the debug:
1) Be able to indicate specific instances through pointer unique identification or something like that.
2) Dumping the window we want to be switching to as well.

Ideally the problem might be reproducible locally with 1.9.2...
Andrew, do we still want this now that we've got the Linux builders running?
(Assignee)

Comment 6

8 years ago
I think bug 640877 may be related to this; thanks for drawing my attention to this bug.  This patch gives me a good idea for additional instrumentation to track activation which will help figure out if we need this patch.  (I don't think the focus manager will tell us directly, so we need to register for this cool activate event...)  I'll pursue on the try-server.
(Assignee)

Comment 7

8 years ago
I'm marking this invalid because this was mainly a stop-gap hack that I think I will have successfully dealt with on bug 640877.  Keypresses, unlike mouse movements/clicks (depending on platform/config), do not in-and-of-themselves cause focus changes, so this was not a realistic thing to be doing.  (Obviously, alt-tab/tab can change focus, but those depend on the existing global/window focus with explicit semantics.)
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.