1375041 - Refactor tabbrowser.xml's "drop" handler to reduce size/complexity

Status: NEW

(Firefox :: Tabbed Browser, enhancement, P5)

Description

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

Per a review comment on bug 1366060,

> ::: browser/base/content/tabbrowser.xml:7043
> (Diff revision 1)
> >                            draggedTab._dragData.animDropIndex;
> >            if (dropIndex && dropIndex > draggedTab._tPos)
> >              dropIndex--;
> >  
> > -          if (oldTranslateX && oldTranslateX != newTranslateX) {
> > +          let animate = Services.prefs.getBoolPref("toolkit.cosmeticAnimations.enabled");
> > +          if (oldTranslateX && oldTranslateX != newTranslateX && animate) {
> 
> Ugh. I mean, this code isn't hot, so I guess it doesn't matter much, but you 
> could move the `dropIndex` changes to the top of the else if clause, and then 
> check the animate bool first, before bothering doing the calculations for the 
> translateX value. That said, because it would split the if condition, the 
> complexity of the code would go up as well, and we can't return early from this 
> method because we're meant to also delete the `_dragData` off the draggedTab all 
> the way at the bottom, and we're already in an if statement so triple-nesting 
> them (which would be the result) doesn't seem attractive either.
> 

> Really, this code needs refactoring so it isn't a single 100-line method, but 
> never mind that for now... Maybe file a followup? Trying to handle all the 
> different drops in a single function without delegating to more specific ones 
> makes this code harder to work with, IMO.