Skip to content

/ servo

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

Implement drawing shadows in canvas. #6355

Merged
merged 1 commit into from Jun 16, 2015
Merged

Implement drawing shadows in canvas. #6355

merged 1 commit into from Jun 16, 2015

Conversation

@hyowon
Copy link
Contributor

hyowon commented Jun 12, 2015

https://html.spec.whatwg.org/multipage/#when-shadows-are-drawn
r? @nox
cc @mmatyas @yichoi

Review on Reviewable
@nox
Copy link
Member

nox commented Jun 12, 2015 

$ ./mach test-tidy
components/canvas/canvas_paint_task.rs:338: (much) overlong line
components/canvas/canvas_paint_task.rs:349: (much) overlong line
components/canvas/canvas_paint_task.rs:367: (much) overlong line
components/canvas/canvas_paint_task.rs:379: (much) overlong line
components/canvas/canvas_paint_task.rs:601: (much) overlong line
components/canvas/canvas_paint_task.rs Outdated
&self.state.shadow_color,
&Point2D(self.state.shadow_offset_x as AzFloat,
self.state.shadow_offset_y as AzFloat),
(self.state.shadow_blur / 2.0f64) as AzFloat,

This comment has been minimized.

Sign in to view
@pcwalton

pcwalton Jun 12, 2015

Contributor

Why divide by two here?

This comment has been minimized.

Sign in to view
@hyowon

hyowon Jun 12, 2015

Author Contributor

The shadowBlur attribute specifies the level of the blurring effect.
And the sigma as the standard deviation to perform a 2D Gaussian blur is half the value of shadowBlur.
Please refer to the 4-1 on the spec https://html.spec.whatwg.org/multipage/#when-shadows-are-drawn.
components/canvas/canvas_paint_task.rs Outdated
&self.state.shadow_color,
&Point2D(self.state.shadow_offset_x as AzFloat,
self.state.shadow_offset_y as AzFloat),
(self.state.shadow_blur / 2.0f64) as AzFloat,

This comment has been minimized.

Sign in to view
@pcwalton

pcwalton Jun 12, 2015

Contributor

Same here.

This comment has been minimized.

Sign in to view
@hyowon

hyowon Jun 12, 2015

Author Contributor

The same as above. :)
@nox
Copy link
Member

nox commented Jun 13, 2015

I think draw_rect(), draw_image() and draw_image_self() should call a higher-order function draw_with_shadow() taking a closure and shadow_src_rect.

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

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

  • components/canvas/canvas_paint_task.rs @ r3
  • tests/wpt/metadata/2dcontext/drawing-rectangles-to-the-canvas/2d.fillRect.shadow.html.ini @ r1
  • tests/wpt/metadata/2dcontext/shadows/2d.shadow.alpha.2.html.ini @ r1
  • tests/wpt/metadata/2dcontext/shadows/2d.shadow.alpha.3.html.ini @ r1
  • tests/wpt/metadata/2dcontext/shadows/2d.shadow.alpha.4.html.ini @ r1
  • tests/wpt/metadata/2dcontext/shadows/2d.shadow.alpha.5.html.ini @ r1
  • tests/wpt/metadata/2dcontext/shadows/2d.shadow.canvas.alpha.html.ini @ r2
  • tests/wpt/metadata/2dcontext/shadows/2d.shadow.canvas.basic.html.ini @ r2
  • tests/wpt/metadata/2dcontext/shadows/2d.shadow.canvas.transparent.2.html.ini @ r2
  • tests/wpt/metadata/2dcontext/shadows/2d.shadow.clip.1.html.ini @ r1
  • tests/wpt/metadata/2dcontext/shadows/2d.shadow.clip.3.html.ini @ r1
  • tests/wpt/metadata/2dcontext/shadows/2d.shadow.composite.1.html.ini @ r1
  • tests/wpt/metadata/2dcontext/shadows/2d.shadow.composite.2.html.ini @ r1
  • tests/wpt/metadata/2dcontext/shadows/2d.shadow.gradient.alpha.html.ini @ r1
  • tests/wpt/metadata/2dcontext/shadows/2d.shadow.gradient.basic.html.ini @ r1
  • tests/wpt/metadata/2dcontext/shadows/2d.shadow.gradient.transparent.2.html.ini @ r1
  • tests/wpt/metadata/2dcontext/shadows/2d.shadow.image.alpha.html.ini @ r2
  • tests/wpt/metadata/2dcontext/shadows/2d.shadow.image.basic.html.ini @ r2
  • tests/wpt/metadata/2dcontext/shadows/2d.shadow.image.scale.html.ini @ r2
  • tests/wpt/metadata/2dcontext/shadows/2d.shadow.image.transparent.2.html.ini @ r2
  • tests/wpt/metadata/2dcontext/shadows/2d.shadow.offset.negativeX.html.ini @ r1
  • tests/wpt/metadata/2dcontext/shadows/2d.shadow.offset.negativeY.html.ini @ r1
  • tests/wpt/metadata/2dcontext/shadows/2d.shadow.offset.positiveX.html.ini @ r1
  • tests/wpt/metadata/2dcontext/shadows/2d.shadow.offset.positiveY.html.ini @ r1
  • tests/wpt/metadata/2dcontext/shadows/2d.shadow.outside.html.ini @ r1
  • tests/wpt/metadata/2dcontext/shadows/2d.shadow.transform.1.html.ini @ r1
  • tests/wpt/metadata/2dcontext/shadows/2d.shadow.transform.2.html.ini @ r1

