remove old SVG text frame classes

RESOLVED FIXED in mozilla28

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: heycam, Assigned: jwatt)

Tracking

Trunk
mozilla28
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(6 attachments)

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.)
Summary: remove old SVG text frames → remove old SVG text frame classes
Depends on: svgtext, 839955
Depends on: 928879
Assignee: nobody → jwatt
Attachment #8333541 - Flags: review?(cam)
Attachment #8333542 - Flags: review?(cam)
Attachment #8333543 - Flags: review?(cam)
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.
Attachment #8333541 - Flags: review?(cam) → review+
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
Attachment #8333542 - Flags: review?(cam) → review+
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.
Attachment #8333543 - Flags: review?(cam) → review+
Attachment #8333545 - Flags: review?(cam) → review+
Attachment #8333546 - Flags: review?(cam) → review+
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.
Attachment #8333548 - Flags: review?(cam) → review+
(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.
Blocks: 939775
(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. :-)
Depends on: 658499
Depends on: 830563
Blocks: 658499
No longer depends on: 658499
Blocks: 830563
No longer depends on: 830563
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.