| Summary: | [chromium] Add draw-time scale delta to compositor thread | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Alexandre Elias <aelias> | ||||||||||||||||||||||||||
| Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||
| Severity: | Normal | CC: | aelias, dglazkov, enne, fishd, jamesr, nduca, webkit.review.bot, wjmaclean | ||||||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||
|
Description
Alexandre Elias
2011-11-04 16:58:56 PDT
Created attachment 113735 [details]
Patch
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API. 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. Created attachment 113742 [details]
Patch
Fixed bug with scrollbar maxScrollPosition (needed a clampNegativeToZero). 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? Created attachment 113803 [details]
Patch
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. 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? 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 Comment on attachment 113803 [details]
Patch
WebKit API changes LGTM
Created attachment 113985 [details]
Patch
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. Created attachment 114153 [details]
Patch
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? 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 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
Created attachment 114170 [details]
Patch
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. 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.
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 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
Comment on attachment 114170 [details]
Patch
cr-linux EWS failure seems unrelated, can't repro locally.
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 Ah, the string you want is "NOBODY (OOPS!)." Created attachment 114299 [details]
Patch
Ah, oops :). Switched to "NOBODY (OOPS!)" and changed nothing else. 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 Created attachment 114327 [details]
Patch
That was an error hidden in a DEBUG-build #ifdef. Fixed and checked that there aren't any others. 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 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
Created attachment 114371 [details]
Patch
Fixed updateLayerTreeViewport NULL pointer access causing the layout test failure. Comment on attachment 114371 [details] Patch Clearing flags on attachment: 114371 Committed r99774: <http://trac.webkit.org/changeset/99774> All reviewed patches have been landed. Closing bug. |