Bug 195863

Summary: Remove the SVG property tear off objects for SVGStringList
Product: WebKit Reporter: Said Abou-Hallawa <sabouhallawa>
Component: SVGAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dino, mcatanzaro, rniwa, simon.fraser, thorton, webkit-bug-importer, zimmermann
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 195862    
Bug Blocks: 191237, 195949    
Attachments:
Description Flags
Patch
none
Patch for review
none
Patch
none
Patch
none
Patch for review
none
Patch none

Description Said Abou-Hallawa 2019-03-17 12:57:38 PDT
Currently some of SVGElements store SVGStringListValues which is an array of Strings. When the DOM requests an interface of SVGStringList a tear off object of type SVGStaticListPropertyTearOff is created to encapsulate SVGStringListValues. Like what we did for SVGAnimatedInteger and SVGAnimatedBoolean, we are going to store Ref<SVGStringList> in the SVGElement.
Comment 1 Said Abou-Hallawa 2019-03-17 12:59:26 PDT
Created attachment 364972 [details]
Patch
Comment 2 Said Abou-Hallawa 2019-03-17 13:03:20 PDT
Created attachment 364973 [details]
Patch for review
Comment 3 Said Abou-Hallawa 2019-03-17 13:09:52 PDT
Created attachment 364974 [details]
Patch
Comment 4 Said Abou-Hallawa 2019-03-17 17:48:38 PDT
Created attachment 364989 [details]
Patch
Comment 5 Said Abou-Hallawa 2019-03-17 18:10:51 PDT
Created attachment 364990 [details]
Patch for review
Comment 6 Simon Fraser (smfr) 2019-03-18 13:46:13 PDT
Comment on attachment 364990 [details]
Patch for review

View in context: https://bugs.webkit.org/attachment.cgi?id=364990&action=review

> Source/WebCore/ChangeLog:12
> +           attribute name in SVGPropertyRegistery. It will also commits changes

it will also commit

> Source/WebCore/svg/SVGElement.h:160
> +    void commitPropertyChange(SVGProperty*) override;

Can SVGProperty be a reference?

> Source/WebCore/svg/SVGStringList.h:39
> +    static Ref<SVGStringList> create(SVGPropertyOwner* owner)

Can SVGPropertyOwner be a reference?

> Source/WebCore/svg/SVGStringList.h:59
> +        const UChar* end = ptr + data.length();

Are we sure that upconvertedCharacters() returns the same number of characters as data contains?

> Source/WebCore/svg/SVGViewElement.h:58
> +    Ref<SVGStringList> m_viewTarget { SVGStringList::create(this) };

Please initialize in the constructor.

> Source/WebCore/svg/properties/SVGList.h:2
> + * Copyright (C) 2018-2019 Apple Inc.  All rights reserved.

Just 2019

> Source/WebCore/svg/properties/SVGPrimitiveList.h:2
> + * Copyright (C) 2018-2019 Apple Inc.  All rights reserved.

2019

> Source/WebCore/svg/properties/SVGProperty.h:37
> +    void attach(SVGPropertyOwner* owner, SVGPropertyAccess access)

Can SVGPropertyOwner be a reference?
Comment 7 Said Abou-Hallawa 2019-03-18 18:31:43 PDT
Created attachment 365099 [details]
Patch
Comment 8 WebKit Commit Bot 2019-03-18 21:23:33 PDT
Comment on attachment 365099 [details]
Patch

Clearing flags on attachment: 365099

Committed r243130: <https://trac.webkit.org/changeset/243130>
Comment 9 WebKit Commit Bot 2019-03-18 21:23:35 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2019-03-18 21:24:17 PDT
<rdar://problem/49006248>
Comment 11 Michael Catanzaro 2019-03-19 06:46:30 PDT
Committed r243137: <https://trac.webkit.org/changeset/243137>