102437 - [FIX]outdated view source charset handling can be removed

Status: VERIFIED FIXED

(Core :: Internationalization, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

nsXMLDocument.cpp has a lot of charset handling stuff for view-source. 
Unfortunately, view-source is no longer an XML document; it's been an HTML
document for over a year now.  I can't figure out whether this charset detection
needs to be moved over to nsHTMLDocument or just deleted or what....

Comment 1

[Roy Yokoyama]

assigning to ftang

Comment 2

[Frank Tang]

please be specific. 

Comment 3

[Boris Zbarsky [:bzbarsky]]

Do you mean I should be specific as to what code I mean?

Comment 4

[Frank Tang]

yea

Comment 5

[Boris Zbarsky [:bzbarsky]]

http://lxr.mozilla.org/seamonkey/source/content/xml/document/src/nsXMLDocument.cpp#467

The code is:

if(bIsHTML &&(0 == nsCRT::strcmp("view-source", aCommand))) { 
  // only do this for view-source
  //view source is now an XML document and we need to make provisions
  //for the usual charset use when displaying those documents, this 
  //code mirrors nsHTMLDocument.cpp

... about 150 lines of charset detection code ...

}

Actually, the enclosing 

462 nsCOMPtr<nsIContentViewer> cv;
463 docShell->GetContentViewer(getter_AddRefs(cv));
464 if (cv) {

also looks like it could go.

Comment 6

[Frank Tang]

nhotta- can you take a look at this one ?

Comment 7

[nhottanscp]

So is this requesting to use the detection code in nsHTMLDocument for XML?

Comment 8

[Boris Zbarsky [:bzbarsky]]

No.

The code in nsXMLDocument is only used if we are viewing source. That means that
we never use it (since view source now uses an HTML document to show the source,
so XML documents never have aCommand == "view-source").  So it's just code
that's doing nothing.  Therefore we should either remove it (reducing bloat) or
move it over to nsHTMLDocument if that does not contain equivalent detection
(improving correctness).

Comment 9

[nhottanscp]

ok, thanks

Comment 10

[nhottanscp]

Reassign to shanjian.

Comment 11

[rbs]

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

Comment 12

[rbs]

I filed bug 129966 and bzbarsky pointed me to this bug. What is needed is just
to delete a chunk of code (c.f. comment #5) that has become useless as a result
of using another approach for the view-source feature.

Comment 13

[rbs]

It goes back to bug 12502 & bug 72299. The following diff (from bug 12502) shows
exactly the .h files & chunk of code that have become unnecessary.

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/content/xml/document/src&command=DIFF_FRAMESET&file=nsXMLDocument.cpp&rev2=1.54&rev1=1.53

Comment 14

[rbs]

updating title to better reflect the reality.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: remove the junk

I realized today that this is doing some expensive and useless work on every
single XML file (including fun shit like XBL bindings and such).

Comment 16

[Boris Zbarsky [:bzbarsky]]

bbaetz?  rbs?  reviews?

Comment 17

[Bradley Baetz (:bbaetz)]

Comment on attachment 105390 [details] [diff] [review]
remove the junk

r=bbaetz

the non-mine stuff was never being run, because bISHTML should never be true
for an XML document, + the strcmp for viewsource would fail anyway.

Comment 18

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

Comment on attachment 105390 [details] [diff] [review]
remove the junk

sr=heikki

I want more patches like this :)

Comment 19

[rbs]

Is the left-over intentional (e.g., in .h)? (c.f. the bonsai diff in comment #13).

Comment 20

[rbs]

Also, |charsetSource| and the whole chunk of code commented as "check channel's
charset..." seem useless now.

Comment 21

[Boris Zbarsky [:bzbarsky]]

The charset/charsetsource is passed to the parser... I left it in fairly
deliberately, because I can't tell for sure that it's safe to remove it.  Is it?

Comment 22

[Boris Zbarsky [:bzbarsky]]

This comment got trapped in mid-air limbo.. it's a response to comment 19:

more or less, since I also left in the code that gets the charset from the
channel... that part may still make sense in XML that's not view-source.

I landed the patch (before seeing rbs' comment).  Patch to clean up the headers
coming up...

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: more junk-busting

Comment 24

[rbs]

r/sr=rbs if you need it.

Comment 25

[Boris Zbarsky [:bzbarsky]]

I do.  heikki?  Bradley?

Comment 26

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

Comment on attachment 105407 [details] [diff] [review]
more junk-busting

r=heikki

Comment 27

[Boris Zbarsky [:bzbarsky]]

Checked in.  The rest of that code looks like it might actually do something (in
fact it almost certainly does, in the case when the XML is not UTF8 and does not
specify an encoding in the XML decl, which is legal if you specify it in the
HTTP headers).  So marking fixed.

Comment 28

[Yuying Long]

No regression was found with view page source on trunk build, so marking as
verified with this code change bug.