-
Notifications
You must be signed in to change notification settings - Fork 2k
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 Oculus Browser to browsers, and compatibility data for api/A* #12642
Conversation
@@ -64,6 +64,16 @@ | |||
"ie": { | |||
"version_added": "10" | |||
}, | |||
"oculus": { | |||
"flags": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does oculus browser support flags? And more specifically does it support this flag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, Oculus Browser supports flags. The URL is chrome://flags similar to Chrome. In general our flags are the Chrome ones, same names, plus a couple of Oculus Browser specific ones. Experimental Web Features specifically is a valid flag for Oculus Browser.
"oculus": { | ||
"accepts_flags": true, | ||
"name": "Oculus Browser", | ||
"pref_url": "chrome://flags", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just double checking this is the correct URL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. For various reasons Oculus Browser uses the chrome:
scheme for these surfaces; chrome://flags
is right.
{ | ||
"browsers": { | ||
"oculus": { | ||
"accepts_flags": true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as comment above, just wanna double check does the oculus browser support flags?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! Oculus Browser supports flags by entering chrome://flags
in the address bar. We don't navigate links to that URL for security reasons but entering it directly brings up a page to search and set flags similar to Chrome for Android.
d4ecdff
to
fa4380e
Compare
Thank you @lukewarlow for the feedback. I've fixed the note and added some replies inline. Please take another look. |
If I could bring particular attention to something, what do you think about the notes on AbsoluteOrientationSensor, Accelerometer and AmbientLightSensor? For a number of APIs (mostly sensors but some others, for example Geolocation, Web Share, etc.) Oculus Browser has an interface present, all the prototypes are right, etc. But the API is a dead end because we don't light up something about the API: Sensors never generate readings; sharing requests are always rejected; etc. (At the moment.) Would it be best to say that is "partial" support so developers get an indication that something's up? That also looks a bit weird because every API surface is "present." |
} | ||
], | ||
"version_added": "5.0", | ||
"notes": "<code>AmbientLightSensor</code> is defined but it never generates any readings." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would at a minimum mark as partially implemented. Same for any other sensors that are defined but don't work.
Though I think potentially while it's still flagged just leave it as version_added false. And then maybe once it's unflagged a decision could be made on how to represent it.
Potentially could set version_added false but leave the note. Not sure what's best, one of the maintainers will be better placed to make that decision.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say as a web developer if it straight up doesn't work and is still flagged the partial with flag is pretty useless information. Once it's unflagged and I can find the interface a note (+ optionally a partial implementation boolean) would be very useful to explain the odd behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think partial is the right call here. If the interface exists, then it'll complicate feature detection; showing version_added: false
is going to tell a simple but misleading story instead.
Hey @dominiccooney, this is looking great so far! Question: are there any compatibility differences between Oculus browser versions with the same Blink version that we would track here in BCD? If not, we should consolidate the releases like we do for NodeJS and strip out any releases where we wouldn't add compatibility data. |
Yes. While the "closest" competitor is Chrome for Android, there are some features Oculus Browser doesn't support (Web Share, Web Payments, Notifications, many of the Sensor subtypes ...) Many have stubs which return rejected Promises/generate no callbacks/etc. And Oculus Browser supports some XR features that Chrome for Android does not (IIRC frame rate control, hands input, composited layers...) |
@dominiccooney I'm not talking about differences between Chrome Android and Oculus Browser. What I mean is: you're adding entries for every version of the Oculus Browser in this PR, most of which have matching engine versions (Oculus 6.0 and 6.1 are powered by Blink 74). Are there any compatibility differences in between these versions, i.e. was there something BCD tracks that was introduced in Oculus 6.1 and wasn't in Oculus 6.0? |
Ah, got it. No. This is one point I was keen for feedback on, should we even mention point releases which don't appear in changes like that? In general point releases can have changes that BCD would track. For example 7.1 added WebXR gamepads and fixed problems with the OVR_multiview2 WebGL extension. |
Thanks for the response! Our stance for minor releases for NodeJS has been to only include "significant" releases (i.e. releases with a change in compatibility) rather than every release, so I feel that we should do the same for Oculus Browser as well (and probably at some point consolidate Samsung Internet releases, but that's another issue), and add in the releases with a minor semver bump as needed. We don't have a guideline for this, admittedly. |
fa4380e
to
c0a8b4f
Compare
@lukewarlow, @queengooborg, @ddbeck I think this is ready for another round of feedback. Please take another look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for putting this together, @dominiccooney! I've just started to dip into this, but I've got a few initial comments for you. Let me know if you have questions and I'll return to this as soon as I can.
"browsers": { | ||
"oculus": { | ||
"accepts_flags": true, | ||
"name": "Oculus Browser", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: would you mind putting this key first? It's nice to have first thing, when referring to the file for version mappings, etc.
"name": "Oculus Browser", | ||
"pref_url": "chrome://flags", | ||
"releases": { | ||
"5.0": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be clear: this means that "5.0" will end up being the first version represented in compat tables, etc. Is that OK? I figure this is preferable to researching older features, but I wanted to confirm it.
"oculus": { | ||
"version_added": "6.0", | ||
"partial_implementation": true, | ||
"notes": "<code>AbsoluteOrientationSensor</code> is defined but it never generates any readings." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know you're not there yet alphabetically, but I'd expect a note consistent with this to appear on the ancestor interface property and method too (i.e., the places where you'd actually try to get the readings).
"oculus": { | ||
"version_added": "6.0", | ||
"partial_implementation": true, | ||
"notes": "<code>Accelerometer</code> is defined but it never generates any readings." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a case where you'll probably want to show partial and a note on the members of the interface too. Otherwise, a reader can go to the page for Accelerometer.x
on MDN and miss out on this information.
@@ -44,6 +44,18 @@ | |||
"ie": { | |||
"version_added": false | |||
}, | |||
"oculus": { | |||
"flags": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: it's not codified anywhere and ought to be, but it's conventional to order this: added, removed, partial, notes, flags (or near enough to that).
} | ||
], | ||
"version_added": "5.0", | ||
"notes": "<code>AmbientLightSensor</code> is defined but it never generates any readings." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think partial is the right call here. If the interface exists, then it'll complicate feature detection; showing version_added: false
is going to tell a simple but misleading story instead.
@dominiccooney ping :) |
I've opened up #15923 to handle the addition of the browser data, cherry-picked from this PR! |
Sorry for the slow reply @queengooborg . My understanding is: People are working on this. But I'm not sure when they plan to update this PR so for now I will close this to reduce clutter. We can always review the feedback in the closed PR. Thanks for the feedback (@ddbeck too) and sorry for the spam keeping this open for so long. |
Summary
This adds Oculus Browser to schema, browsers, templates and tests with compatibility data for everything in api/A*.
Test results and supporting details
npm run lint
passeshttps://developer.oculus.com/documentation/web/browser-release-notes/
Related issues
See #12303