Bug 234138 - [GPUP] Create WebGL context with task id token
Summary: [GPUP] Create WebGL context with task id token
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-12-10 03:40 PST by John Cunningham
Modified: 2021-12-16 10:17 PST (History)
6 users (show)

See Also:


Attachments
Patch (2.01 KB, patch)
2021-12-10 03:43 PST, John Cunningham
no flags Details | Formatted Diff | Diff
Patch (3.04 KB, patch)
2021-12-10 11:46 PST, John Cunningham
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (4.33 KB, patch)
2021-12-10 12:18 PST, John Cunningham
no flags Details | Formatted Diff | Diff
Patch (4.31 KB, patch)
2021-12-10 13:33 PST, John Cunningham
no flags Details | Formatted Diff | Diff
Patch (4.16 KB, patch)
2021-12-10 13:46 PST, John Cunningham
no flags Details | Formatted Diff | Diff
Patch for landing (4.25 KB, patch)
2021-12-15 09:29 PST, John Cunningham
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Cunningham 2021-12-10 03:40:40 PST
Use EGL_ANGLE_metal_create_context_ownership_identity to create a WebGL context when GPUP and WebGL in GPUP is enabled
Comment 1 John Cunningham 2021-12-10 03:43:15 PST
Created attachment 446707 [details]
Patch
Comment 2 John Cunningham 2021-12-10 11:46:52 PST
Created attachment 446772 [details]
Patch
Comment 3 John Cunningham 2021-12-10 12:18:25 PST
Created attachment 446781 [details]
Patch
Comment 4 Darin Adler 2021-12-10 12:27:36 PST
Comment on attachment 446781 [details]
Patch

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

A few coding style tweaks

> Source/WebCore/platform/graphics/angle/GraphicsContextGLANGLE.h:367
> +    GraphicsContextGLANGLE(GraphicsContextGLAttributes, ProcessIdentity&);

Could this be "const ProcessIdentity&"? It’s a bit unusual to pass a non-constant lvalue reference and I don’t see why we need to.

> Source/WebCore/platform/graphics/cocoa/GraphicsContextGLCocoa.mm:204
> +GraphicsContextGLCocoa::GraphicsContextGLCocoa(GraphicsContextGLAttributes &&creationAttributes, ProcessIdentity &&resourceOwner)

Why moving these "&&"? WebKit coding style calls for the way we had it already.

> Source/WebCore/platform/graphics/cocoa/GraphicsContextGLCocoa.mm:336
> +    const char *displayExtensions = EGL_QueryString(m_displayObj, EGL_EXTENSIONS);

WebKit coding style calls for "const char*" or "auto".

> Source/WebCore/platform/graphics/cocoa/GraphicsContextGLCocoa.mm:340
> +        task_id_token_t ownerTaskIdToken = resourceOwner.taskIdToken();

Seems like we don’t need this local variable.
Comment 5 John Cunningham 2021-12-10 13:08:50 PST
Comment on attachment 446781 [details]
Patch

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

>> Source/WebCore/platform/graphics/angle/GraphicsContextGLANGLE.h:367
>> +    GraphicsContextGLANGLE(GraphicsContextGLAttributes, ProcessIdentity&);
> 
> Could this be "const ProcessIdentity&"? It’s a bit unusual to pass a non-constant lvalue reference and I don’t see why we need to.

I'll change it to const. I don't believe leaving it as && would work as that would lead to a use after move bug at GraphicsContextGLCocoa.mm:215. Once the fix there is resolved, it would be good to remove this workaround and keep it as &&.

>> Source/WebCore/platform/graphics/cocoa/GraphicsContextGLCocoa.mm:204
>> +GraphicsContextGLCocoa::GraphicsContextGLCocoa(GraphicsContextGLAttributes &&creationAttributes, ProcessIdentity &&resourceOwner)
> 
> Why moving these "&&"? WebKit coding style calls for the way we had it already.

Above comment

>> Source/WebCore/platform/graphics/cocoa/GraphicsContextGLCocoa.mm:336
>> +    const char *displayExtensions = EGL_QueryString(m_displayObj, EGL_EXTENSIONS);
> 
> WebKit coding style calls for "const char*" or "auto".

'webkit-patch format' changes it from "const char* " to "const char *", but I'm also happy to just use auto here.

>> Source/WebCore/platform/graphics/cocoa/GraphicsContextGLCocoa.mm:340
>> +        task_id_token_t ownerTaskIdToken = resourceOwner.taskIdToken();
> 
> Seems like we don’t need this local variable.

