Closed
Bug 889736
Opened 11 years ago
Closed 11 years ago
remove old SVG text frame classes
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
FIXED
mozilla28
People
(Reporter: heycam, Assigned: jwatt)
References
Details
(Whiteboard: [qa-])
Attachments
(6 files)
120.55 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
17.70 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
16.50 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
30.91 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
2.57 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
19.48 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
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•11 years ago
|
||
*that's when Nightly is 29
Assignee | ||
Comment 2•11 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.)
Updated•11 years ago
|
Summary: remove old SVG text frames → remove old SVG text frame classes
Assignee | ||
Updated•11 years ago
|
Assignee | ||
Comment 3•11 years ago
|
||
Assignee: nobody → jwatt
Attachment #8333541 -
Flags: review?(cam)
Assignee | ||
Comment 4•11 years ago
|
||
Attachment #8333542 -
Flags: review?(cam)
Assignee | ||
Comment 5•11 years ago
|
||
Attachment #8333543 -
Flags: review?(cam)
Assignee | ||
Comment 6•11 years ago
|
||
Attachment #8333545 -
Flags: review?(cam)
Assignee | ||
Comment 7•11 years ago
|
||
Attachment #8333546 -
Flags: review?(cam)
Assignee | ||
Comment 8•11 years ago
|
||
Attachment #8333548 -
Flags: review?(cam)
Assignee | ||
Comment 9•11 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•11 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•11 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•11 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•11 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•11 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•11 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•11 years ago
|
Attachment #8333543 -
Flags: review?(cam) → review+
Reporter | ||
Updated•11 years ago
|
Attachment #8333545 -
Flags: review?(cam) → review+
Reporter | ||
Updated•11 years ago
|
Attachment #8333546 -
Flags: review?(cam) → review+
Reporter | ||
Comment 16•11 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•11 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 | ||
Comment 18•11 years ago
|
||
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
Assignee | ||
Comment 19•11 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•11 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. :-)
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
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Updated•11 years ago
|
Updated•11 years ago
|
Updated•11 years ago
|
Whiteboard: [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•