Closed Bug 943524 Opened 11 years ago Closed 11 years ago

Fix holly after bug 805374 broke tests

Categories

(Firefox :: Menus, defect)

defect
Not set
blocker

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: Gijs, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [Australis:P1])

Attachments

(2 files, 1 obsolete file)

      No description provided.
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 on attachment 8338733 [details] [diff] [review]
fix issues in holly caused by bug 805374,

Per comment #1
Attachment #8338733 - Flags: feedback?(jaws)
Adjusted per discussion on IRC, and fixed some bits I missed earlier. Tested this, this fixes the test failures for me locally.
Attachment #8338781 - Flags: review?(jaws)
Attachment #8338733 - Attachment is obsolete: true
Attachment #8338733 - Flags: feedback?(jaws)
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 { ... }
Attachment #8338781 - Flags: review?(jaws) → review+
remote:   https://hg.mozilla.org/integration/fx-team/rev/a1c1679379fd
Whiteboard: [Australis:P1] → [Australis:P1][fixed-in-fx-team][leave open]
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. :-\
https://hg.mozilla.org/mozilla-central/rev/a1c1679379fd
Whiteboard: [Australis:P1][fixed-in-fx-team][leave open] → [Australis:P1][leave open]
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).
Attachment #8339246 - Flags: review?(jaws)
Attachment #8338781 - Flags: checkin+
Blocks: 936442
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.
Attachment #8339246 - Flags: review?(jaws) → review+
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.'.
(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.
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.
remote:   https://hg.mozilla.org/integration/fx-team/rev/daadeaee9c25
Whiteboard: [Australis:P1][leave open] → [Australis:P1][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/daadeaee9c25
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P1][fixed-in-fx-team] → [Australis:P1]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: