Last Comment Bug 739866 - Store last accessed timestamp
: Store last accessed timestamp
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 14
Assigned To: Pallani Kumaran
:
Mentors:
Depends on: 747338 874847
Blocks: 586067
  Show dependency treegraph
 
Reported: 2012-03-27 18:25 PDT by Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
Modified: 2014-02-05 17:11 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Initial Patch (1.83 KB, patch)
2012-04-05 00:15 PDT, Pallani Kumaran
paul: feedback+
Details | Diff | Splinter Review
Second Patch with test (2.55 KB, patch)
2012-04-07 10:18 PDT, Pallani Kumaran
paul: feedback+
Details | Diff | Splinter Review
Patch with nits removed (2.95 KB, patch)
2012-04-13 05:56 PDT, Pallani Kumaran
paul: feedback+
Details | Diff | Splinter Review
Patch with more nits removed (3.66 KB, patch)
2012-04-14 20:11 PDT, Pallani Kumaran
paul: review+
Details | Diff | Splinter Review

Description Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-03-27 18:25:25 PDT
Per bug  comment 6 & 7, we'll start storing the timestamp for the last access time for each tab.

A couple things to consider:
* the value is mutable (so session restore can set it), so it could be "wrong"
* we need a sensible default. I guess null/undefined is ok, though 0 may be better
* we'll only update on TabSelect, so the timestamp doesn't reflect the last time it was visible.

Pallani, since this needs to be done before the session restore work, would you be interested in working on this as well?
Comment 1 Pallani Kumaran 2012-03-27 22:36:17 PDT
Yeah, sure. Please guide me on how i would start off with this bug. Thanks.
Comment 2 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-03-28 14:17:24 PDT
To store the value, I would add a <field> to the tabbrowser-tab binding: https://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#3801 This lets you set the default value very easily.

This is where we would want to update the value: https://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#911

We should add some tests for this too, but 1 step at a time.
Comment 3 Pallani Kumaran 2012-04-05 00:15:47 PDT
Created attachment 612465 [details] [diff] [review]
Initial Patch

Hey Paul, Would you check if what i have done is correct?
Comment 4 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-04-05 14:27:58 PDT
Comment on attachment 612465 [details] [diff] [review]
Initial Patch

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

Thanks Pallani, it's looking good. Just one minor change and then we need to add a test. I'm going to clear review until we have that, but feedback+

For tests, we can add a browser-chrome (https://developer.mozilla.org/en/Browser_chrome_tests) test in browser/base/content/tests that 
1. adds a new tab (but doesn't select it)
2. checks the timestamp on the new tab is 0
3. selects the new tab, checks that the timestamp is updated (>0, <= Date.now() [due to precision there])
4. select the original tab & make sure the new tab's timestamp hasn't changed

::: browser/base/content/tabbrowser.xml
@@ +922,5 @@
>                                            true, false);
>              }
> +            
> +            // Bug 739866 - Store last accessed timestamp
> +            this.selectedTab.lastAccessed = Date.now();

2 things:
1. We don't need to put the comment in
2. I think we only want to update this if we're not in previewMode. I think previewMode is used for aero peak and doesn't actually select the tab.
Comment 5 Pallani Kumaran 2012-04-07 10:18:10 PDT
Created attachment 613119 [details] [diff] [review]
Second Patch with test
Comment 6 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-04-09 11:52:20 PDT
Comment on attachment 613119 [details] [diff] [review]
Second Patch with test

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

A couple general nits on the test:
1. Please include a license header. Typically our tests are released to the public domain with
/* Any copyright is dedicated to the Public Domain.
   http://creativecommons.org/publicdomain/zero/1.0/ */

