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

RESOLVED FIXED in mozilla1.8alpha6

Status

SeaMonkey
General
--
minor
RESOLVED FIXED
13 years ago
10 years ago

People

(Reporter: sgautherie, Assigned: sgautherie)

Tracking

({regression})

Trunk
mozilla1.8alpha6
regression
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

13 years ago
[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.
(Assignee)

Updated

13 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.8alpha6
(Assignee)

Comment 1

13 years ago
Created attachment 167182 [details] [diff] [review]
(Av1) <contentAreaUtils.js>
Assignee: general → gautheri
(Assignee)

Updated

13 years ago
Attachment #167182 - Flags: superreview?(jst)
Attachment #167182 - Flags: review?(cbiesinger)
(Assignee)

Updated

13 years ago
Depends on: 263697
Keywords: regression
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...
Attachment #167182 - Flags: review?(cbiesinger) → review?(neil.parkwaycc.co.uk)
(Assignee)

Comment 3

13 years ago
(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 !?)
cc'ing bz bceause he caused this ;)

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

Comment 5

13 years ago
Why not just pass url.originCharset to unEscapeURIForUI?

Comment 6

13 years ago
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

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

Updated

13 years ago
Attachment #167310 - Flags: superreview?(bzbarsky)
Attachment #167310 - Flags: review?(cbiesinger)
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.
Attachment #167310 - Flags: review?(cbiesinger) → review+
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...
Attachment #167310 - Flags: superreview?(bzbarsky) → superreview+

Comment 10

13 years ago
Fix checked in.

The creator appears to be ioService.newURI via makeURI(aURL, aOriginCharset)
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Blocks: 160454, 263697
No longer depends on: 263697
(Assignee)

Updated

13 years ago
Attachment #167182 - Attachment is obsolete: true
Attachment #167182 - Flags: superreview?(jst)
Attachment #167182 - Flags: review?(neil.parkwaycc.co.uk)
(Assignee)

Updated

13 years ago
Attachment #167310 - Attachment description: As per my previous comments → As per my previous comments [Checked in: Comment 10]
Attachment #167310 - Attachment is obsolete: true
(Assignee)

Comment 11

13 years ago
(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
(Assignee)

Updated

12 years ago
Attachment #167310 - Attachment description: As per my previous comments [Checked in: Comment 10] → (Bv1-MAS) As per my previous comments [Checked in: Comment 10]
(Assignee)

Comment 12

12 years ago
Created attachment 177661 [details] [diff] [review]
(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.
Attachment #177661 - Flags: review?(mconnor)
(Assignee)

Updated

12 years ago
Attachment #167310 - Flags: approval1.7.6?
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-
Attachment #167310 - Flags: approval1.7.6? → approval1.7.6-
(Assignee)

Comment 14

12 years ago
(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  :-(

Updated

12 years ago
Attachment #177661 - Flags: review?(mconnor)
(Assignee)

Comment 15

12 years ago
Created attachment 186642 [details] [diff] [review]
(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.
(Assignee)

Updated

12 years ago
Attachment #177661 - Attachment is obsolete: true
Attachment #186642 - Flags: review?(bryner)

Updated

12 years ago
Attachment #167310 - Attachment is obsolete: false
(Assignee)

Comment 16

12 years ago
(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 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).
Attachment #186642 - Flags: review?(bryner)
(Assignee)

Comment 18

12 years ago
(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.
Keywords: helpwanted
Whiteboard: [SergeG: Need help to test Cv1b-FF patch.]
(Assignee)

Comment 19

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

Untested: see comment 18.
Attachment #186642 - Flags: review?(bugs.mano)
Comment on attachment 186642 [details] [diff] [review]
(Cv1b-FF) <contentAreaUtils.js>
[Checkin: See comment 21]

Get this tested first.
Attachment #186642 - Flags: review?(bugs.mano)
(Assignee)

Comment 21

10 years ago
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... :-/
Attachment #186642 - Attachment description: (Cv1b-FF) <contentAreaUtils.js> → (Cv1b-FF) <contentAreaUtils.js> [Checkin: See comment 21]
Attachment #186642 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Blocks: 294754
Keywords: helpwanted
Whiteboard: [SergeG: Need help to test Cv1b-FF patch.]
You need to log in before you can comment on or make changes to this bug.