829603 - Don't use atoms for charset names in nsIDocShell

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Henri Sivonen (:hsivonen)]

nsIDocShell uses an atom for e.g. forcedCharset, which makes no sense. Should be an nsACString.

Comment 1

[Alex]

I'm brand new to Firefox development and I'd like to help out with this bug, if that's still possible.

Comment 2

[Henri Sivonen (:hsivonen)]

Yes, it’s still possible. I suggest applying the patches from bug 234628 in your local Mercurial Queue (until they land) in order to avoid merge conflicts.

Comment 3

[Henri Sivonen (:hsivonen)]

No need to apply pathes locally anymire since bug 234628 landed.

Comment 4

[khodzha shamir]

Attachment: Changes nsIAtom charsets to nsACString

I'm new to mozilla and i've got a feeling my bug fixing failed

Comment 5

[Henri Sivonen (:hsivonen)]

Comment on attachment 718038 [details] [diff] [review]
Changes nsIAtom charsets to nsACString

Did you try to compile this?

See https://developer.mozilla.org/en-US/docs/Mozilla_internal_string_guide for how to use strings as local variables (no nsCOMPtr) and as arguments.

Comment 6

[khodzha shamir]

Henri, thank you for this guide, but can I ask you some more questions?

Are there any guides about .idl files?
What's the purpose of nsIAtom class?

Comment 7

[Henri Sivonen (:hsivonen)]

(In reply to khodzha shamir from comment #6)
> Are there any guides about .idl files?

https://developer.mozilla.org/en-US/docs/XPIDL

> What's the purpose of nsIAtom class?

nsIAtom represents an interned string. Interned strings can be pointer-compared for equality.

Comment 8

[Michael Hoffmann]

Attachment: Change charset members in nsIDocShell to strings

Hi, I'm new to mozilla development. I've attached a patch for review of this first attempt of mine to fix a bug.

Comment 9

[Masatoshi Kimura [:emk]]

I'd rather make this enum because the supported encodings are supposed to be a fixed, finite set now.

Comment 10

[Michael Hoffmann]

(In reply to Masatoshi Kimura [:emk] from comment #9)
> I'd rather make this enum because the supported encodings are supposed to be
> a fixed, finite set now.

Is this set defined anywhere? I can see that there are some lists in for example /intl/uconv/src/nsUConvModule.cpp or /dom/locales/en-US/chrome/charsetTitles.properties, but I don't know if they are exhaustive. Should this enum be created or is it available somewhere?

Comment 11

[Masatoshi Kimura [:emk]]

(In reply to Michael Brennan from comment #10)
> Is this set defined anywhere? I can see that there are some lists in for
> example /intl/uconv/src/nsUConvModule.cpp or
> /dom/locales/en-US/chrome/charsetTitles.properties, but I don't know if they
> are exhaustive. Should this enum be created or is it available somewhere?

Not exist yet, I meant we should define and use it throughout the tree.
But it will require too much task for the good first bug. We can do that in follow-up bugs. At the moment, go for it with nsCString.

Comment 12

[Henri Sivonen (:hsivonen)]

Comment on attachment 743295 [details] [diff] [review]
Change charset members in nsIDocShell to strings

Thanks!

Did you happen to check if comm-central needs patching, too?

Comment 13

[Michael Hoffmann]

I've checked comm-central now, but all references to forcedCharset and parentCharset I can find are within the mozilla subdirectory. So I guess no patch is needed there as it is the same repo?

Seems like I missed a line in the file content/widgets/browser.xml in my patch. Let me investigate that and run it on the try server so I can get back with an update.

Comment 14

[Michael Hoffmann]

Attachment: Change charset members in nsIDocShell to strings

Same patch but also fixes that line I missed in widgets/browser.xml.

I also was told that I should get this reviewed before running on try.

Comment 15

[Henri Sivonen (:hsivonen)]

Comment on attachment 754125 [details] [diff] [review]
Change charset members in nsIDocShell to strings

(In reply to Michael Brennan from comment #14)
> I also was told that I should get this reviewed before running on try.

Strange advice. Anyway, r+. Thanks.

Comment 16

[Michael Hoffmann]

(In reply to Henri Sivonen (:hsivonen) from comment #15)
> Comment on attachment 754125 [details] [diff] [review]
> Change charset members in nsIDocShell to strings
> 
> (In reply to Michael Brennan from comment #14)
> > I also was told that I should get this reviewed before running on try.
> 
> Strange advice. Anyway, r+. Thanks.

Happy to help :) What should I do now? Is this ready to be set to checkin-needed?

Comment 17

[Henri Sivonen (:hsivonen)]

I'll push the patch to try. If it looks OK, I'll land it.

Comment 18

[Henri Sivonen (:hsivonen)]

This was OK on try, but now the tree is closed. Marking checkin-needed to get this queued up for landing once the tree opens.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/843f12d62972

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/843f12d62972