886624 - Defect - Tests fail on HiDPI systems

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Tests, defect, P2)

Description

[Jonathan Wilde [:jwilde]]

As discussed with jimm, mochitests currently fail on HiDPI systems due to issues with nsIDOMWindowUtils. The Mozilla test slaves currently don't test HiDPI. We'll need to have that eventually. The solution for this will take place in two parts.

Part 1:
- apply SpecialPowers to set prefs such that

Part 2:
- modify nsIDOMWindowUtils to honor HiDPI settings
- [set up test slaves that can run HiDPI tests]

Comment 1

[Jonathan Wilde [:jwilde]]

(In reply to Jonathan Wilde [:jwilde] from comment #0)
> Part 1:
> - apply SpecialPowers to set prefs such that

*such that the tests currently broken on HiDPI displays will have the `layout.css.devPixelsPerPx` pref set to `1.0`.

This SpecialPowers code should be removed when Part 2 lands.

Comment 2

[Jonathan Wilde [:jwilde]]

Attachment: patch for part 1, v1

Comment 3

[Marco Mucci [:MarcoM]]

Hi Jonathan, what point value do you estimate for this Defect?

Comment 4

[Jonathan Wilde [:jwilde]]

I don't really have the expertise to complete both parts of the bug (hrm, maybe we should split this?), but I could see this potentially being 20, given the large swath of Mozilla testing infrastructure that'd get affected by the part 2 changes.

Part 1 is a p=1, though.

Comment 5

[Marco Mucci [:MarcoM]]

Thanks Jonathan.  Yes, we'll deal with just Part 1 for now as you've described.

Comment 6

[Jim Mathies [:jimm]]

(In reply to Jonathan Wilde [:jwilde] from comment #4)
> I don't really have the expertise to complete both parts of the bug (hrm,
> maybe we should split this?), but I could see this potentially being 20,
> given the large swath of Mozilla testing infrastructure that'd get affected
> by the part 2 changes.
> 
> Part 1 is a p=1, though.

part 2 should be a different bug filed in Core/Testing. It's probably been discussed as well, may already be filed.

Comment 7

[Jim Mathies [:jimm]]

Comment on attachment 767004 [details] [diff] [review]
patch for part 1, v1

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

::: browser/metro/base/tests/mochitest/head.js
@@ +28,5 @@
>  }
>  
> +function setDevPixelEqualToPx()
> +{
> +  todo(false, "browser_selection_frame needs devPixelsPerPx set to 1.0");

why the todo for browser_selection_frame?

Comment 8

[Jim Mathies [:jimm]]

test issues: bug 859742

Comment 9

[Jonathan Wilde [:jwilde]]

(In reply to Jim Mathies [:jimm] from comment #7)
> Comment on attachment 767004 [details] [diff] [review]
> patch for part 1, v1
> 
> Review of attachment 767004 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/metro/base/tests/mochitest/head.js
> @@ +28,5 @@
> >  }
> >  
> > +function setDevPixelEqualToPx()
> > +{
> > +  todo(false, "browser_selection_frame needs devPixelsPerPx set to 1.0");
> 
> why the todo for browser_selection_frame?

he todo was intended to help track in the logs where we're applying this. That really should be a more generic todo message like "testcase sets devPixelsPerPx to 1.0", though.

Comment 10

[Jonathan Wilde [:jwilde]]

(In reply to Jim Mathies [:jimm] from comment #8)
> test issues: bug 859742

Sweet. I'll update the title on here and drop the leaveopen.

Comment 11

[Jonathan Wilde [:jwilde]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/61102ea334c7

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/61102ea334c7

Comment 13

[juan becerra [:juanb]]

Jonathan, are these tests passing now, for part 1?

Comment 14

[Jonathan Wilde [:jwilde]]

Yes, they are. :D

Comment 15

[Jonathan Wilde [:jwilde]]

Though, I haven't developing/running tests on a HiDPI machine in a couple weeks, so I should probably double check that's still the case.

Comment 16

[Jonathan Wilde [:jwilde]]

Just compiled m-c and ran all tests on a device that's HiDPI. Everything passed.

Comment 17

[Samvedana (:Samvedana)]

Can any one provide steps to test this?

Comment 18

[Samvedana (:Samvedana)]

Hi Jim, Can you please provide steps to test this?

Comment 19

[Jim Mathies [:jimm]]

(In reply to Samvedana (:Samvedana) from comment #18)
> Hi Jim, Can you please provide steps to test this?

This is an automated tests change, if our tests are succeeding on tbpl (which they are), this bug is verified.

Comment 20

[Jim Mathies [:jimm]]

Marco if this is on your testing spreadsheet, it can be removed. Nothing to test here in the app.

Comment 21

[Marco Mucci [:MarcoM]]

Thanks Jim.  Makes sense.  I'll remove it.

Comment 22

[Jonathan Wilde [:jwilde]]

Sorry for missing this. (I didn't have "Include Closed" checked in "My Dashboard" on Bugzilla.)

Do we have HiDPI test slaves for TBPL now?

The (possibly outdated) issue I recall from a while back was that the TBPL machines defaulted to devPixelsPerPx = 1.0, so the test suite could be green there, but still fail on a device with a higher pixel density.

Comment 23

[Jim Mathies [:jimm]]

(In reply to Jonathan Wilde [:jwilde] from comment #22)
> Sorry for missing this. (I didn't have "Include Closed" checked in "My
> Dashboard" on Bugzilla.)
> 
> Do we have HiDPI test slaves for TBPL now?

Not yet, bug 859742 was filed on running tests at higher values.