Issue 252903004: Avoid using SVGElementInstance in calcViewport

Issue 252903004: Avoid using SVGElementInstance in calcViewport (Closed)

Created:
6 years, 7 months ago by rwlbuis

Modified:
6 years, 7 months ago

Reviewers:
pdr., Erik Dahlström (inactive)

CC:
blink-reviews, chrishtr, krit, bemjb+rendering_chromium.org, dsinclair, zoltan1, eae+blinkwatch, leviw+renderwatch, kouhei+svg_chromium.org, ed+blinkwatch_opera.com, f(malita), gyuyoung.kim_webkit.org, jchaffraix+rendering, ojan, Stephen Chennney, rune+blink

Base URL:
https://chromium.googlesource.com/chromium/blink.git@master

Visibility:
Public.

More Reviews

Description

Avoid using SVGElementInstance in calcViewport Avoid using SVGElementInstance in calcViewport as it is scheduled to be removed. Instead implement the width/height transfer rules in SVGUseElement. This speeds up the SierpinskiCarpet test a lot as profiling showed the instance traversal in calcViewport was very high. BUG=313438 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=173258

Patch Set 1 #

Patch Set 2 : Cleaner patch #

Total comments: 4

Patch Set 3 : Add tests and change patch to pass them #

Patch Set 4 : Unsquash #

Patch Set 5 : Use isSpecified #

Total comments: 2

Patch Set 6 : Add animation test #

Patch Set 7 : Also test shadow tree <svg> #

Patch Set 8 : Use attributeType=XML #

Patch Set 9 : Add NeedsRebaseline magic #

Total comments: 2

Patch Set 10 : Fix nits and rebase #

Patch Set 11 : Rebase one more time #

Created: 6 years, 7 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+290 lines, -49 lines)			Patch
M	LayoutTests/TestExpectations	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+2 lines, -0 lines	0 comments	Download
M	LayoutTests/platform/linux/svg/custom/relative-sized-shadow-tree-content-with-symbol-expected.png	View	1 2 3 4 5 6 7	Binary file		0 comments	Download
A	LayoutTests/svg/animations/script-tests/use-animate-width-and-height.js	View	1 2 3 4 5 6 7	1 chunk	+122 lines, -0 lines	0 comments	Download
A +	LayoutTests/svg/animations/use-animate-width-and-height.html	View	1 2 3 4 5	1 chunk	+1 line, -1 line	0 comments	Download
A	LayoutTests/svg/animations/use-animate-width-and-height-expected.txt	View	1 2 3 4 5 6	1 chunk	+51 lines, -0 lines	0 comments	Download
A	LayoutTests/svg/custom/use-attribute-invalidations.html	View	1 2	1 chunk	+27 lines, -0 lines	0 comments	Download
A	LayoutTests/svg/custom/use-attribute-invalidations-expected.html	View	1 2	1 chunk	+17 lines, -0 lines	0 comments	Download
A	LayoutTests/svg/custom/use-dynamic-attribute-setting.html	View	1 2	1 chunk	+24 lines, -0 lines	0 comments	Download
A	LayoutTests/svg/custom/use-dynamic-attribute-setting-expected.html	View	1 2	1 chunk	+14 lines, -0 lines	0 comments	Download
M	Source/core/rendering/svg/RenderSVGViewportContainer.cpp	View	1 2	1 chunk	+0 lines, -48 lines	0 comments	Download
M	Source/core/svg/SVGUseElement.cpp	View	1 2 3 4 5 6 7 8 9	4 chunks	+32 lines, -0 lines	0 comments	Download

Messages

Total messages: 35 (0 generated)

Expand Messages | Collapse Messages

rwlbuis

On 2014/04/28 19:54:14, rwlbuis wrote: > PTAL Hmm, closed by accident?

6 years, 7 months ago (2014-04-28 23:18:24 UTC) #2

pdr.

I like where this patch is going! Before your patch it looks like we didn't ...

6 years, 7 months ago (2014-04-29 04:30:58 UTC) #3

I like where this patch is going!

