Don't expand <symbol> elements in <use> that are not targets

Don't expand <symbol> elements in <use> that are not targets

When a <use> was referencing a subtree which contained a <symbol>
element, the <symbol> would get replaced by the "replace <symbol>" part
of the <use> expansion. This would result in content being rendered that
should not (since only <symbol>s that are directly referenced by <use>
should render.)
Instead of blindly expanding <symbol> elements, replace them directly
when producing the instance clone.

Replace the old faulty test svg/custom/use-on-g-containing-symbol.svg
with a new test.

Test from: https://bugs.webkit.org/show_bug.cgi?id=154576

BUG=589682
Committed: https://crrev.com/908f01e663ef0073b3ef667258432e78add83a7b
Cr-Commit-Position: refs/heads/master@{#379532}

[fs, 2016-02-26 17:58:22]

Description was changed from

==========
Remove <symbol> elements from cloned <use> shadow trees

When a <use> was referencing a subtree which contained a <symbol>
element, the <symbol> would get replaced by the "replace <symbol>" part
of the <use> expansion. This would result in content being rendered that
should not (since only <symbol>s that are directly referenced by <use>
should render.)
Add an additional clean-up pass that removes any <symbol> elements
before any further expansion take place, because any <symbol> in the
clone (except for the root) should never be rendered.

Replace the old faulty test svg/custom/use-on-g-containing-symbol.svg
with a new test.

BUG=589682
==========

to

==========
Remove <symbol> elements from cloned <use> shadow trees

When a <use> was referencing a subtree which contained a <symbol>
element, the <symbol> would get replaced by the "replace <symbol>" part
of the <use> expansion. This would result in content being rendered that
should not (since only <symbol>s that are directly referenced by <use>
should render.)
Add an additional clean-up pass that removes any <symbol> elements
before any further expansion take place, because any <symbol> in the
clone (except for the root) should never be rendered.

Replace the old faulty test svg/custom/use-on-g-containing-symbol.svg
with a new test.

Somewhat inspired by patch in:
  https://bugs.webkit.org/show_bug.cgi?id=154576
  by Said Abou-Hallawa

BUG=589682
==========

[fs, 2016-02-26 17:58:58]

Description was changed from

==========
Remove <symbol> elements from cloned <use> shadow trees

When a <use> was referencing a subtree which contained a <symbol>
element, the <symbol> would get replaced by the "replace <symbol>" part
of the <use> expansion. This would result in content being rendered that
should not (since only <symbol>s that are directly referenced by <use>
should render.)
Add an additional clean-up pass that removes any <symbol> elements
before any further expansion take place, because any <symbol> in the
clone (except for the root) should never be rendered.

Replace the old faulty test svg/custom/use-on-g-containing-symbol.svg
with a new test.

Somewhat inspired by patch in:
  https://bugs.webkit.org/show_bug.cgi?id=154576
  by Said Abou-Hallawa

BUG=589682
==========

to

==========
Remove <symbol> elements from cloned <use> shadow trees

When a <use> was referencing a subtree which contained a <symbol>
element, the <symbol> would get replaced by the "replace <symbol>" part
of the <use> expansion. This would result in content being rendered that
should not (since only <symbol>s that are directly referenced by <use>
should render.)
Add an additional clean-up pass that removes any <symbol> elements
before any further expansion take place, because any <symbol> in the
clone (except for the root) should never be rendered.

Replace the old faulty test svg/custom/use-on-g-containing-symbol.svg
with a new test.

Somewhat inspired by patch in:
  https://bugs.webkit.org/show_bug.cgi?id=154576
  by Said Abou-Hallawa <sabouhallawa@apple.com>

BUG=589682
==========

[fs, 2016-02-26 18:00:06]

Description was changed from

==========
Remove <symbol> elements from cloned <use> shadow trees

When a <use> was referencing a subtree which contained a <symbol>
element, the <symbol> would get replaced by the "replace <symbol>" part
of the <use> expansion. This would result in content being rendered that
should not (since only <symbol>s that are directly referenced by <use>
should render.)
Add an additional clean-up pass that removes any <symbol> elements
before any further expansion take place, because any <symbol> in the
clone (except for the root) should never be rendered.

Replace the old faulty test svg/custom/use-on-g-containing-symbol.svg
with a new test.

Somewhat inspired by patch in:
  https://bugs.webkit.org/show_bug.cgi?id=154576
  by Said Abou-Hallawa <sabouhallawa@apple.com>

BUG=589682
==========

to

==========
Remove <symbol> elements from cloned <use> shadow trees

When a <use> was referencing a subtree which contained a <symbol>
element, the <symbol> would get replaced by the "replace <symbol>" part
of the <use> expansion. This would result in content being rendered that
should not (since only <symbol>s that are directly referenced by <use>
should render.)
Add an additional clean-up pass that removes any <symbol> elements
before any further expansion take place, because any <symbol> in the
clone (except for the root) should never be rendered.

Rearrange code in buildShadowAndInstanceTree() a bit, so that the cloned
tree is attached to the shadow root after it has been cleaned per above.

Replace the old faulty test svg/custom/use-on-g-containing-symbol.svg
with a new test.

Somewhat inspired by patch in:
  https://bugs.webkit.org/show_bug.cgi?id=154576
  by Said Abou-Hallawa <sabouhallawa@apple.com>

BUG=589682
==========

[fs, 2016-02-26 19:15:57]

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

[pdr., 2016-02-29 23:45:40]

Sorry about taking a while to get to this review.


I wonder if it would be possible to rework the disallowed element logic to
include symbol? There's some shared logic between the two concepts (but also
differences).

