Bug 178248 – SVGPathElement should cache the built-up Path of its non animating pathByteStream()

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Said Abou-Hallawa]

Instead of creating a Path object from the non animating pathByteStream() every time we need to updatePathFromPathElement(), the Path object can be cached once it is created and used for later calls.

[Radar WebKit Bug Importer]

<rdar://problem/34968657>

[Said Abou-Hallawa]

Created attachment 323753 [details]
Patch

[Build Bot]

Comment on attachment 323753 [details]
Patch

Attachment 323753 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4851900

New failing tests:
svg/W3C-SVG-1.1-SE/paths-dom-02-f.svg
css3/masking/clip-path-reference-restore.html
svg/dom/SVGPathSegList-segment-modification.svg
svg/carto.net/tabgroup.svg
svg/dom/SVGPathSegList-xml-dom-synchronization2.xhtml
svg/repaint/svgsvgelement-repaint-children.html

[Build Bot]

Created attachment 323775 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Said Abou-Hallawa]

Created attachment 323776 [details]
Patch

[Build Bot]

Comment on attachment 323776 [details]
Patch

Attachment 323776 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4852744

New failing tests:
svg/W3C-SVG-1.1-SE/paths-dom-02-f.svg
css3/masking/clip-path-reference-restore.html
svg/dom/SVGPathSegList-segment-modification.svg
svg/carto.net/tabgroup.svg
svg/dom/SVGPathSegList-xml-dom-synchronization2.xhtml
svg/repaint/svgsvgelement-repaint-children.html

[Build Bot]

Created attachment 323784 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 323776 [details]
Patch

Attachment 323776 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4852836

New failing tests:
svg/W3C-SVG-1.1-SE/paths-dom-02-f.svg
css3/masking/clip-path-reference-restore.html
svg/dom/SVGPathSegList-segment-modification.svg
svg/carto.net/tabgroup.svg
svg/dom/SVGPathSegList-xml-dom-synchronization2.xhtml
svg/repaint/svgsvgelement-repaint-children.html

[Build Bot]

Created attachment 323787 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 323776 [details]
Patch

Attachment 323776 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4852943

New failing tests:
svg/W3C-SVG-1.1-SE/paths-dom-02-f.svg
css3/masking/clip-path-reference-restore.html
svg/dom/SVGPathSegList-segment-modification.svg
svg/carto.net/tabgroup.svg
svg/dom/SVGPathSegList-xml-dom-synchronization2.xhtml
svg/repaint/svgsvgelement-repaint-children.html

[Build Bot]

Created attachment 323790 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 323776 [details]
Patch

Attachment 323776 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4853127

New failing tests:
imported/blink/svg/custom/viewport-resource-inval.svg
css3/masking/clip-path-reference-restore.html
svg/W3C-SVG-1.1-SE/paths-dom-02-f.svg

[Build Bot]

Created attachment 323792 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Said Abou-Hallawa]

Created attachment 323835 [details]
Patch

[Build Bot]

Comment on attachment 323835 [details]
Patch

Attachment 323835 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4860763

New failing tests:
css3/masking/clip-path-reference-restore.html

[Build Bot]

Created attachment 323836 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 323835 [details]
Patch

Attachment 323835 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4860770

New failing tests:
css3/masking/clip-path-reference-restore.html

[Build Bot]

Created attachment 323837 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 323835 [details]
Patch

Attachment 323835 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4860788

New failing tests:
css3/masking/clip-path-reference-restore.html

[Build Bot]

Created attachment 323838 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 323835 [details]
Patch

Attachment 323835 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4860794

New failing tests:
imported/blink/svg/custom/viewport-resource-inval.svg
css3/masking/clip-path-reference-restore.html

[Build Bot]

Created attachment 323839 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Said Abou-Hallawa]

Created attachment 323875 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 323875 [details]
Patch

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

> Source/WebCore/platform/graphics/cairo/PathCairo.cpp:68
> +    m_path = other.m_path;
> +    other.m_path = nullptr;

Can this just be m_path = WTFMove(other.m_path)?

> Source/WebCore/platform/graphics/cairo/PathCairo.cpp:106
> +    m_path = other.m_path;
> +    other.m_path = nullptr;

Can this just be m_path = WTFMove(other.m_path)?

> Source/WebCore/platform/graphics/cg/PathCG.cpp:119
> +    m_path = other.m_path;
> +    other.m_path = nullptr;

Can this just be m_path = WTFMove(other.m_path)?

> Source/WebCore/platform/graphics/cg/PathCG.cpp:139
> +    m_path = other.m_path;
> +    other.m_path = nullptr;

Can this just be m_path = WTFMove(other.m_path)?

> Source/WebCore/platform/graphics/win/PathDirect2D.cpp:173
> +    m_path = other.m_path;
> +    m_activePath = other.m_activePath;
> +    m_activePathGeometry = other.m_activePathGeometry;
> +    other.m_path = nullptr;
> +    other.m_activePath = nullptr;
> +    other.m_activePathGeometry = nullptr;

Can these WTFMove?

> Source/WebCore/platform/graphics/win/PathDirect2D.cpp:190
> +    m_path = other.m_path;
> +    m_activePath = other.m_activePath;
> +    m_activePathGeometry = other.m_activePathGeometry;

Can these WTFMove?

> Source/WebCore/rendering/svg/SVGPathData.cpp:49
> +        return path;

Maybe return { } and move the declaration of path lower down.

> Source/WebCore/rendering/svg/SVGPathData.cpp:66
> +        return path;

Maybe return { } and move the declaration of path lower down.

> Source/WebCore/rendering/svg/SVGPathData.cpp:119
> +        return path;

Maybe return { } and move the declaration of path lower down.

> Source/WebCore/rendering/svg/SVGPathData.cpp:134
> +        return path;

Maybe return { } and move the declaration of path lower down.

[Said Abou-Hallawa]

Created attachment 324478 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 323875 [details]
Patch

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

>> Source/WebCore/platform/graphics/cg/PathCG.cpp:119
>> +    other.m_path = nullptr;
> 
> Can this just be m_path = WTFMove(other.m_path)?

A raw pointer does not necessarily represent an ownership relationship. So WTFMove() or std::move does not set the raw pointer to nullptr after moving it. m_path = WTFMove(other.m_path) can only work if PlatformPathPtr is a std::unique_ptr<> or a RefPtr<>.

>> Source/WebCore/rendering/svg/SVGPathData.cpp:49
>> +        return path;
> 
> Maybe return { } and move the declaration of path lower down.

Done.

[WebKit Commit Bot]

Comment on attachment 324478 [details]
Patch

Clearing flags on attachment: 324478

Committed r223804: <https://trac.webkit.org/changeset/223804>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 323875 [details]
Patch

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

>>> Source/WebCore/platform/graphics/cg/PathCG.cpp:119
>>> +    other.m_path = nullptr;
>> 
>> Can this just be m_path = WTFMove(other.m_path)?
> 
> A raw pointer does not necessarily represent an ownership relationship. So WTFMove() or std::move does not set the raw pointer to nullptr after moving it. m_path = WTFMove(other.m_path) can only work if PlatformPathPtr is a std::unique_ptr<> or a RefPtr<>.

For raw pointers, a good idiom is:

    m_path = std::exchange(other.m_path, nullptr);