645653 - Middle-click on reload button to duplicate orphan tabs does not create a group

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P3)

Description

[Tim Taubert [:ttaubert] (inactive)]

When middle-clicking the reload button while displaying a single orphaned tab the shown tab is duplicated but overlays the source tab (in panorama view). A group should be created containing these two tabItems.

Comment 1

[Miguel Ojeda]

Attachment: Proposed quick&dirty patch, missing test case

This quick&dirty patch fixes the problem and passes browser/base/content/test/tabview/ tests, although maybe there is a better place to handle this (I'm new to the code).

It lacks a test case for it (I will add it in another attachment ASAP).

Comment 2

[Miguel Ojeda]

Attachment: Simple test case for the bug

Test case for the bug. It duplicates a non-blank orphaned tab and checks whether a new group is created with two tabs (the original one as the first one of the group).

Without the other attachment, the test fails.
With it, the test passes.

Comment 3

[Ian Gilman [:iangilman]]

Comment on attachment 522384 [details] [diff] [review]
Proposed quick&dirty patch, missing test case

Miguel, sorry for the review tennis; we're still figuring out who does what in the new post-Fx4 world. 

Tim, can you do feedback on this bug? I can take the final review.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 522384 [details] [diff] [review]
Proposed quick&dirty patch, missing test case

Thanks Miguel for taking this!

The solution looks good to me. This also solves bug 643119 (which is covered by your test, too). One nit:

+        // XXX Bug 645653 - Middle-click on reload button to duplicate orphan tabs does not create a group

Maybe you could say what exactly we're doing here instead of just referring to the bug. The bug's title doesn't say anything about the solution and so we can save a lookup :) And this handles tab duplications in general and not just via middle-click (bug 643119).

I'll feedback your test next.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 522455 [details] [diff] [review]
Simple test case for the bug

The overall approach looks good - there are some nits and we should match the general test style.

+/* ***** BEGIN LICENSE BLOCK *****
+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1

Please use a public domain license header (see bug 629514).

>+function test() {
>+  waitForExplicitFinish();
>+  window.addEventListener("tabviewshown", onTabViewWindowLoaded, false);
>+  TabView.toggle();
>+}

We have some useful helper functions in base/content/test/tabview/head.js, so you can use:
showTabView(onTabViewWindowLoaded);

>+function afterTabLoaded(tab, callback) {

Please use afterAllTabsLoaded(callback) from head.js.

>+  let contentWindow = document.getElementById("tab-view").contentWindow;

Structure or names could change, better:
let contentWindow = TabView.getContentWindow();

>+    afterTabLoaded(originalTabItem.tab, function() {

See above.

>+    afterTabLoaded(originalTabItem.tab, function() {

Same.

>+        gBrowser.removeTab(groupItem.getChild(1).tab);
>+        finish();

Please make sure we finish the test with a single about:blank tab. We're currently finishing with (probably, please check that closing was successful) one about:mozilla tab.

F+ with all those issues addressed. Thanks again and please ask Ian for review again with your next patch version.

Comment 6

[Miguel Ojeda]

Tim, you're welcome! :) I've tried to use afterAllTabsLoaded(), but it does not work when loading a page in the current tab: I get readyState == "complete", so afterAllTabsLoaded() does not addEventListener(). Do you know how to solve it?

Ian, don't worry for the "review tennis", in fact, I didn't expect the review to take place so soon! I am attaching the new patch (it includes both the test case and the fix), please review.

Comment 7

[Miguel Ojeda]

Attachment: Proper patch (includes test case)

* I fixed all the issues detected by Tim expect for afterAllTabsLoaded(), I can only use it in one case.
* I fixed a bug in afterTabLoaded() -I was not removing the event listener correctly-.
* I added a check to verify that the new tab has loadad about:mozilla (these tests fail if you use afterAllTabsLoaded()).
* I changed openURLLinkIn() to loadURI().

Comment 8

[Ian Gilman [:iangilman]]

Comment on attachment 522969 [details] [diff] [review]
Proper patch (includes test case)

Looks great! Thank you for doing this.

Comment 9

[Ian Gilman [:iangilman]]

Tim, sounds like there's a problem with afterAllTabsLoaded, if it doesn't work for Miguel here? Perhaps file a follow up to fix that issue?

Comment 10

[Miguel Ojeda]

Ian, thanks for the prompt review!

About afterAllTabsLoaded, it would be nice to know what it is happening. I just started reading Firefox's code a couple of days ago, so I suppose I'm missing something fundamental, but at the moment afterAllTabsLoaded can't be used for waiting for pages loaded in the current tab.

It may be possible to fix it checking inside the for loop whether the tab is the current selected tab and it is loading (I don't know how to check for the second condition though, I thought readyState was meant for that). I will take a look tomorrow again.

Comment 11

[Ian Gilman [:iangilman]]

Ah, so the crucial difference here is that you're waiting for a tab that already has content to load content for a new URL, whereas afterAllTabsLoaded expects all of the tabs to be fresh tabs.

Maybe that is a different enough scenario that we can't rely on the same function to handle both. On the other hand, maybe there is some sort of flag we can check for "even though the old page was loaded, now we need to be loading this new URL".

Comment 12

[Miguel Ojeda]

Ian, sorry for the delay, I will see this weekend if I can implement a function for handling both cases. This week is being quite busy on my side.

In any case, I think we should create a separate patch for that, which would add the function to head.js and change the existing code to use it. What do you think?

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Patch including head.js changes

Hey Miguel. I just re-read this bug and head a quick idea that seems to work. I updated your test to use afterAllTabsLoaded() and of course updated the function itself.

Pushed to try:

http://tbpl.mozilla.org/?tree=MozillaTry&pusher=[e-mail]&rev=9a33afeca9b0

Comment 14

[Ian Gilman [:iangilman]]

Comment on attachment 524399 [details] [diff] [review]
Patch including head.js changes

Beautiful, thank you.

Comment 15

[Miguel Ojeda]

Thank you, Tim! It worked perfectly!

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 524399 [details] [diff] [review]
Patch including head.js changes

Passed try:

http://tbpl.mozilla.org/?tree=MozillaTry&pusher=[e-mail]&rev=9a33afeca9b0

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch for checkin

Thanks again, Miguel!

Comment 18

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/75ff341e00a6

Comment 19

[George Carstoiu]

Verified with Mozilla/5.0 (Windows NT 5.1; rv:2.2a1pre) Gecko/20110410 Firefox/4.2a1pre