Bug 151327 - Use the media queries to dynamically set the stage for the graphics benchmark
Summary: Use the media queries to dynamically set the stage for the graphics benchmark
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-11-16 15:24 PST by Said Abou-Hallawa
Modified: 2015-11-17 16:08 PST (History)
6 users (show)

See Also:


Attachments
Patch (8.20 KB, patch)
2015-11-16 16:47 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (8.20 KB, patch)
2015-11-16 16:52 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (9.23 KB, patch)
2015-11-17 11:10 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (10.04 KB, patch)
2015-11-17 11:19 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (9.95 KB, patch)
2015-11-17 15:11 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2015-11-16 15:24:57 PST
Currently a css rule is added at run time to change the style of the stage. document.stylesheets.cssRules may return null for local document for some browsers. So this way seems unreliable.
Comment 1 Said Abou-Hallawa 2015-11-16 16:47:35 PST
Created attachment 265639 [details]
Patch
Comment 2 Said Abou-Hallawa 2015-11-16 16:52:56 PST
Created attachment 265640 [details]
Patch
Comment 3 Simon Fraser (smfr) 2015-11-16 18:36:07 PST
Comment on attachment 265640 [details]
Patch

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

> PerformanceTests/Animometer/runner/resources/animometer.css:243
> +@media screen and (min-width: 1800px) {

min-width tests window width, not screen width. You need min-device-width for screen width.

> PerformanceTests/Animometer/runner/resources/animometer.css:370
> +@media screen and (-webkit-min-device-pixel-ratio: 2), (min-resolution: 192dpi) {
> +    section#running > #running-test > iframe.normalized {
> +        width: 200%;
> +        height: 200%;
> +        transform: scale(0.5) translate(-50%, -50%);
> +    }
> +}

I think it's wrong to scale the whole iframe on retina.

> PerformanceTests/Animometer/tests/text/resources/layering-text.js:146
> -    var fontHeight = lineHeight / 1.5;
> +    var fontHeight = lineHeight / 1.4;
>      var fontSize = fontHeight * 72.0 / 96.0;
> -    DocumentExtension.insertCssRuleAfter(".text-layer", ".text-layer { font-size: " + fontSize.toFixed(2) + "px; }");
> +    DocumentExtension.appendCssRule(".text-layer { font-size: " + fontSize.toFixed(2) + "px; }");

This should be done through css.
Comment 4 Said Abou-Hallawa 2015-11-17 11:10:08 PST
Created attachment 265682 [details]
Patch
Comment 5 Said Abou-Hallawa 2015-11-17 11:13:03 PST
Comment on attachment 265640 [details]
Patch

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

>> PerformanceTests/Animometer/runner/resources/animometer.css:243
>> +@media screen and (min-width: 1800px) {
> 
> min-width tests window width, not screen width. You need min-device-width for screen width.

Done.

>> PerformanceTests/Animometer/runner/resources/animometer.css:370
>> +}
> 
> I think it's wrong to scale the whole iframe on retina.

The stage is scaled instead of scaling the iframe on retina.

>> PerformanceTests/Animometer/tests/text/resources/layering-text.js:146
>> +    DocumentExtension.appendCssRule(".text-layer { font-size: " + fontSize.toFixed(2) + "px; }");
> 
> This should be done through css.

Done. Using em font size seems to fix the issue without having to insert css rules or using media queries.
Comment 6 Said Abou-Hallawa 2015-11-17 11:19:52 PST
Created attachment 265683 [details]
Patch
Comment 7 Simon Fraser (smfr) 2015-11-17 13:15:29 PST
Comment on attachment 265683 [details]
Patch

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

> PerformanceTests/Animometer/tests/resources/stage.js:40
> +    if (options["normalize-for-device-scale-factor"])
> +        this.element.classList.add("normalized");

I don't know why we'd want this option at all.
Comment 8 Said Abou-Hallawa 2015-11-17 15:11:27 PST
Created attachment 265711 [details]
Patch
Comment 9 Said Abou-Hallawa 2015-11-17 15:21:15 PST
Comment on attachment 265683 [details]
Patch

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

>> PerformanceTests/Animometer/tests/resources/stage.js:40
>> +        this.element.classList.add("normalized");
> 
> I don't know why we'd want this option at all.

This option was removed.
Comment 10 WebKit Commit Bot 2015-11-17 16:08:42 PST
Comment on attachment 265711 [details]
Patch

Clearing flags on attachment: 265711

Committed r192549: <http://trac.webkit.org/changeset/192549>
Comment 11 WebKit Commit Bot 2015-11-17 16:08:48 PST
All reviewed patches have been landed.  Closing bug.