Bug 107802 – [WK2] Call LayerTreeHost::deviceOrPageScaleFactorChanged() when a device or page scale factor is changed.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Dongseong Hwang]

Currently, LayerTreeHostMac and LayerTreeHostGtk override
LayerTreeHost::deviceScaleFactorDidChange(), but two implementations just call
GraphicsLayer::deviceOrPageScaleFactorChanged(). It means we must call
LayerTreeHost::deviceScaleFactorDidChange() when a page scale factor as well as a
device scale factor are changed.

So this patch makes scalePage() and setDeviceScaleFactor() in WebPage call
LayerTreeHost::deviceScaleFactorDidChange(), and renames from
deviceScaleFactorDidChange() to deviceOrPageScaleFactorChanged().

[Dongseong Hwang]

Created attachment 184436 [details]
Patch

[Early Warning System Bot]

Comment on attachment 184436 [details]
Patch

Attachment 184436 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16067789

[Dongseong Hwang]

Currently, only DrawingAreaImpl::updateBackingStoreState calls LayerTreeHost::deviceOrPageScaleFactorChanged.

void DrawingAreaImpl::updateBackingStoreState(uint64_t stateID, bool respondImmediately, float deviceScaleFactor, const WebCore::IntSize& size, const WebCore::IntSize& scrollOffset)
{
 ...
        if (m_layerTreeHost) {
            m_layerTreeHost->deviceOrPageScaleFactorChanged();
 ...
}

I think this patch is covered by
geometry/fixed-position-composited-page-scale-scroll.html
geometry/fixed-position-composited-page-scale-smaller-than-viewport.html
geometry/fixed-position-iframe-composited-page-scale.html
geometry/fixed-position-transform-composited-page-scale-down.html
geometry/fixed-position-composited-page-scale-down.html
geometry/fixed-position-composited-page-scale-smaller-than-viewport-expected.html
geometry/fixed-position-transform-composited-page-scale.html
geometry/fixed-position-iframe-composited-page-scale-down.html
geometry/fixed-position-composited-page-scale.html
layer-creation/fixed-position-out-of-view-scaled-iframe.html
layer-creation/fixed-position-out-of-view-scaled-iframe-scroll.html
layer-creation/fixed-position-out-of-view-scaled-scroll.html
layer-creation/fixed-position-out-of-view-scaled.html
overflow/overflow-scaled-descendant-overlapping.html
repaint/page-scale-repaint.html
scaling/tiled-layer-recursion.html

Gtk WK2 does not perform pixel test yet, and Gtk does not use page scale factor in WebCore. So I can not verify this patch in Gtk.

I wonder how mac WK2 passes above pixel test until now.
I guess even if mac WK2 enables AC, DrawingAreaImpl::updateBackingStoreState is called.

I need feedback.

[Dongseong Hwang]

Created attachment 184443 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 184443 [details]
Patch

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

> Source/WebKit2/ChangeLog:10
> +        Currently, LayerTreeHostMac and LayerTreeHostGtk override
> +        LayerTreeHost::deviceScaleFactorDidChange(), but two implementations just call
> +        GraphicsLayer::deviceOrPageScaleFactorChanged(). It means we must call

Currently LayerTreeHostMac and *Gtk overrides LayerTreeHost::deviceScaleFactorDidChange(),
but the override just calls GraphicsLayer::deviceOrPageScaleFactorChanged().

OK, that seems right. The device scale changed, so it calls a functions which should be called when device scale (or page scale) changes.

> Source/WebKit2/ChangeLog:12
> +        LayerTreeHost::deviceScaleFactorDidChange() when a page scale factor as well as a
> +        device scale factor are changed.

Why does it mean that? Why does that mean you must call ::deviceScaleFactorChange() when the page scale changed? 

How do you deduct that?

> Source/WebKit2/ChangeLog:16
> +        So this patch makes scalePage() and setDeviceScaleFactor() in WebPage call
> +        LayerTreeHost::deviceScaleFactorDidChange(), and renames from
> +        deviceScaleFactorDidChange() to deviceOrPageScaleFactorChanged().

I dont get this. You need to explain it better.

scalePage doesn't include the device scale right? so then why do we need to call deviceScaleFactorDidChange()?

Anyway you are actually renaming that to deviceOrPage... which clear up the naming, but you still don't explain why it must be called.

[Dongseong Hwang]

(In reply to comment #5)
> (From update of attachment 184443 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184443&action=review
Thank you for your review! I'll refine changelog.

> Anyway you are actually renaming that to deviceOrPage... which clear up the naming, but you still don't explain why it must be called.

Yeah, you are right. It is because I don't fully understand Mac's rendering routine yet.

Although Gtk override it, Gtk does not use both device and page scale factor AFAIK. So I must explain using Mac but I could not do that yet.

btw, this is preparation of Bug 105978 that makes EFL and Qt use page scale factor.
Without this patch, EFL and Qt have problem. I can verify this patch's justice with Bug 105978 on EFL and Qt.

[Dongseong Hwang]

Created attachment 184688 [details]
Patch

[Dongseong Hwang]

Bug 66229 created deviceScaleChanged callback for LayerTreeHost.

[Dongseong Hwang]

thank you for your review!

[WebKit Review Bot]

Comment on attachment 184688 [details]
Patch

Clearing flags on attachment: 184688

Committed r141171: <http://trac.webkit.org/changeset/141171>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.