Summary: | getIntersectionList causes transforms to be recalculated in SVG | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Philip Rogers <pdr> | ||||||||||||
Component: | SVG | Assignee: | Peter Beverloo <peter> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | peter, rwlbuis, webkit.review.bot, zimmermann | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 11274 | ||||||||||||||
Attachments: |
|
Description
Philip Rogers
2012-01-26 20:16:30 PST
Bisecting Chromium builds ends up in the following WebKit commit range (r91856-r91828, http://trac.webkit.org/log/trunk/?rev=91856&stop_rev=91828&verbose=on), most interesting clearly is this commit by rbuis: http://trac.webkit.org/changeset/91850 The issue seems to be on line 127 of RenderSVGModelObject.cpp (http://trac.webkit.org/changeset/91850/trunk/Source/WebCore/rendering/svg/RenderSVGModelObject.cpp), which retrieves the renderer's localToParentTransform AffineTransform, removes the const and multiplies it with the currently known transform. Patch upcoming. Created attachment 124723 [details]
Patch
No new tests as the rect's get(Screen)CTM() methods don't seem to be influenced by this, though verified manually. Pointers in how to write one would be welcome, of course!
Attachment 124723 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9
Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: Fix compilation errors on build-webkit --debug --no-workers on mac.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/qt/Skipped
CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac.
When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".
rebase refs/remotes/origin/master: command returned error: 1
Died at Tools/Scripts/update-webkit line 164.
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 124746 [details]
Patch (+ test)
Added a test following a discussion with pdr in #ksvg.
Attachment 124746 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9
Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: Fix compilation errors on build-webkit --debug --no-workers on mac.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/qt/Skipped
CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac.
When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".
rebase refs/remotes/origin/master: command returned error: 1
Died at Tools/Scripts/update-webkit line 164.
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #4) > Created an attachment (id=124746) [details] > Patch (+ test) > > Added a test following a discussion with pdr in #ksvg. I think this change looks good overall. A couple very minor nits: I think git is messing up the png in your ChangeLog, which is causing the style guide to complain. A common pattern in pixel tests is to have an untranslated green square as "pass". Could you make the initial rotation be 0 or add a comment in the test describing that a rotated green square should be considered passing? This will make it easier for sheriffs/rebaseliners. One way would be to rotate the entire svg element by -50deg. (In reply to comment #6) > (In reply to comment #4) > > Created an attachment (id=124746) [details] [details] > > Patch (+ test) > > > > Added a test following a discussion with pdr in #ksvg. > > I think this change looks good overall. > > A couple very minor nits: I think git is messing up the png in your ChangeLog, which is causing the style guide to complain. > A common pattern in pixel tests is to have an untranslated green square as "pass". Could you make the initial rotation be 0 or add a comment in the test describing that a rotated green square should be considered passing? This will make it easier for sheriffs/rebaseliners. One way would be to rotate the entire svg element by -50deg. Thank you. The style-bot was having troubles yesterday so I'm fairly sure it's unrelated to the patch. The new patch should be fine. I've made the test clearer by making the square blue instead of green, and added a comment that a non-rotated square in the top-left corner is the expected result, whereas other positions and/or rotations are failures. Created attachment 124925 [details]
Patch (+ test)
Updated patch.
Comment on attachment 124925 [details] Patch (+ test) View in context: https://bugs.webkit.org/attachment.cgi?id=124925&action=review Good catch, r=me with comments: > LayoutTests/svg/custom/intersection-list-transforms.svg:13 > + <!-- This test passes if there is a blue non-rotated square visible on the top-left corner of the page. --> It should be green. Created attachment 124927 [details]
Patch for landing
Thank you for the review! Changed the test and results to create a green square.
Comment on attachment 124927 [details] Patch for landing Clearing flags on attachment: 124927 Committed r106464: <http://trac.webkit.org/changeset/106464> All reviewed patches have been landed. Closing bug. |