1147526 - Port Bug 1147311: migrateUI() should migrate font.language.group to a supported value

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Comment 1

[aleth [:aleth]]

Attachment: migratelangggroup.diff

Comment 2

[:aceman]

Comment on attachment 8583243 [details] [diff] [review]
migratelangggroup.diff

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

::: mail/base/modules/mailMigrator.js
@@ +310,5 @@
>          foStream.QueryInterface(Ci.nsISafeOutputStream).finish();
>          foStream.close();
>        }
>  
> +      if (currentUIVersion < 11) {

Please add a comment before this line documenting what this migration does and why. As all other versions above do. You have some comment later in the code so it could be moved here and completed.

Comment 3

[aleth [:aleth]]

(In reply to :aceman from comment #2)
> Please add a comment before this line documenting what this migration does
> and why. As all other versions above do. You have some comment later in the
> code so it could be moved here and completed.

This was just a straight port of the m-c patch. I didn't request review because bug 1147311 has not been checked in yet.

Comment 4

[aleth [:aleth]]

Attachment: migratelangggroup.diff v2

Matches version that is going to land on m-c, with comment moved as recommended by aceman.

Comment 5

[:aceman]

Comment on attachment 8584095 [details] [diff] [review]
migratelangggroup.diff v2

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

This works for me feature-wise.

::: mail/base/modules/mailMigrator.js
@@ +310,5 @@
>          foStream.QueryInterface(Ci.nsISafeOutputStream).finish();
>          foStream.close();
>        }
>  
> +      // Latin language groups were consolidated.

Please make this more explaining, like "Several Latin language groups were consolidated into x-western".

@@ +318,5 @@
> +          group = Services.prefs.getComplexValue("font.language.group",
> +                                                 Ci.nsIPrefLocalizedString);
> +        } catch (ex) {}
> +        if (group &&
> +            ["tr", "x-baltic", "x-central-euro"].some(g => g == group.data)) {

Would [].contains(group.data) work and be more readable?

Comment 6

[:aceman]

(In reply to :aceman from comment #5)
> Would [].contains(group.data) work and be more readable?
Sorry, [].includes().

Comment 7

[aleth [:aleth]]

Attachment: migratelangggroup.diff v3

Amended comment.

> @@ +318,5 @@
> > +          group = Services.prefs.getComplexValue("font.language.group",
> > +                                                 Ci.nsIPrefLocalizedString);
> > +        } catch (ex) {}
> > +        if (group &&
> > +            ["tr", "x-baltic", "x-central-euro"].some(g => g == group.data)) {
> 
> Would [].contains(group.data) work and be more readable?

Apart from the fact that I really dislike changing ported code as it makes future comparisons with m-c much harder, Array.includes is nightly-only.

Comment 8

[:aceman]

Comment on attachment 8584118 [details] [diff] [review]
migratelangggroup.diff v3

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

They could have at least used .indexOf(). This .some() construct seems unnecessarily complicated.

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 8584118 [details] [diff] [review]
migratelangggroup.diff v3

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

Looks good, thx! r=mkmelin

Comment 10

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/45366cd2d72a

Comment 11

[aleth [:aleth]]

Comment on attachment 8584118 [details] [diff] [review]
migratelangggroup.diff v3

[Approval Request Comment]
[Feature/regressing bug #]: 756022
[User impact if declined]: This prevents possible old pref values in affected locales

Comment 12

[aleth [:aleth]]

https://hg.mozilla.org/releases/comm-aurora/rev/9e636fb9a213