SVG objects with same idrefs conflict when under different shadow root

SVG objects with same idrefs conflict when under different shadow root

When SVG idrefs are the same, even though the
LayoutSVGResourcesContainers are created under different shadow roots,
they conflict and only the last one is available. The problem is that,
currently, the HashMap mapping id's to LayoutSVGResourcesContainers are
owned/scoped to the document instead of the treeScope.
This CL moves that hash map from document to treeScope, so that
LayoutSVGResourcesContainers with the same id in different shadow roots
won't conflict.

Currently the following 2 maps (which are wrapped into the
SVGDocumentExtensions class) are owned by document instead of treeScope:

  HashMap<AtomicString, LayoutSVGResourceContainer*> m_resources;
  HeapHashMap<AtomicString, Member<SVGPendingElements>> m_pendingResources;

Thus when a new LayoutSVGResourcesContainer with the same id is created,
it is inserted into m_resources and overwrites the one with the same
key (id).

This CL separates these 2 maps from SVGDocumentExtensions and wrap them
into a new class (named SVGTreeScopeResources), and lets TreeScope have
them as a member variable (m_svgTreeScopedResources).
This CL also moves the corresponding methods accessing these 2 maps into
the new class.

To make this work together with <use>, we need to make sure to use the
TreeScope of the "original" element. Move the helper from LayoutSVGTextPath
to SVGElement::treeScopeForIdResolution and then use that for this.

Based on https://codereview.chromium.org/2107153002 by Taijin Tei.

BUG=597080
Review-Url: https://codereview.chromium.org/2633143002
Cr-Commit-Position: refs/heads/master@{#445378}
Committed: https://chromium.googlesource.com/chromium/src/+/8138e62d38b64d24e2220d50907b8aeb71efd32e

[fs, 2017-01-16 14:20:28]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-16 14:20:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-16 16:27:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-16 16:27:15]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[fs, 2017-01-17 16:02:19]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-17 16:02:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-17 18:26:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-17 18:26:44]

Dry run: This issue passed the CQ dry run.

[fs, 2017-01-18 11:46:06]

Description was changed from

==========
SVG objects with same idrefs conflict when under different shadow root

When SVG idrefs are the same, even though the
LayoutSVGResourcesContainers are created under different shadow roots,
they conflict and only the last one is available. The problem is that,
currently, the HashMap mapping id's to LayoutSVGResourcesContainers are
owned/scoped to the document instead of the treeScope.
This CL moves that hash map from document to treeScope, so that
LayoutSVGResourcesContainers with the same id in different shadow roots
won't conflict.

Currently the following 2 maps (which are wrapped into the
SVGDocumentExtensions class) are owned by document instead of treeScope:

  HashMap<AtomicString, LayoutSVGResourceContainer*> m_resources;
  HeapHashMap<AtomicString, Member<SVGPendingElements>> m_pendingResources;

Thus when a new LayoutSVGResourcesContainer with the same id is created,
it is inserted into m_resources and overwrites the one with the same
key (id).

This CL separates these 2 maps from SVGDocumentExtensions and wrap them
into a new class (named SVGTreeScopeResources), and let treeScope have
them as a member variable (m_svgTreeScopedResources).
This CL also moves the corresponding methods accessing these 2 maps into
the new class SVGTreeScopeResources.

Based on https://codereview.chromium.org/2107153002 by Taijin Tei.

BUG=597080
==========

to

==========
SVG objects with same idrefs conflict when under different shadow root

When SVG idrefs are the same, even though the
LayoutSVGResourcesContainers are created under different shadow roots,
they conflict and only the last one is available. The problem is that,
currently, the HashMap mapping id's to LayoutSVGResourcesContainers are
owned/scoped to the document instead of the treeScope.
This CL moves that hash map from document to treeScope, so that
LayoutSVGResourcesContainers with the same id in different shadow roots
won't conflict.

Currently the following 2 maps (which are wrapped into the
SVGDocumentExtensions class) are owned by document instead of treeScope:

  HashMap<AtomicString, LayoutSVGResourceContainer*> m_resources;
  HeapHashMap<AtomicString, Member<SVGPendingElements>> m_pendingResources;

Thus when a new LayoutSVGResourcesContainer with the same id is created,
it is inserted into m_resources and overwrites the one with the same
key (id).

This CL separates these 2 maps from SVGDocumentExtensions and wrap them
into a new class (named SVGTreeScopeResources), and let treeScope have
them as a member variable (m_svgTreeScopedResources).
This CL also moves the corresponding methods accessing these 2 maps into
the new class SVGTreeScopeResources.

To make this work together with <use>, we need to make sure to use the
TreeScope of the "original" element. Move the helper from LayoutSVGTextPath
to SVGElement::treeScopeForIdResolution and then use that for this.

Based on https://codereview.chromium.org/2107153002 by Taijin Tei.

BUG=597080
==========

[fs, 2017-01-18 11:48:42]

