Make offscreen canvas rendering context use offscreen canvas' size

[jdm]

Spec: https://html.spec.whatwg.org/multipage/canvas.html#dom-offscreencanvas-width

#24464 adds width and height members, but these are duplicated from the ones stored in the associated OffscreenCanvas. We should do the following:

• remove the duplicate members
• remove the Option from OffscreenCanvasRenderingContext2D's canvas member, since there is always an associated canvas
• use the associated canvas' size when necessary
• when OffscreenCanvas's SetWidth or SetHeight methods are called, send a CanvasMsg::Recreate message to the canvas paint thread (see CanvasRenderingContext2D::set_bitmap_dimensions for a model, called from HTMLCanvasElement::recreate_contexts)

@bblanke This would be a good task for your group to work on. It should allow some more tests to pass, too.

[bblanke]

Cool! I'll start looking into it

[bblanke]

Should I also remove the size parameter in the initializer(s) for OffscreenCanvasRenderingContext2D because of the associated canvas already having a height/width?

[bblanke]

Also, given that it's not yet merged, is it possible for me to check out the code from #24464 to make changes, or is it easier to just wait for the merge to occur?

[jdm]

git add remote rasviitanen https://github.com/rasviitanen/servo/
git fetch rasviitanen offscreen-canvas
git checkout rasviitanen/offscreen-canvas
git checkout -b offscreen-canvas-size

That will set your current git head to the head of that branch, using your own local branch with the name offscreen-canvas-size.

[bblanke]

There are a lot of mismatches between Size2D<u64> and Size2D<u32> between offscreencanvasrenderingcontext2D and canvasrenderingcontext2D (the former seems to prefer u64 and the later u32). Should I add failures when a downcast fails, or should I update u32 values to use u64 when necessary?

[jdm]

Either updating them to use u64 or creating new Size2D values with the inner width and height values cast to the appropriate type.

[bblanke]

Okay cool. If I create the new Size2D<u32> objects with values from Size2D<u64>, I'll have to cast them, right? Should I just add error messages if the cast fails?

[jdm]

If it requires casting u64 to u32, we should probably update the CanvasState code to use u64 internally.

[bblanke]

So, converting everything to u64 seems to be quite an extensive undertaking that makes OffscreenCanvas' size implementation inconsistent with that of HTMLCanvasElement's size (which uses u32). While attempting to switch OffscreenCanvasRenderingContext2D to use u64 sizes, it required changing several methods that other objects (like HTMLCanvasElement) relied upon (for example, some in macros.rs and lib.rs).

I propose that we instead change OffscreenCanvas.rs to use u32 sizes to bring it in-line with the implementation of HTMLCanvasElement and CanvasRenderingContext2D.

This requires changing the OffscreenCanvasBinding. If we use u64 on OffscreenCanvasRenderingContext2D, we'd need to change HTMLCanvasElement's bindings (because of shared methods).

Questions:

1. What do you think? Am I making this more complicated than it needs to be?
2. If we go forward with changing bindings, how can I do that? It looks like they're auto-generated.

Here's a link to a branch with the build failing because of the OffscreenCanvas's u64 sizes and OffscreenCanvasRenderingContext2D's u32 sizes:
https://github.com/bblanke/servo/tree/u32-rendering-context-u64-offscreen-canvas

[jdm]

We face a limitation, because OffscreenCanvasBinding is generated automatically from https://github.com/servo/servo/blob/master/components/script/dom/webidls/OffscreenCanvas.webidl, which comes from https://html.spec.whatwg.org/multipage/canvas.html#the-offscreencanvas-interface and explicitly requires unsigned long long types (which turn into u64 in Rust).

Let's take a step back - what are the actual problems that you were running into in #24465 (comment)? I checked and CanvasState already already accepts a u64 size in its constructor, so what sort of mismatches are you talking about? The code in CanvasRenderingContext2D and OffscreenCanvasRenderingContext2D should be distinct, they just share the common use of CanvasState.

[bblanke]

Right! The issue is specifically in GetImageData(), PutImageData(), and PutImageData_() in OffscreenCanvasRenderingContext2D. Those all call corresponding methods in CanvasRenderingContext2D, and in those calls, I'm passing in the associated canvas' size (u64) (GetImageData in OffscreenCanvasRenderingContext2D). The issue is that in CanvasRenderingContext2D, there are calls to actually write/read ImageData, and this has height/width properties that are u32 and are constrained by bindings (GetImageData in CanvasRenderingContext2D) (ImageData).

[jdm]

So, to be pedantic, those methods actually call CanvasState::GetImageData/PutImageData/PutImageData_, which are hand-written and not generated. Is there a problem updating those to accept u64 values, and casting the u32 values used in CanvasRenderingContext2D::GetImageData/PutImageData/PutImageData_? Similarly, ImageData could store u64 values internally and still accept u32 values in the public generated APIs.

[bblanke]

Ah okay–my bad. That seems reasonable. I'll try and modify ImageData! I may need to modify a few more methods as well to accept u64 (associated with rectangle manipulation) but that shouldn't be a problem

[bblanke]

Alright; on the final stretch here.

I have the types switched over successfully and am now working on the final task:

"when OffscreenCanvas's SetWidth or SetHeight methods are called, send a CanvasMsg::Recreate message to the canvas paint thread (see CanvasRenderingContext2D::set_bitmap_dimensions for a model, called from HTMLCanvasElement::recreate_contexts)"

I'm working on adding the necessary methods, and it seems pretty straightforward, but I have a question.

In canvasrenderingcontext2d.rs, CanvasRenderingContext2D is able to access the members of CanvasState (stored in its own canvas_state member). This allows it to provide functions like CanvasRenderingContext2D::set_bitmap_dimensions. OffscreenCanvasRenderingContext2D is not able to access the internal members of CanvasState. For example, I get the compiler output:

error[E0616]: field `saved_states` of struct `dom::canvasrenderingcontext2d::CanvasState` is private
  --> components/script/dom/offscreencanvasrenderingcontext2d.rs:86:9
   |
86 |         self.canvas_state.borrow().saved_states.borrow_mut().clear();
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

I did some searching to figure out why this is the case, but everything I've read seems to point to the idea that structs' members are private unless explicitly marked pub. How, then, can CanvasRenderingContext2D access these? And can I obtain similar access through OffscreenCanvasRenderingContext2D without implementing public methods on CanvasState?

[jdm]

We either need to make the members public or add a method that encapsulates the operation on the private members. I think adding a clear_saved_states method is the best way forward. Then it can be used by the 2d canvas code in #24496 as well.

[jdm]

Oh, and the 2d canvas code can access them because it is currently in the same module as CanvasState (because they're in the same file), and privacy exists at the module boundary.