Bug 101874 - Eliminate ancestor tree walk computing outlineBoundsForRepaint() when updating layer positions
Summary: Eliminate ancestor tree walk computing outlineBoundsForRepaint() when updatin...
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: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on: 101779 102675
Blocks:
  Show dependency treegraph
 
Reported: 2012-11-11 17:50 PST by Simon Fraser (smfr)
Modified: 2012-11-19 04:40 PST (History)
13 users (show)

See Also:


Attachments
Patch (36.66 KB, patch)
2012-11-11 18:29 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (38.19 KB, patch)
2012-11-11 21:26 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (38.30 KB, patch)
2012-11-12 14:23 PST, Simon Fraser (smfr)
hyatt: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2012-11-11 17:50:33 PST
Eliminate ancestor tree walk computing outlineBoundsForRepaint() when updating layer positions
Comment 1 Simon Fraser (smfr) 2012-11-11 18:29:19 PST
Created attachment 173532 [details]
Patch
Comment 2 mitz 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?
Comment 3 Simon Fraser (smfr) 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!
Comment 4 Build Bot 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
Comment 5 Peter Beverloo (cr-android ews) 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
Comment 6 EFL EWS Bot 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
Comment 7 Simon Fraser (smfr) 2012-11-11 21:26:51 PST
Created attachment 173549 [details]
Patch
Comment 8 Simon Fraser (smfr) 2012-11-12 14:23:25 PST
Created attachment 173726 [details]
Patch
Comment 9 Build Bot 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
Comment 10 Dave Hyatt 2012-11-13 14:01:14 PST
Comment on attachment 173726 [details]
Patch

r=me
Comment 11 Simon Fraser (smfr) 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.
Comment 12 Radar WebKit Bug Importer 2012-11-14 17:14:35 PST
<rdar://problem/12705362>
Comment 13 Simon Fraser (smfr) 2012-11-16 18:05:18 PST
http://trac.webkit.org/changeset/135025