[EFL][Qt][WK2] We should consider a page scale factor in WebCore instead of our own scale factor.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Dongseong Hwang]

Curruntly, Coordinated Graphics manages a duplicated scale. Remove code related to a duplicated scale.

There are two major changes.
1. Don't keep scale. Instead query the scale from WebPage[Proxy].
2. CoordinatedLayerTreeHost::SetVisibleContentsRect message does not send a scale anymore. Instead reuse IPC communication of WebPage, and callback mechanism of GraphicsLayer.

FYI, note what is effectiveScaleFactor.
effectiveScaleFactor is pageScaleFactor * deviceScaleFactor.
platform webview and pageViewportController know both pageScaleFactor and deviceScaleFactor.
But CoordinatedGraphicsLayer and TiledBackingStore need to know only effectiveScaleFactor.
CoordinatedGraphicsLayer would snap pixel alignment using effectiveScaleFactor, and TiledBackingStore paints contents using effectiveScaleFactor.

However, GraphicsLayerCA snaps pixel alignment using pageScaleFactor. I think it is a potential bug, but apple uses only 2 as a deviceScaleFactor, so this potential bug does not appear yet.
In conclusion, CoordinatedGraphicsLayer and TiledBackingStore need to know final scale for device, and it is effectiveScaleFactor.

[Dongseong Hwang]

Created attachment 181120 [details]
WIP Patch

[Dongseong Hwang]

Comment on attachment 181120 [details]
WIP Patch

Post WIP patch to clarify the direction.

I'm not sure yet this code is correct in each platform specific code: webview, pageClient, pageViewportControllerClient. And I need more test.

I worked based on kenneth's xmaxpatch :)

[Build Bot]

Comment on attachment 181120 [details]
WIP Patch

Attachment 181120 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15639622

[Mikhail Pozdnyakov]

Comment on attachment 181120 [details]
WIP Patch

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

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:114
> +    WebKit::WebPageProxy* page() const { return m_pageProxy.get(); }

this method returns mutable pointer, so should not be const

[Andras Becsi]

