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

Add attributes for canvas shadows. #6337

Merged
merged 1 commit into from Jun 12, 2015
Merged

Add attributes for canvas shadows. #6337

merged 1 commit into from Jun 12, 2015

Conversation

@hyowon
Copy link
Contributor

hyowon commented Jun 11, 2015

The first step of the implementation for shadows in canvas.
r? @nox @jdm
cc @yichoi

Review on Reviewable

@highfive
Copy link

highfive commented Jun 11, 2015

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @mbrubeck (or someone else) soon.

@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented Jun 11, 2015

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

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.

@nox
Copy link
Member

nox commented Jun 11, 2015

Isn't it weird to just support the attributes but not actually do anything with them? Is the next step more significant to implement and that's why you submitted that part for now?

Apart from that and a few nits, I'll happily r+ it.

-S-awaiting-review +S-awaiting-answer +S-needs-code-changes


Review status: all files reviewed, 9 unresolved discussions, all commit checks successful.
Reviewed files:

  • components/canvas/canvas_paint_task.rs @ r1
  • components/canvas_traits/lib.rs @ r1
  • components/script/Cargo.toml @ r1
  • components/script/dom/canvasrenderingcontext2d.rs @ r1
  • components/script/dom/webidls/CanvasRenderingContext2D.webidl @ r1
  • components/script/lib.rs @ r1
  • tests/wpt/metadata/2dcontext/shadows/2d.shadow.attributes.shadowBlur.initial.html.ini @ r1
  • tests/wpt/metadata/2dcontext/shadows/2d.shadow.attributes.shadowColor.initial.html.ini @ r1
  • tests/wpt/metadata/2dcontext/shadows/2d.shadow.attributes.shadowColor.valid.html.ini @ r1
  • tests/wpt/metadata/2dcontext/shadows/2d.shadow.attributes.shadowOffset.initial.html.ini @ r1
  • tests/wpt/metadata/2dcontext/the-canvas-state/2d.state.saverestore.shadowBlur.html.ini @ r1
  • tests/wpt/metadata/2dcontext/the-canvas-state/2d.state.saverestore.shadowColor.html.ini @ r1
  • tests/wpt/metadata/2dcontext/the-canvas-state/2d.state.saverestore.shadowOffsetX.html.ini @ r1
  • tests/wpt/metadata/2dcontext/the-canvas-state/2d.state.saverestore.shadowOffsetY.html.ini @ r1
  • tests/wpt/metadata/html/dom/interfaces.html.ini @ r1

components/canvas/canvas_paint_task.rs, line 175 [r1] (raw file):
Wouldn't it help in the future if these two properties were a Point2D value?


components/script/dom/canvasrenderingcontext2d.rs, line 1069 [r1] (raw file):
Link to spec.


components/script/dom/canvasrenderingcontext2d.rs, line 1073 [r1] (raw file):
Ditto.


components/script/dom/canvasrenderingcontext2d.rs, line 1078 [r1] (raw file):
Ditto.


components/script/dom/canvasrenderingcontext2d.rs, line 1082 [r1] (raw file):
Ditto.


components/script/dom/canvasrenderingcontext2d.rs, line 1087 [r1] (raw file):
Ditto.


components/script/dom/canvasrenderingcontext2d.rs, line 1091 [r1] (raw file):
Ditto.


components/script/dom/canvasrenderingcontext2d.rs, line 1096 [r1] (raw file):
Ditto.


components/script/dom/canvasrenderingcontext2d.rs, line 1102 [r1] (raw file):
Ditto.


Comments from the review on Reviewable.io

@nox nox self-assigned this Jun 11, 2015
@hyowon hyowon force-pushed the hyowon:shadow_attrs branch from 154f368 to 473afdb Jun 12, 2015
@nox
Copy link
Member

nox commented Jun 12, 2015

@bors-servo: r+

No problem, given many things are unimplemented anyway everywhere it doesn't matter much whether the thing is complete or not.

-S-awaiting-answer -S-awaiting-review +S-awaiting-merge


Review status: :shipit: all files reviewed, all discussions resolved, all commit checks successful.
Reviewed files:

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

Comments from the review on Reviewable.io

@bors-servo
Copy link
Contributor

bors-servo commented Jun 12, 2015

📌 Commit 473afdb has been approved by nox

@bors-servo
Copy link
Contributor

bors-servo commented Jun 12, 2015

Testing commit 473afdb with merge 8811b60...

bors-servo pushed a commit that referenced this pull request Jun 12, 2015
The first step of the implementation for shadows in canvas.
r? @nox @jdm 
cc @yichoi

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6337)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jun 12, 2015

💔 Test failed - linux3

@hyowon
Copy link
Contributor Author

hyowon commented Jun 12, 2015

I couldn't reproduce /css21_dev/html4/absolute-replaced-height-014.htm fail on my linux desktop.
It's weird because the reported failure doesn't seem to be related with my commit.

@nox
Copy link
Member

nox commented Jun 12, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Jun 12, 2015

Previous build results for android, gonk, linux1, linux2, mac2 are reusable. Rebuilding only linux3, mac1...

@bors-servo
Copy link
Contributor

bors-servo commented Jun 12, 2015

The build was interrupted to prioritize another pull request.

@bors-servo
Copy link
Contributor

bors-servo commented Jun 12, 2015

Previous build results for android, gonk, linux1, linux2, mac2 are reusable. Rebuilding only linux3, mac1...

@bors-servo
Copy link
Contributor

bors-servo commented Jun 12, 2015

The build was interrupted to prioritize another pull request.

@bors-servo
Copy link
Contributor

bors-servo commented Jun 12, 2015

Testing commit 473afdb with merge f163f2b...

bors-servo pushed a commit that referenced this pull request Jun 12, 2015
The first step of the implementation for shadows in canvas.
r? @nox @jdm 
cc @yichoi

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6337)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jun 12, 2015

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

@bors-servo bors-servo merged commit 473afdb into servo:master Jun 12, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@hyowon hyowon deleted the hyowon:shadow_attrs branch Jun 16, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.