Closed Bug 582195 Opened 10 years ago Closed 9 years ago

Create sane equivalent of ConvertibleToNative() function for frozen linkage

Categories

(MailNews Core :: Backend, defect, minor)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.3a2

People

(Reporter: jhorak, Assigned: neil)

References

Details

Attachments

(3 files, 2 obsolete files)

There are missing NS_CopyUnicodeToNative and NS_CopyNativeToUnicode functions in frozen linkage.
We need to add proper code to compile it successfuly with --enable-incomplete-external-linkage.

comm-central/mailnews/base/util/nsMsgUtils.cpp:376: undefined reference to `NS_CopyUnicodeToNative(nsAString const&, nsACString&)'
comm-central/mailnews/base/util/nsMsgUtils.cpp:377: undefined reference to `NS_CopyNativeToUnicode(nsACString const&, nsAString&)'

Reproduce steps:
compile with --enable-incomplete-external-linkage parameter

Actual results:
comm-central/mailnews/base/util/nsMsgUtils.cpp:376: undefined reference to `NS_CopyUnicodeToNative(nsAString const&, nsACString&)'
comm-central/mailnews/base/util/nsMsgUtils.cpp:377: undefined reference to `NS_CopyNativeToUnicode(nsACString const&, nsAString&)'
Blocks: 377319
Component: General → Backend
Product: Thunderbird → MailNews Core
QA Contact: general → backend
Blocks: 591098
Attached patch proposed patch 1 (obsolete) — Splinter Review
Assignee: nobody → jhorak
Status: NEW → ASSIGNED
Attachment #490841 - Flags: review?(neil)
Comment on attachment 490841 [details] [diff] [review]
proposed patch 1

I doubt that this is going to do the right thing.
Attachment #490841 - Flags: review?(neil) → review?(bienvenu)
Comment on attachment 490841 [details] [diff] [review]
proposed patch 1

why do we think the native strings are utf8?
I see that the old code basically did the same thing - Neil, why do you think this might not do the right thing?

The only use of this function I see is in NS_MsgHashIfNecessary, which is used for creating filenames for folders with illegal characters.
As you pointed out, it will only work when native strings are UTF8.
(In reply to comment #5)
> As you pointed out, it will only work when native strings are UTF8.

The failure case is probably where the string isn't valid utf8, in which case the function would return false, and we would hash. I think the new code does the same thing as the old; the question is, what should the code be doing...
Comment on attachment 490841 [details] [diff] [review]
proposed patch 1

I think I'd prefer that you inline this code the one place ConvertibleToNative is used, so that no one else is tempted to use it. r=me, with that change.
Attachment #490841 - Flags: review?(bienvenu) → review+
Can I set checkin-needed or superreview required?
(In reply to comment #9)
> Can I set checkin-needed or superreview required?

You aren't changing an API or adding one checking needed should be enough.
Keywords: checkin-needed
(In reply to comment #6)
> (In reply to comment #5)
> > As you pointed out, it will only work when native strings are UTF8.
> The failure case is probably where the string isn't valid utf8
The string is UTF16, and so probably will always convert to UTF8, unless there's a bug in our editor and it allows the insertion of lone surrogates.

I can think of two potential failure cases:
1. (Older?) Linux systems with non-UTF8 filesystems.
2. Potential Windows backwards-compatibility issue with previous builds.
If these don't concern you then the code could be removed completely.
Checked in: http://hg.mozilla.org/comm-central/rev/3015fb52fbe9
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.3a2
No longer blocks: 377319
Sorry about this but I just stumbled over the code that I wanted to use here.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Restore ConvertibleToNative (obsolete) — Splinter Review
Attachment #529680 - Flags: review?(dbienvenu)
Also removing an additional comment that I overlooked.
Assignee: jhorak → neil
Attachment #529680 - Attachment is obsolete: true
Attachment #529682 - Flags: review?(dbienvenu)
Attachment #529680 - Flags: review?(dbienvenu)
Attachment #501633 - Attachment description: Removed ConvertibleToNative in favor of frozen linkage → Removed ConvertibleToNative in favor of frozen linkage [Checked in: Comment 12]
Attachment #529682 - Flags: review?(dbienvenu) → review+
Pushed changeset 129468a02962 to both comm-central and comm-2.0 and merged.
This represents attachment 529681 [details] [diff] [review].
The merge changesets represent attachment 529682 [details] [diff] [review].
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Attachment #529681 - Attachment description: Diff against 1.9.1 for comparison → Diff against 1.9.1 for comparison [Checked in: Comment 17]
Attachment #529682 - Attachment description: Restore ConvertibleToNative → Restore ConvertibleToNative [Checked in: Comment 17]
You need to log in before you can comment on or make changes to this bug.