add lastAccessed to tabs.Tab

UNCONFIRMED
Unassigned

Status

()

Toolkit
WebExtensions: General
P5
normal
UNCONFIRMED
2 months ago
23 days ago

People

(Reporter: kernp25, Unassigned, Mentored)

Tracking

({good-first-bug})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [tabs] triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 months ago
User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20170316213829



Actual results:

tabs.Tab should have the lastAccessed property from tabbrowser.xml

Updated

a month ago
Priority: -- → P5
Whiteboard: [tabs] triaged

Updated

a month ago
Keywords: good-first-bug
Mentor: bob.silverberg@gmail.com

Comment 1

a month ago
Hey, could you give more information how to fix the bug?
Flags: needinfo?(bob.silverberg)
(In reply to Kristina Kurshakova from comment #1)
> Hey, could you give more information how to fix the bug?

Hi Kristina. There are a few places where code will need to be added for this, but it's just a small amount of code in each place.

1. You'll need to add a get method for `lastAccessed` to the `Tab` class in browser/components/extensions/ext-utils.js [1], which would be similar to what's already in there for `audible` [2].

2. You'll need to do the same in mobile/android/components/extensions/ext-utils.js [3].

3. You'll need to add an abstract method for `lastAccessed` to the `TabBase` class in toolkit/components/extensions/ExtensionTabs.jsm [4], which would be similar to what's already in there for `audible` [5].

4. You'll need to add `lastAccessed` to the convert method of `TabBase`in toolkit/components/extensions/ExtensionTabs.jsm [6].

5. After making those changes, a test should be added or updated to verify that this new property actually works.

Please let me know if you have any other questions, and thanks for your interest in working on this bug.

[1] http://searchfox.org/mozilla-central/source/browser/components/extensions/ext-utils.js#469
[2] http://searchfox.org/mozilla-central/source/browser/components/extensions/ext-utils.js#474
[3] http://searchfox.org/mozilla-central/source/mobile/android/components/extensions/ext-utils.js#368
[4] http://searchfox.org/mozilla-central/source/toolkit/components/extensions/ExtensionTabs.jsm#65
[5] http://searchfox.org/mozilla-central/source/toolkit/components/extensions/ExtensionTabs.jsm#267-275
[6] http://searchfox.org/mozilla-central/source/toolkit/components/extensions/ExtensionTabs.jsm#459
Flags: needinfo?(bob.silverberg)
Thinking about this some more, I guess we should also update the schema for tabs to add this new property [1][2], and we should also have the sessions.getRecentlyClosed() method that can return tabs also include this property. That one is slightly more complicated, so let's tackle this first stuff first.

[1] http://searchfox.org/mozilla-central/source/browser/components/extensions/schemas/tabs.json#58
[2] http://searchfox.org/mozilla-central/source/mobile/android/components/extensions/schemas/tabs.json#58
When the time comes to deal with the sessions API, we're going to have to do something like this in Tab.convertFromSessionStoreClosedData in ext-utils.js [1]:

lastAccessed: tabData.state ? tabData.state.lastAccessed : tabData.lastAccessed

which I suppose isn't really particularly complicated after all.

[1] http://searchfox.org/mozilla-central/source/browser/components/extensions/ext-utils.js#555

Comment 5

a month ago
(In reply to Bob Silverberg [:bsilverberg] from comment #2)
> (In reply to Kristina Kurshakova from comment #1)
> > Hey, could you give more information how to fix the bug?
> 
> Hi Kristina. There are a few places where code will need to be added for
> this, but it's just a small amount of code in each place.
> 
> 1. You'll need to add a get method for `lastAccessed` to the `Tab` class in
> browser/components/extensions/ext-utils.js [1], which would be similar to
> what's already in there for `audible` [2].
> 
> 2. You'll need to do the same in
> mobile/android/components/extensions/ext-utils.js [3].
> 
> 3. You'll need to add an abstract method for `lastAccessed` to the `TabBase`
> class in toolkit/components/extensions/ExtensionTabs.jsm [4], which would be
> similar to what's already in there for `audible` [5].
> 
> 4. You'll need to add `lastAccessed` to the convert method of `TabBase`in
> toolkit/components/extensions/ExtensionTabs.jsm [6].
> 
> 5. After making those changes, a test should be added or updated to verify
> that this new property actually works.
> 
> Please let me know if you have any other questions, and thanks for your
> interest in working on this bug.
> 
> [1]
> http://searchfox.org/mozilla-central/source/browser/components/extensions/
> ext-utils.js#469
> [2]
> http://searchfox.org/mozilla-central/source/browser/components/extensions/
> ext-utils.js#474
> [3]
> http://searchfox.org/mozilla-central/source/mobile/android/components/
> extensions/ext-utils.js#368
> [4]
> http://searchfox.org/mozilla-central/source/toolkit/components/extensions/
> ExtensionTabs.jsm#65
> [5]
> http://searchfox.org/mozilla-central/source/toolkit/components/extensions/
> ExtensionTabs.jsm#267-275
> [6]
> http://searchfox.org/mozilla-central/source/toolkit/components/extensions/
> ExtensionTabs.jsm#459

A few stupid questions here.

1. Assuming that I applied all suggested changes here, where should I see them in the browser?
2. What's the purpose of this property (is it when the tab was focussed last time?) and type (I'm assuming date?)
3. Should [1] be involved as well (replace to `return this.getAttribute("lastAccessed")` and implement set method)?

[1] http://searchfox.org/mozilla-central/source/browser/base/content/tabbrowser.xml#7241
Flags: needinfo?(bob.silverberg)
(In reply to Kristina Kurshakova from comment #5)
> 
> A few stupid questions here.

There are no stupid questions. :)

> 
> 1. Assuming that I applied all suggested changes here, where should I see
> them in the browser?

There isn't anywhere you'd see them in the browser, per se. These are changes to WebExtensions APIs, so you would see them if you created a simple WebExtension that retrieves tabs. Then you would see the lastAccessed property of the tabs that you retrieved.

> 2. What's the purpose of this property (is it when the tab was focussed last
> time?) and type (I'm assuming date?)

Yes, it's the last time the tab was accessed, which is also when it was last focussed (at least I think so). It is a number representing the number of milliseconds since the epoch, similar to what you see in the browser with `Date.now()`.

> 3. Should [1] be involved as well (replace to `return
> this.getAttribute("lastAccessed")` and implement set method)?

You don't need to change anything in tabbrowser.xml. You will be using that property, that is already available to you in nativeTab.lastAccessed, because it is available in tabbrowser.xml.

Does that answer your questions?
Flags: needinfo?(bob.silverberg)
Another way to "see" the changes is via a test. If you change an existing test we have for the tabs API to now also look for the new lastAccessed property, you will be able to tell if it exists or not. A good test for that might be browser_ext_tabs_query.js [1]. 

[1] http://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_tabs_query.js

Comment 8

25 days ago
Created attachment 8863920 [details] [diff] [review]
lastAccess.patch

I was sure if the unit test should be on its own or included with another test.
Attachment #8863920 - Flags: review?(tomica)
Hi Kristina, if you are still planning to work on this, you can just notify us before we are done reviewing Timothy's patch, and I'll just assign the bug to you.  Or, if you wish, we can find another good-first-bug, and either me or Bob will be glad to help with that as well. 


(In reply to Timothy Johnson from comment #8)
> Created attachment 8863920 [details] [diff] [review]

Timothy, I didn't notice last night that Kristina was already interested/working on this.  In the future, you should ask the previous contributor if they still plan to work on the bug before attaching a patch.
Flags: needinfo?(kurshakovakristina)

Comment 10

24 days ago
(In reply to Tomislav Jovanovic :zombie from comment #9)
> Hi Kristina, if you are still planning to work on this, you can just notify
> us before we are done reviewing Timothy's patch, and I'll just assign the
> bug to you.  Or, if you wish, we can find another good-first-bug, and either
> me or Bob will be glad to help with that as well. 

Sure, you can take away this bug.
Flags: needinfo?(kurshakovakristina)
Ok, here is a list of other good first bugs, let us know if one of the interests you.
Oops: https://mzl.la/2qFgAaa
Comment on attachment 8863920 [details] [diff] [review]
lastAccess.patch

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

Thanks Timothy, this looks like the right direction.  A few general comments first:

 - Could you please use MozRevew [1]?  It's much easier to run tests and follow through on comments.
 - Separate test is OK, or if you want to just add your checks to an existing one, that's fine too.
 - Could you address the part from Bob's comment #3?  Or it could be a separate bug if you don't.
 - You are missing the test on android.

1) https://reviewboard.mozilla.org/dashboard/

::: browser/components/extensions/schemas/tabs.json
@@ +64,4 @@
>            "highlighted": {"type": "boolean", "description": "Whether the tab is highlighted."},
>            "active": {"type": "boolean", "description": "Whether the tab is active in its window. (Does not necessarily mean the window is focused.)"},
>            "pinned": {"type": "boolean", "description": "Whether the tab is pinned."},
> +          "lastAccessed": {"type": "integer", "optional": true, "description": "The last time the tab was accessed as the number of milliseconds since epoch."},

Also on the android side.

::: browser/components/extensions/test/browser/browser-common.ini
@@ +120,4 @@
>  [browser_ext_tabs_move_window_pinned.js]
>  [browser_ext_tabs_onHighlighted.js]
>  [browser_ext_tabs_onUpdated.js]
> +[browser_ext_tabs_lastAccessed.js]

Sorted alphabetically, please.

::: browser/components/extensions/test/browser/browser_ext_tabs_lastAccessed.js
@@ +1,2 @@
> +/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
> +/* vim: set sts=2 sw=2 et tw=80: */

Don't copy/paste this.

@@ +1,5 @@
> +/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
> +/* vim: set sts=2 sw=2 et tw=80: */
> +"use strict";
> +
> +add_task(function* () {

nit: Please name the test function.  Also, use async function with `await` instead of `yield`.

@@ +2,5 @@
> +/* vim: set sts=2 sw=2 et tw=80: */
> +"use strict";
> +
> +add_task(function* () {
> +  let tab1 = yield BrowserTestUtils.openNewForegroundTab(gBrowser, "about:robots");

Please use "https://example.com/" instead.

@@ +4,5 @@
> +
> +add_task(function* () {
> +  let tab1 = yield BrowserTestUtils.openNewForegroundTab(gBrowser, "about:robots");
> +
> +  gBrowser.selectedTab = tab1;

Not needed.

@@ +8,5 @@
> +  gBrowser.selectedTab = tab1;
> +
> +  let extension = ExtensionTestUtils.loadExtension({
> +    manifest: {
> +      "permissions": ["tabs"],

The permission shouldn't be needed here.

@@ +14,5 @@
> +
> +    background: function() {
> +      browser.tabs.query({
> +        active: true,
> +      }, function(tabs) {

Please use promises instead of callbacks, or better yet, an async function with `await`.

@@ +15,5 @@
> +    background: function() {
> +      browser.tabs.query({
> +        active: true,
> +      }, function(tabs) {
> +        let now = new Date();

nit: It might be simpler to use Date.now() and just deal with integers.

@@ +16,5 @@
> +      browser.tabs.query({
> +        active: true,
> +      }, function(tabs) {
> +        let now = new Date();
> +        let hourago = new Date(now.getTime() - 1000 * 60 * 60);

camelCase please.  Also, an hour is way too long (test timeout is something like 45 seconds).

@@ +17,5 @@
> +        active: true,
> +      }, function(tabs) {
> +        let now = new Date();
> +        let hourago = new Date(now.getTime() - 1000 * 60 * 60);
> +        let lastAccessed = tabs[0].lastAccessed;

Seeing how the implementation of `lastAccessed` returns the current time in some cases [2], I think this test should use two tabs, and check that:

    5s ago < prevTab.lastAccessed < currentTab.lastAccessed <= now

2) http://searchfox.org/mozilla-central/source/browser/base/content/tabbrowser.xml#7215

@@ +18,5 @@
> +      }, function(tabs) {
> +        let now = new Date();
> +        let hourago = new Date(now.getTime() - 1000 * 60 * 60);
> +        let lastAccessed = tabs[0].lastAccessed;
> +        browser.test.assertTrue(lastAccessed < now && lastAccessed > hourago, 0, "lastAccessed sane");

nit: A bit more readable message please.

::: toolkit/components/extensions/ExtensionTabs.jsm
@@ +271,5 @@
> +   *        @readonly
> +   *        @abstract
> +   */
> +  get lastAccessed() {
> +    throw new Error("Not implemented");

You can just add the code for property getter here, as the implementation is the same between desktop and android.
Attachment #8863920 - Flags: review?(tomica) → review-
Comment hidden (mozreview-request)

Updated

24 days ago
Attachment #8863920 - Attachment is obsolete: true

Comment 15

23 days ago
mozreview-review
Comment on attachment 8864310 [details]
Bug 1348911 - Added lastAccessed property to tabs.Tab.

https://reviewboard.mozilla.org/r/135936/#review139354

::: browser/components/extensions/ext-utils.js:577
(Diff revision 1)
>        windowId: window && windowTracker.getId(window),
>        highlighted: false,
>        active: false,
>        pinned: false,
>        incognito: Boolean(tabData.state && tabData.state.isPrivate),
> +      lastAccessed: tabData.state ? tabData.state.lastAccessed : tabData.lastAccessed,

This also needs a test, likely in browser_ext_sessions_getRecentlyClosed_tabs.js.

::: browser/components/extensions/test/browser/browser_ext_tabs_lastAccessed.js:3
(Diff revision 1)
> +"use strict";
> +
> +add_task(async function testLastAcessed () {

nit: no space after function name

::: browser/components/extensions/test/browser/browser_ext_tabs_lastAccessed.js:7
(Diff revision 1)
> +
> +add_task(async function testLastAcessed () {
> +  let extension = ExtensionTestUtils.loadExtension({
> +    background: async function () {
> +      let tab1 = await browser.tabs.create({url: "https://example.com/"});
> +      let tab2 = await browser.tabs.create({url: "https://example.com/"});

I guess this would work, but this would not be testing two different ways the `lastAccessed` is determined, so I suggest you keep the `tabs.query` approach from the first version.

::: browser/components/extensions/test/browser/browser_ext_tabs_lastAccessed.js:12
(Diff revision 1)
> +      let tab2 = await browser.tabs.create({url: "https://example.com/"});
> +
> +      let now = Date.now();
> +      let past = now - 1000 * 5;
> +
> +      browser.test.assertTrue(past < tab1.lastAccessed < tab2.lastAccessed <= now, 0,

This is called "interval comparison", and it doesn't work in javascript.  Or, it does "work", but doesn't *do* what you might think.  Try it:
    1 < 5 < 3

::: browser/components/extensions/test/browser/browser_ext_tabs_lastAccessed.js:18
(Diff revision 1)
> +        "tab.lastAccessed contains a valid timestamp.");
> +
> +      await browser.tabs.remove([tab1.id, tab2.id]);
> +
> +      browser.test.notifyPass("tabs.lastAccessed");
> +

nit: no unnecessary empty lines

::: mobile/android/components/extensions/test/mochitest/mochitest.ini:22
(Diff revision 1)
>  [test_ext_tabs_captureVisibleTab.html]
>  [test_ext_tabs_create.html]
>  [test_ext_tabs_events.html]
>  [test_ext_tabs_executeScript.html]
>  [test_ext_tabs_executeScript_bad.html]
> +[test_ext_tabs_lastAccess.html]

- this will skip the test (the ini files are weird)
- same name as the browser version, please
- alphabetically sorted

::: mobile/android/components/extensions/test/mochitest/test_ext_tabs_lastAccess.html:4
(Diff revision 1)
> +<!DOCTYPE HTML>
> +<html>
> +<head>
> +  <title>Tabs onUpdated Test</title>

wrong title

::: mobile/android/components/extensions/test/mochitest/test_ext_tabs_lastAccess.html:19
(Diff revision 1)
> +"use strict";
> +
> +add_task(async function testLastAcessed () {
> +  let extension = ExtensionTestUtils.loadExtension({
> +    background: async function () {
> +      let tab1 = await browser.tabs.create({url: "https://example.com/"});

Same notes as for the browser version.
Attachment #8864310 - Flags: review?(tomica) → review-
You need to log in before you can comment on or make changes to this bug.