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

API/StorageManager: add compat data #2272

Merged
merged 3 commits into from
Jul 20, 2018
Merged

API/StorageManager: add compat data #2272

merged 3 commits into from
Jul 20, 2018

Conversation

@@ -5,34 +5,56 @@
"mdn_url": "https://developer.mozilla.org/docs/Web/API/StorageManager",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

First of all, I have added the compat data despite the ongoing discussion in #190 about representing the "secure context" requirement.

In my opinion, a browser could be considered compatible if it offers the feature in the secure context as defined by the spec. On the other hand, I wouldn't consider a browser incompatible, if it also offers the feature outside of the [required] secure context, but instead point this out in a "notes" entry.

},
"chrome": {
"version_added": null
"version_added": "48"
Copy link
Contributor Author

@caugner caugner Jun 9, 2018

Choose a reason for hiding this comment

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

MDN stated 55, but basic and full support was already established in 48 and 52 respectively:

{
"type": "preference",
"name": "dom.storageManager.enabled",
"value_to_set": "true"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

MDN stated that, from version 55, the preference browser.storageManager.enabled was also required, but I found no evidence for this (neither in the repository nor in the bugs).

{
"type": "preference",
"name": "dom.storageManager.enabled",
"value_to_set": "true"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

},
"ie": {
"version_added": null
},
"opera": {
"version_added": null
"version_added": true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since I updated the Chrome versions and am not aware of a Chrome version to Opera version mapping (like [the one for Samsung Internet]), I have reset these versions to true.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Opera is usually Chrome - 13, so in this case 35. But I've always been a little bit uncomfortable doing that on an automatic basis, so you don't actually have to change this. I'll leave it up to your judgment

@Elchi3 Elchi3 added the data:api 🐇 Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API label Jun 13, 2018
Copy link
Collaborator

@ddbeck ddbeck left a comment

Choose a reason for hiding this comment

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

Nice research on this @caugner. A few points, but this is pretty close. Thank you!

{
"version_added": "51",
"version_removed": "57",
"notes": "See <a href='https://bugzil.la/1304966'>Bug 1304966</a> and <a href='https://bugzil.la/1399038'>Bug 1399038</a>.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we routinely capitalize "bug"

Copy link
Contributor Author

@caugner caugner Jul 20, 2018

Choose a reason for hiding this comment

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

Thanks! I just that in 274f8ab and added issue #2509 to potentially check this as part of linting.

"firefox_android": {
"version_added": null
"version_added": "51",
"notes": "See <a href='https://bugzil.la/1304966'>Bug 1304966</a> and <a href='https://bugzil.la/1399038'>Bug 1399038</a>.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Bug capitalization here too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ditto

},
"ie": {
"version_added": null
},
"opera": {
"version_added": null
"version_added": true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Opera is usually Chrome - 13, so in this case 35. But I've always been a little bit uncomfortable doing that on an automatic basis, so you don't actually have to change this. I'll leave it up to your judgment

@@ -5,34 +5,56 @@
"mdn_url": "https://developer.mozilla.org/docs/Web/API/StorageManager",
"support": {
"webview_android": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since I've suggested other changes, may as well sort the browsers too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, I just did that in 4125b1a.

@caugner
Copy link
Contributor Author

caugner commented Jul 20, 2018

@ddbeck Regarding the Opera version (for some reason I cannot directly reply to that comment), I would think it's better – for the sake of traceability – to make such derivations systematically in a PR dedicated to that purpose.

@ddbeck
Copy link
Collaborator

ddbeck commented Jul 20, 2018

Good idea! I opened #2510 to tackle the question generally.

Copy link
Collaborator

@ddbeck ddbeck left a comment

Choose a reason for hiding this comment

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

Thank you @caugner! 🎉

@ddbeck ddbeck merged commit d678f5b into mdn:master Jul 20, 2018
@caugner caugner deleted the API/StorageManager branch July 20, 2018 22:13
@foolip
Copy link
Collaborator

foolip commented Jul 23, 2021

3 years have passed, but I thought I'd point out for anyone subscribed to this PR that there was a problem with how the Chromium versions were determined. Just looking at when Web IDL was added to Chromium isn't reliable, because it can still be behind a runtime flag. In this case, that's the RuntimeEnabled=DurableStorage in https://chromium.googlesource.com/chromium/src/+/21ba553558ed7cd65f59199b28dd8732f5dc93a0%5E%21/third_party/WebKit/Source/modules/quota/StorageManager.idl

The data is being updated in #11698.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data:api 🐇 Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants