Bug 107424 - Make page scale shrink FrameView in applyPageScaleInCompositor mode
Summary: Make page scale shrink FrameView in applyPageScaleInCompositor mode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexandre Elias
URL:
Keywords:
Depends on:
Blocks: 111532 105978 107391 108561
  Show dependency treegraph
 
Reported: 2013-01-21 00:10 PST by Alexandre Elias
Modified: 2013-03-06 00:09 PST (History)
25 users (show)

See Also:


Attachments
Patch (25.34 KB, patch)
2013-01-21 00:19 PST, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (26.91 KB, patch)
2013-01-22 13:43 PST, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (35.99 KB, patch)
2013-01-25 01:11 PST, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (36.18 KB, patch)
2013-01-25 16:20 PST, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (36.15 KB, patch)
2013-01-28 10:41 PST, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (37.49 KB, patch)
2013-01-28 14:32 PST, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (38.06 KB, patch)
2013-01-28 16:43 PST, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch for landing (38.42 KB, patch)
2013-01-28 16:54 PST, Alexandre Elias
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexandre Elias 2013-01-21 00:10:08 PST
Make page scale shrink FrameView in applyPageScaleInCompositor mode
Comment 1 Alexandre Elias 2013-01-21 00:19:49 PST
Created attachment 183725 [details]
Patch
Comment 2 Alexandre Elias 2013-01-22 00:48:15 PST
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.
Comment 3 WebKit Review Bot 2013-01-22 00:52:10 PST
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 4 Build Bot 2013-01-22 02:00:52 PST
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 5 W. James MacLean 2013-01-22 09:09:03 PST
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 6 Simon Fraser (smfr) 2013-01-22 11:06:33 PST
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.
Comment 7 Alexandre Elias 2013-01-22 13:43:23 PST
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
Comment 8 Alexandre Elias 2013-01-25 01:11:16 PST
Created attachment 184693 [details]
Patch

Add tests; fix layoutSize() in non-fixed-layout
Comment 9 WebKit Review Bot 2013-01-25 04:37:12 PST
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
Comment 10 Adam Barth 2013-01-25 09:40:45 PST
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.
Comment 11 Alexandre Elias 2013-01-25 16:20:21 PST
Created attachment 184836 [details]
Patch

Rebase to 140872
Comment 12 Simon Fraser (smfr) 2013-01-25 16:34:36 PST
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?
Comment 13 Alexandre Elias 2013-01-25 16:39:42 PST
(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.
Comment 14 Alexandre Elias 2013-01-28 10:41:25 PST
Created attachment 185005 [details]
Patch

Rebase to 140978
Comment 15 Levi Weintraub 2013-01-28 13:17:18 PST
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?
Comment 16 Adam Barth 2013-01-28 14:03:43 PST
(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.
Comment 17 Alexandre Elias 2013-01-28 14:32:32 PST
Created attachment 185064 [details]
Patch

Added detailed ChangeLogs; simplified unscaledContentsSize and rename to contentsSize()
Comment 18 Levi Weintraub 2013-01-28 15:30:34 PST
(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 19 Levi Weintraub 2013-01-28 15:41:25 PST
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 :)
Comment 20 Alexandre Elias 2013-01-28 16:43:42 PST
Created attachment 185103 [details]
Patch

Fix Changelogs; Fix DivAutoZoomScaleFontScaleFactorTestCompositorScaling not to use >1 minimum scale
Comment 21 Levi Weintraub 2013-01-28 16:49:06 PST
Comment on attachment 185103 [details]
Patch

I take it back, these are still really long :-/
Comment 22 Alexandre Elias 2013-01-28 16:54:26 PST
Created attachment 185107 [details]
Patch for landing
Comment 23 Alexandre Elias 2013-01-28 16:55:35 PST
Done.  Sorry, I git stashed those ChangeLog fixes accidentally in my last upload.
Comment 24 WebKit Review Bot 2013-01-28 22:20:42 PST
Comment on attachment 185107 [details]
Patch for landing

Clearing flags on attachment: 185107

Committed r141053: <http://trac.webkit.org/changeset/141053>
Comment 25 WebKit Review Bot 2013-01-28 22:20:48 PST
All reviewed patches have been landed.  Closing bug.
Comment 26 Florin Malita 2013-01-29 08:34:56 PST
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.
Comment 27 Alexandre Elias 2013-01-29 09:13:37 PST
Thanks, I'll investigate.  Let's not revert over that.
Comment 28 Florin Malita 2013-01-29 10:11:44 PST
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:********************************************************************************
Comment 29 Dongseong Hwang 2013-01-30 14:31:18 PST
Hi Alexandre, it is a great work.

Now EFL and QT use applyPageScaleInCompositor. See Bug 105978
I'll ask you something frequently :)
Good luck!
Comment 30 Alexandre Elias 2013-01-30 17:05:08 PST
(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.
Comment 31 Dongseong Hwang 2013-01-30 19:34:00 PST
(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 32 Dongseong Hwang 2013-02-04 20:12:35 PST
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.
Comment 33 Alexandre Elias 2013-02-04 20:24:07 PST
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.
Comment 34 Dongseong Hwang 2013-02-04 21:26:05 PST
(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.)