Bug 107424 – Make page scale shrink FrameView in applyPageScaleInCompositor mode

• Component: New Bugs
• Status: RESOLVED FIXED

[Alexandre Elias]

Make page scale shrink FrameView in applyPageScaleInCompositor mode

[Alexandre Elias]

Created attachment 183725 [details]
Patch

[Alexandre Elias]

Comment on attachment 183725 [details]
Patch

Hi, this patch improves the functionality of pageScaleFactor when the setting applyPageScaleInCompositor is true.  This is a setting we introduced for Chromium in http://trac.webkit.org/changeset/130321 that disables the document-level CSS scale transform associated with pageScaleFactor and makes frameScaleFactor() always return 1 (since that function is generally used to cancel the effect of the scale transform).  The CSS transform has proven to be a source of complexity and bugs for us so we prefer to have our compositor manage scaling.  However, this setting will be even more useful if the FrameView understands that the viewport size shrinks in response to scale and can therefore scroll normally, thus this patch.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Build Bot]

Comment on attachment 183725 [details]
Patch

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

New failing tests:
compositing/iframes/scrolling-iframe.html
compositing/iframes/connect-compositing-iframe.html
compositing/iframes/overlapped-iframe.html
compositing/iframes/enter-compositing-iframe.html
compositing/iframes/become-overlapped-iframe.html
compositing/visible-rect/iframe-and-layers.html
compositing/iframes/composited-parent-iframe.html
compositing/iframes/invisible-nested-iframe-show.html
compositing/iframes/connect-compositing-iframe-delayed.html
compositing/iframes/page-cache-layer-tree.html
compositing/iframes/resizer.html
compositing/iframes/connect-compositing-iframe2.html
compositing/iframes/iframe-resize.html
compositing/iframes/connect-compositing-iframe3.html

[W. James MacLean]

Comment on attachment 183725 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:2815
> +    if (!m_frame->settings()->applyPageScaleFactorInCompositor() || m_frame != m_frame->page()->mainFrame())

Will we ever have a frame that's not a main frame that sets scale in the compositor? Perhaps a comment here would explain that case.

[Simon Fraser (smfr)]

Comment on attachment 183725 [details]
Patch

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

> Source/WebCore/platform/ScrollView.h:155
> +    IntSize unscaledContentSize(bool includeScrollbars) const;
> +    virtual float visibleContentScaleFactor() const { return 1; }

You need some comments here to say how these are different from the existing, related methods.

[Alexandre Elias]

Created attachment 184043 [details]
Patch

Rename to unscaledVisibleContentSize and add comment; fix HistoryController with our setting; ceil instead of flooring the scaled size; make unscaledVisibleContentSize consider platformVisibleContentRect and fixedVisibleContentRect special cases

[Alexandre Elias]

Created attachment 184693 [details]
Patch

Add tests; fix layoutSize() in non-fixed-layout

[WebKit Review Bot]

Comment on attachment 184693 [details]
Patch

Attachment 184693 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16111524

New failing tests:
inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html

[Adam Barth]

Who is the right person to review this change?  I could potentially review the parts in  WebKit/chromium/src, but I'm not sure I understand the changes to WebCore in enough detail to review them well.

[Alexandre Elias]

Created attachment 184836 [details]
Patch

Rebase to 140872

[Simon Fraser (smfr)]

Comment on attachment 184836 [details]
Patch

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

> Source/WebCore/loader/HistoryController.cpp:92
> +    Page* page = m_frame->page();
> +    if (page && page->mainFrame() == m_frame)
> +        item->setPageScaleFactor(page->pageScaleFactor());

Your change log says "Since all behavior changes are tied to applyPageScaleFactorInCompositor", but that doesn't seem to be the case here. Is this a bug fix? Can it be tested?

[Alexandre Elias]

