Store last accessed timestamp

RESOLVED FIXED in Firefox 14

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: zpao, Assigned: Pallani Kumaran)

Tracking

Trunk
Firefox 14
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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?
(Assignee)

Comment 1

5 years ago
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.
(Assignee)

Comment 3

5 years ago
Created attachment 612465 [details] [diff] [review]
Initial Patch

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+
(Assignee)

Comment 5

5 years ago
Created attachment 613119 [details] [diff] [review]
Second Patch with test
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+
(Assignee)

Comment 7

5 years ago
Created attachment 614754 [details] [diff] [review]
Patch with nits removed
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+
(Assignee)

Comment 9

5 years ago
Created attachment 615105 [details] [diff] [review]
Patch with more nits removed
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+

Comment 11

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 14

5 years ago
Hey Paul, I would like to continue on the bug 586067. How do I start? Sorry for the late reply.

Updated

5 years ago
Depends on: 747338

Updated

4 years ago
Depends on: 874847
You need to log in before you can comment on or make changes to this bug.