491577 - new API: allow to delete a single closed window

Status: RESOLVED FIXED

(Firefox :: Session Restore, enhancement)

Description

[Michael Kraft [:morac]]

Since bug 394759 dropped, it would be nice to add an API to delete a closed window similar to how bug 461634 added an API to delete a closed tab.

Comment 1

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Adding this to my radar. It should be easy enough & mirror the work done in bug 461634.

Comment 2

[Michael Kraft [:morac]]

I originally assigned this to myself since I wrote the patch for bug 461634.  I kind of forgot about this bug.  

Are you just watching this or did you want to write the patch for it?

Comment 3

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #2)
> Are you just watching this or did you want to write the patch for it?

Just watching. Feel free to write this.

Comment 4

[Michael Kraft [:morac]]

Attachment: Patch to add forgetClosedWindow function plus test files

Finally got around to this.  Modeled the patch after the one I wrote to forget closed tabs.  The closed window list is slightly different since it also keeps track of popups, but I don't think that needs to be taken into account in this case since the caller is specifying the specific window to remove from the closed window list.

Comment 5

[Simon Bünzli]

Comment on attachment 393441 [details] [diff] [review]
Patch to add forgetClosedWindow function plus test files

>+    // default to the most-recently closed tab

s/tab/window/

>+ * The Initial Developer of the Original Code is
>+ * Simon Bünzli <[e-mail]>.

You?

>+ * Portions created by the Initial Developer are Copyright (C) 2008

2009?

>+                { url: "http://example.com", title: REMEMBER }

Have you intentionally set this title to REMEMBER?

>+    let closedWindows = eval("(" + ss.getClosedWindowData() + ")");

JSON.parse, please.

r+=me with these fixed.

Comment 6

[Michael Kraft [:morac]]

Attachment: take 2

(In reply to comment #5)
> s/tab/window/
> 
I thought I caught all of those but missed one, sigh.


> >+ * The Initial Developer of the Original Code is
> >+ * Simon Bünzli <[e-mail]>.
> 
> You?
>

The test in this patch was originated from the test in bug 461634.  That test was originated from the test you wrote for bug 464199.  So technically you are the initial developer since I started with code you wrote. I can change it if you desire.

> 
> >+ * Portions created by the Initial Developer are Copyright (C) 2008
> 
> 2009?
> 

See above.

> >+                { url: "http://example.com", title: REMEMBER }
> 
> Have you intentionally set this title to REMEMBER?
>

No I didn't.  It doesn't really make a difference, but I'll change it to "title" like the rest.

> 
> >+    let closedWindows = eval("(" + ss.getClosedWindowData() + ")");
> 
> JSON.parse, please.
> 

Fixed.

Comment 7

[Dietrich Ayala (:dietrich)]

Comment on attachment 393454 [details] [diff] [review]
take 2

r=me, thanks

Comment 8

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/1f828c511a8d

Comment 9

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/b45fff56cb71

You should at least run your own test before claiming it's ready for check-in.

Comment 10

[Michael Kraft [:morac]]

(In reply to comment #9)
> http://hg.mozilla.org/mozilla-central/rev/b45fff56cb71
> 
> You should at least run your own test before claiming it's ready for check-in.

Sorry about that.  I actually had run the tests, which all passed, but forgot to run them again after changing "eval" to "JSON.parse", since I did so on a different machine without the source installed.

Comment 11

[Dietrich Ayala (:dietrich)]

Comment on attachment 393454 [details] [diff] [review]
take 2

i'm not a superreviewer, you'll need to get SR from mconnor or another SR for the api change.

Comment 12

[Michael Kraft [:morac]]

(In reply to comment #11)
> (From update of attachment 393454 [details] [diff] [review])
> i'm not a superreviewer, you'll need to get SR from mconnor or another SR for
> the api change.

?  
You SRed the patch to bug 461634 so I assumed you cold SR this one.

I added mconnor as a SR, but it's already been checked-in at this point so everything is kind of backwards.

Comment 13

[Mike Connor [:mconnor]]

Comment on attachment 393454 [details] [diff] [review]
take 2

>diff -r 05d7e81676bd browser/components/sessionstore/nsISessionStore.idl

>@@ -164,6 +164,11 @@
>   nsIDOMWindow undoCloseWindow(in unsigned long aIndex);
> 
>   /**
>+   * @param aIndex  is the index of the closed window to be removed (FIFO ordered).
>+   */
>+  nsIDOMNode forgetClosedWindow(in unsigned long aIndex);
>+
>+  /**

need to add:

@throws NS_ERROR_INVALID_ARG
  when aIndex does not map to a closed window

sr=me

Comment 14

[Michael Kraft [:morac]]

Attachment: take 3 - added @throws as directed

I added the "@throws" comment, but I'll mention that all the API calls in sessionstore can throw at least one exception and none of the other entries in sessionstore.idl list @throws in the definition.

It should be added to all the other function definitions as well.

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 394162 [details] [diff] [review]
take 3 - added @throws as directed

This won't apply, since attachment 393454 [details] [diff] [review] is already checked in.

Comment 16

[Michael Kraft [:morac]]

Attachment: Diff patch from version 2

I had a feeling that might be the case.  Here's a diff patch from the latest trunk.

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 394211 [details] [diff] [review]
Diff patch from version 2

http://hg.mozilla.org/mozilla-central/rev/3511005c7340