Closed Bug 943262 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox for Android :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 31
Tracking Status
firefox29 --- wontfix
firefox30 --- fixed
firefox31 --- fixed
fennec 29+ ---

People

(Reporter: hsivonen, Assigned: Margaret)

References

Details

Attachments

(3 files, 4 obsolete files)

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.
Summary: Don't use charsetTitles.properties on Android, only allow Encoding Standard-compliant Character Encoding menu items → Don't use charsetTitles.properties on Android, only allow Character Encoding menu items that desktop allows
Blocks: 943252
tracking-fennec: --- → ?
tracking-fennec: ? → 29+
Whiteboard: [mentor=margaret][lang=js]
Assignee: nobody → margaret.leibovic
Attached patch WIP (obsolete) — Splinter Review
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?
Attachment #8387303 - Flags: feedback?(mark.finkle)
Attachment #8387303 - Flags: feedback?(bnicholson)
Whiteboard: [mentor=margaret][lang=js]
(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
See Also: → 832910
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #8387303 - Attachment is obsolete: true
Attachment #8387303 - Flags: feedback?(mark.finkle)
Attachment #8387303 - Flags: feedback?(bnicholson)
Attachment #8389368 - Flags: review?(bnicholson)
(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 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.
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.
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) 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.
(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.
Attached patch patch that matches desktop (obsolete) — Splinter Review
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.
Attachment #8389368 - Attachment is obsolete: true
Attachment #8389368 - Flags: review?(bnicholson)
Attachment #8390972 - Flags: review?(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.
Attachment #8390972 - Flags: review?(bnicholson) → review+
(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.
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.
Attachment #8390972 - Attachment is obsolete: true
(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
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).
(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
Flags: needinfo?(margaret.leibovic) → needinfo?(hsivonen)
Attached patch Remove test_bug713519 (obsolete) — Splinter Review
My try run was green.

Please redirect if you're not an appropriate reviewer.
Attachment #8392598 - Flags: review?(hsivonen)
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?
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
Attachment #8392598 - Attachment is obsolete: true
Attachment #8392598 - Flags: review?(hsivonen)
Attachment #8393896 - Flags: review?(gavin.sharp)
This is tracking Fx29.
(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 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.
Attachment #8393896 - Flags: review?(gavin.sharp) → review+
(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.
Flags: needinfo?(hsivonen)
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
Attachment #8395714 - Flags: approval-mozilla-beta?
Attachment #8395714 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/632c0d6e8647
https://hg.mozilla.org/mozilla-central/rev/8f0786e1655c
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Attachment #8395714 - Flags: approval-mozilla-beta?
Attachment #8395714 - Flags: approval-mozilla-beta+
Attachment #8395714 - Flags: approval-mozilla-aurora?
Attachment #8395714 - Flags: approval-mozilla-aurora+
Depends on: 1003897
Depends on: 1005107
This was backed out of mozilla-release for causing bug 1003897:
http://hg.mozilla.org/releases/mozilla-release/rev/6b14de55b252
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 ?
(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.
(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.
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.
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.
You need to log in before you can comment on or make changes to this bug.