943262 - Don't use charsetTitles.properties on Android, only allow Character Encoding menu items that desktop allows

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Henri Sivonen (:hsivonen)]

Firefox for Android relies on charsetTitles.properties for UI strings for the Character Encoding menu. Once bug 936442 lands, this property file will become dead code for desktop Firefox and is going away in bug 943252.

For desktop Firefox, bug 805374 introduced a new property file, browser/locales/en-US/chrome/browser/charsetMenu.properties that contain less cluttered UI strings that make sense for a menu that only include items that are in the list given in browser/modules/CharsetMenu.jsm . Firefox for Android should stop showing encodings that aren't on that list in the menu anyway (e.g. bug 832917).

Please make sure Firefox for Android never lists encodings that aren't on the list that's in browser/modules/CharsetMenu.jsm and please use browser/locales/en-US/chrome/browser/charsetMenu.properties as the source of UI strings for those encodings.

Comment 1

[Henri Sivonen (:hsivonen)]

The new files mentioned in comment 0 have moved to toolkit:
https://mxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/global/charsetMenu.properties
https://mxr.mozilla.org/mozilla-central/source/toolkit/modules/CharsetMenu.jsm

Comment 2

[:Margaret Leibovic]

Attachment: WIP

This removes our dependence on charsetTitles.properties, and instead uses charsetMenu.properties. I decided not to use CharsetMenu.jsm, since it looks like that was designed for building a XUL menu, which isn't useful to us.

I removed iso-8859-1 from the "intl.charsetmenu.browser.static" pref, since it's not included in charsetMenu.properties, so I assume it's not a supported encoding.

I'm not familiar with this history our of character encoding menu, so I have some questions:

* Why are we using our own "intl.charsetmenu.browser.static" pref, instead of using the one in toolkit? http://mxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/global/intl.properties#54

* Why do we have this normalizeCharsetCode function? Was that just to get the correct string for the label?

* Are these overrides in mobile/android/locales/jar.mn even used? It looks like there are some declarations in there we could get rid of.

* What's a good page to test that changing the character encoding actually worked?

Comment 3

[Henri Sivonen (:hsivonen)]

(In reply to :Margaret Leibovic from comment #2)
> This removes our dependence on charsetTitles.properties, and instead uses
> charsetMenu.properties.

This implements the "Don't use charsetTitles.properties on Android" part but it doesn't implement the "only allow Character Encoding menu items that desktop allows" part.

> I decided not to use CharsetMenu.jsm, since it looks
> like that was designed for building a XUL menu, which isn't useful to us.

Reusing the non-XUL-specific code from CharsetMenu.jsm would be useful for two reasons:

 a) CharsetMenu.jsm knows that the localization key for gbk isn't "gbk" but "gbk.bis". I.e. due to an unfortunate sequence of events, the localization key isn't the Gecko-canonical encoding name in all cases.

 b) CharsetMenu.jsm knows what the whitelist for encodings that may appear in the menu is and knows how to sort the menu. Using the whitelist would automatically fix bug 832917. Note that the property file does not have titles for encodings that aren't on the whitelist.

> * Why are we using our own "intl.charsetmenu.browser.static" pref, instead
> of using the one in toolkit?
> http://mxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/
> global/intl.properties#54

FWIW, the one in toolkit is no longer relevant for Firefox for desktop, since the desktop no longer needs to hoist some encodings to the top level of the menu, because the sublevels are now gone. intl.charsetmenu.browser.static will probably soon stop being relevant to SeaMonkey, too.

> * What's a good page to test that changing the character encoding actually
> worked?

http://hsivonen.com/test/moz/charset-menu/test-1-nothing-declared-ascii-compatible.htm

Comment 4

[:Margaret Leibovic]

Attachment: patch

This patch just updates the character encoding menu logic to use CharsetMenu.jsm to get the strings, but it does not change how we choose which encodings we choose to show in the menu, so it doesn't fix bug 832917.

It definitely seems like there's room for improvement here, but I don't want to expand the scope of this bug too much.

Comment 5

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #4)
> Created attachment 8389368 [details] [diff] [review]
> patch
> 
> This patch just updates the character encoding menu logic to use
> CharsetMenu.jsm to get the strings, but it does not change how we choose
> which encodings we choose to show in the menu, so it doesn't fix bug 832917.
> 
> It definitely seems like there's room for improvement here, but I don't want
> to expand the scope of this bug too much.

