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

Canvas: implement context state save/restore. #5731

Merged
merged 1 commit into from Apr 19, 2015

Conversation

mmatyas
Copy link
Contributor

@mmatyas mmatyas commented Apr 17, 2015

This patch enables the use of save() and restore() for the canvas context, which is used by a lot of sites and scripts.

Depends on servo/rust-azure#153.

Review on Reviewable

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Apr 17, 2015
@hoppipolla-critic-bot
Copy link

Critic review: https://critic.hoppipolla.co.uk/r/4710

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

@jdm
Copy link
Member

jdm commented Apr 17, 2015

Reviewed files:

  • components/canvas/canvas_paint_task.rs @ r1
  • components/script/dom/canvasrenderingcontext2d.rs @ r2
  • components/script/dom/webidls/CanvasRenderingContext2D.webidl @ r1
  • tests/wpt/metadata/2dcontext/line-styles/2d.line.width.transformed.html.ini @ r1
  • tests/wpt/metadata/2dcontext/the-canvas-state/2d.state.saverestore.bitmap.html.ini @ r1
  • tests/wpt/metadata/2dcontext/the-canvas-state/2d.state.saverestore.fillStyle.html.ini @ r1
  • tests/wpt/metadata/2dcontext/the-canvas-state/2d.state.saverestore.globalAlpha.html.ini @ r1
  • tests/wpt/metadata/2dcontext/the-canvas-state/2d.state.saverestore.lineCap.html.ini @ r1
  • tests/wpt/metadata/2dcontext/the-canvas-state/2d.state.saverestore.lineJoin.html.ini @ r1
  • tests/wpt/metadata/2dcontext/the-canvas-state/2d.state.saverestore.lineWidth.html.ini @ r1
  • tests/wpt/metadata/2dcontext/the-canvas-state/2d.state.saverestore.miterLimit.html.ini @ r1
  • tests/wpt/metadata/2dcontext/the-canvas-state/2d.state.saverestore.stack.html.ini @ r1
  • tests/wpt/metadata/2dcontext/the-canvas-state/2d.state.saverestore.stackdepth.html.ini @ r1
  • tests/wpt/metadata/2dcontext/the-canvas-state/2d.state.saverestore.strokeStyle.html.ini @ r1
  • tests/wpt/metadata/2dcontext/the-canvas-state/2d.state.saverestore.transformation.html.ini @ r1
  • tests/wpt/metadata/2dcontext/the-canvas-state/2d.state.saverestore.underflow.html.ini @ r1
  • tests/wpt/metadata/2dcontext/transformations/2d.transformation.scale.negative.html.ini @ r1
  • tests/wpt/metadata/html/dom/interfaces.html.ini @ r1

components/script/dom/canvasrenderingcontext2d.rs, line 347 [r2] (raw file):
Where's this defined?


components/script/dom/canvasrenderingcontext2d.rs, line 387 [r2] (raw file):
nit: indentation


components/script/dom/canvasrenderingcontext2d.rs, line 885 [r2] (raw file):
Why &?


components/script/dom/canvasrenderingcontext2d.rs, line 901 [r2] (raw file):
Why &?


components/script/dom/canvasrenderingcontext2d.rs, line 919 [r2] (raw file):
Why &?


components/script/dom/canvasrenderingcontext2d.rs, line 937 [r2] (raw file):
Why &?



Comments from the review on Reviewable.io

@jdm jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Apr 17, 2015
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Apr 17, 2015
@mmatyas
Copy link
Contributor Author

mmatyas commented Apr 17, 2015

components/script/dom/canvasrenderingcontext2d.rs, line 347 [r2] (raw file):
This line sets the current state to the last one in the stack. Do you know a better way to do this?


components/script/dom/canvasrenderingcontext2d.rs, line 885 [r2] (raw file):
Whoops, fixed.



Comments from the review on Reviewable.io

@jdm
Copy link
Member

jdm commented Apr 17, 2015

Reviewed files:

  • components/script/dom/canvasrenderingcontext2d.rs @ r3

components/script/dom/canvasrenderingcontext2d.rs, line 347 [r2] (raw file):
I mean where is clone_from defined? I don't see it anywhere in this pull request.



Comments from the review on Reviewable.io

@jdm jdm added S-awaiting-answer Someone asked a question that requires an answer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Apr 17, 2015
@mmatyas
Copy link
Contributor Author

mmatyas commented Apr 18, 2015

components/script/dom/canvasrenderingcontext2d.rs, line 347 [r2] (raw file):
Ah, that comes with #[derive(Clone)], at the definition of struct CanvasContextState.



Comments from the review on Reviewable.io

@jdm jdm added S-needs-squash Some (or all) of the commits in the PR should be combined. and removed S-awaiting-answer Someone asked a question that requires an answer. labels Apr 18, 2015
@jdm
Copy link
Member

jdm commented Apr 18, 2015

Ok! Go ahead and squash, and use the appropriate update-cargo command to grab the rust-azure changes.

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Apr 18, 2015
@mmatyas
Copy link
Contributor Author

mmatyas commented Apr 18, 2015

Ok, squashed this and servo/rust-azure#153 too, I'll update the lock files as soon as it gets merged.

@jdm jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-squash Some (or all) of the commits in the PR should be combined. labels Apr 19, 2015
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Apr 19, 2015
@mmatyas
Copy link
Contributor Author

mmatyas commented Apr 19, 2015

Updated & rebased.

@jdm
Copy link
Member

jdm commented Apr 19, 2015

@bors-servo: r+

@bors-servo
Copy link
Contributor

📌 Commit 5b8416a has been approved by jdm

@bors-servo
Copy link
Contributor

⌛ Testing commit 5b8416a with merge 9c7c289...

bors-servo pushed a commit that referenced this pull request Apr 19, 2015
This patch enables the use of `save()` and `restore()` for the canvas context, which is used by *a lot* of sites and scripts.

Depends on servo/rust-azure#153.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/5731)
<!-- Reviewable:end -->
@jdm jdm added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Apr 19, 2015
@bors-servo
Copy link
Contributor

☀️ Test successful - android, gonk, linux1, linux2, mac1, mac2

@bors-servo bors-servo merged commit 5b8416a into servo:master Apr 19, 2015
@mmatyas mmatyas deleted the canvas_saverestore branch November 14, 2016 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants