Bug 234138 – [GPUP] Create WebGL context with task id token

• Component: WebKit2
• Status: RESOLVED FIXED

[John Cunningham]

Use EGL_ANGLE_metal_create_context_ownership_identity to create a WebGL context when GPUP and WebGL in GPUP is enabled

[John Cunningham]

Created attachment 446707 [details]
Patch

[John Cunningham]

Created attachment 446772 [details]
Patch

[John Cunningham]

Created attachment 446781 [details]
Patch

[Darin Adler]

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.

[John Cunningham]

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

[John Cunningham]

Created attachment 446803 [details]
Patch

[Darin Adler]

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?

[Darin Adler]

(In reply to John Cunningham from comment #5)
> 'webkit-patch format' changes it from "const char* " to "const char *"

Incredible! Why would it?

[John Cunningham]

Created attachment 446804 [details]
Patch

[John Cunningham]

(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.

[Kimmo Kinnunen]

(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.

[Darin Adler]

(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".

[John Cunningham]

Created attachment 447243 [details]
Patch for landing

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/86583119>