Last Comment Bug 474356 - Implement device-pixel-ratio
: Implement device-pixel-ratio
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: ---
Assigned To: Josh Matthews [:jdm] (on vacation until Dec 5)
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks: 547469 1176968
  Show dependency treegraph
 
Reported: 2009-01-19 14:22 PST by Robert O'Callahan (:roc) (email my personal email if necessary)
Modified: 2015-11-09 09:57 PST (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add -moz-device-pixel-ratio media query. (5.19 KB, patch)
2010-09-07 16:49 PDT, Josh Matthews [:jdm] (on vacation until Dec 5)
no flags Details | Diff | Splinter Review
Add -moz-device-pixel-ratio media query. (4.80 KB, patch)
2010-09-07 16:50 PDT, Josh Matthews [:jdm] (on vacation until Dec 5)
no flags Details | Diff | Splinter Review
Add -moz-device-pixel-ratio media query. (7.38 KB, patch)
2010-09-08 08:39 PDT, Josh Matthews [:jdm] (on vacation until Dec 5)
no flags Details | Diff | Splinter Review
Add -moz-device-pixel-ratio media query. (8.82 KB, patch)
2010-09-08 08:40 PDT, Josh Matthews [:jdm] (on vacation until Dec 5)
no flags Details | Diff | Splinter Review
Add -moz-device-pixel-ratio media query. (10.24 KB, patch)
2010-09-10 10:48 PDT, Josh Matthews [:jdm] (on vacation until Dec 5)
dbaron: review+
Details | Diff | Splinter Review
Add -moz-device-pixel-ratio media query. (10.65 KB, patch)
2010-09-11 06:28 PDT, Josh Matthews [:jdm] (on vacation until Dec 5)
josh: review+
roc: approval2.0+
Details | Diff | Splinter Review
Add -moz-device-pixel-ratio media query. a=approval2.0 (10.74 KB, patch)
2010-09-13 11:29 PDT, Josh Matthews [:jdm] (on vacation until Dec 5)
josh: review+
Details | Diff | Splinter Review

Description Robert O'Callahan (:roc) (email my personal email if necessary) 2009-01-19 14:22:15 PST
We should expose the ratio of CSS pixels to device pixels as a media query value in nsMediaFeatures. Webkit does, and it's useful.
Comment 1 David Baron :dbaron: ⌚️UTC-10 2009-01-19 17:07:45 PST
This should be implemented with a -moz- prefix unless it's in the spec.  Or has WebKit shipped an implementation without a prefix?  If so, we may as well copy what they did, as long as we're careful to do the same thing.
Comment 2 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-01-19 17:21:46 PST
I think they used a -webkit prefix.

BTW Hyatt said they'd document it here:
http://webkit.org/specs/MediaQueriesExtensions.html
although it's not there yet.
Comment 3 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-09-07 11:45:30 PDT
We need this!
Comment 4 Josh Matthews [:jdm] (on vacation until Dec 5) 2010-09-07 16:49:13 PDT
Created attachment 472831 [details] [diff] [review]
Add -moz-device-pixel-ratio media query.


All tests pass; thanks for the help.
Comment 5 Josh Matthews [:jdm] (on vacation until Dec 5) 2010-09-07 16:50:44 PDT
Created attachment 472833 [details] [diff] [review]
Add -moz-device-pixel-ratio media query.


Whoops, qref.
Comment 6 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-09-07 16:59:37 PDT
Webkit actually exposes a float here instead of a media-query ratio. I think we should probably follow suit. It looks like ratios in the current spec are always actual width-to-height ratios, but this isn't.

It looks like Webkit's 'ratio' is the number of device pixels per CSS pixel.
Comment 7 David Baron :dbaron: ⌚️UTC-10 2010-09-07 20:07:26 PDT
It would probably be good to have parsing tests in test_media_queries.html, even if the serious functionality tests need to be separate.
Comment 8 Josh Matthews [:jdm] (on vacation until Dec 5) 2010-09-08 08:39:33 PDT
Created attachment 473064 [details] [diff] [review]
Add -moz-device-pixel-ratio media query.

Comments addressed, now with more tests.
Comment 9 Josh Matthews [:jdm] (on vacation until Dec 5) 2010-09-08 08:40:51 PDT
Created attachment 473065 [details] [diff] [review]
Add -moz-device-pixel-ratio media query.

Sigh, qref.
Comment 10 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-09-08 13:15:46 PDT
In the tests, you probably want to check nsIDOMWindowUtils::screenPixelsPerCSSPixel at the start. If it's not 1.0 (e.g. the tests are running zoomed by default, or there's an extra scale factor being applied above fullZoom) we still want the tests to pass. Probably the best thing to do is to document.write out the <style> block with adjusted expected device-pixel-ratio values.

I would have thought that if we set fullZoom to 2, device-pixel-ratio is 2. Indeed,
+  float ratio = float(aPresContext->AppUnitsPerDevPixel()) /
+      aPresContext->AppUnitsPerCSSPixel();
is upside down; it's returning the number of CSS pixels per device pixel. aPresContext->CSSPixelsToDevPixels(1.0) would be the easiest way to write this.
Comment 11 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-09-08 13:16:02 PDT
BTW, dbaron is the one who needs to do the real review here.
Comment 12 Josh Matthews [:jdm] (on vacation until Dec 5) 2010-09-10 10:48:14 PDT
Created attachment 474121 [details] [diff] [review]
Add -moz-device-pixel-ratio media query.
Comment 13 David Baron :dbaron: ⌚️UTC-10 2010-09-10 11:23:54 PDT
Comment on attachment 474121 [details] [diff] [review]
Add -moz-device-pixel-ratio media query.

>diff -r e024bccd4df5 -r 1caccfebdc75 layout/style/nsCSSStyleSheet.cpp
>+        NS_ASSERTION(mFeature->mValueType || nsMediaFeature::eFloat,
>+                     "bad actual float value");

Drop these two lines.

>+            nsAutoString buffer;
>+            buffer.AppendFloat(expr.mValue.GetFloatValue());
>+            aString.Append(buffer);

Replace these three lines with:
  // Use 'line-height' as a property that takes float values
  // written in the normal way.
  expr.mValue.AppendToString(eCSSProperty_line_height, aString);

>diff -r e024bccd4df5 -r 1caccfebdc75 layout/style/nsMediaFeatures.cpp
>@@ -378,6 +386,13 @@
> 
>     // Mozilla extensions
>     {
>+      &nsGkAtoms::_moz_device_pixel_ratio,
>+      nsMediaFeature::eMinMaxAllowed,
>+      nsMediaFeature::eFloat,
>+      { nsnull },
>+      GetDevicePixelRatio
>+    },

Please indent this the same way as the surrounding lines.

And please add your new GetDevicePixelRatio function between GetGrid and GetSystemMetric to match the ordering here.


