Closed Bug 586553 Opened 14 years ago Closed 14 years ago

Optimize tab movement in reorderTabsBasedOnTabItemOrder

Categories

(Firefox Graveyard :: Panorama, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 4.0b8

People

(Reporter: mitcho, Assigned: mitcho)

Details

(Keywords: perf, Whiteboard: [good first bug], [qa-])

Attachments

(1 file, 4 obsolete files)

Just what it says. :)
Mass moving all Tab Candy bugs from Mozilla Labs to Firefox::Tab Candy.  Filter the bugmail spam with "tabcandymassmove".
Product: Mozilla Labs → Firefox
Target Milestone: -- → ---
Version: unspecified → Trunk
QA Contact: tabcandy → tabcandy
Keywords: helpwanted
Priority: -- → P3
Mitcho, care to elaborate a bit? Is this a Firefox 4 sorta thing? I'm not sure I understand yet.
Mitcho, I'm punting this to the future as I don't understand it. Feel free to drag it back.
Target Milestone: --- → Future
Thanks for checking Aza. I don't think this is a fx4 sort of thing, but it is a possibly performance-improving refactoring, so could end up being a good fx4 item. I'll do some investigating/test-writing now to see if this is something to really pursue from a perf point of view.
Whiteboard: [perf][good first bug]
Keywords: perf
Whiteboard: [perf][good first bug] → [good first bug]
I've established that we do indeed often make many more gBrowser.moveTabTo calls (which trigger TabMove) than we need to. Working on a fix.
Assignee: nobody → mitcho
Status: NEW → ASSIGNED
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #487261 - Flags: feedback?(ian)
Mitcho good call. As a perf thing this is a b8 bug. Feel free to mark it as such (especially now that you have a patch).
Target Milestone: Future → Firefox 4.0b8
Comment on attachment 487261 [details] [diff] [review]
Patch v1

The approach looks good. Comments:

>+//        Utils.log('new indices', currentIndices, targetIndices);

Kill this.

>+      let targetRange = new Range(targetIndices[index - 1] || -1, targetIndices[index + 1] || Infinity);

What if targetIndicies[index - 1] is 0 (which will then get changed to -1 in targetRange)? Does that work out okay?

>+//        Utils.log('move ' + originalIndex + '>' + targetIndex);

Kill.

>+function onTabMove(e) {
>+  let tab = e.target;
>+  contentWindow.Utils.log(tab.linkedBrowser.contentTitle || 'new', tab._tPos);
>+  moves++;
>+}

I suppose we can keep this log, but shouldn't it be some sort of mochitest info call?

>+  is(moves, 1, "Only one move should be necessary for this basic move.");

You should also test to make sure that the tabs were moved to the correct positions.

>\ No newline at end of file

Fix.
Attachment #487261 - Flags: feedback?(ian) → feedback-
(In reply to comment #8) 
> >+      let targetRange = new Range(targetIndices[index - 1] || -1, targetIndices[index + 1] || Infinity);

Good catch. Changed the first argument to check whether index is > 0 or not.
Attached patch Patch v2 (obsolete) — Splinter Review
Incorporating Ian's feedback.
Attachment #487261 - Attachment is obsolete: true
Attachment #488127 - Flags: feedback?(ian)
Comment on attachment 488127 [details] [diff] [review]
Patch v2

Excellent, except:

>+  is(originalTab._tPos, 0, "Original tab is in position 0");
>+  is(newTabs[0]._tPos, 1, "Robots is in position 1");
>+  is(newTabs[1]._tPos, 2, "Mozilla is in position 2");
>+  is(newTabs[2]._tPos, 3, "Credits is in position 3");

The text here agrees with the code... 

>+  is(originalTab._tPos, 1, "Original tab is in position 0");
>+  is(newTabs[0]._tPos, 2, "Robots is in position 1");
>+  is(newTabs[1]._tPos, 3, "Mozilla is in position 2");
>+  is(newTabs[2]._tPos, 0, "Credits is in position 3");

...the text here does not.

f+ with that fixed. :)
Attachment #488127 - Flags: feedback?(ian) → feedback+
Attached patch Patch v2.1 (obsolete) — Splinter Review
Attachment #488127 - Attachment is obsolete: true
Attachment #488714 - Flags: review?(dietrich)
While minor, this is a perf bug. Requesting blocking.
blocking2.0: --- → ?
(In reply to comment #13)
> While minor, this is a perf bug. Requesting blocking.

Note that Dietrich won't review this patch if it's not blocking.
Attachment #488714 - Flags: review?(dietrich)
Attachment #488714 - Flags: review+
Attachment #488714 - Flags: approval2.0?
Attached patch Patch 2.1, updated for rot (obsolete) — Splinter Review
Sent to try: 964cbaec9f78

Can we get approval+ on this?
Attachment #488714 - Attachment is obsolete: true
Attachment #492223 - Flags: approval2.0?
Attachment #488714 - Flags: approval2.0?
Attachment #492223 - Flags: approval2.0? → approval2.0+
Still waiting a bit to make sure try is completely clean... but should be, and
looking good so far.
Attachment #492223 - Attachment is obsolete: true
Nevermind... ready for checkin. No failures. Win debug didn't build, but that seems to be a general issue with try now.
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/9aeed6e4ec1d

The "patch for check-in" attachment did not include the unit test file, which caused the tree to turn red.  I landed the unit test separately:

http://hg.mozilla.org/mozilla-central/rev/7ce0cc6b8cc9

Please ensure that the patches have all of the necessary files in the future to prevent such incidents.  Thanks!
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Keywords: helpwanted
Belated blocking- because there's no indication of how much of a perf win this was.
blocking2.0: ? → betaN+
blocking2.0: betaN+ → ---
Whiteboard: [good first bug] → [good first bug], [qa-]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: