Bug 88047 – Remove support for target-densitydpi in the viewport meta tag

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

Remove support for target-densitydpi in the viewport meta tag

[Adam Barth]

Created attachment 145214 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Konrad Piascik]

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.

[John Mellor]

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)?

[Konrad Piascik]

(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.

[John Mellor]

(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 :)

[Adam Barth]

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.

[Konrad Piascik]

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.

[Konrad Piascik]

(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.

[Adam Barth]

@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.

[Kenneth Rohde Christiansen]

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.

[Adam Barth]

Created attachment 145643 [details]
Patch

[Kenneth Rohde Christiansen]

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?

[Adam Barth]

(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.

[Konrad Piascik]

+cc a few people who may care to comment on this before it lands.

[Konrad Piascik]

(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.

[Adam Barth]

I believe that's https://bugs.webkit.org/show_bug.cgi?id=88114

[Konrad Piascik]

(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.

[WebKit Review Bot]

Comment on attachment 145643 [details]
Patch

Clearing flags on attachment: 145643

Committed r119527: <http://trac.webkit.org/changeset/119527>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

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.

[Adam Barth]

> Please review & verify the correctness of this build fix.

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