Although I just realized I still failed to implement the "only allow Character Encoding menu items that desktop allows" part here. I'm not sure the best way to go about doing this, I feel like we'd need to get rid of intl.charsetmenu.browser.static, and just use the encodings specified in CharsetMenu.jsm, although that would make a huge list, which isn't as mobile-friendly.

I suppose we could also just check to see if the encodings we read out of the pref are in the set in CharsetMenu.jsm, perhaps we should consider adding a new method to CharsetMenu that filters an array of encodings that we read from a pref.

Comment 6

[:Margaret Leibovic]

Comment on attachment 8389368 [details] [diff] [review]
patch

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

::: mobile/android/locales/en-US/chrome/browser.properties
@@ +154,5 @@
>  
>  # LOCALIZATION NOTE (intl.charsetmenu.browser.static): Set to a series of comma separated
>  # values for charsets that the user can select from in the Character Encoding menu.
> +# These values should match keys in toolkit/locales/.../chrome/global/charsetMenu.properties
> +intl.charsetmenu.browser.static=UTF-8,Big5,ISO-2022-JP,Shift_JIS,EUC-JP

I now realize we probably need to rev this entity. I wonder if there's any way we can avoid using this pref altogether and just match what desktop does. I assume this was added to avoid showing a giant long list of character encodings, but maybe that's not so bad? Hopefully someone with more historical context can chime in here.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Based on bug 611580 comment 26, I think it was just an attempt to be consistent with desktop - though desktop kind of uses that pref differently, so not sure the consistency was useful.

Desktop moved away from having the list of encodings be localized, so I think Android should too. Not sure what to suggest about you wanting a shorter list - for the menu to be truly useful you kind of do need all the entries.

Comment 8

[:Margaret Leibovic]

