943524 - Fix holly after bug 805374 broke tests

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Comment 1

[:Gijs (he/him)]

Attachment: fix issues in holly caused by bug 805374,

Not asking for review yet as I still need to test this, but Jared, can you check if you're OK with detecting the appmenu like this or if you have a better suggestion?

Comment 2

[:Gijs (he/him)]

Comment on attachment 8338733 [details] [diff] [review]
fix issues in holly caused by bug 805374,

Per comment #1

Comment 3

[:Gijs (he/him)]

Attachment: fix issues in holly caused by bug 805374,

Adjusted per discussion on IRC, and fixed some bits I missed earlier. Tested this, this fixes the test failures for me locally.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8338781 [details] [diff] [review]
fix issues in holly caused by bug 805374,

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

::: browser/modules/CharsetMenu.jsm
@@ +81,5 @@
>    "windows-1252"
>  ];
>  
>  this.CharsetMenu = Object.freeze({
> +  build: function BuildCharsetMenu(event, disableAccessKeys) {

nit, boolean arguments should default to the positive association. So in this case it should be: BuildCharsetMenu(event, showAccessKeys) so the code later on can be if (showAccessKeys) { ... } else { ... }

Comment 5

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/a1c1679379fd

Comment 6

[:Gijs (he/him)]

And then I had a brighter moment just before going to bed. This will break the charset detectors in the app menu because of this code:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#5100

and we strictly speaking still have duplicate IDs for the charsets that get put in the menus, as if you open both menus, both will be fully populated and will have the same IDs. I'm not sure if the RDF menu did any better in that department as I've never bothered to figure out how that one worked (ie if it would have persisted after hiding the popup, thus creating the duplicate IDs - you couldn't have both open simultaneously so if it cleaned up after itself you'd be alright).


The patch will fix the test failures and improve the situation in the sense that there'll be no more accesskeys and (initially) no duplicate IDs, but we should still land a followup patch to fix the charset detectors in the app menu and distinguish the IDs of the charsets there, too. :-\

Comment 7

[Carsten Book [:Tomcat]]

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

Comment 8

[:Gijs (he/him)]

Attachment: fix charset and chardet processing to allow for unique IDs,

So on beta at least we have duplicate IDs if you open both the charset menus (appmenu and 'normal' menu), so I suppose fixing that isn't critical. However, I figured that while we're here we might as well do the whole thing properly, especially as we'll soon be duplicating stuff for Australis as well (we need to update the charset widget there to have the same options as this thing).

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8339246 [details] [diff] [review]
fix charset and chardet processing to allow for unique IDs,

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

::: browser/modules/CharsetMenu.jsm
@@ +81,5 @@
>    "windows-1252"
>  ];
>  
>  this.CharsetMenu = Object.freeze({
> +  build: function BuildCharsetMenu(event, idPrefix="", showAccessKeys) {

Because you introduced a default parameter to the second argument, you should set an explicit default parameter for the third argument. In this case, it's not super necessary (showAccessKeys being undefined will still be falsy), but it should be more obvious to developers in the future that the third argument is optional.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8339246 [details] [diff] [review]
fix charset and chardet processing to allow for unique IDs,

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

::: browser/base/content/browser.js
@@ +5086,5 @@
>          BrowserCharsetReload();
>          SelectDetector(event, false);
>      } else if (name == 'charsetGroup') {
>          var charset = node.getAttribute('id');
> +        charset = charset.substring(charset.indexOf('charset.') + 'charset.'.length);

I'm not sure how necessary this is, but we should probably have some check for if 'charset.' doesn't exist here? (-1 + 'charset.'.length will produce quite the unexpected result :) Ditto for 'chardet.'.

Comment 11

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] from comment #10)
> Comment on attachment 8339246 [details] [diff] [review]
> fix charset and chardet processing to allow for unique IDs,
> 
> Review of attachment 8339246 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser.js
> @@ +5086,5 @@
> >          BrowserCharsetReload();
> >          SelectDetector(event, false);
> >      } else if (name == 'charsetGroup') {
> >          var charset = node.getAttribute('id');
> > +        charset = charset.substring(charset.indexOf('charset.') + 'charset.'.length);
> 
> I'm not sure how necessary this is, but we should probably have some check
> for if 'charset.' doesn't exist here? (-1 + 'charset.'.length will produce
> quite the unexpected result :) Ditto for 'chardet.'.

The result would have been similarly unexpected in the previous incarnation (which hardcoded substring'ing from just 'charset.'.length), but if you want I could add the check, I suppose.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

I'm indifferent, and in that I guess we should leave off the check. I don't see it helping us, and it will just add more noise to the code. A test that covered this would be perfect as it would provide a good separation but also make sure this doesn't break in the future. Do we have one for it? If not, you can file a follow-up bug to add a test for it.

Comment 13

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/daadeaee9c25

Comment 14

[Carsten Book [:Tomcat]]

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