Misak Khachatryan Assignee
	Description • 15 years ago

Summary says all
I did back end part, some questions about interface part, will add it later.

	neil@parkwaycc.co.uk
	Comment 1 • 15 years ago

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).

	Misak Khachatryan Assignee
	Comment 2 • 15 years ago

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.

	neil@parkwaycc.co.uk
	Comment 3 • 15 years ago

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.

	neil@parkwaycc.co.uk
	Comment 4 • 15 years ago

How hard would it be to return an array of just the closed window titles?

	neil@parkwaycc.co.uk
	Comment 6 • 15 years ago

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!

	Misak Khachatryan Assignee
	Comment 7 • 15 years ago

Sorry :(

	neil@parkwaycc.co.uk
	Comment 8 • 15 years ago

(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 ;-)

	neil@parkwaycc.co.uk
	Comment 9 • 15 years ago

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.

	neil@parkwaycc.co.uk
	Comment 10 • 15 years ago

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 ;-)

	Misak Khachatryan Assignee
	Comment 11 • 15 years ago

Carrying forward r+ and sr+ from Neil.

	Misak Khachatryan Assignee
	Comment 12 • 15 years ago

Carrying forward r+ and sr+ from Neil

	Misak Khachatryan Assignee
	Comment 13 • 15 years ago

This is a test part, it times out now, can't understand why. Help needed.

	neil@parkwaycc.co.uk
	Comment 14 • 15 years ago

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.

	neil@parkwaycc.co.uk
	Comment 15 • 15 years ago

(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).

	Misak Khachatryan Assignee
	Comment 16 • 15 years ago

this one passes on my Linux box, not sure i understood and did everything correct though.

	neil@parkwaycc.co.uk
	Comment 17 • 15 years ago

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)

	Misak Khachatryan Assignee
	Comment 18 • 15 years ago

Fixed nits, carrying forward r+ from Neil. Test passes on my Linux box.

	Robert Kaiser
	Comment 19 • 15 years ago

Pushed to comm-central:

attachment 395565 [details] [diff] [review]: http://hg.mozilla.org/comm-central/rev/c43e364dec91
attachment 395566 [details] [diff] [review]: http://hg.mozilla.org/comm-central/rev/0fe6bf0ddb61
attachment 396178 [details] [diff] [review]: http://hg.mozilla.org/comm-central/rev/cea01fa911fb

Misak, feel free to resolve this bug as fixed if nothing more is to be done in here.

	Robert Kaiser
	Comment 20 • 15 years ago

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?

	Misak Khachatryan Assignee
	Comment 21 • 15 years ago

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.

	Serge Gautherie (:sgautherie)
	Comment 22 • 15 years ago

(In reply to comment #20)

Ftr:
{
mochitest-browser-chrome:

browser_bug510890.js | Timed out
browser_Browser.js | Timed out
}

	Robert Kaiser
	Comment 23 • 15 years ago

I'll check in attachment 396727 [details] [diff] [review] as a bustage fix, in terms of tests it is that.

	Robert Kaiser
	Comment 24 • 15 years ago

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.

	Robert Kaiser
	Comment 25 • 15 years ago

I quoted the wrong bug number in the checkin comment, but the bustage fix has made tests pass again, it seems. Marking FIXED again.

	Robert Kaiser
	Comment 26 • 15 years ago

Adding fixed-seamonkey2.0 to make it go away from "unfixed wanted" query