Bug 151327

Summary: Use the media queries to dynamically set the stage for the graphics benchmark
Product: WebKit Reporter: Said Abou-Hallawa <sabouhallawa>
Component: AnimationsAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, dino, jonlee, rniwa, simon.fraser
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

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.