| Summary: | Remove support for target-densitydpi in the viewport meta tag | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Adam Barth <abarth> | ||||||||
| Component: | New Bugs | Assignee: | Adam Barth <abarth> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | aelias, danakj, dglazkov, efidler, fsamuel, gyuyoung.kim, johnme, kenneth, klobag, kpiascik, mjs, mlattanzio, rakuco, rniwa, simon.fraser, tdanderson, tonikitoo, webkit.review.bot | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Bug Depends on: | |||||||||||
| Bug Blocks: | 88114 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Adam Barth
2012-05-31 23:10:24 PDT
Created attachment 145214 [details]
Patch
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 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 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 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)? (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. (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 :) 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. 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. (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. @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 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. Created attachment 145643 [details]
Patch
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? (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. +cc a few people who may care to comment on this before it lands. (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. I believe that's https://bugs.webkit.org/show_bug.cgi?id=88114 (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 on attachment 145643 [details] Patch Clearing flags on attachment: 145643 Committed r119527: <http://trac.webkit.org/changeset/119527> All reviewed patches have been landed. Closing bug. 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. > Please review & verify the correctness of this build fix.
Thanks for the build fix. It looks correct to me.
|