Closed Bug 920013 Opened 6 years ago Closed 5 years ago

Rewrite test_cocoa_focus.html to use SpecialPowers

Categories

(Core :: Plug-ins, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: martijn.martijn, Assigned: martijn.martijn)

References

(Blocks 1 open bug, )

Details

Attachments

(1 file, 5 obsolete files)

Attached patch cocoa.diff (obsolete) — Splinter Review
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 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.
(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.
(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.
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?
(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.
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.
Attached patch 920013.diff (obsolete) — Splinter Review
Pushed this to try: https://tbpl.mozilla.org/?tree=Try&rev=84f031582b74
Attachment #809160 - Attachment is obsolete: true
Attached patch 920013.diff (obsolete) — Splinter Review
Previous patch still gave failures. This is another try.
Attachment #810027 - Attachment is obsolete: true
That didn't work either.
Attached patch 920013.diff (obsolete) — Splinter Review
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.
Attachment #810468 - Attachment is obsolete: true
Attachment #811387 - Flags: review?(smichaud)
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.
Steven, did you take a look at this?
Flags: needinfo?(smichaud)
> 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.
Flags: needinfo?(smichaud)
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.
Flags: needinfo?(smichaud)
Sorry Martijn :-(

This bug has been on my list for months now, but keeps getting bumped by other things.
Flags: needinfo?(smichaud)
There was some talk about removal of enablePrivilege usage, lately, in bug 984012. This is one of the few tests that still use it.
Blocks: 984012
Flags: needinfo?(smichaud)
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?
Flags: needinfo?(smichaud) → needinfo?(martijn.martijn)
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".
> 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.
Attached patch 920013.diff (obsolete) — Splinter Review
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.
Attachment #811387 - Attachment is obsolete: true
Attachment #811387 - Flags: review?(smichaud)
Flags: needinfo?(martijn.martijn)
Attachment #8580744 - Flags: review?(smichaud)
Assignee: nobody → martijn.martijn
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.
Attached patch 920013.diffSplinter Review
Ok, they don't seem to be necessary in order for the mochitest to pass, anyway.
Attachment #8580744 - Attachment is obsolete: true
Attachment #8580744 - Flags: review?(smichaud)
Attachment #8580791 - Flags: review?(smichaud)
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 on attachment 8580791 [details] [diff] [review]
920013.diff

The tryserver tests finished (finally), and they went fine.
Attachment #8580791 - Flags: review?(smichaud) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b5d1278ee01b
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.