Before your patch it looks like we didn't handle the <use> case at all. Could
you add a test such as the following?
<!DOCTYPE HTML>
<html>
<body>
This test passes if there are two green squares below:<br>
<svg width="300" height="300">
  <defs>
    <svg id="svg" width="10" height="10">
      <rect width="100%" height="100%" fill="green"/>
    </svg>
  </defs>
  <use id="use1" xlink:href="#svg" x="0" y="0" width="100" height="100"/>
  <use id="use2" xlink:href="#svg" x="100" y="100" width="10" height="10"/>
</svg>
<script>
  onload = function() {
    // Wait until first layout is finished.
    window.requestAnimationFrame(function() {
      document.getElementById('use2').setAttribute('width', '100');
      document.getElementById('use2').setAttribute('height', '100');
    });
  }
</script>
</body>
</html>


I'm also worried about invalidations. Today they go through the instance tree
but I think that logic will eventually be in SVGElement so the test will come in
handy in the future. Could you add a test such as the following?
<!DOCTYPE HTML>
<html>
<body>
This test passes if there are three green squares below:<br>
<svg width="300" height="300">
  <defs>
    <symbol id="symbol" width="100">
      <rect width="100%" height="100%" fill="green"/>
    </symbol>
  </defs>
  <svg x="0" y="0" width="100" height="100">
    <use id="use1" xlink:href="#symbol"/>
  </svg>
  <use id="use2" xlink:href="#symbol" x="100" y="100" width="10" height="100"/>
  <use id="use3" xlink:href="#symbol" x="200" y="200" width="100" height="10"/>
</svg>
<script>
  onload = function() {
    // Wait until first layout is finished.
    window.requestAnimationFrame(function() {
      document.getElementById('use2').removeAttribute('width');
      document.getElementById('use3').setAttribute('height', '100');
    });
  }
</script>
</body>
</html>

https://codereview.chromium.org/252903004/diff/20001/Source/core/svg/SVGUseEl...
File Source/core/svg/SVGUseElement.cpp (right):

https://codereview.chromium.org/252903004/diff/20001/Source/core/svg/SVGUseEl...
Source/core/svg/SVGUseElement.cpp:403: inline void
transferUseWidthAndHeightIfNeeded(const SVGUseElement& use, Element& element)
It may be better to put the relevant code from this function into
expandSymbolElementsInShadowTree and expandUseElementsInShadowTree instead of
breaking it into a function.

https://codereview.chromium.org/252903004/diff/20001/Source/core/svg/SVGUseEl...
Source/core/svg/SVGUseElement.cpp:725: transferUseWidthAndHeightIfNeeded(*use,
*newChild);
I think the use width/height code would go here.

https://codereview.chromium.org/252903004/diff/20001/Source/core/svg/SVGUseEl...
Source/core/svg/SVGUseElement.cpp:769:
svgElement->cloneDataFromElement(*toElement(element));
I think the symbol-specific width/height code would go here.

Erik Dahlström (inactive)

https://codereview.chromium.org/252903004/diff/20001/Source/core/svg/SVGUseElement.cpp File Source/core/svg/SVGUseElement.cpp (right): https://codereview.chromium.org/252903004/diff/20001/Source/core/svg/SVGUseElement.cpp#newcode415 Source/core/svg/SVGUseElement.cpp:415: if (use.hasAttribute(SVGNames::widthAttr)) Will this work if the width/height attributes ...

6 years, 7 months ago (2014-04-29 13:01:01 UTC) #4

rwlbuis

On 2014/04/29 13:01:01, Erik Dahlström wrote: > https://codereview.chromium.org/252903004/diff/20001/Source/core/svg/SVGUseElement.cpp > File Source/core/svg/SVGUseElement.cpp (right): > > https://codereview.chromium.org/252903004/diff/20001/Source/core/svg/SVGUseElement.cpp#newcode415 ...

6 years, 7 months ago (2014-04-29 15:08:18 UTC) #5

rwlbuis

On 2014/04/29 04:30:58, pdr wrote: > I like where this patch is going! > > ...

