1882655 - Cache a value for application start time in tabbrowser-tab.js' lastSeenActive

Status: RESOLVED FIXED

(Firefox :: Firefox View, task, P3)

Description

[Sam Foster [:sfoster] (he/him)]

Filing under Firefox View as its currently the only consumer of this getter.

In get lastSeenActive, we compare any lastAccessed timestamp to the application start time. This means a call to Services.startup.getStartupInfo().start.getTime() for each and every open tab that hasn't been selected in this session. We could stand to optimize this.

Comment 1

[JL]

first time contributing or doing any work on firefox, I'm still setting up my dev environment and reading the code, I'm hoping to eventually take this bug.

I'm still looking over the process of contributing since it's a brand new experience for me with very different tools compared to what I'm used to, so it might take me some time to get the hang out of it.

just wanted to confirm that the code in question is located on browser/components/tabbrowser/content/tab.js right? (since the bug description mentions a different filename)

as for the idea of caching the start time, would a good approach be to call Services.startup.getStartupInfo().start.getTime() in the constructor and keep that as a property?

thanks in advanced for any help (and patience since I'm pretty new here)

Comment 2

[Sam Foster [:sfoster] (he/him)]

Thanks for taking an interest in this bug.

(In reply to JL from comment #1)

I'm still looking over the process of contributing since it's a brand new experience for me with very different tools compared to what I'm used to, so it might take me some time to get the hang out of it.

Yeah, that is expected and is kind the point of these good-first-bugs. Most of your work will be getting set up to make changes to the firefox codebase, and take a patch through the code review process. Help is available in the #introduction channel on matrix if you get stuck or need pointers. Note that for this front-end code change, you can use an artifact build, and don't need the ability to compile all the c++ and rust code.

just wanted to confirm that the code in question is located on browser/components/tabbrowser/content/tab.js right? (since the bug description mentions a different filename)

No, tabbrowser-tab.js is the correct file. This is for the MozTabbrowserTab class - its the container for all the individual tabs.

as for the idea of caching the start time, would a good approach be to call Services.startup.getStartupInfo().start.getTime() in the constructor and keep that as a property?

The time returned by Services.startup.getStartupInfo().start.getTime() is when the whole browser started. For our purposes, I think just caching the value in the initialize method, in a property like this._startupTime would work well.

We already have tests written that cover this functionality, so it shouldn't be necessary to write or change anything like that. It just turns out that the getStartupInfo() call is a little expensive and as this is a value that never changes during a given browsing session, its a bit silly to keep requesting it.

Comment 3

[JL]

Yeah, that is expected and is kind the point of these good-first-bugs. Most of your work will be getting set up to make changes to the firefox codebase, and take a patch through the code review process. Help is available in the #introduction channel on matrix if you get stuck or need pointers. Note that for this front-end code change, you can use an artifact build, and don't need the ability to compile all the c++ and rust code.

thanks I appreciate it! fortunately I already did a build (and it compiled with everything, and it wasn't that slow)

No, tabbrowser-tab.js is the correct file. This is for the MozTabbrowserTab class - its the container for all the individual tabs.

searching for this file: https://searchfox.org/mozilla-central/source/browser/base/content/tabbrowser-tab.js gives me a 404, I also can't find it on the source code (but the link you posted does work...which confused me, since I couldn't find it in the local repo I pulled) I did a search for get lastSeenActive() and the only place I found it was in browser/components/tabbrowser/content/tab.js.

https://searchfox.org/mozilla-central/source/browser/base/content <- which is the path the original link gave doesn't have tabbrowser-tab.js anymore (and looking for that file on the local repo I just pulled doesn't show the file either)

I was thinking that maybe it got renamed when something was refactored, but if browser/components/tabbrowser/content/tab.js is wrong, do you know in which path is tabbrowser-tab.js located? (it has lastSeenActive() in it)

I might be confusing things, which I guess it's expected for such a big project like a browser :(

The time returned by Services.startup.getStartupInfo().start.getTime() is when the whole browser started. For our purposes, I think just caching the value in the initialize method, in a property like this._startupTime would work well.

We already have tests written that cover this functionality, so it shouldn't be necessary to write or change anything like that. It just turns out that the getStartupInfo() call is a little expensive and as this is a value that never changes during a given browsing session, its a bit silly to keep requesting it.

makes sense, I'll go ahead and work on it as soon as I figure out if the file I'm looking at on my local repo is the correct one or not, thanks for the patience.

Comment 4

[Sam Foster [:sfoster] (he/him)]

I was thinking that maybe it got renamed when something was refactored, but if browser/components/tabbrowser/content/tab.js is wrong, do you know in which path is tabbrowser-tab.js located? (it has lastSeenActive() in it)

Ah, I see what happened - you are right, this stuff got moved recently: https://hg.mozilla.org/mozilla-central/log/tip/browser/components/tabbrowser/content/tab.js. Yeah you are in the right spot.

Comment 5

[JL]

Attachment: Bug 1882655 cache startup time value by calling it in the initialize function, update remove intermediate variable and fix typo, move getting startuptime to tabs.js r=sfoster

Comment 6

[JL]

https://phabricator.services.mozilla.com/D215032

that was an adventure q__q, I wasn't sure how to run the specific tests for this, so I just kept running them all, I tried ./mach try auto but I got an error probably didn't have permissions? so I only ran tests on my OS, only instructions for running remotely was try auto :(

sorry if I'm jumping the gun and I should have tested on all platforms before submitting anything for review If that's the case you can disregard my commit and I'll try to figure out how to do the multi-platform testing

Comment 7

[Sam Foster [:sfoster] (he/him)]

I've added note in phabricator, but I'll also note here, you probably only need to run ./mach test browser/components/tabbrowser/test/browser/tabs/browser_lastSeenActive.js to verify your changes don't break something. And I don't think we anticipate different outcomes on each platform so as long as it passes for you locally that is probably good enough in this case.

Comment 8

[Sam Foster [:sfoster] (he/him)]

(In reply to JL from comment #6)

I wasn't sure how to run the specific tests for this, so I just kept running them all, I tried ./mach try auto but I got an error probably didn't have permissions? so I only ran tests on my OS, only instructions for running remotely was try auto :(

Yeah you do need slightly elevated permissions to be able to push jobs to run under our automation. I can do that for you, but for a well-bounded change like this the chance of unexpected fallout is small enough that I think given tests will be run anyway before merging the patch, we don't need to do anything more here.

(In reply to Sam Foster [:sfoster] (he/him) from comment #7)

I've added note in phabricator, but I'll also note here, you probably only need to run ./mach test browser/components/tabbrowser/test/browser/tabs/browser_lastSeenActive.js to verify your changes don't break something.

I guess there is one more place you can check. In Firefox View, we use this property to drive the sorting on the Recent Browsing lists. The test coverage for that is mostly in browser/components/firefoxview/tests/browser/browser_opentabs_recency.js

Comment 9

[JL]

(In reply to Sam Foster [:sfoster] (he/him) from comment #8)

(In reply to JL from comment #6)

I wasn't sure how to run the specific tests for this, so I just kept running them all, I tried ./mach try auto but I got an error probably didn't have permissions? so I only ran tests on my OS, only instructions for running remotely was try auto :(

Yeah you do need slightly elevated permissions to be able to push jobs to run under our automation. I can do that for you, but for a well-bounded change like this the chance of unexpected fallout is small enough that I think given tests will be run anyway before merging the patch, we don't need to do anything more here.

(In reply to Sam Foster [:sfoster] (he/him) from comment #7)

I've added note in phabricator, but I'll also note here, you probably only need to run ./mach test browser/components/tabbrowser/test/browser/tabs/browser_lastSeenActive.js to verify your changes don't break something.

I guess there is one more place you can check. In Firefox View, we use this property to drive the sorting on the Recent Browsing lists. The test coverage for that is mostly in browser/components/firefoxview/tests/browser/browser_opentabs_recency.js

I saw your comments, I'll make the changes sometime tomorrow then run the test, then update the patch, I appreciate the help! as confusing as everything seemed at first this has been a pretty pleasant experience! ^_^

Comment 10

[JL]

I think I messed up something cause when I ran the moz-phab, it reverted the changes :(, i'll double check and submit it again, sorry for the issues :(

Comment 11

[JL]

finally fixed it, got a bit ahead of myself and ran $ moz-phab patch DNUMBER thinking this would update, but instead it just updated the branch to my original patch so when I then ran moz-phab it just submitted the same thing with no changes >_<

built it then
ran the test and got
Passed: 19
Failed: 0
Todo: 0
Mode: e10s

and finally submitted the correct update!

Comment 12

[JL]

Thanks for the great experience on my patch/bug, it was a very good experience!

is there anything else I need to do for the patch to land? or is it just waiting for the other reviewers to approve/reject/ask for changes?

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8d6f36e7c997
cache startup time value by calling it in the initialize function, update remove intermediate variable and fix typo, move getting startuptime to tabs.js r=tabbrowser-reviewers,sfoster,dao

Comment 14

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/8d6f36e7c997