Bug 114225 – Repaint rect too small on elements with shadows

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Simon Fraser (smfr)]

Repaint rect too small on elements with shadows

[Simon Fraser (smfr)]

Created attachment 196973 [details]
Patch

[Simon Fraser (smfr)]

13129974

[Darin Adler]

Comment on attachment 196973 [details]
Patch

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

Would have been nice to have the rename of blur to radius in a first, separate patch.

> Source/WebCore/rendering/style/ShadowData.h:77
> +    int paintingExtent() const
> +    {
> +        const float radiusExtentMultplier = 1.3;
> +        return ceilf(m_radius * radiusExtentMultplier);
> +    };

Needs a comment to explain why 1.3 is a correct, helpful value.

Typo: Multplier.

Extraneous semicolon here.

The name “extent” is a strange word for a scalar that represents the amount to inflate a dirty rect with. It’s related to the extent of the shadow, but the number itself does not seem to be an extent. Is there some other word that would be more accurate for this concept?

[Philip Rogers]

This test result doesn't quite look right: LayoutTests/platform/mac/svg/css/group-with-shadow-expected.png

[Simon Fraser (smfr)]

(In reply to comment #4)
> This test result doesn't quite look right: LayoutTests/platform/mac/svg/css/group-with-shadow-expected.png

Agreed. Investigating.

[Simon Fraser (smfr)]

(In reply to comment #5)
> (In reply to comment #4)
> > This test result doesn't quite look right: LayoutTests/platform/mac/svg/css/group-with-shadow-expected.png
> 
> Agreed. Investigating.

Not a regression from this patch. I filed bug 114229.

[Simon Fraser (smfr)]

(In reply to comment #3)
> (From update of attachment 196973 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=196973&action=review
> 
> Would have been nice to have the rename of blur to radius in a first, separate patch.
> 
> > Source/WebCore/rendering/style/ShadowData.h:77
> > +    int paintingExtent() const
> > +    {
> > +        const float radiusExtentMultplier = 1.3;
> > +        return ceilf(m_radius * radiusExtentMultplier);
> > +    };
> 
> Needs a comment to explain why 1.3 is a correct, helpful value.
> 
> Typo: Multplier.
> 
> Extraneous semicolon here.

Will fix.

> The name “extent” is a strange word for a scalar that represents the amount to inflate a dirty rect with. It’s related to the extent of the shadow, but the number itself does not seem to be an extent. Is there some other word that would be more accurate for this concept?

I could not think of one.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/box-shadow/shadow-repaint-expected.txt', u'LayoutTests/fast/box-shadow/shadow-repaint.html', u'LayoutTests/platform/mac/fast/multicol/shadow-breaking-expected.png', u'LayoutTests/platform/mac/fast/multicol/shadow-breaking-expected.txt', u'LayoutTests/platform/mac/fast/repaint/moving-shadow-on-container-expected.txt', u'LayoutTests/platform/mac/fast/repaint/moving-shadow-on-path-expected.png', u'LayoutTests/platform/mac/fast/repaint/moving-shadow-on-path-expected.txt', u'LayoutTests/platform/mac/svg/css/arrow-with-shadow-expected.png', u'LayoutTests/platform/mac/svg/css/circle-in-mask-with-shadow-expected.png', u'LayoutTests/platform/mac/svg/css/clippath-with-shadow-expected.png', u'LayoutTests/platform/mac/svg/css/composite-shadow-example-expected.png', u'LayoutTests/platform/mac/svg/css/composite-shadow-example-expected.txt', u'LayoutTests/platform/mac/svg/css/composite-shadow-text-expected.txt', u'LayoutTests/platform/mac/svg/css/composite-shadow-with-opacity-expected.png', u'LayoutTests/platform/mac/svg/css/composite-shadow-with-opacity-expected.txt', u'LayoutTests/platform/mac/svg/css/group-with-shadow-expected.png', u'LayoutTests/platform/mac/svg/css/group-with-shadow-expected.txt', u'LayoutTests/platform/mac/svg/css/mask-with-shadow-expected.png', u'LayoutTests/platform/mac/svg/css/path-with-shadow-expected.png', u'LayoutTests/platform/mac/svg/css/path-with-shadow-expected.txt', u'LayoutTests/platform/mac/svg/css/shadow-and-opacity-expected.png', u'LayoutTests/platform/mac/svg/css/shadow-and-opacity-expected.txt', u'LayoutTests/platform/mac/svg/css/shadow-changes-expected.png', u'LayoutTests/platform/mac/svg/css/shadow-changes-expected.txt', u'LayoutTests/platform/mac/svg/css/shadow-with-large-radius-expected.png', u'LayoutTests/platform/mac/svg/css/shadow-with-negative-offset-expected.png', u'LayoutTests/platform/mac/svg/css/stars-with-shadow-expected.png', u'LayoutTests/platform/mac/svg/css/stars-with-shadow-expected.txt', u'LayoutTests/platform/mac/svg/css/text-gradient-shadow-expected.png', u'LayoutTests/platform/mac/svg/css/text-gradient-shadow-expected.txt', u'LayoutTests/platform/mac/svg/css/text-shadow-multiple-expected.png', u'LayoutTests/platform/mac/svg/custom/repaint-shadow-expected.png', u'LayoutTests/platform/mac/svg/custom/transform-with-shadow-and-gradient-expected.txt', u'LayoutTests/svg/css/arrow-with-shadow-expected.txt', u'LayoutTests/svg/css/circle-in-mask-with-shadow-expected.txt', u'LayoutTests/svg/css/clippath-with-shadow-expected.txt', u'LayoutTests/svg/css/mask-with-shadow-expected.txt', u'LayoutTests/svg/css/shadow-with-large-radius-expected.txt', u'LayoutTests/svg/css/shadow-with-negative-offset-expected.txt', u'LayoutTests/svg/css/text-shadow-multiple-expected.txt', u'LayoutTests/svg/custom/repaint-shadow-expected.txt', u'LayoutTests/svg/filters/shadow-on-rect-with-filter-expected.txt', u'LayoutTests/svg/repaint/repaint-webkit-svg-shadow-container-expected.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/editing/mac/EditorMac.mm', u'Source/WebCore/page/animation/CSSPropertyAnimation.cpp', u'Source/WebCore/rendering/EllipsisBox.cpp', u'Source/WebCore/rendering/InlineTextBox.cpp', u'Source/WebCore/rendering/RenderBoxModelObject.cpp', u'Source/WebCore/rendering/style/RenderStyle.cpp', u'Source/WebCore/rendering/style/ShadowData.cpp', u'Source/WebCore/rendering/style/ShadowData.h', u'Source/WebCore/rendering/svg/SVGRenderingContext.cpp']" exit_code: 1
Source/WebCore/rendering/EllipsisBox.cpp:45:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/page/animation/CSSPropertyAnimation.cpp:117:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 38 files


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

[Build Bot]

Comment on attachment 196973 [details]
Patch

Attachment 196973 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17717015

New failing tests:
fast/box-shadow/shadow-repaint.html
compositing/iframes/scrolling-iframe.html
compositing/layer-creation/rotate3d-overlap.html
compositing/iframes/connect-compositing-iframe.html
compositing/iframes/overlapped-iframe.html
compositing/iframes/enter-compositing-iframe.html
compositing/geometry/foreground-layer.html
compositing/iframes/iframe-resize.html
compositing/visible-rect/iframe-and-layers.html
compositing/iframes/composited-parent-iframe.html
compositing/iframes/invisible-nested-iframe-show.html
compositing/iframes/connect-compositing-iframe-delayed.html
compositing/iframes/page-cache-layer-tree.html
compositing/iframes/connect-compositing-iframe2.html
compositing/iframes/connect-compositing-iframe3.html

[Build Bot]

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

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

[Simon Fraser (smfr)]

https://trac.webkit.org/r148049