Bug 107424

Created attachment 183725 [details] Patch

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.

Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

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

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.

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.

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

Created attachment 184693 [details] Patch Add tests; fix layoutSize() in non-fixed-layout

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

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.

Created attachment 184836 [details] Patch Rebase to 140872

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?

(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.

Created attachment 185005 [details] Patch Rebase to 140978

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?

(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.

Created attachment 185064 [details] Patch Added detailed ChangeLogs; simplified unscaledContentsSize and rename to contentsSize()

(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!

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 :)

Created attachment 185103 [details] Patch Fix Changelogs; Fix DivAutoZoomScaleFontScaleFactorTestCompositorScaling not to use >1 minimum scale

Comment on attachment 185103 [details] Patch I take it back, these are still really long :-/

Created attachment 185107 [details] Patch for landing

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

Comment on attachment 185107 [details] Patch for landing Clearing flags on attachment: 185107 Committed r141053: <http://trac.webkit.org/changeset/141053>

All reviewed patches have been landed. Closing bug.

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.

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

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:********************************************************************************

Hi Alexandre, it is a great work. Now EFL and QT use applyPageScaleInCompositor. See Bug 105978 I'll ask you something frequently :) Good luck!

(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.

(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! :)

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.

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.

(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.)