1074125 - Avoid duplication of labelsencodings.properties in charsetalias.properties

Status: RESOLVED FIXED

(MailNews Core :: Internationalization, defect)

Description

[Henri Sivonen (:hsivonen)]

In order to avoid maintaining a copy of labelsencodings.properties in charsetalias.properties, charsetalias.properties should include only those entries that differ from the Encoding Standard and nsCharsetAlias::GetPreferredInternal should first check charsetalias.properties and if there is no match, continue to call EncodingUtils::EncodingForLabel().

Comment 1

[Magnus Melin [:mkmelin]]

Attachment: bug1074125_avoid_encoding_alias_duplication.patch

For removing duplicate labels I did a
 grep -vxf mozilla/dom/encoding/labelsencodings.properties mailnews/intl/charsetalias.properties

.. and then cleaned up the file some.

Try run @ https://treeherder.mozilla.org/ui/#/jobs?repo=try-comm-central&revision=ec7ab055016e

Comment 2

[Henri Sivonen (:hsivonen)]

Comment on attachment 8523547 [details] [diff] [review]
bug1074125_avoid_encoding_alias_duplication.patch

Assuming that it's intentional not to deal in this patch with encodings whose decoders were removed from m-c but whose decoders haven't (yet?) been put into c-c either or with encodings that became labels of other encodings on m-c, this makes sense.

(I.e. assuming that e.g. leaving iso-8859-6-i=ISO-8859-6-I in place here is intentional and will be dealt with in bug 1069909.)

Comment 3

[Magnus Melin [:mkmelin]]

Yes, I think it's easier to deal with non-duplciated labels elsewhere.

Comment 4

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8523547 [details] [diff] [review]
bug1074125_avoid_encoding_alias_duplication.patch

Review of attachment 8523547 [details] [diff] [review]:
-----------------------------------------------------------------

It pains me to see this as a long list in a properties file still--I would rather see this as a smaller, hard-coded list inside nsCharsetAlias. But I don't think that's feasible unless we get rid of at least the dead charsets and maybe also the identity mapping. (I'm hoping the list boils down to a few extra labels for UTF-7).

r+, on condition of a good answer to the license block question.

::: mailnews/intl/charsetalias.properties
@@ -1,4 @@
> -# This Source Code Form is subject to the terms of the Mozilla Public
> -# License, v. 2.0. If a copy of the MPL was not distributed with this
> -# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> -#

Is deleting the MPL license block intentional? If so, why?

::: mailnews/intl/nsCharsetAlias.cpp
@@ +37,5 @@
> +   if (NS_SUCCEEDED(rv)) {
> +     return NS_OK;
> +   }
> +   return EncodingUtils::FindEncodingForLabel(key, oResult) ?
> +      NS_OK: NS_ERROR_NOT_AVAILABLE;

Personally, I would prefer checking EncodingUtils::FindEncodingForLabel first and then falling back to our list. I'm not entirely sure of what regressions that might end up causing, though (since we use this function to map to keys for other property tables), so I'll avoid requiring that you do that.

Comment 5

[Magnus Melin [:mkmelin]]

(In reply to Joshua Cranmer [:jcranmer] from comment #4)
> Comment on attachment 8523547 [details] [diff] [review]
> bug1074125_avoid_encoding_alias_duplication.patch
> 
> Review of attachment 8523547 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It pains me to see this as a long list in a properties file still--I would
> rather see this as a smaller, hard-coded list inside nsCharsetAlias. But I
> don't think that's feasible unless we get rid of at least the dead charsets
> and maybe also the identity mapping. (I'm hoping the list boils down to a
> few extra labels for UTF-7).
> 
> r+, on condition of a good answer to the license block question.
> 
> ::: mailnews/intl/charsetalias.properties
> @@ -1,4 @@
> > -# This Source Code Form is subject to the terms of the Mozilla Public
> > -# License, v. 2.0. If a copy of the MPL was not distributed with this
> > -# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> > -#
> 
> Is deleting the MPL license block intentional? If so, why?

Ah, good catch. grep ate it (since it was duplicated in the other file)

https://hg.mozilla.org/comm-central/rev/1ea664afd08e -> FIXED

Comment 6

[Henri Sivonen (:hsivonen)]

(In reply to Joshua Cranmer [:jcranmer] from comment #4)
> Personally, I would prefer checking EncodingUtils::FindEncodingForLabel
> first and then falling back to our list.

Looks like the failure to do this caused bug 1174580.