Bug 114225 - Repaint rect too small on elements with shadows
Summary: Repaint rect too small on elements with shadows
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:
Blocks:
 
Reported: 2013-04-08 17:13 PDT by Simon Fraser (smfr)
Modified: 2013-04-09 13:52 PDT (History)
19 users (show)

See Also:


Attachments
Patch (1.74 MB, patch)
2013-04-08 17:19 PDT, Simon Fraser (smfr)
darin: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion (662.08 KB, application/zip)
2013-04-09 11:02 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2013-04-08 17:13:58 PDT
Repaint rect too small on elements with shadows
Comment 1 Simon Fraser (smfr) 2013-04-08 17:19:59 PDT
Created attachment 196973 [details]
Patch
Comment 2 Simon Fraser (smfr) 2013-04-08 17:20:56 PDT
13129974
Comment 3 Darin Adler 2013-04-08 17:37:08 PDT
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?
Comment 4 Philip Rogers 2013-04-08 17:40:57 PDT
This test result doesn't quite look right: LayoutTests/platform/mac/svg/css/group-with-shadow-expected.png
Comment 5 Simon Fraser (smfr) 2013-04-08 18:07:34 PDT
(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.
Comment 6 Simon Fraser (smfr) 2013-04-08 18:21:19 PDT
(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.
Comment 7 Simon Fraser (smfr) 2013-04-08 18:34:09 PDT
(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.
Comment 8 WebKit Review Bot 2013-04-08 20:26:14 PDT
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.
Comment 9 Build Bot 2013-04-09 11:02:18 PDT
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
Comment 10 Build Bot 2013-04-09 11:02:21 PDT
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
Comment 11 Simon Fraser (smfr) 2013-04-09 13:52:09 PDT
https://trac.webkit.org/r148049