337451 - better charset support (feed preview)

Status: RESOLVED FIXED

(Firefox Graveyard :: RSS Discovery and Preview, defect)

Description

[Ria Klaassen (not reading all bugmail)]

When I look at this feed: http://planet.mozilla.org/rss20.xml , mr Smedberg seems to say weird things, so I want to change the encoding but the page doesn't react.
When I run in in the old pretty print in Firefox 1.5.3 however, I see the right text: It’s not Bias, It’s Discrimination.

Comment 1

[Ria Klaassen (not reading all bugmail)]

Attachment: screenshot of the problem

Comment 2

[Ria Klaassen (not reading all bugmail)]

Attachment: minimum font-size set to 14

This becomes really ugly when you set a minimum font-size of 14 in Options > Content > Fonts & Colors > Advanced.

Comment 3

[Ria Klaassen (not reading all bugmail)]

(In reply to comment #2)
> Created an attachment (id=221684) [edit]
> minimum font-size set to 14
> 
> This becomes really ugly when you set a minimum font-size of 14 in Options >
> Content > Fonts & Colors > Advanced.
>
Sorry, posted in the wrong bug :( 

Comment 4

[Robert Sayre]

http://benjamin.smedbergs.us/blog/feed/

wfm. planet has encoding problems. Not sure changing the encoding will help here, since the feed preview will always be utf-8.

Comment 5

[Ria Klaassen (not reading all bugmail)]

Attachment: screenshot

This piece of French text looks alien. A big part of the words look like curses. Sounds very weird to me if this can't be solved.

Comment 6

[Phil Ringnalda (:philor)]

Well, there's "can't be solved" and then there's "that's not the way to solve it."

Filed bug 339409 against planet.m.o, which is sending UTF-8 as text/xml with no charset param and no encoding in the XML decl, probably the very worst possible way to send XML.

Rob, am I guessing right from our mangled characters, that we're using ISO-8859-1 as a default in the absence of a charset? That doesn't seem like a very happy choice, if we can avoid it.

Comment 7

[Robert Sayre]

(In reply to comment #6)
> 
> Rob, am I guessing right from our mangled characters, that we're using
> ISO-8859-1 as a default in the absence of a charset? That doesn't seem like a
> very happy choice, if we can avoid it.

I suspect we are sniffing it, as there are cp-1252 characters near the beginning.
http://feedvalidator.org/check.cgi?url=http%3A%2F%2Fplanet.mozilla.org%2Frss20.xml

I will try that feed with those chars edited out.

Comment 8

[Robert Sayre]

Attachment: sample xml

Comment 9

[Robert Sayre]

OK, planet is totally busted, but we can do better. 

Comment 10

[Robert Sayre]

Attachment: Use TryDocumentCharset the way nsXMLDocument does

> Rob, am I guessing right from our mangled characters, that we're using
> ISO-8859-1 as a default in the absence of a charset?

Phil wins. That's exactly what was happening, if I understand nsParser. This patch  will silently drop the 1252 characters (there were only a few), just like our view-source view does (fair enough, they are control characters in utf-8).

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 223568 [details] [diff] [review]
Use TryDocumentCharset the way nsXMLDocument does

>Index: parser/xml/src/nsSAXXMLReader.cpp
> nsSAXXMLReader::OnStartRequest(nsIRequest *aRequest, nsISupports *aContext)
> {
>   NS_ENSURE_TRUE(mIsAsyncParse, NS_ERROR_FAILURE);
>-  nsresult rv = InitParser(mParserObserver);
>+  nsresult rv;
>+  nsCOMPtr<nsIChannel> channel = do_QueryInterface(aRequest);
>+  if (channel)
>+    rv = InitParser(mParserObserver, channel);
>+  else
>+    rv = InitParser(mParserObserver, nsnull);

This looks a lot like just doing |rv = InitParser(mParserObserver, channel);| always :)


>+nsSAXXMLReader::TryChannelCharset(nsIChannel *aChannel,
>+                                  PRInt32& aCharsetSource,
>+                                  nsACString& aCharset)
>+{
>+  if (kCharsetFromChannel <= aCharsetSource) {

Please switch these two around, that makes it easier to read IMHO.

With both those fixed, r=me

Comment 12

[Robert Sayre]

Attachment: address comments from sicking

Comment 13

[Peter Van der Beken [:peterv]]

Comment on attachment 224378 [details] [diff] [review]
address comments from sicking

>+// from nsDocument.cpp

Too bad we can't share this :-(.

>+nsSAXXMLReader::TryChannelCharset(nsIChannel *aChannel,

>+        rv = calias->GetPreferred(charsetVal,
>+                                  preferred);

This can go on one line.

Comment 14

[Robert Sayre]

Checked in on trunk.

Checking in parser/xml/src/Makefile.in;
/cvsroot/mozilla/parser/xml/src/Makefile.in,v  <--  Makefile.in
new revision: 1.3; previous revision: 1.2
done
Checking in parser/xml/src/nsSAXXMLReader.h;
/cvsroot/mozilla/parser/xml/src/nsSAXXMLReader.h,v  <--  nsSAXXMLReader.h
new revision: 1.3; previous revision: 1.2
done
/cvsroot/mozilla/parser/xml/src/nsSAXXMLReader.cpp,v  <--  nsSAXXMLReader.cpp
new revision: 1.7; previous revision: 1.6
done

Comment 15

[Robert Sayre]

Attachment: patch I checked in

Comment 16

[Robert Sayre]

Checking in parser/xml/src/Makefile.in;
/cvsroot/mozilla/parser/xml/src/Makefile.in,v  <--  Makefile.in
new revision: 1.2.2.3; previous revision: 1.2.2.2
done
Checking in parser/xml/src/nsSAXXMLReader.cpp;
/cvsroot/mozilla/parser/xml/src/nsSAXXMLReader.cpp,v  <--  nsSAXXMLReader.cpp
new revision: 1.6.2.4; previous revision: 1.6.2.3
done
Checking in parser/xml/src/nsSAXXMLReader.h;
/cvsroot/mozilla/parser/xml/src/nsSAXXMLReader.h,v  <--  nsSAXXMLReader.h
new revision: 1.2.2.4; previous revision: 1.2.2.3
done