Bug 173319 – Configure screen scale for running layout tests on plus devices

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jonathan Bedard]

Layout tests expect the screen to have a scale of 2.  Explicitly set this scale for iOS devices and simulators so that tests can be run on plus devices.

[Radar WebKit Bug Importer]

<rdar://problem/32740521>

[Jonathan Bedard]

Created attachment 312778 [details]
Patch

[Jonathan Bedard]

I'd like to see iso-sim EWS come back clean before landing this.

[Jonathan Bedard]

Created attachment 312786 [details]
Patch

[Jonathan Bedard]

Created attachment 312788 [details]
Patch

[Jonathan Bedard]

Created attachment 312794 [details]
Patch

[Build Bot]

Attachment 312794 [details] did not pass style-queue:


ERROR: Tools/TestRunnerShared/spi/UIKitTestSPI.h:39:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/TestRunnerShared/spi/UIKitTestSPI.h:71:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestRunnerShared/spi/UIKitTestSPI.h:76:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jonathan Bedard]

Created attachment 312795 [details]
Patch

[Jonathan Bedard]

Created attachment 312807 [details]
Patch

[Build Bot]

Attachment 312807 [details] did not pass style-queue:


ERROR: Tools/TestRunnerShared/spi/UIKitTestSPI.h:39:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/TestRunnerShared/spi/UIKitTestSPI.h:74:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestRunnerShared/spi/UIKitTestSPI.h:79:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jonathan Bedard]

Created attachment 312810 [details]
Patch

[Build Bot]

Attachment 312810 [details] did not pass style-queue:


ERROR: Tools/TestRunnerShared/spi/UIKitTestSPI.h:39:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/TestRunnerShared/spi/UIKitTestSPI.h:77:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestRunnerShared/spi/UIKitTestSPI.h:82:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jonathan Bedard]

Created attachment 312814 [details]
Patch

[Build Bot]

Attachment 312814 [details] did not pass style-queue:


ERROR: Tools/TestRunnerShared/spi/UIKitTestSPI.h:39:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/TestRunnerShared/spi/UIKitTestSPI.h:77:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestRunnerShared/spi/UIKitTestSPI.h:82:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jonathan Bedard]

Created attachment 312827 [details]
Patch

[Build Bot]

Attachment 312827 [details] did not pass style-queue:


ERROR: Tools/TestRunnerShared/spi/UIKitTestSPI.h:39:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/TestRunnerShared/spi/UIKitTestSPI.h:77:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestRunnerShared/spi/UIKitTestSPI.h:82:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

Comment on attachment 312827 [details]
Patch

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

> Source/WebKit2/Platform/spi/ios/UIKitSPI.h:216
> +#ifndef UI_KEYBOARD
> +#define UI_KEYBOARD 1

This should probably be more specifically WebKitty so there's no chance for conflicts. We should probably move things around so we don't need this, but that can happen later, maybe leave a fixme? Unless perhaps someone else has an idea?

[Jonathan Bedard]

Created attachment 312830 [details]
Patch

[Jonathan Bedard]

I filed <https://bugs.webkit.org/show_bug.cgi?id=173341> to combine UIKitSPI headers into WebCore.

[Build Bot]

Attachment 312830 [details] did not pass style-queue:


ERROR: Tools/TestRunnerShared/spi/UIKitTestSPI.h:39:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/TestRunnerShared/spi/UIKitTestSPI.h:78:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestRunnerShared/spi/UIKitTestSPI.h:83:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jonathan Bedard]

