Bug 114029

Summary: Incorrect evaluation of resolution media queries
Product: WebKit Reporter: Rune Lillesveen <rune>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: alexander.shalamov, bdakin, commit-queue, eric.carlson, esprehn+autocc, jer.noble, kenneth, koivisto, macpherson, menard, ojan.autocc, simon.fraser, syoichi, webkit-ews, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Rune Lillesveen 2013-04-05 06:00:45 PDT
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/
Comment 1 Rune Lillesveen 2013-04-05 06:28:15 PDT
Created attachment 196626 [details]
Patch
Comment 2 WebKit Review Bot 2013-04-05 06:33:01 PDT
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.
Comment 3 Rune Lillesveen 2013-04-10 06:14:51 PDT
Created attachment 197253 [details]
Patch
Comment 4 EFL EWS Bot 2013-04-10 06:21:32 PDT
Comment on attachment 197253 [details]
Patch

Attachment 197253 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17732056
Comment 5 Early Warning System Bot 2013-04-10 06:26:25 PDT
Comment on attachment 197253 [details]
Patch

Attachment 197253 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17728027
Comment 6 Kenneth Rohde Christiansen 2013-04-10 06:26:34 PDT
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"
Comment 7 Rune Lillesveen 2013-04-10 06:39:57 PDT
Created attachment 197257 [details]
Patch
Comment 8 Rune Lillesveen 2013-04-10 06:43:30 PDT
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.
Comment 9 Kenneth Rohde Christiansen 2013-04-10 06:44:54 PDT
Comment on attachment 197257 [details]
Patch

r=me, will you look at print afterwards?
Comment 10 Kenneth Rohde Christiansen 2013-04-10 06:46:05 PDT
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
Comment 11 Rune Lillesveen 2013-04-10 06:49:08 PDT
(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?
Comment 12 Kenneth Rohde Christiansen 2013-04-10 06:50:49 PDT
> 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
Comment 13 Rune Lillesveen 2013-04-10 06:53:09 PDT
(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.
Comment 14 Kenneth Rohde Christiansen 2013-04-10 06:54:26 PDT
 > 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!
Comment 15 Rune Lillesveen 2013-04-10 09:06:49 PDT
Created attachment 197274 [details]
Patch
Comment 16 Kenneth Rohde Christiansen 2013-04-10 09:11:23 PDT
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
Comment 17 Rune Lillesveen 2013-04-10 09:26:28 PDT
Created attachment 197305 [details]
Patch
Comment 18 WebKit Commit Bot 2013-04-10 11:24:23 PDT
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.
Comment 19 Kenneth Rohde Christiansen 2013-04-11 01:12:04 PDT
Comment on attachment 197305 [details]
Patch

Want to set cq?
Comment 20 WebKit Commit Bot 2013-04-11 01:58:58 PDT
Comment on attachment 197305 [details]
Patch

Clearing flags on attachment: 197305

Committed r148186: <http://trac.webkit.org/changeset/148186>
Comment 21 WebKit Commit Bot 2013-04-11 01:59:01 PDT
All reviewed patches have been landed.  Closing bug.