271981 - In <contentAreaUtils.js>, "Warning: redeclaration of var charset"

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8a5) Gecko/20041122] (release) (W98SE)

{{
Warning: redeclaration of var charset
Source File: chrome://communicator/content/contentAreaUtils.js
Line: 735, Column: 10
Source Code:
      var charset = getCharsetforSave(aDocument);
}}

Note: in MAS file only, not present in FireFox file.

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) <contentAreaUtils.js>

Comment 2

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

Comment on attachment 167182 [details] [diff] [review]
(Av1) <contentAreaUtils.js>

why not also move the getCharsetforSave call to the top of the function?

I think I'd prefer neil to review this...

Comment 3

[Serge Gautherie (:sgautherie)]

(In reply to comment #2)
> (From update of attachment 167182 [details] [diff] [review])
> why not also move the getCharsetforSave call to the top of the function?

Why not, indeed ! (I'm not the one who wants to decide. Neil !?)

Comment 4

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

cc'ing bz bceause he caused this ;)

cc'ing neil so he sees the above question...

Comment 5

[[e-mail]]

Why not just pass url.originCharset to unEscapeURIForUI?

Comment 6

[[e-mail]]

In fact, we should never be trying to unescape it as UTF-8 (unless the origin
character set was indeed UTF-8 of course), should we?

Comment 7

[[e-mail]]

Attachment: (Bv1-MAS) As per my previous comments [Checked in: Comment 10]

Comment 8

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

Comment on attachment 167310 [details] [diff] [review]
(Bv1-MAS) As per my previous comments
[Checked in: Comment 10]

r=biesi. I'm not sure whether all callers do create the uri with a charset, but
if they don't, that's a separate bug that should be fixed.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 167310 [details] [diff] [review]
(Bv1-MAS) As per my previous comments
[Checked in: Comment 10]

sr=bzbarsky, but please do check where that object comes from.	I'm 99% sure
that the caller messes up URI creation...

Comment 10

[[e-mail]]

Fix checked in.

The creator appears to be ioService.newURI via makeURI(aURL, aOriginCharset)

Comment 11

[Serge Gautherie (:sgautherie)]

(In reply to comment #7)
> Created an attachment (id=167310)
> As per my previous comments

Neil:
Shouldn't your fix be ported to FireFox too ?
http://lxr.mozilla.org/mozilla/source/browser/base/content/contentAreaUtils.js#733

Comment 12

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1-FF) <contentAreaUtils.js>

Bv1 port to FireFox.

I don't know the steps to test this one:
Could you test/review/check in this patch ? Thanks.

Comment 13

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 167310 [details] [diff] [review]
(Bv1-MAS) As per my previous comments
[Checked in: Comment 10]

Please stop asking for branch approval on strict warning patches.  a-

Comment 14

[Serge Gautherie (:sgautherie)]

(In reply to comment #13)
> (From update of attachment 167310 [details] [diff] [review] [edit])
> Please stop asking for branch approval on strict warning patches.  a-

I was not complaining about the warning is the JS.Console (anymore):
I though you would be interested in Neil's fix about the charset handling.
But it seems I was wrong again  :-(

Comment 15

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1b-FF) <contentAreaUtils.js> [Checkin: See comment 21]

Cv1 updated to current Trunk. (Bv1 port to FireFox)

I don't know the steps to test this one:
Could you test/review/check in this patch ? Thanks.

Comment 16

[Serge Gautherie (:sgautherie)]

(In reply to comment #15)
> Created an attachment (id=186642) [edit]
> (Cv1b-FF) <contentAreaUtils.js>

You(/Toolkit guys) might want this for v1.8b3 !?

Comment 17

[Brian Ryner (not reading)]

Comment on attachment 186642 [details] [diff] [review]
(Cv1b-FF) <contentAreaUtils.js>
[Checkin: See comment 21]

I don't have a lot of time to spend on this and so I can't confidently say this does the right thing... can you ask someone else to review?

It would help a lot if you could test your patches before requesting reviews, that's standard coding practice (reviewers aren't your testing service).

Comment 18

[Serge Gautherie (:sgautherie)]

(In reply to comment #17)
> (From update of attachment 186642 [details] [diff] [review] [edit])
> can you ask someone else to review?

Would you have a name to suggest ?

> It would help a lot if you could test your patches before requesting reviews,
> that's standard coding practice (reviewers aren't your testing service).

I know but, while I have the minimal skill to prepare this patch, I'm not a FireFox user and don't know how to test this fix.
Yet I could follow steps if someone provided them.

Helpwanted.

Comment 19

[Serge Gautherie (:sgautherie)]

Comment on attachment 186642 [details] [diff] [review]
(Cv1b-FF) <contentAreaUtils.js>
[Checkin: See comment 21]

Untested: see comment 18.

Comment 20

[Asaf Romano (gone)]

Comment on attachment 186642 [details] [diff] [review]
(Cv1b-FF) <contentAreaUtils.js>
[Checkin: See comment 21]

Get this tested first.

Comment 21

[Serge Gautherie (:sgautherie)]

Comment on attachment 186642 [details] [diff] [review]
(Cv1b-FF) <contentAreaUtils.js>
[Checkin: See comment 21]

Actually, this was included in
[
1.71	mozilla.mano%sent.com	2005-06-28 07:41	 	Bug 294759 - port various contentAreaUtils fixes ('Save As', etc.) to the toolkit version. r=mconnor, a=bsmedberg.
]
in the meantime... :-/