Closed Bug 586067 Opened 14 years ago Closed 11 years ago

Store last accessed timestamp for tabs

Categories

(Firefox :: Session Restore, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 23

People

(Reporter: zpao, Assigned: prasanth_b4u)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file, 2 obsolete files)

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.
Whiteboard: [good first bug][mentor=zpao][lang=js]
Shane indicated on IRC that he was interested in working on this.
Assignee: nobody → shane.zilinskas
Back to nobody@mozilla.org so that whoever wants to actually work on this can take it.
Assignee: shane.zilinskas → nobody
Hello. I would like to work on this bug. How do i start?
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
Assignee: nobody → pallanikumaran
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[]?
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.
(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
Hey Paul, I would like to continue on this bug. How do I start? Sorry for the late reply.
Whiteboard: [good first bug][mentor=zpao][lang=js] → [good first bug][mentor=ttaubert][lang=js]
Pallani, would you still like to work on this? Tim is now going to mentor here since Paul left.
Pallani, if you want to pick this up again, just leave a note. In the meantime I'll mark this as unassigned.
Assignee: pallanikumaran → nobody
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
(In reply to koolkrik from comment #11)
> Hi,
> This bug is showing up as assigned, can i take it on?

yep!
Assignee: nobody → koolkrik
(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
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
(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.
(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.
(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.
Flags: needinfo?(ttaubert)
(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.
Flags: needinfo?(ttaubert)
Assignee: koolkrik → nobody
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.
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.
(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.
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?
Flags: needinfo?(ttaubert)
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!
Flags: needinfo?(ttaubert)
(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.
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)
Flags: needinfo?(ttaubert)
(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 :)
Flags: needinfo?(ttaubert)
Attached patch Patch (obsolete) — Splinter Review
Added code to store last accessed time of a tab in CollectTabData.
Attachment #735402 - Flags: review?(ttaubert)
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!
Attachment #735402 - Flags: review?(ttaubert) → review+
Attached patch Patch for checkin (obsolete) — Splinter Review
New patch for check in.
Attachment #735402 - Attachment is obsolete: true
Attachment #736339 - Flags: checkin?(ttaubert)
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!
(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.
Assignee: nobody → prasanth_b4u
Status: NEW → ASSIGNED
(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.
Replaced colon with hyphen as separator between bug # and description in comment and also appended the reviewer name.
Attachment #736339 - Attachment is obsolete: true
Attachment #736339 - Flags: checkin?(ttaubert)
Attachment #736446 - Flags: checkin?(ttaubert)
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 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
Attachment #736446 - Flags: checkin?(ttaubert) → checkin+
(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!
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 :)
https://hg.mozilla.org/mozilla-central/rev/2b9e5948213f
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
Blocks: 445461
You need to log in before you can comment on or make changes to this bug.