[webkit-reviews] review denied: [Bug 42387] SVG - stroke-width:0 bug with stroke other than "none" : [Attachment 62465] stroke-width:0 fix

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Fady Samuel <fsamuel at chromium.org>'s
request for review:
Bug 42387: SVG - stroke-width:0 bug with stroke other than "none"
https://bugs.webkit.org/show_bug.cgi?id=42387

Attachment 62465: stroke-width:0 fix
https://bugs.webkit.org/attachment.cgi?id=62465&action=review

------- Additional Comments from Dirk Schulze <krit at webkit.org>
(In reply to comment #22)
> Created an attachment (id=62465) [details]
> stroke-width:0 fix
> 
> So the initial patch broke some already flaky layout tests. It turns out
fixing this bug revealed a couple of others. First, the video controls in the
video element were not rendered correctly as the stroke-width was not
specified. Secondly, stroke-width defaults to 0, this patch defaults it to 1
now as the spec describes. 
> 
> This bug corresponds to Issue 15461 on chromium.org:
> 
> http://code.google.com/p/chromium/issues/detail?id=15461
> 
> I may be missing some more layout tests. There may need to be more
rebaselining in the gtk platform and in the windows platforms. My
chromium-mac/mac platforms were baselined on Snow Leopard, this may or may not
be what is expected.

Great patch! Just some notes. IIRC the default for stroke-width is not 1px but
1. So no need for SVG_RS_DEFINE_ATTRIBUTE_DATAREF_WITH_INITIAL_REFCOUNTED2.

Also, this patch fixes more than one issue. The SVG fix and the Skia changes
should be seperated of each other.
Is it possible to check for !stroke-width in RenderPath's fillAndStrokePath?
This could avoid building a gradient or pattern if we don't draw the stroke
anyway.