Bug 46051 – SVG: Make RenderPath DRT output platform-independent

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Andreas Kling]

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

[Andreas Kling]

Created attachment 68026 [details]
Proposed patch, first stab

A first modest attempt at this - not gonna bother rebaselining tests just yet.

[WebKit Review Bot]

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.

[Antonio Gomes]

See also bug 33784.

[Nikolas Zimmermann]

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)) { ...}

[Andreas Kling]

Created attachment 68073 [details]
Proposed patch v2

Using tag names instead of adding virtuals, as per Niko's request.

[Nikolas Zimmermann]

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.

[Andreas Kling]

(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.)

[Nikolas Zimmermann]

(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?

[Andreas Kling]

I've asked Ossy to help with Qt results and Tor Arne with Mac.

[Csaba Osztrogonác]

(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". ;)

[Csaba Osztrogonác]

(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.

[Andreas Kling]

(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?

[Dirk Schulze]

(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?

[Dirk Schulze]

(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.

[Nikolas Zimmermann]

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).

[Nikolas Zimmermann]

(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.

[Nikolas Zimmermann]

Comment on attachment 68073 [details]
Proposed patch v2

Ok, we made up our mind.

[Nikolas Zimmermann]

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.

[Andreas Kling]

Committed r69386: <http://trac.webkit.org/changeset/69386>