Bug 212529 - [WebXR] Pass an unsigned long to cancelAnimationCallback() as handle
Summary: [WebXR] Pass an unsigned long to cancelAnimationCallback() as handle
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sergio Villar Senin
URL:
Keywords: InRadar
Depends on: 213047
Blocks: 208988
  Show dependency treegraph
 
Reported: 2020-05-29 09:51 PDT by Sergio Villar Senin
Modified: 2020-06-11 01:09 PDT (History)
12 users (show)

See Also:


Attachments
Patch (3.40 KB, patch)
2020-05-29 09:55 PDT, Sergio Villar Senin
youennf: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>