Description was changed from

==========
SVG objects with same idrefs conflict when under different shadow root

When SVG idrefs are the same, even though the
LayoutSVGResourcesContainers are created under different shadow roots,
they conflict and only the last one is available. The problem is that,
currently, the HashMap mapping id's to LayoutSVGResourcesContainers are
owned/scoped to the document instead of the treeScope.
This CL moves that hash map from document to treeScope, so that
LayoutSVGResourcesContainers with the same id in different shadow roots
won't conflict.

Currently the following 2 maps (which are wrapped into the
SVGDocumentExtensions class) are owned by document instead of treeScope:

  HashMap<AtomicString, LayoutSVGResourceContainer*> m_resources;
  HeapHashMap<AtomicString, Member<SVGPendingElements>> m_pendingResources;

Thus when a new LayoutSVGResourcesContainer with the same id is created,
it is inserted into m_resources and overwrites the one with the same
key (id).

This CL separates these 2 maps from SVGDocumentExtensions and wrap them
into a new class (named SVGTreeScopeResources), and let treeScope have
them as a member variable (m_svgTreeScopedResources).
This CL also moves the corresponding methods accessing these 2 maps into
the new class SVGTreeScopeResources.

To make this work together with <use>, we need to make sure to use the
TreeScope of the "original" element. Move the helper from LayoutSVGTextPath
to SVGElement::treeScopeForIdResolution and then use that for this.

Based on https://codereview.chromium.org/2107153002 by Taijin Tei.

BUG=597080
==========

to

==========
SVG objects with same idrefs conflict when under different shadow root

When SVG idrefs are the same, even though the
LayoutSVGResourcesContainers are created under different shadow roots,
they conflict and only the last one is available. The problem is that,
currently, the HashMap mapping id's to LayoutSVGResourcesContainers are
owned/scoped to the document instead of the treeScope.
This CL moves that hash map from document to treeScope, so that
LayoutSVGResourcesContainers with the same id in different shadow roots
won't conflict.

Currently the following 2 maps (which are wrapped into the
SVGDocumentExtensions class) are owned by document instead of treeScope:

  HashMap<AtomicString, LayoutSVGResourceContainer*> m_resources;
  HeapHashMap<AtomicString, Member<SVGPendingElements>> m_pendingResources;

Thus when a new LayoutSVGResourcesContainer with the same id is created,
it is inserted into m_resources and overwrites the one with the same
key (id).

This CL separates these 2 maps from SVGDocumentExtensions and wrap them
into a new class (named SVGTreeScopeResources), and lets TreeScope have
them as a member variable (m_svgTreeScopedResources).
This CL also moves the corresponding methods accessing these 2 maps into
the new class.

To make this work together with <use>, we need to make sure to use the
TreeScope of the "original" element. Move the helper from LayoutSVGTextPath
to SVGElement::treeScopeForIdResolution and then use that for this.

Based on https://codereview.chromium.org/2107153002 by Taijin Tei.

BUG=597080
==========

[fs, 2017-01-18 12:38:21]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-18 12:38:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-18 14:35:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-18 14:35:33]

Dry run: This issue passed the CQ dry run.

[fs, 2017-01-18 15:16:58]

fs@opera.com changed reviewers:
+ hayato@chromium.org, pdr@chromium.org

[fs, 2017-01-18 15:18:28]

This https://codereview.chromium.org/2107153002 respun on a later revision
(PS1), with test failures fixed (PS2) and some simple tests added (PS3).

Thanks to Taijin (CCd) for the initial work done here.

PTAL.

[hayato, 2017-01-19 07:06:58]

LGTM in terms of treeScope. I think you need a review from pdr@ because I am not
so familiar with SVG.

Thank you for writing the awesome description too.

[pdr., 2017-01-20 06:32:01]

I'm sorry for not getting to this today. I pushed it on the stack for tomorrow.

[pdr., 2017-01-23 04:34:12]

This is great, LGTM!

https://codereview.chromium.org/2633143002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/TreeScope.h (right):

https://codereview.chromium.org/2633143002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/TreeScope.h:132: SVGTreeScopeResources&
accessSVGTreeScopedResources();
optional: ensureSVGTreeScopedResources so it's more obvious that this does work?

I'd be okay with leaving this for a followup, or not doing it at all on the
basis that the current code uses access*. Grepping around the codebase, ensure*
just seems to be preferred.

https://codereview.chromium.org/2633143002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGTreeScopeResources.h (right):

https://codereview.chromium.org/2633143002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGTreeScopeResources.h:20: class
SVGTreeScopeResources
Can you add a very short comment here describing the class and why the resources
are on a treescope?

[fs, 2017-01-23 11:17:06]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 11:17:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fs, 2017-01-23 11:18:24]

https://codereview.chromium.org/2633143002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/TreeScope.h (right):

