Bug 93673 - NRWT: Make it possible to run layout tests on Retina MBP
Summary: NRWT: Make it possible to run layout tests on Retina MBP
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-09 18:08 PDT by Ryosuke Niwa
Modified: 2013-03-22 13:38 PDT (History)
12 users (show)

See Also:


Attachments
work in progress (10.57 KB, patch)
2012-08-09 18:12 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
patch to make DRT+WKTR windows 1x (3.15 KB, patch)
2013-03-22 04:33 PDT, Tim Horton
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2012-08-09 18:08:16 PDT
We need a separate baseline on machines with deviceScaleFactor > 1 such as Retina MacBook Pro.
Comment 1 Ryosuke Niwa 2012-08-09 18:11:35 PDT
A big question is how we're going to support both WK2 and high resolution as the same time.

Say we're on Retina MBP with Lion and running tests under WK1. Fallback path will probably look like:

mac-lion-highresolution > mac-lion > ... > mac

But what if we were running tests under WK2?

mac-lion-highresolution-wk2 > mac-lion-highresolution > mac-lion > ... > mac?
or mac-lion-wk2-highresolution > mac-lion-wk2 > mac-lion > ... > mac?
or something crazy like mac-lion-wk2-highresolution > mac-highresolution > mac-lion-wk2 > mac-lion > ... > mac?
Comment 2 Ryosuke Niwa 2012-08-09 18:12:51 PDT
Created attachment 157603 [details]
work in progress
Comment 3 Nico Weber 2012-08-09 18:34:34 PDT
See also bug 93647 (no patch)
Comment 4 Ryosuke Niwa 2012-08-09 19:53:48 PDT
What is the canonical name for these sorts of displays? "high resolution" and "high DPI" are not good descriptions because they just mean that screen size is big when measured in device pixels. It's the scaling factor that's unique.

