Closed Bug 510890 Opened 10 years ago Closed 10 years ago

Port Bug 394759 (Add undo close window feature) to SeaMonkey

Categories

(SeaMonkey :: Session Restore, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.0b2

People

(Reporter: misak.bugzilla, Assigned: misak.bugzilla)

References

Details

(Keywords: fixed-seamonkey2.0)

Attachments

(4 files, 6 obsolete files)

Attached patch Sessionstore backend part (obsolete) — Splinter Review
Summary says all
I did back end part, some questions about interface part, will add it later.
Attachment #394813 - Flags: review?(neil)
Flags: wanted-seamonkey2?
Flags: wanted-seamonkey2? → wanted-seamonkey2+
Attachment #394813 - Flags: review?(neil) → review+
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).
Attached patch Interface part (obsolete) — Splinter Review
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.
Attachment #395056 - Flags: ui-review?(neil)
Attachment #395056 - Flags: review?(neil)
Attachment #395056 - Flags: ui-review?(neil) → ui-review+
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.
How hard would it be to return an array of just the closed window titles?
Attached patch Interface part, nits fixed (obsolete) — Splinter Review
Attachment #395056 - Attachment is obsolete: true
Attachment #395551 - Flags: superreview?(neil)
Attachment #395551 - Flags: review?(neil)
Attachment #395056 - Flags: review?(neil)
Attachment #394813 - Flags: superreview?(neil)
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!
Attached patch Interface part, more nits fixed (obsolete) — Splinter Review
Sorry :(
Attachment #395551 - Attachment is obsolete: true
Attachment #395555 - Flags: superreview?(neil)
Attachment #395555 - Flags: review?(neil)
Attachment #395551 - Flags: superreview?(neil)
Attachment #395551 - Flags: review?(neil)
(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 ;-)
Attachment #394813 - Flags: superreview?(neil) → superreview+
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.
Attachment #395555 - Flags: superreview?(neil)
Attachment #395555 - Flags: superreview+
Attachment #395555 - Flags: review?(neil)
Attachment #395555 - Flags: review+
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 ;-)
Carrying forward r+ and sr+ from Neil.
Attachment #394813 - Attachment is obsolete: true
Carrying forward r+ and sr+ from Neil
Attachment #395555 - Attachment is obsolete: true
Attached patch test (obsolete) — Splinter Review
This is a test part, it times out now, can't understand why. Help needed.
Attachment #395567 - Flags: review?(neil)
Attachment #395565 - Attachment description: Backend part, nits fixed, for checkin → [for checkin] Backend part, nits fixed r=neil sr=neil
Attachment #395565 - Flags: superreview+
Attachment #395565 - Flags: review+
Attachment #395566 - Attachment description: Interface part, all nits fixed, for checkin → [for checkin] Interface part, all nits fixed. r=neil sr=neil
Attachment #395566 - Flags: superreview+
Attachment #395566 - Flags: review+
Keywords: checkin-needed
Whiteboard: [checkin see comment 11&12]
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.
Attachment #395567 - Flags: review?(neil) → review-
(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).
Attached patch fixed test (obsolete) — Splinter Review
this one passes on my Linux box, not sure i understood and did everything correct though.
Attachment #395567 - Attachment is obsolete: true
Attachment #395785 - Flags: review?(neil)
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)
Attachment #395785 - Flags: review?(neil) → review+
Blocks: 511640
Fixed nits, carrying forward r+ from Neil. Test passes on my Linux box.
Attachment #395785 - Attachment is obsolete: true
Attachment #396178 - Flags: review+
Whiteboard: [checkin see comment 11&12] → [checkin see comment 11&12&18]
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
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?
Attached patch modified testSplinter Review
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.
Attachment #396727 - Flags: review?(neil)
(In reply to comment #20)

Ftr:
{
mochitest-browser-chrome:

browser_bug510890.js | Timed out
browser_Browser.js | Timed out
}
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I'll check in attachment 396727 [details] [diff] [review] as a bustage fix, in terms of tests it is that.
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.
Attachment #396727 - Flags: review?(neil)
I quoted the wrong bug number in the checkin comment, but the bustage fix has made tests pass again, it seems. Marking FIXED again.
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Adding fixed-seamonkey2.0 to make it go away from "unfixed wanted" query
Duplicate of this bug: 39165
Blocks: FF2SM
No longer blocks: FF2SM
Blocks: 547720
No longer blocks: 547720
Depends on: 547720
Depends on: 726521
You need to log in before you can comment on or make changes to this bug.