Bug 88047 - Remove support for target-densitydpi in the viewport meta tag
Summary: Remove support for target-densitydpi in the viewport meta tag
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks: 88114
  Show dependency treegraph
 
Reported: 2012-05-31 23:10 PDT by Adam Barth
Modified: 2012-06-05 17:16 PDT (History)
18 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2012-05-31 23:10:24 PDT
Remove support for target-densitydpi in the viewport meta tag
Comment 1 Adam Barth 2012-05-31 23:18:17 PDT
Created attachment 145214 [details]
Patch
Comment 2 WebKit Review Bot 2012-06-01 02:20:22 PDT
Comment on attachment 145214 [details]
Patch

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 WebKit Review Bot 2012-06-01 02:20:27 PDT
Created attachment 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 Konrad Piascik 2012-06-01 06:13:33 PDT
Comment on attachment 145214 [details]
Patch

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 John Mellor 2012-06-01 06:30:59 PDT
Comment on attachment 145214 [details]
Patch

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 Konrad Piascik 2012-06-01 07:10:12 PDT
(In reply to comment #5)
> (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.

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 John Mellor 2012-06-01 08:07:55 PDT
(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 Adam Barth 2012-06-01 08:59:55 PDT
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 Konrad Piascik 2012-06-01 09:03:47 PDT
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 Konrad Piascik 2012-06-01 09:04:44 PDT
(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 Adam Barth 2012-06-01 09:06:18 PDT
@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 Kenneth Rohde Christiansen 2012-06-02 05:44:16 PDT
Comment on attachment 145214 [details]
Patch

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 Adam Barth 2012-06-04 15:38:51 PDT
Created attachment 145643 [details]
Patch
Comment 14 Kenneth Rohde Christiansen 2012-06-04 15:44:23 PDT
Comment on attachment 145643 [details]
Patch

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 Adam Barth 2012-06-04 15:46:07 PDT
(In reply to comment #14)
> (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?

Generally we try to avoid forward-looking statements because they make lawyers sad.
Comment 16 Konrad Piascik 2012-06-04 15:50:24 PDT
+cc a few people who may care to comment on this before it lands.
Comment 17 Konrad Piascik 2012-06-04 15:56:50 PDT
(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 Adam Barth 2012-06-04 15:58:26 PDT
I believe that's https://bugs.webkit.org/show_bug.cgi?id=88114
Comment 19 Konrad Piascik 2012-06-04 16:10:40 PDT
(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 WebKit Review Bot 2012-06-05 14:37:13 PDT
Comment on attachment 145643 [details]
Patch

Clearing flags on attachment: 145643

Committed r119527: <http://trac.webkit.org/changeset/119527>
Comment 21 WebKit Review Bot 2012-06-05 14:37:21 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 Ryosuke Niwa 2012-06-05 17:06:47 PDT
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 Adam Barth 2012-06-05 17:16:07 PDT
> Please review & verify the correctness of this build fix.

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