Bug 114029 – Incorrect evaluation of resolution media queries

• Component: CSS
• Status: RESOLVED FIXED

[Rune Lillesveen]

The current implementation of the resolultion media feature, inside ENABLE(RESOLUTION_MEDIA_QUERY) if-defs, do calculations involving the physical resolution instead of the actual CSS resolution directly.

In addition to the Media Queries spec, see:

http://www.w3.org/blog/CSS/2012/06/14/unprefix-webkit-device-pixel-ratio/

[Rune Lillesveen]

Created attachment 196626 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/media/mq-resolution.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSPrimitiveValue.h', u'Source/WebCore/css/MediaQueryEvaluator.cpp']" exit_code: 1
Source/WebCore/css/MediaQueryEvaluator.cpp:303:  device_pixel_ratioMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 5 files


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

[Rune Lillesveen]

Created attachment 197253 [details]
Patch

[EFL EWS Bot]

Comment on attachment 197253 [details]
Patch

Attachment 197253 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17732056

[Early Warning System Bot]

Comment on attachment 197253 [details]
Patch

Attachment 197253 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17728027

[Kenneth Rohde Christiansen]

Comment on attachment 197253 [details]
Patch

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

Please do the style fix in a separate patch.

So "print" now follows the dpi of the screen? It would be nice that the high resolution images were used when printing

> Source/WebCore/ChangeLog:14
> +
> +        To pass the coding style, the media feature enums and method names
> +        have been changed.

We usually don't do such changes together with a bug fix. It should really be a separate patch. The style comments are only "educational"

[Rune Lillesveen]

Created attachment 197257 [details]
Patch

[Rune Lillesveen]

Comment on attachment 197253 [details]
Patch

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

>> Source/WebCore/ChangeLog:14
>> +        have been changed.
> 
> We usually don't do such changes together with a bug fix. It should really be a separate patch. The style comments are only "educational"

OK. I've re-uploaded the first patch now.

[Kenneth Rohde Christiansen]

Comment on attachment 197257 [details]
Patch

r=me, will you look at print afterwards?

[Kenneth Rohde Christiansen]

Comment on attachment 197257 [details]
Patch

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

> LayoutTests/fast/media/mq-resolution.html:-60
>  
> -        window.internals.settings.setResolutionOverride(96, 96);

btw can't you remove this testing method as well? If it is not used for something else, I think you ought to

[Rune Lillesveen]

(In reply to comment #10)
> (From update of attachment 197257 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=197257&action=review
> 
> > LayoutTests/fast/media/mq-resolution.html:-60
> >  
> > -        window.internals.settings.setResolutionOverride(96, 96);
> 
> btw can't you remove this testing method as well? If it is not used for something else, I think you ought to

In the same patch?

[Kenneth Rohde Christiansen]

> In the same patch?

That would be fine, but it could also be a separate patch if you commit to doing it within reasonable time. I think that I would prefer the former

[Rune Lillesveen]

(In reply to comment #6)

> So "print" now follows the dpi of the screen? It would be nice that the high resolution images were used when printing

No, print will use 300dpi as before my patch. The evaluation code for devicePixelRatio, now also used for resolution, already did that too.

[Kenneth Rohde Christiansen]

 > No, print will use 300dpi as before my patch. The evaluation code for devicePixelRatio, now also used for resolution, already did that too.

Ah great, perfect!

[Rune Lillesveen]

Created attachment 197274 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 197274 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        evaluating the resolution and device-pixel-ratio media features.
> +
> +        * WebCore.exp.in:

I would add "Covered by existing tests" or similar

[Rune Lillesveen]

Created attachment 197305 [details]
Patch

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'LayoutTests/ChangeLog', u'LayoutTests/fast/media/mq-resolution.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/css/CSSPrimitiveValue.h', u'Source/WebCore/css/MediaQueryEvaluator.cpp', u'Source/WebCore/page/Screen.cpp', u'Source/WebCore/page/Screen.h', u'Source/WebCore/page/Settings.cpp', u'Source/WebCore/page/Settings.h', u'Source/WebCore/testing/InternalSettings.cpp', u'Source/WebCore/testing/InternalSettings.h', u'Source/WebCore/testing/InternalSettings.idl', u'Source/WebKit/ChangeLog', u'Source/WebKit/WebKit.vcxproj/WebKitExportGenerator/WebKitExports.def.in', u'Source/WebKit/win/ChangeLog', u'Source/WebKit/win/WebKit.vcproj/WebKitExports.def.in', u'Source/autotools/symbols.filter']" exit_code: 1
Source/WebCore/css/MediaQueryEvaluator.cpp:303:  device_pixel_ratioMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 8 files


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

[Kenneth Rohde Christiansen]

Comment on attachment 197305 [details]
Patch

Want to set cq?

[WebKit Commit Bot]

Comment on attachment 197305 [details]
Patch

Clearing flags on attachment: 197305

Committed r148186: <http://trac.webkit.org/changeset/148186>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.