SVG object with same idrefs get conflicted even they are under different shadow root

Overview :
When SVG idrefs is same, even though the LayoutSVGResourcesContainers are created under different shadow root,
they got conflicted and only the latest created one is reflected.
The problem is that, currently, the HashMap mapping LayoutSVGResourcesContainers to its id is owned by the document instead of treeScope.
This CL moves that hash map from document to treeScope, so that LayoutSVGResourcesContainers with same id in different shadow root won’t get conflict.

Detail :
Currently below 3 HashMap/HeapHashMap (which are wrapped into class SVGDocumentExtensions) are owned by document instead of treeScope.
(Member<SVGDocumentExtensions> m_svgExtensions)

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

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

Solution :
This CL separated these 3 HashMap/HeapHashMap from SVGDocumentExtensions and wrapped them into a new class(which is named SVGTreeScopeResources), and let treeScope have them as member variable(Member<SVGTreeScopeResources> m_svgTreeScopedResources).
Also this CL moved corresponding methods using these 3 HashMap/HeapHashMap into the new class SVGTreeScopeResources.

BUG=597080

[hayato, 2016-06-30 03:38:17]

hayato@chromium.org changed reviewers:
+ hayato@chromium.org

[hayato, 2016-06-30 03:38:18]

Thanks. Looks promising. I will take a look tomorrow.

[taijin, 2016-07-02 07:00:02]

Description was changed from

==========
Issue 597080
Refers to this issue :
https://bugs.chromium.org/p/chromium/issues/detail?id=597080
I separated these 3 HashMap/HeapHashMap from SVGDocumentExtensions,
wrapped them into a new class(which I named it SVGTreeScopeResources),
and let treeScope have them as member variable(Member<SVGTreeScopeResources>
m_svgTreeScopedResources).
Also I moved corresponding methods using these 3 HashMap/HeapHashMap into the
new class SVGTreeScopeResources.
This worked fine in my environment(mac os x version 10.11.2).

Added Taijin's name as the instructions says.


issue 597080


 Issue 597080


Issue 597080


ISSUE 597080

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

to

==========
Overview :
When SVG idrefs is same, even though the LayoutSVGResourcesContainers are
created under different shadow root,
they got conflicted and only the latest created one is reflected.
The problem is that, currently, the HashMap mapping LayoutSVGResourcesContainers
to its id is owned by the document instead of treeScope.
This CL moves that hash map from document to treeScope, so that
LayoutSVGResourcesContainers with same id in different shadow root won’t get
conflict.

Detail :
Currently below 3 HashMap/HeapHashMap (which are wrapped into class
SVGDocumentExtensions) are owned by document instead of treeScope.
(Member<SVGDocumentExtensions> m_svgExtensions)

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

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

Solution :
This CL separated these 3 HashMap/HeapHashMap from SVGDocumentExtensions and
wrapped them into a new class(which is named SVGTreeScopeResources), and let
treeScope have them as member variable(Member<SVGTreeScopeResources>
m_svgTreeScopedResources).
Also this CL moved corresponding methods using these 3 HashMap/HeapHashMap into
the new class SVGTreeScopeResources.

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

[hayato, 2016-07-04 04:27:29]

https://codereview.chromium.org/2107153002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/TreeScope.cpp (right):

https://codereview.chromium.org/2107153002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/TreeScope.cpp:496: const
SVGTreeScopeResources* TreeScope::svgTreeScopedResources()
Could we rename this to TreeScope::svgExtensions()?

"TreeScope" in a function name looks redundant to me.

[taijin, 2016-07-05 06:37:14]

https://codereview.chromium.org/2107153002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/TreeScope.cpp (right):

https://codereview.chromium.org/2107153002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/TreeScope.cpp:496: const
SVGTreeScopeResources* TreeScope::svgTreeScopedResources()
On 2016/07/04 04:27:29, hayato wrote:
> Could we rename this to TreeScope::svgExtensions()?
> 
> "TreeScope" in a function name looks redundant to me.

Sure, thanks, will do it:)

[taijin, 2016-07-05 15:43:57]

On 2016/07/04 04:27:29, hayato wrote:
>
https://codereview.chromium.org/2107153002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/dom/TreeScope.cpp (right):
> 
>
https://codereview.chromium.org/2107153002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/dom/TreeScope.cpp:496: const
> SVGTreeScopeResources* TreeScope::svgTreeScopedResources()
> Could we rename this to TreeScope::svgExtensions()?
> 
> "TreeScope" in a function name looks redundant to me.

Renamed the method and also added below lines in the constructure of
SVGTreeScopeResources.

// Whenever an object of SVGTreeScopeResources is created, to keep the code
behave as before,
+    // the document should also have an instance of SVGDocumentExtensions
created.
+    // Thus below lines are added
+    if (!treeScope->document().svgExtensions())
+        treeScope->document().accessSVGExtensions();

[hayato, 2016-07-07 08:29:21]

Thank you. I will take another look later.

[hayato, 2016-07-12 00:56:57]

Could you add a test?
I am not familiar with how *resources* are used in SVG world.
You might want to add a reviewer who is familiar with svg.

[taijin, 2016-07-12 08:46:12]

On 2016/07/12 00:56:57, hayato wrote:
> Could you add a test?
> I am not familiar with how *resources* are used in SVG world.
> You might want to add a reviewer who is familiar with svg.

Sure will do both. Thanks:)

[fs, 2016-10-12 11:08:15]

On 2016/07/12 at 08:46:12, taijin.passion wrote:
> On 2016/07/12 00:56:57, hayato wrote:
> > Could you add a test?
> > I am not familiar with how *resources* are used in SVG world.
> > You might want to add a reviewer who is familiar with svg.
> 
> Sure will do both. Thanks:)

Any chance to see movement on this? From an SVG PoV I think what you need to
worry about is references from within <use> instances. Picking the right
TreeScope within SVGResources::buildResources should hopefully enough to handle
that (for now.) I hope there are at least some tests covering that.

[fs, 2017-01-16 15:31:21]

On 2016/10/12 at 11:08:15, fs wrote:
> On 2016/07/12 at 08:46:12, taijin.passion wrote:
> > On 2016/07/12 00:56:57, hayato wrote:
> > > Could you add a test?
> > > I am not familiar with how *resources* are used in SVG world.
> > > You might want to add a reviewer who is familiar with svg.
> > 
> > Sure will do both. Thanks:)
> 
> Any chance to see movement on this? From an SVG PoV I think what you need to
worry about is references from within <use> instances. Picking the right
TreeScope within SVGResources::buildResources should hopefully enough to handle
that (for now.) I hope there are at least some tests covering that.

I rebased and updated this to current master and uploaded it to
https://codereview.chromium.org/2633143002 . Trybots seem to indicate that there
indeed is problem with resource references from within <use> instances.