Fix incorrect evaluation of resolution media queries

Enable resolution media queries with fixed implementation
    
The fixes to the original implementation are partly based on a
patch by Rune Lillesveen <rune@opera.com>;

The implementation used the physical resolution to evaluate the
resolution media features. Changed to use the actual CSS resolution,
also known as the device-pixel-ratio, instead. Unified the code for
evaluating the resolution and device-pixel-ratio media features.

Intent to ship thread:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/odQrXmT5vHk

BUG=158744, 230393
WKBUG=114029
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=150142

[Peter Beverloo, 2013-04-23 16:12:32]

+johnme, rune

The RESOLUTION_MEDIA_QUERY flag is still disabled, isn't it?  Do you intend to
implement it?  If so, it should probably have an intent to ship.

[johnme, 2013-04-23 18:16:23]

Nice cleanup! Did you remove the non-square pixels checks on grounds that
WebKit/Blink don't support non-square pixels anyway? I just have one question
about getFloatValue:

https://codereview.chromium.org/14408004/diff/5001/Source/core/css/MediaQuery...
File Source/core/css/MediaQueryEvaluator.cpp (right):

https://codereview.chromium.org/14408004/diff/5001/Source/core/css/MediaQuery...
Source/core/css/MediaQueryEvaluator.cpp:309: floorf(0.5 + 100 *
resolution->getFloatValue(CSSPrimitiveValue::CSS_DPPX)) / 100, op);
I don't understand this line. A resolution for which isResolution is true can be
of type DPCM or DPI in addition to DPPX. So how does
resolution->getFloatValue(CSSPrimitiveValue::CSS_DPPX) work? getFloatValue
doesn't seem to do any unit conversions, unless I'm missing something?

[kenneth.r.christiansen, 2013-04-23 19:30:23]

On 2013/04/23 18:16:23, johnme wrote:
> Nice cleanup! Did you remove the non-square pixels checks on grounds that
> WebKit/Blink don't support non-square pixels anyway?

Yes

> Source/core/css/MediaQueryEvaluator.cpp:309: floorf(0.5 + 100 *
> resolution->getFloatValue(CSSPrimitiveValue::CSS_DPPX)) / 100, op);
> I don't understand this line. A resolution for which isResolution is true can
be
> of type DPCM or DPI in addition to DPPX. So how does
> resolution->getFloatValue(CSSPrimitiveValue::CSS_DPPX) work? getFloatValue
> doesn't seem to do any unit conversions, unless I'm missing something?

They get converted to DPPX. So if the original value is in DPI, say 96dpi it
will return 1.0.

Something like 85dpcm will return 2.2489583333333334 and thus never match 2.25,
unless compared with less precision. As the conversion is 2 decimal (DPI / 2.54)
comparing to a value of 2 decimal points (2.25 version 2.248...) seems
appropiate. The spec recommends to use integer values for DPPX, but as we all
know it is common that devices use at least up to two decimal points (OK, I am
ignoring the crazy DPPX of the Nexus 7 :-)).

[kenneth.r.christiansen, 2013-04-23 19:31:39]

On 2013/04/23 16:12:32, Peter Beverloo wrote:
> +johnme, rune
> 
> The RESOLUTION_MEDIA_QUERY flag is still disabled, isn't it?  Do you intend to
> implement it?  If so, it should probably have an intent to ship.

Couldn't that be done later? There are some issues with reporting bad usage that
Shalamov is currently looking into. When that is fixed we can do the intent to
ship.

[rune, 2013-04-23 21:25:29]

On 2013/04/23 19:31:39, kenneth.r.christiansen wrote:
> On 2013/04/23 16:12:32, Peter Beverloo wrote:
> > +johnme, rune
> > 
> > The RESOLUTION_MEDIA_QUERY flag is still disabled, isn't it?  Do you intend
to
> > implement it?  If so, it should probably have an intent to ship.
> 
> Couldn't that be done later? There are some issues with reporting bad usage
that
> Shalamov is currently looking into. When that is fixed we can do the intent to
> ship.

Are you talking about addResolutionWarningMessageToConsole? I was a bit unsure
if that was a good thing to have there, and part of why I hesitated to remove
the ifdefs in the WebKit patch.

[rune, 2013-04-23 21:55:04]