6 years, 7 months ago (2014-04-29 15:53:04 UTC) #6

On 2014/04/29 04:30:58, pdr wrote:
> I like where this patch is going!
> 
> Before your patch it looks like we didn't handle the <use> case at all. Could
> you add a test such as the following?
> <!DOCTYPE HTML>
> <html>
> <body>
> This test passes if there are two green squares below:<br>
> <svg width="300" height="300">
>   <defs>
>     <svg id="svg" width="10" height="10">
>       <rect width="100%" height="100%" fill="green"/>
>     </svg>
>   </defs>
>   <use id="use1" xlink:href="#svg" x="0" y="0" width="100" height="100"/>
>   <use id="use2" xlink:href="#svg" x="100" y="100" width="10" height="10"/>
> </svg>
> <script>
>   onload = function() {
>     // Wait until first layout is finished.
>     window.requestAnimationFrame(function() {
>       document.getElementById('use2').setAttribute('width', '100');
>       document.getElementById('use2').setAttribute('height', '100');
>     });
>   }
> </script>
> </body>
> </html>

It looks like we do not render correctly even without my patch, the green
rectangles are just
10x10 compared to the 100x100 in FF.
 
> I'm also worried about invalidations. Today they go through the instance tree
> but I think that logic will eventually be in SVGElement so the test will come
in
> handy in the future. Could you add a test such as the following?
> <!DOCTYPE HTML>
> <html>
> <body>
> This test passes if there are three green squares below:<br>
> <svg width="300" height="300">
>   <defs>
>     <symbol id="symbol" width="100">
>       <rect width="100%" height="100%" fill="green"/>
>     </symbol>
>   </defs>
>   <svg x="0" y="0" width="100" height="100">
>     <use id="use1" xlink:href="#symbol"/>
>   </svg>
>   <use id="use2" xlink:href="#symbol" x="100" y="100" width="10"
height="100"/>
>   <use id="use3" xlink:href="#symbol" x="200" y="200" width="100"
height="10"/>
> </svg>
> <script>
>   onload = function() {
>     // Wait until first layout is finished.
>     window.requestAnimationFrame(function() {
>       document.getElementById('use2').removeAttribute('width');
>       document.getElementById('use3').setAttribute('height', '100');
>     });
>   }
> </script>
> </body>
> </html>

I am not sure about this one yet, I think we do it correct even though FF
renders differently.
Should I put the tests in a new CL? Ideally with the fix :)

pdr.

On 2014/04/29 15:53:04, rwlbuis wrote: > On 2014/04/29 04:30:58, pdr wrote: > > I like ...

6 years, 7 months ago (2014-04-29 17:20:45 UTC) #7

On 2014/04/29 15:53:04, rwlbuis wrote:
> On 2014/04/29 04:30:58, pdr wrote:
> > I like where this patch is going!
> > 
> > Before your patch it looks like we didn't handle the <use> case at all.
Could
> > you add a test such as the following?
> > <!DOCTYPE HTML>
> > <html>
> > <body>
> > This test passes if there are two green squares below:<br>
> > <svg width="300" height="300">
> >   <defs>
> >     <svg id="svg" width="10" height="10">
> >       <rect width="100%" height="100%" fill="green"/>
> >     </svg>
> >   </defs>
> >   <use id="use1" xlink:href="#svg" x="0" y="0" width="100" height="100"/>
> >   <use id="use2" xlink:href="#svg" x="100" y="100" width="10" height="10"/>
> > </svg>
> > <script>
> >   onload = function() {
> >     // Wait until first layout is finished.
> >     window.requestAnimationFrame(function() {
> >       document.getElementById('use2').setAttribute('width', '100');
> >       document.getElementById('use2').setAttribute('height', '100');
> >     });
> >   }
> > </script>
> > </body>
> > </html>
> 
> It looks like we do not render correctly even without my patch, the green
> rectangles are just
> 10x10 compared to the 100x100 in FF.
>  
> > I'm also worried about invalidations. Today they go through the instance
tree
> > but I think that logic will eventually be in SVGElement so the test will
come
> in
> > handy in the future. Could you add a test such as the following?
> > <!DOCTYPE HTML>
> > <html>
> > <body>
> > This test passes if there are three green squares below:<br>
> > <svg width="300" height="300">
> >   <defs>
> >     <symbol id="symbol" width="100">
> >       <rect width="100%" height="100%" fill="green"/>
> >     </symbol>
> >   </defs>
> >   <svg x="0" y="0" width="100" height="100">
> >     <use id="use1" xlink:href="#symbol"/>
> >   </svg>
> >   <use id="use2" xlink:href="#symbol" x="100" y="100" width="10"
> height="100"/>
> >   <use id="use3" xlink:href="#symbol" x="200" y="200" width="100"
> height="10"/>
> > </svg>
> > <script>
> >   onload = function() {
> >     // Wait until first layout is finished.
> >     window.requestAnimationFrame(function() {
> >       document.getElementById('use2').removeAttribute('width');
> >       document.getElementById('use3').setAttribute('height', '100');
> >     });
> >   }
> > </script>
> > </body>
> > </html>
> 
> I am not sure about this one yet, I think we do it correct even though FF
> renders differently.
> Should I put the tests in a new CL? Ideally with the fix :)

