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.

*that's when Nightly is 29

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.)

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 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 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 on attachment 8333542 [details] [diff] [review] part 2 - Remove nsSVGTextPathFrame Review of attachment 8333542 [details] [diff] [review]: ----------------------------------------------------------------- r=me

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. :/

(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.

(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 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.

(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.

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

(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.

(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. :-)

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