Bug 213462

Summary: [WebXR] Introducing XRLayer
Product: WebKit Reporter: Sergio Villar Senin <svillar>
Component: New BugsAssignee: Sergio Villar Senin <svillar>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, benjamin, calvaris, cdumez, cgarcia, darin, esprehn+autocc, ews-watchlist, gyuyoung.kim, kangil.han, koivisto, kondapallykalyan, ryuan.choi, sam, sergio, svillar, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 208988    
Attachments:
Description Flags
Patch youennf: review+

Description Sergio Villar Senin 2020-06-22 07:38:54 PDT
[WebXR] Introducing XRLayer
Comment 1 Sergio Villar Senin 2020-06-22 09:39:13 PDT
Created attachment 402484 [details]
Patch
Comment 2 youenn fablet 2020-06-23 06:37:57 PDT
Comment on attachment 402484 [details]
Patch

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

> Source/WebCore/Modules/webxr/WebXRLayer.h:48
> +    WebXRLayer(ScriptExecutionContext*);

explicit and ScriptExecutionContext& or is there a point in supporting nullptr?

> Source/WebCore/Modules/webxr/WebXRLayer.h:51
> +    ScriptExecutionContext* scriptExecutionContext() const override { return ContextDestructionObserver::scriptExecutionContext(); }

final?

> Source/WebCore/Modules/webxr/WebXRLayer.h:57
> +    void derefEventTarget() override { deref(); }

final for all of these?

> Source/WebCore/Modules/webxr/WebXRSession.h:80
> +    ScriptExecutionContext* scriptExecutionContext() const override { return ActiveDOMObject::scriptExecutionContext(); }

final?

> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:107
>  WebXRWebGLLayer::WebXRWebGLLayer(WebXRSession& session, WebXRRenderingContext&& context, const XRWebGLLayerInit& init)

Would be best to have a Ref<WebXRSession>&&

> Source/WebCore/Modules/webxr/WebXRWebGLLayer.h:62
> +    virtual ~WebXRWebGLLayer();

Not needed I believe
Comment 3 Sergio Villar Senin 2020-06-23 09:33:25 PDT
Comment on attachment 402484 [details]
Patch

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

Thanks for the review Youenn!

>> Source/WebCore/Modules/webxr/WebXRLayer.h:48
>> +    WebXRLayer(ScriptExecutionContext*);
> 
> explicit and ScriptExecutionContext& or is there a point in supporting nullptr?

OK

>> Source/WebCore/Modules/webxr/WebXRLayer.h:57
>> +    void derefEventTarget() override { deref(); }
> 
> final for all of these?

Ok

>> Source/WebCore/Modules/webxr/WebXRSession.h:80
>> +    ScriptExecutionContext* scriptExecutionContext() const override { return ActiveDOMObject::scriptExecutionContext(); }
> 
> final?

Ok

>> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:107
>>  WebXRWebGLLayer::WebXRWebGLLayer(WebXRSession& session, WebXRRenderingContext&& context, const XRWebGLLayerInit& init)
> 
> Would be best to have a Ref<WebXRSession>&&

Ok.

>> Source/WebCore/Modules/webxr/WebXRWebGLLayer.h:62
>> +    virtual ~WebXRWebGLLayer();
> 
> Not needed I believe

Indeed.
Comment 4 Sergio Villar Senin 2020-06-23 09:36:11 PDT
Committed r263403: <https://trac.webkit.org/changeset/263403>
Comment 5 Radar WebKit Bug Importer 2020-06-23 09:37:18 PDT
<rdar://problem/64648039>