Bug 49580 - Convert SVGPathSeg/SVGPathSegList to the new SVGPropertyTearOff concept
Summary: Convert SVGPathSeg/SVGPathSegList to the new SVGPropertyTearOff concept
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
: 10827 19741 30219 43388 (view as bug list)
Depends on: 49728 49730
Blocks: 47905
  Show dependency treegraph
 
Reported: 2010-11-15 23:26 PST by Nikolas Zimmermann
Modified: 2010-11-19 05:20 PST (History)
10 users (show)

See Also:


Attachments
Patch (1.18 MB, patch)
2010-11-17 06:55 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch v2 (1.17 MB, patch)
2010-11-17 07:18 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch v3 (1.16 MB, patch)
2010-11-17 08:06 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch v4 (1.17 MB, patch)
2010-11-18 02:44 PST, Nikolas Zimmermann
krit: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 2010-11-15 23:26:05 PST
Convert the last missing type to the new concept!
Comment 1 Nikolas Zimmermann 2010-11-17 06:28:31 PST
*** Bug 30219 has been marked as a duplicate of this bug. ***
Comment 2 Nikolas Zimmermann 2010-11-17 06:29:08 PST
*** Bug 10827 has been marked as a duplicate of this bug. ***
Comment 3 Nikolas Zimmermann 2010-11-17 06:35:55 PST
*** Bug 19741 has been marked as a duplicate of this bug. ***
Comment 4 Nikolas Zimmermann 2010-11-17 06:39:22 PST
*** Bug 43388 has been marked as a duplicate of this bug. ***
Comment 5 Nikolas Zimmermann 2010-11-17 06:39:53 PST
My upcoming patch fixes all of the bugs, that I marked as duplicate of this one.
Comment 6 Nikolas Zimmermann 2010-11-17 06:55:39 PST
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.
Comment 7 Nikolas Zimmermann 2010-11-17 07:18:26 PST
Created attachment 74111 [details]
Patch v2

Updated to trunk so the bots can apply the patch.
Comment 8 Dirk Schulze 2010-11-17 07:45:53 PST
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.
Comment 9 Nikolas Zimmermann 2010-11-17 08:06:00 PST
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!
Comment 10 Nikolas Zimmermann 2010-11-17 08:15:47 PST
(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.
Comment 11 Dirk Schulze 2010-11-17 08:35:14 PST
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?
Comment 12 Nikolas Zimmermann 2010-11-17 08:41:33 PST
(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!
Comment 13 Nikolas Zimmermann 2010-11-18 02:34:42 PST
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..
Comment 14 Nikolas Zimmermann 2010-11-18 02:44:34 PST
Created attachment 74220 [details]
Patch v4

Fixed Dirks comments.
Comment 15 Early Warning System Bot 2010-11-18 03:21:02 PST
Attachment 74220 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6132047
Comment 16 Nikolas Zimmermann 2010-11-18 03:28:57 PST
(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
Comment 17 Nikolas Zimmermann 2010-11-18 03:46:15 PST
ok, fixed Qt, accidently added SVGPathSegListPropertyTearOff.cpp to the HEADERS listed, instead of SOURCES. Won't upload a new patch though.
Comment 18 Nikolas Zimmermann 2010-11-18 05:44:32 PST
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.
Comment 19 Dirk Schulze 2010-11-18 06:27:01 PST
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)
Comment 20 WebKit Review Bot 2010-11-18 07:19:10 PST
http://trac.webkit.org/changeset/72288 might have broken GTK Linux 64-bit Debug
Comment 21 Nikolas Zimmermann 2010-11-19 03:21:04 PST
SVGPathSegList.cpp was missing from SVGAllInOne.cpp, this should fix the windows build. Relanding.
Comment 22 Nikolas Zimmermann 2010-11-19 04:55:27 PST
Relanded in r72381. Closing bug once everything built fine.
Comment 23 WebKit Review Bot 2010-11-19 05:06:24 PST
http://trac.webkit.org/changeset/72381 might have broken Qt Windows 32-bit Release
Comment 24 Nikolas Zimmermann 2010-11-19 05:13:18 PST
It builds on Windows 7, closing bug.
Comment 25 Nikolas Zimmermann 2010-11-19 05:20:46 PST
Oops, forgot to reapply the Qt build fix when landing the second time.
Fixed in r72384.