947507 - Current character Encoding auto-detect method is gone when upgrade to Nightly/Holly 28 from Aurora27

Status: VERIFIED FIXED

(Firefox :: Menus, defect)

Description

[Alice0775 White]

Steps To Reproduce
1. Strat Aurora27.0a2
2. Select Character Encoding > auto-detect to "Universal"
3. Close browser

4. Start Nightly28.0a1 with same profile of the above
5. Select Character Encoding > auto-detect 

Actual Results:
No item is selected

Expected Results:
"Universal" should be indicated and selected.
OR
At least, current auto-detect method should be indicated.

Comment 1

[Masatoshi Kimura [:emk]]

"Universal" was intentionally removed from the menu. Users can easily select other items from the menu.

Comment 2

[Alice0775 White]

Migration code should be provided in this case.
Or Browser should be warn to user to set other value when upgrade.

Of course release note is necessary even if Browser take whichever.

Comment 3

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

(In reply to Alice0775 White from comment #2)
> Migration code should be provided in this case.

What other migration would make sense here?

Comment 4

[Henri Sivonen (:hsivonen)]

(In reply to :Gavin Sharp (email [e-mail]) from comment #3)
> (In reply to Alice0775 White from comment #2)
> > Migration code should be provided in this case.
> 
> What other migration would make sense here?

Resetting the pref so that if the user has previously selected "Universal" or "CJK", the setting turns into Japanese instead of "(off)".

Comment 5

[Henri Sivonen (:hsivonen)]

Attachment: Reset the pref if value not settable from UI

Comment 6

[Henri Sivonen (:hsivonen)]

Probably breaks tests. Let's see which ones:
https://tbpl.mozilla.org/?tree=Try&rev=35022538b9f0

Comment 7

[Masatoshi Kimura [:emk]]

Looks like the failures are expected: unsupported detectors no longer work because the setting will be unset in the HTML parser.

Comment 8

[Henri Sivonen (:hsivonen)]

I just realized that  doing this in Gecko code  would leave Thunderbird in an  unpolished state. It would be better to put the pref  migration into Firefox-specific code.

Comment 9

[Henri Sivonen (:hsivonen)]

Attachment: Don't bother Thunderbird

https://tbpl.mozilla.org/?tree=Try&rev=7a0d12016429

Thunderbird uses the same pref name for uses of detectors from outside the HTML parser. Unlike the previous patch, this patch shouldn't leave Thunderdbird in an unpolished state.

Comment 10

[Henri Sivonen (:hsivonen)]

Comment on attachment 8349273 [details] [diff] [review]
Don't bother Thunderbird

It's unclear to me who should review this. Trying smontagu for what this does and Gavin for where the code is placed.

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 8349273 [details] [diff] [review]
Don't bother Thunderbird

>+    // Limit the encoding detector pref to values reachable from the UI.
>+    this._migrateDetector();
>+
>     // handle any UI migration
>     this._migrateUI();

>+  _migrateDetector: function BG__migrateDetector() {
>+    try {
>+      let detector = Services.prefs.getComplexValue("intl.charset.detector",
>+                                                    Ci.nsIPrefLocalizedString).data;
>+      if (!(detector == "" ||
>+            detector == "ja_parallel_state_machine" ||
>+            detector == "ruprob" ||
>+            detector == "ukprob")) {
>+        Services.prefs.clearUserPref("intl.charset.detector");
>+      }
>+    } catch (ex) {}
>+  },

This code would run again and again, more precisely on every startup, which feels kind of wrong. Can you move it to _migrateUI where we execute migration steps only once?

Please don't wrap stuff in try/catch that you don't expect to throw exceptions. I assume that's everything but the getComplexValue call.

Comment 12

[Henri Sivonen (:hsivonen)]

(In reply to Dão Gottwald [:dao] from comment #11)
> Can you move it to _migrateUI where we execute migration steps only once?

I didn't put the code there, because this didn't seem to correlate with the UI_VERSION concept.

The new menu implementation that this migration is associated with landed in Firefox 28--both m-c and holly. If I put the code in _migrateUI  under what condition should I make the code run? I'm guessing the answer even depends on whether the migration code gets uplifted.

Comment 13

[Dão Gottwald [:dao]]

(In reply to Henri Sivonen (:hsivonen) from comment #12)
> (In reply to Dão Gottwald [:dao] from comment #11)
> > Can you move it to _migrateUI where we execute migration steps only once?
> 
> I didn't put the code there, because this didn't seem to correlate with the
> UI_VERSION concept.
> 
> The new menu implementation that this migration is associated with landed in
> Firefox 28--both m-c and holly. If I put the code in _migrateUI  under what
> condition should I make the code run? I'm guessing the answer even depends
> on whether the migration code gets uplifted.

The UI_VERSION is just an arbitrary number with no external dependency. You need to bump it e.g. from 18 to 19 and add your migration step.

Comment 14

[Simon Montagu :smontagu]

Comment on attachment 8349273 [details] [diff] [review]
Don't bother Thunderbird

Recusing myself. It's quite possible that this is the right thing to do and I'm too involved in the history of the encoding autodetection to see it.

Comment 15

[Henri Sivonen (:hsivonen)]

Attachment: m-c patch

Comment 16

[Henri Sivonen (:hsivonen)]

Attachment: Holly/Aurora patch

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 8350010 [details] [diff] [review]
m-c patch

>+      let detector = null;    
>+      try {
>+        detector = Services.prefs.getComplexValue("intl.charset.detector",
>+                                                  Ci.nsIPrefLocalizedString).data;
>+      } catch (ex) {}
>+      if (!(detector == "" ||
>+            detector == "ja_parallel_state_machine" ||
>+            detector == "ruprob" ||
>+            detector == "ukprob")) {
>+        // If the encoding detector pref value is not reachable from the UI,
>+        // reset to default (varies by localization).
>+        Services.prefs.clearUserPref("intl.charset.detector");
>+      }

Should detector == "" be !detector instead, to cover the case of getComplexValue throwing? Not sure when exactly you expect it to throw.

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 8350011 [details] [diff] [review]
Holly/Aurora patch

Same question as for the other patch.

Comment 19

[Henri Sivonen (:hsivonen)]

(In reply to Dão Gottwald [:dao] from comment #17)
> Comment on attachment 8350010 [details] [diff] [review]
> m-c patch
> 
> >+      let detector = null;    
> >+      try {
> >+        detector = Services.prefs.getComplexValue("intl.charset.detector",
> >+                                                  Ci.nsIPrefLocalizedString).data;
> >+      } catch (ex) {}
> >+      if (!(detector == "" ||
> >+            detector == "ja_parallel_state_machine" ||
> >+            detector == "ruprob" ||
> >+            detector == "ukprob")) {
> >+        // If the encoding detector pref value is not reachable from the UI,
> >+        // reset to default (varies by localization).
> >+        Services.prefs.clearUserPref("intl.charset.detector");
> >+      }
> 
> Should detector == "" be !detector instead, to cover the case of
> getComplexValue throwing? Not sure when exactly you expect it to throw.

I don't know when it would throw, but existing UI code seemed to think throwing was possible. It's detector == "" so that if an exception is thrown, detector is not == with "", so the pref gets cleared.

Thanks.

Comment 20

[Henri Sivonen (:hsivonen)]

Thanks.

https://hg.mozilla.org/projects/holly/rev/36fcd299b24f
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd19bec07e82

Comment 21

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

https://hg.mozilla.org/mozilla-central/rev/dd19bec07e82

Comment 22

[Henri Sivonen (:hsivonen)]

Comment on attachment 8350011 [details] [diff] [review]
Holly/Aurora patch

[Approval Request Comment]
> Bug caused by (feature/regressing bug #): 

Bug 805374.

> User impact if declined: 

If the user has previously chosen a character encoding detector configuration that is not the default for any localization, that choice is no longer reflected in the menu and the user's character encoding detector configuration is in a state that can no longer be reached from the menu.

> Testing completed (on m-c, etc.): 

Landed on Holly. I tested that the migration code runs when migrating from a profile used with Firefox 26 to the 2013-12-20 Holly Mac nightly.

> Risk to taking this patch (and alternatives if risky): 

Low risk. The Holly patch applies to Aurora as-is.

> String or IDL/UUID changes made by this patch:

None.

Comment 23

[Henri Sivonen (:hsivonen)]

Thanks. Landed:
https://hg.mozilla.org/releases/mozilla-aurora/rev/6395885b7e0e

Unclear to me, why Bugzilla doesn't let me change status-firefox28.

Relnoting this seems a bit of an overkill to me, but if we relnote anyway, I'd say something like:
"The character encoding detection menu now shows only detection modes that are enabled by default in at least one Firefox localization. If a mode that wasn't enabled by default anywhere had been chosen manually, the setting resets to the default for the localization."

Comment 24

[Henri Sivonen (:hsivonen)]

Well, Bugzilla reset the flags it didn't let me edit when submitting the previous comment and now lets me edit the flags. Setting status-firefox28 to fixed and otherwise restoring the flags set by lsblakk.

Comment 25

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Henri Sivonen (:hsivonen) from comment #23)
> Thanks. Landed:
> https://hg.mozilla.org/releases/mozilla-aurora/rev/6395885b7e0e
> 
> Unclear to me, why Bugzilla doesn't let me change status-firefox28.
> 
> Relnoting this seems a bit of an overkill to me, but if we relnote anyway,
> I'd say something like:
> "The character encoding detection menu now shows only detection modes that
> are enabled by default in at least one Firefox localization. If a mode that
> wasn't enabled by default anywhere had been chosen manually, the setting
> resets to the default for the localization."

That's a lot of words for a release note, I think we can let it go and if there's a bunch of user feedback in Beta suggesting users are confused we can tap Sumo to feature an article about this change.

Comment 26

[u279076]

Does this have or need tests?

Comment 27

[Henri Sivonen (:hsivonen)]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #26)
> Does this have or need tests?

It doesn't have tests. Having automated tests for this would be an overkill: The complication of having to have an old profile and launching Firefox with that profile would massively outweigh the benefit of being sure this never regresses.

Comment 28

[u279076]

Alice, please verify this is working for you now.

Comment 29

[Alice0775 White]

Steps To Reproduce
1. Start Firefoxa27.0b6
2. Select Character Encoding > auto-detect to "Universal"
3. Close browser

4. Start Aurora28.0a2 with same profile of the above
5. Select Character Encoding > auto-detect 

Actual Results:
"off" is selected.
Performed silently without any notification...


Is this intentional? if so, I think this is problematic solution.

Comment 30

[Henri Sivonen (:hsivonen)]

(In reply to Alice0775 White from comment #29)
> Steps To Reproduce
> 1. Start Firefoxa27.0b6
> 2. Select Character Encoding > auto-detect to "Universal"
> 3. Close browser
> 
> 4. Start Aurora28.0a2 with same profile of the above
> 5. Select Character Encoding > auto-detect 
> 
> Actual Results:
> "off" is selected.
> Performed silently without any notification...
> 
> Is this intentional?

This is intentional if the localization of your copy of Firefox is other than Japanese, Russian or Ukrainian. Is your localization one of those three?

Comment 31

[Alice0775 White]

I use en-US build.

Comment 32

[u279076]

Thanks for your help Alice. I think it would be good to have multiple locales tested here just to make sure it's working as intended. Flagging for QA to do this testing.

Comment 33

[Catalin Varga [QA][:VarCat]]

I've verified this bug using the following environment:

FF 28.02
User Agent : Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0
Build Id: 20140210161136
Localization: en, de , ru ,uk , ja

OS: Win 7 x64

FF Aurora27.0a2
User Agent : Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0
Build Id: 20131206004003

Character Encoding->Autodetect->Universal  on Aurora

traslates to:

Character Encoding->Autodetect->Off for en and de localization
Character Encoding->Autodetect->uk for uk localization
Character Encoding->Autodetect->ja for ja localization
Character Encoding->Autodetect->ru for ru localization

Comment 34

[Manuela Muntean [Away]]

Verified as fixed on Win 7 x64.

having FF Aurora27.0a2
User Agent : Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0
Build Id: 20131206004003

Character Encoding->Autodetect->Universal  on Aurora

traslates on Firefox 29 beta 1 to:

Character Encoding->Autodetect->Off for en-US and de localization
Character Encoding->Autodetect->uk for uk localization
Character Encoding->Autodetect->ja for ja localization
Character Encoding->Autodetect->ru for ru localization