Bug 61600

Created attachment 95104 [details] Patch 1 Patch to fix the above. As this is my first WebKit patch, please let me know if I'm incorrect in adding r? and cq?

Attachment 95104 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/svg/SVGRadial..." exit_code: 1 ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 95105 [details] Patch 2

Attachment 95105 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/svg/SVGRadial..." exit_code: 1 ChangeLog:6: Line contains tab character. [whitespace/tab] [5] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 95106 [details] Attempt to fix style errors

Comment on attachment 95106 [details] Attempt to fix style errors How do we test this?

(In reply to comment #6) > (From update of attachment 95106 [details]) > How do we test this? Unfortunately, I'm not sure. I'm not familiar with the WebKit code to know what may cause this path to be hit, just that it was "clearly" wrong. Either one of the calls to cy.isRelative() should be removed (since it is redundant) or the more likely answer, that because isRelative() has no side-effects/is const, the likely intent was for cx() and cy() to be checked, just as fx() and fy() are checked. For what it's worth, the downstream report is at http://code.google.com/p/chromium/issues/detail?id=84141 (originally http://code.google.com/p/chromium/issues/detail?id=83873#c2 )

(In reply to comment #7) > (In reply to comment #6) > > (From update of attachment 95106 [details] [details]) > > How do we test this? > > Unfortunately, I'm not sure. I'm not familiar with the WebKit code to know what may cause this path to be hit, just that it was "clearly" wrong. Either one of the calls to cy.isRelative() should be removed (since it is redundant) or the more likely answer, that because isRelative() has no side-effects/is const, the likely intent was for cx() and cy() to be checked, just as fx() and fy() are checked. > > For what it's worth, the downstream report is at http://code.google.com/p/chromium/issues/detail?id=84141 (originally http://code.google.com/p/chromium/issues/detail?id=83873#c2 ) I have a suggestion for a test: Use LayoutTests/svg/custom/relative-sized-content.xhtml as template. The hasRelativeLength() stuff is relevant, when you eg. apply your radialGradient with cx/cy in percentages to a target rectangle, which itself is relatively postionioned. Currently when resizing the window you won't see any changes of 'cx' (if it is relative, and 'cy' is absolute). You can "mimic" window size changes, by embedding your <svg> in an outer <div> and resize that, this is what the test example I cited above does. To summarize: You'll need a svg with width/height 100%, a rect with w/h="100%" a radGrad with cy="50" (sth. absolute) and a percentage based cx. That should expose the bug.... Good catch, btw...

@zimmermann: Do you think you can write a test for this? This bug was caught by a static code analysis tool and Ryan was just trying to be helpful by writing the patch to fix the bug. I think this is his first webkit patch so it might be quicker if you can write the layout test.

CC'ing Leo/Dirk/Rob - does any of you have some time for this? I'm still blocked by the CSS & SVG Font patches.

Hi Niko, (In reply to comment #10) > CC'ing Leo/Dirk/Rob - does any of you have some time for this? I'm still blocked by the CSS & SVG Font patches. Yes I'll have a look. Cheers, Rob.

I looked into this but I am not sure whether these relative values are used at all, or at least when having the viewport width changed. Basically once the viewport changes the client for the gradient resource is removed from dictionary of clients, then in applyResource it is newly calculated and the percentage calculation will be fine. Let me know if I am missing anything... Cheers, Rob.

(In reply to comment #12) > I looked into this but I am not sure whether these relative values are used at all, or at least when having the viewport width changed. Basically once the viewport changes the client for the gradient resource is removed from dictionary of clients, then in applyResource it is newly calculated and the percentage calculation will be fine. Let me know if I am missing anything... > Cheers, > > Rob. Rob is right, we discussed this on IRC. When assigning absolute values to the <rect> that uses the gradient, it still works. This is the reason: Breakpoint 8, WebCore::RenderSVGResourceGradient::removeClientFromCache (this=0x11b48cc98, client=0x11b43cc08, markForInvalidation=false) at RenderSVGResourceGradient.cpp:68 68 ASSERT(client); (gdb) bt #0 WebCore::RenderSVGResourceGradient::removeClientFromCache (this=0x11b48cc98, client=0x11b43cc08, markForInvalidation=false) at RenderSVGResourceGradient.cpp:68 #1 0x00000001021e33a0 in WebCore::SVGResources::removeClientFromCache (this=0x11b43cd20, object=0x11b43cc08, markForInvalidation=false) at SVGResources.cpp:316 #2 0x00000001021ee8c3 in WebCore::invalidateResourcesOfChildren (start=0x11b43cc08) at SVGRenderSupport.cpp:210 #3 0x00000001021f10b8 in WebCore::SVGRenderSupport::layoutChildren (start=0x11b442738, selfNeedsLayout=false) at SVGRenderSupport.cpp:258 #4 0x00000001021f825f in WebCore::RenderSVGRoot::layout (this=0x11b442738) at RenderSVGRoot.cpp:240 #5 0x000000010220b10b in WebCore::RenderObject::layoutIfNeeded (this=0x11b442738) at RenderObject.h:539 #6 0x000000010212ba60 in WebCore::RenderBlock::layoutInlineChildren (this=0x11b449278, relayoutChildren=true, repaintLogicalTop=@0x7fff5fbfd8a8, repaintLogicalBottom=@0x7fff5fbfd8a4) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/rendering/RenderBlockLineLayout.cpp:1175 #7 0x000000010210869b in WebCore::RenderBlock::layoutBlock (this=0x11b449278, relayoutChildren=true, pageLogicalHeight=0) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1236 #8 0x000000010210026c in WebCore::RenderBlock::layout (this=0x11b449278) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1134 #9 0x0000000102107067 in WebCore::RenderBlock::layoutBlockChild (this=0x1081d7a38, child=0x11b449278, marginInfo=@0x7fff5fbfda70, previousFloatLogicalBottom=@0x7fff5fbfdae4, maxFloatLogicalBottom=@0x7fff5fbfdc80) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1977 #10 0x0000000102108046 in WebCore::RenderBlock::layoutBlockChildren (this=0x1081d7a38, relayoutChildren=false, maxFloatLogicalBottom=@0x7fff5fbfdc80) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1915 #11 0x00000001021086b4 in WebCore::RenderBlock::layoutBlock (this=0x1081d7a38, relayoutChildren=false, pageLogicalHeight=0) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1238 #12 0x000000010210026c in WebCore::RenderBlock::layout (this=0x1081d7a38) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1134 #13 0x0000000102107067 in WebCore::RenderBlock::layoutBlockChild (this=0x1081ecd88, child=0x1081d7a38, marginInfo=@0x7fff5fbfde50, previousFloatLogicalBottom=@0x7fff5fbfdec4, maxFloatLogicalBottom=@0x7fff5fbfe060) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1977 #14 0x0000000102108046 in WebCore::RenderBlock::layoutBlockChildren (this=0x1081ecd88, relayoutChildren=false, maxFloatLogicalBottom=@0x7fff5fbfe060) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1915 #15 0x00000001021086b4 in WebCore::RenderBlock::layoutBlock (this=0x1081ecd88, relayoutChildren=false, pageLogicalHeight=0) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1238 #16 0x000000010210026c in WebCore::RenderBlock::layout (this=0x1081ecd88) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1134 #17 0x0000000102107067 in WebCore::RenderBlock::layoutBlockChild (this=0x106da8178, child=0x1081ecd88, marginInfo=@0x7fff5fbfe230, previousFloatLogicalBottom=@0x7fff5fbfe2a4, maxFloatLogicalBottom=@0x7fff5fbfe440) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1977 #18 0x0000000102108046 in WebCore::RenderBlock::layoutBlockChildren (this=0x106da8178, relayoutChildren=false, maxFloatLogicalBottom=@0x7fff5fbfe440) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1915 #19 0x00000001021086b4 in WebCore::RenderBlock::layoutBlock (this=0x106da8178, relayoutChildren=false, pageLogicalHeight=0) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1238 #20 0x000000010210026c in WebCore::RenderBlock::layout (this=0x106da8178) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1134 #21 0x000000010226fad9 in WebCore::RenderView::layout (this=0x106da8178) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/rendering/RenderView.cpp:130 #22 0x0000000101aa0617 in WebCore::FrameView::layout (this=0x11a659690, allowSubtree=true) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/page/FrameView.cpp:1000 #23 0x00000001018c3bad in WebCore::Document::updateLayout (this=0x107119000) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/dom/Document.cpp:1612 #24 0x0000000102189d69 in WebCore::RenderLayer::hitTest (this=0x106deaf28, request=@0x7fff5fbfeab0, result=@0x7fff5fbfe850) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/rendering/RenderLayer.cpp:2846 #25 0x00000001018c2f1b in WebCore::Document::prepareMouseEvent (this=0x107119000, request=@0x7fff5fbfeab0, documentPoint=@0x7fff5fbfe940, event=@0x7fff5fbfebd0) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/dom/Document.cpp:2625 #26 0x0000000101a00529 in WebCore::EventHandler::prepareMouseEvent (this=0x1070d8818, request=@0x7fff5fbfeab0, mev=@0x7fff5fbfebd0) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/page/EventHandler.cpp:1930 #27 0x0000000101a07d90 in WebCore::EventHandler::handleMouseMoveEvent (this=0x1070d8818, mouseEvent=@0x7fff5fbfebd0, hoveredNode=0x7fff5fbfeb00) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/page/EventHandler.cpp:1610 #28 0x0000000101a08333 in WebCore::EventHandler::mouseMoved (this=0x1070d8818, event=@0x7fff5fbfebd0) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/page/EventHandler.cpp:1542 #29 0x0000000101a0e306 in WebCore::EventHandler::mouseMoved (this=0x1070d8818, event=0x11b473fc0) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/page/mac/EventHandlerMac.mm:606 #30 0x00000001010de126 in -[WebHTMLView(WebPrivate) _updateMouseoverWithEvent:] (self=0x106dee300, _cmd=0x7fff8481d2b5, event=0x11b473fc0) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebKit/mac/WebView/WebHTMLView.mm:1619 #31 0x00000001010c9b21 in -[WebHTMLView(WebPrivate) _updateMouseoverWithFakeEvent] (self=0x106dee300, _cmd=0x7fff8483af0e) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebKit/mac/WebView/WebHTMLView.mm:1201 #32 0x00000001010d4188 in -[WebHTMLView mouseUp:] (self=0x106dee300, _cmd=0x7fff82b1368c, event=0x11b40b610) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebKit/mac/WebView/WebHTMLView.mm:3665 #33 0x00007fff8250d3d9 in -[NSWindow sendEvent:] () #34 0x00000001000421b5 in ?? () #35 0x0000000100042142 in ?? () #36 0x00007fff82442a86 in -[NSApplication sendEvent:] () #37 0x0000000100038e72 in ?? () #38 0x00007fff823d94da in -[NSApplication run] () #39 0x00007fff823d21a8 in NSApplicationMain () #40 0x0000000100009f18 in ?? () (gdb)

So what's the way forward here? Accept the patch as-is?

Comment on attachment 95106 [details] Attempt to fix style errors View in context: https://bugs.webkit.org/attachment.cgi?id=95106&action=review r=m, but please fix the changelog. Not sure if you have commit rights. If not, please upload a new patch and set 'r' and 'cq' to '?'. I'll review it again. > ChangeLog:8 > + Reviewed by NOBODY (OOPS!). > + > + Correct the logic for checking for relative lengths in an SVGRadialGradientElement to consider cx() in addition to cy() > + https://bugs.webkit.org/show_bug.cgi?id=61600. > + > + * Source/WebCore/svg/SVGRadialGradientElement.cpp: This should be: Reviewed by NOBODY (OOPS!). SVGRadialGradientElement::selfHasRelativeLengths() doesn't consider if cx() is relative https://bugs.webkit.org/show_bug.cgi?id=61600. Correct the logic for checking for relative lengths in an SVGRadialGradientElement to consider cx() in addition to cy() ... (With correct indention of course) Please add a comment that there is no way to test it and why it can't be tested as well.

Created attachment 99050 [details] Patch

Comment on attachment 99050 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=99050&action=review r- because you're not a commiter yet and can't correct the patch before landing it. > Source/WebCore/ChangeLog:9 > + No new tests, as there is no way to currently test this. With the bug present, a side-effect is causing the expected/correct image to be rendered. This is because the cached SVG resource is getting invalided through a different path, forcing a repaint. The repaint takes into consideration the proper dimensions, hiding the bug. Just a snippet, can you please break the line? This comment is too long to stay in one line. Lines shouldn't be longer than the line with the sentence "Correct the logic....".

Created attachment 99051 [details] Patch

Comment on attachment 99051 [details] Patch Updated the ChangeLog per your request. Since this has happened with both patches that I've submitted - where they didn't conform to certain styles/expectations - is there any documentation about this on webkit.org that I might read, saving cycles for both myself and reviewer? I've read much of the documentation, technical articles, and read perhaps way too much on the Wiki, but it seems these minor things keep biting me.

(In reply to comment #19) > (From update of attachment 99051 [details]) > Updated the ChangeLog per your request. > > Since this has happened with both patches that I've submitted - where they didn't conform to certain styles/expectations - is there any documentation about this on webkit.org that I might read, saving cycles for both myself and reviewer? > > I've read much of the documentation, technical articles, and read perhaps way too much on the Wiki, but it seems these minor things keep biting me. You can use the script: Tools/Scripts/prepare-ChangeLog --bug <bug number> --email=<your email> It will create the ChangeLogs for you. You just have to fill the description.

(In reply to comment #20) > (In reply to comment #19) > > (From update of attachment 99051 [details] [details]) > > Updated the ChangeLog per your request. > > > > Since this has happened with both patches that I've submitted - where they didn't conform to certain styles/expectations - is there any documentation about this on webkit.org that I might read, saving cycles for both myself and reviewer? > > > > I've read much of the documentation, technical articles, and read perhaps way too much on the Wiki, but it seems these minor things keep biting me. > > You can use the script: > > > Tools/Scripts/prepare-ChangeLog --bug <bug number> --email=<your email> > > It will create the ChangeLogs for you. You just have to fill the description. Thanks. Unfortunately, that's exactly the tool I was using, which didn't help with this particular style issue. I suppose I expected it to complain about the line length (since check-webkit-style is so good about such issues), or, failing that, I was hoping to find documentation somewhere as far as expectations for comment style, tests format, etc, similar to the WebKit coding style documentation. While less of an issue with this bug, compared to the other bug I filed, I wanted to try to be a bit more proactive in understanding "The WebKit Way" for such contributions. Thanks for the r+/cq+.

Comment on attachment 99051 [details] Patch Clearing flags on attachment: 99051 Committed r90003: <http://trac.webkit.org/changeset/90003>

All reviewed patches have been landed. Closing bug.