Bug 106460 – Fix scale of screen.width, window.outerWidth and @media device-width when page scale not applied in compositor.

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

[John Mellor]

Fix scale of screen.width, window.outerWidth and @media device-width when page scale not applied in compositor.

[John Mellor]

Created attachment 181948 [details]
Patch

[Alexandre Elias]

Hmm, applyPageScaleFactorInCompositor defaults to false so this patch may not be a no-op for non-Chromium ports.  How about moving applyDeviceScaleFactorInCompositor to WebCore/page/Settings.h and defaulting that value to true?

[Kenneth Rohde Christiansen]

Comment on attachment 181948 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181948&action=review

> Source/WebCore/ChangeLog:40
> +        No new tests, as I don't know how to setup DRT so it
> +            a) has a deviceScaleFactor other than 1, and

This is indeed done in some tests... try grepping for deviceScaleFactor

> Source/WebCore/page/DOMWindow.cpp:1122
> -    return static_cast<int>(page->chrome()->windowRect().height());
> +    float height = page->chrome()->windowRect().height();
> +    if (!page->settings()->applyPageScaleFactorInCompositor())
> +        height /= page->deviceScaleFactor();
> +    return static_cast<int>(height);

This is against what iOS, Qt etc are doing. windowRect should returns values in UI units instead. Can't you move this code there?

> Source/WebCore/platform/chromium/PlatformScreenChromium.cpp:92
> +static FloatRect scaleToUIPixels(FloatRect rect, Widget* widget)
> +{
> +    if (widget->isFrameView()) {
> +        Page* page = static_cast<FrameView*>(widget)->frame()->page();
> +        if (page && !page->settings()->applyPageScaleFactorInCompositor())
> +            rect.scale(1 / page->deviceScaleFactor());
> +    }
> +    return rect;
> +}

mac already uses similar functions (though their OS reports values already in UI units, but they need flipping). I think they are in their webkit2 Pageclient class.

[John Mellor]

Created attachment 182104 [details]
Patch

[John Mellor]

(In reply to comment #2)
> applyPageScaleFactorInCompositor defaults to false so this patch may not be a no-op for non-Chromium ports.  How about moving applyDeviceScaleFactorInCompositor to WebCore/page/Settings.h and defaulting that value to true?

Oops! Done.

[Kenneth Rohde Christiansen]

Comment on attachment 182104 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182104&action=review

> Source/WebCore/page/DOMWindow.cpp:1121
> +    float height = page->chrome()->windowRect().height();
> +    if (!page->settings()->applyDeviceScaleFactorInCompositor())
> +        height /= page->deviceScaleFactor();

I still don't think this is the right place for this. Other ports are treading windowRect as in ui units

[John Mellor]

Comment on attachment 181948 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181948&action=review

>> Source/WebCore/ChangeLog:40
>> +            a) has a deviceScaleFactor other than 1, and
> 
> This is indeed done in some tests... try grepping for deviceScaleFactor

Yes, it seems one can use testRunner.setBackingScaleFactor for a); but b) is tricky. I guess I could expose the applyDeviceScaleFactorInCompositor setting on InternalSettings and set it from a test (and the test would presumably check that with dSF of 2, DRT has a 400x300px window instead of 800x600px). Do you think it would be worth exposing that just for this?

>> Source/WebCore/page/DOMWindow.cpp:1122
>> +    return static_cast<int>(height);
> 
> This is against what iOS, Qt etc are doing. windowRect should returns values in UI units instead. Can't you move this code there?

I agree; I'd assumed that would break things in Chrome for Android's current implementation, but actually this seems to work fine. Alex, can you confirm that this new approach is ok? I'll upload a new patch in a minute.

>> Source/WebCore/platform/chromium/PlatformScreenChromium.cpp:92
>> +}
> 
> mac already uses similar functions (though their OS reports values already in UI units, but they need flipping). I think they are in their webkit2 Pageclient class.

Yes, they have toUserSpace and toDeviceSpace in platform/mac/PlatformScreenMac.mm - are you suggesting I use the same name (toUserSpace)?

[John Mellor]

Created attachment 182129 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 182129 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182129&action=review

> Source/WebCore/ChangeLog:42
> +        No new tests, as I don't know how to setup DRT so it
> +            a) has a deviceScaleFactor other than 1, and
> +            b) applyPageScaleFactorInCompositor is false.
> +        Advice welcome if this is actually possible!

You should probably rewrite this part

is b) true for Android only? Then just make the test expected failure on non-android chrome? it should pass for other platforms doing this right

[Kenneth Rohde Christiansen]

> Yes, it seems one can use testRunner.setBackingScaleFactor for a); but b) is tricky. I guess I could expose the applyDeviceScaleFactorInCompositor setting on InternalSettings and set it from a test (and the test would presumably check that with dSF of 2, DRT has a 400x300px window instead of 800x600px). Do you think it would be worth exposing that just for this?

doesnt seem worth it. Cant you not just make the test pass on Android and have expected failure for desktop Chrome?

> Yes, they have toUserSpace and toDeviceSpace in platform/mac/PlatformScreenMac.mm - are you suggesting I use the same name (toUserSpace)?

It is not the best names in the world, but consistency is important, so it might be worth it

[Alexandre Elias]

As far as I know, adjusting windowRect should be fine, although please double-check that there isn't a call site that ends up double-applying the scale.

On http://trac.webkit.org/wiki/ScalesAndZooms we wrote "DIP pixel" (for density independent pixel) which is the same term that's used in Chromium.  It's not prevalent in WebKit yet, but it's unambiguous so I'd suggest that one?

[John Mellor]

Comment on attachment 182129 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182129&action=review

>> Source/WebCore/ChangeLog:42
>> +        Advice welcome if this is actually possible!
> 
> You should probably rewrite this part
> 
> is b) true for Android only? Then just make the test expected failure on non-android chrome? it should pass for other platforms doing this right

Actually, I mean applyDeviceScaleFactorInCompositor. And sadly no, it's true for all platforms except Chrome for Android. So for example, I could write a test to check that when deviceScaleFactor == 1, then screen.width == @media device-width == 800; and that when deviceScaleFactor == 2, then screen.width == @media device-width == still 800 (because then WebCore assumes the 800px is in UI pixels, and so is still the correct value for these APIs). But that's not testing the right thing. In fact, that test won't even fail on Android, because Android DRT does AFAIK have applyDeviceScaleFactorInCompositor true, like all the desktop platforms.

I've changed the explanation to "No new tests, as applyDeviceScaleFactorInCompositor appears to always be true in DumpRenderTree, so this situation cannot occur there."

> It is not the best names in the world, but consistency is important, so it [renaming to toUserSpace] might be worth it

Done.

> As far as I know, adjusting windowRect should be fine, although please double-check that there isn't a call site that ends up double-applying the scale.

Done - they all seem benign.

> On http://trac.webkit.org/wiki/ScalesAndZooms we wrote "DIP pixel" (for density independent pixel)

I've switched to that term in the ChangeLog & comments.

[John Mellor]

Created attachment 182190 [details]
Patch

[WebKit Review Bot]

Comment on attachment 182190 [details]
Patch

Clearing flags on attachment: 182190

Committed r139356: <http://trac.webkit.org/changeset/139356>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.