Bug 156860 – Subpixel rendering: Cleanup RenderLayerBacking::updateGeometry.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[alan]

nested/clipped/containment/masks etc.

[alan]

Created attachment 276946 [details]
WIP patch

[alan]

Created attachment 279987 [details]
WIP

rebaselined

[Build Bot]

Comment on attachment 279987 [details]
WIP

Attachment 279987 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1393148

New failing tests:
fast/scrolling/ios/overflow-scrolling-ancestor-clip.html
fast/scrolling/ios/overflow-scrolling-ancestor-clip-size.html

[Build Bot]

Created attachment 279993 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

[alan]

Created attachment 285395 [details]
Patch

[Build Bot]

Comment on attachment 285395 [details]
Patch

Attachment 285395 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1814735

New failing tests:
fast/scrolling/ios/overflow-scrolling-ancestor-clip.html
fast/scrolling/ios/overflow-scrolling-ancestor-clip-size.html

[Build Bot]

Created attachment 285398 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.5

[alan]

Created attachment 285478 [details]
UpdateGeometry layers

[alan]

Created attachment 285553 [details]
Patch

[Build Bot]

Comment on attachment 285553 [details]
Patch

Attachment 285553 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1832013

New failing tests:
fast/scrolling/ios/overflow-scrolling-ancestor-clip.html
fast/scrolling/ios/overflow-scrolling-ancestor-clip-size.html

[Build Bot]

Created attachment 285555 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.5

[alan]

Created attachment 285629 [details]
Patch

[alan]

RenderLayerBacking::computeParentGraphicsLayerRect() should drop ancestorClippingLayerOffset somehow.

[alan]

Created attachment 286186 [details]
Patch

[Build Bot]

Comment on attachment 286186 [details]
Patch

Attachment 286186 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1882656

New failing tests:
fast/scrolling/ios/subpixel-overflow-scrolling-with-ancestor.html

[Build Bot]

Created attachment 286197 [details]
Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.5

[alan]

Created attachment 286212 [details]
Patch

[alan]

rdar://problem/25432352

[alan]

Created attachment 286216 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 286216 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=286216&action=review

> Source/WebCore/ChangeLog:11
> +        It also subpixel jiggling with clipping layers (both ancestor and child containment layers). 

it also fixes?

> Source/WebCore/rendering/RenderLayerBacking.cpp:719
> +    LayoutSize m_subpixelOffset;
> +    LayoutSize m_devicePixelOffset;

I think logically I would put m_devicePixelOffset first, so the ordering matches SnappedRectInfo

> Source/WebCore/rendering/RenderLayerBacking.cpp:750
> +    LayoutSize ancestorRenderLayerOffsetFromAncestorGraphicsLayer = -(LayoutSize(compositedAncestor->backing()->graphicsLayer()->offsetFromRenderer())
> +        + compositedAncestor->backing()->subpixelOffsetFromRenderer());

Maybe RLB should expose a function that returns offsetFromRenderer() + subpixelOffsetFromRenderer()

> Source/WebCore/rendering/RenderLayerBacking.cpp:768
> +        if (!m_fromParentGraphicsLayer)
> +            m_fromParentGraphicsLayer = fromAncestorGraphicsLayer() - m_parentGraphicsLayerOffset;
> +        return m_fromParentGraphicsLayer.value();

Isn't there some Optional magic that allows you to do this on one line?

> Source/WebCore/rendering/RenderLayerBacking.cpp:782
> +            RenderLayer* compositedAncestor = m_renderLayer.ancestorCompositingLayer();

ancestorCompositingLayer() is an ancestor layer walk so I'm surprised we call it so much.

> Source/WebCore/rendering/RenderLayerBacking.cpp:809
> +    RenderLayer* compositedAncestor = m_owningLayer.ancestorCompositingLayer();

like here

> Source/WebCore/rendering/RenderLayerBacking.cpp:826
> +        LayoutSize clippingBoxOffset = computeOffsetFromAncestorGraphicsLayer(m_owningLayer.ancestorCompositingLayer(), clippingBox.location());

and here

> Source/WebCore/rendering/RenderLayerBacking.cpp:835
> +        LayoutRect paddingBox(renderBox.borderLeft(), renderBox.borderTop(),
> +            renderBox.width() - renderBox.borderLeft() - renderBox.borderRight(),
> +            renderBox.height() - renderBox.borderTop() - renderBox.borderBottom());

