Bug 72983

Created attachment 116264 [details] Patch

This looks fine to me. Thanks.

Created attachment 116324 [details] Patch

I ended up adding a call site to the computePageScaleLimits function -- it's generally needed to call it on contents size change. I also put in a couple more clamping fixes that turned up during debugging. Please take another look.

Comment on attachment 116324 [details] Patch Attachment 116324 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10615267 New failing tests: compositing/geometry/fixed-in-composited.html compositing/geometry/limit-layer-bounds-clipping-ancestor.html compositing/geometry/fixed-position-iframe-composited-page-scale-down.html compositing/geometry/limit-layer-bounds-fixed-positioned.html compositing/iframes/become-composited-nested-iframes.html compositing/geometry/vertical-scroll-composited.html compositing/geometry/limit-layer-bounds-transformed-overflow.html compositing/layers-inside-overflow-scroll.html compositing/geometry/fixed-position-composited-page-scale-down.html accessibility/dimensions-include-descendants.html compositing/direct-image-compositing.html compositing/flat-with-transformed-child.html compositing/geometry/tall-page-composited.html compositing/generated-content.html animations/state-at-end-event.html compositing/geometry/transfrom-origin-on-zero-size-layer.html animations/suspend-transform-animation.html animations/opacity-transform-animation.html animations/missing-values-last-keyframe.html animations/missing-values-first-keyframe.html

Created attachment 116586 [details] Patch

Created attachment 117361 [details] Patch

(In reply to comment #7) > Created an attachment (id=117361) [details] > Patch Why do we need to force a layout when contents size changes?

Making this block viewport upstreaming so I remember to keep an eye on this patch.

> Why do we need to force a layout when contents size changes? The contents size change in the middle of FrameView::layout(). If we don't relayout after changing the page scale there, then we violate an assertion that no needs-layout flag can be set coming out of that layout function. See http://b/issue?id=5662731

(In reply to comment #10) > > Why do we need to force a layout when contents size changes? > > The contents size change in the middle of FrameView::layout(). If we don't relayout after changing the page scale there, then we violate an assertion that no needs-layout flag can be set coming out of that layout function. See http://b/issue?id=5662731 Ohh good catch! I was thinking that might've been the case as I wrote the question. I handle this from render_view_impl.cc which is probably not the right place to put it. I prefer your solution, frankly, thanks :-)

Created attachment 117470 [details] Patch

Comment on attachment 117470 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117470&action=review > Source/WebKit/chromium/src/ChromeClientImpl.cpp:558 > + m_webView->didChangeContentsSize(); hmm, are you sure you wouldn't be better off completing your work before delegating out to the embedder? calling webframe->client()->didChangeContentsSize gives control to the embedder. they could drastically change the system in that time, potentially destroying the WebView. usually, it is better to do your work first, and then notify the embedder. > Source/WebKit/chromium/src/WebViewImpl.cpp:2567 > + FrameView* view = mainFrameImpl()->frameView(); what kind of performance impact will this have? it seems like this could cause us to force layout more frequently than we would otherwise do. could this slow down page load? during page load, isn't the content size rapidly changing?

Created attachment 118238 [details] Patch

Comment on attachment 117470 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117470&action=review >> Source/WebKit/chromium/src/ChromeClientImpl.cpp:558 >> + m_webView->didChangeContentsSize(); > > hmm, are you sure you wouldn't be better off completing your work before delegating > out to the embedder? calling webframe->client()->didChangeContentsSize gives control > to the embedder. they could drastically change the system in that time, potentially > destroying the WebView. usually, it is better to do your work first, and then notify > the embedder. OK, moved this call above the embedder call. >> Source/WebKit/chromium/src/WebViewImpl.cpp:2567 >> + FrameView* view = mainFrameImpl()->frameView(); > > what kind of performance impact will this have? it seems like this could cause > us to force layout more frequently than we would otherwise do. could this slow > down page load? during page load, isn't the content size rapidly changing? Touch devices do need a few extra layouts to switch to the correct page scale on page load when the content size is determined. I tried to minimize the force layouts to the minimum that are really needed. computePageScaleFactorLimits will only set needsLayout if the content width changes, which in my logs happens only a few times during load. needsLayout cannot be set for any other reason as didChangeContentsSize is called immediately after the main layout in FrameView::layout(). Also, computePageScaleFactorLimits() is a no-op on platforms which have never called setPageScaleFactorLimits().

On second thought, after discussing with fsamuel, I'm adding an extra early return to make sure we don't force-layout if needsLayout was set for some other unexpected reason.

Created attachment 118246 [details] Patch

Comment on attachment 118246 [details] Patch Clearing flags on attachment: 118246 Committed r102292: <http://trac.webkit.org/changeset/102292>

All reviewed patches have been landed. Closing bug.