83425 - Old HTML->TXT converter used

Status: VERIFIED FIXED

(Core :: DOM: Serializers, defect)

Description

[Ben Bucksch (:BenB)]

Sometime during the move to nsPlainTextSerializer.cpp, an older revision of the
file nsHTMLToTXTSinkStream.cpp has been used.

A direct result, where I catched it, is that bug 52042 regressed, which caused
bug 32336 to regress.

The proof:
<http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/htmlparser/src/nsHTMLToTXTSinkStream.cpp>,
line 475 shows "moz-txt", which is correct. However, the current revision of
nsPlaintextSerializer.cpp (the predecessor)
<http://lxr.mozilla.org/mozilla/source/content/base/src/nsPlainTextSerializer.cpp#1565>
uses just "txt", which is the old version. this started with the first revision
of that file
<http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/base/src/nsPlainTextSerializer.cpp&rev=1.1>
(line 1435).

I don't know, which or how many other bugs regressed. Please check, which
revisions have been skipped and merge these changes into the newest version of
nsPlainTextSerializer.cpp.

Comment 1

[Ben Bucksch (:BenB)]

s/predecessor/successor/

Comment 2

[vidur (gone)]

Hmm...seems like nsHTMLToTXTSinkStream revision 3.87
(http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsHTMLToTXTSinkStream.cpp&root=/cvsroot&subdir=mozilla/htmlparser/src/Attic&command=DIFF_FRAMESET&rev1=3.86&rev2=3.87)
was excluded. There is a partial discrepancy between code in
nsPlainTextSerializer::AddToLine() and the equivalent modified code in
nsHTMLToTXTSinkStream revision 3.89
(http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsHTMLToTXTSinkStream.cpp&root=/cvsroot&subdir=mozilla/htmlparser/src/Attic&command=DIFF_FRAMESET&rev1=3.88&rev2=3.89),
but it's not clear to me whether the former is incorrect.

Akkana, since I'm no longer familiar with the specifics of this code, I think it
makes more sense for you to determine whether the changes should be made now.
I'll attach a patch with the missing modifications.

Comment 3

[vidur (gone)]

This seems to be the only modification missed:

Index: nsPlainTextSerializer.cpp
===================================================================
RCS file: /cvsroot/mozilla/content/base/src/nsPlainTextSerializer.cpp,v
retrieving revision 1.24
diff -u -r1.24 nsPlainTextSerializer.cpp
--- nsPlainTextSerializer.cpp   2001/04/26 19:32:56     1.24
+++ nsPlainTextSerializer.cpp   2001/06/01 22:17:14
@@ -1562,8 +1562,8 @@
   nsAutoString value;
   nsresult rv = GetAttributeValue(nsHTMLAtoms::kClass, value);
   return (NS_SUCCEEDED(rv) &&
-          (value.EqualsIgnoreCase("txt", 3) ||
-           value.EqualsIgnoreCase("\"txt", 4)));
+          (value.EqualsIgnoreCase("moz-txt", 3) ||
+           value.EqualsIgnoreCase("\"moz-txt", 4)));
 }

Comment 4

[Ben Bucksch (:BenB)]

vidur, the change you pasted is definitely needed, otherwise that will cause
bugs (see above).

> AddToLine

If I saw it correctly, bug 54016 was the reason for that change. Seems needed.

Comment 5

[Ben Bucksch (:BenB)]

vidur, in the above patch, you need to add 4 for the comparison-length numbers,
i.e. 7 and 8 instead of 3 and 4. Otherwise r=BenB.

Comment 6

[Ben Bucksch (:BenB)]

ping

Comment 7

[Ben Bucksch (:BenB)]

Nominating for nsBranch. Please excuse me, if I'm not allowed to do that.

Rationale:
This bug caused bug 32336 to regress. Have you seen that Mailnews sometimes
sends out the same URL 2 times (in plaintext). Fixing this bug will prevent that
in almost all cases.
Fixing this bug is very low risk. The change has already been made long ago and
it was just an accident that it has practically been backed out.

Comment 8

[vidur (gone)]

Sorry for the delay on this. I'm looking for a response from Akkana on this,
since this is in her domain. Reassigning to her to get this in.

Comment 9

[Akkana Peck]

You were waiting for me?  I didn't know that ...  I don't own the serializers
any more (Anthonyd does) but yes, of course we should restore code that got
stomped accidentally.  Looks fine to me (with the correct comparison lengths) if
Ben says it's the right thing.  r=akkana.

Comment 10

[vidur (gone)]

The fix has been checked into the trunk. With the goal of making a case for
getting it into the branch to the PDT, Akanna and I tried to recreate the
symptoms described in bug 32336, but weren't able to. Ben, do you have any
thoughts on why the "double formatting" mentioned in bug 32336 doesn't happen
even without this fix?

Comment 11

[Ben Bucksch (:BenB)]

Try the reproduction of bug 58527.

Comment 12

[Ben Bucksch (:BenB)]

What about the AddToLine change? (Sorry if that was resolved already.)

Comment 13

[Daniel Bratell]

To me, it looks correct in the current nsPlaintextSerializer. The change was
added in v1.3 by jst, right after the split.

Comment 14

[Akkana Peck]

Yes: without the fix, I see the problem described in bug 58527, doubled link
after save as draft.  With the fix (i.e. with today's trunk), I see only one
copy of the link.  The branch will still exhibit bug 58527 with Save as Draft.

Comment 15

[anthonyd]

resolving this bug as fixed. branch is closed.

Comment 16

[sujay]

Ben/bratell can you verify this and mark verified-fixed? thanks.

Comment 17

[Ben Bucksch (:BenB)]

vrfy by:
- checking source via LXR
- Trying reproduction of bug 58527* on Linux

VERIFIED on Linux and source.

*this bug does not completely fix bug 58527, so don't close that bug just yet.

Comment 18

[sujay]

marking verified per Ben's comments.