(In reply to Build Bot from comment #20)
> Attachment 312830 [details] did not pass style-queue:
> 
> 
> ERROR: Tools/TestRunnerShared/spi/UIKitTestSPI.h:39:  Alphabetical sorting
> problem.  [build/include_order] [4]
> ERROR: Tools/TestRunnerShared/spi/UIKitTestSPI.h:78:  Weird number of spaces
> at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> ERROR: Tools/TestRunnerShared/spi/UIKitTestSPI.h:83:  Weird number of spaces
> at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Total errors found: 3 in 14 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

These were existing bugs in UIKitSPI.h.  Ignoring them since the file was renamed.

[WebKit Commit Bot]

Comment on attachment 312830 [details]
Patch

Clearing flags on attachment: 312830

Committed r218275: <http://trac.webkit.org/changeset/218275>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Daniel Bates]

(In reply to Jonathan Bedard from comment #0)
> Layout tests expect the screen to have a scale of 2.  

This is not correct. Test <https://trac.webkit.org/browser/trunk/LayoutTests/fast/hidpi/hidpi-3x-device-pixel-ratio.html?rev=201904> is an example of a test that will only pass on a device with a 3x device pixel ratio. Currently we skip this test (see bug #158618) as we do not run tests on a simulator device with a 3x device pixel ratio AND the infrastructure to dynamically adjust the window resolution added in the patch that added this test (bug #158597) does not seem to effect window.devicePixelRatio (I haven't looked into this issue, yet). Although we could investigate and fix the dynamic window resolution infrastructure to fix this test I am unclear of the value to do so and would suggest that we look to remove such logic once we start running on-device testing because this test exemplifies the purpose for having on-device testing - to catch regressions specific to- or affected by- the capabilities of the underlying hardware. Having a 3x display is a hardware trait that affects the value returned by window.devicePixelRatio.

> Explicitly set this scale for iOS devices and simulators so that tests can be run on plus devices.

This is moving us in the opposite direction from where we want to go with on-device testing. We want to test hardware capabilities. Without loss of generality, we want to test that on an iPhone 7 Plus, which has a 3x display, that window.devicePixelRatio returns 3.

[Daniel Bates]

(In reply to WebKit Commit Bot from comment #22)
> Comment on attachment 312830 [details]
> Patch
> 
> Clearing flags on attachment: 312830
> 
> Committed r218275: <http://trac.webkit.org/changeset/218275>

This breaks running the test LayoutTests/fast/hidpi/hidpi-3x-device-pixel-ratio.html in the iOS Simulator when simulating an iPhone 7 Plus:

Tools/Scripts/run-webkit-tests --device-type com.apple.CoreSimulator.SimDeviceType.iPhone-7-Plus --no-retry-failures --no-sample-on-timeout --ios-simulator --force LayoutTests/fast/hidpi/hidpi-3x-device-pixel-ratio.html

[Daniel Bates]

(In reply to Jonathan Bedard from comment #19)
> I filed <https://bugs.webkit.org/show_bug.cgi?id=173341> to combine UIKitSPI
> headers into WebCore.

We should not move the UIKitSPI header to WebCore because it is almost never correct to call UIKit from WebCore (code in WebCore may not be in executed in same process as the process responsible for UI; think WebKit2). Calling UIKit from the WebProcess will invariably lead to bugs and much sadness. Let's not incentive it by having the SPI header be part of the WebCore framework.

[Daniel Bates]

(In reply to Daniel Bates from comment #26)
> (In reply to Jonathan Bedard from comment #19)
> > I filed <https://bugs.webkit.org/show_bug.cgi?id=173341> to combine UIKitSPI
> > headers into WebCore.
> 
> We should not move the UIKitSPI header to WebCore because it is almost never
> correct to call UIKit from WebCore (code in WebCore may not be in executed
> in same process as the process responsible for UI; think WebKit2). Calling
> UIKit from the WebProcess will invariably lead to bugs and much sadness.
> Let's not incentive it by having the SPI header be part of the WebCore
> framework.

I can't type. This should read:

We should not move the UIKitSPI header to WebCore because it is almost never correct to call UIKit from WebCore (code in WebCore may not be executed in same process as the process responsible for UI; think WebKit2). For instance, calling UIKit from the WebProcess will invariably lead to bugs and much sadness. Let's not incentivize such behavior by having the SPI header be part of the WebCore framework.

[Alexey Proskuryakov]

> This is moving us in the opposite direction from where we want to go with on-device testing.

I think that the main use case is for someone with an iOS device to reproduce a failure that a device bot found, without having to use exactly the same kind of device.

Implementing tests that require specific devices would be a secondary goal.

[Tim Horton]

(In reply to Alexey Proskuryakov from comment #28)
> > This is moving us in the opposite direction from where we want to go with on-device testing.
> 
> I think that the main use case is for someone with an iOS device to
> reproduce a failure that a device bot found, without having to use exactly
> the same kind of device.
> 
> Implementing tests that require specific devices would be a secondary goal.

I agree with Alexey and Jonathan; I think since 99% of tests would like to be consistent, we should provide a consistent platform, and tests that specifically care about something like DPR should specify that they want it left alone (or specify that we want to force it to something specific). This is how it has always worked on Mac, and it is fine there.

[Tim Horton]

(In reply to Daniel Bates from comment #26)
> (In reply to Jonathan Bedard from comment #19)
> > I filed <https://bugs.webkit.org/show_bug.cgi?id=173341> to combine UIKitSPI
> > headers into WebCore.
> 
> We should not move the UIKitSPI header to WebCore because it is almost never
> correct to call UIKit from WebCore (code in WebCore may not be in executed
> in same process as the process responsible for UI; think WebKit2). Calling
> UIKit from the WebProcess will invariably lead to bugs and much sadness.
> Let's not incentive it by having the SPI header be part of the WebCore
> framework.

OTOH, this part is a fairly good point.

[Daniel Bates]

(In reply to Tim Horton from comment #29)
> tests that specifically care about something like DPR should specify that they want it left alone (or specify that we want to force it to something specific). This is how it has always worked on Mac, and it is fine there.

Where do we do this in this patch?

[Tim Horton]

(In reply to Daniel Bates from comment #31)
> (In reply to Tim Horton from comment #29)
> > tests that specifically care about something like DPR should specify that they want it left alone (or specify that we want to force it to something specific). This is how it has always worked on Mac, and it is fine there.
> 
> Where do we do this in this patch?

I don't think I said we do, I said we should :)

That said, the test you cite, fast/hidpi/hidpi-3x-device-pixel-ratio.html, *already has* a mechanism to force a particular DPR, so this mechanism already exists... it just doesn't work (and the test is marked as failing globally, and has been for a long time. See r201904.

[Daniel Bates]

(In reply to Tim Horton from comment #32)
> (In reply to Daniel Bates from comment #31)
> > (In reply to Tim Horton from comment #29)
> > > tests that specifically care about something like DPR should specify that they want it left alone (or specify that we want to force it to something specific). This is how it has always worked on Mac, and it is fine there.
> > 
> > Where do we do this in this patch?
> 
> I don't think I said we do, I said we should :)
> 
> That said, the test you cite, fast/hidpi/hidpi-3x-device-pixel-ratio.html,
> *already has* a mechanism to force a particular DPR, so this mechanism
> already exists... it just doesn't work (and the test is marked as failing
> globally, and has been for a long time. See r201904.

Looking at the code change in r201904 it only implemented the mechanism for Mac :( We should implement it for iOS based on the main use case for on-device testing that Alexey mentioned in comment #28. I am not at a computer to be able to easily check if we ever tried to port this to iOS (only to be left broken and/or never re-enabling the test). I suspect we did not port this mechanism to iOS given the code change in this patch does that resemble the DPR switching logic we have for Mac. I would have expected this patch to implement such a mechanism as opposed to hardcode a scale factor though this patch is a step in the right direction assuming the main use case for on-device testing described in comment #28.

[Jonathan Bedard]

I think we should implement some sort of screen-scale changing for iOS.  I will look into this.  I think for the time being, we should keep this hard-coded version.  I'll look into this at a high-level today (6/15) and file another bug, linking to this one.

To Dan's point about UIKit in WebCore, bug 173341 tracks this proposed change.  I'd like to keep discussion about the UIKit headers there.

[Jonathan Bedard]

Filed https://bugs.webkit.org/show_bug.cgi?id=173420 to track allowing tests to define a different screen scale.