On 2013/04/23 19:30:23, kenneth.r.christiansen wrote:
> On 2013/04/23 18:16:23, johnme wrote:
> > Nice cleanup! Did you remove the non-square pixels checks on grounds that
> > WebKit/Blink don't support non-square pixels anyway?
> 
> Yes
> 
> > Source/core/css/MediaQueryEvaluator.cpp:309: floorf(0.5 + 100 *
> > resolution->getFloatValue(CSSPrimitiveValue::CSS_DPPX)) / 100, op);
> > I don't understand this line. A resolution for which isResolution is true
can
> be
> > of type DPCM or DPI in addition to DPPX. So how does
> > resolution->getFloatValue(CSSPrimitiveValue::CSS_DPPX) work? getFloatValue
> > doesn't seem to do any unit conversions, unless I'm missing something?
> 
> They get converted to DPPX. So if the original value is in DPI, say 96dpi it
> will return 1.0.
> 
> Something like 85dpcm will return 2.2489583333333334 and thus never match
2.25,
> unless compared with less precision. As the conversion is 2 decimal (DPI /
2.54)
> comparing to a value of 2 decimal points (2.25 version 2.248...) seems
> appropiate. The spec recommends to use integer values for DPPX, but as we all
> know it is common that devices use at least up to two decimal points (OK, I am
> ignoring the crazy DPPX of the Nexus 7 :-)).

I can understand the rounding for dpcm since it's hard to get an exact match,
and probably impossible for some values due to float precision, but it feels
wrong to get a match for (resolution: 216.001dpi) or (resolution: 2.251dppx).

Could the rounding be limited to dpcm only?

[kenneth.r.christiansen, 2013-04-25 10:01:26]

> Are you talking about addResolutionWarningMessageToConsole? I was a bit unsure
> if that was a good thing to have there, and part of why I hesitated to remove
> the ifdefs in the WebKit patch.

It was requested by Google employees at some point if I am not mistaken. They
are only applied for screen.

[kenneth.r.christiansen, 2013-04-25 10:01:50]

> Could the rounding be limited to dpcm only?

Sure thing.

[rune, 2013-04-25 10:44:32]

On 2013/04/25 10:01:26, kenneth.r.christiansen wrote:
> > Are you talking about addResolutionWarningMessageToConsole? I was a bit
unsure
> > if that was a good thing to have there, and part of why I hesitated to
remove
> > the ifdefs in the WebKit patch.
> 
> It was requested by Google employees at some point if I am not mistaken. They
> are only applied for screen.

I'm fine with it.

[johnme, 2013-04-25 10:46:37]

On 2013/04/23 19:30:23, kenneth.r.christiansen wrote:
> On 2013/04/23 18:16:23, johnme wrote:
> > how does resolution->getFloatValue(CSSPrimitiveValue::CSS_DPPX) work?
> 
> They get converted to DPPX. So if the original value is in DPI, say 96dpi it
> will return 1.0.

Ah, I see. I noticed that conversionToCanonicalUnitsScaleFactor in
CSSPrimitiveValue.cpp doesn't have an entry for DPPX. That's fine, since the
default case works, but for consistency with other unit types which explicitly
mention their canonical unit, we should probably add an entry at some point.

On 2013/04/25 10:01:26, kenneth.r.christiansen wrote:
> > Are you talking about addResolutionWarningMessageToConsole? I was a bit
unsure
> > if that was a good thing to have there, and part of why I hesitated to
remove
> > the ifdefs in the WebKit patch.
> 
> It was requested by Google employees at some point if I am not mistaken. They
> are only applied for screen.

Heh, it was originally my suggestion. Basically I was worried that developers
would look up say the iPhone 4 spec sheet, see that it's 326 dpi, and hence
write:

    @media (min-resolution: 320dpi) { /* retina stuff */ }

But of course this won't work because the CSS dpi of a Retina iPhone is 2*96 =
192 (a fact probably only realized by a few dozen people like us in the world).

In the spirit of not giving developers foot-guns, it would hence be best to only
allow the dppx unit. However someone pointed out that for print media, CSS dpi
does refer to the physical resolution and is actually useful.

So the warning in screen media was a compromise, to discourage use of dpi and
dpcm except in print media.

One slight downside of the warning, is that some older browsers currently
support the resolution MQ, but only with dpi (and maybe dpcm) units. So for
cross-browser compatibility it can currently be useful to use dpi units in
screen media, as long as you understand what you're doing. Perhaps we should
skip showing the warning if a dppx expression is included in the same media
query?

