remove old SVG text frame classes

RESOLVED FIXED in mozilla28

Status

()

RESOLVED FIXED
6 years ago
5 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)

(Reporter)

Description

6 years ago
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.
(Reporter)

Comment 1

6 years ago
*that's when Nightly is 29
(Assignee)

Comment 2

6 years ago
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
(Assignee)

Updated

5 years ago
Depends on: 655877, 839955
(Assignee)

Updated

5 years ago
Depends on: 928879
(Assignee)

Comment 3

5 years ago
Created attachment 8333541 [details] [diff] [review]
part 1 - remove nsSVGTextFrame and nsSVGGlyphFrame
Assignee: nobody → jwatt
Attachment #8333541 - Flags: review?(cam)
(Assignee)

Comment 4

5 years ago
Created attachment 8333542 [details] [diff] [review]
part 2 - Remove nsSVGTextPathFrame
Attachment #8333542 - Flags: review?(cam)
(Assignee)

Comment 5

5 years ago
Created attachment 8333543 [details] [diff] [review]
part 3 - Remove nsSVGTSpanFrame
Attachment #8333543 - Flags: review?(cam)
(Assignee)

Comment 6

5 years ago
Created attachment 8333545 [details] [diff] [review]
part 4 - Remove nsSVGTextContainerFrame
Attachment #8333545 - Flags: review?(cam)
(Assignee)

Comment 7

5 years ago
Created attachment 8333546 [details] [diff] [review]
part 5 - Remove nsISVGGlyphFragmentNode and nsISVGGlyphFragmentLeaf
Attachment #8333546 - Flags: review?(cam)
(Assignee)

Comment 8

5 years ago
Created attachment 8333548 [details] [diff] [review]
part 6 - Stop setting the svg.text.css-frames.enabled pref
Attachment #8333548 - Flags: review?(cam)
(Assignee)

Comment 9

5 years ago
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.
(Reporter)

Comment 10

5 years ago
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+
(Assignee)

Comment 11

5 years ago
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.
(Reporter)

Comment 12

5 years ago
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+
(Assignee)

Comment 13

5 years ago
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. :/
(Reporter)

Comment 14

5 years ago
(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.
(Reporter)

Comment 15

5 years ago
(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.
(Reporter)

Updated

5 years ago
Attachment #8333543 - Flags: review?(cam) → review+
(Reporter)

Updated

5 years ago
Attachment #8333545 - Flags: review?(cam) → review+
(Reporter)

Updated

5 years ago
Attachment #8333546 - Flags: review?(cam) → review+
(Reporter)

Comment 16

5 years ago
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+
(Reporter)

Comment 17

5 years ago
(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.
(Assignee)

Updated

5 years ago
Blocks: 939775
(Assignee)

Comment 19

5 years ago
(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.
(Reporter)

Comment 20

5 years ago
(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

Updated

5 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.