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

Jump to bottom

Snap to screen pixels instead of px #7909

Merged
merged 1 commit into from Oct 9, 2015
Merged

Snap to screen pixels instead of px #7909

merged 1 commit into from Oct 9, 2015

Conversation

paulrouget
Copy link
Contributor

Fixes #7904

Review on Reviewable

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 7, 2015
@metajack
Copy link
Contributor

metajack commented Oct 7, 2015

r? @glennw

@glennw
Copy link
Member

glennw commented Oct 7, 2015

@paul This looks like a good fix, thanks! However, we have some CI related issues with pixel-ratio=2 tests at the moment, and I'm a little concerned merging this now will create a lot of intermittent failures (unrelated to your fix).

@jdm Thoughts on the above? Shall we hold off on merging this for now? Merge it with the test marked as flaky?

@jdm
Copy link
Member

jdm commented Oct 7, 2015

If we have a plan to tackle the pixel snapping test failure, then let's hold off. Otherwise merging with flaky is acceptable.

@glennw
Copy link
Member

glennw commented Oct 7, 2015

I just checked the reftest.rs code and the assert in question will fire even if the test is marked as flaky. I could update reftest.rs to not assert here if marked as flaky (as a temporary workaround), or we may have to just disable this test for now - thoughts @jdm?

@jdm
Copy link
Member

jdm commented Oct 7, 2015

I would extend the flaky check to include the image size for now.

@glennw
Copy link
Member

glennw commented Oct 7, 2015

OK, I'll change the reftest code today and then we'll be able to merge this. @paulrouget could you update this PR to mark your new test as flaky in the meantime?

@paulrouget
Copy link
Contributor Author

As recommended by @mbrubeck I set resolution=300x300 to avoid the giant window (which I guess it's why the test is flaky)

@glennw
Copy link
Member

glennw commented Oct 8, 2015

@bors-servo r+

Thanks!

@bors-servo
Copy link
Contributor

📌 Commit 0c4ca2b has been approved by glennw

@highfive highfive 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 Oct 8, 2015
@bors-servo
Copy link
Contributor

⌛ Testing commit 0c4ca2b with merge f7b2c3e...

bors-servo pushed a commit that referenced this pull request Oct 8, 2015
Auto merge of #7909 - paulrouget:subpixelPos, r=glennw
f7b2c3e 
Snap to screen pixels instead of px

Fixes #7904

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

💔 Test failed - linux-rel

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Oct 8, 2015
@mbrubeck
Copy link
Contributor

mbrubeck commented Oct 8, 2015

@bors-servo retry

#7870

@bors-servo
Copy link
Contributor

🔒 Merge conflict

@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #7920) made this pull request unmergeable. Please resolve the merge conflicts.

@highfive highfive added S-needs-rebase There are merge conflict errors. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Oct 8, 2015
@jdm jdm removed the S-tests-failed The changes caused existing tests to fail. label Oct 8, 2015
@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 8, 2015
@mbrubeck
Copy link
Contributor

mbrubeck commented Oct 8, 2015

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit 0c4ca2b has been approved by mbrubeck

@highfive highfive 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. S-needs-rebase There are merge conflict errors. labels Oct 8, 2015
@jdm jdm closed this Oct 9, 2015
@jdm jdm reopened this Oct 9, 2015
@jdm
Copy link
Member

jdm commented Oct 9, 2015

@bors-servo: r=mbrubeck

@bors-servo
Copy link
Contributor

📌 Commit 99cccb2 has been approved by mbrubeck

@bors-servo
Copy link
Contributor

⌛ Testing commit 99cccb2 with merge 8ef8c53...

bors-servo pushed a commit that referenced this pull request Oct 9, 2015
Auto merge of #7909 - paulrouget:subpixelPos, r=mbrubeck
8ef8c53 
Snap to screen pixels instead of px

Fixes #7904

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

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt

@bors-servo bors-servo merged commit 99cccb2 into servo:master Oct 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@glennw glennw

Labels
S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@paulrouget @metajack @glennw @jdm @bors-servo @mbrubeck @highfive