Closed Bug 1024926 Opened 7 years ago Closed 7 years ago
SVG Path get
Total Length() does not change after modifying path Seg List
User Agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:31.0) Gecko/20100101 Firefox/31.0 (Beta/Release) Build ID: 20140609004001 Steps to reproduce: http://jsfiddle.net/7LpsZ/ 1. Create a path with some data. 2. Read getTotalLength() from the path. 3. Add/remove path segments to/from the pathSegList. 4. Read getTotalLength() from the path again. Actual results: The old value from the first time getTotalLength() was called is returned. It appears a cached value didn't get invalidated after the pathSegList was modified. Expected results: A new value should be returned from getTotalLength() after the pathSegList is modified.
This bug was introduced in Firefox 31, was previously working in Firefox 30. Note the jsfiddle example throws an exception in Firefox 30 due to a separate bug in getTotalLength(). See: https://bugzilla.mozilla.org/show_bug.cgi?id=1024860
(In reply to alancutter from comment #1) > This bug was introduced in Firefox 31, was previously working in Firefox 30. > > Note the jsfiddle example throws an exception in Firefox 30 due to a > separate bug in getTotalLength(). > See: https://bugzilla.mozilla.org/show_bug.cgi?id=1024860 Can you provide an example that does actually work in Fx 30? That'll make it easier for someone to figure out when this regressed. :-)
bug 1024860 says that getTotalLength throws when there's no path. Is that all you're seeing as that's what the jsfiddle does or is there something else?
Sorry, I can see it'd be confusing with two bugs in the same example now. :) Updated example to avoid bug 1024860: http://jsfiddle.net/7LpsZ/2/ Test output for Firefox 31.0a2: """ Created path getTotalLength() == 500 Expected 500 Removed 2 path segments getTotalLength() == 500 Expected 150 """ Test output for Firefox 30.0b9 and Chrome: """ Created path getTotalLength() == 500 Expected 500 Removed 2 path segments getTotalLength() == 150 Expected 150 """
Regression window(m-i) Good: https://hg.mozilla.org/integration/mozilla-inbound/rev/ea97b234c940 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0 ID:20140528161650 Bad: https://hg.mozilla.org/integration/mozilla-inbound/rev/8cb1217dc3ff Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0 ID:20140528165349 Pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ea97b234c940&tochange=8cb1217dc3ff Regressed by: 8cb1217dc3ff Bas Schouten — Bug 992850: Cache Moz2D path for length or position measuring. r=jwatt
Status: UNCONFIRMED → NEW
Ever confirmed: true
Jonathan, Daniel do you think we should get bug 992850 backed out of everywhere except the trunk? If so can one of you do that please? If not we will need to land this instead. Personally I'd go with the backout.
Sorry, I was out last week. Tagging jwatt for needinfo for response to Comment 7, since he was involved [as reviewer] on the bug in question, whereas I don't really know the backstory.
Comment on attachment 8440337 [details] [diff] [review] cache.txt Bas, could you help for the review? We could accept a late fix for beta 7 (Thursday) Thanks
Comment on attachment 8440337 [details] [diff] [review] cache.txt This code needs an SVG peer to review it. I'm reviewing it just now.
Wait for Jwatt's review though :-).
Comment on attachment 8440337 [details] [diff] [review] cache.txt I did a close audit of the code and fixed one remaining source of stale measuring paths in DOMSVGPathSeg.cpp. Otherwise this looks correct though, so r+. I pushed the patch plus the DOMSVGPathSeg.cpp fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/5cfae96d4136
Comment on attachment 8440337 [details] [diff] [review] cache.txt Approval Request Comment [Feature/regressing bug #]: bug 992850 [User impact if declined]: some broken SVG [Describe test coverage new/current, TBPL]: been on m-i overnight [Risks and why]: very low - only clears a cached object, forcing us to recreate it when appropriate [String/UUID change made/needed]: none
Comment on attachment 8440337 [details] [diff] [review] cache.txt Recent regression. Approved.
I see this is covered by test in: https://hg.mozilla.org/releases/mozilla-beta/diff/a6cde8bb0578/content/svg/content/test/test_pathLength.html
You need to log in before you can comment on or make changes to this bug.