Closed Bug 739866 Opened 12 years ago Closed 12 years ago

Store last accessed timestamp

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 14

People

(Reporter: zpao, Assigned: pallanikumaran)

References

Details

Attachments

(1 file, 3 obsolete files)

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?
Yeah, sure. Please guide me on how i would start off with this bug. Thanks.
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.
Attached patch Initial Patch (obsolete) — Splinter Review
Hey Paul, Would you check if what i have done is correct?
Attachment #612465 - Flags: review?(paul)
Attachment #612465 - Attachment is patch: true
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.
Attachment #612465 - Flags: review?(paul) → feedback+
Attached patch Second Patch with test (obsolete) — Splinter Review
Attachment #612465 - Attachment is obsolete: true
Attachment #613119 - Flags: review?(paul)
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.
Attachment #613119 - Flags: review?(paul) → feedback+
Attached patch Patch with nits removed (obsolete) — Splinter Review
Attachment #613119 - Attachment is obsolete: true
Attachment #614754 - Flags: review?(paul)
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.
Attachment #614754 - Flags: review?(paul) → feedback+
Attachment #614754 - Attachment is obsolete: true
Attachment #615105 - Flags: review?(paul)
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.
Attachment #615105 - Flags: review?(paul) → review+
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
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
Assignee: nobody → pallanikumaran
Target Milestone: --- → Firefox 14
https://hg.mozilla.org/mozilla-central/rev/e78c08c87860
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
OS: Mac OS X → All
Hardware: x86 → All
Hey Paul, I would like to continue on the bug 586067. How do I start? Sorry for the late reply.
Depends on: 747338
Depends on: 874847
You need to log in before you can comment on or make changes to this bug.