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

Reset context on resurface #1293

Merged
merged 1 commit into from
Nov 20, 2018
Merged

Reset context on resurface #1293

merged 1 commit into from
Nov 20, 2018

Conversation

Hakerh400
Copy link
Contributor

@Hakerh400 Hakerh400 commented Oct 27, 2018

Fixes #1292
Did a lot of refactoring.
If any of these is better to stay as it was implemented before, I can revert it:

  • Hid some non-standard context prototype accessors
  • Replaced deprecated __defineGetter__ with defineProperty
  • Added new private method _updateStateInfo to track when the surface gets reset
  • Hid lastFillStyle and similar properties
  • Reduced some code duplication

  • Updated the changelog
  • Added a test

lib/context2d.js Outdated Show resolved Hide resolved
lib/context2d.js Outdated Show resolved Hide resolved
@zbjornson
Copy link
Collaborator

Really like the direction this is headed 👍. Hope to read it more closely tonight.

Copy link
Collaborator

@zbjornson zbjornson left a comment

Choose a reason for hiding this comment

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

Looks great! Sorry for being slow to review. Just a few little bits of suggested cleanup, but otherwise good with me to land!

lib/context2d.js Outdated Show resolved Hide resolved
src/Canvas.cc Outdated Show resolved Hide resolved
src/CanvasRenderingContext2d.cc Outdated Show resolved Hide resolved
src/CanvasRenderingContext2d.cc Outdated Show resolved Hide resolved
src/CanvasRenderingContext2d.cc Outdated Show resolved Hide resolved
src/CanvasRenderingContext2d.cc Outdated Show resolved Hide resolved
src/CanvasRenderingContext2d.cc Outdated Show resolved Hide resolved
src/CanvasRenderingContext2d.cc Outdated Show resolved Hide resolved
src/CanvasRenderingContext2d.cc Outdated Show resolved Hide resolved
src/CanvasRenderingContext2d.cc Outdated Show resolved Hide resolved
@Hakerh400
Copy link
Contributor Author

Hakerh400 commented Nov 16, 2018

Added some commits and rebased onto master. I'll add createImageData, but want to make sure everything so far is ok before adding it.

Checking if one v8::Value is an instance of another v8::Value doesn't seem trivial, unless I'm missing something. It can be done by constructing a new function using global.Function constructor that performs JS instanceof operator on the arguments and returns the result, but it is very slow.

Two questions: the BMP PR added a line into the changelog section for 2.1.0, but it landed after 2.1.0, right? I can move it in the unreleased section, if that is the case. Another question: tests registerFont and Context2d#measureText().width are flaky and fail randomly (see this, this, this, this (before restart), and this). Should we increase their timeout? If yes, let me know, all of that can be done in this PR.

@zbjornson
Copy link
Collaborator

the BMP PR added a line into the changelog section for 2.1.0, but it landed after 2.1.0, right?

Yeah, appears so. Thanks for finding.

tests registerFont and Context2d#measureText().width are flaky and fail randomly

I don't think increasing the timeout is the right thing to do (they shouldn't take more than 2000 ms, unless the host has some I/O issue with slow font loading?). I've only seen them be flaky in CI, not locally. Not sure what the answer is there. I guess we could try increasing the timeout in a branch and run CI a bunch of times.

Copy link
Collaborator

@zbjornson zbjornson left a comment

Choose a reason for hiding this comment

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

Nice work!


int stride = canvas->stride();
double Bpp = static_cast<double>(stride) / canvas->getWidth();
int nBytes = static_cast<int>(Bpp * width * height + .5);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like this also fixes a bug in the original JS impl. 👍

CHANGELOG.md Show resolved Hide resolved
lib/context2d.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@LinusU LinusU left a comment

Choose a reason for hiding this comment

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

This looks great!! Thank you for the huge improvement 👏

I've left some small comments, but they don't have to block the merge, I can fix them later if you would prefer that 🎉

package.json Outdated Show resolved Hide resolved
@LinusU
Copy link
Collaborator

LinusU commented Nov 19, 2018

Awesome 👏

@LinusU
Copy link
Collaborator

LinusU commented Nov 20, 2018

Thank you for this 🚀

@LinusU LinusU merged commit d54fa34 into Automattic:master Nov 20, 2018
@Hakerh400 Hakerh400 deleted the resetctx branch December 3, 2018 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants