Closed Bug 1431029 Opened 3 years ago Closed 3 years ago

Show a "last accessed" column in the site data manager

Categories

(Firefox :: Preferences, enhancement, P1)

58 Branch
enhancement

Tracking

()

RESOLVED FIXED
Firefox 60
Tracking Status
firefox60 --- fixed

People

(Reporter: johannh, Assigned: johannh)

References

(Blocks 1 open bug)

Details

(Whiteboard: [storage-v2])

Attachments

(2 files)

This was a suggestion from the Site Data Manager meeting in Austin. Since we have data for quota storage and cookies about when they were last written/accessed, we could show this in the site data manager to give the user a better understanding of what data can potentially be removed and what site data/cookies have recently been accessed.
Priority: -- → P2
Whiteboard: [storage-v2][triage] → [storage-v2]
Jan, you mentioned in Austin that getting the last access timestamp from quota managed storage should not be too hard. To be honest I'm not sure where to look for that data. Is this already saved somewhere or would we have to add it? If it's more or less just about adding a timestamp to some data structure I'm happy to get pointers on which code would need modification and could do it myself.

Andrew, I already asked you this on IRC but I wasn't really available afterwards due to time zone differences, so I'll just needinfo you here. Do you have any idea if this is feasible?

Thanks :)
Flags: needinfo?(jvarga)
Flags: needinfo?(bugmail)
Ahh... I totally mis-parsed your question on IRC.

The timestamp of last access is stored in those .metadata-v2 files you see in the root of each origin's storage in QM.

The GetUsageOp that looks up the usage of every origin already reads that information out into the timestamp here:
https://searchfox.org/mozilla-central/source/dom/quota/ActorsParent.cpp#6938

It's mainly a question of adding the data to OriginUsage (defined at https://searchfox.org/mozilla-central/source/dom/quota/PQuotaUsageRequest.ipdl#11) and UsageResult (https://searchfox.org/mozilla-central/source/dom/quota/QuotaResults.h#13) and nsIQuotaUsageResult (https://searchfox.org/mozilla-central/source/dom/quota/nsIQuotaResults.idl#10) and all the plumbing in between.
Flags: needinfo?(jvarga)
Flags: needinfo?(bugmail)
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
This isn't as well-tested as I'd like it to be, mostly because I feel like we're missing some piece in the chain that reliably unit tests SiteDataManager.jsm without going through UI. I filed bug 1439547 for that.
Yeah, what asuth said. I also quickly went over the C++ patch and it looks good.
Comment on attachment 8952357 [details]
Bug 1431029 - Show a "last accessed" column in the site data manager.

https://reviewboard.mozilla.org/r/221610/#review227506

::: browser/components/preferences/in-content/tests/browser_siteData.js:153
(Diff revision 1)
> +  let formatter = new Services.intl.DateTimeFormat(undefined, {
> +    dateStyle: "short", timeStyle: "short",
> +  });

The other advantage of using a fixed date will be that you can put this right next to where you use it...

::: browser/components/preferences/in-content/tests/browser_siteData.js:167
(Diff revision 1)
> +  // Set a creation date to compare with. This doesn't have to be too precise,
> +  // since we only go down to minute level in the UI.
> +  let creationDate = formatter.format(new Date());

But this will have a hard cut-off point, right? So let's say that the site cookie date and the new Date() here end up differing by 10ms on a slow mac debug build. Then the chance that those 20ms cross a minute distinction is 10 / 60,0000. So this will be orange once per 6000 runs. Which probably means once every week or 2 on our current infra set, if the test actually ran (several times) on each push.

Can't we just pass the cookie lastAccessed date here directly? (I'm assuming cookies.add() returns it...)

::: browser/locales/en-US/chrome/browser/preferences/siteDataSettings.dtd:10
(Diff revision 1)
>  <!ENTITY     window1.title                 "Settings - Cookies and Site Data">
>  <!ENTITY     hostCol.label                 "Site">
>  <!ENTITY     statusCol.label               "Status">
>  <!ENTITY     cookiesCol.label              "Cookies">
>  <!ENTITY     usageCol.label                "Storage">
> +<!ENTITY     lastAccessedCol.label                "Last Used">

Why not label this 'last accessed' ? "Used" is much more active terminology, but I imagine that cookie usage from 3rd party domains will mean that this labeling will confuse people. People don't use a toplevel google.com page every time a google cookie is accessed or changed.
Attachment #8952357 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8952356 [details]
Bug 1431029 - Expose timestamp for quota managed storage.

https://reviewboard.mozilla.org/r/221608/#review227748
Attachment #8952356 - Flags: review?(bugmail) → review+
Comment on attachment 8952357 [details]
Bug 1431029 - Show a "last accessed" column in the site data manager.

https://reviewboard.mozilla.org/r/221610/#review227506

> But this will have a hard cut-off point, right? So let's say that the site cookie date and the new Date() here end up differing by 10ms on a slow mac debug build. Then the chance that those 20ms cross a minute distinction is 10 / 60,0000. So this will be orange once per 6000 runs. Which probably means once every week or 2 on our current infra set, if the test actually ran (several times) on each push.
> 
> Can't we just pass the cookie lastAccessed date here directly? (I'm assuming cookies.add() returns it...)

It does not return it, but you have a point and getting the cookies manually doesn't cost us anything, so I did that. Thanks!

> Why not label this 'last accessed' ? "Used" is much more active terminology, but I imagine that cookie usage from 3rd party domains will mean that this labeling will confuse people. People don't use a toplevel google.com page every time a google cookie is accessed or changed.

I'll check with jsavory.
Hmm I didn't get ahold of Jacqueline but I'd like to move forward and not let this bitrot (I'm quite busy on this code right now), so I'll land this with the strings from the design spec and will revise soon if necessary.
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/963a227648af
Expose timestamp for quota managed storage. r=asuth
https://hg.mozilla.org/integration/autoland/rev/dced0221e5df
Show a "last accessed" column in the site data manager. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/963a227648af
https://hg.mozilla.org/mozilla-central/rev/dced0221e5df
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
You need to log in before you can comment on or make changes to this bug.