Bug 212099

Created attachment 399760 [details] Patch

Gentle ping

Comment on attachment 399760 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399760&action=review > Source/WebCore/Modules/webxr/WebXRSession.cpp:124 > +int WebXRSession::requestAnimationFrame(Ref<XRFrameRequestCallback>&& callback) Why do we return a signed integer, should we return an unsigned? > Source/WebCore/Modules/webxr/WebXRSession.cpp:135 > + scheduleAnimation(); scheduleAnimation() is only called there right now. In the future, it will be called more? How is it supposed to be hooked, a repeat timer? > Source/WebCore/Modules/webxr/XRFrameRequestCallback.h:44 > + int callbackId() { return m_id; } const > Source/WebCore/Modules/webxr/XRFrameRequestCallback.h:45 > + void setCallbackId(int id) { m_id = id; } I guess we should assert that m_id is zero if we initialise it to zero.

Comment on attachment 399760 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399760&action=review >> Source/WebCore/Modules/webxr/WebXRSession.cpp:124 >> +int WebXRSession::requestAnimationFrame(Ref<XRFrameRequestCallback>&& callback) > > Why do we return a signed integer, should we return an unsigned? I thought the same but the IDL specifies a "long" which is a signed integer. I might raise an issue to the WG because I don't think it makes much sense either. >> Source/WebCore/Modules/webxr/WebXRSession.cpp:135 >> + scheduleAnimation(); > > scheduleAnimation() is only called there right now. In the future, it will be called more? > How is it supposed to be hooked, a repeat timer? Authors need to call requestAnimationFrame() in their callbacks to get the next frame. Note that we aren't returning pose information yet. The retrieval of pose information from devices will be done independently of the existence of requests or not. We should generate XRFrame's as we get data from devices, and pass it to the author in the callbacks of requestAnimationFrame() if any. >> Source/WebCore/Modules/webxr/XRFrameRequestCallback.h:44 >> + int callbackId() { return m_id; } > > const OK. >> Source/WebCore/Modules/webxr/XRFrameRequestCallback.h:45 >> + void setCallbackId(int id) { m_id = id; } > > I guess we should assert that m_id is zero if we initialise it to zero. Good point although the thing is that id is a signed integer to in theory we could overflow positive integers and then reach 0 again after passing by all the negative ones.

Comment on attachment 399760 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399760&action=review >>> Source/WebCore/Modules/webxr/WebXRSession.cpp:124 >>> +int WebXRSession::requestAnimationFrame(Ref<XRFrameRequestCallback>&& callback) >> >> Why do we return a signed integer, should we return an unsigned? > > I thought the same but the IDL specifies a "long" which is a signed integer. I might raise an issue to the WG because I don't think it makes much sense either. An issue sounds good. >>> Source/WebCore/Modules/webxr/XRFrameRequestCallback.h:45 >>> + void setCallbackId(int id) { m_id = id; } >> >> I guess we should assert that m_id is zero if we initialise it to zero. > > Good point although the thing is that id is a signed integer to in theory we could overflow positive integers and then reach 0 again after passing by all the negative ones. Sure, but still that might capture the issue of overwriting the id, which would not be great. We should also probably add an assert to the getter that it is not zero. Or maybe we should have a class taking a Ref<XRFrameRequestCallback> and an ID in constructor, plus a settable boolean m_cancelled.

Created attachment 400255 [details] Patch Added ASSERTs

(In reply to youenn fablet from comment #5) > Comment on attachment 399760 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=399760&action=review > > >>> Source/WebCore/Modules/webxr/WebXRSession.cpp:124 > >>> +int WebXRSession::requestAnimationFrame(Ref<XRFrameRequestCallback>&& callback) > >> > >> Why do we return a signed integer, should we return an unsigned? > > > > I thought the same but the IDL specifies a "long" which is a signed integer. I might raise an issue to the WG because I don't think it makes much sense either. > > An issue sounds good. JFTR https://github.com/immersive-web/webxr/pull/1062 It looks like it will be approved, it's sensible and it also matches window's rAF (consistency yay!). I'll likely replace it before landing or after the next review.

(In reply to Sergio Villar Senin from comment #7) > (In reply to youenn fablet from comment #5) > > Comment on attachment 399760 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=399760&action=review > > > > >>> Source/WebCore/Modules/webxr/WebXRSession.cpp:124 > > >>> +int WebXRSession::requestAnimationFrame(Ref<XRFrameRequestCallback>&& callback) > > >> > > >> Why do we return a signed integer, should we return an unsigned? > > > > > > I thought the same but the IDL specifies a "long" which is a signed integer. I might raise an issue to the WG because I don't think it makes much sense either. > > > > An issue sounds good. > > JFTR https://github.com/immersive-web/webxr/pull/1062 > > It looks like it will be approved, it's sensible and it also matches > window's rAF (consistency yay!). I'll likely replace it before landing or > after the next review. Oh I didn't notice that you had r+'ed the previous patch and uploaded a new version. I guess you're fine with me landing this one too. I'll wait a bit for the WG resolution though as I just need one more approval to land the spec change (already got one).

Created attachment 400315 [details] Patch for landing

Committed r262188: <https://trac.webkit.org/changeset/262188>

<rdar://problem/63667956>