Bug 88047 - Remove support for target-densitydpi in the viewport meta tag
: Remove support for target-densitydpi in the viewport meta tag
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
: 88114
  Show dependency treegraph
 
Reported: 2012-05-31 23:10 PST by
Modified: 2012-06-05 17:16 PST (History)


Attachments
Patch (19.70 KB, patch)
2012-05-31 23:18 PST, Adam Barth
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (629.26 KB, application/zip)
2012-06-01 02:20 PST, WebKit Review Bot
no flags Details
Patch (20.26 KB, patch)
2012-06-04 15:38 PST, Adam Barth
no flags Review Patch | Details | Formatted Diff | Diff


Note

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


Description From 2012-05-31 23:10:24 PST
Remove support for target-densitydpi in the viewport meta tag
------- Comment #1 From 2012-05-31 23:18:17 PST -------
Created an attachment (id=145214) [details]
Patch
------- Comment #2 From 2012-06-01 02:20:22 PST -------
(From update of attachment 145214 [details])
Attachment 145214 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12875009

New failing tests:
WebFrameTest.DeviceScaleFactorUsesDefaultWithoutViewportTag
WebFrameTest.FixedLayoutInitializeAtMinimumPageScale
------- Comment #3 From 2012-06-01 02:20:27 PST -------
Created an attachment (id=145246) [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
------- Comment #4 From 2012-06-01 06:13:33 PST -------
(From update of attachment 145214 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=145214&action=review

> Source/WebCore/dom/ViewportArguments.cpp:64
> +    result.devicePixelRatio = deviceDPI;

how is this right?  The deviceDPI is a number that will never be 1.0 and is not a ratio.  This disrupts the rest of the meta viewport calculation.  Look at lines 68-71 (new line numbers), you're now dividing the availableWidth/Height and deviceWidth/Height by the deviceDPI.
------- Comment #5 From 2012-06-01 06:30:59 PST -------
(From update of attachment 145214 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=145214&action=review

>> Source/WebCore/dom/ViewportArguments.cpp:64
>> +    result.devicePixelRatio = deviceDPI;
> 
> how is this right?  The deviceDPI is a number that will never be 1.0 and is not a ratio.  This disrupts the rest of the meta viewport calculation.  Look at lines 68-71 (new line numbers), you're now dividing the availableWidth/Height and deviceWidth/Height by the deviceDPI.

Konrad is right - this should be deviceDPI/160.

And instead of hardcoding the value 160, it should ideally vary in correspondence with the definition of a CSS pixel (http://www.w3.org/TR/css3-values/#reference-pixel), such that it's 96 dpi on desktop monitors and laptops that you sit approximately arms length from, and 160 dpi on phones that you hold about 0.6 arm lengths away, and potentially somewhere in between for tablets. Ditto everywhere else that hardcodes 160. The embedder should probably provide this value. Feel free to hardcode it for now and do this as a new bug.

> Source/WebCore/dom/ViewportArguments.cpp:-371
> -        "Viewport target-densitydpi has to take a number between 70 and 400 as a valid target dpi, try using \"device-dpi\", \"low-dpi\", \"medium-dpi\" or \"high-dpi\" instead for future compatibility."

I wonder if (at least in the short term) it's worth adding an error message to note that target-densityDpi isn't supported (in case devs who know we used to support it are trying to debug)?
------- Comment #6 From 2012-06-01 07:10:12 PST -------
(In reply to comment #5)
> (From update of attachment 145214 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145214&action=review
> 
> >> Source/WebCore/dom/ViewportArguments.cpp:64
> >> +    result.devicePixelRatio = deviceDPI;
> > 
> > how is this right?  The deviceDPI is a number that will never be 1.0 and is not a ratio.  This disrupts the rest of the meta viewport calculation.  Look at lines 68-71 (new line numbers), you're now dividing the availableWidth/Height and deviceWidth/Height by the deviceDPI.
> 
> Konrad is right - this should be deviceDPI/160.
> 
> And instead of hardcoding the value 160, it should ideally vary in correspondence with the definition of a CSS pixel (http://www.w3.org/TR/css3-values/#reference-pixel), such that it's 96 dpi on desktop monitors and laptops that you sit approximately arms length from, and 160 dpi on phones that you hold about 0.6 arm lengths away, and potentially somewhere in between for tablets. Ditto everywhere else that hardcodes 160. The embedder should probably provide this value. Feel free to hardcode it for now and do this as a new bug.

I agree that it shouldn't be hardcoded, but wouldn't allowing the embedder to spcify a value just be a substitute for target-densityDPI?  Unless I'm misunderstanding your idea of what an embedder is.
> 
> > Source/WebCore/dom/ViewportArguments.cpp:-371
> > -        "Viewport target-densitydpi has to take a number between 70 and 400 as a valid target dpi, try using \"device-dpi\", \"low-dpi\", \"medium-dpi\" or \"high-dpi\" instead for future compatibility."
> 
> I wonder if (at least in the short term) it's worth adding an error message to note that target-densityDpi isn't supported (in case devs who know we used to support it are trying to debug)?

I've got mixed feelings about error messages.  I'm not against it but, want to just mention that we've had warnings about ; usage in the meta viewport for a while now.  I'm not sure how many web authors know about or even look at these errors/warnings.
------- Comment #7 From 2012-06-01 08:07:55 PST -------
(In reply to comment #6)
> (In reply to comment #5)
> > And instead of hardcoding the value 160, it should ideally vary in correspondence with the definition of a CSS pixel (http://www.w3.org/TR/css3-values/#reference-pixel), such that it's 96 dpi on desktop monitors and laptops that you sit approximately arms length from, and 160 dpi on phones that you hold about 0.6 arm lengths away, and potentially somewhere in between for tablets. Ditto everywhere else that hardcodes 160. The embedder should probably provide this value. Feel free to hardcode it for now and do this as a new bug.
> 
> I agree that it shouldn't be hardcoded, but wouldn't allowing the embedder to spcify a value just be a substitute for target-densityDPI?  Unless I'm misunderstanding your idea of what an embedder is.

By embedder I mean the WebKit port (not web pages), via something like WebCore/page/ChromeClient.h

> > I wonder if (at least in the short term) it's worth adding an error message to note that target-densityDpi isn't supported (in case devs who know we used to support it are trying to debug)?
> 
> I've got mixed feelings about error messages.  I'm not against it but, want to just mention that we've had warnings about ; usage in the meta viewport for a while now.  I'm not sure how many web authors know about or even look at these errors/warnings.

A reasonable number of developers will use the error console at least occasionally; I've certainly been enlightened several times by the ; error messages when debugging other people's pages, though I'm not your typical webdev :)
------- Comment #8 From 2012-06-01 08:59:55 PST -------
We use the value 160 elsewhere in WebCore.  I'll find them and replace them with a constant.  I don't think there's any value in letting the WebKit layer specify the value since everyone is just going to specify 160 anyway.
------- Comment #9 From 2012-06-01 09:03:47 PST -------
After discussing this internally we believe that while the current implementation of target-densitydpi is not ideal it's ability to allow you scale your viewport to a given target density as well as to allow you to not scale (deviceDPI) are both desired features of this API that we'd like to have available to developers.

This is useful for 2 cases:
1) automatically scaling content to the AutoValue (ie 160) DPI, which is what most mobile optimized sites do
2) allowing the user to have pixel perfect rendering on a given device.
------- Comment #10 From 2012-06-01 09:04:44 PST -------
(In reply to comment #9)
> After discussing this internally we believe that while the current implementation of target-densitydpi is not ideal it's ability to allow you scale your viewport to a given target density as well as to allow you to not scale (deviceDPI) are both desired features of this API that we'd like to have available to developers.
> 
> This is useful for 2 cases:
> 1) automatically scaling content to the AutoValue (ie 160) DPI, which is what most mobile optimized sites do
> 2) allowing the user to have pixel perfect rendering on a given device.

By user and you I mean web author.
------- Comment #11 From 2012-06-01 09:06:18 PST -------
@Konrad: If you'd like to argue for retaining this feature, please reply to the webkit-dev thread on this topic.  I'd prefer to use the bug thread for discussing technical details of the patch.
------- Comment #12 From 2012-06-02 05:44:16 PST -------
(From update of attachment 145214 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=145214&action=review

r- due to the devicePixelRation = deviceDPI error

>>> Source/WebCore/dom/ViewportArguments.cpp:-371
>>> -        "Viewport target-densitydpi has to take a number between 70 and 400 as a valid target dpi, try using \"device-dpi\", \"low-dpi\", \"medium-dpi\" or \"high-dpi\" instead for future compatibility."
>> 
>> I wonder if (at least in the short term) it's worth adding an error message to note that target-densityDpi isn't supported (in case devs who know we used to support it are trying to debug)?
> 
> I've got mixed feelings about error messages.  I'm not against it but, want to just mention that we've had warnings about ; usage in the meta viewport for a while now.  I'm not sure how many web authors know about or even look at these errors/warnings.

I think a warning would be fine. It shouldn't hurt anyone.
------- Comment #13 From 2012-06-04 15:38:51 PST -------
Created an attachment (id=145643) [details]
Patch
------- Comment #14 From 2012-06-04 15:44:23 PST -------
(From update of attachment 145643 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=145643&action=review

Maybe some blackberry guy wants to take a look at the changes to their code?

> Source/WebCore/dom/ViewportArguments.cpp:333
> +        "Viewport target-densitydpi is not supported.",

anymore? Maybe make it clear that we have no intention of supporting it either?
------- Comment #15 From 2012-06-04 15:46:07 PST -------
(In reply to comment #14)
> (From update of attachment 145643 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145643&action=review
> 
> Maybe some blackberry guy wants to take a look at the changes to their code?
> 
> > Source/WebCore/dom/ViewportArguments.cpp:333
> > +        "Viewport target-densitydpi is not supported.",
> 
> anymore? Maybe make it clear that we have no intention of supporting it either?

Generally we try to avoid forward-looking statements because they make lawyers sad.
------- Comment #16 From 2012-06-04 15:50:24 PST -------
+cc a few people who may care to comment on this before it lands.
------- Comment #17 From 2012-06-04 15:56:50 PST -------
(In reply to comment #11)
> @Konrad: If you'd like to argue for retaining this feature, please reply to the webkit-dev thread on this topic.  I'd prefer to use the bug thread for discussing technical details of the patch.

Paste from email just sent to webkit-dev:
it would still be useful for ports to set their own target density. I know currently 160 is what the auto value is and all content is scaled to this DPI it would be nice to have the option to not scale content, even if it is for just the embedder to decide.

We want this on BlackBerry, and I'm sure that there are others who may find this useful.
------- Comment #18 From 2012-06-04 15:58:26 PST -------
I believe that's https://bugs.webkit.org/show_bug.cgi?id=88114
------- Comment #19 From 2012-06-04 16:10:40 PST -------
(In reply to comment #18)
> I believe that's https://bugs.webkit.org/show_bug.cgi?id=88114

I believe that 88114 depends on this bug and should be tracked as such. I wasn't aware that there were such plans.
------- Comment #20 From 2012-06-05 14:37:13 PST -------
(From update of attachment 145643 [details])
Clearing flags on attachment: 145643

Committed r119527: <http://trac.webkit.org/changeset/119527>
------- Comment #21 From 2012-06-05 14:37:21 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #22 From 2012-06-05 17:06:47 PST -------
This patch caused a chromium os build failure when we rolled webkit because it left a chunk of redundant code. I removed it in http://trac.webkit.org/changeset/119534

Please review & verify the correctness of this build fix.
------- Comment #23 From 2012-06-05 17:16:07 PST -------
> Please review & verify the correctness of this build fix.

Thanks for the build fix.  It looks correct to me.