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)
Core
Graphics: Canvas2D
Tracking
()
RESOLVED
INACTIVE
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)
(deleted),
text/x-review-board-request
|
gw280
:
review+
ehsan.akhgari
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
Details | |
(deleted),
text/x-review-board-request
|
Details | |
(deleted),
text/x-review-board-request
|
Details | |
(deleted),
text/x-review-board-request
|
Details | |
(deleted),
text/x-review-board-request
|
Details | |
(deleted),
text/x-review-board-request
|
Details | |
(deleted),
text/x-review-board-request
|
Details | |
(deleted),
text/x-review-board-request
|
Details | |
(deleted),
text/x-review-board-request
|
Details | |
(deleted),
text/x-review-board-request
|
Details | |
(deleted),
text/x-review-board-request
|
Details | |
(deleted),
text/x-review-board-request
|
Details | |
(deleted),
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.
Reporter | ||
Updated•8 years ago
|
Blocks: houdini-painting
Reporter | ||
Updated•8 years ago
|
OS: Unspecified → All
Hardware: Unspecified → All
Updated•8 years ago
|
Whiteboard: [gfx-noted]
Updated•8 years ago
|
Flags: needinfo?(aschen)
Comment hidden (mozreview-request) |
Updated•8 years ago
|
Keywords: leave-open
(In reply to Milan Sreckovic [:milan] from comment #1) > Created attachment 8813686 [details] > Bug 1318573: Explicitly separate canvas interfaces. > > Review commit: https://reviewboard.mozilla.org/r/95100/diff/#index_header > See other reviews: https://reviewboard.mozilla.org/r/95100/ https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b7666dff7f611e0aa232b1b096e329e2fb89266
(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•8 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•8 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•8 years ago
|
||
As I synced with Kevin, he will work on this to support css-paint-api.
Assignee: howareyou322 → kechen
Comment 7•8 years ago
|
||
(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•8 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
Comment 10•8 years ago
|
||
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.
Updated•8 years ago
|
Attachment #8813686 -
Flags: superreview?(ehsan)
Comment 12•8 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•8 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•8 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•8 years ago
|
||
Pushed by msreckovic@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0032dbb19284 Explicitly separate canvas interfaces. r=Ehsan,gw280
Comment 18•8 years ago
|
||
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 19•8 years ago
|
||
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.
Comment 22•8 years ago
|
||
(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.
Updated•8 years ago
|
Attachment #8820209 -
Flags: feedback?(gwright)
Updated•8 years ago
|
Keywords: dev-doc-needed
Comment 28•8 years ago
|
||
These patches have passed the tests on try server, the remaining work is rebasing and naming.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Updated•8 years ago
|
Attachment #8827360 -
Attachment is obsolete: true
Updated•8 years ago
|
Attachment #8827361 -
Attachment is obsolete: true
Updated•8 years ago
|
Attachment #8827359 -
Attachment is obsolete: true
Updated•8 years ago
|
Attachment #8827362 -
Attachment is obsolete: true
Updated•8 years ago
|
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) |
Comment 52•8 years ago
|
||
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.
Comment 53•8 years ago
|
||
Jet, do you know of a more suitable reviewer for this?
Flags: needinfo?(bugs)
Reporter | ||
Comment 54•8 years ago
|
||
Markus: can we redirect these reviews to you?
Flags: needinfo?(bugs) → 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) |
Comment 68•8 years ago
|
||
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?
Comment 69•8 years ago
|
||
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.
Comment 70•8 years ago
|
||
(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) |
Comment 84•8 years ago
|
||
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)
Updated•7 years ago
|
Priority: -- → P3
Comment 85•7 years ago
|
||
Unassigned myself since I am not actively working on this.
Assignee: kechen → nobody
Comment 86•6 years ago
|
||
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)
Comment 87•6 years ago
|
||
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)
Comment 88•5 years ago
|
||
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)
Updated•5 years ago
|
Flags: needinfo?(lsalzman)
Comment 89•5 years ago
|
||
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)
Updated•5 years ago
|
Flags: needinfo?(lsalzman)
Comment 90•4 years ago
|
||
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)
Comment 91•4 years ago
|
||
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
Updated•4 years ago
|
Keywords: leave-open
Updated•4 years ago
|
Flags: needinfo?(mstange)
Attachment #8838452 -
Flags: review?(mstange)
Updated•4 years ago
|
Attachment #8838453 -
Flags: review?(mstange)
Updated•4 years ago
|
Attachment #8838454 -
Flags: review?(mstange)
Updated•4 years ago
|
Attachment #8838455 -
Flags: review?(mstange)
Updated•4 years ago
|
Attachment #8842313 -
Flags: review?(mstange)
Updated•4 years ago
|
Attachment #8842314 -
Flags: review?(mstange)
Updated•4 years ago
|
Attachment #8842315 -
Flags: review?(mstange)
Updated•4 years ago
|
Attachment #8842316 -
Flags: review?(mstange)
Updated•4 years ago
|
Attachment #8842317 -
Flags: review?(mstange)
Updated•4 years ago
|
Attachment #8842318 -
Flags: review?(mstange)
Updated•4 years ago
|
Attachment #8842319 -
Flags: review?(mstange)
Updated•4 years ago
|
Attachment #8843906 -
Flags: review?(mstange)
Updated•4 years ago
|
Attachment #8848016 -
Flags: review?(mstange)
You need to log in
before you can comment on or make changes to this bug.
Description
•