889736 - remove old SVG text frame classes

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Cameron McCormack (:heycam)]

At some point we can remove nsSVG{Text{,Container,Path},TSpan}.cpp and remove all of the code branches that deal with them, e.g. in the SVG DOM methods on the content classes.  How long should we give it?  The new code was turned on for Firefox 25, so how about removing it from Nightly once Release gets to Firefox 26?  So that's when Release is 29.

Comment 1

[Cameron McCormack (:heycam)]

*that's when Nightly is 29

Comment 2

[Jonathan Watt [:jwatt]]

I think that's a good time to reevaluate. You may find that a continuous trickle of regressions keep coming in, and that it's useful to keep the pref around to help easily distinguish between regression bugs and other bugs. (At least that has been the case for SVG display lists.)

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: part 1 - remove nsSVGTextFrame and nsSVGGlyphFrame

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: part 2 - Remove nsSVGTextPathFrame

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: part 3 - Remove nsSVGTSpanFrame

Comment 6

[Jonathan Watt [:jwatt]]

Attachment: part 4 - Remove nsSVGTextContainerFrame

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: part 5 - Remove nsISVGGlyphFragmentNode and nsISVGGlyphFragmentLeaf

Comment 8

[Jonathan Watt [:jwatt]]

Attachment: part 6 - Stop setting the svg.text.css-frames.enabled pref

Comment 9

[Jonathan Watt [:jwatt]]

We don't seem to have had any issues with the new SVG text code paths that are bad enough for us to disable the pref for beta or aurora, let alone nightly. If Cameron is okay with taking these patches now, so am I.

Comment 10

[Cameron McCormack (:heycam)]

Comment on attachment 8333541 [details] [diff] [review]
part 1 - remove nsSVGTextFrame and nsSVGGlyphFrame

Review of attachment 8333541 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed.

::: layout/base/RestyleManager.cpp
@@ +208,1 @@
>          // Invalidate and reflow the entire nsSVGTextFrame2:

Re-indent the block.