I'm not sure your new patch correctly handles the invalidation case. Before,
we'd propagate that through in some cases whereas now we may miss it (or, it's
not obvious to me when it's handled). Even though we didn't pass these tests
before your patch, does your patch change the behavior on these tests?

rwlbuis

On 2014/04/29 17:20:45, pdr wrote: > On 2014/04/29 15:53:04, rwlbuis wrote: > > On 2014/04/29 ...

6 years, 7 months ago (2014-04-29 17:31:50 UTC) #8

On 2014/04/29 17:20:45, pdr wrote:
> On 2014/04/29 15:53:04, rwlbuis wrote:
> > On 2014/04/29 04:30:58, pdr wrote:
> > > I like where this patch is going!
> > > 
> > > Before your patch it looks like we didn't handle the <use> case at all.
> Could
> > > you add a test such as the following?
> > > <!DOCTYPE HTML>
> > > <html>
> > > <body>
> > > This test passes if there are two green squares below:<br>
> > > <svg width="300" height="300">
> > >   <defs>
> > >     <svg id="svg" width="10" height="10">
> > >       <rect width="100%" height="100%" fill="green"/>
> > >     </svg>
> > >   </defs>
> > >   <use id="use1" xlink:href="#svg" x="0" y="0" width="100" height="100"/>
> > >   <use id="use2" xlink:href="#svg" x="100" y="100" width="10"
height="10"/>
> > > </svg>
> > > <script>
> > >   onload = function() {
> > >     // Wait until first layout is finished.
> > >     window.requestAnimationFrame(function() {
> > >       document.getElementById('use2').setAttribute('width', '100');
> > >       document.getElementById('use2').setAttribute('height', '100');
> > >     });
> > >   }
> > > </script>
> > > </body>
> > > </html>
> > 
> > It looks like we do not render correctly even without my patch, the green
> > rectangles are just
> > 10x10 compared to the 100x100 in FF.
> >  
> > > I'm also worried about invalidations. Today they go through the instance
> tree
> > > but I think that logic will eventually be in SVGElement so the test will
> come
> > in
> > > handy in the future. Could you add a test such as the following?
> > > <!DOCTYPE HTML>
> > > <html>
> > > <body>
> > > This test passes if there are three green squares below:<br>
> > > <svg width="300" height="300">
> > >   <defs>
> > >     <symbol id="symbol" width="100">
> > >       <rect width="100%" height="100%" fill="green"/>
> > >     </symbol>
> > >   </defs>
> > >   <svg x="0" y="0" width="100" height="100">
> > >     <use id="use1" xlink:href="#symbol"/>
> > >   </svg>
> > >   <use id="use2" xlink:href="#symbol" x="100" y="100" width="10"
> > height="100"/>
> > >   <use id="use3" xlink:href="#symbol" x="200" y="200" width="100"
> > height="10"/>
> > > </svg>
> > > <script>
> > >   onload = function() {
> > >     // Wait until first layout is finished.
> > >     window.requestAnimationFrame(function() {
> > >       document.getElementById('use2').removeAttribute('width');
> > >       document.getElementById('use3').setAttribute('height', '100');
> > >     });
> > >   }
> > > </script>
> > > </body>
> > > </html>
> > 
> > I am not sure about this one yet, I think we do it correct even though FF
> > renders differently.
> > Should I put the tests in a new CL? Ideally with the fix :)
> 
> I'm not sure your new patch correctly handles the invalidation case. Before,
> we'd propagate that through in some cases whereas now we may miss it (or, it's
> not obvious to me when it's handled). Even though we didn't pass these tests
> before your patch, does your patch change the behavior on these tests?

