Bug 68041 – REGRESSION (Safari 5.1-r95043): Incorrect box-shadow offset

• Component: Layout and Rendering
• Status: RESOLVED FIXED

In the URL, the black shadow should be visible along the bottom of the red square, but in r95043 on OS X Lion, it isn’t.

[Radar WebKit Bug Importer]

<rdar://problem/10121212>

[Matthew Delaney]

Created attachment 107465 [details]
Patch

Comment on attachment 107465 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1028
> +    if (!this->isAcceleratedContext()) {

Why the “this->” here?

[Matthew Delaney]

(In reply to comment #3)
> (From update of attachment 107465 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107465&action=review
> 
> > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1028
> > +    if (!this->isAcceleratedContext()) {
> 
> Why the “this->” here?

I realize it's unnecessary. Just thought it reads clearer, as if to explicitly say "I am" - and I recall Simon using that convention too. Not sure if it's against WebKit style rules.

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 107465 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=107465&action=review
> > 
> > > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1028
> > > +    if (!this->isAcceleratedContext()) {
> > 
> > Why the “this->” here?
> 
> I realize it's unnecessary. Just thought it reads clearer, as if to explicitly say "I am" - and I recall Simon using that convention too. Not sure if it's against WebKit style rules.

I’ve only seen this used when there’s a same-named identifier in local scope (e.g. bool foo = this->foo()).

[Matthew Delaney]

Created attachment 107502 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 107502 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1030
> +        // Work around <rdar://problem/5539388> by ensuring that the offsets will get truncated
> +        // to the desired integer.

You may want to add a link to <rdar://problem/10056277> as well.

> LayoutTests/fast/box-shadow/one-pixel-offset-bottom-right.html:1
> +<div style="width: 100px; height: 100px; -webkit-box-shadow: black 1px 1px; background: red;">

Please don't use red in a passing test.

Don't we have existing tests that show this shadow offset problem? I'm not sure we need a new one.

[Matthew Delaney]

(In reply to comment #7)
> (From update of attachment 107502 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107502&action=review
> 
> > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1030
> > +        // Work around <rdar://problem/5539388> by ensuring that the offsets will get truncated
> > +        // to the desired integer.
> 
> You may want to add a link to <rdar://problem/10056277> as well.
Done.

> 
> > LayoutTests/fast/box-shadow/one-pixel-offset-bottom-right.html:1
> > +<div style="width: 100px; height: 100px; -webkit-box-shadow: black 1px 1px; background: red;">
> 
> Please don't use red in a passing test.
> 
> Don't we have existing tests that show this shadow offset problem? I'm not sure we need a new one.

None that I found that hit the correct combination of offsets and div size to trigger the bug while also being outside the default tolerance of the pixel tests. I'm adding in a better test that hits a handful of those cases enough to fail.

[Matthew Delaney]

Created attachment 107514 [details]
Patch

[Matthew Delaney]

Committed r95207: <http://trac.webkit.org/changeset/95207>

[Matthew Delaney]

*** Bug 67052 has been marked as a duplicate of this bug. ***