https://codereview.chromium.org/1736283003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/SVGUseElement.cpp (right):

https://codereview.chromium.org/1736283003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGUseElement.cpp:314: // The subtree is not
in document so this won't generate events that could mutate the tree.
Can you assert this here and/or above--that the subtree is not in the document?

https://codereview.chromium.org/1736283003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGUseElement.cpp:435:
removeSymbolElementsFromSubtree(toSVGElement(*newChild));
expandSymbolElementsInShadowTree is called below and does another full subtree
walk--is that necessary now or can we just check expand the target if it is a
symbol?

https://codereview.chromium.org/1736283003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGUseElement.cpp:648: // Remove any <symbol>
elements from the clone, because if not,
I like how webkit wraps this complexity into a single location (cloneTarget)
instead of having to duplicate it twice. Do you think we could make a similar
change by extracting out this common functionality
(removeSymbolElementsFromSubtree, transferUseWidthAndHeightIfNeeded,
removeDisallowedElementsFromSubtree, etc)?

[fs, 2016-03-01 09:50:07]

On 2016/02/29 at 23:45:40, pdr wrote:
> Sorry about taking a while to get to this review.
> 
> 
> I wonder if it would be possible to rework the disallowed element logic to
include symbol? There's some shared logic between the two concepts (but also
differences).

I had planned to work on cleaning this code up a bit yesterday, but got
embroidered into other things... new attempt today maybe.

> 
>
https://codereview.chromium.org/1736283003/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/svg/SVGUseElement.cpp (right):
> 
>
https://codereview.chromium.org/1736283003/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/svg/SVGUseElement.cpp:314: // The subtree is
not in document so this won't generate events that could mutate the tree.
> Can you assert this here and/or above--that the subtree is not in the
document?

Not even sure this restriction is required (any more?)

>
https://codereview.chromium.org/1736283003/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/svg/SVGUseElement.cpp:435:
removeSymbolElementsFromSubtree(toSVGElement(*newChild));
> expandSymbolElementsInShadowTree is called below and does another full subtree
walk--is that necessary now or can we just check expand the target if it is a
symbol?
> 
>
https://codereview.chromium.org/1736283003/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/svg/SVGUseElement.cpp:648: // Remove any
<symbol> elements from the clone, because if not,
> I like how webkit wraps this complexity into a single location (cloneTarget)
instead of having to duplicate it twice. Do you think we could make a similar
change by extracting out this common functionality
(removeSymbolElementsFromSubtree, transferUseWidthAndHeightIfNeeded,
removeDisallowedElementsFromSubtree, etc)?

Yes, I think we could clean this up quite significantly...

[fs, 2016-03-04 19:29:01]

Description was changed from

==========
Remove <symbol> elements from cloned <use> shadow trees

