Bug 72983 - [chromium] Preserve original pageScale limits in WebViewImpl
Summary: [chromium] Preserve original pageScale limits in WebViewImpl
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: 70559 73495
  Show dependency treegraph
 
Reported: 2011-11-22 12:47 PST by Alexandre Elias
Modified: 2011-12-07 17:54 PST (History)
7 users (show)

See Also:


Attachments
Patch (6.03 KB, patch)
2011-11-22 12:49 PST, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (8.02 KB, patch)
2011-11-22 23:56 PST, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (8.45 KB, patch)
2011-11-25 02:25 PST, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (8.61 KB, patch)
2011-12-01 00:17 PST, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (8.88 KB, patch)
2011-12-01 13:09 PST, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (8.90 KB, patch)
2011-12-07 11:24 PST, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (9.12 KB, patch)
2011-12-07 11:49 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 2011-11-22 12:47:24 PST
[chromium] Preserve original pageScale limits in WebViewImpl
Comment 1 Alexandre Elias 2011-11-22 12:49:15 PST
Created attachment 116264 [details]
Patch
Comment 2 Fady Samuel 2011-11-22 12:57:16 PST
This looks fine to me. Thanks.
Comment 3 Alexandre Elias 2011-11-22 23:56:07 PST
Created attachment 116324 [details]
Patch
Comment 4 Alexandre Elias 2011-11-22 23:58:22 PST
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 5 WebKit Review Bot 2011-11-23 04:17:44 PST
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
Comment 6 Alexandre Elias 2011-11-25 02:25:15 PST
Created attachment 116586 [details]
Patch
Comment 7 Alexandre Elias 2011-12-01 00:17:37 PST
Created attachment 117361 [details]
Patch
Comment 8 Fady Samuel 2011-12-01 08:14:01 PST
(In reply to comment #7)
> Created an attachment (id=117361) [details]
> Patch

Why do we need to force a layout when contents size changes?
Comment 9 Fady Samuel 2011-12-01 08:16:18 PST
Making this block viewport upstreaming so I remember to keep an eye on this patch.
Comment 10 Alexandre Elias 2011-12-01 10:57:21 PST
> 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
Comment 11 Fady Samuel 2011-12-01 11:01:28 PST
(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 :-)
Comment 12 Alexandre Elias 2011-12-01 13:09:40 PST
Created attachment 117470 [details]
Patch
Comment 13 Darin Fisher (:fishd, Google) 2011-12-07 10:27:00 PST
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?
Comment 14 Alexandre Elias 2011-12-07 11:24:37 PST
Created attachment 118238 [details]
Patch
Comment 15 Alexandre Elias 2011-12-07 11:25:57 PST
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().
Comment 16 Alexandre Elias 2011-12-07 11:48:14 PST
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.
Comment 17 Alexandre Elias 2011-12-07 11:49:12 PST
Created attachment 118246 [details]
Patch
Comment 18 WebKit Review Bot 2011-12-07 17:53:57 PST
Comment on attachment 118246 [details]
Patch

Clearing flags on attachment: 118246

Committed r102292: <http://trac.webkit.org/changeset/102292>
Comment 19 WebKit Review Bot 2011-12-07 17:54:03 PST
All reviewed patches have been landed.  Closing bug.