Maybe "scaled"?
Comment 5 Dirk Pranke 2012-08-09 19:58:16 PDT
(In reply to comment #4)
> What is the canonical name for these sorts of displays? "high resolution" and "high DPI" are not good descriptions because they just mean that screen size is big when measured in device pixels. It's the scaling factor that's unique.
> 
> Maybe "scaled"?

This may be beside the point, since I'm not sure that a different implementation of the display would have the same characteristics or matter ... maybe we should just call it "retina" ?
Comment 6 Nico Weber 2012-08-09 19:59:46 PDT
On Windows, we might end up with 1.4 and 1.8 scale factors for metro as opposed to 2x, so just "retina" might not be a good idea. Maybe "scaled-200"?
Comment 7 Ryosuke Niwa 2012-08-09 20:07:04 PDT
As I understand it, Retina is Apple's trademark and I'd rather not take a risk of using that term.

Also, I really hope we don't have to support multiple scaling factors on our tests.
Comment 8 Terry Anderson 2012-08-10 10:39:51 PDT
(In reply to comment #3)
> See also bug 93647 (no patch)

Can you please clarify the relationship between the two bugs? AFAICT, I still need to fix bug 93647 to ensure that bug 90192 can land (i.e., to check the correctness of high dpi pixel tests in chromium for both the software and hardware paths).
Comment 9 Beth Dakin 2012-08-10 11:01:38 PDT
(In reply to comment #8)
> (In reply to comment #3)
> > See also bug 93647 (no patch)
> 
> Can you please clarify the relationship between the two bugs? AFAICT, I still need to fix bug 93647 to ensure that bug 90192 can land (i.e., to check the correctness of high dpi pixel tests in chromium for both the software and hardware paths).


If my understanding is correct, bug 93647 is about making the tests in fast/hidpi work. Those tests use WTR/DRT to simulate HiDPI mode on ANY device, so the results are expected to always be in 2x whether they are run on a 1x device, a 2x device, or a magical 16x device! 

This bug is about running ALL of the tests on a retina mac, which is a 2x device. There are many tests in other directories that have results checked in that expect a 1x device, so they fail on a 2x machine. See these bugs to get an idea of the kind of tests that require a platform/retina or platform/2x directory:

https://bugs.webkit.org/show_bug.cgi?id=89709
https://bugs.webkit.org/show_bug.cgi?id=89711
https://bugs.webkit.org/show_bug.cgi?id=89713
https://bugs.webkit.org/show_bug.cgi?id=89715
Comment 10 Ryosuke Niwa 2012-08-10 11:10:48 PDT
Not only that but a lot of pixel tests baselines for Chromium Mac don't match :(
Comment 11 Dirk Pranke 2012-08-10 11:33:10 PDT
(In reply to comment #7)
> As I understand it, Retina is Apple's trademark and I'd rather not take a risk of using that term.

Good point.

> Also, I really hope we don't have to support multiple scaling factors on our tests.

Yes, that would be nice, but so far I'm not optimistic :)

Do we know what is failling when deviceScaleFactor > 1? Is it anything we could potentially work around or fuzz for in the test? I assume it's mostly differences due to interpolating things differently, and maybe we could do a fuzzy image compare for that?
Comment 12 Philip Rogers 2012-08-10 11:38:15 PDT
(In reply to comment #11)
> (In reply to comment #7)
> > As I understand it, Retina is Apple's trademark and I'd rather not take a risk of using that term.
> 
> Good point.
> 
> > Also, I really hope we don't have to support multiple scaling factors on our tests.
> 
> Yes, that would be nice, but so far I'm not optimistic :)
> 
> Do we know what is failling when deviceScaleFactor > 1? Is it anything we could potentially work around or fuzz for in the test? I assume it's mostly differences due to interpolating things differently, and maybe we could do a fuzzy image compare for that?

Primarily font aliasing but there are also scrollbar differences (where scrollbars appear now, but they didn't before). I don't think we could get away with fuzzy image compare, sadly.
Comment 13 Beth Dakin 2012-08-10 11:41:21 PDT
(In reply to comment #11)
> (In reply to comment #7)
> > As I understand it, Retina is Apple's trademark and I'd rather not take a risk of using that term.
> 
> Good point.
> 
> > Also, I really hope we don't have to support multiple scaling factors on our tests.
> 
> Yes, that would be nice, but so far I'm not optimistic :)
> 
> Do we know what is failling when deviceScaleFactor > 1? Is it anything we could potentially work around or fuzz for in the test? I assume it's mostly differences due to interpolating things differently, and maybe we could do a fuzzy image compare for that?

If you look at the links I pasted into comment 9, you will see a number of things that are failing. I think there is a legitimate need for a platform directory here because not all of the failures represent bugs. For example, there is no way to test media queries that query device-pixel-ratio without having different expectations for 1x and 2x. There are also a lot of legitimate differences with <canvas> when you are on a  1x versus a 2x device, and the only way to test those is again allowing for different expectations.
Comment 14 Tim Horton 2013-03-22 04:33:26 PDT
Created attachment 194513 [details]
patch to make DRT+WKTR windows 1x

Attaching a hacky patch that implements an alternative solution: running the tests at 1x always. I realize we probably want to test 2x in the future, but if we want a quick fix that makes existing test results match on Retina hardware, we could do this...
Comment 15 Tim Horton 2013-03-22 04:34:06 PDT
Comment on attachment 194513 [details]
patch to make DRT+WKTR windows 1x

View in context: https://bugs.webkit.org/attachment.cgi?id=194513&action=review

> Tools/WebKitTestRunner/mac/PlatformWebViewMac.mm:131
> -    NSRect windowRect = NSOffsetRect(rect, -10000, [(NSScreen *)[[NSScreen screens] objectAtIndex:0] frame].size.height - rect.size.height + 10000);
> +    NSRect windowRect = NSOffsetRect(rect, 0, [(NSScreen *)[[NSScreen screens] objectAtIndex:0] frame].size.height - rect.size.height);

Pretend this part isn't here, it was just for testing.
Comment 16 Ryosuke Niwa 2013-03-22 09:00:11 PDT
(In reply to comment #14)
> Created an attachment (id=194513) [details]
> patch to make DRT+WKTR windows 1x
> 
> Attaching a hacky patch that implements an alternative solution: running the tests at 1x always. I realize we probably want to test 2x in the future, but if we want a quick fix that makes existing test results match on Retina hardware, we could do this...

Nice! Do all tests pass with this change? Or do some tests still fail? If some of them do fail, then we probably want to do this and what I proposed earlier so that we may check in -expected.txt as needed.
Comment 17 Ryosuke Niwa 2013-03-22 11:09:42 PDT
Nice! It seems like all the tests pass.
Comment 18 Levi Weintraub 2013-03-22 11:10:09 PDT
Awesome!!!
Comment 19 Ryosuke Niwa 2013-03-22 11:17:52 PDT
Comment on attachment 194513 [details]
patch to make DRT+WKTR windows 1x

Tim, could you add a change log to this? r=me with that and reverting the change you mentioned were only for testing.
Comment 20 Beth Dakin 2013-03-22 11:43:10 PDT
Hopefully this patch does not break testRunner.setBackingScaleFactor(). That's a mode that I added a while ago to test 2X from a 1X machine. The tests that use it are in fast/hidpi.
Comment 21 Ryosuke Niwa 2013-03-22 11:47:29 PDT
(In reply to comment #20)
> Hopefully this patch does not break testRunner.setBackingScaleFactor(). That's a mode that I added a while ago to test 2X from a 1X machine. The tests that use it are in fast/hidpi.

I’ve ran it locally and they seem to pass still (I seem to have some color profile isssues thouh might be Lion vs. ML difference)
Comment 22 Beth Dakin 2013-03-22 11:48:37 PDT
(In reply to comment #21)
> (In reply to comment #20)
> > Hopefully this patch does not break testRunner.setBackingScaleFactor(). That's a mode that I added a while ago to test 2X from a 1X machine. The tests that use it are in fast/hidpi.
> 
> I’ve ran it locally and they seem to pass still (I seem to have some color profile isssues thouh might be Lion vs. ML difference)
 
Cool! (Well, the colorspace issue is not cool, but I'm glad that generally the mode works.)
Comment 23 Tim Horton 2013-03-22 12:52:01 PDT
(In reply to comment #19)
> (From update of attachment 194513 [details])
> Tim, could you add a change log to this? r=me with that and reverting the change you mentioned were only for testing.

Will do, shortly. Glad that it helps :D
Comment 24 Tim Horton 2013-03-22 13:38:05 PDT
http://trac.webkit.org/changeset/146650

Tests will now run at 1x on all devices (for the Mac port).