components/canvas/canvas_paint_task.rs, line 625 [r3] (raw file):
Link to spec.

https://html.spec.whatwg.org/multipage/scripting.html#when-shadows-are-drawn

components/canvas/canvas_paint_task.rs, line 626 [r3] (raw file):
The spec says "non-zero".

components/canvas/canvas_paint_task.rs, line 629 [r3] (raw file):
The spec says that alpha should be non-zero and one of these three too. It lacks parentheses.

Comments from the review on Reviewable.io
@nox nox self-assigned this Jun 13, 2015
@bors-servo
Copy link
Contributor

bors-servo commented Jun 13, 2015

☔ The latest upstream changes (presumably #6356) made this pull request unmergeable. Please resolve the merge conflicts.
@nox nox added the S-needs-rebase label Jun 13, 2015
@hyowon hyowon force-pushed the hyowon:drawing_shadows branch from 49b2a47 to 7b9a368 Jun 15, 2015
@hyowon
Copy link
Contributor Author

hyowon commented Jun 15, 2015

As you commented, I've added draw_with_shadow(). It makes the code more concise. Thanks.

Review status: 26 of 27 files reviewed, 5 unresolved discussions, some commit checks failed.

components/canvas/canvas_paint_task.rs, line 625 [r3] (raw file):
Done.

components/canvas/canvas_paint_task.rs, line 626 [r3] (raw file):
Done.

components/canvas/canvas_paint_task.rs, line 629 [r3] (raw file):
Added parentheses. Done.

Comments from the review on Reviewable.io
@hyowon hyowon force-pushed the hyowon:drawing_shadows branch from 7b9a368 to b335480 Jun 15, 2015
@nox
Copy link
Member

nox commented Jun 15, 2015

-S-awaiting-review -S-needs-rebase +S-needs-squash

Looks good, squash and I will r+ it.

Reviewed 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

Comments from the review on Reviewable.io
@hyowon
Implement drawing shadows in canvas.
Loading status checks…
465cea8
@hyowon hyowon force-pushed the hyowon:drawing_shadows branch from b335480 to 465cea8 Jun 15, 2015
@nox
Copy link
Member

nox commented Jun 16, 2015

@bors-servo: r+

-S-awaiting-review -S-needs-squash +S-awaiting-merge

Reviewed 3 of 3 files at r6.
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

Comments from the review on Reviewable.io
@bors-servo
Copy link
Contributor

bors-servo commented Jun 16, 2015

📌 Commit 465cea8 has been approved by nox
bors-servo pushed a commit that referenced this pull request Jun 16, 2015
bors-servo
Auto merge of #6355 - hyowon:drawing_shadows, r=nox
Loading status checks…
48e1d45 
https://html.spec.whatwg.org/multipage/#when-shadows-are-drawn
r? @nox 
cc @mmatyas @yichoi

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

bors-servo commented Jun 16, 2015

⌛ Testing commit 465cea8 with merge 48e1d45...
@bors-servo
Copy link
Contributor

bors-servo commented Jun 16, 2015

☀️ Test successful - android, gonk, linux1, linux2, linux3, mac1, mac2, mac3
@bors-servo bors-servo merged commit 465cea8 into servo:master Jun 16, 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:drawing_shadows branch Jun 17, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@nox nox

Labels
S-awaiting-merge
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@hyowon @nox @bors-servo @pcwalton @highfive
You can’t perform that action at this time.