Bug 99077 – Add support for resolution media query

• Component: CSS
• Status: RESOLVED FIXED

[Kenneth Rohde Christiansen]

Incl. support for dppx!

[Kenneth Rohde Christiansen]

Created attachment 168234 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebCore/css/MediaQueryExp.cpp:163:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/css/MediaQueryEvaluator.cpp:507:  min_resolutionMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/css/MediaQueryEvaluator.cpp:512:  max_resolutionMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 3 in 11 files


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

[Build Bot]

Comment on attachment 168234 [details]
Patch

Attachment 168234 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14258621

[WebKit Review Bot]

Comment on attachment 168234 [details]
Patch

Attachment 168234 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14248984

New failing tests:
fast/media/mq-resolution.html

[Alexander Shalamov]

Comment on attachment 168234 [details]
Patch

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

> Source/WebCore/css/MediaQueryEvaluator.cpp:200
> +bool compareResolution(unsigned min, unsigned max, unsigned value, MediaFeaturePrefix op)

Should it be const, so that it will stay in this compilation unit. Maybe that's why mac build fails.

/Volumes/Data/EWS/WebKit/Source/WebCore/css/MediaQueryEvaluator.cpp:200:6: error: no previous prototype for function 'compareResolution' [-Werror,-Wmissing-prototypes]
bool compareResolution(unsigned min, unsigned max, unsigned value, MediaFeaturePrefix op)
     ^
1 error generated.

[Alexander Shalamov]