(In reply to comment #0)
> Curruntly, Coordinated Graphics manages a duplicated scale. Remove code related to a duplicated scale.
> 
> There are two major changes.
> 1. Don't keep scale. Instead query the scale from WebPage[Proxy].
> 2. CoordinatedLayerTreeHost::SetVisibleContentsRect message does not send a scale anymore. Instead reuse IPC communication of WebPage, and callback mechanism of GraphicsLayer.
> 
> FYI, note what is effectiveScaleFactor.
> effectiveScaleFactor is pageScaleFactor * deviceScaleFactor.
> platform webview and pageViewportController know both pageScaleFactor and deviceScaleFactor.
> But CoordinatedGraphicsLayer and TiledBackingStore need to know only effectiveScaleFactor.
> CoordinatedGraphicsLayer would snap pixel alignment using effectiveScaleFactor, and TiledBackingStore paints contents using effectiveScaleFactor.
> 
> However, GraphicsLayerCA snaps pixel alignment using pageScaleFactor. I think it is a potential bug, but apple uses only 2 as a deviceScaleFactor, so this potential bug does not appear yet.
> In conclusion, CoordinatedGraphicsLayer and TiledBackingStore need to know final scale for device, and it is effectiveScaleFactor.

I think this patch should be aligned with the work I do for Qt on retina display.
Since the coordinates in Qt are always logical and the device scale factor is applied on the backing store and during painting in QtWebPageSGNode, the device scale factor logic in PageViewportController is redundant and will be removed.
My patch for that also still needs testing, and I have to align the change with the way EFL is handling dpxr.

I'm back from vacation on monday, please do not land this before next week.

[Mikhail Pozdnyakov]

Comment on attachment 181120 [details]
WIP Patch

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

> Source/WebKit2/ChangeLog:8
> +        Curruntly, Coordinated Graphics manages a duplicated scale. Remove code about a

Spelling.

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:507
> +    return page()->pageScaleFactor() * page()->deviceScaleFactor();

Are you sure 'page()->pageScaleFactor()' is initialized properly here?

> Source/WebKit2/UIProcess/PageViewportController.cpp:50
> +    , m_effectiveScaleFactor(1)

maybe renaming worth creating a separate patch?

> Source/WebKit2/UIProcess/efl/PageViewportControllerClientEfl.h:55
> +    virtual void setContentsScale(float) { }

could this method be removed?

[Dongseong Hwang]

(In reply to comment #6)
> (From update of attachment 181120 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181120&action=review

Thank Mikhail for review.
I rewrite the second WIP patch. I will apply your review :)

(In reply to comment #5)
> (In reply to comment #0)
> I think this patch should be aligned with the work I do for Qt on retina display.
What a coincidence! great.
> Since the coordinates in Qt are always logical and the device scale factor is applied on the backing store and during painting in QtWebPageSGNode, the device scale factor logic in PageViewportController is redundant and will be removed.
Do you means TiledBackingStore must know both pageScale and deviceScale? However TileCache in Apple knows only effectiveScale ( = pageScale * deviceScale). And I think PageViewportController needs to know effectiveScale because PageViewportController must adjust scroll position to avoid noncompositing layer blurring.

> My patch for that also still needs testing, and I have to align the change with the way EFL is handling dpxr.
> 
> I'm back from vacation on monday, please do not land this before next week.

Ok, I wait for you. I found a lot of problems as made this bug. I need to cowork and discuss in depth. :)


As I mentioned, I encountered many problems. I list some of them.
1. delegating scroll : we sometimes change scrollPosition of ScrollView  (i.e. select text using mouse) and sometimes ignore (most cases). However, changing scrollPosition of WebCore does not make anything except for calling PageDidRequestScroll callback.
I think we will remove scroll delegating so that WebCore draws scroll bar and we use Scrolling thread. Am I right?
Page::setPageScaleFactor(float scale, const IntPoint& origin) requires scroll position, but different scroll position arguments do not make difference on Coordinated Graphics.

2. page scale : Currently we never change page scale when using Minibrowser, but we can change via WK2 API. CoordiantedGraphicsLayer and TiledBackingStore seems to assume pageScale is always 1. We need to change CoordiantedGraphicsLayer and TiledBackingStore to get along with pageScale in WebCore.

3. There are complex and slow scroll position and scale communication routine related to PageViewportController.
For example, we need 4 times IPC communication to run wheel event.

      UI Process                      Message               Web Process
WebPageProxy                       (wheel ->)                WebPage
PageViewportController    (<- PageDidRequestScroll)   WebPage
PageViewportController    (setVisibleContentsRect ->) CoordinatedLayerTree
PageViewportController        (<- didRenderFrame)      CoordinatedLayerTree
LayerTreeRenderer::paint

Scale handling ditto.
We implemented PageViewportController runs drawing after receiving callback from Web Process. We need to change.

[Mikhail Pozdnyakov]

> 2. page scale : Currently we never change page scale when using Minibrowser, but we can change via WK2 API. CoordiantedGraphicsLayer and TiledBackingStore seems to assume pageScale is always 1. We need to change CoordiantedGraphicsLayer and TiledBackingStore to get along with pageScale in WebCore.
> 

https://bugs.webkit.org/show_bug.cgi?id=104374 is targeted to solve it.
However the solution would also require fix of the current bug.

[Dongseong Hwang]

(In reply to comment #8)
> https://bugs.webkit.org/show_bug.cgi?id=104374 is targeted to solve it.
> However the solution would also require fix of the current bug.

Aha, this bug was already created! I think both are duplicated.

[Dongseong Hwang]

*** Bug 104374 has been marked as a duplicate of this bug. ***

[Dongseong Hwang]

Created attachment 181873 [details]
WIP Patch2

[Dongseong Hwang]

Comment on attachment 181873 [details]
WIP Patch2

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

This is the first working patch, that means the first patch work abnormally :)

I post this patch to explain the direction.
We need prerequisite refactoring before this patch.
1. Decide whether transform include deviceScale as well as pageScale.
2. The size of WebPage (a.k.a nonCompositingLayer) is on viewport coordinated system, not contents coordinated system although the size of all GrahpicsLayer is on contents coordinated system.

TODO: As I mentioned, sending scroll and scall to Web Process is not efficient. We need to refactor further.

> Source/WebCore/css/StyleResolver.cpp:1456
> +//        documentStyle->setPageScaleTransform(frame ? frame->frameScaleFactor() : 1);

If we don't comment out, coordinated graphics draws with pageScale^2. It means if pageScale is 2, we can see 4 times scaled contents.

In addition, if we really want transform includes scale info, we should make transform include deviceRatioFactor and we should make GraphicsLayer not know about scale.
IMO current implementation is ambiguous.

> Source/WebKit2/UIProcess/PageViewportController.h:112
> +    float m_contentsScale; // Should always be cssScale * devicePixelRatio.

Define m_contentsScale that is cssScale * devicePixelRatio because GraphicsLayerCA and TiledBackingStore also use the contentsScale term.

[Build Bot]

Comment on attachment 181873 [details]
WIP Patch2

Attachment 181873 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15763457

New failing tests:
fast/frames/frame-set-scaling-hit.html
fast/dom/window-scroll-scaling.html
compositing/layer-creation/fixed-position-out-of-view-scaled.html
fast/dom/Range/scale-page-client-rects.html
fast/dom/elementFromPoint-scaled-scrolled.html
inspector/elements/highlight-node-scaled.html
fast/frames/frame-set-rotation-hit.html
compositing/layer-creation/fixed-position-out-of-view-scaled-scroll.html
fast/dom/Element/scale-page-bounding-client-rect.html
fast/dom/Element/scale-page-client-rects.html
fast/events/page-scaled-mouse-click-iframe.html
fast/dom/Range/scale-page-bounding-client-rect.html
fast/transforms/selection-bounds-in-transformed-view.html
svg/as-image/image-respects-pageScaleFactor.html
compositing/tiling/tile-cache-zoomed.html

[Dominik Röttsches (drott)]

(In reply to comment #12)

> In addition, if we really want transform includes scale info, we should make transform include deviceRatioFactor and we should make GraphicsLayer not know about scale.

Can you explain this a bit more - I don't really understand the comment. What I do know is that it's beneficial for the graphics context to know the scale factor.

[Dongseong Hwang]

(In reply to comment #14)
> (In reply to comment #12)
> 
> > In addition, if we really want transform includes scale info, we should make transform include deviceRatioFactor and we should make GraphicsLayer not know about scale.
> 
> Can you explain this a bit more - I don't really understand the comment. What I do know is that it's beneficial for the graphics context to know the scale factor.

That's good question! I should have explained well. Sorry for lazy comment.

GraphicsLayer should use final effective scale to draw contents on the device screen.
The effective scale is page scale * device scale.

Current WebCore slightly apply page scale to local transform of GraphicsLayer. But RenderLayerBacking notifies page scale and device scale to GraphicsLayer already. I think it is a bit confused that local transform include page scale. We must remember page scale and local transform are not orthogonal.
And if we really want local transform to include scale info, local transform should include device scale also.

I prefer local transform not to include scale info.

In addition, Page::pageScale() method causes style recalc. IMO it is only for applying page scale to local transform. If it is true, the style recalc can be avoidable as we don't change local transform.

And I don't fully understand why Page::pageScale() causes layout yet. Now, changing page scale causes layout after this patch. I'm not sure whether it is acceptable or not.

[Kenneth Rohde Christiansen]

Are you going to update this patch?

[Dongseong Hwang]

Created attachment 184193 [details]
Patch

[Dongseong Hwang]

After this patch, we need to reset the results of many layout tests because we now change a page scale factor in WebCore.

I tested pinch zoom using QtMiniBrowser and device scale factor using -r option of EFLMiniBrowser. I think this patch is fine.

However, I'm sure this patch makes new bugs that are not identified yet. We need to fix them gradually.

[Kenneth Rohde Christiansen]

Comment on attachment 184193 [details]
Patch

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

> Source/WebKit2/ChangeLog:11
> +        Currently, PageViewportController sends a page scale factor to Coordinated
> +        Graphics System regardless of the page scale factor in WebCore. This patch makes
> +        Coordinated Graphics System use the page scale factor in WebCore to match other
> +        ports.

Exactly what we want, to be more inline with the other ports

> Source/WebKit2/ChangeLog:15
> +        WebCore is changed, CoordinatedGraphicsLayer can be notified via

gets? instead of can be

> Source/WebKit2/ChangeLog:23
> +        We set true to applyDeviceScaleFactorInCompositor and
> +        ApplyPageScaleFactorInCompositor in Settings like chromium, because
> +        TiledBackingStore that is a backing store of each GraphicsLayer applies the
> +        scale to our raster graphics engines instead of applying the scale to the local
> +        transform of each render object.

that sounds right. Then you dont need to relayout after changing scale

> Source/WebKit2/ChangeLog:43
> +        * UIProcess/API/efl/EwkViewImpl.cpp:
> +        (EwkViewImpl::EwkViewImpl):
> +        * UIProcess/API/qt/qquickwebview.cpp:
> +        (QQuickWebViewPrivate::initialize):
> +        (QQuickWebViewLegacyPrivate::updateViewportSize):
> +        * UIProcess/API/qt/raw/qrawwebview.cpp:
> +        (QRawWebView::setSize):
> +        * UIProcess/CoordinatedGraphics/CoordinatedLayerTreeHostProxy.cpp:
> +        (WebKit::CoordinatedLayerTreeHostProxy::CoordinatedLayerTreeHostProxy):
> +        (WebKit::CoordinatedLayerTreeHostProxy::setVisibleContentsRect):
> +        * UIProcess/CoordinatedGraphics/CoordinatedLayerTreeHostProxy.h:
> +        (CoordinatedLayerTreeHostProxy):
> +        * UIProcess/DrawingAreaProxy.h:
> +        (WebKit::DrawingAreaProxy::setVisibleContentsRect):
> +        * UIProcess/DrawingAreaProxyImpl.cpp:

a bit of detailed info here would be useful

> Source/WebKit2/UIProcess/PageViewportController.cpp:202
> +        // FIXME : the contents position does not affect anything when delegates scrolling is enabled.
> +        m_webPageProxy->scalePage(m_pageScaleFactor, roundedIntPoint(m_contentsPosition));

Why is that a FIXME? Should it?

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1218
>  
> +#if USE(ACCELERATED_COMPOSITING)
> +    if (m_drawingArea->layerTreeHost())
> +        m_drawingArea->layerTreeHost()->deviceOrPageScaleFactorChanged();
> +#endif

smfr should probably look at this change

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1254
> +#if USE(ACCELERATED_COMPOSITING)
> +    if (m_drawingArea->layerTreeHost())
> +        m_drawingArea->layerTreeHost()->deviceOrPageScaleFactorChanged();
> +#endif

diddo

> Source/WebKit2/WebProcess/WebPage/mac/LayerTreeHostMac.mm:165
> -void LayerTreeHostMac::deviceScaleFactorDidChange()
> +void LayerTreeHostCA::deviceOrPageScaleFactorChanged()

diddo

[EFL EWS Bot]

Comment on attachment 184193 [details]
Patch

Attachment 184193 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16080219

[Early Warning System Bot]

Comment on attachment 184193 [details]
Patch

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

[Build Bot]

Comment on attachment 184193 [details]
Patch

Attachment 184193 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16065585

[Build Bot]

Comment on attachment 184193 [details]
Patch

Attachment 184193 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16067468

[Dongseong Hwang]

(In reply to comment #19)
> (From update of attachment 184193 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184193&action=review
Thank you for review!

> > Source/WebKit2/ChangeLog:15
> > +        WebCore is changed, CoordinatedGraphicsLayer can be notified via
> 
> gets? instead of can be

yes

> > Source/WebKit2/ChangeLog:23
> > +        We set true to applyDeviceScaleFactorInCompositor and
> > +        ApplyPageScaleFactorInCompositor in Settings like chromium, because
> > +        TiledBackingStore that is a backing store of each GraphicsLayer applies the
> > +        scale to our raster graphics engines instead of applying the scale to the local
> > +        transform of each render object.
> 
> that sounds right. Then you dont need to relayout after changing scale

But we need to change Page::setPageScaleFactor to no relayout. I think putting "if (ApplyPageScaleFactorInCompositor)" in relevant place.

> > Source/WebKit2/ChangeLog:43
> > +        (WebKit::DrawingAreaProxy::setVisibleContentsRect):
> > +        * UIProcess/DrawingAreaProxyImpl.cpp:
> 
> a bit of detailed info here would be useful

I'll update.

> 
> > Source/WebKit2/UIProcess/PageViewportController.cpp:202
> > +        // FIXME : the contents position does not affect anything when delegates scrolling is enabled.
> > +        m_webPageProxy->scalePage(m_pageScaleFactor, roundedIntPoint(m_contentsPosition));
> 
> Why is that a FIXME? Should it?

I'll recomment without FIXME.
The comment is a bit irrelevant. What I really want to say is that Page::setPageScaleFactor is somewhat weird. I don't understand we really need updateLayout in  Page::setPageScaleFactor when delegatesScrolling is enabled.

void Page::setPageScaleFactor(float scale, const IntPoint& origin)
{
    ...
    if (scale == m_pageScaleFactor) {
        if (view && (view->scrollPosition() != origin || view->delegatesScrolling())) {
            document->updateLayoutIgnorePendingStylesheets();
            view->setScrollPosition(origin);
        }
        return;
    }
    ...
}

> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1218
> >  
> > +#if USE(ACCELERATED_COMPOSITING)
> > +    if (m_drawingArea->layerTreeHost())
> > +        m_drawingArea->layerTreeHost()->deviceOrPageScaleFactorChanged();
> > +#endif
> 
> smfr should probably look at this change

I'll separate this part in another bug so that smfr review conveniently :)

[Dongseong Hwang]

Build fail due to Bug 107742.
I fixed in Bug 107760

[Dongseong Hwang]

Created attachment 184438 [details]
Patch

[Early Warning System Bot]

Comment on attachment 184438 [details]
Patch

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

[Dongseong Hwang]

Created attachment 184445 [details]
Patch

[Dongseong Hwang]

Created attachment 185449 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 185449 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1276
>      m_page->settings()->setFixedPositionCreatesStackingContext(fixed);
> +    m_page->settings()->setApplyDeviceScaleFactorInCompositor(fixed);
> +    m_page->settings()->setApplyPageScaleFactorInCompositor(fixed);
>  #endif

I tihnk we are already setting these when creating our EwkView (or we should be)

[Dongseong Hwang]

(In reply to comment #30)
> (From update of attachment 185449 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185449&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1276
> >      m_page->settings()->setFixedPositionCreatesStackingContext(fixed);
> > +    m_page->settings()->setApplyDeviceScaleFactorInCompositor(fixed);
> > +    m_page->settings()->setApplyPageScaleFactorInCompositor(fixed);
> >  #endif
> 
> I tihnk we are already setting these when creating our EwkView (or we should be)

we can not set in UI process because these api are not open via WebPreference.

[Kenneth Rohde Christiansen]

LGTM, please have a WebKit2 owner review

[Noam Rosenthal]

(In reply to comment #32)
> LGTM, please have a WebKit2 owner review
LGTM as well. A step forward towards having less home-brew code around...

[Simon Fraser (smfr)]

Comment on attachment 185449 [details]
Patch

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

> Source/WebKit2/UIProcess/PageViewportController.cpp:201
> +        // FIXME : Page::setPageScaleFactor performs layout even if the scale is not changed.

Does that layout actually do any work though?

[Dongseong Hwang]

(In reply to comment #34)
> (From update of attachment 185449 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185449&action=review
> 
> > Source/WebKit2/UIProcess/PageViewportController.cpp:201
> > +        // FIXME : Page::setPageScaleFactor performs layout even if the scale is not changed.
> 
> Does that layout actually do any work though?

Thank you for review!

after r141053 (Bug 107424) the comment became stale.
I mean that there is no layout with applyPageScaleInCompositor.

I'll get rid of the comment and the cq+.

[Dongseong Hwang]

Created attachment 185556 [details]
Patch for landing

[Dongseong Hwang]

Thank mikhail for cq+ :)

[WebKit Review Bot]

Comment on attachment 185556 [details]
Patch for landing

Clearing flags on attachment: 185556

Committed r141310: <http://trac.webkit.org/changeset/141310>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.