[kenneth.r.christiansen, 2013-04-29 11:30:47]

> One slight downside of the warning, is that some older browsers currently
> support the resolution MQ, but only with dpi (and maybe dpcm) units. So for
> cross-browser compatibility it can currently be useful to use dpi units in
> screen media, as long as you understand what you're doing. Perhaps we should
> skip showing the warning if a dppx expression is included in the same media
> query?

I don't get what you mean

[johnme, 2013-04-29 11:36:14]

On 2013/04/29 11:30:47, kenneth.r.christiansen wrote:
> I don't get what you mean

See:
- http://www.w3.org/blog/CSS/2012/06/14/unprefix-webkit-device-pixel-ratio/
-
http://www.brettjankord.com/2012/11/28/cross-browser-retinahigh-resolution-me...

For example, the only way to write "min-resolution:2dppx" that IE9 understands
is "min-resolution:192dpi" (because it doesn't support device-pixel-ratio or
dppx). It's unfortunate to show the warning if authors are using this correctly
for cross-browser compatibility; yet it would be nice to keep the warning for
authors who are shooting themselves in the foot.

[kenneth.r.christiansen, 2013-04-29 12:01:18]

> For example, the only way to write "min-resolution:2dppx" that IE9 understands
> is "min-resolution:192dpi" (because it doesn't support device-pixel-ratio or
> dppx). It's unfortunate to show the warning if authors are using this
correctly
> for cross-browser compatibility; yet it would be nice to keep the warning for
> authors who are shooting themselves in the foot.

So if someone writes (resolution: 96dpi) you don't want to emit a warning but if
they write (resolution: 160) you want to?

[johnme, 2013-04-29 13:06:54]

On 2013/04/29 12:01:18, kenneth.r.christiansen wrote:
> So if someone writes (resolution: 96dpi) you don't want to emit a warning but
if
> they write (resolution: 160) you want to?

Yes (assuming you mean 160dpi not 160). An arbitrary threshold clearly isn't
future-proof. It seems we have 4 options:

1. Keep the warning, and accept that it will trigger slightly too often.

2. Remove the warning, and accept that lots of devs will trip up on this.

3. Only warn when the dpi is a multiple of 80 (e.g. we'd warn for 160, 240 and
320, corresponding to popular 1x, 1.5x and 2x devices).

4. Only warn if there isn't a matching dppx value in the same expression. For
example we'd warn for:
    (resolution: 192dpi)
and for:
    (resolution: 320dpi), (resolution: 2dppx)
but not for:
    (resolution: 192dpi), (resolution: 2dppx)
because 192dpi == 2dppx.

I think #4 is probably the best option. It'll catch 100% of cases where people
are misusing CSS dpi, but provides a clean way of avoiding the warning for users
that have understood, and encourages use of the more intuitive dppx unit.

[johnme, 2013-04-29 13:25:45]

Patch lgtm by the way, apart from the comment below. We'll need to get approval
from an OWNER too - I've added abarth.

We can work out what to do about the warning later...

https://codereview.chromium.org/14408004/diff/19001/Source/core/testing/Inter...
File Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/14408004/diff/19001/Source/core/testing/Inter...
Source/core/testing/Internals.cpp:1778: page->setDeviceScaleFactor(scaleFactor);
Shouldn't you restore the original device scale factor in
Internals::resetToConsistentState?

[kenneth.r.christiansen, 2013-04-29 14:28:31]

>
https://codereview.chromium.org/14408004/diff/19001/Source/core/testing/Inter...
> Source/core/testing/Internals.cpp:1778:
page->setDeviceScaleFactor(scaleFactor);
> Shouldn't you restore the original device scale factor in
> Internals::resetToConsistentState?

If we start testing with bots using a dppx of 2 by default that would make
sense. Unfortunately, adding it to InternalSettings::Backup seems a bit weird as
it is really setting oriented and this is not really a settings. I could do that
though if an owner thinks that is the proper way; if not I would like to keep it
as it is now.

Adam what do you think?

[johnme, 2013-04-29 14:40:46]

Ok, that's probably fine for now. Though the page()->setDeviceScaleFactor(1);
should probably still go in Internals::resetToConsistentState() instead of
InternalSettings::resetToConsistentState() ?