When a <use> was referencing a subtree which contained a <symbol>
element, the <symbol> would get replaced by the "replace <symbol>" part
of the <use> expansion. This would result in content being rendered that
should not (since only <symbol>s that are directly referenced by <use>
should render.)
Add an additional clean-up pass that removes any <symbol> elements
before any further expansion take place, because any <symbol> in the
clone (except for the root) should never be rendered.

Rearrange code in buildShadowAndInstanceTree() a bit, so that the cloned
tree is attached to the shadow root after it has been cleaned per above.

Replace the old faulty test svg/custom/use-on-g-containing-symbol.svg
with a new test.

Somewhat inspired by patch in:
  https://bugs.webkit.org/show_bug.cgi?id=154576
  by Said Abou-Hallawa <sabouhallawa@apple.com>

BUG=589682
==========

to

==========
Don't expand <symbol> elements in <use> that are not targets

When a <use> was referencing a subtree which contained a <symbol>
element, the <symbol> would get replaced by the "replace <symbol>" part
of the <use> expansion. This would result in content being rendered that
should not (since only <symbol>s that are directly referenced by <use>
should render.)
Instead of blindly expanding <symbol> elements, replace them directly
when producing the instance clone.

Replace the old faulty test svg/custom/use-on-g-containing-symbol.svg
with a new test.

Test from: https://bugs.webkit.org/show_bug.cgi?id=154576

BUG=589682
==========

[fs, 2016-03-04 19:29:01]

fs@opera.com changed reviewers:
+ schenney@chromium.org

[fs, 2016-03-04 20:12:39]

On 2016/03/01 at 09:50:07, fs wrote:
> On 2016/02/29 at 23:45:40, pdr wrote:
> > Sorry about taking a while to get to this review.
> > 
> > 
> > I wonder if it would be possible to rework the disallowed element logic to
include symbol? There's some shared logic between the two concepts (but also
differences).

So after a quick(?) interlude (and some quick reviews by schenney) we're back
here doing just that. Feast your eyes.

[pdr., 2016-03-04 22:15:01]

On 2016/03/04 at 20:12:39, fs wrote:
> On 2016/03/01 at 09:50:07, fs wrote:
> > On 2016/02/29 at 23:45:40, pdr wrote:
> > > Sorry about taking a while to get to this review.
> > > 
> > > 
> > > I wonder if it would be possible to rework the disallowed element logic to
include symbol? There's some shared logic between the two concepts (but also
differences).
> 
> So after a quick(?) interlude (and some quick reviews by schenney) we're back
here doing just that. Feast your eyes.

Amazing! This is the most delicious patch my eyes have ever tasted.

LGTM!!

[fs, 2016-03-07 08:40:29]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2016-03-07 08:40:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1736283003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1736283003/40001

[commit-bot: I haz the power, 2016-03-07 10:22:07]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-03-07 10:23:12]

Description was changed from

==========
Don't expand <symbol> elements in <use> that are not targets

When a <use> was referencing a subtree which contained a <symbol>
element, the <symbol> would get replaced by the "replace <symbol>" part
of the <use> expansion. This would result in content being rendered that
should not (since only <symbol>s that are directly referenced by <use>
should render.)
Instead of blindly expanding <symbol> elements, replace them directly
when producing the instance clone.

Replace the old faulty test svg/custom/use-on-g-containing-symbol.svg
with a new test.

Test from: https://bugs.webkit.org/show_bug.cgi?id=154576

BUG=589682
==========

to

==========
Don't expand <symbol> elements in <use> that are not targets

When a <use> was referencing a subtree which contained a <symbol>
element, the <symbol> would get replaced by the "replace <symbol>" part
of the <use> expansion. This would result in content being rendered that
should not (since only <symbol>s that are directly referenced by <use>
should render.)
Instead of blindly expanding <symbol> elements, replace them directly
when producing the instance clone.

Replace the old faulty test svg/custom/use-on-g-containing-symbol.svg
with a new test.

Test from: https://bugs.webkit.org/show_bug.cgi?id=154576

BUG=589682

Committed: https://crrev.com/908f01e663ef0073b3ef667258432e78add83a7b
Cr-Commit-Position: refs/heads/master@{#379532}
==========

[commit-bot: I haz the power, 2016-03-07 10:23:13]

Patchset 3 (id:??) landed as
https://crrev.com/908f01e663ef0073b3ef667258432e78add83a7b
Cr-Commit-Position: refs/heads/master@{#379532}