Closed Bug 959061 Opened 10 years ago Closed 10 years ago

Have only one "Chinese, Simplified" item (gbk) in the Character Encoding menu

Categories

(Firefox :: Menus, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 29

People

(Reporter: hsivonen, Assigned: hsivonen)

References

Details

Attachments

(2 files, 1 obsolete file)

Currently, gb18030 is a superset of the gbk decoder and if bug 959058 is fixed, they'll have the exact same decoder.

This means that right now, if the user sees mojibake and expects the page to use a legacy Simplified Chinese encoding, it always makes sense to try gb18030 instead of trying gbk. When bug 959058 is fixed, this means that the distinction between choosing gb18030 and gbk will be too subtle for user to know about and even when users do know about the distinction, they can't know what a particular site requires.

We should remove one of the menu items. If we did did it right now before fixing bug 959058, we should remove gbk and leave gb18030. However, if we wait  until after bug 959058  is fixed, we should probably remove gb18030 and leave gbk  to prefer the more conservative encoder behavior for misauthored pages.

Either way,  we should probably make the remaining "Chinese, Simplified"  item be checked when the user looks at the menu regardless of whether the encoding of the current document in the the top-level browsing context is gbk or gb18030. (While implementing that, we should probably make the "Hebrew" item checked even if the encoding of the page is iso-8859-8-i.)

emk, annevk, opinions on which one to remove and which one to leave?
I prefer to wait until bug 959058 is fixed.
Given that the menu is mostly about decoding, gb18030 makes the most sense to me.
(In reply to Anne (:annevk) from comment #2)
> Given that the menu is mostly about decoding, gb18030 makes the most sense
> to me.

So either way, the UI string for the menu item would be "Chinese, Simplified" without saying "GBK" or "GB18030". The question is how form submissions should work if the user overrides mojibake to "Chinese, Simplified". If a site is clueless enough not to have the right label, wouldn't it be more likely that it can't deal with the 2-byte sequences than that it can deal with them?
Oh right. If we have the gbk means gb18030 for decoding, gbk makes more sense. Sorry.
FWIW, I didn't fix this immediately after bug 959058, because I didn't want to  collide with bug 936442, which is more important.
Summary: Have only one "Chinese, Simplified" item in the Character Encoding menu → Have only one "Chinese, Simplified" item (gbk) in the Character Encoding menu
This patch applies on top of the one being proposed for bug 936442.  Requesting review already in the hopes of getting this landed after bug 936442 lands  but before the Firefox 29  train moves to Aurora.

(I care about the trains, because I don't want to make too many changes to this area in one release in order to be able to attribute telemetry changes correctly and I'm planning to land another patch that I expect to affect the usage of this menu in the Chinese locales during the Firefox 30 cycle.)
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Attachment #8367299 - Flags: review?(bmcbride)
The Hebrew-related change is a follow-up to bug 939311, which makes sense to do if we do a similar thing in the Simpified Chinese case.
Comment on attachment 8367299 [details] [diff] [review]
Collapse gb18030 and gbk into one in the menu. While at it, mark Hebrew checked if ISO Hebrew used.

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

Will need another patch following Part 2 in bug 936442 to add the FoldCharset functionality to CustomizableWidgets.jsm. But as an upside, that part can be landed directly on Aurora if need be.

::: toolkit/locales/en-US/chrome/global/charsetMenu.properties
@@ +6,5 @@
>  # Localizations may add or delete properties where the property key ends with
>  # ".key" as appropriate for the localization. The code that uses this data can
>  # deal with the absence of an access key for an item.
> +#
> +# For gbk, gbk.bis and gbk.bis.key are used to trigger string changes in 

Nit: Trailing whitespace

@@ +64,5 @@
>  ISO-8859-2       = Central European (ISO)
>  
>  # Chinese, Simplified
> +gbk.bis.key      =          S
> +gbk.bis          = Chinese, Simplified

Just out of curiosity, what does "bis" stand for?
Attachment #8367299 - Flags: review?(bmcbride) → review+
Sorry about not thinking about this properly before: It's probably a bad idea to remove the old strings in case bug 959058 needs to be backed out after the string freeze.

The code in this patch is the same as in the previous version, but no strings are removed from the property file.

(In reply to Blair McBride [:Unfocused] from comment #8)
> Will need another patch following Part 2 in bug 936442 to add the
> FoldCharset functionality to CustomizableWidgets.jsm. But as an upside, that
> part can be landed directly on Aurora if need be.

OK.

> Nit: Trailing whitespace

Fixed.

> Just out of curiosity, what does "bis" stand for?

For a second time. http://www.thefreedictionary.com/bis
Attachment #8367299 - Attachment is obsolete: true
Attachment #8367818 - Flags: review?(bmcbride)
Comment on attachment 8367818 [details] [diff] [review]
Collapse gb18030 and gbk into one in the menu. While at it, mark Hebrew checked if ISO Hebrew used. Don't remove the old strings for now.

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

Ah, hadn't realised that was a possibility (I have no context of the riskiness of the other bug).
Attachment #8367818 - Flags: review?(bmcbride) → review+
(In reply to Blair McBride from comment #8)
> (From update of attachment 8367299 [details] [diff] [review])
> Will need another patch following Part 2 in bug 936442 to add the
> FoldCharset functionality to CustomizableWidgets.jsm. But as an upside, that
> part can be landed directly on Aurora if need be.

I'm surprised it wasn't added directly to CharsetMenu.jsm in the first place.
https://hg.mozilla.org/mozilla-central/rev/0847543adbfd
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
(This was backed out on holly because it conflicted heavily and breaks the front-end freeze; if it's super-important this gets into 29 even if at some point in the future we end up using holly (ie, we back Australis out of Aurora/Beta), you'll need to write a separate patch for holly)
(In reply to neil@parkwaycc.co.uk from comment #12)
> (In reply to Blair McBride from comment #8)
> > (From update of attachment 8367299 [details] [diff] [review])
> > Will need another patch following Part 2 in bug 936442 to add the
> > FoldCharset functionality to CustomizableWidgets.jsm. But as an upside, that
> > part can be landed directly on Aurora if need be.
> 
> I'm surprised it wasn't added directly to CharsetMenu.jsm in the first place.

Yes, I also wondered that...
Can we at least leave the added strings in, please, so that fixing this later doesn't break the string freeze? I'd really like to get this in 29 to be able to correctly aatribute telemetry results from 30 to another change.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Henri Sivonen (:hsivonen) from comment #16)
> Can we at least leave the added strings in, please, so that fixing this
> later doesn't break the string freeze? I'd really like to get this in 29 to
> be able to correctly aatribute telemetry results from 30 to another change.

Sure, r=me to land just the strings on https://hg.mozilla.org/projects/holly/ .
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #17)
> (In reply to Henri Sivonen (:hsivonen) from comment #16)
> > Can we at least leave the added strings in, please, so that fixing this
> > later doesn't break the string freeze? I'd really like to get this in 29 to
> > be able to correctly aatribute telemetry results from 30 to another change.
> 
> Sure, r=me to land just the strings on
> https://hg.mozilla.org/projects/holly/ .

Landed this for you:

https://hg.mozilla.org/projects/holly/rev/9b6d8d865c0f
Thank you.
I pushed this follow up to remove an else-after-return:

https://hg.mozilla.org/integration/fx-team/rev/fe097821c110
Let's get this done in Holly, too, just in case.
Attachment #8387580 - Flags: review?(bmcbride)
Attachment #8387580 - Flags: review?(bmcbride) → review+
Comment on attachment 8387580 [details] [diff] [review]
Backport to Holly

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

Not a regression. Seeking parity between Aurora and Holly. This change already rode the trains to mozilla-aurora.

>User impact if declined: 

If we ship from the backout branch without this, it will be harder to attribute telemetry findings about the Character Encoding menu correctly.

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

Equivalent change has been on central for weeks.

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

The risk is mainly the general risk of creating a branch-specific patch. Low risk.

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

None. The necessary strings are on Holly already.
Attachment #8387580 - Flags: approval-mozilla-aurora?
(Note: the request is really for Holly and not for normal Aurora.)
Attachment #8387580 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.