(In reply to :Gavin Sharp (email [e-mail]) from comment #7)
> Based on bug 611580 comment 26, I think it was just an attempt to be
> consistent with desktop - though desktop kind of uses that pref differently,
> so not sure the consistency was useful.
> 
> Desktop moved away from having the list of encodings be localized, so I
> think Android should too. Not sure what to suggest about you wanting a
> shorter list - for the menu to be truly useful you kind of do need all the
> entries.

I just made an assumption about us wanting a shorter list, but it looks like that's actually an incorrect assumption if we were just trying to match old desktop behavior.

I would be in favor of getting rid of this pref and just totally matching desktop.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to :Margaret Leibovic from comment #8)

> I just made an assumption about us wanting a shorter list, but it looks like
> that's actually an incorrect assumption if we were just trying to match old
> desktop behavior.
> 
> I would be in favor of getting rid of this pref and just totally matching
> desktop.

Agreed. Let's just match desktop.

Comment 10

[:Margaret Leibovic]

Attachment: patch that matches desktop

It feels good to remove things. I also agree with Gavin that the menu is probably more useful if you have all the items in it.

Since desktop separates "pinned" charsets from "other" charsets, I decided to just leave those at the top.

Comment 11

[Brian Nicholson (:bnicholson)]

Comment on attachment 8390972 [details] [diff] [review]
patch that matches desktop

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

::: mobile/android/chrome/content/browser.js
@@ -6409,4 @@
>      let selected = 0;
>      let charsetCount = this._charsets.length;
>      for (; selected < charsetCount && this._charsets[selected].code != docCharset; selected++);
> -    if (selected == charsetCount) {

Is it possible that the document can specify a charset we don't support? If so, we probably want to keep some kind of safety check here; otherwise, we'll crash in Java when we try to use an invalid selected index.

As a fallback, maybe we can just set the selected index to 0 if the document charset isn't in our list.

Comment 12

[:Margaret Leibovic]

(In reply to Brian Nicholson (:bnicholson) from comment #11)
> Comment on attachment 8390972 [details] [diff] [review]
> patch that matches desktop
> 
> Review of attachment 8390972 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/chrome/content/browser.js
> @@ -6409,4 @@
> >      let selected = 0;
> >      let charsetCount = this._charsets.length;
> >      for (; selected < charsetCount && this._charsets[selected].code != docCharset; selected++);
> > -    if (selected == charsetCount) {
> 
> Is it possible that the document can specify a charset we don't support? If
> so, we probably want to keep some kind of safety check here; otherwise,
> we'll crash in Java when we try to use an invalid selected index.
>
> As a fallback, maybe we can just set the selected index to 0 if the document
> charset isn't in our list.

Hm... if this is the case, and the user opens and closes the charset menu, won't we then re-set the character encoding to that first value? I suppose if the document charset is set to one we don't support, it's already broken, so this is an edge case anyway.

Comment 13

[:Margaret Leibovic]

Attachment: (v2) patch that matches desktop

Updated the patch to default selected to -1, which means that no items will be selected. I think this is more accurate for the user.

I haven't had a chance to test this yet.

Comment 14

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #13)
> Created attachment 8391257 [details] [diff] [review]
> (v2) patch that matches desktop
> 
> Updated the patch to default selected to -1, which means that no items will
> be selected. I think this is more accurate for the user.
> 
> I haven't had a chance to test this yet.

I verified this works (including a manual hack to see what happens when selected is -1).

https://hg.mozilla.org/integration/fx-team/rev/e7f7cc81c103

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed this out in https://hg.mozilla.org/integration/fx-team/rev/2569cd05832a for breaking tests: 
https://tbpl.mozilla.org/php/getParsedLog.php?id=36159414&tree=Fx-Team
and
https://tbpl.mozilla.org/php/getParsedLog.php?id=36159718&tree=Fx-Team

Comment 16

[Henri Sivonen (:hsivonen)]

The line
% override chrome://global/locale/charsetTitles.properties chrome://browser/locale/overrides/charsetTitles.properties
in mobile/android/locales/jar.mn should go away, too, right?

As for the failing test, we should remove it form m-c (potentially move it to c-c).

Comment 17

[:Margaret Leibovic]

(In reply to Henri Sivonen (:hsivonen) (Unable to do reviews or read bugmail right now.)) from comment #16)
> The line
> % override chrome://global/locale/charsetTitles.properties
> chrome://browser/locale/overrides/charsetTitles.properties
> in mobile/android/locales/jar.mn should go away, too, right?

Good catch! Thanks.

> As for the failing test, we should remove it form m-c (potentially move it
> to c-c).

Can you review this change? I'm not familiar with this part of the tree.

Here's a try push with that test removed:
https://tbpl.mozilla.org/?tree=Try&rev=332d63ec546d

Comment 18

[:Margaret Leibovic]

Attachment: Remove test_bug713519

My try run was green.

Please redirect if you're not an appropriate reviewer.

Comment 19

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8392598 [details] [diff] [review]
Remove test_bug713519

Should the getCharsetAlias part of this test remain, since that isn't being removed by bug 943252's patch?

Comment 20

[:Margaret Leibovic]

Attachment: Remove getCharsetTitle part of test_bug713519.js

Good call, you're right that we only need to remove that one part.

Green on try:
https://tbpl.mozilla.org/?tree=Try&rev=c5e8d883c430

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

This is tracking Fx29.

Comment 22

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #21)
> This is tracking Fx29.

Is there any reason this would need to actually be in 29? I think it's just tracking 29 because that's when bug 936442 landed, but I think it would be fine to just land this now and let it ride the trains.

Comment 23

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8393896 [details] [diff] [review]
Remove getCharsetTitle part of test_bug713519.js

Just wanted to note for the record why this is needed now rather than waiting for bug 943252 (i.e. why a fennec-only change is requiring a core test change), since I was confused:

Fennec is only shipping the toolkit/core l10n files it needs since bug 792077, so removing the explicit inclusion of charsetTitles.properties from Fennec breaks the core getCharsetTitle method that depends on it. Desktop still ships charsetTitles.properties despite not directly using it (at least until bug 943252), so this test (test_bug713519.js) isn't failing there.

Comment 24

[:Gavin Sharp [email: [e-mail]]]

(In reply to :Margaret Leibovic from comment #22)
> Is there any reason this would need to actually be in 29? I think it's just
> tracking 29 because that's when bug 936442 landed, but I think it would be
> fine to just land this now and let it ride the trains.

I agree it's not strictly needed (bug 943252 is not a critical fix), but it would be nice for mobile to be consistent with Desktop in 29.

Comment 25

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/632c0d6e8647
https://hg.mozilla.org/integration/fx-team/rev/8f0786e1655c

Comment 26

[:Margaret Leibovic]

Attachment: folded patch for uplift

Here's a folded patch that leaves the unused intl.charsetmenu.browser.static entity in browser.properties to avoid l10n churn.

We wait to verify this on nightly before uplifting, but I wanted to request approval before forgetting.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): desktop character encoding menu changes in bug 936442
User impact if declined: inconsistency with desktop character encoding menu
Testing completed (on m-c, etc.): just landed on fx-team
Risk to taking this patch (and alternatives if risky): low-risk, changes how we create the character encoding menu (actually better tested now that we're sharing more of the desktop implementation)
String or IDL/UUID changes made by this patch: none

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/632c0d6e8647
https://hg.mozilla.org/mozilla-central/rev/8f0786e1655c

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5f7077cb406a
https://hg.mozilla.org/releases/mozilla-beta/rev/b84e23f32f81

Comment 29

[:Margaret Leibovic]

This was backed out of mozilla-release for causing bug 1003897:
http://hg.mozilla.org/releases/mozilla-release/rev/6b14de55b252

Comment 30

[u427052]

i use this in my add-on :
 
pub.getDefaultCharsets = function() {
var prefs = Components.classes["@mozilla.org/preferences-service;1"]
                .getService(Components.interfaces.nsIPrefService)
                .getBranch("");
var charsets = prefs.getComplexValue("intl.charsetmenu.browser.static",
                                         Components.interfaces.nsIPrefLocalizedString);
return charsets.toString().split(", ");
}

but since "intl.charsetmenu.browser.static" has disappear how to get default charset now ?

Comment 31

[:Margaret Leibovic]

(In reply to James Cole from comment #30)
> i use this in my add-on :
>  
> pub.getDefaultCharsets = function() {
> var prefs = Components.classes["@mozilla.org/preferences-service;1"]
>                 .getService(Components.interfaces.nsIPrefService)
>                 .getBranch("");
> var charsets = prefs.getComplexValue("intl.charsetmenu.browser.static",
>                                         
> Components.interfaces.nsIPrefLocalizedString);
> return charsets.toString().split(", ");
> }
> 
> but since "intl.charsetmenu.browser.static" has disappear how to get default
> charset now ?

It looks like that would give you a list of charsets, right? If that's what you're looking for, you can use CharsetMenu.jsm. See these lines for a sample of how we do this in the browser:
https://hg.mozilla.org/releases/mozilla-aurora/rev/5f7077cb406a#l3.81

The diff actually shows that we used to do something just like your add-on code.

Comment 32

[Henri Sivonen (:hsivonen)]

(In reply to James Cole from comment #30)
> i use this in my add-on :
>  
> pub.getDefaultCharsets = function() {
> var prefs = Components.classes["@mozilla.org/preferences-service;1"]
>                 .getService(Components.interfaces.nsIPrefService)
>                 .getBranch("");
> var charsets = prefs.getComplexValue("intl.charsetmenu.browser.static",
>                                         
> Components.interfaces.nsIPrefLocalizedString);
> return charsets.toString().split(", ");
> }
> 
> but since "intl.charsetmenu.browser.static" has disappear how to get default
> charset now ?

The old default lived in intl.charset.default, so the above code has always been inappropriate if the purpose was to get the default encoding.

But let's take a step back: What's your use case?

The only code that legitimately should care about the fallback encoding is the C++ code involved in text/html and text/plain loading. And it already does.

Why does your add-on need to know the fallback encoding? Note that these days, the fallback encoding varies based on the top-level domain of the site, since that signal is more strongly connected with a site than the UI language of the browser, so there isn't just one fallback encoding anymore.

Comment 33

[u427052]

My add-on is a mail client for Firefox, so i need to allow user to choose encoding character when they write a new message.

Comment 34

[Henri Sivonen (:hsivonen)]

Mail clients have supported receiving UTF-8-encoded messages for over a decade. The email client on Firefox OS always sends UTF-8. I suggest you do the same.