Closed Bug 829603 Opened 7 years ago Closed 7 years ago

Don't use atoms for charset names in nsIDocShell

Categories

(Core :: DOM: Navigation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: hsivonen, Assigned: brennan.brisad)

Details

(Whiteboard: [mentor=hsivonen][lang=C++])

Attachments

(1 file, 2 obsolete files)

nsIDocShell uses an atom for e.g. forcedCharset, which makes no sense. Should be an nsACString.
I'm brand new to Firefox development and I'd like to help out with this bug, if that's still possible.
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.
No need to apply pathes locally anymire since bug 234628 landed.
I'm new to mozilla and i've got a feeling my bug fixing failed
Attachment #718038 - Flags: review?(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.
Attachment #718038 - Flags: review?(hsivonen) → review-
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?
(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.
Hi, I'm new to mozilla development. I've attached a patch for review of this first attempt of mine to fix a bug.
Attachment #743295 - Flags: review?(hsivonen)
I'd rather make this enum because the supported encodings are supposed to be a fixed, finite set now.
(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?
(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 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?
Attachment #743295 - Flags: review?(hsivonen) → review+
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.
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.
Attachment #743295 - Attachment is obsolete: true
Attachment #754125 - Flags: review?(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.
Attachment #754125 - Flags: review?(hsivonen) → review+
Attachment #718038 - Attachment is obsolete: true
(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?
I'll push the patch to try. If it looks OK, I'll land it.
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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/843f12d62972
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.