Summary: | Incorrect evaluation of resolution media queries | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Rune Lillesveen <rune> | ||||||||||||
Component: | CSS | Assignee: | 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
Rune Lillesveen
2013-04-05 06:00:45 PDT
Created attachment 196626 [details]
Patch
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.
Created attachment 197253 [details]
Patch
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 on attachment 197253 [details] Patch Attachment 197253 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17728027 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" Created attachment 197257 [details]
Patch
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 on attachment 197257 [details]
Patch
r=me, will you look at print afterwards?
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 (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?
> 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
(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. > 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! Created attachment 197274 [details]
Patch
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 Created attachment 197305 [details]
Patch
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 on attachment 197305 [details]
Patch
Want to set cq?
Comment on attachment 197305 [details] Patch Clearing flags on attachment: 197305 Committed r148186: <http://trac.webkit.org/changeset/148186> All reviewed patches have been landed. Closing bug. |