Closed Bug 1318573 Opened 8 years ago Closed 4 years ago

Refactor Canvas2D API to support a subset for Houdini Paint API

Categories

(Core :: Graphics: Canvas2D, defect, P3)

Product:
Core
Component:
Graphics: Canvas2D
Type:
defect

Tracking

()

Status:
RESOLVED INACTIVE
Tracking Flags:
Tracking Status
firefox53 --- affected

People

(Reporter: bugs, Unassigned)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-needed, Whiteboard: [gfx-noted])

Attachments

(14 files, 6 obsolete files)

Bug 1318573: Explicitly separate canvas interfaces.
58 bytes, text/x-review-board-request
gw280
: review+
ehsan.akhgari
: review+
Details
Bug 1318573 - (Part 1) Move CanvasState to BasicRenderingContext2D.
59 bytes, text/x-review-board-request
Details
Bug 1318573 - (Part 2) Move CanvasTransform to BasicRenderingContext2D.
59 bytes, text/x-review-board-request
Details
Bug 1318573 - (Part 3) Move CanvasCompositing to BasicRenderingContext2D.
59 bytes, text/x-review-board-request
Details
Bug 1318573 - (Part 4) Move CanvasImageSmoothing to BasicRenderingContext2D.
59 bytes, text/x-review-board-request
Details
Bug 1318573 - (Part 5) Move CanvasFillStrokeStyles to BasicRenderingContext2D.
59 bytes, text/x-review-board-request
Details
Bug 1318573 - (Part 6) Move CanvasShadowStyles to BasicRenderingContext2D.
59 bytes, text/x-review-board-request
Details
Bug 1318573 - (Part 7) Move CanvasRect to BasicRenderingContext2D.
59 bytes, text/x-review-board-request
Details
Bug 1318573 - (Part 8) Move CanvasDrawPath to BasicRenderingContext2D.
59 bytes, text/x-review-board-request
Details
Bug 1318573 - (Part 9) Move CanvasPathDrawingStyles to BasicRenderingContext2D.
59 bytes, text/x-review-board-request
Details
Bug 1318573 - (Part 10) Move CanvasPath to BasicRenderingContext2D.
59 bytes, text/x-review-board-request
Details
Bug 1318573 - (Part 11) Move CanvasDrawImage to BasicRenderingContext2D.
59 bytes, text/x-review-board-request
Details
Bug 1318573 - (Part 12) Move EnsureDrawTarget and shutdown observer to BasicRenderingContext2D.
59 bytes, text/x-review-board-request
Details
Bug 1318573 - (Part 13) Decouple filter related features from ContextState.
59 bytes, text/x-review-board-request
Details 
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.
Blocks: houdini-painting
OS: Unspecified → All
Hardware: Unspecified → All
Whiteboard: [gfx-noted]
Flags: needinfo?(aschen)
(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?
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
(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
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 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.
Comment on attachment 8813686 [details]
Bug 1318573: Explicitly separate canvas interfaces.

https://reviewboard.mozilla.org/r/95100/#review98672
Attachment #8813686 - Flags: review+
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 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)
Pushed by msreckovic@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0032dbb19284
Explicitly separate canvas interfaces. r=Ehsan,gw280
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)
These patches have passed the tests on try server, the remaining work is rebasing and naming.
No longer blocks: 1323948
Depends on: 1331857
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
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)
Markus: can we redirect these reviews to you?
Flags: needinfo?(bugs) → needinfo?(mstange)
Sure.
Flags: needinfo?(mstange)
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.
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)
Unassigned myself since I am not actively working on this.
Assignee: kechen → nobody
The leave-open keyword is there and there is no activity for 6 months.
:lsalzman, maybe it's time to close this bug?
Flags: needinfo?(lsalzman)
Markus, can you do decide what to do with this bug? Jet's gone, so I am not sure what the status is with all these Houdini bugs?
Flags: needinfo?(lsalzman)

The leave-open keyword is there and there is no activity for 6 months.
:lsalzman, maybe it's time to close this bug?

Flags: needinfo?(lsalzman)
Flags: needinfo?(lsalzman)

The leave-open keyword is there and there is no activity for 6 months.
:lsalzman, maybe it's time to close this bug?

Flags: needinfo?(lsalzman)
Flags: needinfo?(lsalzman)

The leave-open keyword is there and there is no activity for 6 months.
:lsalzman, maybe it's time to close this bug?

Flags: needinfo?(lsalzman)

Since there has been no inactivity on this bug in years, and it's not clear who is going to take ownership, I am going to mark this as inactive. If someone wants to take it back up, they can reopen the bug.

Status: NEW → RESOLVED
Closed: 4 years ago
Flags: needinfo?(lsalzman)
Resolution: --- → INACTIVE
Flags: needinfo?(mstange)
Attachment #8838452 - Flags: review?(mstange)
Attachment #8838453 - Flags: review?(mstange)
Attachment #8838454 - Flags: review?(mstange)
Attachment #8838455 - Flags: review?(mstange)
Attachment #8842313 - Flags: review?(mstange)
Attachment #8842314 - Flags: review?(mstange)
Attachment #8842315 - Flags: review?(mstange)
Attachment #8842316 - Flags: review?(mstange)
Attachment #8842317 - Flags: review?(mstange)
Attachment #8842318 - Flags: review?(mstange)
Attachment #8842319 - Flags: review?(mstange)
Attachment #8843906 - Flags: review?(mstange)
Attachment #8848016 - Flags: review?(mstange)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: