259206 - XML prettyprint broken

Status: RESOLVED FIXED

(Core :: XML, defect)

Description

[Johnny Stenback (:jst)]

Using todays nightly build (and older ones too, not sure when it broke), XML
pretty printing isn't working on linux (Fedora Core 2, Gtk2). I tracked this
down to the fact that the system maps .xsl files to "text/x-xslt", and we don't
know anything about that mimetype, and thus can't apply the transformation that
we apply when doing pretty printing of XML. I've got a oneliner that overrides
the mimetype for .xsl files in Firefox that I'll attach for others to comment on.

Comment 1

[Johnny Stenback (:jst)]

Attachment: Override the xsl file extension in the ext helper service.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 158827 [details] [diff] [review]
Override the xsl file extension in the ext helper service.

r=bzbarsky.  Peterv, want to sr?  This basically makes us not ask the OS for
the "xsl" extension -- any time we need to map that extension to a MIME type we
will just get text/xml with this patch.

Comment 3

[Peter Van der Beken [:peterv]]

Comment on attachment 158827 [details] [diff] [review]
Override the xsl file extension in the ext helper service.

I'm assuming this is only for local files.

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

> the system maps .xsl files to "text/x-xslt"

(this is actually a duplicate, of a bug that got marked invalid or wontfix or
something. bug 259100)


Wouldn't a better fix be to set a content type hint for XSLT channels, so that
the helperappservice isn't even involved?

Comment 5

[Boris Zbarsky [:bzbarsky]]

That's true... If we set the type on the channel before calling Open/AsyncOpen,
it will be treated by necko as a hint to be used if the protocol itself does not
provide a type.

Comment 6

[Peter Van der Beken [:peterv]]

So we want to call SetContentType(NS_LITERAL_CSTRING("text/xml")) at least
somewhere around
http://lxr.mozilla.org/seamonkey/source/extensions/transformiix/source/xslt/txMozillaStylesheetCompiler.cpp#523
Trying to think about other load paths that might want it.

Comment 7

[Axel Hecht]

I actually think that the right fix is to not ask gnome for mimetypes anymore, if
they have no clue about it.

Comment 8

[Anne (:annevk)]

Since text/xml will be deprecated somewhere in the future, can't we change this
to application/xml? (text/xml has some charset issues too.) If the patch is
allowed, that is.

(<http://lists.w3.org/Archives/Public/www-html/2004Jul/0004.html>)

Comment 9

[Johnny Stenback (:jst)]

(In reply to comment #8)
> Since text/xml will be deprecated somewhere in the future, can't we change this
> to application/xml? (text/xml has some charset issues too.) If the patch is
> allowed, that is.
> 
> (<http://lists.w3.org/Archives/Public/www-html/2004Jul/0004.html>)

It really doesn't matter in this case, and few will ever see what mimetype we
internally map .xsl files to.

Comment 10

[Peter Van der Beken [:peterv]]

Attachment: Alternate patch

Jst: could you try out this patch? I haven't been able to reproduce this yet so
no idea if it fixes the bug.

Comment 11

[Johnny Stenback (:jst)]

Nope, that does *not* fix the problem :-(

Comment 12

[Johnny Stenback (:jst)]

Attachment: Alternatest patch

This I believe is what we want to do here.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 159026 [details] [diff] [review]
Alternatest patch

r=bzbarsky

Comment 14

[Peter Van der Beken [:peterv]]

Comment on attachment 159026 [details] [diff] [review]
Alternatest patch

Hmm, will that do the right thing? The aChannelIsSync argument when calling
nsSyncLoader::LoadDocument will be false. I think we should make
LoadLocalDocument pass true for aForceToXML.

Comment 15

[Johnny Stenback (:jst)]

Attachment: Force local loads to load XML.

Comment 16

[Johnny Stenback (:jst)]

Comment on attachment 159114 [details] [diff] [review]
Force local loads to load XML.

This attachment is now checked in on the trunk, requesting branch approval.

Comment 17

[Asa Dotzler [:asa]]

Comment on attachment 159114 [details] [diff] [review]
Force local loads to load XML.

a=asa for branch checkin. Did this also break on the 1.7 branch? If so, it
should be landed there too.

Comment 18

[Peter Van der Beken [:peterv]]

Attachment: Additional patch

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 159197 [details] [diff] [review]
Additional patch

r+sr=bzbarsky

Comment 20

[Johnny Stenback (:jst)]

"Force local loads to load XML." landed on the aviary and 1.7 branches (since it
was broken on the 1.7 branch too). Marking FIXED.

Comment 21

[Ali Ebrahim]

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

Comment 22

[Ali Ebrahim]

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

Comment 23

[Joe Drew (not getting mail)]

Is this still an issue? Attachment 159197 [details] [diff] never got checked in to either trunk
or aviary branch, and I know that I've seen this problem on Fedora Core N, N >= 2.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Yeah, reopening.  We need to finish landing the patches for this bug....

Comment 25

[Mike Schroepfer]

Will take patch if checked in but not holding beta for it.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 159197 [details] [diff] [review]
Additional patch

We need to get this fix actually landed...

Comment 27

[Boris Zbarsky [:bzbarsky]]

Fixed again, trunk and 1.8 branch.  Not quite fixed on 1.7, but I don't think we
want to mess with that at this point.