Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support changing the DPI in tests #7140

Open
wpt-issue-mover opened this issue Aug 31, 2017 · 12 comments
Open

Support changing the DPI in tests #7140

wpt-issue-mover opened this issue Aug 31, 2017 · 12 comments
Labels
infra priority:backlog type:untestable wptrunner The automated test runner, commonly called through ./wpt run

Comments

@wpt-issue-mover
Copy link

wpt-issue-mover commented Aug 31, 2017

Originally posted as w3c/wptrunner#240 by @gsnedders on 21 Mar 2017, 17:50 UTC:

Split out from w3c/wptrunner#166

[edit: see #7140 (comment) for the current status]

@gsnedders gsnedders added infra wptrunner The automated test runner, commonly called through ./wpt run labels Aug 31, 2017
@progers
Copy link
Contributor

progers commented Feb 17, 2019

Is this issue for adding support for changing DPI in web platform tests?

I just ran into this when trying to write a WPT test for a chromium bug. Our rendering is different with different DPI values and we had a bug that was only present at non-1.0 DPIs (https://crbug.com/925676). I wanted to add a WPT test for this but was not able to because we don't have an API for that yet. Blink's test runner has SetBackingScaleFactor(double) which is roughly the API we'd need to add. In blink we have 45 tests that use SetBackingScaleFactor.

@gsnedders
Copy link
Member

Is this issue for adding support for changing DPI in web platform tests?

Yes. Essentially we have undocumented support in wptrunner for this for Servo only, and given it's something that is generally useful it's been a low-priority to-do item for the long time.

I just ran into this when trying to write a WPT test for a chromium bug. Our rendering is different with different DPI values and we had a bug that was only present at non-1.0 DPIs (https://crbug.com/925676). I wanted to add a WPT test for this but was not able to because we don't have an API for that yet. Blink's test runner has SetBackingScaleFactor(double) which is roughly the API we'd need to add. In blink we have 45 tests that use SetBackingScaleFactor.

The current support we have relies on <meta name=device-pixel-ratio contents=[double]>. Given some browsers need restarted to actually pay attention to that changing, we need something statically readable, as that is. I wouldn't be surprised if we put a wpt- prefix on the name or something.

@progers
Copy link
Contributor

progers commented Feb 19, 2019

In the review of my chromium bug fix, a reviewer pointed out that chromium has two ways to write DPI web tests: the testRunner.SetBackingScaleFactor API and a chromium test feature called "virtual test suites". Virtual test suites are where certain test directories re-run with different flags such as gpu acceleration enabled/disabled, directwrite fonts enabled/disabled, threaded compositing enabled/disabled, various scale factors, etc. To avoid running all tests many times, virtual suites are limited to certain directories (see: VirtualTestSuites). These approaches are chromium-specific though, so we need a way to write upstreamed WPT tests.

I would support either of these approaches in WPT tests: special directories that are declared to depend on DPI, or an explicit DPI flag used in the test itself. I looked through chromium's existing tests that use SetBackingScaleFactor and it would not be an issue to switch these to use a static, declarative API such as <meta name=wpt-device-pixel-ratio contents=[double]> (in other words, the existing tests are not testing changing DPIs).

Image srcset is an example of a web feature that needs DPI control to test. I didn't find existing srcset WPT tests that cover srcset resolutions (there are some DPR header tests but I didn't see any that test the rendering of 2x images). (@yoavweiss , do you know if this is tested and I just missed it?) I think we need DPI control to write WPT srcset tests.

Do you mind if we change the title of this issue to "Add DPI support to WPT tests"?

aarongable pushed a commit to chromium/chromium that referenced this issue Feb 19, 2019
cc::EffectNode tracks a raster surface contents scale, and this is
updated in EffectTree::UpdateSurfaceContentsScale. If perspective is
present in the to-screen transform, a fallback codepath is used which
uses cc::TransformTree::device_scale_factor, and this value was not set
when BlinkGenPropertyTrees was enabled.

This patch updates cc::TransformTree::device_scale_factor and adds a
test. This could not be a WPT test due to missing APIs for changing DPI,
and web-platform-tests/wpt#7140 has been
updated asking for this API.

Bug: 925676
Change-Id: Ifa037d1412de37f196885f4cbf69148f37e512d5
Reviewed-on: https://chromium-review.googlesource.com/c/1476264
Commit-Queue: Philip Rogers <[email protected]>
Reviewed-by: Xianzhu Wang <[email protected]>
Cr-Commit-Position: refs/heads/master@{#633275}
@gsnedders gsnedders changed the title Support changing the dpi in non-servo executors Support changing the DPI in tests Feb 19, 2019
@yoavweiss
Copy link
Contributor

yoavweiss commented Mar 24, 2019 via email

@noamr
Copy link
Contributor

noamr commented Feb 17, 2020

I've added tests for image-set, and encountered the same need.... src selection in image-set can only be effectively tested when diffrent DPRs are allowed

@noamr
Copy link
Contributor

noamr commented Jul 29, 2020

I'm currently writing tests that truly need this: #23789, otherwise they don't test a lot of the functionality.

I tried to fix this myself but got lost in the complexities of the python infrastructure... any help with this would be appreciated.

@stephenmcgruer
Copy link
Contributor

So today, Edge (the Chromium-base one), Chrome, and Safari have their reftest-support in tools/wptrunner/wptrunner/executors/executorwebdriver.py, and Firefox has it in tools/wptrunner/wptrunner/executors/executormarionette.py.

As you can see, we currently assert that dpi is None, because only Servo supports changing the dpi (check out executorservo.py). I believe they are able to support this because they always shutdown and re-launch their browser after every test, whereas the others re-use a browser instance between tests where possible for speed.

Options for adding dpi support that I can think of:

  1. Detect when dpi is non-default, and restart the browser with the appropriate flag in such cases. (E.g. for Chromium there's --force-device-scale-factor which I think is equivalent to changing dpi?). This is a bit weird as its a violation of abstraction for most browsers; normally the higher-level testrunner.py (tools/wptrunner/wptrunner/testrunner.py) takes care of the browser lifecycle. (Servo appears to be an exception to this, which is interesting).
  2. Add a WebDriver or other web-exposed API for setting the DPI. This is a longer path given it requires getting browsers to agree on this being useful and feasible, but its much less hacky than restarting browsers and passing special flags. (And works on any browser that adheres to the standard).

cc @jgraham @Hexcles who may have thoughts (also gsnedders, but they're already on this issue)

@jgraham
Copy link
Contributor

jgraham commented Aug 3, 2020

There are a couple of hooks here to help. settings is for things that require a full browser restart because of a change in the test setup e.g. [1]. If the settings dictionary changes from one test to the next the runner will arrange for a full restart, passing in the settings dictionary [2].

If the pref can be changed at runtime without a restart, it can form part of the test "environment" [3], which is basically a mechanism to do some stateful things before a test, but only if necessary.

So if this can be done in terms of prefs, I think it's pretty easy to do in wptrunner. In gecko it can be controlled using the layout.css.devPixelsPerPx pref, and that can be set at runtime. That means for gecko at least this can form part of the test environment.

I independently think that this makes sense as a WebDriver feature, and would certainly support a PR against that spec to make it an option. That may be needed to support WebKit (I have no idea how settings in WebKit works, so it may also not be needed).

[1] https://searchfox.org/mozilla-central/source/testing/web-platform/tests/tools/wptrunner/wptrunner/browsers/firefox.py#725
[2] https://searchfox.org/mozilla-central/source/testing/web-platform/tests/tools/wptrunner/wptrunner/testrunner.py#222
[3] https://searchfox.org/mozilla-central/source/testing/web-platform/tests/tools/wptrunner/wptrunner/executors/base.py#285

@noamr
Copy link
Contributor

noamr commented Aug 3, 2020

There are a couple of hooks here to help. settings is for things that require a full browser restart because of a change in the test setup e.g. [1]. If the settings dictionary changes from one test to the next the runner will arrange for a full restart, passing in the settings dictionary [2].

If the pref can be changed at runtime without a restart, it can form part of the test "environment" [3], which is basically a mechanism to do some stateful things before a test, but only if necessary.

So if this can be done in terms of prefs, I think it's pretty easy to do in wptrunner. In gecko it can be controlled using the layout.css.devPixelsPerPx pref, and that can be set at runtime. That means for gecko at least this can form part of the test environment.

I independently think that this makes sense as a WebDriver feature, and would certainly support a PR against that spec to make it an option. That may be needed to support WebKit (I have no idea how settings in WebKit works, so it may also not be needed).

WebKit internal tests can modify the dpr before loading a page using Javascript (setBackingScaleFactor). It's used by internal srcset tests.

@noamr
Copy link
Contributor

noamr commented Aug 3, 2020

I think the following issue in WebDriver is about this: w3c/webdriver#1378

@stephenmcgruer
Copy link
Contributor

WebKit internal tests can modify the dpr before loading a page using Javascript (setBackingScaleFactor). It's used by internal srcset tests.

I suspect that feature is likely not exposed in main Safari though, I presume? (Similar to Chromium's internals testing API, which only exists in our content_shell not chrome) As such it would present the problem that runners using Safari (e.g. what we do here in wpt) would never pass those tests.

@noamr
Copy link
Contributor

noamr commented Aug 3, 2020

WebKit internal tests can modify the dpr before loading a page using Javascript (setBackingScaleFactor). It's used by internal srcset tests.

I suspect that feature is likely not exposed in main Safari though, I presume? (Similar to Chromium's internals testing API, which only exists in our content_shell not chrome) As such it would present the problem that runners using Safari (e.g. what we do here in wpt) would never pass those tests.

When wpt is imported to webkit it uses the internal testing API rather than what's exposed to Safari. I'm not sure what's exposed in Safari, I don't know the Safari WebDriver code.
But in any case, I think WebDriver is the way to go, something simple-ish like setting scale-factor for a WebDriver context that would affect subsequent navigations in a window.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infra priority:backlog type:untestable wptrunner The automated test runner, commonly called through ./wpt run
Projects
None yet
Development

No branches or pull requests

8 participants