Created attachment 143928 [details] work in progress

This patch is from the chromium-android branch. I'm trying to sort out whether this is a valuable change that we should make upstream or whether it's something we need to remove from the branch. Fady seemed to indicate that this was a valuable change, but that there were some complexities in creating a correct implementation.

Comment on attachment 143928 [details] work in progress devicePixelRatioForDeviceDimensions just doesn't make sense. devicePixelRatio is a fixed number (1 or 2).

(In reply to comment #2) > This patch is from the chromium-android branch. I'm trying to sort out whether this is a valuable change that we should make upstream or whether it's something we need to remove from the branch. Fady seemed to indicate that this was a valuable change, but that there were some complexities in creating a correct implementation. We wanted to do the same thing but with defaultDeviceScaleFactor(), but opted for making WebCore mostly oblivious to device scaling due to bugs in fixed layout and time constraints.

> devicePixelRatioForDeviceDimensions just doesn't make sense. devicePixelRatio is a fixed number (1 or 2). Yes, I agree that the patch, as written, doesn't make much sense. I had first posted the reverse of this patch to the chromium-android patch to revert the change, but Fady suggested that we discuss whether it makes sense to do something like this upstream. IMHO, it's better to have those sorts of discussions in the WebKit bug tracker. ;)

I'm having some difficulty understanding this value and why it's defined as: result.devicePixelRatioForDeviceDimensions = isAutoDPI ? (deviceDPI / 160.0f) : result.devicePixelRatio; In the autoDPI case which is 99.9% of websites (any website without a viewport tag explicitly requesting DPI), this value is just the same as defaultDeviceScaleFactor (the physical DPI of the device). In the rare explicitly-requested-DPI case, it's devicePixelRatio (= deviceScaleFactor). I don't fully understand the need to introduce this third value, could we just use defaultDeviceScaleFactor instead? John, what do you think?

Created attachment 143935 [details] Alternate version that doesn't introduce a new variable

Hmm, looking at this further, I think we actually do need a third value because of ChromeOS's different deviceScale decisions. Clank's viewport is specified in physical pixels whereas ChromeOS's is in CSS pixels. So actually, ChromeOS does not need this diff at all, and we could rename the new value to viewportScaleFactor and set it to: viewportScaleFactor = defaultDeviceScaleFactor / WebViewImpl::m_deviceScaleInCompositor; This would give a value of 1 on ChromeOS and of defaultDeviceScaleFactor on Android.

It is wise for Android and ChromeOS to make different decisions in this regard? That seems like a recipe for sadness in the future.

Not very, no, and we had a debate about this. Each approach has its own set of bugs which impact one platform more severely than the other, so we ended up being forced to diverge for now.

Was that discusion captured in a bug I can read? I'm not sure how it affects this patch.

(In reply to comment #3) > (From update of attachment 143928 [details]) > devicePixelRatioForDeviceDimensions just doesn't make sense. devicePixelRatio is a fixed number (1 or 2). Actually some Nokia devices and Android devices use the value of 1.5, so basically it can be 1, 1.5 and 2

Comment on attachment 143935 [details] Alternate version that doesn't introduce a new variable View in context: https://bugs.webkit.org/attachment.cgi?id=143935&action=review > Source/WebCore/page/DOMWindow.cpp:1098 > - return static_cast<int>(page->chrome()->windowRect().height()); > + return static_cast<int>(page->chrome()->windowRect().height() / page->settings()->defaultDeviceScaleFactor()); hmm I don't get this. For Qt, the size of the windowRect is in device pixels and not upscaled

(In reply to comment #11) > Was that discusion captured in a bug I can read? I'm not sure how it affects this patch. The different ChromeOS behavior was introduced in bug 86051, not sure how much it would clarify things to read all of it though.

For CROS we did it in such a way that the frame's size is equal to the number of CSS pixels, so it's not inflated to physical pixels as would be done with fixed-layout-mode. So the devicePixelRatioForDeviceDimensions might be 1 in that case.. and the variable is more a ratio between CSS/layout pixels and the frame's size?

I'm hesitant to introduce yet another scale factor into the project. It seems that folks cannot even keep the ones we have currently straight.

One remaining question I have is what size viewport does the Mac port pass in from the embedder? Physical pixels or device-independent pixels?

(In reply to comment #17) > One remaining question I have is what size viewport does the Mac port pass in from the embedder? Physical pixels or device-independent pixels? I suspect bdakin might be able to answer your question.

IMHO, we should use the same approach here across ports, and Android and Chrome OS should also use that same approach. Using a different architecture on Android and Chrome OS might be easier in the short term, but it's a recipe for sadness and not in our long-term interests.

(In reply to comment #19) > IMHO, we should use the same approach here across ports, and Android and Chrome OS should also use that same approach. Using a different architecture on Android and Chrome OS might be easier in the short term, but it's a recipe for sadness and not in our long-term interests. AIUI Chrome OS plans to use the same approach as Android, and so these two should converge, but it was not feasible for the next Chromium milestone. The difference right now is essentially that Chrome OS applies the device scale during composite instead of as a CSS transform on the root layer. Doing otherwise at the moment broke some important things, like Google Docs and CSS fixed position elements.

(In reply to comment #16) > I'm hesitant to introduce yet another scale factor into the project. It seems that folks cannot even keep the ones we have currently straight. Oh yes.. I don't mean to do that. I think we have too many probably, and it's confusing because they are not the right ones currently. IMO, we should have the most basic factors possible and then combine and use them as appropriate in code. It just seems like this one's name is misleading if the device scale is not part of the page scale (and thus transforming the root layer)?

(In reply to comment #17) > One remaining question I have is what size viewport does the Mac port pass in from the embedder? Physical pixels or device-independent pixels? I must admit that I don't fully understand the question. What you mean by the embedder? But generally we use CSS aka device-independent pixels for just about everything.

(In reply to comment #20) > The difference right now is essentially that Chrome OS applies the device scale during composite instead of as a CSS transform on the root layer. I'm confused - why is either port transforming things by the deviceScaleFactor? Do you mean the pageScaleFactor?

(In reply to comment #23) > (In reply to comment #20) > > The difference right now is essentially that Chrome OS applies the device scale during composite instead of as a CSS transform on the root layer. > > I'm confused - why is either port transforming things by the deviceScaleFactor? Do you mean the pageScaleFactor? In Android deviceScaleFactor is a component of pageScaleFactor, so I do. In ChromeOS right now, pageScaleFactor is always 1, and everything is scaled during draw by the deviceScaleFactor. This is to avoid scaling the Frame as pageScaleFactor would have done, and to match the Mac port more closely.

defaultDeviceScaleFactor doesn't exist anymore.

(In reply to comment #25) > defaultDeviceScaleFactor doesn't exist anymore. That seems orthogonal to "DOM APIs don't deflate device dimensions by the device scale factor". We still need to divide device dimensions by deviceScaleFactor on Android and other ports that (unfortunately) size their screen/windowRect in physical screen pixels (as mentioned in comment #8, Chrome OS doesn't need this since they apparently already have it in CSS pixels).

(In reply to comment #26) > (In reply to comment #25) > > defaultDeviceScaleFactor doesn't exist anymore. > > That seems orthogonal to "DOM APIs don't deflate device dimensions by the device scale factor". We still need to divide device dimensions by deviceScaleFactor on Android and other ports that (unfortunately) size their screen/windowRect in physical screen pixels (as mentioned in comment #8, Chrome OS doesn't need this since they apparently already have it in CSS pixels). I think Android should just do the same thing as ChromeOS / Mac/ etc.

(In reply to comment #27) > (In reply to comment #26) > > (In reply to comment #25) > > > defaultDeviceScaleFactor doesn't exist anymore. > > > > That seems orthogonal to "DOM APIs don't deflate device dimensions by the device scale factor". We still need to divide device dimensions by deviceScaleFactor on Android and other ports that (unfortunately) size their screen/windowRect in physical screen pixels (as mentioned in comment #8, Chrome OS doesn't need this since they apparently already have it in CSS pixels). > > I think Android should just do the same thing as ChromeOS / Mac/ etc. Sounds reasonable. Alex, what's our status here?