Bug 101874

Summary: Eliminate ancestor tree walk computing outlineBoundsForRepaint() when updating layer positions
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: New BugsAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, dglazkov, d-r, eric, fmalita, leviw, mitz, pdr, peter+ews, schenney, simon.fraser, webkit-bug-importer, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 101779, 102675    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch hyatt: review+, buildbot: commit-queue-

Simon Fraser (smfr)
Reported 2012-11-11 17:50:33 PST
Eliminate ancestor tree walk computing outlineBoundsForRepaint() when updating layer positions
Attachments
Patch (36.66 KB, patch)
2012-11-11 18:29 PST, Simon Fraser (smfr) 		no flags
DetailsFormatted DiffDiff
Patch (38.19 KB, patch)
2012-11-11 21:26 PST, Simon Fraser (smfr) 		no flags
DetailsFormatted DiffDiff
Patch (38.30 KB, patch)
2012-11-12 14:23 PST, Simon Fraser (smfr) 		hyatt: review+
buildbot: commit-queue-
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Simon Fraser (smfr)
Comment 1 2012-11-11 18:29:19 PST
Created attachment 173532 [details] Patch
mitz
Comment 2 2012-11-11 18:56:49 PST
Comment on attachment 173532 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173532&action=review > Source/WebCore/ChangeLog:34 > + parameter with an optional RenderGeometryMap, and it use to map the compute rect to Typo: “it use to map the compute” > Source/WebCore/page/FrameView.cpp:1213 > + RenderGeometryMap geometryMap(UseTransforms); Why does this need to be scoped? > Source/WebCore/rendering/RenderGeometryMap.cpp:58 > FloatPoint RenderGeometryMap::absolutePoint(const FloatPoint& p) const > { > - FloatPoint result; > - > - if (!hasFixedPositionStep() && !hasTransformStep() && !hasNonUniformStep()) > - result = p + m_accumulatedOffset; > - else { > - TransformState transformState(TransformState::ApplyTransformDirection, p); > - mapToAbsolute(transformState); > - result = transformState.lastPlanarPoint(); > - } > - > -#if !ASSERT_DISABLED > - FloatPoint rendererMappedResult = m_mapping.last().m_renderer->localToAbsolute(p, UseTransforms | SnapOffsetForTransforms); > - ASSERT(rendererMappedResult == result); > -#endif > - > - return result; > + return mapToContainer(p, 0); > } > > FloatRect RenderGeometryMap::absoluteRect(const FloatRect& rect) const > { > - FloatRect result; > - > - if (!hasFixedPositionStep() && !hasTransformStep() && !hasNonUniformStep()) { > - result = rect; > - result.move(m_accumulatedOffset); > - } else { > - TransformState transformState(TransformState::ApplyTransformDirection, rect.center(), rect); > - mapToAbsolute(transformState); > - result = transformState.lastPlanarQuad().boundingBox(); > - } > - > -#if !ASSERT_DISABLED > - FloatRect rendererMappedResult = m_mapping.last().m_renderer->localToAbsoluteQuad(rect).boundingBox(); > - // Inspector creates renderers with negative width <https://bugs.webkit.org/show_bug.cgi?id=87194>. > - // Taking FloatQuad bounds avoids spurious assertions because of that. > - ASSERT(enclosingIntRect(rendererMappedResult) == enclosingIntRect(FloatQuad(result).boundingBox())); > -#endif > - > - return result; > + return mapToContainer(rect, 0).boundingBox(); > } > Should these functions be moved into the header now so that they can be inlined?
Simon Fraser (smfr)
Comment 3 2012-11-11 19:04:22 PST
(In reply to comment #2) > (From update of attachment 173532 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=173532&action=review > > > Source/WebCore/ChangeLog:34 > > + parameter with an optional RenderGeometryMap, and it use to map the compute rect to > > Typo: “it use to map the compute” Thanks > > > Source/WebCore/page/FrameView.cpp:1213 > > + RenderGeometryMap geometryMap(UseTransforms); > > Why does this need to be scoped? It doesn't. It just seemed neater, since it's only used for this one call. Another way to do this would be to make a wrapper in RenderLayer for those few lines. > > Source/WebCore/rendering/RenderGeometryMap.cpp:58 > > FloatPoint RenderGeometryMap::absolutePoint(const FloatPoint& p) const ... > > FloatRect RenderGeometryMap::absoluteRect(const FloatRect& rect) const ... > Should these functions be moved into the header now so that they can be inlined? Sure!
Build Bot
Comment 4 2012-11-11 19:25:30 PST
Comment on attachment 173532 [details] Patch Attachment 173532 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14794781 New failing tests: fast/spatial-navigation/snav-imagemap-overlapped-areas.html
Peter Beverloo (cr-android ews)
Comment 5 2012-11-11 20:00:38 PST
Comment on attachment 173532 [details] Patch Attachment 173532 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/14785966
EFL EWS Bot
Comment 6 2012-11-11 20:29:25 PST
Comment on attachment 173532 [details] Patch Attachment 173532 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14803513
Simon Fraser (smfr)
Comment 7 2012-11-11 21:26:51 PST
Created attachment 173549 [details] Patch
Simon Fraser (smfr)
Comment 8 2012-11-12 14:23:25 PST
Created attachment 173726 [details] Patch
Build Bot
Comment 9 2012-11-12 20:38:19 PST
Comment on attachment 173726 [details] Patch Attachment 173726 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14818408 New failing tests: http/tests/inspector/stacktraces/csp-inline-warning-contains-stacktrace.html
Dave Hyatt
Comment 10 2012-11-13 14:01:14 PST
Comment on attachment 173726 [details] Patch r=me
Simon Fraser (smfr)
Comment 11 2012-11-13 15:55:48 PST
I can't land this because it's causing some assertions in RenderGeometryMap because of pixel snapping differences.
Radar WebKit Bug Importer
Comment 12 2012-11-14 17:14:35 PST
<rdar://problem/12705362>
Simon Fraser (smfr)
Comment 13 2012-11-16 18:05:18 PST
http://trac.webkit.org/changeset/135025
Note You need to log in before you can comment on or make changes to this bug.