Refactor Canvas2D API to support a subset for Houdini Paint API

NEW
Unassigned

Status

()

Core
Canvas: 2D
P3
normal
2 years ago
5 months ago

People

(Reporter: jet, Unassigned, NeedInfo)

Tracking

(Blocks: 2 bugs, {dev-doc-needed, leave-open})

Trunk
dev-doc-needed, leave-open
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 affected)

Details

(Whiteboard: [gfx-noted])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(14 attachments, 6 obsolete attachments)

58 bytes, text/x-review-board-request
gw280
: review+
Ehsan
: review+
Details | Review
59 bytes, text/x-review-board-request
kechen
: review?
mstange
Details | Review
59 bytes, text/x-review-board-request
kechen
: review?
mstange
Details | Review
59 bytes, text/x-review-board-request
kechen
: review?
mstange
Details | Review
59 bytes, text/x-review-board-request
kechen
: review?
mstange
Details | Review
59 bytes, text/x-review-board-request
kechen
: review?
mstange
Details | Review
59 bytes, text/x-review-board-request
kechen
: review?
mstange
Details | Review
59 bytes, text/x-review-board-request
kechen
: review?
mstange
Details | Review
59 bytes, text/x-review-board-request
kechen
: review?
mstange
Details | Review
59 bytes, text/x-review-board-request
kechen
: review?
mstange
Details | Review
59 bytes, text/x-review-board-request
kechen
: review?
mstange
Details | Review
59 bytes, text/x-review-board-request
kechen
: review?
mstange
Details | Review
59 bytes, text/x-review-board-request
kechen
: review?
mstange
Details | Review
59 bytes, text/x-review-board-request
kechen
: review?
mstange
Details | Review
(Reporter)

Description

2 years ago
Compare the Houdini API exposed here:
https://drafts.css-houdini.org/css-paint-api-1/#2d-rendering-context

...with the Canvas2D API exposed here:
https://html.spec.whatwg.org/multipage/scripting.html#canvasrenderingcontext2d

We should refactor this so that one is a true subclass of the other, enabling both API's to coexist with minimal code duplication.
(Reporter)

Updated

2 years ago
Blocks: 1302328
(Reporter)

Updated

2 years ago
OS: Unspecified → All
Hardware: Unspecified → All

Updated

2 years ago
Whiteboard: [gfx-noted]

Updated

