896986 - Recent closed tab history middle click only opens top entry

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Description

[Jim Jeffery not reading bug-mail 1/2/11]

Looks like the issue with Recently Closed tabs has returned.

STR:
Click App-button
Click Recent Closed tabs
Try to open any tab in the closed tab list - using middle-click
NOTE: that the Top tab in the 'recent closed' list is the only one that will open.


Seems to fail if link is opened with middle-click - normal click on recent closed will open the tab...
Also happens in a new profile - no addons

cset af4e3ce8c487 ok
cset 0d0263a58f06 bad

http://hg.mozilla.org/mozilla-central/p ... 0263a58f06 regression range <- 223 cset's

need help finding the m-i regressor

Comment 1

[Jim Jeffery not reading bug-mail 1/2/11]

Forgot to add that left-clicking opens the link in a 'New Tab' unlike the old way of opening it on the 'active' tab, that's why using middle-click is desired if you want to reopen it in a new tab rather than the currently displayed 'active' tab.

Comment 2

[Marco Bonardo [:mak]]

while the code lives in the history menu, it's not related to Bookmarks & History functionality, moving to menus.

Comment 3

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

This may have possibly came as a result in the change of behavior with undoCloseTab.

Comment 4

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

Attachment: Patch

HistoryMenu._undoCloseMiddleClick called undoCloseTab with the index as a String, and undoCloseTab was only accepting Number arguments. It's more important to fix undoCloseTab since there may be other callers that are passing a string as the argument.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

If you're getting rid of the restriction that it has to be passed a number, shouldn't you remove the isInteger check entirely and just always coerce? Or does it still need to distinguish undefined, in which case an explicit check would probably be better...

Comment 6

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

Attachment: Patch v2 (aurora)

Good point. The previous patch actually had a bug with this in that it entered the code path for restoring multiple tabs. This is now fixed.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Attachment: alternate patch

Your patch works, but I took this opportunity to clean up a bit:
- use explicit vs. implicit conversion (Number() vs. +)
- adjust the bounds checking code to be more readable, the old code was hard to follow

Comment 8

[:Gavin Sharp [email: [e-mail]]]

For aurora uplift, I'm fine just taking your patch FWIW.

Comment 9

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

Comment on attachment 790464 [details] [diff] [review]
alternate patch

Review of attachment 790464 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.js
@@ +6208,3 @@
>    }
>  
>    while (numberOfTabsToUndoClose > 0 &&

Please move the declaration of |tab| to before the while loop since it is now not referenced until later.

Comment 10

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

Comment on attachment 790447 [details] [diff] [review]
Patch v2 (aurora)

Marking r+ based on https://bugzilla.mozilla.org/show_bug.cgi?id=896986#c8.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 887515
User impact if declined: The "Recently Closed Tabs" menu is broken.
Testing completed (on m-c, etc.): locally
Risk to taking this patch (and alternatives if risky): none expected
String or IDL/UUID changes made by this patch: none

Comment 11

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/integration/fx-team/rev/55f9c5aeece6

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 790447 [details] [diff] [review]
Patch v2 (aurora)

This doesn't handle aIndex being 0, as far as I can see. numberOfTabsToUndoClose should be 0 in that case.

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 790464 [details] [diff] [review]
alternate patch

This has the same problem.

Comment 14

[Dão Gottwald [:dao]]

backed out from fx-team: https://hg.mozilla.org/integration/fx-team/rev/4dbcd4a4369c

Comment 15

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #12)
> Comment on attachment 790447 [details] [diff] [review]
> Patch v2 (aurora)
> 
> This doesn't handle aIndex being 0, as far as I can see.
> numberOfTabsToUndoClose should be 0 in that case.

Err, numberOfTabsToUndoClose needs to be 1 of course.

Comment 16

[Dão Gottwald [:dao]]

I would recommend:

  if (isNaN(index)) {
    index = 0;
    numberOfTabsToUndoClose = ss.getNumberOfTabsClosedLast(window);
  } else {
    if (index < 0 || index >= ss.getClosedTabCount(window))
      return null;
    numberOfTabsToUndoClose = 1;
  }

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Thanks, good catch. Somewhat embarassing!

I went with your suggestion:
https://hg.mozilla.org/integration/fx-team/rev/636e3b9d1f72

We should have a test for this...

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/636e3b9d1f72

Comment 19

[Ryan VanderMeulen [:RyanVM]]

So....what needs uplifting here? The Aurora patch with a+ and r- or the patch that managed to land but doesn't have approval?

Comment 20

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

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #19)
> So....what needs uplifting here? The Aurora patch with a+ and r- or the
> patch that managed to land but doesn't have approval?

A new patch will need to be written and I'll re-request aurora approval. The patch that made it to m-c is good.

Comment 21

[:Gavin Sharp [email: [e-mail]]]

Attachment: fixed patch

Let's just go with the same patch for Aurora.

Comment 22

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fdb2be03094d

Comment 23

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

Backed out of Firefox 25 since the parent bug (887515) has been backed out of Fx25.

Comment 24

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

Backed out by bug 914258.