50742 - Investigate switching output to use DOM Serializer

Status: VERIFIED FIXED

(Core :: DOM: Serializers, defect, P1)

Description

[Akkana Peck]

Vidur's DOM serializer seems to work fine in the simple tests I've run, and we
could potentially see a huge gain in performance if we switched to use it
instead of XIF for all of our output.  The output system can already handle
converting from html to plaintext, so all we have to do is use the dom
serializer instead of nsXIFConverter/nsHTMLContentSinkStream for the html output
path (though for plaintext, we'll need to investigate the performance win of DOM
Serializer HTML -> plaintext vs. Document XIF -> plaintext).

This might also simplify output of selection, since we could use selection
iterators instead of going through the document's "iterate over whole document
calling IsInSelection" algorithm.  Cc'ing Joe since he's also thought about the
conversion to iterators.

Comment 1

[Akkana Peck]

perf, M19.

Comment 2

[Akkana Peck]

JST and Vidur are working on this now.

Comment 3

[Joe Francis]

Editor work on copy/paste needs jst&vidurs work here to land.  Beth, could you 
make the case to pdt for this?

Comment 4

[rubydoo123]

this fix must get in so our p1 & p2 bug fixes can land

Comment 5

[rubydoo123]

adding Nisheeth

Comment 6

[rubydoo123]

PDT: in conversation with clayton, marking p1 and double +, this is major fix
for several editor bugs

jst, vidur, akkana and joe --- please use due diligence in following checkin rules

Comment 7

[Johnny Stenback (:jst)]

I, Vidur, Joe and Jim Roskind talked this over and we decided that it would be
best if we hold off with checkin this in for beta3, once beta3 is out we'll land
the changes on the trunk, let them live there for a few days and then once we're
certain that the changes are ok, we'll check them into the RTM (nsbeta3) branch.

Comment 8

[rubydoo123]

please note that the dependent editor bugs will be checked in on the heels of
this bug so they can all bake on the trunk

Comment 9

[Nisheeth Ranjan]

In light of Johnny's comment, removing nsbeta3++ marker, nominating for rtm and 
marking rtm++.

Comment 10

[Nisheeth Ranjan]

Marking nsbeta3- to get off beta 3 radar.

Comment 11

[rubydoo123]

*** Bug 43008 has been marked as a duplicate of this bug. ***

Comment 12

[Nisheeth Ranjan]

On second thought, removing rtm++ from bug.  The PDT should make this call.

Comment 13

[Phil Peterson]

PDT says, If this is The NoXIF bug, that's fine. But if we're just trading old
bugs or performance problems for new bugs or performance problems, let's wait
until after RTM.

Comment 14

[Johnny Stenback (:jst)]

This is indeed the noxif bug.

Comment 15

[lchiang]

What are some of the test cases that QA would need to run once this is fixed?
Suggested testing areas?  This sounds like a large change that we'd want to make
sure we get coverage on.  Thanks.

Comment 16

[Phil Peterson]

PDT marking [rtm need info] since this bug doesn't show the patch or code
reviewers yet. Also, it's getting very late for reworking big parts of the app.
Don't expect to let this fix wait until the last day.

Comment 17

[Joe Francis]

hey, just wait til I wanna land all the changes needed to make the editor work 
correctly with whitespace!  I haven't even *started* that yet.

meanwhile back on the farm, we are in the processing of verifying our merge of 
the trunk tip onto the NOXIF branch.  Once that happens there are some details in 
paste I have to finish up.  My guesstimate is that tomorrow morning might be a 
good time for us to land on the trunk and expose this to a wider audience.  

No good trying to rush things.. sell no wine yadda yadda.

Comment 18

[Joe Francis]

Johnny and i think things are looking pretty good.  We worked on the noxif branch 
over the past two days doing testing and fixing problems.  At this point we are 
actively seeking reviews and permission to land on the trunk, in that order.  Kin 
is going to review editor changes with me, hopefully very early tomorrow morning.  

