582195 - Create sane equivalent of ConvertibleToNative() function for frozen linkage

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Jan Horak [:jhorak]]

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&)'

Comment 1

[Jan Horak [:jhorak]]

Attachment: proposed patch 1

Comment 2

[[e-mail]]

Comment on attachment 490841 [details] [diff] [review]
proposed patch 1

I doubt that this is going to do the right thing.

Comment 3

[David :Bienvenu]

Comment on attachment 490841 [details] [diff] [review]
proposed patch 1

why do we think the native strings are utf8?

Comment 4

[David :Bienvenu]

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.

Comment 5

[[e-mail]]

As you pointed out, it will only work when native strings are UTF8.

Comment 6

[David :Bienvenu]

(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 7

[David :Bienvenu]

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.

Comment 8

[Jan Horak [:jhorak]]

Attachment: Removed ConvertibleToNative in favor of frozen linkage [Checked in: Comment 12]

Based on comment #7.

Comment 9

[Jan Horak [:jhorak]]

Can I set checkin-needed or superreview required?

Comment 10

[Ludovic Hirlimann [:Usul]]

(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.

Comment 11

[[e-mail]]

(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.

Comment 12

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/3015fb52fbe9

Comment 13

[[e-mail]]

Sorry about this but I just stumbled over the code that I wanted to use here.

Comment 14

[[e-mail]]

Attachment: Restore ConvertibleToNative

Comment 15

[[e-mail]]

Attachment: Diff against 1.9.1 for comparison [Checked in: Comment 17]

Comment 16

[[e-mail]]

Attachment: Restore ConvertibleToNative [Checked in: Comment 17]

Also removing an additional comment that I overlooked.

Comment 17

[[e-mail]]

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].