Last Comment Bug 586067 - Store last accessed timestamp for tabs
: Store last accessed timestamp for tabs
Status: RESOLVED FIXED
[good first bug][mentor=ttaubert][lan...
:
Product: Firefox
Classification: Client Software
Component: Session Restore (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: Firefox 23
Assigned To: Prasanth Balakrishnan
:
:
Mentors:
Depends on: 739866
Blocks: 701090 445461 735868
  Show dependency treegraph
 
Reported: 2010-08-10 13:33 PDT by Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
Modified: 2013-07-26 23:35 PDT (History)
17 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (1.13 KB, patch)
2013-04-09 14:03 PDT, Prasanth Balakrishnan
ttaubert: review+
Details | Diff | Splinter Review
Patch for checkin (1.21 KB, patch)
2013-04-11 09:33 PDT, Prasanth Balakrishnan
no flags Details | Diff | Splinter Review
Patch for checkin (v2) (1.19 KB, patch)
2013-04-11 12:47 PDT, Prasanth Balakrishnan
ttaubert: checkin+
Details | Diff | Splinter Review

Description Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2010-08-10 13:33:01 PDT
It would be nice to restore tabs in MRU, but in order to do that, we need to store a timestamp for each tab. I'm thinking just do it on TabSelect. It won't be the last time the tab was actually looked at but it will be good enough to represent MRU.

Sync code already does this & adds the timestamp to extdata.
Comment 1 Josh Matthews [:jdm] 2011-12-17 09:51:02 PST
Shane indicated on IRC that he was interested in working on this.
Comment 2 Dão Gottwald [:dao] 2012-02-27 10:38:16 PST
Back to nobody@mozilla.org so that whoever wants to actually work on this can take it.
Comment 3 Pallani Kumaran 2012-03-13 13:29:52 PDT
Hello. I would like to work on this bug. How do i start?
Comment 4 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-03-14 14:46:48 PDT
Hey Pallani, I'm glad to see somebody interested! I'll be happy to help you along. If you haven't already, getting a build set up is the most important first step. Just in case, https://developer.mozilla.org/En/Developer_Guide/ is a great resource.

Session Restore does its work in nsSessionStore.js [1]. When tabs are selected, there's a TabSelect even that's fired, which we listen to and do some work. Starting there and digging around is a good way to get going.

[1]: https://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/nsSessionStore.js
Comment 5 Pallani Kumaran 2012-03-20 23:46:22 PDT
Hey Paul, I read through both _collectTabData and TabSelect functions. How should I proceed? How do i get the current time to put into the tabData[]?
Comment 6 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-03-21 12:39:34 PDT
Dão, Gavin: how would you feel about adding a property to #tabbrowser-tab to track this at a more public level (instead of just sessionstore)? Or I guess just an attribute would do as well. Then we could update it in updateCurrentBrowser (if !_previewMode).

(In reply to Pallani Kumaran from comment #5)
> Hey Paul, I read through both _collectTabData and TabSelect functions. How
> should I proceed? How do i get the current time to put into the tabData[]?

I think I want to expose this at a higher level, then the default action would be to just grab that value in _collectTabData. Then on TabSelect we could update browser.__SS_data (since that cached value will get used in _collectTabData in cases where no navigation has happened). Or we update _collectTabData to make sure it updates the cached value when called.

Let's hold off on what Dão/Gavin have to say and go from there.
Comment 7 Dão Gottwald [:dao] 2012-03-26 07:30:59 PDT
(In reply to Paul O'Shannessy [:zpao] from comment #6)
> Dão, Gavin: how would you feel about adding a property to #tabbrowser-tab to
> track this at a more public level (instead of just sessionstore)? Or I guess
> just an attribute would do as well. Then we could update it in
> updateCurrentBrowser (if !_previewMode).

WFM
Comment 8 Pallani Kumaran 2012-04-30 08:16:55 PDT
Hey Paul, I would like to continue on this bug. How do I start? Sorry for the late reply.
Comment 9 Dão Gottwald [:dao] 2012-09-07 07:29:25 PDT
Pallani, would you still like to work on this? Tim is now going to mentor here since Paul left.
Comment 10 Dão Gottwald [:dao] 2012-09-19 01:47:10 PDT
Pallani, if you want to pick this up again, just leave a note. In the meantime I'll mark this as unassigned.
Comment 11 koolkrik 2012-09-20 00:14:43 PDT
Hi,
This bug is showing up as assigned, can i take it on? While i wait for confirmation, i will work on getting the basic setup done.

Thanks,
Kartik
Comment 12 Dão Gottwald [:dao] 2012-09-20 00:16:53 PDT
(In reply to koolkrik from comment #11)
> Hi,
> This bug is showing up as assigned, can i take it on?

yep!
Comment 13 koolkrik 2012-09-23 19:47:00 PDT
(In reply to Dão Gottwald [:dao] from comment #12)
> (In reply to koolkrik from comment #11)
> > Hi,
> > This bug is showing up as assigned, can i take it on?
> 
> yep!

Hi Dao,

Here is what i understood so far:

Problem statement: Need to restore tabs in MRU order.

(In reply to Paul O'Shannessy [:zpao] (no longer moco, slower to respond) from comment #0)
> It would be nice to restore tabs in MRU, but in order to do that, we need to
> store a timestamp for each tab. I'm thinking just do it on TabSelect. It
> won't be the last time the tab was actually looked at but it will be good
> enough to represent MRU.
> 
> Sync code already does this & adds the timestamp to extdata.

Doubts:
 - Why won't TabSelect be called when the tab is looked at each time? What is      called each time the tab is looked at?

 - When we do a session restore,is tabselect called for each tab that is restored?

Thoughts:
Is the following approach right?

Store the timestamp in _collectTabData.
When the tab is selected, onTabSelect is called, where we update tab.linkedBrowser.__SS_data
(This is from Paul's comment above, still trying to understand the code fully)

Thanks
Comment 14 Tim Taubert [:ttaubert] 2012-09-25 01:07:17 PDT
Hey Kartik,

good to see you'll be working on this! The idea is to update a certain <xul:tab> attribute in _updateCurrentBrowser() here:

https://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#831

We should update the last accesed timestamp always when (!this._previewMode) - that is when tabbrowser would dispatch the TabSelect event. That way other parts of the browser can make use of this data as well, not only session store. Session store on the other hand needs to collect this information when collecting data for tabs and put it into tabData:

https://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/SessionStore.jsm#1877
Comment 15 Dão Gottwald [:dao] 2012-09-25 01:18:11 PDT
(In reply to Tim Taubert [:ttaubert] from comment #14)
> good to see you'll be working on this! The idea is to update a certain
> <xul:tab> attribute in _updateCurrentBrowser() here:
> 
> https://mxr.mozilla.org/mozilla-central/source/browser/base/content/
> tabbrowser.xml#831
> 
> We should update the last accesed timestamp always when (!this._previewMode)
> - that is when tabbrowser would dispatch the TabSelect event. That way other
> parts of the browser can make use of this data as well, not only session
> store.

This was already done in bug 739866.

> Session store on the other hand needs to collect this information
> when collecting data for tabs and put it into tabData:
> 
> https://mxr.mozilla.org/mozilla-central/source/browser/components/
> sessionstore/src/SessionStore.jsm#1877

That's what this bug is about.
Comment 16 Tim Taubert [:ttaubert] 2012-09-25 01:58:46 PDT
(In reply to Dão Gottwald [:dao] from comment #15)
> This was already done in bug 739866.

Oops, sorry. Didn't want to confuse anyone, I totally forgot about this.

> > Session store on the other hand needs to collect this information
> > when collecting data for tabs and put it into tabData:
> > 
> > https://mxr.mozilla.org/mozilla-central/source/browser/components/
> > sessionstore/src/SessionStore.jsm#1877
> 
> That's what this bug is about.

Right, so then let's collect the value from the field introduced by bug 739866 and put it into tabData. Good thing is that this bug itself is now even a little more self-contained.
Comment 17 koolkrik 2012-10-02 14:27:13 PDT
(In reply to Tim Taubert [:ttaubert] from comment #16)
> (In reply to Dão Gottwald [:dao] from comment #15)
> > This was already done in bug 739866.
> 
> Oops, sorry. Didn't want to confuse anyone, I totally forgot about this.
> 
> > > Session store on the other hand needs to collect this information
> > > when collecting data for tabs and put it into tabData:
> > > 
> > > https://mxr.mozilla.org/mozilla-central/source/browser/components/
> > > sessionstore/src/SessionStore.jsm#1877
> > 
> > That's what this bug is about.
> 
> Right, so then let's collect the value from the field introduced by bug
> 739866 and put it into tabData. Good thing is that this bug itself is now
> even a little more self-contained.

Hi Tim,

So it would be something like tabData.lastAccessed = aTab.lastAccessed  ?

Is this right? Also, i guess some thought needs to go into all the different situations in that piece of code. I see that tabData gets updated differently beased on the situation.

Please advise, i am new to mozilla codebase.

Thanks.
Comment 18 Tim Taubert [:ttaubert] 2012-10-16 03:25:08 PDT
(In reply to koolkrik from comment #17)
> So it would be something like tabData.lastAccessed = aTab.lastAccessed  ?

Yes, that's to be done in _collectTabData().

> Also, i guess some thought needs to go into all the different
> situations in that piece of code. I see that tabData gets updated
> differently beased on the situation.

That's pretty easy. We can collect this data at the top of the function because all we need is the tab. There's no special situations here. Even if we don't really update the value it doesn't hurt to do so.
Comment 19 Nikhil Johny 2013-02-22 11:50:12 PST
hi, i would like to work on this bug, i have been checking out the code mentioned in the comments. 

i am new to mozilla codebase. what does _SS_extdate, or any _SS_ mean? and is there some purpose of writing a '_' before a identifier?

Thank you.
Comment 20 Nikhil Johny 2013-02-26 10:57:04 PST
hi Tim,

i have been trying to understand the code with the help of the MDN docs. 
i understood what the final result needs to be, but dont know how to get there...

in the description it is mentioned to store the timestamp for each tab in Tabselect, how do i do that?

like koolkrik mentioned in comment 17, if we add tabData.lastAccessed = aTab.lastAccessed in _collectTabData, how does it help in restoring the tabs in MRU order.
Comment 21 Dão Gottwald [:dao] 2013-02-26 13:24:14 PST
(In reply to Nikhil Johny from comment #20)
> like koolkrik mentioned in comment 17, if we add tabData.lastAccessed =
> aTab.lastAccessed in _collectTabData, how does it help in restoring the tabs
> in MRU order.

You shouldn't worry about restoring tabs in MRU order here. That's a separate step that bug 701090 was filed for.
Comment 22 Prasanth Balakrishnan 2013-04-08 11:57:51 PDT
Hi,

My name is Prasanth and i am very much interested in contributing to the Firefox browser. I have been a C# developer all my life and trying to make sense of the Mozilla code that i am seeing. I have setup my machine and taken the latest build. How do i start off fixing the bug.. I would need great help.. I have found the _collectTabData at line no 1862 of SessionStore.jsm file. Is this the function that needs to be modified? I need to add the line tabData.lastAccessed = aTab.lastAccessed right after the line var tabData = { entries : [] };
Will that be able to fix the bug? Where else would the dependencies lie?
Comment 23 Tim Taubert [:ttaubert] 2013-04-08 14:11:11 PDT
Hey Prasanth,

great to hear you want to help us developing Firefox!

Yes, we need to modify _collectTabData(). Right after line 1864 we should store the tab's .lastAccessed property in tabData. That is all we need to make sure last accessed timestamps are properly stored on disk.

This bug is quite small and self-contained and a prerequisite for bug 701090. After we stored all those timestamps we're going to use this information to speed up the session restore process.

After you changed code in SessionStore.jsm you can do a "./mach build browser" in your source directory to re-build Firefox. This change will unfortunately not be very visible but you can open your custom build with a couple of tabs, switch between them each X seconds and then quit the browser. If you then inspect your $firefoxprofile/sessionstore.js file and search for "lastAccessed" you should see your timestamps in there.

If there's any open questions don't hesitate to ask!
Comment 24 Tim Taubert [:ttaubert] 2013-04-08 14:15:38 PDT
(In reply to Tim Taubert [:ttaubert] from comment #23)
> Yes, we need to modify _collectTabData(). Right after line 1864 we should
> store the tab's .lastAccessed property in tabData. That is all we need to
> make sure last accessed timestamps are properly stored on disk.

Another possibility is of course to change line 1863 and initialize the 'tabData' variable with an object that has the properties 'entries' and 'lastAccessed' with their respective values right from the very beginning.
Comment 25 Prasanth Balakrishnan 2013-04-09 00:29:26 PDT
Thanks for the response!

I will re-build firefox and test the changes!

Could you expand a little more on the other possibility of initializing the tabData at the beginning?

I think a separate line would be easier to comment and denote the change made by me. While making a comment there i can put the bug # i hope?

Flags: needinfo?(ttaubert@mozilla.com)
Comment 26 Tim Taubert [:ttaubert] 2013-04-09 00:32:54 PDT
(In reply to Prasanth Balakrishnan from comment #25)
> Could you expand a little more on the other possibility of initializing the
> tabData at the beginning?

I meant that you could do something like:

var tabData = { entries: [], lastAccessed: aTab.lastAccessed };

That would initialize the tabData object with the right keys and values.

> I think a separate line would be easier to comment and denote the change
> made by me.

I'd be also fine with a separate line, your call.

> While making a comment there i can put the bug # i hope?

We don't usually do that for changes, especially not if they are one-liners :)
Comment 27 Prasanth Balakrishnan 2013-04-09 14:03:49 PDT
Created attachment 735402 [details] [diff] [review]
Patch

Added code to store last accessed time of a tab in CollectTabData.
Comment 28 Tim Taubert [:ttaubert] 2013-04-10 04:59:14 PDT
Comment on attachment 735402 [details] [diff] [review]
Patch

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

That patch looks great.

All that is left to do now is to prepare the patch for checkin so that we can land it. Here's a good write-up on how this is done:
http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed

When you're done please attach the new patch and mark it as "checkin-needed".

Thank you, Prasanth!
Comment 29 Prasanth Balakrishnan 2013-04-11 09:33:03 PDT
Created attachment 736339 [details] [diff] [review]
Patch for checkin

New patch for check in.
Comment 30 Prasanth Balakrishnan 2013-04-11 11:50:27 PDT
While testing the patch i opened the firefox build and opened three tabs and switched between them and then closed it. What i could see was the first two tabs having lastAccessed as 0 and the last tab having a long number set as lastAccessed. I am guessing it must be the timestamp. But couldn't understand why the first two were showing 0!
Comment 31 Tim Taubert [:ttaubert] 2013-04-11 12:22:23 PDT
(In reply to Prasanth Balakrishnan from comment #30)
> While testing the patch i opened the firefox build and opened three tabs and
> switched between them and then closed it. What i could see was the first two
> tabs having lastAccessed as 0 and the last tab having a long number set as
> lastAccessed. I am guessing it must be the timestamp. But couldn't
> understand why the first two were showing 0!

Don't worry, that's expected. The field lastAccessed is initialized with the value '0'. So if you open a tab but never select it will keep lastAccessed=0. We also will restore the lastAccessed value for restored tabs in the future.
Comment 32 Tim Taubert [:ttaubert] 2013-04-11 12:28:42 PDT
(In reply to Prasanth Balakrishnan from comment #29)
> New patch for check in.

Prasanth, can you please use a hyphen to separate the bug number and the description like "Bug 586067 - Store ...", that's how we usually do it. Also we need to mark the patch as reviewed by me by appending " r=ttaubert" to the end of the description.
Comment 33 Prasanth Balakrishnan 2013-04-11 12:47:06 PDT
Created attachment 736446 [details] [diff] [review]
Patch for checkin (v2)

Replaced colon with hyphen as separator between bug # and description in comment and also appended the reviewer name.
Comment 34 Prasanth Balakrishnan 2013-04-11 12:50:46 PDT
Thanks for pointing that out. In the article about Mercurial Queues, the examples show colon used as separator between Bug # and description. That's why i followed that. Do you think we need to edit that?
Comment 35 Tim Taubert [:ttaubert] 2013-04-11 12:51:24 PDT
Comment on attachment 736446 [details] [diff] [review]
Patch for checkin (v2)

Great work Prasanth, thank you! I pushed the bug to the fx-team branch and it should be merged to trunk (m-c) tomorrow.

https://hg.mozilla.org/integration/fx-team/rev/2b9e5948213f
Comment 36 Tim Taubert [:ttaubert] 2013-04-11 12:53:45 PDT
(In reply to Prasanth Balakrishnan from comment #34)
> Thanks for pointing that out. In the article about Mercurial Queues, the
> examples show colon used as separator between Bug # and description. That's
> why i followed that. Do you think we need to edit that?

I just took a look a mozilla-central (https://tbpl.mozilla.org/) and in fact we're using both, ":" and "-". I didn't know that, sorry. Thanks for fixing this anyway!
Comment 37 Prasanth Balakrishnan 2013-04-11 12:56:44 PDT
Thanks for helping me out! This was easy. You told me the code and line no and i just pasted it :D However, i have got the feel of how to take a build and work with mq and to prepare a patch. Gonna start looking for other bugs :)
Comment 38 Ryan VanderMeulen [:RyanVM] 2013-04-12 14:40:29 PDT
https://hg.mozilla.org/mozilla-central/rev/2b9e5948213f

Note You need to log in before you can comment on or make changes to this bug.