WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(46.33 KB, patch)
2011-11-04 17:52 PDT
,
Alexandre Elias
no flags
Details
Formatted Diff
Diff
Patch
(46.52 KB, patch)
2011-11-06 14:32 PST
,
Alexandre Elias
no flags
Details
Formatted Diff
Diff
Patch
(50.03 KB, patch)
2011-11-07 21:36 PST
,
Alexandre Elias
no flags
Details
Formatted Diff
Diff
Patch
(50.40 KB, patch)
2011-11-08 14:22 PST
,
Alexandre Elias
no flags
Details
Formatted Diff
Diff
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
Details
Patch
(51.43 KB, patch)
2011-11-08 16:15 PST
,
Alexandre Elias
no flags
Details
Formatted Diff
Diff
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
Details
Patch
(51.44 KB, patch)
2011-11-09 09:06 PST
,
Alexandre Elias
no flags
Details
Formatted Diff
Diff
Patch
(54.25 KB, patch)
2011-11-09 11:33 PST
,
Alexandre Elias
no flags
Details
Formatted Diff
Diff
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
Details
Patch
(54.42 KB, patch)
2011-11-09 14:37 PST
,
Alexandre Elias
no flags
Details
Formatted Diff
Diff
Show Obsolete
(8)
View All
Add attachment
proposed patch, testcase, etc.
Alexandre Elias
Comment 1
Saturday, November 5, 2011 12:59:55 AM UTC
Created
attachment 113735
[details]
Patch
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
Created
attachment 113742
[details]
Patch
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
Created
attachment 113803
[details]
Patch
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
Created
attachment 113985
[details]
Patch
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
Created
attachment 114153
[details]
Patch
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
Created
attachment 114170
[details]
Patch
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
Created
attachment 114299
[details]
Patch
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
Created
attachment 114327
[details]
Patch
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
Created
attachment 114371
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug