873556 - Add a timestamp to closedWindows and closedTabs.

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Josiah Bruner [:jsbruner]]

If one does:

var undoItems = JSON.parse(this._ss.getClosedTabData(window));

we have an array of closedTabs, but no way to figure out when each individual tab was closed. Although not usually important, Bug 565771 will merge recently closed Windows and Tabs into one pane, and the order of items must be based on a timestamp.

Currently we have no way to organize this data effectively, we will simply have an array of tabs and an array of Windows + tabs, and they won't be able to be combined into one list. If we had a timestamp we could re-arrange this data based on how recently something was closed.

I was unable to locate exactly what getClosedTabData and getClosedWindowData return, other than the fact that it is an array of tab/window data. 

Essentially we need to be able to access data like this:

undoItems[i].dateClosed = blah blah blah

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Tim, can you give some guidance to Josiah on this?

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Sure. Closed window data is saved here:

http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/SessionStore.jsm#950

Closed tab data here:

http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/SessionStore.jsm#1296

All we'd need to do is attach a 'dateClosed' or 'closedAt' property to those objects we're unshifting. Feel free to ask if you need some more guidance.

Comment 3

[Seif Lotfy]

Attachment: 0001-Bug-873556-Add-a-timestamp-to-closedWindows-and-clos.patch

Here is my first attmept. Not sure if it is right or wrong. But I would like to get mentored on this bug

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8351470 [details] [diff] [review]
0001-Bug-873556-Add-a-timestamp-to-closedWindows-and-clos.patch

Review of attachment 8351470 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you seif(?) for taking a stab at this!

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +1033,4 @@
>        winData._shouldRestore = true;
>  #endif
>  
> +      // Store the window's close Data

The comment leaves room for improvement :)

@@ +1033,5 @@
>        winData._shouldRestore = true;
>  #endif
>  
> +      // Store the window's close Data
> +      windData.dateClosed = new Date();

I now think that .closedAt would be a better name. We also should store Date.now() instead of a Date object.

Also, the variable name is wrong, must be |winData|.

@@ +1349,5 @@
>          state: tabState,
>          title: tabTitle,
>          image: tabbrowser.getIcon(aTab),
> +        pos: aTab._tPos,
> +        dateClosed: new Date()

Same here, closedAt: Date.now().

Comment 5

[Seif Lotfy]

Attachment: 0001-Bug-873556-Add-a-timestamp-to-closedWindows-and-clos.patch

Take 2 :D

Comment 6

[Seif Lotfy]

Is this still alive?

Comment 7

[Josh Matthews [:jdm]]

You'll want to set a review flag, otherwise it's easy to miss new patches.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8351558 [details] [diff] [review]
0001-Bug-873556-Add-a-timestamp-to-closedWindows-and-clos.patch

Review of attachment 8351558 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you, Seif! Can you please prepare the patch for checkin-needed [1]?

[1] http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed

Comment 9

[Seif Lotfy]

AFAICT i did all that... do you want me to rebase and try again?

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Seif Lotfy from comment #9)
> AFAICT i did all that... do you want me to rebase and try again?

Almost :) The patch description needs to be "Bug 873556 - Add a timestamp to closedWindows and closedTabs r=ttaubert" to denote that this patch has been reviewed by a peer. Also you should then set the "checkin-needed" keyword at the top of this page after you uploaded the new patch.

Comment 11

[Seif Lotfy]

Attachment: 0001-Bug-873556-Add-a-timestamp-to-closedWindows-and-clos.patch

Like that?

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Great, thank you!

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/cd03f802a3b9

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cd03f802a3b9