Defect - Tests fail on HiDPI systems

RESOLVED FIXED in Firefox 25

Status

Firefox for Metro
Tests
P2
normal
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jwilde, Assigned: jwilde)

Tracking

unspecified
Firefox 25
x86
Windows 8.1
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: feature=defect c=tbd u=tbd p=1)

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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]
(Assignee)

Comment 1

5 years ago
(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.

Updated

5 years ago
Blocks: 859003
Whiteboard: [leave open] → [leave open] feature=defect c=tbd u=tbd p=0
(Assignee)

Updated

5 years ago
Summary: Defect - Tests fail on HiDPI systems → Defect - Tests not supported on HiDPI systems
(Assignee)

Comment 2

5 years ago
Created attachment 767004 [details] [diff] [review]
patch for part 1, v1
(Assignee)

Updated

5 years ago
Attachment #767004 - Flags: review?(jmathies)
Hi Jonathan, what point value do you estimate for this Defect?
Flags: needinfo?(jwilde)
(Assignee)

Comment 4

5 years ago
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.
Flags: needinfo?(jwilde)
Thanks Jonathan.  Yes, we'll deal with just Part 1 for now as you've described.

Updated

5 years ago
Whiteboard: [leave open] feature=defect c=tbd u=tbd p=0 → [leave open] feature=defect c=tbd u=tbd p=1

Comment 6

5 years ago
(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

5 years ago
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?
Attachment #767004 - Flags: review?(jmathies) → review+

Comment 8

5 years ago
test issues: bug 859742
(Assignee)

Comment 9

5 years ago
(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.
(Assignee)

Comment 10

5 years ago
(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.
Summary: Defect - Tests not supported on HiDPI systems → Defect - Tests fail on HiDPI systems
Whiteboard: [leave open] feature=defect c=tbd u=tbd p=1 → feature=defect c=tbd u=tbd p=1

Updated

5 years ago
Blocks: 882638
No longer blocks: 859003
QA Contact: jbecerra

Updated

5 years ago
Priority: -- → P2
https://hg.mozilla.org/mozilla-central/rev/61102ea334c7
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
Jonathan, are these tests passing now, for part 1?
Flags: needinfo?(jwilde)
(Assignee)

Comment 14

5 years ago
Yes, they are. :D
Flags: needinfo?(jwilde)
(Assignee)

Comment 15

5 years ago
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.
(Assignee)

Comment 16

5 years ago
Just compiled m-c and ran all tests on a device that's HiDPI. Everything passed.
Can any one provide steps to test this?
Flags: needinfo?(jwilde)
Hi Jim, Can you please provide steps to test this?
Flags: needinfo?(jmathies)

Comment 19

5 years ago
(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.
Flags: needinfo?(jmathies)

Updated

5 years ago
Flags: needinfo?(jwilde)

Comment 20

5 years ago
Marco if this is on your testing spreadsheet, it can be removed. Nothing to test here in the app.
Flags: needinfo?(mmucci)
Thanks Jim.  Makes sense.  I'll remove it.
Flags: needinfo?(mmucci)
(Assignee)

Comment 22

5 years ago
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

5 years ago
(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.
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.