Bug 49580 – Convert SVGPathSeg/SVGPathSegList to the new SVGPropertyTearOff concept

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

[Nikolas Zimmermann]

Convert the last missing type to the new concept!

[Nikolas Zimmermann]

*** Bug 30219 has been marked as a duplicate of this bug. ***

[Nikolas Zimmermann]

*** Bug 10827 has been marked as a duplicate of this bug. ***

[Nikolas Zimmermann]

*** Bug 19741 has been marked as a duplicate of this bug. ***

[Nikolas Zimmermann]

*** Bug 43388 has been marked as a duplicate of this bug. ***

[Nikolas Zimmermann]

My upcoming patch fixes all of the bugs, that I marked as duplicate of this one.

[Nikolas Zimmermann]

Created attachment 74108 [details]
Patch

Uploading a first version of my patch. Build has only been tested on Mac Leopard.
ChangeLog is not filled yet, I only want to get early EWS results, especially whether it builds on chromium/v8.

Fixes lots of bugs, and gives us a huge performance boost, as static parsing doesn't involve building SVGPathSegLists anymore.

[Nikolas Zimmermann]

Created attachment 74111 [details]
Patch v2

Updated to trunk so the bots can apply the patch.

[Dirk Schulze]

Comment on attachment 74108 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74108&action=review

Ok, I need some more information, what is 'role' good for?!? To determine in which list the PathSeg is (normalized, unaltered), or just for the difference between m and M?? But we do we need to give a role to the create function in SVGPathElement? Shouldn't it be always normalized for Abs and unaltered for Rel?!? We have the contextElement for every PathSeg, but how do we cover if the PathSeg is in a list? How do we cover if the dAttr and the ByteStream needs to update?
Also can't we join PathParsingMode and PathSegRole, since they cover the same (with the exception of unkown, but this shouldn't be a big problem)?

> WebCore/bindings/scripts/CodeGeneratorV8.pm:686
> -    if ($implClassName eq "SVGZoomEvent") {
> -        $skipContext = 1;
> -    }
>  
>      my $getterStringUsesImp = $implClassName ne "SVGNumber";

Did this change?

> WebCore/svg/SVGAnimatedPathData.idl:-33
> -    interface [Conditional=SVG, ObjCProtocol, OmitConstructor] SVGAnimatedPathData {
> -        readonly attribute SVGPathSegList   pathSegList;
> -        readonly attribute SVGPathSegList   normalizedPathSegList;
> -        readonly attribute SVGPathSegList   animatedPathSegList;
> -        readonly attribute SVGPathSegList   animatedNormalizedPathSegList;
> -    };

You didn't worte a ChangeLog, so I don't know if removing this is ok.

> WebCore/svg/SVGPathElement.cpp:285
> +    // FIXME!

You could write at least a short note here ;-)

> WebCore/svg/SVGPathElement.cpp:306
> +    // FIXME!

Ditto. Don't we have bug reports for this? Can you add them here please?

> WebCore/svg/SVGPathElement.cpp:324
> +        notImplemented();

Can you add a Fixme here please?

> WebCore/svg/SVGPathSeg.h:56
> +enum SVGPathSegRole {
> +    PathSegUnalteredRole = 0,
> +    PathSegNormalizedRole = 1,
> +    PathSegUndefinedRole = 2
> +};

Not sure if there is something about enum changes later, but we already have an enum for normalized and unaltered SegList (PathParsingMode?), can we combine those?

> WebCore/svg/SVGPathSegClosePath.h:34
> +    static PassRefPtr<SVGPathSegClosePath> create(SVGPathElement* element, SVGPathSegRole role)
> +    {
> +        return adoptRef(new SVGPathSegClosePath(element, role));
> +    }

There is no difference on closeSeg, so no role needed.

[Nikolas Zimmermann]

Created attachment 74115 [details]
Patch v3

Build works fine on Chromium/V8, filled the ChangeLog with all necessary information.
The patch can now be considered ready for review.

A follow-up patch can remove SVGList, JSSVGContextCache, JSSVGPODTypeWrapper and all v8 counterparts, finally!

