Bug 190653 - [WebGPU] Implement WebGPU bindings up through WebGPUDevice creation
Summary: [WebGPU] Implement WebGPU bindings up through WebGPUDevice creation
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: Justin Fan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-10-16 16:44 PDT by Justin Fan
Modified: 2018-10-18 09:28 PDT (History)
6 users (show)

See Also:


Attachments
Patch (45.04 KB, patch)
2018-10-16 16:51 PDT, Justin Fan
no flags Details | Formatted Diff | Diff
Patch (44.55 KB, patch)
2018-10-16 17:05 PDT, Justin Fan
no flags Details | Formatted Diff | Diff
Patch (44.40 KB, patch)
2018-10-17 12:47 PDT, Justin Fan
no flags Details | Formatted Diff | Diff
Patch (44.35 KB, patch)
2018-10-17 12:51 PDT, Justin Fan
no flags Details | Formatted Diff | Diff
Patch (44.26 KB, patch)
2018-10-17 13:36 PDT, Justin Fan
no flags Details | Formatted Diff | Diff
Patch (44.26 KB, patch)
2018-10-17 13:53 PDT, Justin Fan
no flags Details | Formatted Diff | Diff
Patch for landing (44.26 KB, patch)
2018-10-17 15:53 PDT, Justin Fan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Fan 2018-10-16 16:44:06 PDT
[WebGPU] Implement WebGPU bindings up through WebGPUDevice creation
Comment 1 Justin Fan 2018-10-16 16:51:55 PDT
Created attachment 352519 [details]
Patch
Comment 2 Justin Fan 2018-10-16 17:05:20 PDT
Created attachment 352521 [details]
Patch
Comment 3 Myles C. Maxfield 2018-10-16 17:13:48 PDT
Comment on attachment 352521 [details]
Patch

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

> Source/WebCore/Modules/webgpu/WebGPU.cpp:46
> +    promise.resolve(WebGPUAdapter::create(descriptor));

'eyyyyyyy

> Source/WebCore/Modules/webgpu/WebGPUDevice.h:42
> +    WebGPUAdapter& adapter() const { return *m_adapter.get(); } // FIXME: Probably not right; possible to deref nullptr.

this is definitely not right.
Comment 4 Dean Jackson 2018-10-16 17:24:37 PDT
Comment on attachment 352521 [details]
Patch

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

> Source/WebCore/Modules/webgpu/DOMWindowWebGPU.cpp:49
> +// static

Remove this.

> Source/WebCore/Modules/webgpu/DOMWindowWebGPU.cpp:61
> +// static

Same.

> Source/WebCore/Modules/webgpu/DOMWindowWebGPU.idl:27
> +// Add a "webgpu" member to Window that contains the global instance of a "WebGPU"

I don't think we need this comment.

> Source/WebCore/Modules/webgpu/WebGPU.cpp:37
> +// static

Remove.

> Source/WebCore/Modules/webgpu/WebGPU.cpp:46
> +    WebGPUAdapterPromise promise = WTFMove(deferred);
> +    promise.resolve(WebGPUAdapter::create(descriptor));

This is ok for now, but this will eventually keep a list of active adapters and pass out references to them.

> Source/WebCore/Modules/webgpu/WebGPU.cpp:49
> +WebGPU::WebGPU() = default;

I think this should go after ::create, or in the header.

> Source/WebCore/Modules/webgpu/WebGPU.h:34
> +class WebGPUAdapter;

Don't need this.

> Source/WebCore/Modules/webgpu/WebGPUAdapter.cpp:37
> +// static

Remove.

> Source/WebCore/Modules/webgpu/WebGPUDevice.h:47
> +    WeakPtr<WebGPUAdapter> m_adapter;

Why is this weak?
Comment 5 Justin Fan 2018-10-17 12:47:01 PDT
Created attachment 352614 [details]
Patch
Comment 6 Justin Fan 2018-10-17 12:51:05 PDT
Created attachment 352617 [details]
Patch
Comment 7 Justin Fan 2018-10-17 13:36:45 PDT
Created attachment 352631 [details]
Patch
Comment 8 Dean Jackson 2018-10-17 13:37:56 PDT
Comment on attachment 352617 [details]
Patch

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

> Source/WebCore/Modules/webgpu/WebGPUAdapter.cpp:45
> +    UNUSED_PARAM(m_descriptor);

This is not needed.

> Source/WebCore/Modules/webgpu/WebGPUDevice.cpp:41
> +    UNUSED_PARAM(m_adapter);

Nor this.
Comment 9 Dean Jackson 2018-10-17 13:38:45 PDT
Comment on attachment 352631 [details]
Patch

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

> Source/WebCore/Modules/webgpu/WebGPUAdapter.cpp:45
> +    UNUSED_PARAM(m_descriptor);

Remove.

> Source/WebCore/Modules/webgpu/WebGPUDevice.cpp:41
> +    UNUSED_PARAM(m_adapter);

Remove.
Comment 10 Justin Fan 2018-10-17 13:53:38 PDT
(In reply to Dean Jackson from comment #9)
> Comment on attachment 352631 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=352631&action=review
> 
> > Source/WebCore/Modules/webgpu/WebGPUAdapter.cpp:45
> > +    UNUSED_PARAM(m_descriptor);
> 
> Remove.
> 
> > Source/WebCore/Modules/webgpu/WebGPUDevice.cpp:41
> > +    UNUSED_PARAM(m_adapter);
> 
> Remove.

If I don't use the private member variables, I get a "Private field __ is unused" error when compiling :(
Comment 11 Justin Fan 2018-10-17 13:53:46 PDT
Created attachment 352634 [details]
Patch
Comment 12 WebKit Commit Bot 2018-10-17 15:25:21 PDT
Comment on attachment 352634 [details]
Patch

Rejecting attachment 352634 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 352634, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: https://webkit-queues.webkit.org/results/9643624
Comment 13 Justin Fan 2018-10-17 15:53:58 PDT
Created attachment 352657 [details]
Patch for landing
Comment 14 WebKit Commit Bot 2018-10-17 16:31:05 PDT
Comment on attachment 352657 [details]
Patch for landing

Clearing flags on attachment: 352657

Committed r237239: <https://trac.webkit.org/changeset/237239>
Comment 15 WebKit Commit Bot 2018-10-17 16:31:06 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Radar WebKit Bug Importer 2018-10-17 16:33:33 PDT
<rdar://problem/45355883>
Comment 17 Truitt Savell 2018-10-18 08:15:02 PDT
the new test webgpu/webgpu-enabled.html

added in https://trac.webkit.org/changeset/237239/webkit

appears to have no test expectations and is showing missing test results. 

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=webgpu%2Fwebgpu-enabled.html