Bug 202415

Summary: Remove SVGPropertyAccess from SVGProperty
Product: WebKit Reporter: Said Abou-Hallawa <sabouhallawa>
Component: SVGAssignee: Said Abou-Hallawa <sabouhallawa>
Status: NEW ---    
Severity: Normal CC: dino, ews-watchlist, fmalita, gyuyoung.kim, pdr, schenney, sergio, zimmermann
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 202411    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch for review sabouhallawa: review?

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 :-)