Closed
Bug 586067
Opened 14 years ago
Closed 11 years ago
Store last accessed timestamp for tabs
Categories
(Firefox :: Session Restore, defect)
Firefox
Session Restore
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)
1.19 KB,
patch
|
ttaubert
:
checkin+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•13 years ago
|
Whiteboard: [good first bug][mentor=zpao][lang=js]
Comment 1•12 years ago
|
||
Shane indicated on IRC that he was interested in working on this.
Assignee: nobody → shane.zilinskas
Comment 2•12 years ago
|
||
Back to nobody@mozilla.org so that whoever wants to actually work on this can take it.
Assignee: shane.zilinskas → nobody
Comment 3•12 years ago
|
||
Hello. I would like to work on this bug. How do i start?
Reporter | ||
Comment 4•12 years ago
|
||
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
Comment 5•12 years ago
|
||
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[]?
Reporter | ||
Comment 6•12 years ago
|
||
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•12 years ago
|
||
(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•12 years ago
|
||
Hey Paul, I would like to continue on this bug. How do I start? Sorry for the late reply.
Updated•12 years ago
|
Whiteboard: [good first bug][mentor=zpao][lang=js] → [good first bug][mentor=ttaubert][lang=js]
Comment 9•12 years ago
|
||
Pallani, would you still like to work on this? Tim is now going to mentor here since Paul left.
Comment 10•12 years ago
|
||
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
Comment 11•12 years ago
|
||
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•12 years ago
|
||
(In reply to koolkrik from comment #11) > Hi, > This bug is showing up as assigned, can i take it on? yep!
Assignee: nobody → koolkrik
Comment 13•12 years ago
|
||
(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•12 years ago
|
||
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•12 years ago
|
||
(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•12 years ago
|
||
(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•12 years ago
|
||
(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.
Updated•12 years ago
|
Flags: needinfo?(ttaubert)
Comment 18•12 years ago
|
||
(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)
Updated•11 years ago
|
Assignee: koolkrik → nobody
Comment 19•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
(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.
Assignee | ||
Comment 22•11 years ago
|
||
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?
Updated•11 years ago
|
Flags: needinfo?(ttaubert)
Comment 23•11 years ago
|
||
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)
Comment 24•11 years ago
|
||
(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.
Assignee | ||
Comment 25•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(ttaubert)
Comment 26•11 years ago
|
||
(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)
Assignee | ||
Comment 27•11 years ago
|
||
Added code to store last accessed time of a tab in CollectTabData.
Attachment #735402 -
Flags: review?(ttaubert)
Comment 28•11 years ago
|
||
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+
Assignee | ||
Comment 29•11 years ago
|
||
New patch for check in.
Attachment #735402 -
Attachment is obsolete: true
Attachment #736339 -
Flags: checkin?(ttaubert)
Assignee | ||
Comment 30•11 years ago
|
||
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•11 years ago
|
||
(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
Comment 32•11 years ago
|
||
(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.
Assignee | ||
Comment 33•11 years ago
|
||
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)
Assignee | ||
Comment 34•11 years ago
|
||
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•11 years ago
|
||
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+
Comment 36•11 years ago
|
||
(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!
Assignee | ||
Comment 37•11 years ago
|
||
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•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2b9e5948213f
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
You need to log in
before you can comment on or make changes to this bug.
Description
•