| Summary: | Convert SVGPathSeg/SVGPathSegList to the new SVGPropertyTearOff concept | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Nikolas Zimmermann <zimmermann> | ||||||||||
| Component: | SVG | Assignee: | Nikolas Zimmermann <zimmermann> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | abarth, adeveria, eric, jeffschiller, krit, mdelaney7, natevw, webkit-ews, webkit.review.bot, zimmermann | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | PC | ||||||||||||
| OS: | OS X 10.5 | ||||||||||||
| Bug Depends on: | 49728, 49730 | ||||||||||||
| Bug Blocks: | 47905 | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Nikolas Zimmermann
2010-11-15 23:26:05 PST
*** Bug 30219 has been marked as a duplicate of this bug. *** *** Bug 10827 has been marked as a duplicate of this bug. *** *** Bug 19741 has been marked as a duplicate of this bug. *** *** Bug 43388 has been marked as a duplicate of this bug. *** My upcoming patch fixes all of the bugs, that I marked as duplicate of this one. 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.
Created attachment 74111 [details]
Patch v2
Updated to trunk so the bots can apply the patch.
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. 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!
(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. 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? (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! 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.. Created attachment 74220 [details]
Patch v4
Fixed Dirks comments.
Attachment 74220 [details] did not build on qt: Build output: http://queues.webkit.org/results/6132047 (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 ok, fixed Qt, accidently added SVGPathSegListPropertyTearOff.cpp to the HEADERS listed, instead of SOURCES. Won't upload a new patch though. 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. 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) http://trac.webkit.org/changeset/72288 might have broken GTK Linux 64-bit Debug SVGPathSegList.cpp was missing from SVGAllInOne.cpp, this should fix the windows build. Relanding. Relanded in r72381. Closing bug once everything built fine. http://trac.webkit.org/changeset/72381 might have broken Qt Windows 32-bit Release It builds on Windows 7, closing bug. Oops, forgot to reapply the Qt build fix when landing the second time. Fixed in r72384. |