Done
Comment 6 John Cunningham 2021-12-10 13:33:57 PST
Created attachment 446803 [details]
Patch
Comment 7 Darin Adler 2021-12-10 13:34:11 PST
Comment on attachment 446781 [details]
Patch

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

>>> Source/WebCore/platform/graphics/cocoa/GraphicsContextGLCocoa.mm:204
>>> +GraphicsContextGLCocoa::GraphicsContextGLCocoa(GraphicsContextGLAttributes &&creationAttributes, ProcessIdentity &&resourceOwner)
>> 
>> Why moving these "&&"? WebKit coding style calls for the way we had it already.
> 
> Above comment

What does that comment have to do with space character placement?

I meant why are you touching this line of code at all, changing "GraphicsContextGLAttributes&& creationAttributes" to "GraphicsContextGLAttributes &&creationAttributes", which has no effect but goes against WebKit coding style?
Comment 8 Darin Adler 2021-12-10 13:34:49 PST
(In reply to John Cunningham from comment #5)
> 'webkit-patch format' changes it from "const char* " to "const char *"

Incredible! Why would it?
Comment 9 John Cunningham 2021-12-10 13:46:06 PST
Created attachment 446804 [details]
Patch
Comment 10 John Cunningham 2021-12-10 13:46:34 PST
(In reply to Darin Adler from comment #7)
> Comment on attachment 446781 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=446781&action=review
> 
> >>> Source/WebCore/platform/graphics/cocoa/GraphicsContextGLCocoa.mm:204
> >>> +GraphicsContextGLCocoa::GraphicsContextGLCocoa(GraphicsContextGLAttributes &&creationAttributes, ProcessIdentity &&resourceOwner)
> >> 
> >> Why moving these "&&"? WebKit coding style calls for the way we had it already.
> > 
> > Above comment
> 
> What does that comment have to do with space character placement?
> 
> I meant why are you touching this line of code at all, changing
> "GraphicsContextGLAttributes&& creationAttributes" to
> "GraphicsContextGLAttributes &&creationAttributes", which has no effect but
> goes against WebKit coding style?

Understood now, unsure of what is going on with automated formatting, but for now I have manually changed to webkit's style.
Comment 11 Kimmo Kinnunen 2021-12-13 00:26:22 PST
(In reply to Darin Adler from comment #8)
> (In reply to John Cunningham from comment #5)
> > 'webkit-patch format' changes it from "const char* " to "const char *"
> 
> Incredible! Why would it?

So the code is Objective C++. I think the tool is correct but the WebKit development practice does not adhere to its own rules.

https://webkit.org/code-style-guidelines/#pointers-non-cpp

  Pointer types in non-C++ code
  Pointer types should be written with a space between the type and the * (so the * is adjacent to the following identifier if any).

Of course it's matter of interpretation, as to whether that "non-C++" refers to "Objective C" only or "Objective C and Objective C++". However, since there are very few .m files, I'd imagine if that rule exists, that covers .mm files, .m files and .h files intended to be used from .m and .mm files.


>> What does that comment have to do with space character placement?
>> 
>> I meant why are you touching this line of code at all, changing
>> "GraphicsContextGLAttributes&& creationAttributes" to
>> "GraphicsContextGLAttributes &&creationAttributes", which has no effect but
>> goes against WebKit coding style?


> Understood now, unsure of what is going on with automated formatting, but for now I > have manually changed to webkit's style.

My bad, I instructed John to use webkit-patch format for ANGLE code, where it works because that code conforms to the Google coding style. I did not explain that currently the automatic formatting is not working as expected for WebKit code.
Comment 12 Darin Adler 2021-12-14 10:44:37 PST
(In reply to Kimmo Kinnunen from comment #11)
> So the code is Objective C++. I think the tool is correct but the WebKit
> development practice does not adhere to its own rules.

The style document is wrong. That should be fixed. Please don’t enforce inaccurate things from the style document. We, the WebKit community, will help you figure out the mistakes there. We even have the collective power to change our style. But this is unambiguously an error in how that document was written, please don’t change our style by interpreting that document as "truth".
Comment 13 John Cunningham 2021-12-15 09:29:34 PST
Created attachment 447243 [details]
Patch for landing
Comment 14 EWS 2021-12-16 10:16:02 PST
Committed r287144 (245328@main): <https://commits.webkit.org/245328@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 447243 [details].
Comment 15 Radar WebKit Bug Importer 2021-12-16 10:17:18 PST
<rdar://problem/86583119>