DRT currently uses the port-specific Path::debugString() to dump RenderPath objects. We should switch to using a platform-independent output format for paths, and also simplify the format used for primitives (rect, circle, ellipse, line and polygon.) See also: https://lists.webkit.org/pipermail/webkit-dev/2010-September/014419.html
Created attachment 68026 [details] Proposed patch, first stab A first modest attempt at this - not gonna bother rebaselining tests just yet.
Attachment 68026 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/rendering/SVGRenderTreeAsText.cpp:65: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
See also bug 33784.
Comment on attachment 68026 [details] Proposed patch, first stab Looks good to me! I'd propose to not add those virtual methods (isRectElement) etc, just for the sake of DRT. Just compare the tag names: if (svgElement->hasTagName(SVGNames::rectTag)) { ...}
Created attachment 68073 [details] Proposed patch v2 Using tag names instead of adding virtuals, as per Niko's request.
Comment on attachment 68073 [details] Proposed patch v2 Looks great Andreas! Can you please upload a seperated patch including all rebaselines first? You also have to take special care of all platform specific results, not living in platform/mac. I'll r+ it afterwards.
(In reply to comment #6) > (From update of attachment 68073 [details]) > Looks great Andreas! Can you please upload a seperated patch including all rebaselines first? > You also have to take special care of all platform specific results, not living in platform/mac. > I'll r+ it afterwards. I only have access to the Qt platform right now, so I can do those - would it be acceptable to use "webkit-patch rebaseline" to complete with results from bots after landing? (I will upload a patch with Qt rebaselines first.)
(In reply to comment #7) > (In reply to comment #6) > > (From update of attachment 68073 [details] [details]) > > Looks great Andreas! Can you please upload a seperated patch including all rebaselines first? > > You also have to take special care of all platform specific results, not living in platform/mac. > > I'll r+ it afterwards. > > I only have access to the Qt platform right now, so I can do those - would it be acceptable to use "webkit-patch rebaseline" to complete with results from bots after landing? (I will upload a patch with Qt rebaselines first.) Hmm, that's a problem, as IIRC the bots exit after 20 failures, so it would require many iterations to fix all Mac bots. Can someone generate the mac results for you, my tree is hosed atm?
I've asked Ossy to help with Qt results and Tor Arne with Mac.
(In reply to comment #8) > Hmm, that's a problem, as IIRC the bots exit after 20 failures, so it would require many iterations to fix all Mac bots. No, fortunately bots use "--exit-after-n-crashes-or-timeouts 20" option instead of previous "--exit-after-n-failures 20". ;)
(In reply to comment #9) > I've asked Ossy to help with Qt results and Tor Arne with Mac. I checked your patch, normally we should update ~140 expected files. But I thought over this task, and I have a better idea. :) We should update platform independent expected files first, and Qt specific after that. I hope we can remove some Qt specific files.
(In reply to comment #10) > No, fortunately bots use "--exit-after-n-crashes-or-timeouts 20" > option instead of previous "--exit-after-n-failures 20". ;) > I checked your patch, normally we should update ~140 expected files. > But I thought over this task, and I have a better idea. :) We should > update platform independent expected files first, and Qt specific > after that. I hope we can remove some Qt specific files. That would be fantastic. :) Nikolas, thoughts?
(In reply to comment #12) > (In reply to comment #10) > > No, fortunately bots use "--exit-after-n-crashes-or-timeouts 20" > > option instead of previous "--exit-after-n-failures 20". ;) > > > I checked your patch, normally we should update ~140 expected files. > > But I thought over this task, and I have a better idea. :) We should > > update platform independent expected files first, and Qt specific > > after that. I hope we can remove some Qt specific files. > > That would be fantastic. :) Nikolas, thoughts? To the patch. Please correct me if I'm wrong, but you may get just the relative values for rect or circle, if x,y,.. are in percentage. It would be great if we could get the repaintRect of the shape. On the other side, we'll get platform dependent results again, because RenderPath::repaintRectInLocalCoordinates() calls Path::strokeBoundaries and Path::fillBoundaries. So I'm still unsure if this is the right way to go. Niko, what is your opinion?
(In reply to comment #11) > (In reply to comment #9) > > I've asked Ossy to help with Qt results and Tor Arne with Mac. > > I checked your patch, normally we should update ~140 expected files. > But I thought over this task, and I have a better idea. :) We should > update platform independent expected files first, and Qt specific > after that. I hope we can remove some Qt specific files. Yes, it's better to test it on mac, create the reuslts and look if other platforms can reuse it afterwards. But we'll still end up with different results because of the fonts.
Comment on attachment 68073 [details] Proposed patch v2 As discussed on IRC, the right way to go is to introduce a RenderSVGShape class whose repaintRectInLocalCoordinates can be dumped. The problem is that dumping RenderPath::repaintRectInLocalCoordinates, is that it's platform-dependant, as Path::boundingBox is used. As first step, I'd introduce RenderSVGShape : public RenderPath (which should be renamed and moved to rendering/svg/RenderSVGPath first) and let SVGCircle/RectElement etc. create RenderSVGShape instead of RenderSVGPath. Once that's done, we need to introduce platform-dependant ways to create rects, circles, instead of the home-brewn code in Path.cpp (addEllipse, etc.) Then we need to write a method which calculates the circle/rect/etc. boundaries in a _cross platform_ fashion. Then RenderSVGShape can use this new logic, and inherit from RenderSVGModelObject, and be completly decoupled from RenderSVGPath. Once that's done the DRT problem vanishes. (This is just a short summary of the IRC discussion).
(In reply to comment #15) > (From update of attachment 68073 [details]) > As discussed on IRC, the right way to go is to introduce a RenderSVGShape class whose repaintRectInLocalCoordinates can be dumped. > The problem is that dumping RenderPath::repaintRectInLocalCoordinates, is that it's platform-dependant, as Path::boundingBox is used. > > As first step, I'd introduce RenderSVGShape : public RenderPath (which should be renamed and moved to rendering/svg/RenderSVGPath first) > and let SVGCircle/RectElement etc. create RenderSVGShape instead of RenderSVGPath. > > Once that's done, we need to introduce platform-dependant ways to create rects, circles, instead of the home-brewn code in Path.cpp (addEllipse, etc.) > Then we need to write a method which calculates the circle/rect/etc. boundaries in a _cross platform_ fashion. > > Then RenderSVGShape can use this new logic, and inherit from RenderSVGModelObject, and be completly decoupled from RenderSVGPath. > Once that's done the DRT problem vanishes. Ok, we had another discussion. This approach is fine. We'll just move SVG away from using the static Path::create* methods, and let them use Path::addEllipse etc. These methods already ask their platform specific implementations, and we don't need to decompose ellipses to paths etc. on our own. Andreas patch is just fine! I'll upload a baseline for mac, soon.
Comment on attachment 68073 [details] Proposed patch v2 Ok, we made up our mind.
Created attachment 70215 [details] Mac layout test results As mentioned on IRC, the patch is missing an " || svgElement->hasTagName(SVGNames::polylineTag)" branch, the rest is perfect. Please land your patch together with the mac results.
Committed r69386: <http://trac.webkit.org/changeset/69386>