Closed Bug 986769 Opened 6 years ago Closed 6 years ago

Australis: Character encoding widget broken since March 21th

Categories

(Firefox :: Toolbars and Customization, defect)

All
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 31
Tracking Status
firefox28 --- unaffected
firefox29 --- verified
firefox30 --- verified
firefox31 --- verified

People

(Reporter: soeren.hentzschel, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

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

Attachments

(3 files)

Attached image 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.
Attached image 2014-03-21.png
I also see this on OS X.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [Australis:P1]
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...
Assignee: nobody → gijskruitbosch+bugs
Blocks: 940038
Status: NEW → ASSIGNED
(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.
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)
Whiteboard: [Australis:P1] → [Australis:P1][fixed-in-fx-team]
(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... :-(
Committing two things with rs=firebot in a row seems like asking for trouble. Jared, does this look right to you?
Attachment #8395198 - Flags: review?(jaws)
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.
Attachment #8395198 - Flags: review?(jaws) → review+
https://hg.mozilla.org/mozilla-central/rev/f1cbdea2e334
https://hg.mozilla.org/mozilla-central/rev/10a1c23ba320
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P1][fixed-in-fx-team] → [Australis:P1]
Target Milestone: --- → Firefox 31
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
(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?
(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.
(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.
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).
Status: RESOLVED → VERIFIED
QA Contact: petruta.rasa
You need to log in before you can comment on or make changes to this bug.