Note: There are a few cases of duplicates in user autocompletion which are being worked on.

add lastAccessed to tabs.Tab

RESOLVED FIXED in Firefox 56

Status

()

Toolkit
WebExtensions: General
P5
normal
RESOLVED FIXED
4 months ago
5 days ago

People

(Reporter: kernp25, Assigned: Timothy Johnson, Mentored)

Tracking

({dev-doc-needed, good-first-bug})

unspecified
mozilla56
dev-doc-needed, good-first-bug
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [tabs] triaged)

MozReview Requests

()

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

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

4 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

3 months ago
Priority: -- → P5
Whiteboard: [tabs] triaged

Updated

3 months ago
Keywords: good-first-bug

Updated

3 months ago
Mentor: bob.silverberg@gmail.com

Comment 1

3 months 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

3 months 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
(Assignee)

Comment 8

3 months 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

3 months 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)
(Assignee)

Updated

3 months ago
Attachment #8863920 - Attachment is obsolete: true

Comment 15

3 months 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-

Comment 16

28 days ago
Created attachment 8881101 [details] [diff] [review]
1348911-add_lastAccessed_to_tabs.Tab.diff

With apologies to Timothy Johnson, I've addressed the review notes on his patch in the hopes of getting this one across the finish line.

I'm also doing a try-run just in case: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4073130566bd1d58a83f2f5643a2053ab34968c3
Attachment #8864310 - Attachment is obsolete: true
Attachment #8881101 - Flags: review?(tomica)
(In reply to Thomas Wisniewski from comment #16)
> With apologies to Timothy Johnson, 

Again, ask the previous assignee if they are still working on a bug.


Timothy, please let us know if you plan to finish this.
Flags: needinfo?(timothy)
Attachment #8881101 - Flags: review?(tomica)
Comment on attachment 8881101 [details] [diff] [review]
1348911-add_lastAccessed_to_tabs.Tab.diff

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

First of all, please use reviewboard[1], it makes it easier to keep track of comments and changes between version of the patch.

Other than that, this looks mostly good, so feel free to add :mixedpuppy to the review once you are done.

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

::: browser/components/extensions/test/browser/browser_ext_sessions_getRecentlyClosed_tabs.js
@@ +36,5 @@
>      },
>      background,
>    });
>  
> +  let url = "about:mozilla";

Why do this?

@@ +43,5 @@
>    await BrowserTestUtils.browserLoaded(win.gBrowser.selectedBrowser);
>    let expectedTabs = [];
>    let tab = win.gBrowser.selectedTab;
>    expectedTabs.push(expectedTabInfo(tab, win));
> +  let lastAccessedTimes = {};

Let's use a `Map` here.

