[Webkit-unassigned] [Bug 38851] Large SVG rect with shadow fails to render

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu May 27 01:38:02 PDT 2010


https://bugs.webkit.org/show_bug.cgi?id=38851


Nikolas Zimmermann <zimmermann at kde.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |zimmermann at kde.org




--- Comment #7 from Nikolas Zimmermann <zimmermann at kde.org>  2010-05-27 01:38:02 PST ---
(In reply to comment #6)
> Hello Niko,
> 
> Thanks for speedy review.
> 
> (In reply to comment #5)
> > (From update of attachment 57192 [details] [details])
> > Damn, my comment disappeared, retyping, but not that explicit anymore :( Anyhow, r- because of some nitpicks:
> > 
> > LayoutTests/svg/filters/shadow-on-rect-large.svg:2
> >  +  <rect x="0" y="0" width="2147483647" height="300" fill="green" style="-webkit-svg-shadow:5px 5px 5px grey;"/>
> > Please add a comment here, stating that it passes if it does not crash, and maybe add the bug numbe.r
> 
> I sort of knew when I uploaded the patch that that was lacking :) Notice there is no crash though, just nothing is rendered! Do you think with that knowledge the test can be reduced further, with no or a smaller pixel result (maybe 1x1)?

Oh sorry, of course it doesn't crash, misread :-) I'm not sure whether the test can be further reduced, I think it's okay this way - the final size is 800x600 anyways, and when the bug would appear again in future, we'd easily spot because of large differences in the pixel tests.


> 
> > The code change itself is fine, though it could be optimized. How about just using:
> > repaintRect.move(shadowLeft, shadowRight);
> > repaintRect.setSize(repaintRect.size() + FloatSize(shadowRight - shadowLeft, shadowBottom - shadowTop));
> > 
> > That would save any intermediate int/float value, and is easier to read IMHO.
> > What do you think?
> 
> I have to see when I get home. I am not sure you know, but when doing bug fixes is it ok/desired to optimize the code in the change? I guess it does make sense when the change is in the same code section, just wondering whether anything solid is written about that.
It's definately okay - the underlying issue is that we're actually using intermediate values, where not necessary. You could of course split up in two patches, the first switching int to float and the next to optimize the code, but I think it's pointless if it just affects a few lines of code.

Cheers,
Niko

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.



More information about the webkit-unassigned mailing list