[Nikolas Zimmermann]

(In reply to comment #8)
> (From update of attachment 74108 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74108&action=review
> 
> Ok, I need some more information, what is 'role' good for?!? To determine in which list the PathSeg is (normalized, unaltered), or just for the difference between m and M?? But we do we need to give a role to the create function in SVGPathElement? Shouldn't it be always normalized for Abs and unaltered for Rel?!? We have the contextElement for every PathSeg, but how do we cover if the PathSeg is in a list? How do we cover if the dAttr and the ByteStream needs to update?

Each SVGPathSeg now stores a RefPtr<SVGPathElement>. That means, we know now to which SVGPathElement a SVGPathSeg belongs. The SVGPathSegRole defines to which list it belongs, either pathSegList or normalizedPathSegList.
When creating a new SVGPathSeg via myPathElement.createSVGPathSeg* the role is PathSegUndefinedRole initially, as the new segment doesn't yet live in a list.

> Also can't we join PathParsingMode and PathSegRole, since they cover the same (with the exception of unkown, but this shouldn't be a big problem)?
That's true, but currently we _could_ parse a path with 'RelativeCoordinates' parsing mode and insert it into a non-normalized pathSegList. Maybe this is not needed, and we should just unify the enums. Agreed?

> 
> > WebCore/bindings/scripts/CodeGeneratorV8.pm:686
> > -    if ($implClassName eq "SVGZoomEvent") {
> > -        $skipContext = 1;
> > -    }
> >  
> >      my $getterStringUsesImp = $implClassName ne "SVGNumber";
> 
> Did this change?
Yes, the context concept is gone. So this also needs to go away.

> 
> > WebCore/svg/SVGAnimatedPathData.idl:-33
> > -    interface [Conditional=SVG, ObjCProtocol, OmitConstructor] SVGAnimatedPathData {
> > -        readonly attribute SVGPathSegList   pathSegList;
> > -        readonly attribute SVGPathSegList   normalizedPathSegList;
> > -        readonly attribute SVGPathSegList   animatedPathSegList;
> > -        readonly attribute SVGPathSegList   animatedNormalizedPathSegList;
> > -    };
> 
> You didn't worte a ChangeLog, so I don't know if removing this is ok.
Sure, it's been merged into SVGPathElement, the new ChangeLog should clarify this.

> 
> > WebCore/svg/SVGPathElement.cpp:285
> > +    // FIXME!
> 
> You could write at least a short note here ;-)
> 
> > WebCore/svg/SVGPathElement.cpp:306
> > +    // FIXME!
> 
> Ditto. Don't we have bug reports for this? Can you add them here please?
True, changed to:
// FIXME: https://bugs.webkit.org/show_bug.cgi?id=15412 - Implement normalized path segment lists!

> 
> > WebCore/svg/SVGPathElement.cpp:324
> > +        notImplemented();
> 
> Can you add a Fixme here please?
Yeah, added the same comment as above.

> 
> > WebCore/svg/SVGPathSeg.h:56
> > +enum SVGPathSegRole {
> > +    PathSegUnalteredRole = 0,
> > +    PathSegNormalizedRole = 1,
> > +    PathSegUndefinedRole = 2
> > +};
> 
> Not sure if there is something about enum changes later, but we already have an enum for normalized and unaltered SegList (PathParsingMode?), can we combine those?
To be discussed, see above.

> 
> > WebCore/svg/SVGPathSegClosePath.h:34
> > +    static PassRefPtr<SVGPathSegClosePath> create(SVGPathElement* element, SVGPathSegRole role)
> > +    {
> > +        return adoptRef(new SVGPathSegClosePath(element, role));
> > +    }
> 
> There is no difference on closeSeg, so no role needed.
Sure, the role parameter tells you to which list this SVGPathSeg belongs.
You always need the SVGPathElement and the SVGPathSegRole parameters, to know to which list the segment belongs.

[Dirk Schulze]

