Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bitmap rendering Context for OffscreenCanvas #4455

Merged
merged 15 commits into from
May 14, 2019

Conversation

fserb
Copy link
Contributor

@fserb fserb commented Mar 27, 2019

This change adds bitmaprenderingcontext to OffscreenCanvas.
The behaviour of the context is the same, just extending it to Offscreen.

@whatwg/canvas @Juanmihd


💥 Error: HTTP Error: 404 Not Found 💥

PR Preview failed to build. (Last tried on Apr 5, 2019, 3:08 PM UTC).

More

🔗 Related URL

If the error is not surfaced properly, please file an issue.

@Juanmihd
Copy link

Addressed fserb comments; pull request ready to be reviewed.

@annevk annevk added addition/proposal New features or enhancements topic: canvas labels Mar 28, 2019
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this! We'll also need:

  1. Implementer support.
  2. Tests.

source Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@fserb
Copy link
Contributor Author

fserb commented Mar 28, 2019

Anne, we are working on the tests.

For support, can you help me get some Firefox folks on the thread?
I was hoping it was a small enough discussion, that we could have it here. But I can set up an issue for that too, wdyt?

@annevk
Copy link
Member

annevk commented Mar 28, 2019

It's fine here; @whatwg/canvas please indicate whether or not you support bitmap rendering context support for OffscreenCanvas. (Pinging WHATWG teams from OP does not work due to our bot unfortunately.)

@annevk annevk added needs implementer interest Moving the issue forward requires implementers to express interest needs tests Moving the issue forward requires someone to write tests labels Mar 28, 2019
@Juanmihd
Copy link

Juanmihd commented Apr 1, 2019

The tests are done!
web-platform-tests/wpt#16032

@fserb
Copy link
Contributor Author

fserb commented Apr 3, 2019

ping @whatwg/canvas ?

source Show resolved Hide resolved
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a couple minor things left still here.

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
@Juanmihd
Copy link

Juanmihd commented Apr 5, 2019

Hey @annevk, I've addressed the comments, and I've found out that I was missing one place to correctly add the bitmaprenderer option, so I've added those changes also.

Thanks againg for taking the time to review this changes!

@Juanmihd
Copy link

Juanmihd commented Apr 8, 2019

Hey @annevk, thanks again for all the reviews. This is my first time editing the specs and I have lots to learn. You're reviews are being very helpful for that.

I've reverted the explanation of the canvas getter, after what you commented and reading carefully the document, I think the original definition of the getter is correct both for "normal" canvas and an offscreen canvas.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, hopefully final nits.

source Show resolved Hide resolved
source Show resolved Hide resolved
@annevk
Copy link
Member

annevk commented Apr 9, 2019

@Juanmihd I'm not sure where the correct place is to give feedback on tests, given the automatically exported PR, so I'll do it here. It would be really good to have some test coverage for the origin-clean flag. In particular, put bitmaps on the context that are not origin-clean and then see that convertToBlob() throws, and that drawing the canvas on another canvas and then exporting somehow fails, etc.

@sideshowbarker sideshowbarker added the impacts documentation Used by documentation communities, such as MDN, to track changes that impact documentation label Apr 13, 2019
@sideshowbarker
Copy link
Contributor

heads-up @whatwg/documentation

@chrisdavidmills
Copy link

Recorded potential documentation need ar https://trello.com/c/N0hJJV95/129-2d-canvas-api

@Juanmihd
Copy link

@annevk I've added a new test to validate the origin-clean flag functionality with convertToBlob.

Do you have any other comments regarding the specs?

@annevk
Copy link
Member

annevk commented Apr 18, 2019

Do you have a link to the test?

@Juanmihd
Copy link

Are there any other comments to address before finalizing the review?

@annevk
Copy link
Member

annevk commented Apr 30, 2019

@Juanmihd thanks; though you want to use promise_rejects (part of the test API). (We should figure out a way to do test review better across repositories. I'll talk to @foolip about that.)

@annevk annevk removed the needs tests Moving the issue forward requires someone to write tests label Apr 30, 2019
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I think we're still lacking these things

  • Implementer interest
  • Implementation bugs

right? Otherwise this looks good to me.

(@musgravejw you want to keep this in mind as this might end up clashing with your refactoring effort in #4565 or the other way around.)

@Juanmihd
Copy link

Test changed to use promise_rejects. https://github.com/web-platform-tests/wpt/pull/16032/files#diff-49891c9a9321f46e9402869e95b45261

Thanks @annevk.

I'll create now the Implementation Bugs.

@Juanmihd
Copy link

@Juanmihd
Copy link

@Juanmihd
Copy link

Juanmihd commented May 7, 2019

ping @whatwg/canvas? any comment to get the Implementer Interest?

@Juanmihd
Copy link

Juanmihd commented May 7, 2019

@annevk Can you help me pinging @whatwg/canvas? I think I'm doing it wrong.

@fserb
Copy link
Contributor Author

fserb commented May 8, 2019

Btw, this will soon be used by WebGPU as the main way of rendering context to screen: kainino0x/gpuweb#4

@annevk
Copy link
Member

annevk commented May 9, 2019

@Juanmihd they are already copied so should be getting emails either way. Might help to reach out to folks directly. For Mozilla you can also use https://github.com/mozilla/standards-positions. I'll add you to the WHATWG organization so you can ping teams in future issues. Do you also want to join the canvas team?

@grorg
Copy link
Contributor

grorg commented May 10, 2019

WebKit would like to implement this.

@domenic domenic removed the needs implementer interest Moving the issue forward requires implementers to express interest label May 10, 2019
@Juanmihd
Copy link

@annevk Thanks for adding me to the whatwg organization! Yes, I'd like to join the canvas team, as it is my primary work here at Google.

And given that the implementers have shown interest, is there anything else to do prior to merging this change?

@annevk annevk merged commit 8433bfc into whatwg:master May 14, 2019
@annevk
Copy link
Member

annevk commented May 14, 2019

No, all good (and you're part of the team). Thanks all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements impacts documentation Used by documentation communities, such as MDN, to track changes that impact documentation topic: canvas
Development

Successfully merging this pull request may close these issues.

7 participants