For the first svg, it is slightly better, the top-left one seems the right size
(100x100) but the bottom-right is still (10x10).
The second svg is quite wrong.
My patch does not seem the right way anyway thinking about the animation
problem. I'll recheck with trunk for how we behave to above two tests.

rwlbuis

On 2014/04/29 17:31:50, rwlbuis wrote: > On 2014/04/29 17:20:45, pdr wrote: > > On 2014/04/29 ...

6 years, 7 months ago (2014-04-29 17:53:32 UTC) #9

rwlbuis

This fixes the two nice and tricky tests. I did not implement yet all of ...

6 years, 7 months ago (2014-04-29 20:05:10 UTC) #10

pdr.

On 2014/04/29 20:05:10, rwlbuis wrote: > I'll have a think about the SMIL animation scenario ...

6 years, 7 months ago (2014-04-29 20:09:59 UTC) #11

On 2014/04/29 20:09:59, pdr wrote: > On 2014/04/29 20:05:10, rwlbuis wrote: > > I'll have ...

6 years, 7 months ago (2014-04-30 07:54:58 UTC) #12

rwlbuis

On 2014/04/30 07:54:58, fs wrote: > On 2014/04/29 20:09:59, pdr wrote: > > On 2014/04/29 ...

6 years, 7 months ago (2014-04-30 15:46:45 UTC) #13

pdr.

This looks pretty good to me. @fs, did you have any further thoughts on this? ...

6 years, 7 months ago (2014-04-30 18:53:51 UTC) #14

rwlbuis

I did a first stab at the animation test. Two problems with the current version: ...

6 years, 7 months ago (2014-05-01 15:30:13 UTC) #15

rwlbuis

On 2014/05/01 15:30:13, rwlbuis wrote: > I did a first stab at the animation test. ...

6 years, 7 months ago (2014-05-01 15:38:55 UTC) #16

rwlbuis

On 2014/05/01 15:38:55, rwlbuis wrote: > On 2014/05/01 15:30:13, rwlbuis wrote: > > I did ...

6 years, 7 months ago (2014-05-01 22:37:02 UTC) #17

Erik Dahlström (inactive)

Thanks, my concerns are addressed by this.

6 years, 7 months ago (2014-05-02 07:15:37 UTC) #18

rwlbuis

I now added a NeedRebaseline for relative-sized-shadow-tree-content-with-symbol.xhtml, the patch has the linux results but we ...

6 years, 7 months ago (2014-05-02 22:50:55 UTC) #19

rwlbuis

I now added a NeedRebaseline for relative-sized-shadow-tree-content-with-symbol.xhtml, the patch has the linux results but we ...

6 years, 7 months ago (2014-05-02 22:51:01 UTC) #20

pdr.

LGTM https://codereview.chromium.org/252903004/diff/140001/Source/core/svg/SVGUseElement.cpp File Source/core/svg/SVGUseElement.cpp (right): https://codereview.chromium.org/252903004/diff/140001/Source/core/svg/SVGUseElement.cpp#newcode197 Source/core/svg/SVGUseElement.cpp:197: inline void transferUseWidthAndHeightIfNeeded(const SVGUseElement& use, SVGElement* shadowElement, SVGElement* ...

6 years, 7 months ago (2014-05-02 23:03:51 UTC) #21