How do you handle moving elements from one list of one SVGPathElement to a list of another one? Looks like the contextElement doesn't change of a SVGPathSeg on a insertBefore call. So does any change on the SVGPathSeg get commited to the wrong Element? And because it is not part of the list doesn't affect anything? Neither on the old, nor the new element?

[Nikolas Zimmermann]

(In reply to comment #11)
> How do you handle moving elements from one list of one SVGPathElement to a list of another one? Looks like the contextElement doesn't change of a SVGPathSeg on a insertBefore call. So does any change on the SVGPathSeg get commited to the wrong Element? And because it is not part of the list doesn't affect anything? Neither on the old, nor the new element?

That's true, the contextElement doesn't change, I need to fix that!
The invalidation works fine, as both lists are touched (path1.pathSegList and path2.pathSegList)
If you'd move item 1 of path1.pathSegList to path2.pathSegList, and modify the item afterwards (using path2.getItem(0).x += 150) the path1 would be invalidated, and no visible change would happen!

Good catch!

[Nikolas Zimmermann]

As dicussed with Dirk, the unification of the enums will be done in a seperated clean-up patch, as it would enlarge the patch even further..

[Nikolas Zimmermann]

Created attachment 74220 [details]
Patch v4

Fixed Dirks comments.

[Early Warning System Bot]

Attachment 74220 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6132047

[Nikolas Zimmermann]

(In reply to comment #15)
> Attachment 74220 [details] did not build on qt:
> Build output: http://queues.webkit.org/results/6132047

Hmm, almost looks like as if SVGPathSegListPropertyTearOff.cpp isn't built even though I added it to WebCore.pro

[Nikolas Zimmermann]

ok, fixed Qt, accidently added SVGPathSegListPropertyTearOff.cpp to the HEADERS listed, instead of SOURCES. Won't upload a new patch though.

[Nikolas Zimmermann]

Landed in r72288. I need to leave now, damn :( I hope it builds, Dirk will watch the bots for another 40 minutes, if there's still something wrong after that period, just roll it out.

[Dirk Schulze]

Broke win, had no time to investigate more, roll it out for now.


   Creating library C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\lib\WebKit.lib and object C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\lib\WebKit.exp
LINK : warning LNK4199: /DELAYLOAD:msimg32.dll ignored; no imports found from msimg32.dll
WebCore.lib(SVGAllInOne.obj) : error LNK2019: unresolved external symbol "public: void __thiscall WebCore::SVGPathSegList::commitChange(class WebCore::SVGElement *)" (?commitChange@SVGPathSegList@WebCore@@QAEXPAVSVGElement@2@@Z) referenced in function "private: virtual void __thiscall WebCore::SVGPathSegListPropertyTearOff::commitChange(void)" (?commitChange@SVGPathSegListPropertyTearOff@WebCore@@EAEXXZ)
WebCore.lib(SVGAllInOne.obj) : error LNK2019: unresolved external symbol "public: class WTF::String __thiscall WebCore::SVGPathSegList::valueAsString(void)const " (?valueAsString@SVGPathSegList@WebCore@@QBE?AVString@WTF@@XZ) referenced in function "private: void __thiscall WebCore::SVGPathElement::synchronizeD(void)" (?synchronizeD@SVGPathElement@WebCore@@AAEXXZ)
C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\bin\WebKit.dll : fatal error LNK1120: 2 unresolved externals
Project : warning PRJ0018 : The following environment variables were not found:
$(PRODUCTION)

[WebKit Review Bot]

http://trac.webkit.org/changeset/72288 might have broken GTK Linux 64-bit Debug

[Nikolas Zimmermann]

SVGPathSegList.cpp was missing from SVGAllInOne.cpp, this should fix the windows build. Relanding.

[Nikolas Zimmermann]

Relanded in r72381. Closing bug once everything built fine.

[WebKit Review Bot]

http://trac.webkit.org/changeset/72381 might have broken Qt Windows 32-bit Release

[Nikolas Zimmermann]

It builds on Windows 7, closing bug.

[Nikolas Zimmermann]

Oops, forgot to reapply the Qt build fix when landing the second time.
Fixed in r72384.