Last Comment Bug 510890 - Port Bug 394759 (Add undo close window feature) to SeaMonkey
: Port Bug 394759 (Add undo close window feature) to SeaMonkey
Status: RESOLVED FIXED
: fixed-seamonkey2.0
Product: SeaMonkey
Classification: Client Software
Component: Session Restore (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.0b2
Assigned To: Misak Khachatryan
:
Mentors:
: 39165 (view as bug list)
Depends on: 394759 547720 726521
Blocks: 511640
  Show dependency treegraph
 
Reported: 2009-08-17 07:11 PDT by Misak Khachatryan
Modified: 2012-03-09 10:51 PST (History)
3 users (show)
kairo: wanted‑seamonkey2.0+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Sessionstore backend part (9.22 KB, patch)
2009-08-17 07:11 PDT, Misak Khachatryan
neil: review+
neil: superreview+
Details | Diff | Splinter Review
Interface part (6.28 KB, patch)
2009-08-18 07:55 PDT, Misak Khachatryan
neil: ui‑review+
Details | Diff | Splinter Review
Interface part, nits fixed (6.23 KB, patch)
2009-08-20 03:06 PDT, Misak Khachatryan
no flags Details | Diff | Splinter Review
Interface part, more nits fixed (5.57 KB, patch)
2009-08-20 04:09 PDT, Misak Khachatryan
neil: review+
neil: superreview+
Details | Diff | Splinter Review
[for checkin] Backend part, nits fixed r=neil sr=neil (9.10 KB, patch)
2009-08-20 05:41 PDT, Misak Khachatryan
philip.chee: review+
philip.chee: superreview+
Details | Diff | Splinter Review
[for checkin] Interface part, all nits fixed. r=neil sr=neil (5.57 KB, patch)
2009-08-20 05:42 PDT, Misak Khachatryan
philip.chee: review+
philip.chee: superreview+
Details | Diff | Splinter Review
test (12.62 KB, patch)
2009-08-20 05:44 PDT, Misak Khachatryan
neil: review-
Details | Diff | Splinter Review
fixed test (12.70 KB, patch)
2009-08-20 22:58 PDT, Misak Khachatryan
neil: review+
Details | Diff | Splinter Review
[for checkin] tests with nits addressed (12.71 KB, patch)
2009-08-23 23:30 PDT, Misak Khachatryan
misak.bugzilla: review+
Details | Diff | Splinter Review
modified test (5.70 KB, patch)
2009-08-26 08:28 PDT, Misak Khachatryan
no flags Details | Diff | Splinter Review

Description Misak Khachatryan 2009-08-17 07:11:14 PDT
Created attachment 394813 [details] [diff] [review]
Sessionstore backend part

Summary says all
I did back end part, some questions about interface part, will add it later.
Comment 1 neil@parkwaycc.co.uk 2009-08-17 08:59:04 PDT
Comment on attachment 394813 [details] [diff] [review]
Sessionstore backend part

Looks reasonable, although I haven't tested it in any way (such as ensuring that session restore still works).
Comment 2 Misak Khachatryan 2009-08-18 07:55:43 PDT
Created attachment 395056 [details] [diff] [review]
Interface part

Well, I've made interface part like our restoreTab works, not sure it right, but working. Please comment if you want it be done like in Firefox. Tests on their way too.
Comment 3 neil@parkwaycc.co.uk 2009-08-18 09:19:45 PDT
Comment on attachment 395056 [details] [diff] [review]
Interface part

UI looks great. Just some code nits:

>+  recentWindowsItem.setAttribute("disabled", !browser || browser.getClosedWindowsList().length == 0);
What happens on the Mac when you have no windows? I'm thinking that getClosedWindowsList has to be a global function so that you can undo close window on the Mac even when no windows are open. (I actually wanted it to be a global function for other reasons anyway.)

>+            var ss = Components.classes["@mozilla.org/suite/sessionstore;1"].
>+                      getService(Components.interfaces.nsISessionStore);
Nit:
var ss = Components.classes["@mozilla.org/suite/sessionstore;1"]
                   .getService(Components.interfaces.nsISessionStore);

>+
>+            var undoItems = JSON.parse(ss.getClosedWindowData());
I think updateRecentWindows should just call this directly.

>+            if (ss.getClosedWindowCount() > (aIndex || 0))
>+              window = ss.undoCloseWindow(aIndex || 0);
I think this check should be in the session store code, not in the caller.
Comment 4 neil@parkwaycc.co.uk 2009-08-18 09:23:22 PDT
How hard would it be to return an array of just the closed window titles?
Comment 5 Misak Khachatryan 2009-08-20 03:06:39 PDT
Created attachment 395551 [details] [diff] [review]
Interface part, nits fixed
Comment 6 neil@parkwaycc.co.uk 2009-08-20 03:39:18 PDT
Comment on attachment 395551 [details] [diff] [review]
Interface part, nits fixed

>+  var ss = Components.classes["@mozilla.org/suite/sessionstore;1"].
>+                      getService(Components.interfaces.nsISessionStore);
[I'm sure I said the . ^ goes here, not at the end of the previous line...]

>+  recentWindowsItem.setAttribute("disabled", !browser || ss.getClosedWindowCount() == 0);
You don't need the !browser check here.

>+  var browser = getBrowser();
Not used.

>+  var list = [];
Delete this.

>+  var ss = Components.classes["@mozilla.org/suite/sessionstore;1"].
>+                      getService(Components.interfaces.nsISessionStore);
As above.

>+    list.push(undoItems[i].title);
>+  }
>+
>+  for (var i = 0; i < list.length; i++) {
Delete this.

>+    var label = list[i];
var label = undoItems[i].title;

>+      <method name="undoCloseWindow">
This goes in navigator.js too.

>+           let ss = Components.classes["@mozilla.org/suite/sessionstore;1"].
>+                    getService(Components.interfaces.nsISessionStore);
As above.

>+            let window = null;
>+            window = ss.undoCloseWindow(aIndex);
>+
>+            return window;
You can do all this in one line of code!
Comment 7 Misak Khachatryan 2009-08-20 04:09:39 PDT
Created attachment 395555 [details] [diff] [review]
Interface part, more nits fixed

Sorry :(
Comment 8 neil@parkwaycc.co.uk 2009-08-20 04:59:26 PDT
(In reply to comment #6)
> >+           let ss = Components.classes["@mozilla.org/suite/sessionstore;1"].
> >+                    getService(Components.interfaces.nsISessionStore);
> As above.
> 
> >+            let window = null;
> >+            window = ss.undoCloseWindow(aIndex);
> >+
> >+            return window;
> You can do all this in one line of code!
One statement to rule them all ;-)
Comment 9 neil@parkwaycc.co.uk 2009-08-20 05:19:02 PDT
Comment on attachment 394813 [details] [diff] [review]
Sessionstore backend part

>+    // default to the most-recently closed window
>+    aIndex = aIndex || 0;
This doesn't actually make any difference here because xpconnect ensures that aIndex is already an unsigned long, so it can be removed.

>+    if (!aIndex in this._closedWindows)
This test is incorrect; ! has higher precedence than in so you need ()s.
I blame Sun for this; they just weren't thinking when they gave "in" such a ridiculously low precedence :-( sr=me with this fixed.

>+      throw (Components.returnCode = Cr.NS_ERROR_INVALID_ARG);
Please return null instead so that Ctrl+Shift+Y won't error on an empty list.
Comment 10 neil@parkwaycc.co.uk 2009-08-20 05:21:21 PDT
Comment on attachment 395555 [details] [diff] [review]
Interface part, more nits fixed

>+  let ss = Components.classes["@mozilla.org/suite/sessionstore;1"]
>+                     .getService(Components.interfaces.nsISessionStore);
Would you mind changing this to be a "var" for consistency? I think it would silly if we "let" this one through ;-)
Comment 11 Misak Khachatryan 2009-08-20 05:41:10 PDT
Created attachment 395565 [details] [diff] [review]
[for checkin] Backend part, nits fixed r=neil sr=neil

Carrying forward r+ and sr+ from Neil.
Comment 12 Misak Khachatryan 2009-08-20 05:42:57 PDT
Created attachment 395566 [details] [diff] [review]
[for checkin] Interface part, all nits fixed. r=neil sr=neil

Carrying forward r+ and sr+ from Neil
Comment 13 Misak Khachatryan 2009-08-20 05:44:28 PDT
Created attachment 395567 [details] [diff] [review]
test

This is a test part, it times out now, can't understand why. Help needed.
Comment 14 neil@parkwaycc.co.uk 2009-08-20 07:31:55 PDT
Comment on attachment 395567 [details] [diff] [review]
test

> _BROWSER_FILES = browser_bug463504.js \
>+    browser_510890.js \
browser_bug510890.js

>+  /** Test for Bug 394759 **/
Oops.

>+    newWin.addEventListener("load", function(aEvent) {
This outer load is OK, but...

>+      newWin.gBrowser.addEventListener("load", function(aEvent) {
>+        newWin.gBrowser.removeEventListener("load", arguments.callee, true);
This is no good, because you listen to the "load" of the tab drag indicator.
Try using "DOMContentLoaded" and/or "pageshow" instead. Note: these events bubble, so you can change the "true" to "false" as well if you like.

>+      let window = openDialog(location, "_blank", settings, url);
Rename this variable, it's confusing.

>+        window.gBrowser.addEventListener("load", function(aEvent) {
>+          // the window _should_ have state with a tab of url, but it doesn't
>+          // always happend before window.close(). addTab ensure we don't treat
>+          // this window as a stateless window
>+          window.gBrowser.addTab();
>+          window.gBrowser.removeEventListener("load", arguments.callee, true);
I think removeEventListener should come first. Also, which event? (I tried everything but I still got errors or timeouts...)

>+      }, true);
This is probably incorrect, and should be false.
Comment 15 neil@parkwaycc.co.uk 2009-08-20 08:05:49 PDT
(In reply to comment #14)
> >+      }, true);
> This is probably incorrect, and should be false.
In fact this is correct, but gBrowser should be replaced by getBrowser()
(Firefox uses a custom gBrowser dynamic property).
Comment 16 Misak Khachatryan 2009-08-20 22:58:36 PDT
Created attachment 395785 [details] [diff] [review]
fixed test

this one passes on my Linux box, not sure i understood and did everything correct though.
Comment 17 neil@parkwaycc.co.uk 2009-08-21 03:57:21 PDT
Comment on attachment 395785 [details] [diff] [review]
fixed test

>+    newWin.addEventListener("load", function(aEvent) {
>+      newWin.getBrowser().addEventListener("pageshow", function(aEvent) {
>+        newWin.getBrowser().removeEventListener("pageshow", arguments.callee, false);
>+
>+        executeSoon(function() {
>+          newWin.getBrowser().addTab();

>+      newWin3.addEventListener("load", function(aEvent) {
>+        newWin3.getBrowser().addEventListener("pageshow", function(aEvent) {
>+          // the window _should_ have state with a tab of url, but it doesn't
>+          // always happend before window.close(). addTab ensure we don't treat
>+          // this window as a stateless window
>+          newWin3.getBrowser().addTab();
>+          newWin3.getBrowser().removeEventListener("pageshow", arguments.callee, true);

If it can be done without making the test go wrong, please:
a) move removing the event listener to the beginning of the function (as above)
b) use bubbling instead of capturing event listeners (as above)
Comment 18 Misak Khachatryan 2009-08-23 23:30:52 PDT
Created attachment 396178 [details] [diff] [review]
[for checkin] tests with nits addressed

Fixed nits, carrying forward r+ from Neil. Test passes on my Linux box.
Comment 20 Robert Kaiser 2009-08-25 13:01:53 PDT
Misak, it looks like a good part of browser_bug510890.js is running OK, but then it runs into a timeout on our unit test boxes. Could you look into that?
Comment 21 Misak Khachatryan 2009-08-26 08:28:31 PDT
Created attachment 396727 [details] [diff] [review]
modified test

I restored original test code for one function and deleted another - it seems fully related to Private Browsing, which we don't have. The test passes on my linux buid.
Comment 22 Serge Gautherie (:sgautherie) 2009-08-26 09:18:54 PDT
(In reply to comment #20)

Ftr:
{
mochitest-browser-chrome:

browser_bug510890.js | Timed out
browser_Browser.js | Timed out
}
Comment 23 Robert Kaiser 2009-08-26 12:43:51 PDT
I'll check in attachment 396727 [details] [diff] [review] as a bustage fix, in terms of tests it is that.
Comment 24 Robert Kaiser 2009-08-26 12:55:56 PDT
Comment on attachment 396727 [details] [diff] [review]
modified test

Pushed this as http://hg.mozilla.org/comm-central/rev/058aa5379fc7 - I took it as a test bustage fix, which it actually is, and bustage fixes don't necessarily need reviews.
Comment 25 Robert Kaiser 2009-08-26 16:15:52 PDT
I quoted the wrong bug number in the checkin comment, but the bustage fix has made tests pass again, it seems. Marking FIXED again.
Comment 26 Robert Kaiser 2009-09-18 06:19:03 PDT
Adding fixed-seamonkey2.0 to make it go away from "unfixed wanted" query
Comment 27 Jens Hatlak (:InvisibleSmiley) 2009-10-19 14:43:55 PDT
*** Bug 39165 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.