2. Please put a description and include the bug number in a comment in the test (I know it's pretty obvious, but it's makes it easier if it's in there)

Just about there!

::: browser/base/content/test/browser_lastAccessedTab.js
@@ +1,2 @@
> +function test() {
> +  let firstTab = gBrowser.addTab("about:blank", {skipAnimation: true});

Ok, first thing. There is already a tab that the test is run from, so you don't need to add 2 tabs. If you want to add 2 tabs you can, but you don't need to. You can remember the first tab with
> let originalTab = gBrowser.selectedTab;

@@ +1,5 @@
> +function test() {
> +  let firstTab = gBrowser.addTab("about:blank", {skipAnimation: true});
> +  gBrowser.selectedTab = firstTab;
> +  let newTab = gBrowser.addTab("about:blank", {skipAnimation: true});
> +  ok(newTab.lastAccessed,0,"Timestamp on the new tab is not 0.");

2 things:
1. whitespace - please add spaces following the commas (this applies to the rest of the code here too)
2. the string we put in here is a description of the success case

@@ +4,5 @@
> +  let newTab = gBrowser.addTab("about:blank", {skipAnimation: true});
> +  ok(newTab.lastAccessed,0,"Timestamp on the new tab is not 0.");
> +  gBrowser.selectedTab = newTab;
> +  let newTabAccessedDate = newTab.lastAccessed;
> +  ok((newTabAccessedDate!=0),"Timestamp on the selected tab is 0.");

Let's be explicit and make sure it's >0, not !=0

@@ +7,5 @@
> +  let newTabAccessedDate = newTab.lastAccessed;
> +  ok((newTabAccessedDate!=0),"Timestamp on the selected tab is 0.");
> +  ok((newTabAccessedDate<=Date.now()),"Timestamp not less than or equal to current Date.");
> +  gBrowser.selectedTab = firstTab;
> +  ok((newTab.lastAccessed==newTabAccessedDate),"New tab's timestamp has changed.");

Instead of ok(actual==expected) you can use is(actual, expected), which results in a better message in our logs if the test ever fails.

The other thing you want to make sure you do is cleanup & close the new tab(s) so that each test can start in a clean state.
Comment 7 Pallani Kumaran 2012-04-13 05:56:28 PDT
Created attachment 614754 [details] [diff] [review]
Patch with nits removed
Comment 8 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-04-13 14:10:32 PDT
Comment on attachment 614754 [details] [diff] [review]
Patch with nits removed

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

Thanks Pallani! 

Just one important change: we need to add the test file to the Makefile.in (in the same directory) at the end of _BROWSER_FILES so that the test gets run.

::: browser/base/content/test/browser_lastAccessedTab.js
@@ +16,5 @@
> +  is(newTab.lastAccessed, 0, "Timestamp on the new tab is 0.");
> +  gBrowser.selectedTab = newTab;
> +  let newTabAccessedDate = newTab.lastAccessed;
> +  ok((newTabAccessedDate>0), "Timestamp on the selected tab is more than 0.");
> +  ok((newTabAccessedDate<=Date.now()), "Timestamp less than or equal current Date.");

Nit: Since there's another change in the file, I'm going to be nitpicky and ask you to put space around comparison operators: a > b. You also don't need the parenthesis surrounding that expression, it'll be evaluated before the comma).

@@ +20,5 @@
> +  ok((newTabAccessedDate<=Date.now()), "Timestamp less than or equal current Date.");
> +  gBrowser.selectedTab = originalTab;
> +  is(newTab.lastAccessed, newTabAccessedDate, "New tab's timestamp remains the same.");
> +  gBrowser.selectedTab = newTab;
> +  gBrowser.removeCurrentTab();

You can just call `gBrowser.removeTab(newTab)` - you don't need to select it first. Not a big deal but it's a fraction faster and every little bit we can save in a long test run helps.
Comment 9 Pallani Kumaran 2012-04-14 20:11:45 PDT
Created attachment 615105 [details] [diff] [review]
Patch with more nits removed
Comment 10 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-04-16 13:24:03 PDT
Comment on attachment 615105 [details] [diff] [review]
Patch with more nits removed

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

Looks great Pallani! I'll run this through try server - I don't foresee any issues - and then we can get it checked in.
Comment 11 Mozilla RelEng Bot 2012-04-16 19:45:49 PDT
Try run for eb95ecf6222a is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=eb95ecf6222a
Results (out of 29 total builds):
    success: 26
    warnings: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/poshannessy@mozilla.com-eb95ecf6222a
Comment 12 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-04-16 23:24:54 PDT
Pushed to inbound. It'll get merged to mozilla-central in the next ~24 hours. Thanks Pallani! If you're still up for working on bug 586067, then we can head back over there and get started.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e78c08c87860
Comment 13 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-04-17 07:52:46 PDT
https://hg.mozilla.org/mozilla-central/rev/e78c08c87860
Comment 14 Pallani Kumaran 2012-04-30 08:18:15 PDT
Hey Paul, I would like to continue on the bug 586067. How do I start? Sorry for the late reply.

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