920013 - Rewrite test_cocoa_focus.html to use SpecialPowers

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Martijn Wargers (dead)]

Attachment: cocoa.diff

I tried to fix this, but I couldn't test this locally, because I would then run into bug 918732.
I've attached the fix that I was trying.
This gave on tryserver the following errors:
79 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/plugins/test/mochitest/test_cocoa_focus.html | Plugin should not have focus. - got true, expected false
80 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/plugins/test/mochitest/test_cocoa_focus.html | Focus event count should be 4 - got 3, expected 4

Comment 1

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 809160 [details] [diff] [review]
cocoa.diff

Review of attachment 809160 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/plugins/test/mochitest/cocoa_focus.html
@@ +97,1 @@
>  

window.blur() removes the focus, while i'd expect SpecialPowers.focus() to do the opposite.

Comment 2

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #1)
> window.blur() removes the focus, while i'd expect SpecialPowers.focus() to
> do the opposite.

Never mind, i missed that this is intended to focus() another window.

Comment 3

[Martijn Wargers (dead)]

(In reply to Georg Fritzsche [:gfritzsche] from comment #2)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #1)
> > window.blur() removes the focus, while i'd expect SpecialPowers.focus() to
> > do the opposite.
> 
> Never mind, i missed that this is intended to focus() another window.

Yes, I did that, because there is no SpecialPowers.blur(). But focusing another window should have the same effect.

Comment 4

[Georg Fritzsche [:gfritzsche]]

Ok, so if i'm not missing something again: Why are you replacing only one (the second) occurence of window.blur() in cocoa_focus.html? I assume if you replace the first one it fails there already?

The test as is depends on the focus/blur having affected the window states when they return. I think with the things SpecialPowers does this doesn't hold anymore.
Maybe you just need to wait for the focus/blur event to happen? Maybe you can even confirm this in a simpler, plugin-less, test-case?

Comment 5

[Martijn Wargers (dead)]

(In reply to Georg Fritzsche [:gfritzsche] from comment #4)
> Ok, so if i'm not missing something again: Why are you replacing only one
> (the second) occurence of window.blur() in cocoa_focus.html? I assume if you
> replace the first one it fails there already?

Oops, sorry, I forgot to replace the first occurence, I should have done that. I guess that might be the reason for the failures on try server.

Comment 6

[Martijn Wargers (dead)]

Another issue might be that dom.disable_window_flip pref is set to true:
http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js#489
I guess with enablePrivilege set, scripts can lower or raise windows as they like.
With enablePrivilege removed, that pref should be set to false, temporarily, perhaps.

Comment 7

[Martijn Wargers (dead)]

Attachment: 920013.diff

Pushed this to try: https://tbpl.mozilla.org/?tree=Try&rev=84f031582b74

Comment 8

[Martijn Wargers (dead)]

Attachment: 920013.diff

Previous patch still gave failures. This is another try.

Comment 9

[Martijn Wargers (dead)]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=bf5256b5bfff

Comment 10

[Martijn Wargers (dead)]

That didn't work either.

Comment 11

[Martijn Wargers (dead)]

Attachment: 920013.diff

I was able to not getting stuck with bug 918732, by adding plugin1.focus() after the sendNativeMouseEvent calls.
I'm not sure if that's allowed. I guess it's not really necessary, because on try and tbpl, bug 918732 doesn't seem to occur.

Anyway, with those plugin1.focus() calls, I was able to fix the failures, which results in this patch.

Comment 12

[Steven Michaud [:smichaud] (Retired)]

I know I originally wrote a large chunk of these tests.  But I need to dig back into whatever documentation still exists (in the relevant bugs), plus whatever memory I can trigger of the issues I faced when I wrote them, before I'll understand the issues here.

I especially need to recover why I used the particular combination of cross-platform and native events in these tests.

I should be able to get to this sometime next week.

Comment 13

[Martijn Wargers (dead)]

Steven, did you take a look at this?

Comment 14

[Steven Michaud [:smichaud] (Retired)]

> Steven, did you take a look at this?

It's been on my list for some time ... but it keeps getting bumped by more urgent things.

No, I haven't forgotten it, and I hope to get to it in the next couple of weeks.

Comment 15

[Martijn Wargers (dead)]

Sorry, I hope I don't annoy you, but if you could take a look at this, at some point, that would be great. There is no rush to it, though.

Comment 16

[Steven Michaud [:smichaud] (Retired)]

Sorry Martijn :-(

This bug has been on my list for months now, but keeps getting bumped by other things.

Comment 17

[Martijn Wargers (dead)]

There was some talk about removal of enablePrivilege usage, lately, in bug 984012. This is one of the few tests that still use it.

Comment 18

[Steven Michaud [:smichaud] (Retired)]

Is it possible to call DOMWindowUtils.screenPixelsPerCSSPixel and DOMWindowUtils.sendNativeMouseEvent() without special privileges?  The test won't work without those.

Also, as best I can tell, this test works fine since my changes to it in bug 1117027.  Is that right?

Comment 19

[Steven Michaud [:smichaud] (Retired)]

Oops, I think I may have misunderstood your question, Martijn.  Are you referring to the following line?  And are you asking whether it's possible for us to remove it?

https://hg.mozilla.org/mozilla-central/annotate/b2e71f32548f/dom/plugins/test/mochitest/cocoa_focus.html#l18

If so, I'm afraid the answer is "I don't know".

Comment 20

[Steven Michaud [:smichaud] (Retired)]

> I'm afraid the answer is "I don't know".

But I'm willing to try it out, if that's what you're after.

Comment 21

[Martijn Wargers (dead)]

Attachment: 920013.diff

Ok, I'm not suffering from bug 918732 anymore, it seems, I was able to run this test locally with this patch applied.
The specialpowers.focus stuff is needed, otherwise the test will fail.

Comment 22

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8580744 [details] [diff] [review]
920013.diff

+      plugin1.focus();

+      plugin2.focus();

These defeat the whole purpose of the test, as best I can tell.  The test is to tell whether or not the synthesized mouse events focus the appropriate plugin.

Comment 23

[Martijn Wargers (dead)]

Attachment: 920013.diff

Ok, they don't seem to be necessary in order for the mochitest to pass, anyway.

Comment 24

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8580791 [details] [diff] [review]
920013.diff

This looks fine.  But I want to run it through the tryservers, just to be sure:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a58cf7c90163

Comment 25

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8580791 [details] [diff] [review]
920013.diff

The tryserver tests finished (finally), and they went fine.

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5d1278ee01b

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b5d1278ee01b