::: layout/generic/nsFrame.cpp
@@ -7377,5 @@
>      uint8_t dominantBaseline;
>      for (const nsIFrame* frame = this; frame; frame = frame->GetParent()) {
>        dominantBaseline = frame->StyleSVGReset()->mDominantBaseline;
> -      if (dominantBaseline != NS_STYLE_DOMINANT_BASELINE_AUTO ||
> -          frame->GetType() == nsGkAtoms::svgTextFrame) {

I think this might have been a typo.  There's no way to traverse up to an nsSVGTextFrame here since we've already checked IsSVGText().  We should be checking whether we've reached the nsSVGTextFrame2.  File a separate bug for this?

::: layout/svg/nsSVGOuterSVGFrame.cpp
@@ +112,5 @@
>  nsSVGMutationObserver::UpdateTextFragmentTrees(nsIFrame *aFrame)
>  {
>    nsIFrame* kid = aFrame->GetFirstPrincipalChild();
>    while (kid) {
> +    UpdateTextFragmentTrees(kid);

UpdateTextFragmentTrees is no longer doing anything, and I think nsSVGMutationObserver can disappear altogether.  Changes to xml:space="" are handled by the UA style sheet rule.  Do that in a separate patch?

::: layout/svg/nsSVGUtils.cpp
@@ +37,5 @@
>  #include "nsSVGFilterFrame.h"
>  #include "nsSVGFilterPaintCallback.h"
>  #include "nsSVGForeignObjectFrame.h"
>  #include "nsSVGGeometryFrame.h"
> +#include "gfxSVGGlyphs.h"

The list of includes is *almost* sorted, so please put this in its right(-ish) place.

Comment 11

[Jonathan Watt [:jwatt]]

Comment on attachment 8333548 [details] [diff] [review]
part 6 - Stop setting the svg.text.css-frames.enabled pref

Note that test_selectSubString2.xhtml, tspan-rotate-02.svg and tspan-rotate-04.svg are currently run with the pref set to false. That's a bit weird.

Comment 12

[Cameron McCormack (:heycam)]

Comment on attachment 8333542 [details] [diff] [review]
part 2 - Remove nsSVGTextPathFrame

Review of attachment 8333542 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 13

[Jonathan Watt [:jwatt]]

In which case it's probably not surprising that test_selectSubString2.xhtml fails, while it may be surprising that tspan-rotate-02.svg and tspan-rotate-04.svg both pass:

https://tbpl.mozilla.org/?tree=Try&rev=476ce90c479a

Also it seems test_text_selection.html fails for some reason. :/

Comment 14

[Cameron McCormack (:heycam)]

(In reply to Jonathan Watt [:jwatt] from comment #13)
> Also it seems test_text_selection.html fails for some reason. :/

Odd.  BTW the part of that test where we have <style>text { display: none }</style> in text-helper-selection.svg, and we remove that <style> element in test_text_selection.html, can be removed.

Comment 15

[Cameron McCormack (:heycam)]

(In reply to Jonathan Watt [:jwatt] from comment #13)
> In which case it's probably not surprising that test_selectSubString2.xhtml
> fails

Oh, so this is a test that selectSubString does not exist on <text> elements in the DOM when the pref is turned off, since text selection doesn't work there.  Your part 6 patch removes the [Pref=...] bit from SVGTextContent.webidl which conditionally hides selectSubString.

Comment 16

[Cameron McCormack (:heycam)]

Comment on attachment 8333548 [details] [diff] [review]
part 6 - Stop setting the svg.text.css-frames.enabled pref

Review of attachment 8333548 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed.

::: content/svg/content/test/test_selectSubString.xhtml
@@ +61,5 @@
>  
>    SimpleTest.finish();
>  }
>  
> +window.addEventListener("load", runTests, false);

Remove the three lines beginning with "ensure that frames are generated with svg.text.css-frames.enabled".

::: content/svg/content/test/test_selectSubString2.xhtml
@@ +28,5 @@
>  
>    SimpleTest.finish();
>  }
>  
> +window.addEventListener("load", runTests, false);

I think this whole test should be deleted.

::: content/svg/content/test/test_text_2.html
@@ +28,5 @@
>    ok(Math.abs(x - y) < 1e-4, message);
>  }
>  
>  function runTest() {
> +  document.getElementById("content").style.display = "block";

Remove this line and the style="display: none" from the markup.

::: content/svg/content/test/test_text_dirty.html
@@ +43,5 @@
>    ok(true);
>    SimpleTest.finish();
>  }
>  
> +window.addEventListener("load", runTest, false);

Remove the five lines beginning with "Recreate the frames for the <text> with the pref set".

::: content/svg/content/test/test_text_lengthAdjust.html
@@ +26,5 @@
>    ok(Math.abs(x - y) < 1e-4, message);
>  }
>  
>  function runTest() {
> +  document.getElementById("content").style.display = "block";

Remove this line and the style="display: none" from the markup.

::: content/svg/content/test/test_text_selection.html
@@ +130,5 @@
>    deselect();
>  
>    SimpleTest.finish();
>  }
>  

Remove the three lines beginning with "Display all the <text> elements." as well as the <style> element from text-helper-selection.svg.

::: layout/reftests/svg/text/reftest.list
@@ -1,1 @@
> -default-preferences pref(svg.text.css-frames.enabled,true)

Remove the blank line too.

Comment 17

[Cameron McCormack (:heycam)]

(In reply to Jonathan Watt [:jwatt] from comment #13)
> Also it seems test_text_selection.html fails for some reason. :/

It passes for me locally if I replace the SpecialPowers.pushPrefEnv call with a SimpleTest.executeSoon.  Not sure why that is, but I suggest just doing that.

Comment 18

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f2f35104ab0b
https://hg.mozilla.org/integration/mozilla-inbound/rev/b01b36f72059
https://hg.mozilla.org/integration/mozilla-inbound/rev/d80e27b74a84
https://hg.mozilla.org/integration/mozilla-inbound/rev/14de5edb2078
https://hg.mozilla.org/integration/mozilla-inbound/rev/c179a6e6ed92
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef5d7b35634b

Comment 19

[Jonathan Watt [:jwatt]]

(In reply to Cameron McCormack (:heycam) from comment #10)
> Re-indent the block.

I think you must have misread this. It seems correctly indented to me.

> > -      if (dominantBaseline != NS_STYLE_DOMINANT_BASELINE_AUTO ||
> > -          frame->GetType() == nsGkAtoms::svgTextFrame) {
> 
> I think this might have been a typo.  There's no way to traverse up to an
> nsSVGTextFrame here since we've already checked IsSVGText().  We should be
> checking whether we've reached the nsSVGTextFrame2.  File a separate bug for
> this?

This definitely looks like a typo, so I switched it to use nsGkAtoms::svgTextFrame2. What would the new bug be for?

> UpdateTextFragmentTrees is no longer doing anything, and I think
> nsSVGMutationObserver can disappear altogether.  Changes to xml:space="" are
> handled by the UA style sheet rule.  Do that in a separate patch?

Filed bug 939775.

(In reply to Cameron McCormack (:heycam) from comment #17)
> (In reply to Jonathan Watt [:jwatt] from comment #13)
> > Also it seems test_text_selection.html fails for some reason. :/
> 
> It passes for me locally if I replace the SpecialPowers.pushPrefEnv call
> with a SimpleTest.executeSoon.  Not sure why that is, but I suggest just
> doing that.

Thanks for checking. I did exactly as you suggested.

Comment 20

[Cameron McCormack (:heycam)]

(In reply to Jonathan Watt [:jwatt] from comment #19)
> I think you must have misread this. It seems correctly indented to me.

Yes I misread.

> > > -      if (dominantBaseline != NS_STYLE_DOMINANT_BASELINE_AUTO ||
> > > -          frame->GetType() == nsGkAtoms::svgTextFrame) {
> > 
> > I think this might have been a typo.  There's no way to traverse up to an
> > nsSVGTextFrame here since we've already checked IsSVGText().  We should be
> > checking whether we've reached the nsSVGTextFrame2.  File a separate bug for
> > this?
> 
> This definitely looks like a typo, so I switched it to use
> nsGkAtoms::svgTextFrame2. What would the new bug be for?

Nothing, now that you've fixed it here. :-)

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f2f35104ab0b
https://hg.mozilla.org/mozilla-central/rev/b01b36f72059
https://hg.mozilla.org/mozilla-central/rev/d80e27b74a84
https://hg.mozilla.org/mozilla-central/rev/14de5edb2078
https://hg.mozilla.org/mozilla-central/rev/c179a6e6ed92
https://hg.mozilla.org/mozilla-central/rev/ef5d7b35634b