Bug 202415 - Remove SVGPropertyAccess from SVGProperty
Summary: Remove SVGPropertyAccess from SVGProperty
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords:
Depends on: 202411
Blocks:
  Show dependency tree / graph
 
Reported: 2019-10-01 11:42 PDT by Said Abou-Hallawa
Modified: 2019-10-30 12:00 PDT (History)
8 users (show)

See Also:

Attachments
Patch (42.48 KB, patch)
2019-10-01 11:45 PDT, Said Abou-Hallawa 		no flags Details | Formatted Diff | Diff
Patch for review (23.74 KB, patch)
2019-10-01 11:47 PDT, Said Abou-Hallawa 		sabouhallawa: review?
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2019-10-01 11:42:28 PDT 
Instead of keeping a member of SVGPropertyAccess in each SVGProperty, we can walk through the SVGPropertyOwner chain till we reach an owner of type SVGAnimatedProperty or SVGAnimatedPropertyList. This owner compares the property to be modified with its animVal. And if they are the same, it will return 'true' through its virtual method isPropertyReadOnly().
Comment 1 Said Abou-Hallawa 2019-10-01 11:45:50 PDT 
Created attachment 379919 [details]
Patch
Comment 2 Said Abou-Hallawa 2019-10-01 11:47:37 PDT 
Created attachment 379921 [details]
Patch for review
Comment 3 Nikolas Zimmermann 2019-10-01 13:28:07 PDT 
Comment on attachment 379921 [details]
Patch for review

Nice, and much cleaner.
Comment 4 Nikolas Zimmermann 2019-10-01 13:29:16 PDT 
You missed at least one instance where SVGPropertyAccess was still used: SVGFitToViewBox.
Comment 5 Said Abou-Hallawa 2019-10-01 13:45:55 PDT 
(In reply to Nikolas Zimmermann from comment #4)
> You missed at least one instance where SVGPropertyAccess was still used:
> SVGFitToViewBox.

It is removed in https://bugs.webkit.org/show_bug.cgi?id=202411 which blocks this bug. The accumulated patch above includes this change. The "Patch for review" is the difference between the accumulated patch and the batch of the other bug. I post patches this way to be not blocked by the code review.
Comment 6 Nikolas Zimmermann 2019-10-01 14:15:13 PDT 
(In reply to Said Abou-Hallawa from comment #5)
> (In reply to Nikolas Zimmermann from comment #4)
> > You missed at least one instance where SVGPropertyAccess was still used:
> > SVGFitToViewBox.
> 
> It is removed in https://bugs.webkit.org/show_bug.cgi?id=202411 which blocks
> this bug. The accumulated patch above includes this change. The "Patch for
> review" is the difference between the accumulated patch and the batch of the
> other bug. I post patches this way to be not blocked by the code review.

Sorry, I missed that :-(
I wish we had a git based workflow, where we could upload a branch containing two commits that can be reviewed independently, but landed together :-)