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

Update some canvas properties as enums instead of DOMString #9689

Merged
merged 1 commit into from Feb 19, 2016

Conversation

@saurvs
Copy link
Contributor

saurvs commented Feb 18, 2016

Fixes #9617

I'm going to incrementally rollout commits and squash them finally.

Review on Reviewable

@highfive
Copy link

highfive commented Feb 18, 2016

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@saurvs
Copy link
Contributor Author

saurvs commented Feb 18, 2016

@jdm In components/script/dom/canvasrenderingcontext2d.rs, the functions LineCap(&self) and SetLineCap(&self, cap_str: DOMString) return and accept a DOMString. Should I modify them to return and accept an appropriate enum instead?

@Ms2ger
Copy link
Contributor

Ms2ger commented Feb 18, 2016

Yep; I don't expect your build to succeed without that change.

@saurvs
Copy link
Contributor Author

saurvs commented Feb 18, 2016

@Ms2ger Does this change make sense?

@jdm
Copy link
Member

jdm commented Feb 18, 2016

@saurvs There are still build errors, so there's some more work to do. Are you building locally?

@nox
Copy link
Member

nox commented Feb 19, 2016

components/script/dom/canvasrenderingcontext2d.rs:1212:5: 1214:6 error: method `LineCap` has an incompatible type for trait:
 expected enum `dom::bindings::codegen::Bindings::CanvasRenderingContext2DBinding::CanvasLineCap`,
    found enum `canvas_traits::LineCapStyle` [E0053]
components/script/dom/canvasrenderingcontext2d.rs:1212     fn LineCap(&self) -> LineCapStyle {
components/script/dom/canvasrenderingcontext2d.rs:1213         self.state.borrow().line_cap
components/script/dom/canvasrenderingcontext2d.rs:1214     }
components/script/dom/canvasrenderingcontext2d.rs:1212:5: 1214:6 help: run `rustc --explain E0053` to see a detailed explanation
components/script/dom/canvasrenderingcontext2d.rs:1217:5: 1220:6 error: method `SetLineCap` has an incompatible type for trait:
 expected enum `dom::bindings::codegen::Bindings::CanvasRenderingContext2DBinding::CanvasLineCap`,
    found enum `canvas_traits::LineCapStyle` [E0053]
components/script/dom/canvasrenderingcontext2d.rs:1217     fn SetLineCap(&self, cap: LineCapStyle) {
components/script/dom/canvasrenderingcontext2d.rs:1218         self.state.borrow_mut().line_cap = cap;
components/script/dom/canvasrenderingcontext2d.rs:1219         self.ipc_renderer.send(CanvasMsg::Canvas2d(Canvas2dMsg::SetLineCap(cap))).unwrap();
components/script/dom/canvasrenderingcontext2d.rs:1220     }
components/script/dom/canvasrenderingcontext2d.rs:1217:5: 1220:6 help: run `rustc --explain E0053` to see a detailed explanation
components/script/dom/canvasrenderingcontext2d.rs:1223:5: 1225:6 error: method `LineJoin` has an incompatible type for trait:
 expected enum `dom::bindings::codegen::Bindings::CanvasRenderingContext2DBinding::CanvasLineJoin`,
    found enum `canvas_traits::LineJoinStyle` [E0053]
components/script/dom/canvasrenderingcontext2d.rs:1223     fn LineJoin(&self) -> LineJoinStyle {
components/script/dom/canvasrenderingcontext2d.rs:1224         self.state.borrow().line_join
components/script/dom/canvasrenderingcontext2d.rs:1225     }
components/script/dom/canvasrenderingcontext2d.rs:1223:5: 1225:6 help: run `rustc --explain E0053` to see a detailed explanation
components/script/dom/canvasrenderingcontext2d.rs:1228:5: 1231:6 error: method `SetLineJoin` has an incompatible type for trait:
 expected enum `dom::bindings::codegen::Bindings::CanvasRenderingContext2DBinding::CanvasLineJoin`,
    found enum `canvas_traits::LineJoinStyle` [E0053]
components/script/dom/canvasrenderingcontext2d.rs:1228     fn SetLineJoin(&self, join: LineJoinStyle) {
components/script/dom/canvasrenderingcontext2d.rs:1229         self.state.borrow_mut().line_join = join;
components/script/dom/canvasrenderingcontext2d.rs:1230         self.ipc_renderer.send(CanvasMsg::Canvas2d(Canvas2dMsg::SetLineJoin(join))).unwrap();
components/script/dom/canvasrenderingcontext2d.rs:1231     }
components/script/dom/canvasrenderingcontext2d.rs:1228:5: 1231:6 help: run `rustc --explain E0053` to see a detailed explanation
error: aborting due to 4 previous errors
@saurvs
Copy link
Contributor Author

saurvs commented Feb 19, 2016

@jdm The build works now.

@@ -194,4 +200,3 @@ interface CanvasPathMethods {
// double rotation, double startAngle, double endAngle,
// boolean anticlockwise);
};

This comment has been minimized.

Copy link
@KiChjang

KiChjang Feb 19, 2016

Member

Undo this change please.

@jdm
Copy link
Member

jdm commented Feb 19, 2016

@saurvs Thanks! This looks great; I can't think of any way to avoid the conversion between the two enum values, since script has to depend on canvas_msg. Once you add back the newline, I'll merge this!

@jdm jdm self-assigned this Feb 19, 2016
@nox
Copy link
Member

nox commented Feb 19, 2016

@bors-servo r=jdm

@bors-servo
Copy link
Contributor

bors-servo commented Feb 19, 2016

📌 Commit 939560b has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Feb 19, 2016

Testing commit 939560b with merge 1ec62c8...

bors-servo added a commit that referenced this pull request Feb 19, 2016
Update some canvas properties as enums instead of DOMString

Fixes #9617

I'm going to incrementally rollout commits and squash them finally.

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

bors-servo commented Feb 19, 2016

💔 Test failed - mac-rel-css

@jdm
Copy link
Member

jdm commented Feb 19, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Feb 19, 2016

Testing commit 939560b with merge c68fbee...

bors-servo added a commit that referenced this pull request Feb 19, 2016
Update some canvas properties as enums instead of DOMString

Fixes #9617

I'm going to incrementally rollout commits and squash them finally.

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

bors-servo commented Feb 19, 2016

@bors-servo bors-servo merged commit 939560b into servo:master Feb 19, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
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

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