Bug 151327 – Use the media queries to dynamically set the stage for the graphics benchmark

• Status: RESOLVED
• Resolution: FIXED
• Component: Animations

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

Created attachment 265639 [details]
Patch

[Said Abou-Hallawa]

Created attachment 265640 [details]
Patch

[Simon Fraser (smfr)]

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.

[Said Abou-Hallawa]

Created attachment 265682 [details]
Patch

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

Created attachment 265683 [details]
Patch

[Simon Fraser (smfr)]

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.

[Said Abou-Hallawa]

Created attachment 265711 [details]
Patch

[Said Abou-Hallawa]

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.

[WebKit Commit Bot]

Comment on attachment 265711 [details]
Patch

Clearing flags on attachment: 265711

Committed r192549: <http://trac.webkit.org/changeset/192549>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.