Don't use atoms for charset names in nsIDocShell

RESOLVED FIXED in mozilla24

Status

()

Core
Document Navigation
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: hsivonen, Assigned: Michael Brennan)

Tracking

unspecified
mozilla24
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

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

Comment 1

5 years ago
I'm brand new to Firefox development and I'd like to help out with this bug, if that's still possible.
(Reporter)

Comment 2

5 years ago
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.
(Reporter)

Comment 3

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

Comment 4

5 years ago
Created attachment 718038 [details] [diff] [review]
Changes nsIAtom charsets to nsACString

I'm new to mozilla and i've got a feeling my bug fixing failed
Attachment #718038 - Flags: review?(hsivonen)
(Reporter)

Comment 5

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

Comment 6

5 years ago
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?
(Reporter)

Comment 7

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

Comment 8

4 years ago
Created attachment 743295 [details] [diff] [review]
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.
Attachment #743295 - Flags: review?(hsivonen)
I'd rather make this enum because the supported encodings are supposed to be a fixed, finite set now.
(Assignee)

Comment 10

4 years ago
(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.
(Reporter)

Comment 12

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

Comment 13

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

Comment 14

4 years ago
Created attachment 754125 [details] [diff] [review]
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.
Attachment #743295 - Attachment is obsolete: true
Attachment #754125 - Flags: review?(hsivonen)
(Reporter)

Comment 15

4 years ago
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+
(Reporter)

Updated

4 years ago
Attachment #718038 - Attachment is obsolete: true
(Assignee)

Comment 16

4 years ago
(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?
(Reporter)

Comment 17

4 years ago
I'll push the patch to try. If it looks OK, I'll land it.
(Reporter)

Comment 18

4 years ago
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/integration/mozilla-inbound/rev/843f12d62972
Assignee: nobody → brennan.brisad
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/843f12d62972
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.