@@ +49,2 @@
>  
> +  for (url of ["about:robots", "about:buildconfig"]) {

Why remove the `let` here.  It's subtle and is easy to miss and mess up later.

::: browser/components/extensions/test/browser/browser_ext_tabs_lastAccessed.js
@@ +7,5 @@
> +  let extension = ExtensionTestUtils.loadExtension({
> +    manifest: {
> +      permissions: ["tabs"],
> +    },
> +    background: async function() {

nit: `async background() {`

@@ +12,5 @@
> +      let tabs = await browser.tabs.query({
> +        url: "https://example.com/",
> +      });
> +
> +      browser.test.assertTrue(tabs.length == 2, "Expected tabs were found");

assertEq()

@@ +15,5 @@
> +
> +      browser.test.assertTrue(tabs.length == 2, "Expected tabs were found");
> +
> +      let tab1 = tabs[0];
> +      let tab2 = tabs[1];

nit `let [tab1, tab2] = tabs;`

@@ +22,5 @@
> +
> +      browser.test.assertTrue(past < tab1.lastAccessed &&
> +                              tab1.lastAccessed < tab2.lastAccessed &&
> +                              tab2.lastAccessed <= now,
> +                              "tab.lastAccessed contains a valid timestamp.");

nit: let's make this message a bit clearer, like "lastAccessed timestamps are recent and in the right order".

::: mobile/android/components/extensions/test/mochitest/test_ext_tabs_lastAccessed.html
@@ +25,5 @@
> +      let tabs = await browser.tabs.query({
> +        url: "https://example.com/",
> +      });
> +
> +      browser.test.assertTrue(tabs.length == 2, "Expected tabs were found");

...same comments here as in the android versions...
(In reply to Tomislav Jovanovic :zombie from comment #17)
> Timothy, please let us know if you plan to finish this.

It's been 5 days, you can have the bug Thomas.
Assignee: nobody → timothy
Flags: needinfo?(timothy)
Comment hidden (mozreview-request)

Comment 21

15 days ago
Thanks zombie. I've put a fresh copy of the patch up on reviewboard now, with your comments addressed.

Note that I didn't realize it before, but the Fennec code wasn't actually exposing lastAccessed, so the Android test was actually failing. I added the relevant getter to mobile/android/chrome/content/browser.js, and also adjusted the tests to be a bit less racy with the past dates when the test happens to run slowly/hiccup.

A try run of this version seems clean so far: https://treeherder.mozilla.org/#/jobs?repo=try&revision=667be0daf23568f7326b80f703eaf5bbd5784897

Comment 22

14 days ago
mozreview-review
Comment on attachment 8884486 [details]
Bug 1348911 - Add lastAccessed to tabs.Tab;

https://reviewboard.mozilla.org/r/155388/#review160490

This is good, though there are a couple of issues that I missed the last time (sorry) that I'd like addressed.

::: browser/components/extensions/test/browser/browser_ext_sessions_getRecentlyClosed_tabs.js:65
(Diff revision 1)
>    extension.sendMessage("check-sessions");
>    let recentlyClosed = await extension.awaitMessage("recentlyClosed");
>    let tabInfo = recentlyClosed[0].tab;
>    let expectedTab = expectedTabs.pop();
>    checkTabInfo(expectedTab, tabInfo);
> +  is(tabInfo.lastAccessed > lastAccessedTimes.get(tabInfo.url), true,

`ok(...)`

here and below

::: browser/components/extensions/test/browser/browser_ext_tabs_lastAccessed.js:4
(Diff revision 1)
> +"use strict";
> +
> +add_task(async function testLastAccessed() {
> +  let past = Date.now() - 1000 * 5;

nit: if you already moved this to the start of the test, you don't need the 5 seconds in the past

::: browser/components/extensions/test/browser/browser_ext_tabs_lastAccessed.js:24
(Diff revision 1)
> +          url: "https://example.com/",
> +        });
> +
> +        browser.test.assertEq(tabs.length, 2, "Expected tabs were found");
> +
> +        let [tab1, tab2] = tabs;

so.. I don't think we are guaranteed to get the tabs in the right order here.  Very unlikely it will ever be an issue, but it should be easy to do properly, just by opening urls with "?1" and "?2".

::: mobile/android/chrome/content/browser.js:3817
(Diff revision 1)
>    getActive: function getActive() {
>      return this.browser.docShellIsActive;
>    },
>  
> +  get lastAccessed() {
> +    return this.lastTouchedAt;

Renaming this property on the android side doesn't seem that much better, so let's keep our changes to web extensions code, and just access `lastTouchedAt` directly in android part of our implementation.

::: toolkit/components/extensions/ExtensionTabs.jsm:275
(Diff revision 1)
> +   *        Returns the last time the tab was accessed as the number of
> +   *        milliseconds since epoch.
> +   *        @readonly
> +   */
> +  get lastAccessed() {
> +    return this.nativeTab.lastAccessed;

We need to make this abstract here (just throw), and actually implement it on both the desktop and android side.
Attachment #8884486 - Flags: review?(tomica)
Comment hidden (mozreview-request)

Comment 24

14 days ago
Alright, I've addressed those comments and amended the patch accordingly.

Comment 25

13 days ago
mozreview-review
Comment on attachment 8884486 [details]
Bug 1348911 - Add lastAccessed to tabs.Tab;

https://reviewboard.mozilla.org/r/155388/#review160520

r=me with the two fixes below.

Please add :mixedpuppy for final review after you are done.

::: browser/components/extensions/test/browser/browser_ext_sessions_getRecentlyClosed_tabs.js:65
(Diff revisions 1 - 2)
>    extension.sendMessage("check-sessions");
>    let recentlyClosed = await extension.awaitMessage("recentlyClosed");
>    let tabInfo = recentlyClosed[0].tab;
>    let expectedTab = expectedTabs.pop();
>    checkTabInfo(expectedTab, tabInfo);
> -  is(tabInfo.lastAccessed > lastAccessedTimes.get(tabInfo.url), true,
> +  ok(tabInfo.lastAccessed > lastAccessedTimes.get(tabInfo.url), true,

ok() only takes one two params, you don't need `true` anymore.

::: browser/components/extensions/test/browser/browser_ext_tabs_lastAccessed.js:24
(Diff revisions 1 - 2)
> -          url: "https://example.com/",
> +          url: "https://example.com/*",
>          });
>  
>          browser.test.assertEq(tabs.length, 2, "Expected tabs were found");
>  
>          let [tab1, tab2] = tabs;

heh, I meant for you to add "?1" and "?2" to be able to query for the tabs separately, so that you are sure of the order.  

Or at least assert that `tab2.url` ends with "?2", so that failure reason would be obvious from the test logs.

::: toolkit/components/extensions/ExtensionTabs.jsm:272
(Diff revisions 1 - 2)
>  
>    /**
>     * @property {integer} lastAccessed
>     *        Returns the last time the tab was accessed as the number of
>     *        milliseconds since epoch.
>     *        @readonly

nit: @abstract
Attachment #8884486 - Flags: review?(tomica) → review+
Comment hidden (mozreview-request)

Comment 27

13 days ago
Thanks :zombie! I've addressed your nits and amended the patch with r?mixedpuppy.

>heh, I meant for you to add "?1" and "?2" to be able to query for the tabs separately

Heh, oops. At the time my brain somehow decided that the tabs would be returned in URL-sorted order, so just adding the suffixes would be enough.

Comment 28

12 days ago
mozreview-review
Comment on attachment 8884486 [details]
Bug 1348911 - Add lastAccessed to tabs.Tab;

https://reviewboard.mozilla.org/r/155388/#review160686

::: commit-message-c45f1:1
(Diff revision 3)
> +Bug 1348911 - Add lastAccessed to tabs.Tab; r?mixedpuppy

You can add multiple people to review, like: `r?zombie,mixedpuppy`

::: browser/components/extensions/test/browser/browser_ext_sessions_getRecentlyClosed_tabs.js:77
(Diff revision 3)
>    recentlyClosed = await extension.awaitMessage("recentlyClosed");
>    let tabInfos = recentlyClosed[0].window.tabs;
>    is(tabInfos.length, 2, "Expected number of tabs in closed window.");
>    for (let x = 0; x < tabInfos.length; x++) {
>      checkTabInfo(expectedTabs[x], tabInfos[x]);
> +    is(tabInfos[x].lastAccessed > lastAccessedTimes.get(tabInfos[x].url), true,

You missed this `ok()`

::: mobile/android/components/extensions/test/mochitest/test_ext_tabs_lastAccessed.html:38
(Diff revision 3)
> +          url: "https://example.com/*",
> +        });
> +
> +        browser.test.assertEq(2, tabs.length, "Expected tabs were found");
> +
> +        let [tab1, tab2] = tabs;

you missed to change the android side
Attachment #8884486 - Flags: review+
Comment hidden (mozreview-request)

Comment 30

10 days ago
mozreview-review
Comment on attachment 8884486 [details]
Bug 1348911 - Add lastAccessed to tabs.Tab;

https://reviewboard.mozilla.org/r/155388/#review161734

The test file looks the same for both browser and android, feels like it should move to toolkit/components/extensions/test/mochitest instead and avoid duplication.
Attachment #8884486 - Flags: review?(mixedpuppy) → review+
Comment hidden (mozreview-request)

Comment 32

10 days ago
Comment on attachment 8881101 [details] [diff] [review]
1348911-add_lastAccessed_to_tabs.Tab.diff

Alright, I've amended the patch to use one test-case instead of two. Thanks for that tip, Shane!

I did a try-run of the patch against today's moz-central and it was green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=84c5ab1a780153467fc0ede110c60ec997f656df

Requesting check-in.
Attachment #8881101 - Attachment is obsolete: true

Updated

10 days ago
Keywords: checkin-needed
(In reply to Shane Caraveo (:mixedpuppy) from comment #30)
> The test file looks the same for both browser and android, feels like it
> should move to toolkit/components/extensions/test/mochitest instead and
> avoid duplication.

I agree we should avoid code duplication, but I'm not sure moving tests for browser APIs to toolkit is the best solution.
let's figure this out before checking in
Keywords: checkin-needed
> I agree we should avoid code duplication, but I'm not sure moving tests for
> browser APIs to toolkit is the best solution.

Hey Shane, we have a number of android tests that are very close to the browser versions, so the issue is not isolated to this patch, and should be solved in a unified manner across our tests.

Until we decide the solution is that we should test browser/ APIs using toolkit tests, I think this should land as separate tests.

Opinion?
Flags: needinfo?(mixedpuppy)
Well, the value is being set in toolkit as well.  However, I'd rather not block landing on this issue.  Lets just be sure there is a followup bug for fixing duplicate browser/android tests.
Flags: needinfo?(mixedpuppy)

Comment 37

8 days ago
:zombie, are you okay with that? If so, I'll reset checkin-needed.
Flags: needinfo?(tomica)
Just confirmed with Shane, he meant "let's land as two separate tests", so please revert to that version and ask for checkin (and sorry to go back and forth with you ;)


(In reply to Shane Caraveo (:mixedpuppy) from comment #36)
> Lets just be sure there is a followup bug for fixing duplicate browser/android tests.

filed bug 1381237
Flags: needinfo?(tomica)
Comment hidden (mozreview-request)

Comment 40

6 days ago
Alright, no problem. I've reverted it back to the two-test-case version.

Requesting check-in.
Keywords: checkin-needed

Comment 41

5 days ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a14a6b94f2ed
Add lastAccessed to tabs.Tab; r=mixedpuppy,zombie
Keywords: checkin-needed
Keywords: dev-doc-needed

Comment 42

5 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a14a6b94f2ed
Status: UNCONFIRMED → RESOLVED
Last Resolved: 5 days ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.