586553 - Optimize tab movement in reorderTabsBasedOnTabItemOrder

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect, P3)

Description

[Michael Yoshitaka Erlewine [:mitcho]]

Just what it says. :)

Comment 1

[(no longer active)]

Mass moving all Tab Candy bugs from Mozilla Labs to Firefox::Tab Candy.  Filter the bugmail spam with "tabcandymassmove".

Comment 2

[Aza Raskin [:aza]]

Mitcho, care to elaborate a bit? Is this a Firefox 4 sorta thing? I'm not sure I understand yet.

Comment 3

[Aza Raskin [:aza]]

Mitcho, I'm punting this to the future as I don't understand it. Feel free to drag it back.

Comment 4

[Michael Yoshitaka Erlewine [:mitcho]]

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.

Comment 5

[Michael Yoshitaka Erlewine [:mitcho]]

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.

Comment 6

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch v1

Comment 7

[Aza Raskin [:aza]]

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).

Comment 8

[Ian Gilman [:iangilman]]

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.

Comment 9

[Michael Yoshitaka Erlewine [:mitcho]]

(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.

Comment 10

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch v2

Incorporating Ian's feedback.

Comment 11

[Ian Gilman [:iangilman]]

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. :)

Comment 12

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch v2.1

Comment 13

[Michael Yoshitaka Erlewine [:mitcho]]

While minor, this is a perf bug. Requesting blocking.

Comment 14

[Ian Gilman [:iangilman]]

(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.

Comment 15

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch 2.1, updated for rot

Sent to try: 964cbaec9f78

Can we get approval+ on this?

Comment 16

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch for checkin

Still waiting a bit to make sure try is completely clean... but should be, and
looking good so far.

Comment 17

[Michael Yoshitaka Erlewine [:mitcho]]

Nevermind... ready for checkin. No failures. Win debug didn't build, but that seems to be a general issue with try now.

Comment 18

[(no longer active)]

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!

Comment 19

[Dietrich Ayala (:dietrich)]

Belated blocking- because there's no indication of how much of a perf win this was.