Bug 126541 – Margin tiles are not repainted when background color changes

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Beth Dakin]

If that tile cache has been given a margin and the page background color changes dynamically, we fail to repaint the edge tiles that are revealed with rubber-banding.

<rdar://problem/15578131>

[Beth Dakin]

Created attachment 220458 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 220458 [details]
Patch

I don't think it's right that GraphicsLayerCA is polluted with knowledge of TiledBacking margins, and I think this happens to fix repaints of hugeRects, but I don't think there's any guarantee that background color changes are always done through whole-layer repaints.

[Simon Fraser (smfr)]

I think we need a better idea of what paints into margin tiles before we fix this, and that will require teaching more of rendering about margin tile painting.

[Beth Dakin]

Created attachment 220592 [details]
Patch

Here's a new patch that takes an approach that Simon and I discussed today.

[WebKit Commit Bot]

Attachment 220592 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/platform/graphics/GraphicsLayer.h', u'Source/WebCore/platform/graphics/blackberry/GraphicsLayerBlackBerry.cpp', u'Source/WebCore/platform/graphics/blackberry/GraphicsLayerBlackBerry.h', u'Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp', u'Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h', u'Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp', u'Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.h', u'Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp', u'Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.h', u'Source/WebCore/rendering/RenderLayerBacking.cpp', u'Source/WebCore/rendering/RenderView.cpp', u'Source/WebCore/rendering/RenderView.h', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/platform/graphics/blackberry/GraphicsLayerBlackBerry.h:93:  The parameter name "shouldClip" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.h:99:  The parameter name "shouldClip" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.h:46:  The parameter name "shouldClip" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/rendering/RenderLayerBacking.cpp:2093:  Missing space after ,  [whitespace/comma] [3]
ERROR: Source/WebCore/rendering/RenderView.cpp:577:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h:109:  The parameter name "shouldClip" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/graphics/GraphicsLayer.h:359:  The parameter name "shouldClip" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 7 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Beth Dakin]

Created attachment 220593 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 220593 [details]
Patch

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

> Source/WebCore/rendering/RenderView.h:357
> +    bool m_currentlyRepaintingRootContents;

I'm not really keep on storing this state here, since it's so transient. Can't we just pass it through the repaint functions?

[Beth Dakin]

(In reply to comment #7)
> (From update of attachment 220593 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=220593&action=review
> 
> > Source/WebCore/rendering/RenderView.h:357
> > +    bool m_currentlyRepaintingRootContents;
> 
> I'm not really keep on storing this state here, since it's so transient. Can't we just pass it through the repaint functions?

Sure.

[Beth Dakin]

Created attachment 220649 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 220649 [details]
Patch

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

> Source/WebCore/rendering/RenderLayerBacking.cpp:2097
> +            FloatRect rectToRepaint = FloatRect(FloatPoint(0, 0), m_graphicsLayer->size());
> +            TiledBacking* tiledBacking = this->tiledBacking();
> +            rectToRepaint.move(-tiledBacking->leftMarginWidth(), -tiledBacking->topMarginHeight());
> +            rectToRepaint.expand(tiledBacking->leftMarginWidth() + tiledBacking->rightMarginWidth(),
> +                tiledBacking->topMarginHeight() + tiledBacking->bottomMarginHeight());

I think it would be better to call a function on TiledBacking that inflates the rect. That way you could eliminate this whole tiledBackingHasMargin() condition.

[Beth Dakin]

Created attachment 220681 [details]
Patch

[Tim Horton]

Comment on attachment 220681 [details]
Patch

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

> Source/WebCore/ChangeLog:46
> +        with a  big enough rect and a ShouldClipToLayer value of DontClipToLayer.

extra space between 'a' and 'big'.

shouldn't it be "DoNotClipToLayer"?

> Source/WebCore/platform/graphics/GraphicsLayer.h:353
> +        DontClipToLayer,

yeah. DoNot.

[Beth Dakin]

(In reply to comment #12)
> (From update of attachment 220681 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=220681&action=review
> 
> > Source/WebCore/ChangeLog:46
> > +        with a  big enough rect and a ShouldClipToLayer value of DontClipToLayer.
> 
> extra space between 'a' and 'big'.
> 

Will fix!

> shouldn't it be "DoNotClipToLayer"?
> 
> > Source/WebCore/platform/graphics/GraphicsLayer.h:353
> > +        DontClipToLayer,
> 
> yeah. DoNot.

Will fix!

[Beth Dakin]

Created attachment 220683 [details]
Patch

New new patch that addresses Tim's two small comments.

[Simon Fraser (smfr)]

Comment on attachment 220683 [details]
Patch

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

> Source/WebCore/platform/graphics/ca/mac/TileController.mm:1085
> +void TileController::inflateRectForMargin(FloatRect& rect) const
> +{
> +    rect.move(-leftMarginWidth(), -topMarginHeight());
> +    rect.expand(leftMarginWidth() + rightMarginWidth(), topMarginHeight() + bottomMarginHeight());
> +}

Rather than add margins to some arbitrary rect, maybe just have it return its size with margins, since the caller is already just passing in the same rect as the TileController size.

> Source/WebCore/rendering/RenderLayerBacking.cpp:2090
> +        FloatRect rectToRepaint = FloatRect(FloatPoint(0, 0), m_graphicsLayer->size());
> +        tiledBacking()->inflateRectForMargin(rectToRepaint);

Null-check here.

[Build Bot]

Comment on attachment 220683 [details]
Patch

Attachment 220683 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5629190430785536

New failing tests:
compositing/backface-visibility/backface-visibility-hierarchical-transform.html
http/tests/canvas/philip/tests/security.pattern.canvas.strokeStyle.html
compositing/composited-negative-zindex-child.html
animations/change-transform-style-during-animation.html
compositing/backing/no-backing-for-clip-overhang.html
compositing/backing/no-backing-for-clip-overlap.html
accessibility/transformed-bounds.html
compositing/columns/ancestor-clipped-in-paginated.html
compositing/bounds-in-flipped-writing-mode.html
compositing/clip-change.html
compositing/background-color/background-color-text-change.html
compositing/animation/state-at-end-event-transform-layer.html
http/tests/canvas/philip/tests/security.drawImage.image.html
animations/animation-direction-reverse-hardware.html
compositing/backface-visibility/backface-visibility-3d.html
accessibility/media-element.html
http/tests/canvas/philip/tests/security.drawImage.canvas.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.5.html
canvas/philip/tests/2d.clearRect.basic.html
http/tests/canvas/philip/tests/security.pattern.canvas.fillStyle.html
compositing/backface-visibility/backface-visibility-webgl.html
compositing/background-color/background-color-text-clip.html
compositing/backface-visibility/backface-visibility-image.html
compositing/background-color/background-color-padding-change.html
animations/animation-direction-reverse-fill-mode-hardware.html
accessibility/noscript-ignored.html
canvas/philip/tests/2d.clearRect+fillRect.basic.html
compositing/color-matching/pdf-image-match.html
compositing/clip-child-by-non-stacking-ancestor.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.html

[Build Bot]

Created attachment 220747 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Beth Dakin]

(In reply to comment #15)
> (From update of attachment 220683 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=220683&action=review
> 
> > Source/WebCore/platform/graphics/ca/mac/TileController.mm:1085
> > +void TileController::inflateRectForMargin(FloatRect& rect) const
> > +{
> > +    rect.move(-leftMarginWidth(), -topMarginHeight());
> > +    rect.expand(leftMarginWidth() + rightMarginWidth(), topMarginHeight() + bottomMarginHeight());
> > +}
> 
> Rather than add margins to some arbitrary rect, maybe just have it return its size with margins, since the caller is already just passing in the same rect as the TileController size.
> 

Sure, that makes sense.

> > Source/WebCore/rendering/RenderLayerBacking.cpp:2090
> > +        FloatRect rectToRepaint = FloatRect(FloatPoint(0, 0), m_graphicsLayer->size());
> > +        tiledBacking()->inflateRectForMargin(rectToRepaint);
> 
> Null-check here.

Thanks for catching!

[Beth Dakin]

http://trac.webkit.org/changeset/161570