bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

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

NEW
Unassigned

Status

()

Firefox
Tabbed Browser
P5
normal
a year ago
10 months ago

People

(Reporter: jaws, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

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.

Updated

10 months ago
Priority: -- → P5
You need to log in before you can comment on or make changes to this bug.