Closed Bug 805374 Opened 7 years ago Closed 6 years ago

Remove Character Encoding menu entries that are not in the Encoding Standard

Categories

(Firefox :: Menus, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: hsivonen, Assigned: hsivonen)

References

(Blocks 4 open bugs)

Details

Attachments

(2 files, 11 obsolete files)

18.38 KB, patch
smontagu
: review+
Details | Diff | Splinter Review
37.51 KB, patch
Unfocused
: review+
Details | Diff | Splinter Review
We are in the process of implementing http://encoding.spec.whatwg.org/ . The process involves removing support for legacy character decoders that aren’t really necessary for supporting existing Web content.

The removal of unnecessary items from the Character Encoding menu can happen in advance to removing the decoders for those encodings.

Expected results:
Expected the Character Encoding menu not to have entries that are not listed in http://encoding.spec.whatwg.org/
Attached patch patch (obsolete) — Splinter Review
I also removed all UTF-16 family and 7-bit encodings to prevent self-XSS.
Attachment #684143 - Flags: review?(smontagu)
It seems that this removes the ability to manually choose UTF-8, but I guess that is OKish to the extent UTF-8 content is supposed to be non-legacy.
(In reply to Henri Sivonen (:hsivonen) from comment #3)
> It seems that this removes the ability to manually choose UTF-8, but I guess
> that is OKish to the extent UTF-8 content is supposed to be non-legacy.
No, UTF-8 (and all encodings not listed at the third level) is displayed at the second level. It's the reason I have to add .notForBrowser for all removed encodings.
Also, UTF-8 will be usually displayed at the first level. But it is not guaranteed because intl.charsetmenu.browser.static is localizable.
Comment on attachment 684143 [details] [diff] [review]
patch

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

::: intl/uconv/src/charsetData.properties
@@ +40,5 @@
>  ibm1125.notForBrowser                   = true
>  ibm1131.notForBrowser                   = true
>  
> +# encodings not in the Encoding Standard
> +iso-8859-1.notForBrowser                = true

I think users still expect to see iso-8859-1 in the encoding menu, especially if they don't happen to have read the Encoding Standard
Attached patch patch v2 (obsolete) — Splinter Review
Unhiding ISO-8859-1
Attachment #684143 - Attachment is obsolete: true
Attachment #684143 - Flags: review?(smontagu)
Attachment #684552 - Flags: review?(smontagu)
(In reply to Masatoshi Kimura [:emk] from comment #4)
> No, UTF-8 (and all encodings not listed at the third level) is displayed at
> the second level.

OK.

(In reply to Simon Montagu from comment #5)
> I think users still expect to see iso-8859-1 in the encoding menu,
> especially if they don't happen to have read the Encoding Standard

This could be addressed by changing the UI label for Windows-1252 from “Western (Windows-1252)” to “Western (Windows-1252/ISO-8859-1)”.
(In reply to Henri Sivonen (:hsivonen) from comment #8)
> (In reply to Simon Montagu from comment #5)
> > I think users still expect to see iso-8859-1 in the encoding menu,
> > especially if they don't happen to have read the Encoding Standard
> 
> This could be addressed by changing the UI label for Windows-1252 from
> “Western (Windows-1252)” to “Western (Windows-1252/ISO-8859-1)”.
We need to sync all localizations. Also, we (and our all localizations) will have to change intl.charset.default. So I want to separate the bug.
Comment on attachment 684552 [details] [diff] [review]
patch v2

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

Does this also remove the encodings from the menus in Thunderbird? Are we OK with that, especially ISO-2022-JP?

Also, in the ES big5-hkscs is an alias for Big5, but we still have them as separate encodings. We probably should keep them separate in the menus for now (see also bug 749052).
Comment on attachment 684552 [details] [diff] [review]
patch v2

Canceling review to reconsider what encodings will be removed at the moment.
Attachment #684552 - Flags: review?(smontagu)
Blocks: 234628
(In reply to Masatoshi Kimura [:emk] from comment #11)
> Canceling review to reconsider what encodings will be removed at the moment.

Is that only about big5-hkscs or are there other issues?
(In reply to Henri Sivonen (:hsivonen) from comment #12)
> (In reply to Masatoshi Kimura [:emk] from comment #11)
> > Canceling review to reconsider what encodings will be removed at the moment.
> 
> Is that only about big5-hkscs or are there other issues?

I'm considering:
- Listed encodings in intl.charset.default.
  https://mxr.mozilla.org/l10n-central/search?string=intl.charset.default
- Useful encodings for mail (Until the mail view encodings menu is separated from the browser's one).

I'll attach a new patch removing only UTF-16 family encodings to unblock bug 234628.
Attachment #684552 - Attachment is obsolete: true
Attachment #691330 - Flags: review?(smontagu)
Attachment #691330 - Flags: review?(smontagu) → review+
Blocks: 822447
(In reply to Simon Montagu from comment #10)
> Does this also remove the encodings from the menus in Thunderbird? Are we OK
> with that, especially ISO-2022-JP?

Yes this broken Thunderbird :( Filed bug 822447 for that.
Blocks: 802033
Depends on: 844042
Depends on: 847886
This is blocking me from introducing sanity-check assertions in bug 863728.

(In reply to Masatoshi Kimura [:emk] from comment #13)
> (In reply to Henri Sivonen (:hsivonen) from comment #12)
> > (In reply to Masatoshi Kimura [:emk] from comment #11)
> > > Canceling review to reconsider what encodings will be removed at the moment.
> > 
> > Is that only about big5-hkscs or are there other issues?
> 
> I'm considering:
> - Listed encodings in intl.charset.default.
>   https://mxr.mozilla.org/l10n-central/search?string=intl.charset.default

intl.charset.default is gone.

> - Useful encodings for mail (Until the mail view encodings menu is separated
> from the browser's one).

What needs to be done for that to happen?
Assignee: nobody → hsivonen
Blocks: 863728
So what's the best way to decouple the mailnews submenus from the browser submenus if notForBrowser filtering is unwanted for the mailnews submenus?
>-#iso-2022-jp.notForBrowser               = true
>+iso-2022-jp.notForBrowser               = true

At least we can't remove iso-2022-jp from the mailnews submenus.
(In reply to Masatoshi Kimura [:emk] from comment #21)
> >-#iso-2022-jp.notForBrowser               = true
> >+iso-2022-jp.notForBrowser               = true
> 
> At least we can't remove iso-2022-jp from the mailnews submenus.

oops that was an editing slip.

I wonder if we should get rid of the submenus in the browser case now that there are fewer items. Especially if we put item that are the most relevant to the current localization higher up in the flat menu.
(In reply to Henri Sivonen (:hsivonen) from comment #22)
> I wonder if we should get rid of the submenus in the browser case now that
> there are fewer items. Especially if we put item that are the most relevant
> to the current localization higher up in the flat menu.

I think it makes sense to pursue a flat menu. The encodings in the Encoding Standard fall into the following groups:

(Globally relevant)
UTF-8
windows-1252

(Japanese)
Shift_JIS
EUC-JP
ISO-2022-JP

(Cyrillic)
windows-1251
ISO-8859-5
KOI8-R
KOI8-U
IBM866
x-mac-cyrillic

(Chinese, Simplified)
gbk
gb18030

(Chinese, Traditional)
Big5

(Korean)
EUC-KR

(Central European)
windows-1250
ISO-8859-2

(Greek)
windows-1253
ISO-8859-7

(Hebrew)
windows-1255
ISO-8859-8
ISO-8859-8-I

(Arabic)
windows-1256
ISO-8859-6

(Baltic)
windows-1257
ISO-8859-4
ISO-8859-13

(Turkish)
windows-1254

(Vietnamese)
windows-1258

(Thai)
windows-874

(Rare European encodings)
ISO-8859-3
ISO-8859-10
ISO-8859-14
ISO-8859-15
ISO-8859-16
macintosh

- -

(Not in menu)
replacement
HZ-GB-2312
x-user-defined
UTF-16BE
UTF-16LE
Big5-HKSCS

I suggest not putting HZ-GB-2312  in the menu, because it's XSS  dangerous. I suggest not putting x-user-defined in the menu,  because x-user-defined  only makes sense as a declared encoding and as an XHR encoding. The probability of a page making sense as x-user-defined  when the encoding is not declared is so close to zero that I think it's  not worthwhile to put x-user-defined in the menu. Big5-HKSCS is on its way out as an independent encoding.

I suggest we always put the "Globally relevant" encodings first in the menu and the "Rare European encodings" last in the menu, since the "Rare European encodings" most likely aren't particularly Web-relevant despite having wide browser support. Then between those, I suggest we put the group that the fallback encoding of the current localization falls into right after the "Globally relevant" group and then put the other groups in an order that makes sense in terms of geographical proximity considering what was put first, precise order to be bikeshedded.

Thoughts?
annevk, emk, smontagu, does a flat menu make sense to you considering that the number of entries is going to be smaller than what we had before?

If not, can you suggest a new submenu structure? (The old submenu structure becomes very unbalanced after removing the non-Encoding Standard encodings.)
Flags: needinfo?(smontagu)
Flags: needinfo?(annevk)
Flags: needinfo?(VYV03354)
Seems to work well for Chrome. And it's a lot more obvious.
Flags: needinfo?(annevk)
I'm all in favour of moving to a flat menu, but I don't think we should over-engineer the ordering. Let's have the "globally relevant" and "recently used" at the top, i.e. the same as we have now in the top-level menu, and the rest alphabetically after a divider, like Chrome.
Flags: needinfo?(smontagu)
(In reply to Simon Montagu :smontagu from comment #26)
> I'm all in favour of moving to a flat menu, 

Great.

> but I don't think we should
> over-engineer the ordering. Let's have the "globally relevant" and "recently
> used" at the top, i.e. the same as we have now in the top-level menu, and
> the rest alphabetically after a divider, like Chrome.

From the engineering perspective, doing the sort of locale-sensitive ordering I proposed is easier than maintaining a recently-used cache, removing the recently-used ones from the overflow list sorting the overflow list by localized strings. In that sense, I don't think what I proposed is overengineering compared to maintaining a list of recent encodings.

Furthermore, what you describe isn't the same as we have now in the top menu, because now we also have locale-dependent items that are statically pinned as if they had always been used recently, which increases complexity.

Do you think the approach you proposed is preferable even when it would be more work engineering-wise than what I proposed? (In this case, the less old code is reused, the easier, so reusing existing code for existing behaviors doesn't make engineering easier than just doing something new and static.)
Flags: needinfo?(smontagu)
I totally agree the flat submenu.
I have no strong opinion about the ordering.
Flags: needinfo?(VYV03354)
Sorry if I misused terminology: when I said "over-engineer" I meant "make too complex from the UX perspective", I wasn't thinking of engineering complexity as such.

I think if we have a flat menu it's more intuitive to *have* a flat menu rather than preserving the memory of the current submenus as groups within the flat menu. Alphabetical ordering will in any case keep similar things close together. On the other hand, it does make sense to retain something like the present top level menu at the top for things that we think are likely to be first choice. If what I described earlier isn't what we currently have in the top menu, the difference was unintentional.
Flags: needinfo?(smontagu)
Oh, I assumed the current top level menu would be left intact and the third-level menu would be merged to the second-level, which would result a similar menu to what IE has.
(In reply to Simon Montagu :smontagu from comment #29)
> I think if we have a flat menu it's more intuitive to *have* a flat menu
> rather than preserving the memory of the current submenus as groups within
> the flat menu. Alphabetical ordering will in any case keep similar things
> close together. On the other hand, it does make sense to retain something
> like the present top level menu at the top for things that we think are
> likely to be first choice. If what I described earlier isn't what we
> currently have in the top menu, the difference was unintentional.

The point of my proposal was to have an outcome where the likely first choices are nearer to the top without having to keep track of recently used items and without having to sort by localized strings.

Would you accept a patch that doesn't track the recently used encodings?

(In reply to Masatoshi Kimura [:emk] from comment #30)
> Oh, I assumed the current top level menu would be left intact and the
> third-level menu would be merged to the second-level, which would result a
> similar menu to what IE has.

I meant:
View menu has a Character Encoding submenu. The Character Encoding submenu has an autodetection submenu followed by a flat list of encodings.
I think features like tracking recently used encodings aren't worthwhile for a feature that goes unused in 99.9% of Firefox sessions even in the case of the Japanese localization.
Attached image Screenshot of current WIP (obsolete) —
> Alphabetical ordering will in any case keep similar things close together.

Not for the stuff I described as "rare European encodings", FWIW.

> View menu has a Character Encoding submenu. The Character Encoding submenu
> has an autodetection submenu followed by a flat list of encodings.

See attachment for the general structure. (The relative order of the groups could be better.)
(In reply to Henri Sivonen (:hsivonen) from comment #31)
> Would you accept a patch that doesn't track the recently used encodings?

Yes, I think that's reasonable
Attached patch Implement in JS (obsolete) — Splinter Review
Attachment #828588 - Attachment is obsolete: true
Attachment #831524 - Attachment is obsolete: true
Attachment #832149 - Attachment description: Implement is JS → Implement in JS
Attached image Screenshot of what the patch implements (obsolete) —
Attachment #832149 - Attachment is obsolete: true
Attachment #832175 - Flags: review?(smontagu)
Attachment #832175 - Flags: review?(gavin.sharp)
Comment on attachment 832175 [details] [diff] [review]
Implement in JS, better commit message

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

Please change the label of Windows-1252 as you proposed in comment 8.

I still want alphabetical order of the menu entries. We have code for that in nsCharsetMenu.cpp. In general I think we should shamelessly imitate the menu in Chrome (apart from entries that they have and we don't want), like this:

Auto-Detect
<separator>
Unicode (UTF-8)
Western (Windows-1252/ISO-8859-1)
<separator>
[everything else, ordered alphabetically]
Comment on attachment 832175 [details] [diff] [review]
Implement in JS, better commit message

Deferring to Gijs to review the mechanics; I support the goal, and love that we're moving away from the RDF/nsCharsetMenu awfulness.

Why do we need to build the menu dynamically, rather than hardcoding it in XUL? Is it because we're not getting rid of the detector menu RDF stuff yet?
Attachment #832175 - Flags: review?(gavin.sharp) → review?(gijskruitbosch+bugs)
Comment on attachment 832175 [details] [diff] [review]
Implement in JS, better commit message

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

::: browser/base/content/browser.js
@@ +5261,5 @@
>          menuitem.setAttribute('checked', 'true');
>      }
>  }
>  
> +// XXX unify this with Australis when it lands

Can you please just put this in a JSM for this patch? It'll simplify matters a lot for Australis. 

Separately, if we're whitelisting here, what's the point of the .notForBrowser changes? It seems the plans have changed since comment #4. There should ideally be only 1 list, or both lists should have clear pointers about the existence of the other list and what the criteria for both are. If the .notForBrowser stuff isn't enough, we should either update that or use this list as well when determining whether to cache stuff, I'd assume ( http://mxr.mozilla.org/mozilla-central/source/toolkit/components/intl/nsCharsetMenu.cpp#1686 )

@@ +5324,5 @@
> +    return;
> +  }
> +  let charsetManager = Cc["@mozilla.org/charset-converter-manager;1"].
> +                       getService(Ci.nsICharsetConverterManager);
> +  for (let encoding of encodings) {

This probably needs changing per comment 38.

@@ +5332,5 @@
> +    try {
> +      menuItem.setAttribute("label", charsetManager.getCharsetTitle(encoding));
> +    } catch (e) {
> +      menuItem.setAttribute("label", encoding);
> +    }

I'd prefer:

let title = encoding;
try {
  title = charsetManager.getCharsetTitle(encoding);
} catch (ex) {}
menuItem.setAttribute("label", title);

@@ +5335,5 @@
> +      menuItem.setAttribute("label", encoding);
> +    }
> +    menuItem.setAttribute("id", "charset." + encoding);
> +    parent.appendChild(menuItem);
> +  }  

Nit: trailing whitespace

@@ +5360,5 @@
>    UpdateCharsetDetector(event.target);
>  }
>  
>  function CreateMenu(node) {
> +  // XXX Move the detector list to JS and get rid of this stuff.

This comment in this place doesn't make sense to me. Can you clarify?

::: intl/uconv/src/charsetData.properties
@@ +40,5 @@
>  ibm1125.notForBrowser                   = true
>  ibm1131.notForBrowser                   = true
>  
>  # encodings not in the Encoding Standard
>  # keep ISO-8859-1 at the moment

This comment is now out of date.
Attachment #832175 - Flags: review?(gijskruitbosch+bugs)
Blocks: 936442
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #39)
> Why do we need to build the menu dynamically, rather than hardcoding it in
> XUL?

To reuse the existing encoding titles that are not in a .dtd.

> Is it because we're not getting rid of the detector menu RDF stuff yet?

No. I didn't touch the decoder stuff on the assumption that r+ is more likely for piecewise changes.

I can do the whole thing in .xul/.dtd in one go, sure. (If that works for Gijs from the Australis perspective.)

(In reply to Simon Montagu :smontagu from comment #38)
> Please change the label of Windows-1252 as you proposed in comment 8.

This will trigger relocalization of the titles anyway, so I guess I could do it all in .xul/.dtd then.

> I still want alphabetical order of the menu entries.

:-( Complexity. I guess I can make JS sort menuitems given as XML in .xul.

> We have code for that in nsCharsetMenu.cpp.

That's of no use, since we're trying to move away from nsCharsetMenu.cpp.

(In reply to :Gijs Kruitbosch from comment #40)
> Can you please just put this in a JSM for this patch? It'll simplify matters
> a lot for Australis. 

If I put this is .xul/.dtd per Gavin's comment, there's no need for a .jsm. (If the Australis widget didn't have customizability, we could #include the same browser-charsetmenu.inc into it, too... I'd prefer the Australis widget put the autodetect stuff in a submenu anyway to avoid advertising it too much.)

> Separately, if we're whitelisting here, what's the point of the
> .notForBrowser changes?

I did't know if there was a point in SeaMonkey or something. Looks like we could remove all the .notForBrowser annotations without ill effects, since nsCharsetMenu.cpp is the only place where they are used outside unit tests.

> There should ideally be only 1 list, or both lists should have clear
> pointers about the existence of the other list and what the criteria for
> both are.

Would you rather have a list instead of menuitems written directly in .xul per Gavin's comment?

> @@ +5360,5 @@
> >    UpdateCharsetDetector(event.target);
> >  }
> >  
> >  function CreateMenu(node) {
> > +  // XXX Move the detector list to JS and get rid of this stuff.
> 
> This comment in this place doesn't make sense to me. Can you clarify?

The CreateMenu() function remains only for the detector stuff.

Gijs, how should I proceed on the point of putting the items directly in .xul vs. keeping the list in JS but making in more easily available to Australis? (I don't know what a .jsm for this purpose would look like, so I'd need some more guidance if you wish to proceed with the .jsm route.)
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Henri Sivonen (:hsivonen) from comment #41)
> > Is it because we're not getting rid of the detector menu RDF stuff yet?
> 
> No. I didn't touch the decoder stuff on the assumption that r+ is more
> likely for piecewise changes.
> 
> I can do the whole thing in .xul/.dtd in one go, sure. (If that works for
> Gijs from the Australis perspective.)

Well, even if it did, I think considering the other ideas in this bug it wouldn't really be an efficient solution to the problem: the list would have to be sorted, and the sorting depends on the localizable visible label, which makes hardcoding it less practical than generating and sorting it on the fly. On top of that, some encodings appear near the top, which AIUI is based on a localizable pref, but which should probably be checked against the same whitelist as well.

> (In reply to Simon Montagu :smontagu from comment #38)
> > Please change the label of Windows-1252 as you proposed in comment 8.
> 
> This will trigger relocalization of the titles anyway, so I guess I could do
> it all in .xul/.dtd then.

Actually this is slightly more interesting because if you change a string you'd have to change the string ID. Unless I missed it when skimming the cpp bits, none of our current code for charset labels is equipped to deal with non-predictable IDs. So you'd have to add that functionality.

> > I still want alphabetical order of the menu entries.
> 
> :-( Complexity. I guess I can make JS sort menuitems given as XML in .xul.

Yeah, so, don't do that, just sort [title, menuitem] pairs before inserting them.

> (In reply to :Gijs Kruitbosch from comment #40)
> > Can you please just put this in a JSM for this patch? It'll simplify matters
> > a lot for Australis. 
> 
> If I put this is .xul/.dtd per Gavin's comment, there's no need for a .jsm.
> (If the Australis widget didn't have customizability, we could #include the
> same browser-charsetmenu.inc into it, too... I'd prefer the Australis widget
> put the autodetect stuff in a submenu anyway to avoid advertising it too
> much.)

The widgets in Australis can't have submenus. So engineering-wise, that's not really possible. Let's not worry about that, though, we can figure out how to modify the JSM for the Australis-based widget, if that's at all necessary, when it's landed. AFAICT because of the title/sorting restrictions, hardcoding in .xul isn't a good idea.
 
> > There should ideally be only 1 list, or both lists should have clear
> > pointers about the existence of the other list and what the criteria for
> > both are.
> 
> Would you rather have a list instead of menuitems written directly in .xul
> per Gavin's comment?

Yes, and I suspect Gavin would agree given added context.

> > @@ +5360,5 @@
> > >    UpdateCharsetDetector(event.target);
> > >  }
> > >  
> > >  function CreateMenu(node) {
> > > +  // XXX Move the detector list to JS and get rid of this stuff.
> > 
> > This comment in this place doesn't make sense to me. Can you clarify?
> 
> The CreateMenu() function remains only for the detector stuff.

Personally, I'd prefer to move the entire thing to JS in one go. Until at least mid-December, I can promise fast reviews as regards the browser JS bits.

> Gijs, how should I proceed on the point of putting the items directly in
> .xul vs. keeping the list in JS but making in more easily available to
> Australis? (I don't know what a .jsm for this purpose would look like, so
> I'd need some more guidance if you wish to proceed with the .jsm route.)

I think we should just go the JSM route. Have a lazily-loaded JSM that exports some object (CharsetMenu.jsm exporting CharsetMenu?) which has a method with a container as an argument, in which to create the menu. You can get a document from aContainer.ownerDocument, and create elements using that. Take the list, skip the items which were included at the top (if I understood Simon's point correctly?) and store the items in a list of pairs, then sort that list based on the title, then insert in-order into a dom fragment and append the fragment to the container. I think that ideally selecting any of these charsets should also go via this JSM, rather than leaving half the functionality in nsCharsetMenu.cpp.

Please feel free to re-needinfo if that's not clear (enough), or ping me on IRC if I'm around.
Flags: needinfo?(gijskruitbosch+bugs)
When sorting by localized title, the menu becomes rather messy with ISO-8859-n with n >= 13 in the mix. Since IE11 doesn't expose those in the menu and it's unlikely that those are used without labeling on the Web, how about we don't put them in the menu either? We'd get a cleaner sorted menu and a shorter menu that way.
FYI, IE always sorts the menu based on the English name regardless of the localized title.
Attached patch Implement as JSM (obsolete) — Splinter Review
Stuff this patch does:

 * Moves the population of the character encoding menu from C++ to XUL and JSM.

 * Removes menu entries that are not part of the Encoding Standard.

 * Removes menu entries that IE11 doesn't have in its corresponding menu, since the design requirement to sort the menu items alphabetically would make a mess if these items were retained. The IE team appears to be very prudent on this sort of stuff, so if they feel they can get away without these items, so can we.

 * Removes HZ from the menu, since it is XSS-dangerous.

 * Removes User-Defined from the menu, since it only makes sense as a declared encoding and the number of user who know what that menu item meant and aren't CCed here is probably pretty close to zero.

 * Uses IE-style UI strings for the encodings: If there is only one item with a particular adjective, avoid technical stuff in parenthesis, e.g. just "Korean". If there are two  items  with a particular adjective, windows-xxxx and ISO-8859-x, just say Foo (Windows) and Foo (ISO).

 * UTF-8 and windows-1252 are always at the top of the menu.  The rest are sorted by the localized adjective in the locale-sensitive collation order except the items that share an adjective are sorted by reverse encoding name to put windows-* and Shift_JIS (most-used) first within each group.

 * The detector menu only includes detectors that ship on by default for some locale.

 * The Character Encoding menu goes away from View Source, because that code predated Firefox and it's not reasonable to hold back progress unless a Gecko dev like me goes and cleans up fringe UI that has been neglected since before Firefox.

Stuff this patch doesn't do:

 * Doesn't fix the new Australis widget. I suggest Australis devs do that as a follow-up.

 * Doesn't refactor global functions from browser.js into the new JSM. I suggest Firefox devs do that as a follow-up. I think it's not reasonable to require that sort of refactoring as a precondition for this exorcism of nsCharsetMenu.cpp to land.

 * This doesn't replicate the old "static" feature that pins a locale-dependent group of items to the top of the list. Please, let's remember how ridiculously little-used this submenu is and not bikeshed that sort of creeping elegance.

 * This doesn't remove code that becomes dead code in Firefox but remains in use in Thunderbird. Once this patch is done, I can prepare a second code removal patch and warned the Thunderbird developers before I landed that one so that they have a chance to move the legacy stuff over to c-c  without burning the tree first.
Attachment #832175 - Attachment is obsolete: true
Attachment #832175 - Flags: review?(smontagu)
Attachment #8334489 - Flags: review?(gijskruitbosch+bugs)
(In reply to Henri Sivonen (:hsivonen) from comment #45)
>  * The detector menu only includes detectors that ship on by default for
> some locale.

Russian, Ukranian and Japanese are the only locales that ship with a detector by default. I figured I'd prune the list to just those when reimplementing the list, since the others all have removal bugs on file anyway. (Bug 844115, bug 844118, bug 844120.) It seems silly to first reimplement UI for them and then remove.

Universal isn't really universal, so it's bad that we tempt ("Universal" looks awesome, right?) users to enable it when it can have adverse effects.

Post-Encoding Standard, Traditional Chinese and Korean each only have one legacy encoding, so there isn't much point in detection. As for Simplied Chinese, HZ is XSS-dangerous (so a bad idea to use depening on user setting) and GBK and GB18030 are the same for the common characters, so for detection purposes, Simplified Chinese might as well have just one legacy encoding.

IE/Chrome/Safari only have this stuff for Japanese, FWIW, so even retaining Russian and Ukranian for now is on the conservative side (but see 845791).
Attached image Screenshot of what the patch implements (obsolete) —
Attachment #832161 - Attachment is obsolete: true
Attachment #8334498 - Attachment is patch: false
Attachment #8334498 - Attachment mime type: text/plain → image/png
Comment on attachment 8334489 [details] [diff] [review]
Implement as JSM

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

::: browser/locales/en-US/chrome/browser/charsetMenu.dtd
@@ +11,5 @@
> +<!ENTITY charsetMenu.unicode.accesskey "U">
> +<!ENTITY charsetMenu.western.label     "Western">
> +<!ENTITY charsetMenu.western.accesskey "W">
> +
> +<!ENTITY charsetMenuAutodet.off.label  "(off)">

I realize this is not likely to ever become relevant, but I just want to note for the record that 'off' has the potential to become a valid language subtag in the future. (It is not currently assigned.) Maybe use '_off_' instead?
Comment on attachment 8334489 [details] [diff] [review]
Implement as JSM

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

Generally, this looks good to me, but I'd like to have another look when you've addressed my comments. For the toolkit removals, please get an rs from Unfocused or another toolkit peer. (I'm suggesting Blair because I suspect he'll be very happy to get this code out of our repo... :-) )

::: browser/base/content/browser-charsetmenu.inc
@@ +31,1 @@
>  #ifndef OMIT_ACCESSKEYS

This is the only file in the world that uses this, and nobody defines it ever. Let's take this opportunity to obliterate this ifdef throughout this file.

@@ +33,3 @@
>  #endif
> +    />
> +    <menuitem type="radio" name="charsetGroup" id="charset.window-1252" label="&charsetMenu.western.label;"

Because these are going to be hardcoded but should be the same as the other ones, can we just hardcode them in the JSM?

::: browser/locales/en-US/chrome/browser/charsetMenu.dtd
@@ +14,5 @@
> +
> +<!ENTITY charsetMenuAutodet.off.label  "(off)">
> +<!ENTITY charsetMenuAutodet.ja.label   "Japanese">
> +<!ENTITY charsetMenuAutodet.ru.label   "Russian">
> +<!ENTITY charsetMenuAutodet.uk.label   "Ukranian">

Please add accesskeys for these, too.

@@ +18,5 @@
> +<!ENTITY charsetMenuAutodet.uk.label   "Ukranian">
> +
> +<!-- Australis widget -->
> +<!ENTITY charsetCustomize.label        "Customize List…">
> +<!ENTITY charsetCustomize.accesskey    "c">

Please just remove the relevant item from panelUI.xml.inc, now that Australis has landed on Nightly. I'll deal with updating the rest of the Australis widget.

::: browser/locales/en-US/chrome/browser/charsetMenu.properties
@@ +1,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +# Arabic

For readability, can you align all of these so the equals signs line up and there's at least 1 space before, and exactly 1 space after the equals sign? I'm not even fussed if you do it per section or globally, but it'll make it a lot easier to read this file.

::: browser/modules/CharsetMenu.jsm
@@ +5,5 @@
> +this.EXPORTED_SYMBOLS = [ "CharsetMenu" ];
> +
> +const Ci = Components.interfaces;
> +const Cc = Components.classes;
> +const Cu = Components.utils;

nit:

const { classes: Cc, interfaces: Ci, utils: Cu} = Components;

@@ +9,5 @@
> +const Cu = Components.utils;
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +const encodings = [

Nit: by convention this should be kEncodings if it's a const.

As there is no order in this list, please use new Set([...]).

@@ +12,5 @@
> +
> +const encodings = [
> +  // Globally relevant, commented out because hard-coded in XUL
> +  // "UTF-8",
> +  // "windows-1252",

As per my previous comment, I'd prefer it if this did include these...

@@ +62,5 @@
> +  // "ISO-8859-14",
> +  // "ISO-8859-15",
> +  // "ISO-8859-16",
> +  // "macintosh"
> +];

And then you'd most likely want to define another constant being an ordered list of which items ought to appear at the top.

@@ +86,5 @@
> +      }
> +      menuItem.setAttribute("id", "charset." + encoding);
> +      return menuItem;
> +    }
> +    

Nit: trailing whitespace, please also fix below after the comments.

@@ +87,5 @@
> +      menuItem.setAttribute("id", "charset." + encoding);
> +      return menuItem;
> +    }
> +    
> +    let list = [];

You'd then be able to clone the set of encodings (let encodings = new Set(kEncodings)), and iterate over the fixed encodings, insert them before the separator and remove them from the cloned set.

@@ +88,5 @@
> +      return menuItem;
> +    }
> +    
> +    let list = [];
> +    for (let encoding of encodings) {

And then iterate over the cloned set with the fixed items removed here.

@@ +94,5 @@
> +    }
> +
> +    list.sort(function (a, b) {
> +      let titleA = a.getAttribute("label");
> +      let titleB = b.getAttribute("label");

DOM access like this in the sort loop makes me sad, but I guess there are few enough items that it shouldn't really matter.

@@ +109,5 @@
> +      if (comp) {
> +        return comp;
> +      }
> +      // secondarily reverse sort by encoding name to sort "windows" or 
> +      // "shift_jis" first.

This makes me somewhat uncomfortable, but I don't really see a better way. Note that you're assuming that the reverse sort is correct even in different locales. Is that assumption valid?

::: toolkit/components/intl/nsCharsetMenu.cpp
@@ -132,5 @@
>  
>    static nsIRDFDataSource * mInner;
>  
>    bool mInitialized;
> -  bool mBrowserMenuInitialized;

I'm not a good reviewer for this removal, but I'm sure Unfocused will be happy to oblige.

::: toolkit/components/viewsource/content/viewSource.xul
@@ -7,5 @@
>  <?xml-stylesheet href="chrome://global/skin/" type="text/css"?> 
>  <?xml-stylesheet href="chrome://global/content/viewSource.css" type="text/css"?>
>  <?xml-stylesheet href="chrome://mozapps/skin/viewsource/viewsource.css" type="text/css"?>
>  <?xul-overlay href="chrome://global/content/editMenuOverlay.xul"?>
> -<?xul-overlay href="chrome://global/content/charsetOverlay.xul"?>

Please file a followup to reintroduce this menu in view source.
Attachment #8334489 - Flags: review?(gijskruitbosch+bugs) → feedback+
(In reply to Gordon P. Hemsley [:GPHemsley] from comment #48)
> I realize this is not likely to ever become relevant, but I just want to
> note for the record that 'off' has the potential to become a valid language
> subtag in the future.

That doesn't matter. The probability of "off" becoming a valid language *and* an "off"-specific detector being introduced is 0. Considering the other browsers get away with having detection only for Japanese and making their Japanese detection less intrusive than ours, I expect the direction in Gecko to be towards having detection only for Japanese and in a way that matches Trident or WebKit/Blink.

> Maybe use '_off_' instead?

browser.js treats "off" as a magic string, so this is consistent with that.
Blocks: 940907
(In reply to :Gijs Kruitbosch from comment #49)
> For the toolkit removals, please get an rs
> from Unfocused or another toolkit peer. (I'm suggesting Blair because I
> suspect he'll be very happy to get this code out of our repo... :-) )

OK.

> ::: browser/base/content/browser-charsetmenu.inc
> @@ +31,1 @@
> >  #ifndef OMIT_ACCESSKEYS
> 
> This is the only file in the world that uses this, and nobody defines it
> ever. Let's take this opportunity to obliterate this ifdef throughout this
> file.

Done.

> @@ +33,3 @@
> >  #endif
> > +    />
> > +    <menuitem type="radio" name="charsetGroup" id="charset.window-1252" label="&charsetMenu.western.label;"
> 
> Because these are going to be hardcoded but should be the same as the other
> ones, can we just hardcode them in the JSM?

Done.

> ::: browser/locales/en-US/chrome/browser/charsetMenu.dtd
> @@ +14,5 @@
> > +
> > +<!ENTITY charsetMenuAutodet.off.label  "(off)">
> > +<!ENTITY charsetMenuAutodet.ja.label   "Japanese">
> > +<!ENTITY charsetMenuAutodet.ru.label   "Russian">
> > +<!ENTITY charsetMenuAutodet.uk.label   "Ukranian">
> 
> Please add accesskeys for these, too.

Done. Added access keys for many encodings, too, while at it.

> @@ +18,5 @@
> > +<!ENTITY charsetMenuAutodet.uk.label   "Ukranian">
> > +
> > +<!-- Australis widget -->
> > +<!ENTITY charsetCustomize.label        "Customize List…">
> > +<!ENTITY charsetCustomize.accesskey    "c">
> 
> Please just remove the relevant item from panelUI.xml.inc, now that
> Australis has landed on Nightly. I'll deal with updating the rest of the
> Australis widget.

Done.

> ::: browser/locales/en-US/chrome/browser/charsetMenu.properties
> @@ +1,5 @@
> > +# This Source Code Form is subject to the terms of the Mozilla Public
> > +# License, v. 2.0. If a copy of the MPL was not distributed with this
> > +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> > +
> > +# Arabic
> 
> For readability, can you align all of these so the equals signs line up and
> there's at least 1 space before, 

Done.

> and exactly 1 space after the equals sign?

Done for the labels. For access keys, though, I aligned the access key letters with the corresponding letter in the UI label.

> ::: browser/modules/CharsetMenu.jsm
> @@ +5,5 @@
> > +this.EXPORTED_SYMBOLS = [ "CharsetMenu" ];
> > +
> > +const Ci = Components.interfaces;
> > +const Cc = Components.classes;
> > +const Cu = Components.utils;
> 
> nit:
> 
> const { classes: Cc, interfaces: Ci, utils: Cu} = Components;

Done.

> @@ +9,5 @@
> > +const Cu = Components.utils;
> > +
> > +Cu.import("resource://gre/modules/Services.jsm");
> > +
> > +const encodings = [
> 
> Nit: by convention this should be kEncodings if it's a const.

Done.

> As there is no order in this list, please use new Set([...]).

Done.

> @@ +12,5 @@
> > +
> > +const encodings = [
> > +  // Globally relevant, commented out because hard-coded in XUL
> > +  // "UTF-8",
> > +  // "windows-1252",
> 
> As per my previous comment, I'd prefer it if this did include these...

Introduced a new constant array for the two pinned items.

> @@ +62,5 @@
> > +  // "ISO-8859-14",
> > +  // "ISO-8859-15",
> > +  // "ISO-8859-16",
> > +  // "macintosh"
> > +];
> 
> And then you'd most likely want to define another constant being an ordered
> list of which items ought to appear at the top.

Done.

> @@ +86,5 @@
> > +      }
> > +      menuItem.setAttribute("id", "charset." + encoding);
> > +      return menuItem;
> > +    }
> > +    
> 
> Nit: trailing whitespace, please also fix below after the comments.

I think I've zapped trailing whitespace now.

> @@ +87,5 @@
> > +      menuItem.setAttribute("id", "charset." + encoding);
> > +      return menuItem;
> > +    }
> > +    
> > +    let list = [];
> 
> You'd then be able to clone the set of encodings (let encodings = new
> Set(kEncodings)), and iterate over the fixed encodings, insert them before
> the separator and remove them from the cloned set.

Done.

> @@ +88,5 @@
> > +      return menuItem;
> > +    }
> > +    
> > +    let list = [];
> > +    for (let encoding of encodings) {
> 
> And then iterate over the cloned set with the fixed items removed here.

Done.

> @@ +94,5 @@
> > +    }
> > +
> > +    list.sort(function (a, b) {
> > +      let titleA = a.getAttribute("label");
> > +      let titleB = b.getAttribute("label");
> 
> DOM access like this in the sort loop makes me sad, but I guess there are
> few enough items that it shouldn't really matter.

The sorting happens at most once per Firefox session and even that one time seems fast enough to me.

> @@ +109,5 @@
> > +      if (comp) {
> > +        return comp;
> > +      }
> > +      // secondarily reverse sort by encoding name to sort "windows" or 
> > +      // "shift_jis" first.
> 
> This makes me somewhat uncomfortable, but I don't really see a better way.
> Note that you're assuming that the reverse sort is correct even in different
> locales. Is that assumption valid?

It is valid, because the secondary sort happens in the lexicographic order on the ids which are not localized.

> ::: toolkit/components/intl/nsCharsetMenu.cpp
> @@ -132,5 @@
> >  
> >    static nsIRDFDataSource * mInner;
> >  
> >    bool mInitialized;
> > -  bool mBrowserMenuInitialized;
> 
> I'm not a good reviewer for this removal, but I'm sure Unfocused will be
> happy to oblige.

OK.

> ::: toolkit/components/viewsource/content/viewSource.xul
> @@ -7,5 @@
> >  <?xml-stylesheet href="chrome://global/skin/" type="text/css"?> 
> >  <?xml-stylesheet href="chrome://global/content/viewSource.css" type="text/css"?>
> >  <?xml-stylesheet href="chrome://mozapps/skin/viewsource/viewsource.css" type="text/css"?>
> >  <?xul-overlay href="chrome://global/content/editMenuOverlay.xul"?>
> > -<?xul-overlay href="chrome://global/content/charsetOverlay.xul"?>
> 
> Please file a followup to reintroduce this menu in view source.

Bug 940907.

Thanks!
Attachment #8334489 - Attachment is obsolete: true
Attachment #8334498 - Attachment is obsolete: true
Attachment #8335192 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8335192 [details] [diff] [review]
Implement as JSM, address review comments

Blair, could you please review / rubber-stamp the toolkit removals in this patch per comment 49? As noted in comment 45, this patch intentionally doesn't remove toolkit code that becomes dead code in Firefox but is still used in Thunderbird. (That removal can happen as a follow-up at a pace that's polite to the Thunderbird developers.)
Attachment #8335192 - Flags: review?(bmcbride)
Attached image Screenshot of what the patch implements (obsolete) —
CCing standard8 for Thunderbird to arrange for the filing of the TB (or c-c) bug.
Status: NEW → ASSIGNED
Comment on attachment 8335192 [details] [diff] [review]
Implement as JSM, address review comments

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

(In reply to Henri Sivonen (:hsivonen) from comment #51)
> > @@ +94,5 @@
> > > +    }
> > > +
> > > +    list.sort(function (a, b) {
> > > +      let titleA = a.getAttribute("label");
> > > +      let titleB = b.getAttribute("label");
> > 
> > DOM access like this in the sort loop makes me sad, but I guess there are
> > few enough items that it shouldn't really matter.
> 
> The sorting happens at most once per Firefox session and even that one time
> seems fast enough to me.


Errm, no, it'll happen once per container node, which is ~1 times per window, but possibly more than once considering we also have the Australis widget. Anyway, I do think that the list is likely small enough that optimizing this (which isn't super-straightforward) would be premature, err, optimization. So let's leave it like this for now.


This looks good enough to get r+ from me. Some nits below, though.

::: browser/locales/en-US/chrome/browser/charsetMenu.properties
@@ +2,5 @@
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +# LOCALIZATION NOTE: The property keys ending with ".key" are for access keys.
> +# Localizations may add or delete properties where the property key ends with 

Nit: Trailing whitespace here and in the rest of this comment

@@ +91,5 @@
> +EUC-KR.key       = K
> +EUC-KR           = Korean
> +
> +# Thai
> +windows-874.key  =  h

I don't think access keys are case sensitive, and you used H for hebrew. Probably should leave out either/both, and the Thai/Hebrew l10n teams can favour 'their' encodings.

::: browser/modules/CharsetMenu.jsm
@@ +13,5 @@
> + *  - XSS-dangerous encodings (except ISO-2022-JP which is assumed to be
> + *    too common not to be included).
> + *  - x-user-defined, which practically never makes sense as an end-user-chosen
> + *    override.
> + *  - Encodings that IE11 doesn't have it its correspoding menu.

Nit: in its...

@@ +84,5 @@
> +      // Detector menu or charset menu already built
> +      return;
> +    }
> +    let doc = parent.ownerDocument;
> +    let sb = Services.strings.createBundle("chrome://browser/locale/charsetMenu.properties");

Sorry I missed this last time. Please make this a lazy getter at the top of the file:

Cu.import("resource://gre/modules/XPCOMUtils.jsm");
XPCOMUtils.defineLazyGetter(this, "gBundle", function() {
  const kUrl = "chrome://browser/locale/charsetMenu.properties";
  return Services.strings.createBundle(kUrl);
});

@@ +109,5 @@
> +
> +    // Clone the set in order to be able to remove the pinned encodings from
> +    // the cloned set.
> +    let encodings = new Set(kEncodings);
> +    

Nit: trailing whitespace

@@ +114,5 @@
> +    for (let encoding of kPinned) {
> +      encodings.delete(encoding);
> +      parent.appendChild(createItem(encoding));
> +    }
> +    

Nit: more trailing whitespace

@@ +116,5 @@
> +      parent.appendChild(createItem(encoding));
> +    }
> +    
> +    parent.appendChild(doc.createElement("menuseparator"));
> +    

Nit: and some more
Attachment #8335192 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to Gijs Kruitbosch from comment #55)
> I don't think access keys are case sensitive
They're case preference. For instance if you have the string "Windows-1252" then "W" will match the first letter, "w" the sixth letter and both "N" and "n" will match the third letter.
Sorry, I misunderstood the comment. Access keys are case insensitive, so you shouldn't duplicate them in the same menu (assuming they are in the same menu; I didn't go as far as verifying that).
(In reply to :Gijs Kruitbosch from comment #55)
> This looks good enough to get r+ from me. Some nits below, though.

Thanks!

> ::: browser/locales/en-US/chrome/browser/charsetMenu.properties
> @@ +2,5 @@
> > +# License, v. 2.0. If a copy of the MPL was not distributed with this
> > +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> > +
> > +# LOCALIZATION NOTE: The property keys ending with ".key" are for access keys.
> > +# Localizations may add or delete properties where the property key ends with 
> 
> Nit: Trailing whitespace here and in the rest of this comment

Fixed.

> @@ +91,5 @@
> > +EUC-KR.key       = K
> > +EUC-KR           = Korean
> > +
> > +# Thai
> > +windows-874.key  =  h
> 
> I don't think access keys are case sensitive, and you used H for hebrew.
> Probably should leave out either/both, and the Thai/Hebrew l10n teams can
> favour 'their' encodings.

Oops. Changed "Thai" to "i" and then "Japanese (ISO-2022-JP) to "n" and then for consistency, changed EUC-JP from "P" to "p" so that all instances of "Japanese" have the access key within that word.

> ::: browser/modules/CharsetMenu.jsm
> @@ +13,5 @@
> > + *  - XSS-dangerous encodings (except ISO-2022-JP which is assumed to be
> > + *    too common not to be included).
> > + *  - x-user-defined, which practically never makes sense as an end-user-chosen
> > + *    override.
> > + *  - Encodings that IE11 doesn't have it its correspoding menu.
> 
> Nit: in its...

Fixed.

> @@ +84,5 @@
> > +      // Detector menu or charset menu already built
> > +      return;
> > +    }
> > +    let doc = parent.ownerDocument;
> > +    let sb = Services.strings.createBundle("chrome://browser/locale/charsetMenu.properties");
> 
> Sorry I missed this last time. Please make this a lazy getter at the top of
> the file:
> 
> Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> XPCOMUtils.defineLazyGetter(this, "gBundle", function() {
>   const kUrl = "chrome://browser/locale/charsetMenu.properties";
>   return Services.strings.createBundle(kUrl);
> });

Done.

> Nit: trailing whitespace

Aargh. Eclipse keeps introducing these. I think I got rid of them now.

Still requesting Blair's r/rs on the toolkit removals per earlier comments.
Attachment #8335192 - Attachment is obsolete: true
Attachment #8335193 - Attachment is obsolete: true
Attachment #8335192 - Flags: review?(bmcbride)
Attachment #8335252 - Flags: review?(bmcbride)
Attachment #8335252 - Attachment is patch: true
Blocks: 942802
No longer depends on: 844042
Attachment #8335252 - Flags: review?(bmcbride) → review+
Blocks: 943252
Filed bug 943252 as a follow-up.
Blocks: 943256
Android follow-up: bug 943262
Forgot to remove [leave open]?
(In reply to Masatoshi Kimura [:emk] from comment #63)
> Forgot to remove [leave open]?

Oops.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → Firefox 28
The patch that landed with this bug touched Australis code, yet it didn't follow the rule of having Australis in the changeset summary so it could be backed out upon merging to Holly. I'll see about getting the notice added to the mozilla-inbound tree.

Gijs has volunteered to try to clean up the code in-tree to fix the two oranges that got introduced by this patch on the holly tree so this patch doesn't need to be backed out.

> TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_bug616836.js | there should be no items with access keys in the app menu popup - Got 12, expected 0
> TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_duplicateIDs.js | chardet.off should be unique
> TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_duplicateIDs.js | chardet.ja_parallel_state_machine should be unique
> TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_duplicateIDs.js | chardet.ruprob should be unique
> TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_duplicateIDs.js | chardet.ukprob should be unique
> TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_duplicateIDs.js | chardet.off should be unique
> TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_duplicateIDs.js | chardet.ja_parallel_state_machine should be unique
> TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_duplicateIDs.js | chardet.ruprob should be unique
> TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_duplicateIDs.js | chardet.ukprob should be unique
Depends on: 943524
Depends on: 943732
(In reply to Jared Wein [:jaws] from comment #65)
> The patch that landed with this bug touched Australis code, yet it didn't
> follow the rule of having Australis in the changeset summary so it could be
> backed out upon merging to Holly.

Sorry. I was not aware of such a rule. Just now, to check if I had missed a chance to become aware of such a rule, I searched the mailing lists that I subscribe to for Australis and holly and didn't find a message announcing the rule. It might be prudent to send an email about this to dev-planning.

In any case, this patch should be in the non-Australis tree (in a non-Australis state), since this enables Gecko-side progress starting with bug 942802.

> I'll see about getting the notice added to
> the mozilla-inbound tree.

Thank you.
 
> Gijs has volunteered to try to clean up the code in-tree to fix the two
> oranges that got introduced by this patch on the holly tree so this patch
> doesn't need to be backed out.

Thanks, Gijs.
Depends on: 946640
Depends on: 947500
Depends on: 947507
Depends on: 952736
Blocks: 416578
Depends on: 1020232
Blocks: self-xss
You need to log in before you can comment on or make changes to this bug.