Comment 19

[Johnny Stenback (:jst)]

Attachment: Propsed fix for everything except the editor, editor patches will follow.

Comment 20

[Johnny Stenback (:jst)]

scc, I'd appreciate it if you could have a look at the changes we've made to:

layout/base/src/nsHTMLContentSerializer.cpp
layout/base/src/nsPlainTextSerializer.cpp
layout/html/content/src/nsGenericHTMLElement.cpp
mailnews/base/src/nsMessenger.cpp
mailnews/compose/src/nsMsgCompUtils.cpp

I'll come over to discuss this with you.

Comment 21

[Ben Bucksch (:BenB)]

I saw that nsPlainTextSerializer.cpp is checked into the trunk (which is good).
Is there a way to test it without going on a branch, e.g. a define?

Comment 22

[Johnny Stenback (:jst)]

These changes just landed on the mozilla trunk, feel free to use them and test
them, of course, no define needed now, it's all built by default, use the
component manager to create either a nsDocumentEncoder or the raw serializer.

Comment 23

[Ben Bucksch (:BenB)]

Found bug 55669.

Comment 24

[Johnny Stenback (:jst)]

Ok, I just attached a fix to bug 55669, however, I don't think it's really a
problem we need to fix for rtm since AFAIK there's no code in mozilla other than
the Debug code in the editor that uses DOM-->TXT conversion on complete
documents, AFAIK the only other place that code is used is in input elements
(i.e. ender lite) to get the value of the input and in that case there's never a
<HEAD> element so there's no problem.

Please correct me if I'm wrong.

Comment 25

[Ben Bucksch (:BenB)]

jst,
- can't we save webpages to txt?
- We can save mail to txt. What about HTML mails (which might contain <style>s)?
- Quoting HTML mails?

Comment 26

[Ben Bucksch (:BenB)]

Found dataloss bug 55806.

Comment 27

[Daniel Bratell]

I think Ben forgot the most common place: When sending a mail. It's only when 
the mail is sent as html mail and nothing but html mail it's not converted to 
plain text (or format=flowed).

Comment 28

[Ben Bucksch (:BenB)]

Daniel, does/can our mail HTML composer generate/compose headers (apart from
Insert|HTML)?

Comment 29

[Daniel Bratell]

Don't know. But I commented on Johnny's comment that this (the DOM->Text 
converter) was only used to extract text from text fields and in some obscure 
places.

Comment 30

[Akkana Peck]

When sending mail, at least most of the time, instead of saving straight to
plaintext, mail saves as html, then later translates the html to plaintext. 
This is a bug, and it's filed (to rhp, I think?), but it's not likely to get
fixed before rtm.

I don't know what mail does in the case of saving as plaintext.  The editor
doesn't, alas, have a save as plaintext option exposed in the non-debug UI.  I
don't know of any case where we go straight from the dom to plaintext, though of
course we should in several cases.

Comment 31

[Johnny Stenback (:jst)]

Bug 55806 has a fix attached now.

Comment 32

[Daniel Bratell]

Akkana, are you talking about the old world, or what will be? I thought that, 
with XIF gone, we would always go straight from DOM to text?

Comment 33

[Akkana Peck]

I don't think noxif changed the circuitous pathways through libmime at all, only
the methods mime has to call to actually make its translations once it's decided
what the format will be.  Johnny can confirm since he's more familiar with the
change (but it doesn't sound like it based on his earlier comments in this bug.)

Comment 34

[Johnny Stenback (:jst)]

Marking rtm++ per discussion with Jim (jar) and PDT.

Comment 35

[Johnny Stenback (:jst)]

Attachment: Complete patch that was checked in on the netscape6 branch

Comment 36

[Johnny Stenback (:jst)]

I'm done here, the last patch is checked in on both the branch and the trunk.
Marking fixed.

Comment 37

[Akkana Peck]

I'll verify this -- the serializer stuff is in, and xif is out.