Skip to content

/ servo

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial implementation of GPUDevice for WebGPU #25029

Merged
merged 1 commit into from Dec 5, 2019
Merged

Initial implementation of GPUDevice for WebGPU #25029

merged 1 commit into from Dec 5, 2019
+270 −16

Conversation

@imiklos
Copy link
Contributor

imiklos commented Dec 3, 2019

Added the WebIDL bindigs for GPUDevice, GPUObjectDescriptorBase, GPUDeviceDescriptor, GPUObjectBase
Implemented the requestDevice function of GPUAdapter

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes addresses a part of #24706

cc @jdm, @kvark, @zakorgy
@highfive
Copy link

highfive commented Dec 3, 2019

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/gpuadapter.rs, components/script/dom/navigator.rs, components/script/dom/mod.rs, components/script/dom/webidls/GPUObjectBase.webidl, components/script/dom/identityhub.rs and 5 more
  • @KiChjang: components/script/dom/gpuadapter.rs, components/script/dom/navigator.rs, components/script/dom/mod.rs, components/script/dom/webidls/GPUObjectBase.webidl, components/script/dom/identityhub.rs and 5 more
@highfive
Copy link

highfive commented Dec 3, 2019

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@kvark
kvark requested changes Dec 3, 2019
View changes
components/webgpu/lib.rs Outdated
@@ -77,6 +83,7 @@ struct WGPU {
adapters: Vec<WebGPUAdapter>,
// Track invalid adapters https://gpuweb.github.io/gpuweb/#invalid
_invalid_adapters: Vec<WebGPUAdapter>,
device_to_adapter: HashMap<WebGPUDevice, WebGPUAdapter>,

This comment has been minimized.

Sign in to view
Copy link
@kvark

kvark Dec 3, 2019

Member

do we need this map?
it seems like we should just carry a JS reference to WebGPUAdapter from the device struct

This comment has been minimized.

Sign in to view
Copy link
@imiklos

imiklos Dec 4, 2019

Author Contributor

We don't use it right now. I will remove this map. The reference to the Adapter is already in place. I will update the pull request with this change. Thank you!
@imiklos imiklos force-pushed the szeged:wgpu_request_device branch from 13aa6ef to be216a3 Dec 4, 2019
@kvark
kvark approved these changes Dec 4, 2019
View changes
@jdm
jdm requested changes Dec 4, 2019
View changes
components/script/dom/gpudevice.rs Outdated
}

