Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add UsageDetails dictionary #69

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jarryd999
Copy link

@jarryd999 jarryd999 commented Apr 22, 2019

This change adds a new dictionary to the result of StorageManager.estimate() that breaks down usage by storage system.

I've opened a related TAG review here: w3ctag/design-reviews#365


Preview | Diff

storage.bs Outdated Show resolved Hide resolved
storage.bs Outdated Show resolved Hide resolved
storage.bs Outdated Show resolved Hide resolved
storage.bs Outdated Show resolved Hide resolved
@jarryd999 jarryd999 force-pushed the usageDetails branch 3 times, most recently from 56dc8be to 07ebfe8 Compare May 6, 2019 16:01
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This overall architecture makes sense to me now (although @annevk should check, as the editor). What remains are editorial issues.

storage.bs Outdated Show resolved Hide resolved
storage.bs Outdated Show resolved Hide resolved
storage.bs Outdated Show resolved Hide resolved
storage.bs Outdated Show resolved Hide resolved
storage.bs Outdated Show resolved Hide resolved
storage.bs Outdated Show resolved Hide resolved
@jarryd999 jarryd999 force-pushed the usageDetails branch 2 times, most recently from ecceb0a to 657ce68 Compare June 3, 2019 22:39
@jarryd999
Copy link
Author

@domenic Comments have been addressed, PTAL. If you have no further comments, then thank you for the review!

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but still needs editor approval. (And web platform tests!)

@inexorabletash
Copy link
Member

@jarryd999 added "tentative" tests already in https://github.com/web-platform-tests/wpt/tree/master/storage (estimate-usage-details...)

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for writing this up and thanks for your patience!

At a high-level I'm wondering about these things:

  1. Are other implementers on board with exposing this?
  2. Are there bugs filed against those implementations?
  3. Is there a PR to make the tests stop being tentative that we can land together with this assuming all is in order? (There's a couple minor things I wonder about with the tests we can maybe tackle there, e.g., I don't see a need to do || 0 for the new fields as they should always be defined in a compliant implementation.)

I also left a couple remarks inline.

storage.bs Outdated
@@ -178,7 +178,21 @@ larger <a>site storage quota</a>. Factors such as navigation frequency, recency
bookmarking, and <a href="#persistence">permission</a> for {{"persistent-storage"}} can be used as
indications of "popularity".

The <dfn export>application cache site storage usage</dfn> for an <a for=/>origin</a>
<var>origin</var> is a rough estimate of the amount of bytes used in <a>Application Cache</a>
in <var>origin</var>'s <a>site storage unit</a>. [[!HTML]]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one can contain opaque responses right? Given that this is a new place where we expose that information, I think we should also detail how to address the issue. Same for caches below.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I intentionally pulled the security part out into a separate PR in which I address the opaque response + padding issues. If you believe they should be merged in the PR, I can consolidate the two. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other PR did not have the necessary wording either, right? But yeah, I think they should be included here and should probably be noted closer to where we define these features. Not in a separate section somewhere. The separate section seems like a fine place to highlight some of the trickier areas, but in general we should note security requirements right alongside general implementation requirements.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've taken a stab at adding a recommendation for padding. Please take a look when you get a chance, thanks!

storage.bs Outdated

The <dfn export>caches site storage usage</dfn> for an <a for=/>origin</a>
<var>origin</var> is a rough estimate of the amount of bytes used in <a attribute>caches</a> API
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be something like {{Caches}} or {{Cache}}, no? Referring to the getter seems a little weird.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding was that it should link here: https://w3c.github.io/ServiceWorker/#cache-objects

When I used {{Cache}}, it linked here https://w3c.github.io/ServiceWorker/#cache.

When I used {{Caches}} or {{cache-objects}}, it did not create a link (it was just grey text).

Please advise.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you want {{CacheStorage}}? @jakearchibald thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other definitions don't link to an interface... I'd expect a link to the top level concept rather than an interface, e.g. https://w3c.github.io/ServiceWorker/#cache-objects

I don't think there's an export from SW there. So yeah, @jakearchibald ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that we could do this by adding a <pre class=anchors>...</pre> section w/o an explicit export from SW, but let's figure out what we want here. FWIW over in https://wicg.github.io/web-locks/ I wanted to do something similar and ended up with "Caches [Service-Workers])" where "Caches" was not a link and latter bit was just a biblio link, assuming readers would figure it out.

@jarryd999
Copy link
Author

jarryd999 commented Jul 12, 2019

Thanks for the review!

  1. Are other implementers on board with exposing this?

There has been no public support, but also no opposition.

  1. Are there bugs filed against those implementations?

Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1565716
Safari: https://bugs.webkit.org/show_bug.cgi?id=199762

  1. Is there a PR to make the tests stop being tentative that we can land together with this assuming all is in order? (There's a couple minor things I wonder about with the tests we can maybe tackle there, e.g., I don't see a need to do || 0 for the new fields as they should always be defined in a compliant implementation.)

https://chromium-review.googlesource.com/c/chromium/src/+/1700564

    squash 47187f6 Address style nits from domenic.
    squash 0fd3b44 Explicitly define each storage system's usage.
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 4, 2019
To be landed when the spec changes land.
whatwg/storage#69

Bug:904000
Change-Id: I630c3b73b4ad52f901eabddc7902973309a655f9
@annevk
Copy link
Member

annevk commented May 4, 2020

@jarryd999 could you check if #86 would make it significantly easier to define this? It seems to me we'd only have to define storage usage of a bucket's area and this would all fall out automatically.

Is there a reason for excluding localStorage?

@jarryd999
Copy link
Author

@annevk I'm not seeing how #86 makes this significantly easier to define. I think we are partially aligned: I've always imagined usageDetails to show a breakdown of usage per-bucket. However, I am confused why the addition of buckets would mean this "would all fall out automatically", as I think the only change would be to the scope of usageDetails. Could you help me understand?

localStorage was excluded because it is currently not quota-managed. My thoughts on this are that all storage should be quota-managed, or that at the least, quota should be aware of all storage, and thus it should be displayed in usageDetails. However, I'm not too familiar with the story around localStorage.

@annevk
Copy link
Member

annevk commented May 7, 2020

Various APIs fit into a single bucket, each assigned to an area. For this we'd only need to define the primitive of obtaining the size of an area and then with some loops we can fill in all the details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants