Closed
Bug 645653
Opened 14 years ago
Closed 14 years ago
Middle-click on reload button to duplicate orphan tabs does not create a group
Categories
(Firefox Graveyard :: Panorama, defect, P3)
Firefox Graveyard
Panorama
Tracking
(Not tracked)
VERIFIED
FIXED
Firefox 5
People
(Reporter: ttaubert, Assigned: miguel.ojeda.sandonis)
References
Details
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•14 years ago
|
||
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).
Attachment #522384 -
Flags: review?(dao)
Assignee | ||
Comment 2•14 years ago
|
||
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.
Attachment #522455 -
Flags: review?(dao)
Updated•14 years ago
|
Attachment #522384 -
Flags: review?(dao) → review?(ian)
Updated•14 years ago
|
Attachment #522455 -
Flags: review?(dao) → review?(ian)
Comment 3•14 years ago
|
||
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.
Attachment #522384 -
Flags: review?(ian) → feedback?(tim.taubert)
Updated•14 years ago
|
Attachment #522455 -
Flags: review?(ian) → feedback?(tim.taubert)
Reporter | ||
Updated•14 years ago
|
OS: Linux → All
Hardware: x86 → All
Reporter | ||
Comment 4•14 years ago
|
||
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.
Attachment #522384 -
Flags: feedback?(tim.taubert) → feedback+
Reporter | ||
Updated•14 years ago
|
Assignee: nobody → miguel.ojeda.sandonis
Status: NEW → ASSIGNED
Reporter | ||
Comment 5•14 years ago
|
||
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.
Attachment #522455 -
Flags: feedback?(tim.taubert) → feedback+
Assignee | ||
Comment 6•14 years ago
|
||
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.
Assignee | ||
Comment 7•14 years ago
|
||
* 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().
Attachment #522384 -
Attachment is obsolete: true
Attachment #522455 -
Attachment is obsolete: true
Attachment #522969 -
Flags: review?(ian)
Comment 8•14 years ago
|
||
Comment on attachment 522969 [details] [diff] [review]
Proper patch (includes test case)
Looks great! Thank you for doing this.
Attachment #522969 -
Flags: review?(ian) → review+
Comment 9•14 years ago
|
||
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?
Assignee | ||
Comment 10•14 years ago
|
||
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•14 years ago
|
||
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".
Assignee | ||
Comment 12•14 years ago
|
||
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?
Reporter | ||
Comment 13•14 years ago
|
||
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=tim.taubert@gmx.de&rev=9a33afeca9b0
Attachment #522969 -
Attachment is obsolete: true
Attachment #524399 -
Flags: review?(ian)
Comment 14•14 years ago
|
||
Comment on attachment 524399 [details] [diff] [review]
Patch including head.js changes
Beautiful, thank you.
Attachment #524399 -
Flags: review?(ian) → review+
Assignee | ||
Comment 15•14 years ago
|
||
Thank you, Tim! It worked perfectly!
Reporter | ||
Comment 16•14 years ago
|
||
Comment on attachment 524399 [details] [diff] [review]
Patch including head.js changes
Passed try:
http://tbpl.mozilla.org/?tree=MozillaTry&pusher=tim.taubert@gmx.de&rev=9a33afeca9b0
Reporter | ||
Comment 17•14 years ago
|
||
Thanks again, Miguel!
Attachment #524399 -
Attachment is obsolete: true
Reporter | ||
Updated•14 years ago
|
Keywords: checkin-needed
Comment 18•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox4.2
Comment 19•14 years ago
|
||
Verified with Mozilla/5.0 (Windows NT 5.1; rv:2.2a1pre) Gecko/20110410 Firefox/4.2a1pre
Status: RESOLVED → VERIFIED
Updated•14 years ago
|
Target Milestone: Firefox5 → Firefox 5
Updated•9 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•