RESOLVED FIXED 71595
[chromium] Add draw-time scale delta to compositor thread
https://bugs.webkit.org/show_bug.cgi?id=71595
Summary [chromium] Add draw-time scale delta to compositor thread
Alexandre Elias
Reported Saturday, November 5, 2011 12:58:56 AM UTC
[chromium] Add draw-time scale delta to compositor thread
Attachments
Patch (46.12 KB, patch)
2011-11-04 16:59 PDT, Alexandre Elias
no flags
Patch (46.33 KB, patch)
2011-11-04 17:52 PDT, Alexandre Elias
no flags
Patch (46.52 KB, patch)
2011-11-06 14:32 PST, Alexandre Elias
no flags
Patch (50.03 KB, patch)
2011-11-07 21:36 PST, Alexandre Elias
no flags
Patch (50.40 KB, patch)
2011-11-08 14:22 PST, Alexandre Elias
no flags
Archive of layout-test-results from ec2-cr-linux-03 (42.30 KB, application/zip)
2011-11-08 16:07 PST, WebKit Review Bot
no flags
Patch (51.43 KB, patch)
2011-11-08 16:15 PST, Alexandre Elias
no flags
Archive of layout-test-results from ec2-cr-linux-01 (42.29 KB, application/zip)
2011-11-08 18:07 PST, WebKit Review Bot
no flags
Patch (51.44 KB, patch)
2011-11-09 09:06 PST, Alexandre Elias
no flags
Patch (54.25 KB, patch)
2011-11-09 11:33 PST, Alexandre Elias
no flags
Archive of layout-test-results from ec2-cr-linux-01 (42.34 KB, application/zip)
2011-11-09 12:17 PST, WebKit Review Bot
no flags
Patch (54.42 KB, patch)
2011-11-09 14:37 PST, Alexandre Elias
no flags
Alexandre Elias
Comment 1 Saturday, November 5, 2011 12:59:55 AM UTC
WebKit Review Bot
Comment 2 Saturday, November 5, 2011 1:02:17 AM UTC
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Alexandre Elias
Comment 3 Saturday, November 5, 2011 1:05:27 AM UTC
Patch description: Add draw-time scale delta to compositor thread. - There are two magnify values: 1. The WebKit-side pageScale value corresponding to the resolution of the painted content. 2. The Impl-side-only scaleDelta value representing the additional degree of current magnification, applied as a draw transform. WebKit is never explicitly aware of this value and the only way to change it currently is via pinch events (and in the future, double-tap/etc events). At the end of a pinch zooms, we multiply pageScale * scaleDelta and tell WebKit to use it as the new pageScale. - Scroll offsets and max-extents are maintained in pageScale space. When the scale changes, we need to take care to convert them to the new scale and avoid getting them clamped to the extents at the wrong scale. - Remove maxScrollPosition on the non-impl side because it makes no sense to precompute it in the presence of an impl-side zoom. - zoomAnimatorTransform is not used because it currently isn't in the branch, and mostly doesn't behave the way we want anyway (it does most things in WebKit thread). We will need to unify these later. - Also introduce deviceOrPageScaleFactorChanged() call which invalidates layers.
Alexandre Elias
Comment 4 Saturday, November 5, 2011 1:52:34 AM UTC
Alexandre Elias
Comment 5 Saturday, November 5, 2011 1:53:56 AM UTC
Fixed bug with scrollbar maxScrollPosition (needed a clampNegativeToZero).
Darin Fisher (:fishd, Google)
Comment 6 Saturday, November 5, 2011 11:01:52 PM UTC
Comment on attachment 113742 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113742&action=review > Source/WebKit/chromium/public/WebLayerTreeViewClient.h:44 > + virtual void applyScrollAndScale(const WebSize&, float) = 0; nit: please give the float parameter a name. i think "scaleFactor" would work. the WebSize parameter could probably also benefit from a name: "scrollDelta" > Source/WebKit/chromium/public/WebView.h:212 > + virtual void setScaleLimits(float minScale, float maxScale) = 0; since this impacts "PageScaleFactor", it seems like the function name should include that term. how about calling this function setPageScaleFactorLimits?
Alexandre Elias
Comment 7 Sunday, November 6, 2011 10:32:46 PM UTC
Alexandre Elias
Comment 8 Sunday, November 6, 2011 10:33:53 PM UTC
Comment on attachment 113742 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113742&action=review >> Source/WebKit/chromium/public/WebLayerTreeViewClient.h:44 >> + virtual void applyScrollAndScale(const WebSize&, float) = 0; > > nit: please give the float parameter a name. i think "scaleFactor" would work. > the WebSize parameter could probably also benefit from a name: "scrollDelta" Done. >> Source/WebKit/chromium/public/WebView.h:212 >> + virtual void setScaleLimits(float minScale, float maxScale) = 0; > > since this impacts "PageScaleFactor", it seems like the function name should include > that term. how about calling this function setPageScaleFactorLimits? Sounds good, renamed everywhere.
Adrienne Walker
Comment 9 Monday, November 7, 2011 8:28:45 PM UTC
Comment on attachment 113803 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113803&action=review > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:214 > + if (firstResize || m_pageScaleDirty) Can you explain why the full-layer invalidation needs to happen in setBounds? Also, I'm a little confused why you need to do this invalidation yourself. Shouldn't WebKit invalidate everything when the scale changes? And, if it somehow doesn't, why not just call LayerChromium::setNeedsDisplay() instead of calling deviceOrPageScaleFactorChanged?
James Robinson
Comment 10 Monday, November 7, 2011 10:27:10 PM UTC
Comment on attachment 113803 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113803&action=review Compositor stuff here looks great. I left a few nits. It looks like the bots weren't able to apply this patch to ToT - could you merge it up and post a new patch so the bots can double-check it? > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:101 > + void setPageMagnifyTransform(const TransformationMatrix& t) { m_pageMagnifyMatrix = t; } we don't typically use one-letter variable names in WebKit even for dumb stuff like this > Source/WebCore/platform/graphics/chromium/NonCompositedContentHost.cpp:74 > + bool pageScaleChanged = m_graphicsLayer->pageScaleFactor() != pageScale; > + if (pageScaleChanged) i think the bool here is kind of unnecessary, just inline the check into the if() > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:219 > + float m_minScale, m_maxScale; nit: could you call these m_minPageScale / m_maxPageScale? > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:175 > bool changed = viewportSize != m_viewportSize; > m_viewportSize = viewportSize; > - if (changed) > + if (changed) { > + updateMaxScrollPosition(); > m_layerRenderer->viewportChanged(); this function would be a little more idomatic if it had an early-return at the top if m_viewportSize == viewportSize and the rest was unguarded. i think it's structured this way for historical reasons (there used to be a lot more going on in here) that aren't relevant any more > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:208 > + bool changed = delta != m_scaleDelta; > + m_scaleDelta = delta; > + > + if (changed) { also here - early return would be more in line with the rest of the code
Darin Fisher (:fishd, Google)
Comment 11 Monday, November 7, 2011 11:26:37 PM UTC
Comment on attachment 113803 [details] Patch WebKit API changes LGTM
Alexandre Elias
Comment 12 Tuesday, November 8, 2011 5:36:49 AM UTC
Alexandre Elias
Comment 13 Tuesday, November 8, 2011 5:43:45 AM UTC
OK, all style nits applied. I also synced up the client. Note that this change conflicted with enne's clipping fix because I removed m_maxScrollPosition from the non-impl side. I needed to introduce a "m_scrollable" boolean to allow the root layer to be discovered, and obtained the content bounds from a child layer. (In reply to comment #9) > (From update of attachment 113803 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=113803&action=review > > > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:214 > > + if (firstResize || m_pageScaleDirty) > > Can you explain why the full-layer invalidation needs to happen in setBounds? > > Also, I'm a little confused why you need to do this invalidation yourself. Shouldn't WebKit invalidate everything when the scale changes? And, if it somehow doesn't, why not just call LayerChromium::setNeedsDisplay() instead of calling deviceOrPageScaleFactorChanged? It's because the invalidation is clipped to the bounds, and the bounds are multiplied by the scale, so we need to defer it until after they're changed. There may be a cleaner way to do this, but it doesn't seem like too bad of a hack for now.
Alexandre Elias
Comment 14 Tuesday, November 8, 2011 10:22:04 PM UTC
James Robinson
Comment 15 Tuesday, November 8, 2011 11:27:43 PM UTC
Comment on attachment 114153 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=114153&action=review > Source/WebCore/ChangeLog:6 > + Reviewed by James Robinson. getting a bit ahead of yourself there I think :) > Source/WebCore/platform/graphics/chromium/LayerChromium.h:135 > + bool scrollable() const { return m_scrollable; } > + void setScrollable(bool scrollable) { m_scrollable = true; } can you update LayerChromiumTest.cpp's checkPropertyChangeCausesCorrectBehavior test for this property?
WebKit Review Bot
Comment 16 Wednesday, November 9, 2011 12:07:55 AM UTC
Comment on attachment 114153 [details] Patch Attachment 114153 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10374228 New failing tests: accessibility/aria-checkbox-checked.html http/tests/appcache/access-via-redirect.php http/tests/appcache/auth.html animations/animation-css-rule-types.html canvas/philip/tests/2d.clearRect+fillRect.alpha0.5.html animations/animation-direction.html canvas/philip/tests/2d.canvas.readonly.html accessibility/aria-checkbox-text.html http/tests/appcache/cyrillic-uri.html http/tests/appcache/credential-url.html http/tests/appcache/crash-when-navigating-away-then-back.html animations/animation-add-events-in-handler.html animations/animation-controller-drt-api.html accessibility/adjacent-continuations-cause-assertion-failure.html canvas/philip/tests/2d.canvas.reference.html accessibility/aria-describedby-on-input.html accessibility/anchor-linked-anonymous-block-crash.html animations/animation-direction-normal.html canvas/philip/tests/2d.clearRect+fillRect.alpha0.html canvas/philip/tests/2d.clearRect+fillRect.basic.html
WebKit Review Bot
Comment 17 Wednesday, November 9, 2011 12:07:59 AM UTC
Created attachment 114169 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Alexandre Elias
Comment 18 Wednesday, November 9, 2011 12:15:56 AM UTC
Alexandre Elias
Comment 19 Wednesday, November 9, 2011 12:17:55 AM UTC
Comment on attachment 114153 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=114153&action=review >> Source/WebCore/ChangeLog:6 >> + Reviewed by James Robinson. > > getting a bit ahead of yourself there I think :) Sorry, don't know the process, reverted. >> Source/WebCore/platform/graphics/chromium/LayerChromium.h:135 >> + void setScrollable(bool scrollable) { m_scrollable = true; } > > can you update LayerChromiumTest.cpp's checkPropertyChangeCausesCorrectBehavior test for this property? Done.
James Robinson
Comment 20 Wednesday, November 9, 2011 1:11:04 AM UTC
Comment on attachment 114170 [details] Patch R=me - holding off on cq for now to see if the cr-linux EWS bot goes red again.
WebKit Review Bot
Comment 21 Wednesday, November 9, 2011 2:07:54 AM UTC
Comment on attachment 114170 [details] Patch Attachment 114170 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10372099 New failing tests: accessibility/aria-checkbox-checked.html http/tests/appcache/access-via-redirect.php http/tests/appcache/auth.html animations/animation-css-rule-types.html canvas/philip/tests/2d.clearRect+fillRect.alpha0.5.html animations/animation-direction.html canvas/philip/tests/2d.canvas.readonly.html accessibility/aria-checkbox-text.html http/tests/appcache/cyrillic-uri.html http/tests/appcache/credential-url.html http/tests/appcache/crash-when-navigating-away-then-back.html animations/animation-add-events-in-handler.html animations/animation-controller-drt-api.html accessibility/adjacent-continuations-cause-assertion-failure.html canvas/philip/tests/2d.canvas.reference.html accessibility/aria-describedby-on-input.html accessibility/anchor-linked-anonymous-block-crash.html animations/animation-direction-normal.html canvas/philip/tests/2d.clearRect+fillRect.alpha0.html canvas/philip/tests/2d.clearRect+fillRect.basic.html
WebKit Review Bot
Comment 22 Wednesday, November 9, 2011 2:07:58 AM UTC
Created attachment 114187 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
James Robinson
Comment 23 Wednesday, November 9, 2011 5:35:44 AM UTC
Comment on attachment 114170 [details] Patch cr-linux EWS failure seems unrelated, can't repro locally.
WebKit Review Bot
Comment 24 Wednesday, November 9, 2011 5:38:30 AM UTC
Comment on attachment 114170 [details] Patch Rejecting attachment 114170 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 NOBODY found in /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog does not appear to be a valid reviewer according to committers.py. ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/10373124
James Robinson
Comment 25 Wednesday, November 9, 2011 5:42:32 AM UTC
Ah, the string you want is "NOBODY (OOPS!)."
Alexandre Elias
Comment 26 Wednesday, November 9, 2011 5:06:33 PM UTC
Alexandre Elias
Comment 27 Wednesday, November 9, 2011 5:19:31 PM UTC
Ah, oops :). Switched to "NOBODY (OOPS!)" and changed nothing else.
WebKit Review Bot
Comment 28 Wednesday, November 9, 2011 6:03:18 PM UTC
Comment on attachment 114299 [details] Patch Rejecting attachment 114299 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: bkit', '--debug', '--chromium', '--update-chromium']" exit_code: 2 tform/graphics/chromium/cc/CCSingleThreadProxy.cpp: In member function 'void WebCore::CCSingleThreadProxy::doCommit()': Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:170: error: 'struct WebCore::CCScrollAndScaleSet' has no member named 'size' make: *** [out/Debug/obj.target/webcore_platform/Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.o] Error 1 make: *** Waiting for unfinished jobs.... Full output: http://queues.webkit.org/results/10370499
Alexandre Elias
Comment 29 Wednesday, November 9, 2011 7:33:49 PM UTC
Alexandre Elias
Comment 30 Wednesday, November 9, 2011 7:35:33 PM UTC
That was an error hidden in a DEBUG-build #ifdef. Fixed and checked that there aren't any others.
WebKit Review Bot
Comment 31 Wednesday, November 9, 2011 8:17:07 PM UTC
Comment on attachment 114327 [details] Patch Attachment 114327 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10395047 New failing tests: accessibility/aria-checkbox-checked.html http/tests/appcache/access-via-redirect.php http/tests/appcache/auth.html animations/animation-css-rule-types.html canvas/philip/tests/2d.clearRect+fillRect.alpha0.5.html animations/animation-direction.html canvas/philip/tests/2d.canvas.readonly.html accessibility/aria-checkbox-text.html http/tests/appcache/cyrillic-uri.html http/tests/appcache/credential-url.html http/tests/appcache/crash-when-navigating-away-then-back.html animations/animation-add-events-in-handler.html animations/animation-controller-drt-api.html accessibility/adjacent-continuations-cause-assertion-failure.html canvas/philip/tests/2d.canvas.reference.html accessibility/aria-describedby-on-input.html accessibility/anchor-linked-anonymous-block-crash.html animations/animation-direction-normal.html canvas/philip/tests/2d.clearRect+fillRect.alpha0.html canvas/philip/tests/2d.clearRect+fillRect.basic.html
WebKit Review Bot
Comment 32 Wednesday, November 9, 2011 8:17:11 PM UTC
Created attachment 114339 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Alexandre Elias
Comment 33 Wednesday, November 9, 2011 10:37:00 PM UTC
Alexandre Elias
Comment 34 Wednesday, November 9, 2011 10:38:02 PM UTC
Fixed updateLayerTreeViewport NULL pointer access causing the layout test failure.
WebKit Review Bot
Comment 35 Wednesday, November 9, 2011 11:48:01 PM UTC
Comment on attachment 114371 [details] Patch Clearing flags on attachment: 114371 Committed r99774: <http://trac.webkit.org/changeset/99774>
WebKit Review Bot
Comment 36 Wednesday, November 9, 2011 11:48:08 PM UTC
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.