Bug 212529

Summary: [WebXR] Pass an unsigned long to cancelAnimationCallback() as handle
Product: WebKit Reporter: Sergio Villar Senin <svillar>
Component: New BugsAssignee: Sergio Villar Senin <svillar>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, darin, dbates, esprehn+autocc, ews-watchlist, kondapallykalyan, ross.kirsling, svillar, webkit-bug-importer, youennf, ysuzuki, zan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 213047    
Bug Blocks: 208988    
Attachments:
Description Flags
Patch youennf: review+

Description Sergio Villar Senin 2020-05-29 09:51:07 PDT
[WebXR] Pass an unsigned long to cancelAnimationCallback() as handle
Comment 1 Sergio Villar Senin 2020-05-29 09:55:23 PDT
Created attachment 400595 [details]
Patch
Comment 2 Sergio Villar Senin 2020-06-05 08:51:56 PDT
Gentle ping
Comment 3 youenn fablet 2020-06-08 01:07:28 PDT
Comment on attachment 400595 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=400595&action=review

> Source/WebCore/Modules/webxr/WebXRSession.cpp:212
> +void WebXRSession::cancelAnimationFrame(XRFrameRequestCallback::Id callbackId)

I would call it XRFrameRequestCallback::Identifier
Comment 4 Sergio Villar Senin 2020-06-08 08:59:44 PDT
Committed r262718: <https://trac.webkit.org/changeset/262718>
Comment 5 Radar WebKit Bug Importer 2020-06-08 09:00:22 PDT
<rdar://problem/64120637>
Comment 6 Darin Adler 2020-06-08 11:39:09 PDT
Comment on attachment 400595 [details]
Patch

IDL "unsigned long" is the same as C++ "unsigned" in WebKit, so all the "unsigned long" and "long" should be changed to "unsigned" and "int" in the C headers.
Comment 7 Sergio Villar Senin 2020-06-10 04:30:54 PDT
(In reply to Darin Adler from comment #6)
> Comment on attachment 400595 [details]
> Patch
> 
> IDL "unsigned long" is the same as C++ "unsigned" in WebKit, so all the
> "unsigned long" and "long" should be changed to "unsigned" and "int" in the
> C headers.

OK, I've seen unsigned long is some other headers in Modules/ so I was not really sure.(In reply to Darin Adler from comment #6)
> Comment on attachment 400595 [details]
> Patch
> 
> IDL "unsigned long" is the same as C++ "unsigned" in WebKit, so all the
> "unsigned long" and "long" should be changed to "unsigned" and "int" in the
> C headers.

Thanks for this, I've seen "unsigned long" in some other modules so I thought it was also OK. I've just filled bug 213020 to address WebXR changes.
Comment 8 Darin Adler 2020-06-10 09:53:40 PDT
(In reply to Sergio Villar Senin from comment #7)
> Thanks for this, I've seen "unsigned long" in some other modules so I
> thought it was also OK. I've just filled bug 213020 to address WebXR changes.

It’s worthwhile to fix those to consistently not use the "long" C types. Should be fairly simple task.

Using "long" for 32-bit integers isn't super-actively harmful but can lead to problems where two types are involved where the types should be the same, even possibly leading to template code bloat or need to have two functions where we'd like to have only one. And it’s basically a difference where there should be no difference. The fact that 32-bit is named "long" in IDL is, in effect, unrelated to the "long" in C.
Comment 9 Yusuke Suzuki 2020-06-10 13:43:49 PDT
Re-opened since this is blocked by bug 213047
Comment 10 Sergio Villar Senin 2020-06-11 01:09:41 PDT
Committed r262898: <https://trac.webkit.org/changeset/262898>