Closed Bug 947507 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Menus, defect)

28 Branch
x86_64
Windows 7
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 29
Tracking Status
firefox27 --- unaffected
firefox28 --- verified
firefox29 --- verified

People

(Reporter: alice0775, Assigned: hsivonen)

References

Details

(Keywords: ux-mode-error, ux-userfeedback, Whiteboard: [fixed-in-holly])

Attachments

(2 files, 2 obsolete files)

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.
Summary: Character Encoding auto-detect item is gone when upgrade to Nightly 28 from Aurora27 → Current character Encoding auto-detect method is gone when upgrade to Nightly 28 from Aurora27
Summary: Current character Encoding auto-detect method is gone when upgrade to Nightly 28 from Aurora27 → Current character Encoding auto-detect method is gone when upgrade to Nightly/Holly 28 from Aurora27
"Universal" was intentionally removed from the menu. Users can easily select other items from the menu.
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.
(In reply to Alice0775 White from comment #2)
> Migration code should be provided in this case.

What other migration would make sense here?
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) 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)".
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Probably breaks tests. Let's see which ones:
https://tbpl.mozilla.org/?tree=Try&rev=35022538b9f0
Looks like the failures are expected: unsupported detectors no longer work because the setting will be unset in the HTML parser.
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.
Attached patch Don't bother Thunderbird (obsolete) — Splinter Review
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.
Attachment #8348710 - Attachment is obsolete: true
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.
Attachment #8349273 - Flags: review?(smontagu)
Attachment #8349273 - Flags: review?(gavin.sharp)
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.
Attachment #8349273 - Flags: review?(gavin.sharp) → review-
(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.
(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 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.
Attachment #8349273 - Flags: review?(smontagu) → review?(VYV03354)
Attachment #8349273 - Attachment is obsolete: true
Attachment #8349273 - Flags: review?(VYV03354)
Attached patch m-c patchSplinter Review
Attachment #8350010 - Flags: review?(dao)
Attachment #8350010 - Flags: review?(VYV03354)
Attachment #8350011 - Flags: review?(dao)
Attachment #8350011 - Flags: review?(VYV03354)
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.
Attachment #8350010 - Flags: review?(dao) → review+
Comment on attachment 8350011 [details] [diff] [review]
Holly/Aurora patch

Same question as for the other patch.
Attachment #8350011 - Flags: review?(dao) → review+
(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.
Attachment #8350010 - Flags: review?(VYV03354) → review+
Attachment #8350011 - Flags: review?(VYV03354) → review+
https://hg.mozilla.org/mozilla-central/rev/dd19bec07e82
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
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.
Attachment #8350011 - Flags: approval-mozilla-aurora?
Attachment #8350011 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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."
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.
(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.
Does this have or need tests?
Flags: needinfo?(hsivonen)
Flags: in-testsuite?
(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.
Flags: needinfo?(hsivonen)
Flags: in-testsuite?
Flags: in-testsuite-
Alice, please verify this is working for you now.
Flags: needinfo?(alice0775)
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.
Flags: needinfo?(alice0775)
(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?
I use en-US build.
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.
Keywords: verifyme
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
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
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.