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

RESOLVED FIXED in seamonkey2.0b2

Status

SeaMonkey
Session Restore
RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: Misak Khachatryan, Assigned: Misak Khachatryan)

Tracking

({fixed-seamonkey2.0})

Trunk
seamonkey2.0b2
fixed-seamonkey2.0
Dependency tree / graph
Bug Flags:
wanted-seamonkey2.0 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 6 obsolete attachments)

(Assignee)

Description

8 years ago
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.
Attachment #394813 - Flags: review?(neil)
Flags: wanted-seamonkey2?

Updated

8 years ago
Flags: wanted-seamonkey2? → wanted-seamonkey2+

Updated

8 years ago
Attachment #394813 - Flags: review?(neil) → review+

Comment 1

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

Comment 2

8 years ago
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.
Attachment #395056 - Flags: ui-review?(neil)
Attachment #395056 - Flags: review?(neil)

Updated

8 years ago
Attachment #395056 - Flags: ui-review?(neil) → ui-review+

Comment 3

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

Comment 4

8 years ago
How hard would it be to return an array of just the closed window titles?
(Assignee)

Comment 5

8 years ago
Created attachment 395551 [details] [diff] [review]
Interface part, nits fixed
Attachment #395056 - Attachment is obsolete: true
Attachment #395551 - Flags: superreview?(neil)
Attachment #395551 - Flags: review?(neil)
Attachment #395056 - Flags: review?(neil)
(Assignee)

Updated

8 years ago
Attachment #394813 - Flags: superreview?(neil)

Comment 6

8 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!
(Assignee)

Comment 7

8 years ago
Created attachment 395555 [details] [diff] [review]
Interface part, more nits fixed

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)

Comment 8

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

Updated

8 years ago
Attachment #394813 - Flags: superreview?(neil) → superreview+

Comment 9

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

Updated

8 years ago
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 ;-)
(Assignee)

Comment 11

8 years ago
Created attachment 395565 [details] [diff] [review]
[for checkin] Backend part, nits fixed r=neil sr=neil

Carrying forward r+ and sr+ from Neil.
Attachment #394813 - Attachment is obsolete: true
(Assignee)

Comment 12

8 years ago
Created attachment 395566 [details] [diff] [review]
[for checkin] Interface part, all nits fixed. r=neil sr=neil

Carrying forward r+ and sr+ from Neil
Attachment #395555 - Attachment is obsolete: true
(Assignee)

Comment 13

8 years ago
Created attachment 395567 [details] [diff] [review]
test

This is a test part, it times out now, can't understand why. Help needed.
Attachment #395567 - Flags: review?(neil)

Updated

8 years ago
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+

Updated

8 years ago
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+

Updated

8 years ago
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).
(Assignee)

Comment 16

8 years ago
Created attachment 395785 [details] [diff] [review]
fixed test

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+
(Assignee)

Updated

8 years ago
Blocks: 511640
(Assignee)

Comment 18

8 years ago
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.
Attachment #395785 - Attachment is obsolete: true
Attachment #396178 - Flags: review+
(Assignee)

Updated

8 years ago
Whiteboard: [checkin see comment 11&12] → [checkin see comment 11&12&18]

Comment 19

8 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.
Keywords: checkin-needed
Whiteboard: [checkin see comment 11&12&18]
(Assignee)

Updated

8 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Comment 20

8 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?
(Assignee)

Comment 21

8 years ago
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.
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 → ---

Comment 23

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

Comment 24

8 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.
Attachment #396727 - Flags: review?(neil)

Comment 25

8 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.
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED

Comment 26

8 years ago
Adding fixed-seamonkey2.0 to make it go away from "unfixed wanted" query
Keywords: fixed-seamonkey2.0
Duplicate of this bug: 39165

Updated

8 years ago
Blocks: 467530
No longer blocks: 467530
(Assignee)

Updated

7 years ago
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.