Closed Bug 873556 Opened 8 years ago Closed 7 years ago

Add a timestamp to closedWindows and closedTabs.

Categories

(Firefox :: Session Restore, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 29

People

(Reporter: jsbruner, Assigned: seif)

References

(Blocks 1 open bug)

Details

(Whiteboard: [good first bug][mentor=ttaubert][lang=js][qa-])

Attachments

(1 file, 2 obsolete files)

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
Tim, can you give some guidance to Josiah on this?
Flags: needinfo?(ttaubert)
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.
Flags: needinfo?(ttaubert)
Whiteboard: [good first bug][mentor=ttaubert][lang=js]
Component: General → Session Restore
Product: Toolkit → Firefox
Version: unspecified → Trunk
Here is my first attmept. Not sure if it is right or wrong. But I would like to get mentored on this bug
Attachment #8351470 - Flags: review?(ttaubert)
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().
Attachment #8351470 - Flags: review?(ttaubert)
Take 2 :D
Attachment #8351470 - Attachment is obsolete: true
Is this still alive?
You'll want to set a review flag, otherwise it's easy to miss new patches.
Attachment #8351558 - Flags: review?(ttaubert)
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
Attachment #8351558 - Flags: review?(ttaubert) → review+
AFAICT i did all that... do you want me to rebase and try again?
(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.
Assignee: nobody → seif
Status: NEW → ASSIGNED
Like that?
Attachment #8351558 - Attachment is obsolete: true
Attachment #8356629 - Flags: checkin+
Keywords: checkin-needed
Attachment #8356629 - Flags: checkin+
Great, thank you!
https://hg.mozilla.org/integration/fx-team/rev/cd03f802a3b9
Keywords: checkin-needed
Whiteboard: [good first bug][mentor=ttaubert][lang=js] → [good first bug][mentor=ttaubert][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/cd03f802a3b9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][mentor=ttaubert][lang=js][fixed-in-fx-team] → [good first bug][mentor=ttaubert][lang=js]
Target Milestone: --- → Firefox 29
Whiteboard: [good first bug][mentor=ttaubert][lang=js] → [good first bug][mentor=ttaubert][lang=js][qa-]
You need to log in before you can comment on or make changes to this bug.