2 years ago
Flags: needinfo?(aschen)
Comment hidden (mozreview-request)
Keywords: leave-open
(In reply to Jet Villegas (:jet) from comment #0)
> ...
> 
> We should refactor this so that one is a true subclass of the other,
> enabling both API's to coexist with minimal code duplication.

Maybe a common base class instead?

Updated

2 years ago
Assignee: nobody → milan
Flags: needinfo?(aschen)
I'll happily do patches along the way, but it's probably not wise to assume I'll do it all, so I'll leave it unassigned.
Assignee: milan → nobody

Comment 5

2 years ago
(In reply to Milan Sreckovic [:milan] from comment #4)
> I'll happily do patches along the way, but it's probably not wise to assume
> I'll do it all, so I'll leave it unassigned.

Assign to me before I find the owner to follow up this.
Assignee: nobody → howareyou322

Comment 6

2 years ago
As I synced with Kevin, he will work on this to support css-paint-api.
Assignee: howareyou322 → kechen
(In reply to Peter Chang[:pchang] from comment #6)
> As I synced with Kevin, he will work on this to support css-paint-api.

Thanks for your support, Peter.

Comment 8

2 years ago
mozreview-review
Comment on attachment 8813686 [details]
Bug 1318573: Explicitly separate canvas interfaces.

https://reviewboard.mozilla.org/r/95100/#review96018

I like
Attachment #8813686 - Flags: review?(gwright) → review+
The autolander is having trouble with the reviewed patch, I don't want to mess something up, so checkin-needed.
Keywords: checkin-needed
Per the Autoland error message (hover over it in MozReview), the WebIDL changes need review from a DOM peer before this can land.
Keywords: checkin-needed
Hover!  Not a discoverable UI :)  I'll get a DOM peer review, although this is just rearranging the code - there is no actual change to the WebIDL.
Attachment #8813686 - Flags: superreview?(ehsan)

Comment 12

2 years ago
mozreview-review
Comment on attachment 8813686 [details]
Bug 1318573: Explicitly separate canvas interfaces.

https://reviewboard.mozilla.org/r/95100/#review98672
Attachment #8813686 - Flags: review+

Comment 13

2 years ago
Milan, an intent to implement for the Houdini Paint API was never sent.  Can you please ask the team working on the implementation to do so?  Thanks!:
Flags: needinfo?(milan)

Comment 14

2 years ago
Comment on attachment 8813686 [details]
Bug 1318573: Explicitly separate canvas interfaces.

Not sure why MozReview left this flag.
Attachment #8813686 - Flags: superreview?(ehsan)
(In reply to :Ehsan Akhgari from comment #13)
> Milan, an intent to implement for the Houdini Paint API was never sent.  Can
> you please ask the team working on the implementation to do so?  Thanks!:

Good idea; I put a note in bug 1302328.
Flags: needinfo?(milan)

Comment 16

2 years ago
Pushed by msreckovic@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0032dbb19284
Explicitly separate canvas interfaces. r=Ehsan,gw280
Created attachment 8820209 [details] [diff] [review]
[WIP] Add PaintRenderingContext2D interface.

Hello Jet,

In this patch, I add a PaintRenderingContext2D interface in PaintRenderingContext2D.webidl.

And to expose the PaintRenderingContext2D interface to PaintWorklet, I have to add a PaintWorklet identifier to the exposed attribute in the interface PaintRenderingContext2D should implement which is not defined in current living standard[1].
Is it appropriate to add the identifier now ? Or should I wait for the standard change ?

Since the functions of PaintRenderingContext2D is the subset of CanvasRenderingContext2D's function, I plan to make a new class (maybe 'BaseRenderingContext2D') to be the parent class of both two or make CanvasRenderingContext2D inherit from PaintRenderingContext2D directly.
I will leave this part in the following bug.

Thank you.

[1] https://html.spec.whatwg.org/multipage/scripting.html#canvasstate
Attachment #8820209 - Flags: feedback?(bugs)
Comment on attachment 8820209 [details] [diff] [review]
[WIP] Add PaintRenderingContext2D interface.

Forward this feedback to George.
Hello George can you help me to take look at comment 18.

Thank you.
Attachment #8820209 - Flags: feedback?(bugs) → feedback?(gwright)
Comment on attachment 8820209 [details] [diff] [review]
[WIP] Add PaintRenderingContext2D interface.

Review of attachment 8820209 [details] [diff] [review]:
-----------------------------------------------------------------

Was there supposed to be a PaintRenderingContext2D.cpp file as well?
Overall, what I would like to see done here:
* Put together PaintRenderingContext2D (as you've started) that makes sense, given the changing standard.
* Extract the base class(es) as appropriate between it and CanvasRenderingContext2D.
* Make Paint... and Canvas... use that base class.
* Land the base class and Canvas... changes.  That's then an actual refactoring, as you're not adding any new functionality.  That would land in this bug.
* Later (when we feel the standard is ready, I'll leave that to others to sort out) land Paint..., probably in a different bug.

It makes sense what you're doing - seeing what Paint... would look like before you can see what the base class for them would be, but I would land the refactoring separately from the Paint... classes, at different times, and in different bugs.
(In reply to Kevin Chen[:kechen] (UTC + 8) from comment #18)
> And to expose the PaintRenderingContext2D interface to PaintWorklet, I have
> to add a PaintWorklet identifier to the exposed attribute in the interface
> PaintRenderingContext2D should implement which is not defined in current
> living standard[1].
> Is it appropriate to add the identifier now ? Or should I wait for the
> standard change ?

I don't see any problem with this, but I've not worked on draft standards before so you may want to ask someone who has. 

> Since the functions of PaintRenderingContext2D is the subset of
> CanvasRenderingContext2D's function, I plan to make a new class (maybe
> 'BaseRenderingContext2D') to be the parent class of both two or make
> CanvasRenderingContext2D inherit from PaintRenderingContext2D directly.
> I will leave this part in the following bug.

I'd rather we created the base class and made CRC2D and PRC2D subclasses now instead of refactoring later.
Attachment #8820209 - Flags: feedback?(gwright)
Keywords: dev-doc-needed
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
These patches have passed the tests on try server, the remaining work is rebasing and naming.
Blocks: 1324747
No longer blocks: 1323948
No longer blocks: 1324747
Depends on: 1331857
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8827360 - Attachment is obsolete: true
Attachment #8827361 - Attachment is obsolete: true
Attachment #8827359 - Attachment is obsolete: true
Attachment #8827362 - Attachment is obsolete: true
Attachment #8827363 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
I'm not all that comfortable reviewing this as I don't know that much about the new houdini stuff. You may want to find someone else to review this.

I am happy to be a secondary reviewer though.
Jet, do you know of a more suitable reviewer for this?
Flags: needinfo?(bugs)
(Reporter)

Comment 54

a year ago
Markus: can we redirect these reviews to you?
Flags: needinfo?(bugs) → needinfo?(mstange)
Sure.
Flags: needinfo?(mstange)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
I had a look at the patches and I think this is mostly fine. I am not going to review them line by line.

It might have been easier to rename CanvasRenderingContext2D to BasicRenderingContext2D and then extract out just the stuff that isn't shared, instead of moving 80% over. But I'm not going to ask you to do this again, I'll just review the result that comes out at the end. :)
And thanks for splitting the patches like this; I am not sure how useful it is to have them separate like that, though, because I think we should focus most our attention on the things that you *didn't* move. I do appreciate the effort you put into splitting them up, though.

I have a few questions.

You moved some of the filter stuff, but not all of it; e.g. UpdateFilter is still in CanvasRenderingContext2D. How did you decide where to draw the line?
Does BasicRenderingContext2D eventually need to be able to run on non-main threads? In that case, filters should probably stay completely in CanvasRenderingContext2D because we can't render SVG filters on non-main threads - we need the SVG DOM for them.
Why did you choose to leave CanvasPattern, CanvasGradient and CanvasPath implementations in CanvasRenderingContext2D?

I also didn't review the cycle collection changes. Are all of them just moved code, or did you have to write something new?
Oh, and are there any methods remaining in BasicRenderingContext2D that do not need to be overridden by the subclasses and which are still virtual, after these patches? In that case you should make them non-virtual.
(In reply to Markus Stange [:mstange] from comment #68)
> I had a look at the patches and I think this is mostly fine. I am not going
> to review them line by line.
> 
> It might have been easier to rename CanvasRenderingContext2D to
> BasicRenderingContext2D and then extract out just the stuff that isn't
> shared, instead of moving 80% over. But I'm not going to ask you to do this
> again, I'll just review the result that comes out at the end. :)
> And thanks for splitting the patches like this; I am not sure how useful it
> is to have them separate like that, though, because I think we should focus
> most our attention on the things that you *didn't* move. I do appreciate the
> effort you put into splitting them up, though.
> 
> I have a few questions.
> 
> You moved some of the filter stuff, but not all of it; e.g. UpdateFilter is
> still in CanvasRenderingContext2D. How did you decide where to draw the line?
> Does BasicRenderingContext2D eventually need to be able to run on non-main
> threads? In that case, filters should probably stay completely in
> CanvasRenderingContext2D because we can't render SVG filters on non-main
> threads - we need the SVG DOM for them.
Yes, it should be able to run on non-main thread and BasicRenderingContex2D do not need filter-related code.
The original idea of moving part of filter code to BasicRenderingContex2D was to prevent virtual functions and duplicated code; but since it confuses people I should decouple them from BasicRenderingContext2D.


> Why did you choose to leave CanvasPattern, CanvasGradient and CanvasPath
> implementations in CanvasRenderingContext2D?
> 
Sorry, I failed to notice that, I will fix it in the next version.

> I also didn't review the cycle collection changes. Are all of them just
> moved code, or did you have to write something new?
No big change here, just moving the code.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Hello Markus,
I updated my patches according to comment 68 and comment 69, could you help me to review the patches ?

The ContextState element in mStyleStack is modified as pointer so that I can reuse the code and the reason that I do not make BasicRenderingContext2D a template for mStyleStack is that there are some static variables in BasicRenderingContext2D and I want them to be shared.

I will do the rebase work after these patches are good.

Thank you.
Flags: needinfo?(mstange)
Priority: -- → P3
Unassigned myself since I am not actively working on this.
Assignee: kechen → nobody
You need to log in before you can comment on or make changes to this bug.