https://codereview.chromium.org/2633143002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/TreeScope.h:132: SVGTreeScopeResources&
accessSVGTreeScopedResources();
On 2017/01/23 at 04:34:12, pdr. wrote:
> optional: ensureSVGTreeScopedResources so it's more obvious that this does
work?
> 
> I'd be okay with leaving this for a followup, or not doing it at all on the
basis that the current code uses access*. Grepping around the codebase, ensure*
just seems to be preferred.

I don't see why not - we're replacing all instances anyway so... Done. Dropped
the svgExtensions() method too since it wasn't used.

https://codereview.chromium.org/2633143002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGTreeScopeResources.h (right):

https://codereview.chromium.org/2633143002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGTreeScopeResources.h:20: class
SVGTreeScopeResources
On 2017/01/23 at 04:34:12, pdr. wrote:
> Can you add a very short comment here describing the class and why the
resources are on a treescope?

Done.

[commit-bot: I haz the power, 2017-01-23 14:25:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-23 14:25:46]

Dry run: This issue passed the CQ dry run.

[fs, 2017-01-23 14:28:38]

The CQ bit was checked by fs@opera.com

[fs, 2017-01-23 14:28:38]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
hayato@chromium.org
Link to the patchset: https://codereview.chromium.org/2633143002/#ps60001
(title: "ensureSVGTreeScopedResources(); add comment")

[commit-bot: I haz the power, 2017-01-23 14:28:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 15:23:15]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485181718086200,
"parent_rev": "5465417d5b891727a5e07266336143a96e0e9f4c", "commit_rev":
"8138e62d38b64d24e2220d50907b8aeb71efd32e"}

[commit-bot: I haz the power, 2017-01-23 15:23:48]

Description was changed from

==========
SVG objects with same idrefs conflict when under different shadow root

When SVG idrefs are the same, even though the
LayoutSVGResourcesContainers are created under different shadow roots,
they conflict and only the last one is available. The problem is that,
currently, the HashMap mapping id's to LayoutSVGResourcesContainers are
owned/scoped to the document instead of the treeScope.
This CL moves that hash map from document to treeScope, so that
LayoutSVGResourcesContainers with the same id in different shadow roots
won't conflict.

Currently the following 2 maps (which are wrapped into the
SVGDocumentExtensions class) are owned by document instead of treeScope:

  HashMap<AtomicString, LayoutSVGResourceContainer*> m_resources;
  HeapHashMap<AtomicString, Member<SVGPendingElements>> m_pendingResources;

Thus when a new LayoutSVGResourcesContainer with the same id is created,
it is inserted into m_resources and overwrites the one with the same
key (id).

This CL separates these 2 maps from SVGDocumentExtensions and wrap them
into a new class (named SVGTreeScopeResources), and lets TreeScope have
them as a member variable (m_svgTreeScopedResources).
This CL also moves the corresponding methods accessing these 2 maps into
the new class.

To make this work together with <use>, we need to make sure to use the
TreeScope of the "original" element. Move the helper from LayoutSVGTextPath
to SVGElement::treeScopeForIdResolution and then use that for this.

Based on https://codereview.chromium.org/2107153002 by Taijin Tei.

BUG=597080
==========

to

==========
SVG objects with same idrefs conflict when under different shadow root

When SVG idrefs are the same, even though the
LayoutSVGResourcesContainers are created under different shadow roots,
they conflict and only the last one is available. The problem is that,
currently, the HashMap mapping id's to LayoutSVGResourcesContainers are
owned/scoped to the document instead of the treeScope.
This CL moves that hash map from document to treeScope, so that
LayoutSVGResourcesContainers with the same id in different shadow roots
won't conflict.

Currently the following 2 maps (which are wrapped into the
SVGDocumentExtensions class) are owned by document instead of treeScope:

  HashMap<AtomicString, LayoutSVGResourceContainer*> m_resources;
  HeapHashMap<AtomicString, Member<SVGPendingElements>> m_pendingResources;

Thus when a new LayoutSVGResourcesContainer with the same id is created,
it is inserted into m_resources and overwrites the one with the same
key (id).

This CL separates these 2 maps from SVGDocumentExtensions and wrap them
into a new class (named SVGTreeScopeResources), and lets TreeScope have
them as a member variable (m_svgTreeScopedResources).
This CL also moves the corresponding methods accessing these 2 maps into
the new class.

To make this work together with <use>, we need to make sure to use the
TreeScope of the "original" element. Move the helper from LayoutSVGTextPath
to SVGElement::treeScopeForIdResolution and then use that for this.

Based on https://codereview.chromium.org/2107153002 by Taijin Tei.

BUG=597080

Review-Url: https://codereview.chromium.org/2633143002
Cr-Commit-Position: refs/heads/master@{#445378}
Committed:
https://chromium.googlesource.com/chromium/src/+/8138e62d38b64d24e2220d50907b...
==========

[commit-bot: I haz the power, 2017-01-23 15:23:50]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/8138e62d38b64d24e2220d50907b...