Last Comment Bug 829603 - Don't use atoms for charset names in nsIDocShell
: Don't use atoms for charset names in nsIDocShell
Status: RESOLVED FIXED
[mentor=hsivonen][lang=C++]
:
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla24
Assigned To: Michael Brennan
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-01-11 07:15 PST by Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03)
Modified: 2013-05-30 09:11 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Changes nsIAtom charsets to nsACString (1.65 KB, patch)
2013-02-25 13:08 PST, khodzha shamir
hsivonen: review-
Details | Diff | Splinter Review
Change charset members in nsIDocShell to strings (7.48 KB, patch)
2013-04-29 14:46 PDT, Michael Brennan
hsivonen: review+
Details | Diff | Splinter Review
Change charset members in nsIDocShell to strings (8.25 KB, patch)
2013-05-25 07:18 PDT, Michael Brennan
hsivonen: review+
Details | Diff | Splinter Review

Description Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03) 2013-01-11 07:15:24 PST
nsIDocShell uses an atom for e.g. forcedCharset, which makes no sense. Should be an nsACString.
Comment 1 Alex 2013-01-16 19:48:32 PST
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) (Not reading bugmail or doing reviews until 2016-10-03) 2013-01-17 00:17:34 PST
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) (Not reading bugmail or doing reviews until 2016-10-03) 2013-02-03 03:21:04 PST
No need to apply pathes locally anymire since bug 234628 landed.
Comment 4 khodzha shamir 2013-02-25 13:08:50 PST
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
Comment 5 Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03) 2013-02-26 23:20:32 PST
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 2013-03-21 13:05:18 PDT
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) (Not reading bugmail or doing reviews until 2016-10-03) 2013-03-25 07:16:38 PDT
(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 Brennan 2013-04-29 14:46:16 PDT
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.
Comment 9 Masatoshi Kimura [:emk] 2013-04-29 20:58:04 PDT
I'd rather make this enum because the supported encodings are supposed to be a fixed, finite set now.
Comment 10 Michael Brennan 2013-05-02 12:50:50 PDT
(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] 2013-05-02 15:09:04 PDT
(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) (Not reading bugmail or doing reviews until 2016-10-03) 2013-05-10 04:59:09 PDT
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 Brennan 2013-05-15 14:51:57 PDT
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 Brennan 2013-05-25 07:18:02 PDT
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.
Comment 15 Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03) 2013-05-26 23:31:58 PDT
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 Brennan 2013-05-27 06:41:20 PDT
(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) (Not reading bugmail or doing reviews until 2016-10-03) 2013-05-28 00:02:42 PDT
I'll push the patch to try. If it looks OK, I'll land it.
Comment 18 Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03) 2013-05-29 04:55:18 PDT
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] 2013-05-29 17:40:27 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/843f12d62972
Comment 20 Ryan VanderMeulen [:RyanVM] 2013-05-30 09:11:00 PDT
https://hg.mozilla.org/mozilla-central/rev/843f12d62972

Note You need to log in before you can comment on or make changes to this bug.