78458 - Remove our broken text/rtf support

Status: VERIFIED FIXED

(Core :: DOM: HTML Parser, defect, P3)

Description

[Boris Zbarsky [:bzbarsky]]

Spun off of bug 77080.

This involves updating parser, nsLayoutDLF, nsContentDLF, possibly others.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Fixing this would fix a bunch of bugs...  adding dependencies

Reassigning to myself

Comment 2

[chris hofmann]

are the fixes on track for landing in the next week or so?
if not lets retarget.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: patch

Comment 4

[Boris Zbarsky [:bzbarsky]]

The patch removes rft from parser and content.

It does not touch CRtfDTD.cpp, which is not built anyway.

It also does not remove it from the list of common mimetypes that the mime
service will detect.  We will now correctly bring up the downloading dialog for
text/rtf, though.

Reviews?

Comment 5

[Fabian Guisset]

You should really ask Rick Gessner ([e-mail]) about this, because I
remember him saying in rtf bugs that he didn't want to remove rtf support.

Comment 6

[Boris Zbarsky [:bzbarsky]]

ccing Rick.

Rick, this change does not remove the RTF parser.  It essentially just prevents
us from trying to handle the text/rtf mimetype, which we don't anyway.  If the
RTF parser is ever at a point where it can be used well, we can reinstate the
mimetype handling.  What do you think?

Comment 7

[chris hofmann]

I think rickg is on vacation.  

nistheeth/harishd?

Comment 8

[Johnny Stenback (:jst)]

The rtf code in mozilla is IMO there for no good reason, it only increases the
complexity and size of the parser, I see no good reasons for having the code in
the tree, and noone is actively maintaining the code either...

IMO it should be removed, if we want it back it's always available in the cvs
repository...

Comment 9

[Boris Zbarsky [:bzbarsky]]

moving out to 0.9.2.  I'd still like to get this in by Friday, though, if people
think that this should happen and like what the patch does... 

Comment 10

[Boris Zbarsky [:bzbarsky]]

Could we get a call one way or the other on this?  To reiterate: as far as I can
tell this patch removes all traces of rtf support in the parser except in files
that are not part of the build.  This is what we want, right?  If not, could we
decide what we _do_ want?

Comment 11

[Heikki Toivonen (remove -bugzilla when emailing directly)]

We want to cvs remove unused files as well. And remove all traces of RTF support
in all the code (if there is no special handling for it it does not need to be
mentioned anywhere).

Comment 12

[Boris Zbarsky [:bzbarsky]]

With the patch in question, there are only five places text/rtf appears in the
code in any form:

1) http://lxr.mozilla.org/seamonkey/source/htmlparser/src/MANIFEST#18

(this should be removed)

2) http://lxr.mozilla.org/seamonkey/source/htmlparser/src/nsParserCIID.h#69

(this can probably be removed)

3) http://lxr.mozilla.org/seamonkey/source/netwerk/mime/public/nsMimeTypes.h#119

(this is used by the next two)

4)
http://lxr.mozilla.org/seamonkey/source/netwerk/mime/src/nsXMLMIMEDataSource.cpp#498
5)
http://lxr.mozilla.org/seamonkey/source/uriloader/exthandler/nsExternalHelperAppService.cpp#102

(these include lists of common mime types such as text/rtf,
application/postscript, and so on.  What should we do with the text/rtf entries
there?  Once we decide on this I will attach an updated patch.)

Whoever is checking in this patch should cvs remove htmlparser/src/CRtfDTD.*

Comments on items 4 and 5?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Pushing out to 0.9.3.  Can someone answer my question regarding points 4 and 5
in my comment from 2001-06-11 11:52 ?

Comment 14

[Heikki Toivonen (remove -bugzilla when emailing directly)]

I am pretty sure you can safely remove the RTF parts in 4) and 5) as well. We
should treat RTF as just any other unknown mime type.

Comment 15

[harishd]

Boris: IMO, all traces of rtf should be yanked. 

CCing gagan,darin, and dougt for more comments. 

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated patch that effaces all traces of text/rtf (and remaining traces of text/cpp)

Comment 17

[Johnny Stenback (:jst)]

sr=jst

Comment 18

[harishd]

r=harishd

Comment 19

[tor]

a=tor for 0.9.2 checkin

Comment 20

[Boris Zbarsky [:bzbarsky]]

My computer is not booting at the moment for unknown reasons.  Could someone
check this in please?  Whoever does so, please also cvs remove
htmlparser/src/CRtfDTD.*

Comment 21

[tor]

This appears to have been checked in on the branch and trunk.
Should this bug be closed?

Comment 22

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Marking fixed, I also saw the checkin.

Comment 23

[jag (Peter Annema)]

Sorry, forgot to close it. Yes, it was checked in.

Comment 24

[bsharma]

Verified on
build: 2001-06-27-04-Trunk
platform: Win NT

Marking it verified as per above developer comments.