impl GPUDevice {
pub fn new_inherited(

This comment has been minimized.

Sign in to view
Copy link
@jdm

jdm Dec 4, 2019

Member

nit: no need for pub.
components/script/dom/gpuadapter.rs
&self.global(),
&self,
Heap::default(),
Heap::default(),

This comment has been minimized.

Sign in to view
Copy link
@jdm

jdm Dec 4, 2019

Member

Will we create non-default values based on descriptor in the future?

This comment has been minimized.

Sign in to view
Copy link
@imiklos

imiklos Dec 5, 2019

Author Contributor

Yes, we will deal with it in a follow up patch.
components/script/dom/webidls/GPUDevice.webidl Outdated
* file, You can obtain one at https://mozilla.org/MPL/2.0/. */

// https://gpuweb.github.io/gpuweb/#gpudevice
[Exposed=(Window, DedicatedWorker)/*, Serializable */]

This comment has been minimized.

Sign in to view
Copy link
@jdm

jdm Dec 4, 2019

Member

This should be Pref="dom.webgpu.enabled".
@jdm jdm assigned jdm and unassigned paulrouget Dec 4, 2019
@CYBAI
CYBAI reviewed Dec 5, 2019
View changes
components/script/dom/gpuadapter.rs Outdated
fn RequestDevice(&self, descriptor: &GPUDeviceDescriptor, comp: InCompartment) -> Rc<Promise> {
let promise = Promise::new_in_current_compartment(&self.global(), comp);
let sender = response_async(&promise, self);
let id = self.global().as_window().Navigator().create_device_id();

This comment has been minimized.

Sign in to view
Copy link
@CYBAI

CYBAI Dec 5, 2019

Collaborator

I just noticed that GPUAdapter is possibly exposed to DedicatedWorker.

Maybe we should call these window methods inside a if let instead 👀?

Ex.

if let Some(window) = self.global().downcast::<Window>() {
    let id = window.Navigator().create_device_id();
    ...
}

This comment has been minimized.

Sign in to view
Copy link
@imiklos

imiklos Dec 5, 2019

Author Contributor

Thank you! I will updated the pull request with this change.
@imiklos
Initial implementation of GPUDevice for WebGPU
Loading status checks…
b15d2bb 
Added the WebIDL bindigs for GPUDevice, GPUObjectDescriptorBase, GPUDeviceDescriptor, GPUObjectBase
Implemented the `requestDevice` function of `GPUAdapter`
@imiklos imiklos force-pushed the szeged:wgpu_request_device branch from be216a3 to b15d2bb Dec 5, 2019
@imiklos
Copy link
Contributor Author

imiklos commented Dec 5, 2019

@jdm @CYBAI Thank you for your review! I've addressed your comments and updated the PR.
@jdm
Copy link
Member

jdm commented Dec 5, 2019

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Dec 5, 2019

📌 Commit b15d2bb has been approved by jdm
@bors-servo
Copy link
Contributor

bors-servo commented Dec 5, 2019

⌛ Testing commit b15d2bb with merge cc0007d...
bors-servo added a commit that referenced this pull request Dec 5, 2019
@bors-servo
Auto merge of #25029 - szeged:wgpu_request_device, r=jdm
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
cc0007d 
Initial implementation of GPUDevice for WebGPU

Added the WebIDL bindigs for GPUDevice, GPUObjectDescriptorBase, GPUDeviceDescriptor, GPUObjectBase
Implemented the `requestDevice` function of `GPUAdapter`

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X]  These changes addresses a part of #24706

cc @jdm, @kvark, @zakorgy

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Contributor

bors-servo commented Dec 5, 2019

💔 Test failed - status-taskcluster
@kvark
Copy link
Member

kvark commented Dec 5, 2019

@jdm error looks unrelated to the PR (android release font linking)
@jdm
Copy link
Member

jdm commented Dec 5, 2019

@bors-servo retry
@bors-servo
Copy link
Contributor

bors-servo commented Dec 5, 2019

⌛ Testing commit b15d2bb with merge 0fe139e...
bors-servo added a commit that referenced this pull request Dec 5, 2019
@bors-servo
Auto merge of #25029 - szeged:wgpu_request_device, r=jdm
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
0fe139e 
Initial implementation of GPUDevice for WebGPU

Added the WebIDL bindigs for GPUDevice, GPUObjectDescriptorBase, GPUDeviceDescriptor, GPUObjectBase
Implemented the `requestDevice` function of `GPUAdapter`

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X]  These changes addresses a part of #24706

cc @jdm, @kvark, @zakorgy

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Contributor

bors-servo commented Dec 5, 2019

💔 Test failed - status-taskcluster
@jdm
Copy link
Member

jdm commented Dec 5, 2019

@bors-servo retry

  • network issue
@bors-servo
Copy link
Contributor

bors-servo commented Dec 5, 2019

⌛ Testing commit b15d2bb with merge 6b2c326...
bors-servo added a commit that referenced this pull request Dec 5, 2019
@bors-servo
Auto merge of #25029 - szeged:wgpu_request_device, r=jdm
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
6b2c326 
Initial implementation of GPUDevice for WebGPU

Added the WebIDL bindigs for GPUDevice, GPUObjectDescriptorBase, GPUDeviceDescriptor, GPUObjectBase
Implemented the `requestDevice` function of `GPUAdapter`

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X]  These changes addresses a part of #24706

cc @jdm, @kvark, @zakorgy

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Contributor

bors-servo commented Dec 5, 2019

💔 Test failed - status-taskcluster
@jdm
Copy link
Member

jdm commented Dec 5, 2019

@bors-servo retry
@bors-servo
Copy link
Contributor

bors-servo commented Dec 5, 2019

⌛ Testing commit b15d2bb with merge bb00a83...
bors-servo added a commit that referenced this pull request Dec 5, 2019
@bors-servo
Auto merge of #25029 - szeged:wgpu_request_device, r=jdm
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
bb00a83 
Initial implementation of GPUDevice for WebGPU

Added the WebIDL bindigs for GPUDevice, GPUObjectDescriptorBase, GPUDeviceDescriptor, GPUObjectBase
Implemented the `requestDevice` function of `GPUAdapter`

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X]  These changes addresses a part of #24706

cc @jdm, @kvark, @zakorgy

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Contributor

bors-servo commented Dec 5, 2019

☀️ Test successful - status-taskcluster
Approved by: jdm
Pushing bb00a83 to master...
@bors-servo bors-servo merged commit b15d2bb into servo:master Dec 5, 2019
2 checks passed
2 checks passed
Community-TC (pull_request) TaskGroup: success
Details
homu Test successful
Details
This was referenced Dec 5, 2019
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CYBAI CYBAI

@jdm jdm

@kvark kvark

Assignees

@jdm jdm

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
@imiklos @highfive @jdm @bors-servo @kvark @CYBAI @paulrouget
You can’t perform that action at this time.