(In reply to comment #5)
Sorry, typo. const -> static

[Kenneth Rohde Christiansen]

Created attachment 168715 [details]
Patch

[Kenneth Rohde Christiansen]

Created attachment 168721 [details]
Patch

[Build Bot]

Comment on attachment 168721 [details]
Patch

Attachment 168721 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14300695

[Build Bot]

Comment on attachment 168721 [details]
Patch

Attachment 168721 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14293722

[Peter Beverloo (cr-android ews)]

Comment on attachment 168721 [details]
Patch

Attachment 168721 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14293726

[Eric Seidel (no email)]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebCore/css/MediaQueryEvaluator.cpp:51:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/css/MediaQueryEvaluator.cpp:523:  min_resolutionMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/css/MediaQueryEvaluator.cpp:528:  max_resolutionMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 3 in 14 files


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

[kov's GTK+ EWS bot]

Comment on attachment 168721 [details]
Patch

Attachment 168721 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14295694

[WebKit Review Bot]

Comment on attachment 168721 [details]
Patch

Attachment 168721 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14297741

[Kenneth Rohde Christiansen]

Created attachment 168914 [details]
Patch

[Build Bot]

Comment on attachment 168914 [details]
Patch

Attachment 168914 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14391122

[Kenneth Rohde Christiansen]

Any clue anyone? 

Undefined symbols for architecture x86_64:
  "__ZN7WebCore8Settings21setResolutionOverrideERKNS_7IntRectE", referenced from:
     -exported_symbol[s_list] command line option
ld: symbol(s) not found for architecture x86_64

[Alexis Menard (darktears)]

Comment on attachment 168914 [details]
Patch

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

> Source/WebCore/page/Settings.cpp:462
> +void Settings::setResolutionOverride(const IntSize& densityPerInchOverride)

I think the declaration doesn't match with your Source/WebCore/WebCore.exp.in declaration (IntSize -> IntRect).

[Kenneth Rohde Christiansen]

(In reply to comment #17)
> Any clue anyone? 
> 
> Undefined symbols for architecture x86_64:
>   "__ZN7WebCore8Settings21setResolutionOverrideERKNS_7IntRectE", referenced from:
>      -exported_symbol[s_list] command line option
> ld: symbol(s) not found for architecture x86_64

s/IntRect/IntSize :-) need more coffee!

[Kenneth Rohde Christiansen]

Created attachment 168925 [details]
Patch

[WebKit Review Bot]

Comment on attachment 168925 [details]
Patch

Attachment 168925 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14396115

New failing tests:
fast/media/mq-resolution.html

[Build Bot]

Comment on attachment 168925 [details]
Patch

Attachment 168925 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14391165

New failing tests:
fast/media/mq-resolution.html

[kov's GTK+ EWS bot]

Comment on attachment 168925 [details]
Patch

Attachment 168925 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14389165

[Kenneth Rohde Christiansen]

Created attachment 168958 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'LayoutTests/ChangeLog', u'La..." exit_code: 1
Source/WebCore/css/MediaQueryEvaluator.cpp:52:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/css/MediaQueryEvaluator.cpp:524:  min_resolutionMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/css/MediaQueryEvaluator.cpp:529:  max_resolutionMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 3 in 19 files


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

[Build Bot]

Comment on attachment 168958 [details]
Patch

Attachment 168958 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14386210

New failing tests:
fast/media/mq-resolution.html

[WebKit Review Bot]

Comment on attachment 168958 [details]
Patch

Attachment 168958 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14396235

New failing tests:
fast/media/mq-resolution.html

[WebKit Review Bot]

Comment on attachment 168958 [details]
Patch

Attachment 168958 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14384297

New failing tests:
fast/media/mq-resolution.html

[Kenneth Rohde Christiansen]

Created attachment 169190 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'LayoutTests/ChangeLog', u'La..." exit_code: 1
LayoutTests/platform/chromium/TestExpectations:174:  Path does not exist.  [test/expectations] [5]
LayoutTests/platform/mac/TestExpectations:968:  Path does not exist.  [test/expectations] [5]
Source/WebCore/css/MediaQueryEvaluator.cpp:52:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/css/MediaQueryEvaluator.cpp:526:  min_resolutionMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/css/MediaQueryEvaluator.cpp:531:  max_resolutionMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 5 in 21 files


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

[Build Bot]

Comment on attachment 169190 [details]
Patch

Attachment 169190 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14392626

[Kenneth Rohde Christiansen]

Created attachment 169200 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'LayoutTests/ChangeLog', u'La..." exit_code: 1
LayoutTests/platform/chromium/TestExpectations:174:  Path does not exist.  [test/expectations] [5]
LayoutTests/platform/mac/TestExpectations:969:  Path does not exist.  [test/expectations] [5]
Source/WebCore/css/MediaQueryEvaluator.cpp:52:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/css/MediaQueryEvaluator.cpp:528:  min_resolutionMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/css/MediaQueryEvaluator.cpp:533:  max_resolutionMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 5 in 21 files


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

[Build Bot]

Comment on attachment 169200 [details]
Patch

Attachment 169200 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14384606

[WebKit Review Bot]

Comment on attachment 169200 [details]
Patch

Attachment 169200 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14392656

New failing tests:
fast/media/mq-resolution.html

[Kenneth Rohde Christiansen]

Created attachment 169228 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'LayoutTests/ChangeLog', u'La..." exit_code: 1
LayoutTests/platform/chromium/TestExpectations:174:  Path does not exist.  [test/expectations] [5]
LayoutTests/platform/mac/TestExpectations:969:  Path does not exist.  [test/expectations] [5]
Source/WebCore/css/MediaQueryEvaluator.cpp:52:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/css/MediaQueryEvaluator.cpp:530:  min_resolutionMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/css/MediaQueryEvaluator.cpp:535:  max_resolutionMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 5 in 21 files


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

[Kenneth Rohde Christiansen]

Created attachment 169229 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'LayoutTests/ChangeLog', u'La..." exit_code: 1
LayoutTests/platform/chromium/TestExpectations:174:  Path does not exist.  [test/expectations] [5]
LayoutTests/platform/mac/TestExpectations:969:  Path does not exist.  [test/expectations] [5]
Source/WebCore/css/MediaQueryEvaluator.cpp:52:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/css/MediaQueryEvaluator.cpp:530:  min_resolutionMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/css/MediaQueryEvaluator.cpp:535:  max_resolutionMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 5 in 21 files


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

[Build Bot]

Comment on attachment 169229 [details]
Patch

Attachment 169229 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14387689

New failing tests:
fast/media/mq-resolution.html

[WebKit Review Bot]

Comment on attachment 169229 [details]
Patch

Attachment 169229 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14388708

New failing tests:
fast/media/mq-resolution.html

[Build Bot]

Comment on attachment 169229 [details]
Patch

Attachment 169229 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14390683

New failing tests:
fast/media/mq-resolution.html

[Kenneth Rohde Christiansen]

Created attachment 169265 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'LayoutTests/ChangeLog', u'La..." exit_code: 1
Source/WebCore/css/MediaQueryEvaluator.cpp:52:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/css/MediaQueryEvaluator.cpp:530:  min_resolutionMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/css/MediaQueryEvaluator.cpp:535:  max_resolutionMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 3 in 21 files


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

[Kenneth Rohde Christiansen]

Created attachment 169928 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'LayoutTests/ChangeLog', u'La..." exit_code: 1
Source/WebCore/css/MediaQueryEvaluator.cpp:52:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/css/MediaQueryEvaluator.cpp:557:  min_resolutionMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/css/MediaQueryEvaluator.cpp:562:  max_resolutionMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 3 in 21 files


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

[Kenneth Rohde Christiansen]

Created attachment 169969 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'LayoutTests/ChangeLog', u'La..." exit_code: 1
Source/WebCore/css/MediaQueryEvaluator.cpp:557:  min_resolutionMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/css/MediaQueryEvaluator.cpp:562:  max_resolutionMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 21 files


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

[Adam Barth]

Comment on attachment 169969 [details]
Patch

I don't see a link to a spec in the ChangeLog.  Is this part of a spec anywhere?  Also, you might want to link to the webkit-dev thread described in http://www.webkit.org/coding/adding-features.html.  Finally, isn't there already a media query for device pixel ratio?

[Kenneth Rohde Christiansen]

(In reply to comment #49)
> (From update of attachment 169969 [details])
> I don't see a link to a spec in the ChangeLog.  Is this part of a spec anywhere?  Also, you might want to link to the webkit-dev thread described in http://www.webkit.org/coding/adding-features.html.  Finally, isn't there already a media query for device pixel ratio?

I will add that; they are:

http://www.w3.org/TR/css3-mediaqueries/#resolution
http://dev.w3.org/csswg/css3-values/#resolution

Yes, basically it was discussed to push device pixel ratio to become spec'ed but resolution was already specced and implemented by other, like Mozilla. As (min-resolution: 192) is harder to understand than (device-pixel-ratio: 2) the dppx syntax was added in css3-values, which represents the same.

[Kenneth Rohde Christiansen]

Created attachment 169980 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'LayoutTests/ChangeLog', u'La..." exit_code: 1
Source/WebCore/css/MediaQueryEvaluator.cpp:557:  min_resolutionMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/css/MediaQueryEvaluator.cpp:562:  max_resolutionMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 21 files


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

[Kenneth Rohde Christiansen]

FYI, this features (incl. dppx) is shipped by Mozilla in Firefox 16. Opera are also shipping the 'resolution' media query, but not with dppx support yet (at least to my knowledge)

[Kenneth Rohde Christiansen]

(In reply to comment #53)
> FYI, this features (incl. dppx) is shipped by Mozilla in Firefox 16. Opera are also shipping the 'resolution' media query, but not with dppx support yet (at least to my knowledge)

Actually Opera has it implemented, but are not shipping it yet: https://twitter.com/frivoal/status/231420753130692608

[Antti Koivisto]

Comment on attachment 169980 [details]
Patch

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

> Source/WebCore/css/MediaQueryEvaluator.cpp:201
> +#if ENABLE(CSS_IMAGE_RESOLUTION)
> +static bool compareResolution(float min, float max, float value, MediaFeaturePrefix op)

It seems bit surprising to tie this to CSS image resolution feature. Maybe the new units could be enabled globally (they shouldn't hurt even without clients) and this feature could have a feature flag of its own.

[Kenneth Rohde Christiansen]

Created attachment 170107 [details]
Patch

[Kenneth Rohde Christiansen]

Created attachment 170139 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'LayoutTests/ChangeLog', u'La..." exit_code: 1
Source/WebCore/css/MediaQueryEvaluator.cpp:557:  min_resolutionMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/css/MediaQueryEvaluator.cpp:562:  max_resolutionMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 35 files


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

[WebKit Review Bot]

Comment on attachment 170139 [details]
Patch

Rejecting attachment 170139 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
T (content): Merge conflict in Source/WebKit2/ChangeLog
Failed to merge in the changes.
Patch failed at 0002 [EFL][WK2] Convert WebEvent's timestamp from millisecond to second.

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.

Full output: http://queues.webkit.org/results/14487910

[Kenneth Rohde Christiansen]

Landed in http://trac.webkit.org/changeset/132227

[Rick Byers]

(In reply to comment #60)
> Landed in http://trac.webkit.org/changeset/132227

Thanks for doing this Kenneth!  I've been thinking we want this in chromium as well, but had hesitated doing it myself because I wasn't sure if it would be difficult to get it to work correctly for print media.  Did you look at the print media cases at all?  I'm not an expert in the relevant code, but the computation of DPI strictly from deviceScaleFactor makes me think it might not do the right thing for print.

[Kenneth Rohde Christiansen]

Yes, there are other follow up patches landed already which deals with printing. Currently it assumes a printer dpi of minimum 300.