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

Simplify implementation of '<canvas>' 'width' and 'height' attributes. #8243

Merged
merged 1 commit into from Nov 4, 2015
Merged

Simplify implementation of '<canvas>' 'width' and 'height' attributes. #8243

merged 1 commit into from Nov 4, 2015

Conversation

@eefriedman
Copy link
Contributor

eefriedman commented Oct 28, 2015

Strictly speaking, this affects correctness for extremely large width and height values... but that's unlikely to matter in practice.

Review on Reviewable
@eefriedman
Copy link
Contributor Author

eefriedman commented Oct 28, 2015

Err, this isn't right; it needs a parse_plain_attribute implementation. Sorry about the noise.
@eefriedman eefriedman force-pushed the eefriedman:canvas-width-height branch 2 times, most recently from 240a3b4 to 0aee24c Oct 28, 2015
@eefriedman
Copy link
Contributor Author

eefriedman commented Oct 28, 2015

Should be fixed now.
@eefriedman eefriedman force-pushed the eefriedman:canvas-width-height branch from 0aee24c to 6e332db Oct 28, 2015
@Ms2ger
Copy link
Contributor

Ms2ger commented Oct 29, 2015

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

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

components/script/dom/htmlcanvaselement.rs, line 126 [r1] (raw file):
This will try to create roots from the layout thread.

components/script/dom/htmlcanvaselement.rs, line 291 [r1] (raw file):
No need for the recreate local.

Comments from the review on Reviewable.io
@Ms2ger
Copy link
Contributor

Ms2ger commented Oct 29, 2015

Btw, I checked... This does fail all the tests.
@eefriedman
Copy link
Contributor Author

eefriedman commented Oct 29, 2015

Addressed review comments. (And I'll try to be more careful about running tests before posting a patch in the future.)

Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

Comments from the review on Reviewable.io
@eefriedman eefriedman force-pushed the eefriedman:canvas-width-height branch from 6e332db to 6a7e6e7 Oct 29, 2015
@frewsxcv
Copy link
Member

frewsxcv commented Oct 30, 2015

components/script/dom/htmlcanvaselement.rs, line 129 [r2] (raw file):
Not a big deal, but this could be map_or instead of map..unwrap_or

Comments from the review on Reviewable.io
@eefriedman eefriedman force-pushed the eefriedman:canvas-width-height branch from 6a7e6e7 to a2ed681 Oct 30, 2015
@eefriedman
Copy link
Contributor Author

eefriedman commented Oct 30, 2015

Updated.
@eefriedman
Simplify implementation of '<canvas>' 'width' and 'height' attributes.
Loading status checks…
4fc0a66
@eefriedman eefriedman force-pushed the eefriedman:canvas-width-height branch from a2ed681 to 4fc0a66 Nov 3, 2015
@eefriedman
Copy link
Contributor Author

eefriedman commented Nov 3, 2015

@Ms2ger Ping.
@Ms2ger
Copy link
Contributor

Ms2ger commented Nov 4, 2015

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Nov 4, 2015

📌 Commit 4fc0a66 has been approved by Ms2ger
@bors-servo
Copy link
Contributor

bors-servo commented Nov 4, 2015

⌛ Testing commit 4fc0a66 with merge 46ad8b9...
bors-servo added a commit that referenced this pull request Nov 4, 2015
@bors-servo
Auto merge of #8243 - eefriedman:canvas-width-height, r=Ms2ger
46ad8b9 
Simplify implementation of '<canvas>' 'width' and 'height' attributes.

Strictly speaking, this affects correctness for extremely large width and height values... but that's unlikely to matter in practice.

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

bors-servo commented Nov 4, 2015

💔 Test failed - linux-rel
@jdm
Copy link
Member

jdm commented Nov 4, 2015

@bors-servo: retry
@bors-servo
Copy link
Contributor

bors-servo commented Nov 4, 2015

⌛ Testing commit 4fc0a66 with merge b39035c...
bors-servo added a commit that referenced this pull request Nov 4, 2015
@bors-servo
Auto merge of #8243 - eefriedman:canvas-width-height, r=Ms2ger
Loading status checks…
b39035c 
Simplify implementation of '<canvas>' 'width' and 'height' attributes.

Strictly speaking, this affects correctness for extremely large width and height values... but that's unlikely to matter in practice.

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

bors-servo commented Nov 4, 2015

💔 Test failed - mac-rel-css
@frewsxcv
Copy link
Member

frewsxcv commented Nov 4, 2015

@bors-servo retry

#7791
@bors-servo
Copy link
Contributor

bors-servo commented Nov 4, 2015

⚡ Previous build results for android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-wpt are reusable. Rebuilding only mac-rel-css...
@bors-servo
Copy link
Contributor

bors-servo commented Nov 4, 2015

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt
@bors-servo bors-servo merged commit 4fc0a66 into servo:master Nov 4, 2015
2 of 3 checks passed
2 of 3 checks passed
code-review/reviewable Review in progress: 0 of 2 files reviewed, all discussions resolved
Details
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
Reviewers
No reviews
Assignees

@Ms2ger Ms2ger

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
@eefriedman @Ms2ger @frewsxcv @bors-servo @jdm @SimonSapin @highfive
You can’t perform that action at this time.