>diff -r e024bccd4df5 -r 1caccfebdc75 layout/style/test/test_media_queries.html
>--- a/layout/style/test/test_media_queries.html	Fri Sep 10 10:09:29 2010 -0400
>+++ b/layout/style/test/test_media_queries.html	Fri Sep 10 13:45:24 2010 -0400
>@@ -26,6 +26,13 @@
> 
> var iframe;
> 
>+function getZoomRatio() {
>+  netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
>+  var utils = window.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
>+                    .getInterface(Components.interfaces.nsIDOMWindowUtils);
>+  return utils ? utils.screenPixelsPerCSSPixel : 1.0;

Don't have a failure case here.  If |utils| is null, the test should throw an exception rather than report incorrect results.

>@@ -329,6 +336,22 @@
>   should_apply("not all and (max-device-aspect-ratio: " + low_dar_2 + ")");
>   expression_should_not_be_parseable("max-device-aspect-ratio");
> 
>+  var real_dpr = 1.0 * getZoomRatio();
>+  var high_dpr = 1.1 * getZoomRatio();
>+  var low_dpr = 0.9 * getZoomRatio();
>+  should_apply("all and (max--moz-device-pixel-ratio: " + real_dpr + ")");
>+  should_apply("all and (min--moz-device-pixel-ratio: " + real_dpr + ")");
>+  should_not_apply("not all and (max--moz-device-pixel-ratio: " + real_dpr + ")");
>+  should_not_apply("not all and (min--moz-device-pixel-ratio: " + real_dpr + ")");

>+  should_not_apply("all and (max--moz-device-pixel-ratio: " + low_dpr + ")");
>+  should_not_apply("all and (min--moz-device-pixel-ratio: " + high_dpr + ")");
>+  should_apply("not all and (max--moz-device-pixel-ratio: " + low_dpr + ")");
>+  should_apply("not all and (min--moz-device-pixel-ratio: " + high_dpr + ")");

You should probably have some should_apply tests for max/high and min/low

>+  should_apply("(-moz-device-pixel-ratio: " + real_dpr + ")");

Should probably test should_not_apply with this against high/low.

>+  should_apply("(-moz-device-pixel-ratio)");


r=dbaron with that
Comment 14 Josh Matthews [:jdm] (on vacation until Dec 5) 2010-09-11 06:28:38 PDT
Created attachment 474365 [details] [diff] [review]
Add -moz-device-pixel-ratio media query.
Comment 15 Josh Matthews [:jdm] (on vacation until Dec 5) 2010-09-11 06:29:01 PDT
Comment on attachment 474365 [details] [diff] [review]
Add -moz-device-pixel-ratio media query.

Carrying over r+.
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-09-11 07:56:29 PDT
Comment on attachment 474365 [details] [diff] [review]
Add -moz-device-pixel-ratio media query.

Let's get this in!
Comment 18 Eric Shepherd [:sheppy] 2010-09-12 21:21:54 PDT
Thanks, teoli. This woulda been missed without your nice catch, since it wasn't flagged for docs.
Comment 19 Josh Matthews [:jdm] (on vacation until Dec 5) 2010-09-13 10:55:24 PDT
This actually bounced the first time due to a test problem, and I'm planning to land it later today.
Comment 20 Josh Matthews [:jdm] (on vacation until Dec 5) 2010-09-13 11:29:47 PDT
Created attachment 474754 [details] [diff] [review]
Add -moz-device-pixel-ratio media query.  a=approval2.0
Comment 21 Josh Matthews [:jdm] (on vacation until Dec 5) 2010-09-13 11:30:33 PDT
Comment on attachment 474754 [details] [diff] [review]
Add -moz-device-pixel-ratio media query.  a=approval2.0

Carrying over r+ and a+.  This is the fixed version that shouldn't bounce this time.
Comment 22 Josh Matthews [:jdm] (on vacation until Dec 5) 2010-09-13 13:24:57 PDT
http://hg.mozilla.org/mozilla-central/rev/7060ac6871d1
Comment 23 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-09-13 19:02:43 PDT
Great stuff, thanks!
Comment 24 David Baron :dbaron: ⌚️UTC-10 2011-11-30 16:35:02 PST
We could actually have avoided adding a new feature here by adding the 'dppx' unit proposed in:
http://dev.w3.org/csswg/css3-images/#resolution-units and just using queries against resolution in dppx units.
Comment 25 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-02-06 12:20:59 PST
Yes, you're right.
Comment 26 fantasai 2012-04-02 16:59:43 PDT
Filed bug 741644 on adding dppx. Do we also want a bug on removing this once that's landed or are we planning to keep device-pixel-ratio indefinitely?

Note You need to log in before you can comment on or make changes to this bug.