72299 - User could not override charset from http server

Status: VERIFIED FIXED

(Core :: Internationalization, defect)

Description

[Yuying Long]

On 03-16 Mtrunk build.
No matter how you turn off or on(all, Japanese, Chinese...etc) the auto-detect,
and set encoding anything other than EUC-JP (the correct encoding of this page),
e.g. western8859-1, Shift-jis, Big5...etc, the page still display Japanese fine. 
There is a similar bug 72074, but I don't know if they are same problem though.

Comment 1

[nhottanscp]

Reassign to shanjian.

Comment 2

[Shanjian Li]

www.yahoo.co.jp now provide charset information through http header in 
content-type field. That will explain part of the problem.

The observed behavior does reveal one problem, that is the decument charset
could not be override.

Comment 3

[Shanjian Li]

Attachment: patch part1, for html document

Comment 4

[Shanjian Li]

Attachment: patch part 2, to xml document

Comment 5

[Shanjian Li]

There is several changes need to be explained:
1, The layout of "{" and "}" in original file was not right. If you
   search the matched pair of {} in 
   "(kCharsetFromUserDefault > charsetSource)" section, you will see 
   what I mean. That cause this problem.
2, "kCharsetFromPreviousLoading" is ambiguous, "kCharsetFromUserForced"
   is more clear. However, "kCharsetFromUserForced" is not used and 
   "kCharsetFromPreviousLoading" is only referenced here. So I replaced
   "kCharsetFromPreviousLoading"  with "kCharsetFromUserForced".
3, File nsXMLDocument.cpp (Patch part 2) might looks very confusing. The 
   original function is almost unreadable. I relayout the whole function
   while keep the semantic. This file also contains mismatched "{" and "}". 
   

Comment 6

[Shanjian Li]

change summary to better state the problem.

Comment 7

[Shanjian Li]

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

Comment 8

[Jaime Rodriguez, Jr.]

Please fix for M0.9.1 and assigned priority P3.

Comment 9

[nhottanscp]

I am looking at the first patch.
So, he current code the following conditions are applied to more lines of code.
if(kCharsetFromUserDefault > charsetSource)
My question is that, why only overriding the server charset was not working?

Comment 10

[Shanjian Li]

Current code author's original intention is to considering each charset source one by one. 
But because a mismatched "{" and "}", the override charset source is skipped in certain 
condition. 

Comment 11

[nhottanscp]

Yes, I understand that, but my question is how that is related to this bug and
what other cases might be affected by the change.

Comment 12

[Shanjian Li]

I do not quite understand your question. How is this related with this bug? Because 
the override charset is skipped, so when http server provides charset, user could 
not override it. What other case might be affected? Hard to say. If it also changed
other behavior, the bahavior that got changed might be very likely to be a bug.

Comment 13

[nhottanscp]

Let me ask a more specific question. Why pnly http header had a problem and meta
was okay?
Both kCharsetFromHTTPHeader and kCharsetFromMetaTag are larger than
kCharsetFromUserDefault.

 85 typedef enum {
 86 kCharsetUninitialized = 0,
 87 kCharsetFromWeakDocTypeDefault,
 88 kCharsetFromUserDefault ,
 89 kCharsetFromDocTypeDefault,
 90 kCharsetFromCache,
 91 kCharsetFromParentFrame,
 92 kCharsetFromBookmarks,
 93 kCharsetFromAutoDetection,
 94 kCharsetFromMetaTag,
 95 kCharsetFromByteOrderMark,
 96 kCharsetFromHTTPHeader,
 97 kCharsetFromUserForced,
 98 kCharsetFromOtherComponent,
 99 kCharsetFromPreviousLoading
100 } nsCharsetSource;

Comment 14

[nhottanscp]

>Why pnly http
Why only http

Comment 15

[Shanjian Li]

If you take a look at the orginal nsHTMLDocument.cpp befor my patch, in 
MSVC editor search for the ending "}" for line 664's "{", you will see 
that it is below all charset comparisons. Before this line, the only 
possible charset assignment is from http server. That's why if charset
is available in http, we will see the problem.

