add lastAccessed to tabs.Tab

UNCONFIRMED
Unassigned

Status

()

Toolkit
WebExtensions: General
P5
normal
UNCONFIRMED
a month ago
3 days ago

People

(Reporter: kernp25, Unassigned, Mentored)

Tracking

({good-first-bug})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [tabs] triaged)

(Reporter)

Description

a month 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

8 days ago
Priority: -- → P5
Whiteboard: [tabs] triaged

Updated

8 days ago
Keywords: good-first-bug
Mentor: bob.silverberg@gmail.com
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
(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
You need to log in before you can comment on or make changes to this bug.