Bug 107802 - [WK2] Call LayerTreeHost::deviceOrPageScaleFactorChanged() when a device or page scale factor is changed.
Summary: [WK2] Call LayerTreeHost::deviceOrPageScaleFactorChanged() when a device or p...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dongseong Hwang
URL:
Keywords:
Depends on:
Blocks: 105978
  Show dependency treegraph
 
Reported: 2013-01-24 01:31 PST by Dongseong Hwang
Modified: 2013-01-29 15:26 PST (History)
8 users (show)

See Also:


Attachments
Patch (7.74 KB, patch)
2013-01-24 01:32 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff
Patch (8.80 KB, patch)
2013-01-24 01:55 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff
Patch (9.25 KB, patch)
2013-01-25 00:32 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dongseong Hwang 2013-01-24 01:31:00 PST
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().
Comment 1 Dongseong Hwang 2013-01-24 01:32:54 PST
Created attachment 184436 [details]
Patch
Comment 2 Early Warning System Bot 2013-01-24 01:48:41 PST
Comment on attachment 184436 [details]
Patch

Attachment 184436 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16067789
Comment 3 Dongseong Hwang 2013-01-24 01:53:04 PST
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.
Comment 4 Dongseong Hwang 2013-01-24 01:55:57 PST
Created attachment 184443 [details]
Patch
Comment 5 Kenneth Rohde Christiansen 2013-01-24 02:02:24 PST
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.
Comment 6 Dongseong Hwang 2013-01-24 14:42:11 PST
(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.
Comment 7 Dongseong Hwang 2013-01-25 00:32:04 PST
Created attachment 184688 [details]
Patch
Comment 8 Dongseong Hwang 2013-01-25 00:35:42 PST
Bug 66229 created deviceScaleChanged callback for LayerTreeHost.
Comment 9 Dongseong Hwang 2013-01-29 15:22:55 PST
thank you for your review!
Comment 10 WebKit Review Bot 2013-01-29 15:26:25 PST
Comment on attachment 184688 [details]
Patch

Clearing flags on attachment: 184688

Committed r141171: <http://trac.webkit.org/changeset/141171>
Comment 11 WebKit Review Bot 2013-01-29 15:26:29 PST
All reviewed patches have been landed.  Closing bug.