Seems like this should be a function on RenderBox

> Source/WebCore/rendering/RenderLayerBacking.cpp:919
> +    // Compute renderer offset from graphics layer. Note that primaryGraphicsLayerRect is in parentGraphicsLayer coordidate system which is not necessarily

which GraphicsLayer?
parentGraphicsLayer's coordinate system

> Source/WebCore/rendering/RenderLayerBacking.cpp:1108
> +    RenderLayer* compositedAncestor = m_owningLayer.ancestorCompositingLayer();

and here

[alan]

Created attachment 286229 [details]
Patch

[alan]

(In reply to comment #20)
> Comment on attachment 286216 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=286216&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        It also subpixel jiggling with clipping layers (both ancestor and child containment layers). 
> 
> it also fixes?
Fixed.

> 
> > Source/WebCore/rendering/RenderLayerBacking.cpp:719
> > +    LayoutSize m_subpixelOffset;
> > +    LayoutSize m_devicePixelOffset;
> 
> I think logically I would put m_devicePixelOffset first, so the ordering
> matches SnappedRectInfo
Fixed.

> 
> > Source/WebCore/rendering/RenderLayerBacking.cpp:750
> > +    LayoutSize ancestorRenderLayerOffsetFromAncestorGraphicsLayer = -(LayoutSize(compositedAncestor->backing()->graphicsLayer()->offsetFromRenderer())
> > +        + compositedAncestor->backing()->subpixelOffsetFromRenderer());
> 
> Maybe RLB should expose a function that returns offsetFromRenderer() +
> subpixelOffsetFromRenderer()
Will do in a separate patch.

> 
> > Source/WebCore/rendering/RenderLayerBacking.cpp:768
> > +        if (!m_fromParentGraphicsLayer)
> > +            m_fromParentGraphicsLayer = fromAncestorGraphicsLayer() - m_parentGraphicsLayerOffset;
> > +        return m_fromParentGraphicsLayer.value();
> 
> Isn't there some Optional magic that allows you to do this on one line?
valueOrCompute() is the closest match but it does not engage the return value.
> 
> > Source/WebCore/rendering/RenderLayerBacking.cpp:782
> > +            RenderLayer* compositedAncestor = m_renderLayer.ancestorCompositingLayer();
> 
> ancestorCompositingLayer() is an ancestor layer walk so I'm surprised we
> call it so much.
> 
> > Source/WebCore/rendering/RenderLayerBacking.cpp:809
> > +    RenderLayer* compositedAncestor = m_owningLayer.ancestorCompositingLayer();
> 
> like here
Fixed.

> 
> > Source/WebCore/rendering/RenderLayerBacking.cpp:826
> > +        LayoutSize clippingBoxOffset = computeOffsetFromAncestorGraphicsLayer(m_owningLayer.ancestorCompositingLayer(), clippingBox.location());
> 
> and here
> 
> > Source/WebCore/rendering/RenderLayerBacking.cpp:835
> > +        LayoutRect paddingBox(renderBox.borderLeft(), renderBox.borderTop(),
> > +            renderBox.width() - renderBox.borderLeft() - renderBox.borderRight(),
> > +            renderBox.height() - renderBox.borderTop() - renderBox.borderBottom());
> 
> Seems like this should be a function on RenderBox
RenderBox already has a paddingBox() function but it computes the paddings in a slightly different way. Need to figure out if it is intentional.

> 
> > Source/WebCore/rendering/RenderLayerBacking.cpp:919
> > +    // Compute renderer offset from graphics layer. Note that primaryGraphicsLayerRect is in parentGraphicsLayer coordidate system which is not necessarily
> 
> which GraphicsLayer?
> parentGraphicsLayer's coordinate system
Fixed.

[Build Bot]

Comment on attachment 286229 [details]
Patch

Attachment 286229 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1884124

New failing tests:
fast/scrolling/ios/subpixel-overflow-scrolling-with-ancestor.html

[Build Bot]

Created attachment 286248 [details]
Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.5

[alan]

Created attachment 286249 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 286249 [details]
Patch

Clearing flags on attachment: 286249

Committed r204552: <http://trac.webkit.org/changeset/204552>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.