(In reply to comment #12)
> (From update of attachment 184836 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184836&action=review
> 
> > Source/WebCore/loader/HistoryController.cpp:92
> > +    Page* page = m_frame->page();
> > +    if (page && page->mainFrame() == m_frame)
> > +        item->setPageScaleFactor(page->pageScaleFactor());
> 
> Your change log says "Since all behavior changes are tied to applyPageScaleFactorInCompositor", but that doesn't seem to be the case here. Is this a bug fix? Can it be tested?

This is a fix specific to applyPageScaleFactorInCompositor, as frameScaleFactor() on the main frame returns pageScaleFactor() when that setting is false and 1 when the setting is true (and subframe values are ignored at read time).  I have a test for it in WebFrameTest::pageScaleFactorWrittenToHistoryItem.

[Alexandre Elias]

Created attachment 185005 [details]
Patch

Rebase to 140978

[Levi Weintraub]

Comment on attachment 185005 [details]
Patch

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

> Source/WebCore/ChangeLog:28
> +        New unit tests in WebFrameTest.cpp.

I'm a little worried about this. Do the Chromium EWS bots run these tests? If not, it'll be easy for other ports to break this and have it not caught until its downstream.

> Source/WebKit/chromium/ChangeLog:40
> +        (WebKit::WebViewImpl::scaledSize):
> +        (WebKit::WebViewImpl::size):
> +        (WebKit::WebViewImpl::resize):
> +        (WebKit::WebViewImpl::handleInputEvent):
> +        (WebKit::WebViewImpl::clampOffsetAtScale):
> +        (WebKit::WebViewImpl::setPageScaleFactorPreservingScrollOffset):
> +        (WebKit::WebViewImpl::setPageScaleFactor):
> +        (WebKit::WebViewImpl::unscaledContentsSize):
> +        (WebKit::WebViewImpl::layoutSize):
> +        (WebKit::WebViewImpl::computePageScaleFactorLimits):
> +        (WebKit::WebViewImpl::didChangeContentsSize):
> +        (WebKit::WebViewImpl::updateLayerTreeViewport):

It'd be great to get a sentence or two describing what's getting changed here to make it easier to review this. I'm not the most familiar with WebViewImpl code. It couldn't hurt for the WebCore changes either.

> Source/Platform/chromium/public/WebLayerTreeView.h:106
> +    // FIXME: remove this after WebKit roll
> +    virtual WebFloatPoint adjustEventPointForPinchZoom(const WebFloatPoint& p) const { return p; }

You may want to file a bug to make sure this gets tracked.

> Source/WebKit/chromium/src/WebViewImpl.cpp:3063
> +    if (!m_page->settings()->applyPageScaleFactorInCompositor())
> +        return root->unscaledDocumentRect().size();
> +    return root->documentRect().size();

If we're using applyPageScaleFactorInCompositor, this code seems a little confusing. Will documentRect == unscaledDocumentRect? If so, can we ASSERT that?

[Adam Barth]

(In reply to comment #15)
> (From update of attachment 185005 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185005&action=review
> 
> > Source/WebCore/ChangeLog:28
> > +        New unit tests in WebFrameTest.cpp.
> 
> I'm a little worried about this. Do the Chromium EWS bots run these tests? If not, it'll be easy for other ports to break this and have it not caught until its downstream.

Thanks to jamesr, the EWS does run these tests.

[Alexandre Elias]

Created attachment 185064 [details]
Patch

Added detailed ChangeLogs; simplified unscaledContentsSize and rename to contentsSize()

[Levi Weintraub]

(In reply to comment #16)
> (In reply to comment #15)
> > (From update of attachment 185005 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=185005&action=review
> > 
> > > Source/WebCore/ChangeLog:28
> > > +        New unit tests in WebFrameTest.cpp.
> > 
> > I'm a little worried about this. Do the Chromium EWS bots run these tests? If not, it'll be easy for other ports to break this and have it not caught until its downstream.
> 
> Thanks to jamesr, the EWS does run these tests.

Super awesome!

[Levi Weintraub]

Comment on attachment 185064 [details]
Patch

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

Okay. This looks good to me.

> Source/WebCore/ChangeLog:31
> +        (WebCore::HistoryController::saveScrollPositionAndViewStateToItem): Use pageScaleFactor here because frameScaleFactor always returns 1 with our setting.

Nit: consistent line wrapping.

> Source/WebCore/ChangeLog:53
> +        (WebCore::RenderLayerCompositor::updateRootLayerPosition): Clip layer should use unscaled size.
> +        * rendering/TextAutosizer.cpp:
> +        (WebCore::TextAutosizer::processSubtree): Text autosizer should use unscaled size.

Saying "why" would be nice, though I understand these cases.

> Source/WebKit/chromium/ChangeLog:40
> +        (WebKit::WebViewImpl::scaledSize): Returns the post page-scale size similar to what visibleContentRect() now returns, except that it may be at a different scale than the current one.
> +        (WebKit::WebViewImpl::size): Back to returning density-independent size without any tricks, not the "layoutSize()" fake viewport.
> +        (WebKit::WebViewImpl::resize):
> +        (WebKit::WebViewImpl::handleInputEvent): No need to apply implTransform anymore as WebKit knows the true scroll offset; just divide event coords by pageScaleFactor.
> +        (WebKit::WebViewImpl::clampOffsetAtScale): Make this method support applyPageScaleFactorInCompositor.  This is used to pre-clamp scroll offsets at a given viewport size.
> +        (WebKit::WebViewImpl::setPageScaleFactorPreservingScrollOffset): Make this method support applyPageScaleFactorInCompositor (don't scale scroll offsets as they are now scale-independent).
> +        (WebKit::WebViewImpl::setPageScaleFactor): Make this method always use clampOffsetAtScale instead of bypassing it, since it's now supported.  Also notify the compositor to update its state.
> +        (WebKit::WebViewImpl::contentsSize): Convenience method, removed difference between scaled and unscaled.
> +        (WebKit::WebViewImpl::layoutSize): This method returned the "fake" size we used to give FrameView.  Now no longer used for much.
> +        (WebKit::WebViewImpl::computePageScaleFactorLimits):
> +        (WebKit::WebViewImpl::didChangeContentsSize): Remove unnecessary resize() now that we can give the true size to FrameView.
> +        (WebKit::WebViewImpl::updateLayerTreeViewport): Use layoutSize() directly now that FrameView no longer uses it.

Thanks! This makes it a lot easier to follow. nit: super long lines.

> Source/Platform/chromium/public/WebLayerTreeView.h:106
> +    // FIXME: remove this after WebKit roll
> +    virtual WebFloatPoint adjustEventPointForPinchZoom(const WebFloatPoint& p) const { return p; }

Make sure we get this :)

[Alexandre Elias]

Created attachment 185103 [details]
Patch

Fix Changelogs; Fix DivAutoZoomScaleFontScaleFactorTestCompositorScaling not to use >1 minimum scale

[Levi Weintraub]

Comment on attachment 185103 [details]
Patch

I take it back, these are still really long :-/

[Alexandre Elias]

Created attachment 185107 [details]
Patch for landing

[Alexandre Elias]

Done.  Sorry, I git stashed those ChangeLog fixes accidentally in my last upload.

[WebKit Review Bot]

Comment on attachment 185107 [details]
Patch for landing

Clearing flags on attachment: 185107

Committed r141053: <http://trac.webkit.org/changeset/141053>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Florin Malita]

This is causing webkit_unit_tests failures on the Android canary bots: http://build.chromium.org/p/chromium.webkit/builders/Android%20Tests%20%28dbg%29/builds/5935/steps/webkit_unit_tests/logs/stdio

CRITICAL:root:Failed:
CRITICAL:root:WebFrameTest.pageScaleFactorShrinksViewport
CRITICAL:root:third_party/WebKit/Source/WebKit/chromium/tests/WebFrameTest.cpp:384: Failure
Value of: unscaledSizeMinusScrollbar.width()
  Actual: 640
Expected: viewportWidthMinusScrollbar
Which is: 625
third_party/WebKit/Source/WebKit/chromium/tests/WebFrameTest.cpp:385: Failure
Value of: unscaledSizeMinusScrollbar.height()
  Actual: 480
Expected: viewportHeightMinusScrollbar
Which is: 465
third_party/WebKit/Source/WebKit/chromium/tests/WebFrameTest.cpp:388: Failure
Value of: scaledSize.width()
  Actual: 320
Expected: ceil(viewportWidthMinusScrollbar / 2.0)
Which is: 313
third_party/WebKit/Source/WebKit/chromium/tests/WebFrameTest.cpp:389: Failure
Value of: scaledSize.height()
  Actual: 240
Expected: ceil(viewportHeightMinusScrollbar / 2.0)
Which is: 233


Please take a look.

[Alexandre Elias]

Thanks, I'll investigate.  Let's not revert over that.

[Florin Malita]

Also: http://build.chromium.org/p/chromium.webkit/builders/Android%20Tests%20%28dbg%29/builds/5935/steps/contentshell_instrumentation_tests/logs/stdio

CRITICAL:root:Failed:
CRITICAL:root:org.chromium.content.browser.ViewportTest#testDefaultViewportSize
CRITICAL:root:junit.framework.AssertionFailedError
	at org.chromium.content.browser.ViewportTest.testDefaultViewportSize(ViewportTest.java:66)
	at java.lang.reflect.Method.invokeNative(Native Method)
	at android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:214)
	at android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:199)
	at android.test.ActivityInstrumentationTestCase2.runTest(ActivityInstrumentationTestCase2.java:192)
	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:169)
	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:154)
	at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:545)
	at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1551)
CRITICAL:root:********************************************************************************

[Dongseong Hwang]

Hi Alexandre, it is a great work.

Now EFL and QT use applyPageScaleInCompositor. See Bug 105978
I'll ask you something frequently :)
Good luck!

[Alexandre Elias]

(In reply to comment #29)
> Hi Alexandre, it is a great work.
> 
> Now EFL and QT use applyPageScaleInCompositor. See Bug 105978
> I'll ask you something frequently :)
> Good luck!

Already?  Nice :).

If you need to make any further patches that change WebCore scaling behavior when the setting is enabled, please cc me on them.  Hopefully those would be fixing bugs affecting both platforms.

[Dongseong Hwang]

(In reply to comment #30)
> (In reply to comment #29)
> Already?  Nice :).
> 
> If you need to make any further patches that change WebCore scaling behavior when the setting is enabled, please cc me on them.  Hopefully those would be fixing bugs affecting both platforms.

Sure, I'll cc you and Levi.

When I implemented Bug 105978, I thought recalcStyle and layout is not necessary, so I filed Bug 107361.
You resolved it. great! :)

[Dongseong Hwang]

Comment on attachment 185107 [details]
Patch for landing

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

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1172
> +        m_clipLayer->setSize(frameView->unscaledVisibleContentSize(false /* exclude scrollbars */));

Hi!
Could you please explain why clip-layer size is exception?
In my understand, we should clip the size of viewport, and from the view of m_clipLayer, the size of viewport is frameView->visibleContentRect().

For instance, let assume the following case,
contentsSize (100, 200)
deviceViewport (100, 100)
pageScaleFactor 2

And then
frameView->visibleContentRect : (50, 50)
frameView->unscaledVisibleContentSize : (100, 100)

In this case, clipping with (50, 50) is enough because we finally draw (50, 50), not (100, 100).


In addition, Qt and EFL use m_fixedVisibleContentRect in ScrollView. It means visibleContentRect() and unscaledVisibleContentSize() return the same value : shrinked viewport.
It is why I ask. This is the only case that Qt and EFL use unscaledVisibleContentSize(). (Qt and EFL use fixed-layout mode)

BTW, I agree that in non-fixed-layout mode, we should use unscaledVisibleContentSize() for getting layout rect to avoid frequent layout.

[Alexandre Elias]

Hi Huang,

The reason is that in Chromium, pageScaleFactor is applied on the root scroll layer.  (This is the same place pageScaleFactor is applied by WebCore when applyPageScaleFactorInCompositor == false).  The clip layer is the parent of the root scroll layer, therefore it needs to be in physical pixels.

[Dongseong Hwang]

(In reply to comment #33)
> Hi Huang,
> 
> The reason is that in Chromium, pageScaleFactor is applied on the root scroll layer.  (This is the same place pageScaleFactor is applied by WebCore when applyPageScaleFactorInCompositor == false).  The clip layer is the parent of the root scroll layer, therefore it needs to be in physical pixels.

Thank you for explanation! However, I don't fully understand yet.

We always call GraphicsLayer::setSize() with the size in the css units.
For example, let's consider that <div width='100' height='50' />.
we always call GraphicsLayer::setSize(IntSize(100, 50)) regardless of page scale factor and device scale factor.

However, chromium calls setSize() of the clip layer with the size in physical pixel unit, right?
If so, I think it is a bit confused, and we need some refactoring...

btw, I think ScrollView::visibleContentRect() is enough for the clip layer on EFL and Qt, because compositor of EFL and Qt will draw contents with the effective scale ( = page scale X device scale).
Do you think am I right? (even though already ScrollView::visibleContentRect() is the same to ScrollView::unscaledVisibleContentSize() on EFL and Qt.)