Remove Cell/DomRefCell member wrappers from CanvasState struct

[jdm]

CanvasState was introduced as part of a big refactor in #23381 that extracted common aspects of the offscreen canvas rendering context and the 2d canvas rendering context. Since the CanvasState struct is stored inside of a DomRefCell itself, that means that we can modify the methods for CanvasState to accept &self or &mut self directly, so the Cell and DomRefCell members inside of CanvasState are redundant and can be removed.

Code: components/script/dom/canvasrenderingcontext2d.rs

To verify that everything behaves the same after changing the code, run ./mach test-wpt tests/wpt/web-platform-tests/2dcontext/ tests/wpt/web-platform-tests/offscreen-canvas.

[highfive]

Hi! If you have any questions regarding this issue, feel free to make a comment here, or ask it in the #servo channel in IRC.

If you intend to work on this issue, then add @highfive: assign me to your comment, and I'll assign this to you. 😄

[chansuke]

@highfive: assign me

[highfive]

Hey @chansuke! Thanks for your interest in working on this issue. It's now assigned to you!

[jdm]

#23381 has not actually been able to merge yet, so this will have to wait.

[jdm]

#23381 has merged, so this can be worked on now.

[chansuke]

@jdm Thanks! I will be going to start wokring on this:)

[chansuke]

@jdm Hi, I've removed the wrapper in CanvasState struct(to state: CanvasContextState) at first and updating like below
let transform = self.state.borrow().transform; ⇒ let transform = self.state.transform;
but I'm stucking with this line(L:1467),
*self.canvas_state.borrow().state.borrow_mut() = CanvasContextState::new();.
Could you give me a suggestion to how to handle this? Thanks.

[jdm]

self.canvas_state.borrow_mut().state = CanvasContextState::new() should do what you want.

[utsavoza]

Hey @jdm, I see that code concerning the issue has changed quite a bit since this both the issue and its PR (#23730) was created. Based on the current implementation of CanvasState, I believe we should rather remove the DomRefCell wrapper for CanvasState since we are currently borrowing it immutably in both CanvasRenderingContext2D and OffscreenCanvasRenderingContext2D (unless we intend to borrow it mutably in the future?). Is there any specific reason I am missing why we should instead opt to remove Cell/DomRefCell member wrappers from CanvasState?

[jdm]

@utsavoza In general I find methods that are &self or &mut self as needed on a struct are easier to read than those same methods performing lots of borrow/borrow_mut calls on their members. If we keep the DomRefCell wrapper around CanvasState and remove the per-member wrappers, that will yield easier to read code in my opinion.

[utsavoza]

I agree. I did actually try to remove the Cell/DomRefCell member wrappers from CanvasState struct. The issue I saw with that is there will be some public methods in CanvasState that will now accept &mut self instead of &self.

For example: draw_image method will now cause canvas_state to be borrowed mutably in CanvasRenderingContext2D. This is a specific example where a wpt test failed with:

  │    9: script::dom::canvasrenderingcontext2d::CanvasRenderingContext2D::send_canvas_2d_msg
  │   10: script::canvas_state::CanvasState::draw_image_internal
  │   11: script::canvas_state::CanvasState::draw_image
  │   12: <std::panic::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once
  │   13: script::dom::bindings::codegen::Bindings::CanvasRenderingContext2DBinding::CanvasRenderingContext2DBinding::drawImage

From the stacktrace, I am assuming that while mutably borrowing canvas_state in CanvasRenderingContext2D::DrawImage, we are trying to borrow canvas_state again via:

• CanvasState::draw_image
• CanvasState::draw_image_internal
• CanvasRenderingContext2D::context.send_canvas_2d_msg

which then causes the wpt to fail.

There are few other similar scenarios that are resulting in failing wpt tests, which is why I considered removing DomRefCell wrapper for CanvasState itself.

[jdm]

From reading the code (since draw_image_internal does not directly invoke send_canvas_2d_msg), it looks like that either comes from CanvasState::draw_html_canvas_element or CanvasState::draw_offscreen_canvas. I think we should only end up with a borrow problem in the case where we try to draw an html canvas inside of itself. One option to avoid that would be to pass the source canvas's CanvasId as an argument to draw_html_canvas_element and call this code with the source canvasid instead. That would avoid having to go through CanvasRenderingContext2D::send_canvas_2d_message.

[jdm]

That being said, if the resulting code is just working around a couple edge cases, you might be right that it will be easier to read and write if we keep the member wrappers instead of the overall CanvasState wrapper. Use your best judgement!