[kenneth.r.christiansen, 2013-04-29 16:17:10]

On 2013/04/29 14:40:46, johnme wrote:
> Ok, that's probably fine for now. Though the page()->setDeviceScaleFactor(1);
> should probably still go in Internals::resetToConsistentState() instead of
> InternalSettings::resetToConsistentState() ?

Done

[abarth-chromium, 2013-04-29 20:13:02]

Yeah, resetToConsistentState() is the right place.  Looking at the details now.

[abarth-chromium, 2013-04-29 20:16:15]

I'm relying on johnme for the CSS details, but the rest LGTM.

[commit-bot: I haz the power, 2013-04-29 20:16:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kenneth.r.christiansen@intel.com/14408...

[commit-bot: I haz the power, 2013-04-29 21:09:06]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[kenneth.r.christiansen, 2013-05-07 07:11:52]

Any chance for a new review?

[eseidel, 2013-05-07 07:15:36]

I've read through the patch, but I feel like I don't have quite enough context
to give you a good review.  I assume this is implementing the Intent To
Implement from last week.  The code looks reasonable, but someone more familiar
with this spec would make a better reviewer here.

[rune, 2013-05-07 07:28:55]

On 2013/05/07 07:11:52, kenneth.r.christiansen wrote:
> Any chance for a new review?

(unauthorized) lgtm

[eseidel, 2013-05-07 07:32:38]

Could you link to the WK bug in the description?  (And maybe give a bit more
context for the details of the change, like you would in a WK ChangeLog?)

[Peter Beverloo, 2013-05-07 13:08:50]

John, could you have another look please?  The patch now also enables the
resolution media query feature.

Some comments about the description:
  * Please clarify in the title that the feature will be *enabled*.  That's the
most critical thing this patch does.
  * Since this ships a feature, refer to the thread (most significantly Eric's
e-mail) which mentions the API owners agreed and you got your LGTMs.
  * nit: You can collapse the two BUG= lines by separating them by commas, i.e.
"BUG=158744, 230393".

[johnme, 2013-05-07 15:46:29]

Updated patch LGTM. I noticed an interesting comment about the naming of the
dppx unit. It would be good if Tab could confirm its current status, but now
that other browsers have started shipping it, there's probably little harm in us
supporting it too.

https://codereview.chromium.org/14408004/diff/47001/Source/core/css/CSSParser...
File Source/core/css/CSSParser.cpp (right):

https://codereview.chromium.org/14408004/diff/47001/Source/core/css/CSSParser...
Source/core/css/CSSParser.cpp:9910: // Keep this compile time guard in place
until that is resolved.
Hmm. Tab, can you comment on the current status of the dppx unit?

[kenneth.r.christiansen, 2013-05-07 15:49:49]

> Source/core/css/CSSParser.cpp:9910: // Keep this compile time guard in place
> until that is resolved.
> Hmm. Tab, can you comment on the current status of the dppx unit?

Firefox ships with support for it and Opera as well as far as I can tell.

[TabAtkins, 2013-05-08 19:22:16]

On 2013/05/07 15:46:29, johnme wrote:
> Hmm. Tab, can you comment on the current status of the dppx unit?

As Ken said, the dppx unit is in FF and old Opera.  CSSWG is planning on keeping
it.

[kenneth.r.christiansen, 2013-05-10 10:29:06]

Eric, Adam, could any of you rubberstamp this patch now?

[darktears, 2013-05-10 11:44:44]

On 2013/05/10 10:29:06, kenneth.r.christiansen wrote:
> Eric, Adam, could any of you rubberstamp this patch now?

Based on all the comments and reviews, lgtm.

[commit-bot: I haz the power, 2013-05-10 11:45:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kenneth.r.christiansen@intel.com/14408...

[commit-bot: I haz the power, 2013-05-10 11:45:29]

Failed to apply patch for Source/core/css/MediaQueryEvaluator.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/css/MediaQueryEvaluator.cpp
  Hunk #1 FAILED at 38.
  Hunk #3 succeeded at 273 (offset 5 lines).
  Hunk #4 FAILED at 281.
  2 out of 4 hunks FAILED -- saving rejects to file
Source/core/css/MediaQueryEvaluator.cpp.rej

Patch:       Source/core/css/MediaQueryEvaluator.cpp
Index: Source/core/css/MediaQueryEvaluator.cpp
diff --git a/Source/core/css/MediaQueryEvaluator.cpp
b/Source/core/css/MediaQueryEvaluator.cpp
index
db54b7745bd630e9548e8c5356a84526e4eac674..80f69690b0b6a899b63a3d8923fb9bef8f9622ae
100644
--- a/Source/core/css/MediaQueryEvaluator.cpp
+++ b/Source/core/css/MediaQueryEvaluator.cpp
@@ -38,6 +38,7 @@
 #include "core/css/MediaQuery.h"
 #include "core/css/MediaQueryExp.h"
 #include "core/css/StyleResolver.h"
+#include "core/css/CSSHelper.h"
 #include "core/dom/NodeRenderStyle.h"
 #include "core/page/Chrome.h"
 #include "core/page/ChromeClient.h"
@@ -195,23 +196,6 @@ static bool compareAspectRatioValue(CSSValue* value, int
width, int height, Medi
     return false;
 }
 
-#if ENABLE(RESOLUTION_MEDIA_QUERY)
-static bool compareResolution(float min, float max, float value,
MediaFeaturePrefix op)
-{
-    switch (op) {
-    case NoPrefix:
-        // A 'resolution' (without a "min-" or "max-" prefix) query
-        // never matches a device with non-square pixels.
-        return value == min && value == max;
-    case MinPrefix:
-        return min >= value;
-    case MaxPrefix:
-        return max <= value;
-    }
-    return false;
-}
-#endif
-
 static bool numberValue(CSSValue* value, float& result)
 {
     if (value->isPrimitiveValue()
@@ -285,7 +269,7 @@ static bool device_aspect_ratioMediaFeatureEval(CSSValue*
value, RenderStyle*, F
     return true;
 }
 
-static bool device_pixel_ratioMediaFeatureEval(CSSValue *value, RenderStyle*,
Frame* frame, MediaFeaturePrefix op)
+static bool evalResolution(CSSValue* value, Frame* frame, MediaFeaturePrefix
op)
 {
     // FIXME: Possible handle other media types than 'screen' and 'print'.
     float deviceScaleFactor = 0;
@@ -298,117 +282,48 @@ static bool device_pixel_ratioMediaFeatureEval(CSSValue
*value, RenderStyle*, Fr
     if (equalIgnoringCase(mediaType, "screen"))
         deviceScaleFactor = frame->page()->deviceScaleFactor();
     else if (equalIgnoringCase(mediaType, "print")) {
-        // The resolution of images while printing should not depend on the dpi
+        // The resolution of images while printing should not depend on the DPI
         // of the screen. Until we support proper ways of querying this info
         // we use 300px which is considered minimum for current printers.
-        deviceScaleFactor = 3.125; // 300dpi / 96dpi;
+        deviceScaleFactor = 300 / cssPixelsPerInch;
     }
 
     if (!value)
         return !!deviceScaleFactor;
 
-    return value->isPrimitiveValue() && compareValue(deviceScaleFactor,
static_cast<CSSPrimitiveValue*>(value)->getFloatValue(), op);
-}
-
-static bool resolutionMediaFeatureEval(CSSValue* value, RenderStyle*, Frame*
frame, MediaFeaturePrefix op)
-{
-#if ENABLE(RESOLUTION_MEDIA_QUERY)
-    // The DPI below is dots per CSS inch and thus not device inch. The
-    // functions should respect this.
-    //
-    // For square pixels, it is simply the device scale factor (dppx) times 96,
-    // per definition.
-    //
-    // The device scale factor is a predefined value which is calculated per
-    // device given the preferred distance in arms length (considered one arms
-    // length for desktop computers and usually 0.6 arms length for phones).
-    //
-    // The value can be calculated as follows (rounded to quarters):
-    //     round((deviceDotsPerInch * distanceInArmsLength / 96) * 4) / 4.
-    // Example (mid-range resolution phone):
-    //     round((244 * 0.6 / 96) * 4) / 4 = 1.5
-    // Example (high-range resolution laptop):
-    //     round((220 * 1.0 / 96) * 4) / 4 = 2.0
-
-    float horiDPI;
-    float vertDPI;
-
-    // This checks the actual media type applied to the document, and we know
-    // this method only got called if this media type matches the one defined
-    // in the query. Thus, if if the document's media type is "print", the
-    // media type of the query will either be "print" or "all".
-    String mediaType = frame->view()->mediaType();
-    if (equalIgnoringCase(mediaType, "screen")) {
-        Screen* screen = frame->document()->domWindow()->screen();
-        horiDPI = screen->horizontalDPI();
-        vertDPI = screen->verticalDPI();
-    } else if (equalIgnoringCase(mediaType, "print")) {
-        // The resolution of images while printing should not depend on the dpi
-        // of the screen. Until we support proper ways of querying this info
-        // we use 300px which is considered minimum for current printers.
-        horiDPI = vertDPI = 300;
-    } else {
-        // FIXME: Possible handle other media types than 'screen' and 'print'.
-        // For now, do not match.
+    if (!value->isPrimitiveValue())
         return false;
-    }
 
-    float leastDenseDPI = std::min(horiDPI, vertDPI);
-    float mostDenseDPI = std::max(horiDPI, vertDPI);
-
-    // According to spec, (resolution) will evaluate to true if (resolution:x)
-    // will evaluate to true for a value x other than zero or zero followed by
-    // a valid unit identifier (i.e., other than 0, 0dpi, 0dpcm, or 0dppx.),
-    // which is always the case. But the spec special cases 'resolution' to
-    // never matches a device with non-square pixels.
-    if (!value) {
-        ASSERT(op == NoPrefix);
-        return leastDenseDPI == mostDenseDPI;
-    }
+    CSSPrimitiveValue* resolution = static_cast<CSSPrimitiveValue*>(value);
 
-    if (!value->isPrimitiveValue())
-        return false;
+    if (resolution->isNumber())
+        return compareValue(deviceScaleFactor, resolution->getFloatValue(),
op);
 
-    // http://dev.w3.org/csswg/css3-values/#resolution defines resolution as a
-    // dimension, which contains a number (decimal point allowed), not just an
-    // integer. Also, http://dev.w3.org/csswg/css3-values/#numeric-types says
-    // "CSS theoretically supports infinite precision and infinite ranges for
-    // all value types;
-    CSSPrimitiveValue* rawValue = static_cast<CSSPrimitiveValue*>(value);
-
-    if (rawValue->isDotsPerPixel()) {
-        // http://dev.w3.org/csswg/css3-values/#absolute-lengths recommends
-        // "that the pixel unit refer to the whole number of device pixels that
-        // best approximates the reference pixel". We compare with 3 decimal
-        // points, which aligns with current device-pixel-ratio's in use.
-        float leastDenseDensity = floorf(leastDenseDPI * 1000 / 96) / 1000;
-        float mostDenseDensity = floorf(leastDenseDPI * 1000 / 96) / 1000;
-        float testedDensity =
rawValue->getFloatValue(CSSPrimitiveValue::CSS_DPPX);
-        return compareResolution(leastDenseDensity, mostDenseDensity,
testedDensity, op);
-    }
+    if (!resolution->isResolution())
+        return false;
 
-    if (rawValue->isDotsPerInch()) {
-        unsigned testedDensity =
rawValue->getFloatValue(CSSPrimitiveValue::CSS_DPI);
-        return compareResolution(leastDenseDPI, mostDenseDPI, testedDensity,
op);
+    if (resolution->isDotsPerCentimeter()) {
+        // To match DPCM to DPPX values, we limit to 2 decimal points.
+        // The http://dev.w3.org/csswg/css3-values/#absolute-lengths recommends
+        // "that the pixel unit refer to the whole number of device pixels that
best
+        // approximates the reference pixel". With that in mind, allowing 2
decimal
+        // point precision seems appropriate.
+        return compareValue(
+            floorf(0.5 + 100 * deviceScaleFactor) / 100,
+            floorf(0.5 + 100 *
resolution->getFloatValue(CSSPrimitiveValue::CSS_DPPX)) / 100, op);
     }
 
-    // http://dev.w3.org/csswg/css3-values/#absolute-lengths recommends "that
-    // the pixel unit refer to the whole number of device pixels that best
-    // approximates the reference pixel".
-    float leastDenseDPCM = roundf(leastDenseDPI / 2.54); // (2.54 cm/in)
-    float mostDenseDPCM = roundf(mostDenseDPI / 2.54);
+    return compareValue(deviceScaleFactor,
resolution->getFloatValue(CSSPrimitiveValue::CSS_DPPX), op);
+}
 
-    if (rawValue->isDotsPerCentimeter()) {
-        float testedDensity =
rawValue->getFloatValue(CSSPrimitiveValue::CSS_DPCM);
-        return compareResolution(leastDenseDPCM, mostDenseDPCM, testedDensity,
op);
-    }
-#else
-    UNUSED_PARAM(value);
-    UNUSED_PARAM(frame);
-    UNUSED_PARAM(op);
-#endif
+static bool device_pixel_ratioMediaFeatureEval(CSSValue *value, RenderStyle*,
Frame* frame, MediaFeaturePrefix op)
+{
+    return (!value || static_cast<CSSPrimitiveValue*>(value)->isNumber()) &&
evalResolution(value, frame, op);
+}
 
-    return false;
+static bool resolutionMediaFeatureEval(CSSValue* value, RenderStyle*, Frame*
frame, MediaFeaturePrefix op)
+{
+    return (!value || static_cast<CSSPrimitiveValue*>(value)->isResolution())
&& evalResolution(value, frame, op);
 }
 
 static bool gridMediaFeatureEval(CSSValue* value, RenderStyle*, Frame*,
MediaFeaturePrefix op)

[commit-bot: I haz the power, 2013-05-10 14:06:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kenneth.r.christiansen@intel.com/14408...

[commit-bot: I haz the power, 2013-05-10 14:06:58]

Failed to apply patch for Source/core/css/MediaFeatureNames.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/css/MediaFeatureNames.h
  Hunk #1 FAILED at 36.
  Hunk #2 succeeded at 51 with fuzz 1 (offset -3 lines).
  1 out of 2 hunks FAILED -- saving rejects to file
Source/core/css/MediaFeatureNames.h.rej

Patch:       Source/core/css/MediaFeatureNames.h
Index: Source/core/css/MediaFeatureNames.h
diff --git a/Source/core/css/MediaFeatureNames.h
b/Source/core/css/MediaFeatureNames.h
index
22b0c5835d5ea964a1a68740aa99059b8654cc53..006c23b9af964ed0aee4038c073adcd4338938a0
100644
--- a/Source/core/css/MediaFeatureNames.h
+++ b/Source/core/css/MediaFeatureNames.h
@@ -36,14 +36,14 @@ namespace WebCore {
     macro(orientation, "orientation") \
     macro(aspect_ratio, "aspect-ratio") \
     macro(device_aspect_ratio, "device-aspect-ratio") \
-    macro(device_pixel_ratio, "-webkit-device-pixel-ratio") \
+    macro(devicePixelRatio, "-webkit-device-pixel-ratio") \
     macro(device_height, "device-height") \
     macro(device_width, "device-width") \
     macro(max_color, "max-color") \
     macro(maxColorIndex, "max-color-index") \
     macro(max_aspect_ratio, "max-aspect-ratio") \
     macro(max_device_aspect_ratio, "max-device-aspect-ratio") \
-    macro(max_device_pixel_ratio, "-webkit-max-device-pixel-ratio") \
+    macro(maxDevicePixelRatio, "-webkit-max-device-pixel-ratio") \
     macro(max_device_height, "max-device-height") \
     macro(max_device_width, "max-device-width") \
     macro(max_height, "max-height") \
@@ -54,7 +54,7 @@ namespace WebCore {
     macro(minColorIndex, "min-color-index") \
     macro(min_aspect_ratio, "min-aspect-ratio") \
     macro(min_device_aspect_ratio, "min-device-aspect-ratio") \
-    macro(min_device_pixel_ratio, "-webkit-min-device-pixel-ratio") \
+    macro(minDevicePixelRatio, "-webkit-min-device-pixel-ratio") \
     macro(min_device_height, "min-device-height") \
     macro(min_device_width, "min-device-width") \
     macro(min_height, "min-height") \

[commit-bot: I haz the power, 2013-05-10 14:21:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kenneth.r.christiansen@intel.com/14408...

[commit-bot: I haz the power, 2013-05-10 21:12:15]

Retried try job too often on win_layout for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...

[commit-bot: I haz the power, 2013-05-10 22:09:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kenneth.r.christiansen@intel.com/14408...

[commit-bot: I haz the power, 2013-05-10 22:09:44]

Change committed as 150142