Comment 16

[nhottanscp]

r=nhotta

Comment 17

[Shanjian Li]

Chris, can you sr this one? I will adjust the brace "{" to end of last line
before check in.

Comment 18

[Jaime Rodriguez, Jr.]

kewl . . . let's fix this one.

Comment 19

[Johnny Stenback (:jst)]

Attach readable diffs (i.e. cvs diff -u) and I will sr this bug.

Comment 20

[Yuying Long]

Actually, there are 2 very popular sites have same problem:
http://www.mainichi.co.jp
http://www.singtao.co.jp

Comment 21

[Shanjian Li]

Attachment: more readable patch, p1

Comment 22

[Shanjian Li]

Attachment: more readable patch, p2

Comment 23

[Shanjian Li]

Johny, I update the patch using -u format. Even with that one, the second patch might still 
looks too complicated. 

Comment 24

[Johnny Stenback (:jst)]

Huh, those new patches are also cvs diff -c patches, and they're hard to read,
but I read the first one and I think I got it. However, I don't approve of this
change as is, the code changes seem ok, but the readability of the code where
those changes are made in nsHTMLDocument.cpp I just can't accept, clean it up!
Get rid of tabs, be consistent with the indentation, use the same brace style as
the rest of the file, i.e.:

  if (...) {
    ...
  }

and not:

  if (...)
  {
    ...
  }

or:

  if (...)
    {
      ...
    }

and so on. Emacs could do that cleanup for you almost automatically...

Do the cleanup, and make sure you really do get diff -u's next time (i.e. the
lines in the diff should be marked with + and -, no changed lines should be
marked with !). And once you're at it, clean up the same code in nsXMLDocument.cpp.

Comment 25

[Shanjian Li]

Attachment: updated part1

Comment 26

[Shanjian Li]

Attachment: updated part 2

Comment 27

[Shanjian Li]

Attachment: repost part2, -u version

Comment 28

[Shanjian Li]

Attachment: repost part2, -ub version

Comment 29

[Shanjian Li]

Johny:

I mistyped  -c for -u when I generating the second batch of patch. Sorry.

For XML document, my original reformated file is missing. So I did one more time of reformatting. 
This time, more extensively that last time. The last 2 part is the -u and -ub version for 
part2. 

Comment 30

[Johnny Stenback (:jst)]

sr=jst

Comment 31

[Brendan Eich [:brendan]]

Still tabs and random indentation:

+                   if(NS_SUCCEEDED(rv) && (nsnull != calias) ) {
+                           nsAutoString preferred;
+                           rv = calias->GetPreferred(theCharset, preferred);
+                           if(NS_SUCCEEDED(rv)) {
+                                   charset = preferred;
+                                   charsetSource = kCharsetFromHTTPHeader;
+          }
+                           nsServiceManager::ReleaseService(kCharsetAliasCID,
calias);

What's the deal?

BTW, use -w over -b if you want to ignore all whitespace differences.

/be

Comment 32

[Shanjian Li]

This is not noticable when I set my tab width to 2. Before I checked in, 
I set tab to 8 and correct all the inconsistence. Now it should be fine.
There should be no tab in that function.

fix checked in.

Comment 33

[Yuying Long]

Checked it on 05-15 trunk build:
http://www.yahoo.co.jp - you can get charset overrided by menu changing.
However, as I montionted on 05-11, http://www.mainichi.co.jp and http://www.singtao.com still see this problem.
I'll re-open it, please change the status if it was cause by something else.

Comment 34

[Shanjian Li]

The 2 websites you listed have meta tag. So the charset information if provided 
by meta tag instead of http server. Please submit new bugs against that. Please 
also note that even thought the display is correct, charset mark in charset menu
is incorrect.

Comment 35

[Yuying Long]

OK, I'll mark it as verified.  Open bug 81244 for the other issue.