[webkit-reviews] review granted: [Bug 212099] [WebXR] Implement XRSession::requestAnimationFrame() : [Attachment 399760] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Sergio Villar Senin
<svillar at igalia.com>'s request for review:
Bug 212099: [WebXR] Implement XRSession::requestAnimationFrame()
https://bugs.webkit.org/show_bug.cgi?id=212099

Attachment 399760: Patch

https://bugs.webkit.org/attachment.cgi?id=399760&action=review




--- Comment #5 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 399760
  --> https://bugs.webkit.org/attachment.cgi?id=399760
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.