Make the copy_buffer_to_buffer size parameter optional

[andyleiserson]

This makes the size parameter to wgpu's copy_buffer_to_buffer implementation optional. When the size is omitted, the copy spans from the source offset to the end of the source buffer. The bug for making the size parameter to copyBufferToBuffer optional in Firefox is https://bugzilla.mozilla.org/show_bug.cgi?id=1959728.

As suggested by @cwfitzgerald, to avoid a breaking change, the type of the argument is impl Into<Option<BufferAddress>>. This allows existing calls to keep working, because Option<T> implements From<T>.

Testing
No tests, right now, other than not breaking existing tests. This is covered by the WebGPU CTS, but due to #7391, the relevant CTS test (webgpu:api,operation,command_buffer,copyBufferToBuffer:single:newSig=true) are not passing even with this fix. Manual inspection of the CTS output does show that with this change, nearly all of the subcases of that CTS test are passing.

Squash or Rebase? Squash

Checklist

• Run cargo fmt.
• Run taplo format.
• Run cargo clippy --tests. If applicable, add:
  • --target wasm32-unknown-unknown
• Run cargo xtask test to run tests.
• If this contains user-facing changes, add a CHANGELOG.md entry.

[crowlKats]

this shouldnt be like this, but rather the paramater of the function calling this should have its type be made Option<u64>

[andyleiserson]

I can update this overload to make the size parameter optional.

I wasn't sure how to describe the 3-argument overload with the #[webidl] attributes. Can you suggest a strategy? (See https://www.w3.org/TR/webgpu/#commands-buffer-copies)

[andyleiserson]

@crowlKats I attempted a deno change to support all the signatures, see here: andyleiserson/wgpu@copy-buffer-optional-size...andyleiserson:wgpu:deno-copy-buf

Can you take a look and let me know if that is a good approach? I can add it to this PR if desired.

(Edit, to preserve context for posterity since I will likely delete the branch at some point: the version we didn't end up going with had the following match statement:

let (src_offs_arg, dst_arg, dst_offs_arg, size_arg) = match args.length() {
    2 => (None, 1, None, None),
    3 => (None, 1, None, Some(2)),
    4 => (Some(1), 2, Some(3), None),
    5 => (Some(1), 2, Some(3), Some(4)),
    _ => return Err(WebIdlError::new(Cow::Borrowed("foo"), (|| Cow::Borrowed("bar")).into(), WebIdlErrorKind::NotFinite)),
};

and then used src_offs_arg.map(...).transpose()?.unwrap_or(0).)

[crowlKats]

sorry for the delay @andyleiserson. that looks interesting, however a bit confusing and somewhat unclear. you could look at what we do with other overloads (ie setBindGroup).

[andyleiserson]

The setBindGroup case is simpler because the overloads are more similar -- there's just two cases, either get the offsets one way from argument 3, or get them a different way from arguments 3-5. Argument 3 is the only argument that can have two possible types.

For copyBufferToBuffer, the 3-argument overload is effectively omitting the 2nd and 4th arguments to the 5-argument overload. So I could change the implementation to take v8::Local<'a, v8::Value> for arguments 2 through 5 instead of using #[varargs], but the logic of the function would otherwise be pretty similar, so I'm not sure if this is what you have in mind to make it less confusing.

If there were a way to define two different functions copy_buffer_to_buffer_with_three_arguments and copy_buffer_to_buffer_with_five_arguments and automatically dispatch to the right one, that seems like it would be clearer, but I don't think there's a way to do that?

Or I could make separate 3-arg and 5-arg helper functions, and dispatch to one or the other from the main WebIDL-exposed function based on the argument count. That would make a linear reading of the code simpler at the cost of some duplication (because both of the helpers would have convert calls for source buffer, dest buffer, and size).

[crowlKats]

automatically dispatch to the right one, that seems like it would be clearer, but I don't think there's a way to do that?

correct, there is no way to do that and unlikely we will addd it.

I think best is probably the last approach you mentioned; duplicate logic for this seems fine to me honestly

[andyleiserson]

@crowlKats I pushed a commit to this PR that mostly implements the last option I described, although I decided that separate helper functions aren't necessary and just put everything in one function (I'd be fine with the helpers too, helpers or not is a toss-up for me on this one). Please take a look.

[crowlKats]

@andyleiserson seems good to me, besides that Cow::Borrowed("foo")

[andyleiserson]

@crowlKats not sure I understand. You are seeing Cow::Borrowed("foo") in the diff for this PR somewhere? I only see it in the snippet I edited in to one of the earlier comments in this thread capturing the previous version of the change, but in the actual diff I think I updated them all with meaningful text.

[crowlKats]

@andyleiserson my bad, seems i had 2 tabs open and accidentally switched to the old one on a second view without notificing. all looks fine now

[cwfitzgerald]

I think we should change wgpu's apis as well - we've done a similar transform where we make the argument type impl Into<Option<u32>> which also accepts naked u32s.

[ErichDonGubler]

LGTM!

[ErichDonGubler]

Looks like you need to fix wgpu's WebGPU backend to make CI happy, but I bet that won't be hard.

[andyleiserson]

The fix for the webgpu backend requires wasm-bindgen changes. I opened a PR for that here: rustwasm/wasm-bindgen#4508

[Wumpf]

is there someone on wasm-bindgen we can ping about this to get it moving?
Otherwise we can also change the vendored bindgen code in here :/

[andyleiserson]

Rebased and added two new commits. I made a small change to the vendor-web-sys so the vendored result is at least reproducible (pulling from my fork of wasm-bindgen on github), but if the opinion is that this is still too much encouragement towards making local changes, I'm happy to undo it.