986769 - Australis: Character encoding widget broken since March 21th

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Sören Hentzschel]

Attachment: 2014-03-20.png

The character encoding widget is broken in current nightly. Screenshot 1 shows the Nightly from March 20th, screenshot 2 shows the Nightly from March 21th.

Comment 1

[Sören Hentzschel]

Attachment: 2014-03-21.png

Comment 2

[Justin Dolske [:Dolske]]

I also see this on OS X.

Comment 3

[:Gijs (he/him)]

Regression range: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e84a391b604b&tochange=c148f0b0c8b4

Comment 4

[:Gijs (he/him)]

This pops up an error in the browser console "doc is not defined" from this line in CustomizableUI.jsm:

        addShortcut(item, doc, elem);

Which makes sense because it's been copy-pasted and in this context, there is no 'doc'. So it seems this was caused by the patch for bug 940038. I'm doublechecking now that there's no other cases...

Comment 5

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #4)
> This pops up an error in the browser console "doc is not defined" from this
> line in CustomizableUI.jsm:


Err, CustomizableWidgets.jsm. D'oh.

Comment 6

[:Gijs (he/him)]

The other calls are all correct, so this was the only issue.

Firebot graciously agreed with me that we could at least just pass the right aDocument here to make the error shut up, and the view work. :-)

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


(I was about to do uplifts, and will make sure to uplift this fix into the aurora/beta pushes for bug 940038)

Comment 7

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #6)
> The other calls are all correct, so this was the only issue.
> 
> Firebot graciously agreed with me that we could at least just pass the right
> aDocument here to make the error shut up, and the view work. :-)
> 
> remote:   https://hg.mozilla.org/integration/fx-team/rev/f1cbdea2e334
> 
> 
> (I was about to do uplifts, and will make sure to uplift this fix into the
> aurora/beta pushes for bug 940038)

Yeah, except that item isn't a node, and so this isn't enough. The charset view doesn't have shortcuts, and isn't copying other nodes (unlike the devtools and sidebars view), so the whole function call doesn't really make sense... :-(

Comment 8

[:Gijs (he/him)]

Attachment: remove call to addShortcut entirely,

Committing two things with rs=firebot in a row seems like asking for trouble. Jared, does this look right to you?

Comment 9

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

Comment on attachment 8395198 [details] [diff] [review]
remove call to addShortcut entirely,

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

Please adjust the commit message to specify that this is for the character encoding widget.

Comment 10

[:Gijs (he/him)]

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

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f1cbdea2e334
https://hg.mozilla.org/mozilla-central/rev/10a1c23ba320

Comment 12

[:Gijs (he/him)]

Landed on aurora and beta because the original patch had approval, but as one cset, because modifying the same line twice seemed senseless.

https://hg.mozilla.org/releases/mozilla-aurora/rev/e7628fbb07b6
https://hg.mozilla.org/releases/mozilla-beta/rev/0c960cf82b4c

Comment 13

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

(In reply to :Gijs Kruitbosch from comment #7)
> (In reply to :Gijs Kruitbosch from comment #6)
> > The other calls are all correct, so this was the only issue.
> > 
> > Firebot graciously agreed with me that we could at least just pass the right
> > aDocument here to make the error shut up, and the view work. :-)
> > 
> > remote:   https://hg.mozilla.org/integration/fx-team/rev/f1cbdea2e334
> > 
> > 
> > (I was about to do uplifts, and will make sure to uplift this fix into the
> > aurora/beta pushes for bug 940038)
> 
> Yeah, except that item isn't a node, and so this isn't enough. The charset
> view doesn't have shortcuts, and isn't copying other nodes (unlike the
> devtools and sidebars view), so the whole function call doesn't really make
> sense... :-(

So does this entail that you checked this in without either testing or review?

Comment 14

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #13)
> (In reply to :Gijs Kruitbosch from comment #7)
> > (In reply to :Gijs Kruitbosch from comment #6)
> > > The other calls are all correct, so this was the only issue.
> > > 
> > > Firebot graciously agreed with me that we could at least just pass the right
> > > aDocument here to make the error shut up, and the view work. :-)
> > > 
> > > remote:   https://hg.mozilla.org/integration/fx-team/rev/f1cbdea2e334
> > > 
> > > 
> > > (I was about to do uplifts, and will make sure to uplift this fix into the
> > > aurora/beta pushes for bug 940038)
> > 
> > Yeah, except that item isn't a node, and so this isn't enough. The charset
> > view doesn't have shortcuts, and isn't copying other nodes (unlike the
> > devtools and sidebars view), so the whole function call doesn't really make
> > sense... :-(
> 
> So does this entail that you checked this in without either testing or
> review?

I messed up how I tested.

It was a weekend, and I would have liked the widget to have been fixed in the next nightly (which didn't end up working out either way because of unrelated actual-turning-the-tree-non-green bustage on fx-team meaning there were no merges before nightly on the 23rd), so I rushed more than I should have done. Mea culpa.

I went and finally did some reviews in bug 967000, so hopefully we will have automated tests for the character encoding widget and some of its friends soon. We should probably work to have automated tests for all of the subviews (including checking that they show what they should be showing).

I also think we should probably update some of the subview code to have clearer variable names. Note how several of the subviews are using "item" and "elem" and they're not actually the same kind of things in all cases (because really, 'item' could be anything).

Finally, we could and maybe should update addShortcut to not even take an aDocument parameter. It's always the node's ownerDocument, so there's no need to pass it.

Comment 15

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

(In reply to :Gijs Kruitbosch from comment #14) 

Thanks for the reply. This was something that I missed in my initial review so I was sad that a mistake was made again. That's good to hear about bug 967000 :)

> I also think we should probably update some of the subview code to have
> clearer variable names. Note how several of the subviews are using "item"
> and "elem" and they're not actually the same kind of things in all cases
> (because really, 'item' could be anything).
> 
> Finally, we could and maybe should update addShortcut to not even take an
> aDocument parameter. It's always the node's ownerDocument, so there's no
> need to pass it.

Yeah, we should do this. I just filed bug 987144 for this.

Comment 16

[Petruta Horea [:phorea], Desktop Test Engineering]

Reproduced on Mac OS X 10.8.5 on Nightly 2014-03-22.
Verified as fixed on Firefox 29 beta 2 (20140324101726), Aurora 30.0a2 (20140324150430) and Nightly 31.0a1 (20140324030203).