[webkit-reviews] review denied: [Bug 100865] Warn in the inspector console when using dpi and dpcm units outside of media="print" : [Attachment 172748] Patch 1

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has denied Alexander Shalamov
<alexander.shalamov at gmail.com>'s request for review:
Bug 100865: Warn in the inspector console when using dpi and dpcm units outside
of media="print"
https://bugs.webkit.org/show_bug.cgi?id=100865

Attachment 172748: Patch 1
https://bugs.webkit.org/attachment.cgi?id=172748&action=review

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=172748&action=review


>> Source/WebCore/css/MediaList.cpp:308
>> +	DEFINE_STATIC_LOCAL(String, mediaQueryMessage, (ASCIILiteral("Consider
using 'dppx' units for 'screen' media type, as the '%replacementUnits%' units
are in CSS units and not actual device units. Media query expression: ")));
> 
> Two slightly confusing things about this message:
> 1. It mentions 'screen' media type, even if you didn't explicitly target
screen (e.g. in CSS rules for all media types).
> 2. It's not obvious what the implications are of dpi being in CSS units.
> 
> How about the following text instead:
> 
> "Consider using 'dppx' units, as 'dpi' is dots-per-CSS-in, not
dots-per-physical-inch, so will not match physical pixel density of mobile
device screens. Media query expression: "
> and
> "Consider using 'dppx' units, as 'dpcm' is dots-per-CSS-cm, not
dots-per-physical-cm, so will not match physical pixel density of mobile device
screens. Media query expression: "

s/dots-per-CSS-in/dots-per-CSS-inch/ :-)
I will change mobile to "all" (mac book pro's have the same issue)

>> Source/WebCore/css/MediaList.cpp:341
>> +	    if (!query->ignored() && (equalIgnoringCase(mediaType, "screen") ||
equalIgnoringCase(mediaType, "all") || mediaType.isEmpty())) {
> 
> What about "handheld", "projection" or "tv" media types? I expect would using
dpi in such media types to suffer from the same issues...

I would do it the other way around, just not complain when using 'print'

>> Source/WebCore/css/MediaList.h:126
>> +// FIXME: should be removed when dpi / dpcm values would be deprecated for
resolution media feature.
> 
> I'm not quite sure what this FIXME means?

Just remove it