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

Use a faster scroll speed under X11 #6373

Merged
merged 1 commit into from
Jun 22, 2015

Conversation

robertknight
Copy link

Platforms may report scroll deltas either in
chunks/lines/rows or pixels, depending on the
platform API and device capabilities.

If the platform reports a line/chunk-based delta
then the application needs to convert the delta
into a suitable number of pixels. Apple's documentation for example states
that the app should interpret the delta as a number of lines or rows to scroll,
depending on the type of view.

This commit just hardcodes it to 57 as
a starting point which matches the value that
Firefox calculates as the max char height
for the root frame on my system.

This depends on this Glutin PR: rust-windowing/glutin#483

Fixes #5660

Review on Reviewable

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jun 13, 2015
@highfive
Copy link

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

@hoppipolla-critic-bot
Copy link

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

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.

@pcwalton
Copy link
Contributor

Great! I'm currently working on cherry-picking over your Glutin change.

@robertknight
Copy link
Author

Thanks @pcwalton! - A related question - what's the current thinking on CEF vs glutin for the Servo goal of an alpha browser this year? I saw there was discussion about whether to support multiple ports or just CEF but I couldn't see what the final outcome was.

@pcwalton
Copy link
Contributor

There hasn't been a final decision AFAIK—thanks for reminding me, since I should bring it up again. I'm still in favor of going CEF-only and recreating the Glutin shell in terms of CEF.

@pcwalton
Copy link
Contributor

@robertknight We've merged the change to Glutin in servo/glutin. Can you update to that commit in this PR? r=me with that.

@pcwalton
Copy link
Contributor

@bors-servo: r+

@bors-servo
Copy link
Contributor

📌 Commit 89e9aa0 has been approved by pcwalton

@bors-servo
Copy link
Contributor

⌛ Testing commit 89e9aa0 with merge aeff2eb...

bors-servo pushed a commit that referenced this pull request Jun 17, 2015
…alton

Use a faster scroll speed under X11

Platforms may report scroll deltas either in
chunks/lines/rows or pixels, depending on the
platform API and device capabilities.

If the platform reports a line/chunk-based delta
then the application needs to convert the delta
into a suitable number of pixels. Apple's documentation for example states
that the app should interpret the delta as a number of lines or rows to scroll,
depending on the type of view.

This commit just hardcodes it to 57 as
a starting point which matches the value that
Firefox calculates as the max char height
for the root frame on my system.

This depends on this Glutin PR: rust-windowing/glutin#483

Fixes #5660

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

💔 Test failed - mac2

@pcwalton
Copy link
Contributor

@robertknight Looks like Mac didn't compile.

@pcwalton pcwalton added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jun 17, 2015
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Jun 17, 2015
@Ms2ger
Copy link
Contributor

Ms2ger commented Jun 20, 2015

There appears to be a merge commit on your PR branch; please rebase on master to get rid of it.

@robertknight robertknight force-pushed the gh5660-rob-x11_scroll_speed branch 2 times, most recently from 87b2927 to 6f24583 Compare June 20, 2015 13:46
@pcwalton
Copy link
Contributor

@bors-servo: r+

@bors-servo
Copy link
Contributor

📌 Commit 6f24583 has been approved by pcwalton

@bors-servo
Copy link
Contributor

⌛ Testing commit 6f24583 with merge e0956e4...

bors-servo pushed a commit that referenced this pull request Jun 22, 2015
…alton

Use a faster scroll speed under X11

Platforms may report scroll deltas either in
chunks/lines/rows or pixels, depending on the
platform API and device capabilities.

If the platform reports a line/chunk-based delta
then the application needs to convert the delta
into a suitable number of pixels. Apple's documentation for example states
that the app should interpret the delta as a number of lines or rows to scroll,
depending on the type of view.

This commit just hardcodes it to 57 as
a starting point which matches the value that
Firefox calculates as the max char height
for the root frame on my system.

This depends on this Glutin PR: rust-windowing/glutin#483

Fixes #5660

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

💔 Test failed - mac2

@pcwalton
Copy link
Contributor

This doesn't build due to a missing glutin import

Platforms may report scroll deltas either in
chunks/lines/rows or pixels, depending on the
platform API and device capabilities.

If the platform reports a line/chunk-based delta
then the application needs to convert the delta
into a suitable number of pixels.

This commit just hardcodes it to 57 as
a starting point which matches the value that
Firefox calculates as the max char height
for the root frame on my system.

Fixes servo#5660
@robertknight
Copy link
Author

Sorry @pcwalton - I hadn't updated the glutin dependency in Cargo.lock for the CEF port. It hadn't occurred to me that used glutin as well. I've just pushed a fix for that.

Is there a better way to keep the various Cargo.lock files in sync?

@pcwalton
Copy link
Contributor

@bors-servo: r+

@bors-servo
Copy link
Contributor

📌 Commit 658df60 has been approved by pcwalton

@bors-servo
Copy link
Contributor

⌛ Testing commit 658df60 with merge 7e0f186...

bors-servo pushed a commit that referenced this pull request Jun 22, 2015
…alton

Use a faster scroll speed under X11

Platforms may report scroll deltas either in
chunks/lines/rows or pixels, depending on the
platform API and device capabilities.

If the platform reports a line/chunk-based delta
then the application needs to convert the delta
into a suitable number of pixels. Apple's documentation for example states
that the app should interpret the delta as a number of lines or rows to scroll,
depending on the type of view.

This commit just hardcodes it to 57 as
a starting point which matches the value that
Firefox calculates as the max char height
for the root frame on my system.

This depends on this Glutin PR: rust-windowing/glutin#483

Fixes #5660

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

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

@bors-servo bors-servo merged commit 658df60 into servo:master Jun 22, 2015
@pcwalton
Copy link
Contributor

\o/ Thanks for your hard work on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-awaiting-review There is new code that needs to be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants