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

Introduce CSSPixel as a replacement for ViewportPx and PagePx. #15700

Merged
merged 1 commit into from Feb 24, 2017

Conversation

@glennw
Copy link
Member

glennw commented Feb 23, 2017

This change is Reviewable

@highfive
Copy link

highfive commented Feb 23, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/servo/media_queries.rs, components/style/viewport.rs
  • @jgraham: components/webdriver_server/lib.rs
  • @emilio: components/style/servo/media_queries.rs, components/layout/display_list_builder.rs, components/style/viewport.rs
  • @fitzgen: components/script/dom/document.rs, components/script_traits/script_msg.rs, components/script_traits/script_msg.rs, components/script_traits/lib.rs, components/script_traits/lib.rs, components/script/dom/window.rs, components/script/dom/mediaquerylist.rs
  • @KiChjang: components/script/dom/document.rs, components/script_traits/script_msg.rs, components/script_traits/script_msg.rs, components/script_traits/lib.rs, components/script_traits/lib.rs, components/script/dom/window.rs, components/script/dom/mediaquerylist.rs
  • @asajeffrey: components/constellation/pipeline.rs, components/webdriver_server/lib.rs, components/constellation/constellation.rs
@highfive
Copy link

highfive commented Feb 23, 2017

warning Warning warning

  • These commits modify style, layout, and script code, but no tests are modified. Please consider adding a test!
@glennw
Copy link
Member Author

glennw commented Feb 23, 2017

This change deserves some explanation.

This PR is (sort of) a WIP. I want to get some feedback on whether
this seems like a viable / correct approach to handling both
page and pinch zoom. Assuming it does, we can either merge this
after review (which would be my preference) or wait until the WR
pinch zoom functionality is ready before merging this part.

The idea is to move pinch-zoom to be handled by WR as (almost)
an internal detail. There would be an API to control the pinch zoom
factor, but apart from that WR would handle pinch-zoom internally.

From what I can tell of the specifications and browser behaviour,
this makes sense - but it's quite possible I've missed an important
detail.

Given the assumptions above, the Servo code now only needs to deal
with three coordinate spaces, rather than four. Specifically, we now
would have:
Device pixels -> Device independent pixels (hi-dpi factor) -> CSS pixels (page zoom factor)

This PR doesn't break any existing functionality, since pinch zoom
doesn't currently work at all. Page zoom is also currently broken
although that's orthogonal to this PR.

@glennw
Copy link
Member Author

glennw commented Feb 23, 2017

@highfive highfive assigned mbrubeck and unassigned nox Feb 23, 2017
@glennw glennw force-pushed the glennw:zoom-wip-2 branch from cc102cc to 937c5fc Feb 23, 2017
@glennw
Copy link
Member Author

glennw commented Feb 23, 2017

Rebased and fixed tidy error.

@jdm jdm added the S-fails-travis label Feb 23, 2017
@jdm
Copy link
Member

jdm commented Feb 23, 2017

Style unit tests need updating.

@glennw glennw force-pushed the glennw:zoom-wip-2 branch from 937c5fc to 30ff2f8 Feb 23, 2017
@glennw
Copy link
Member Author

glennw commented Feb 23, 2017

Thanks, updated the unit tests.

/// TODO(gw): Once WR supports pinch zoom, use a type directly from webrender_traits.
#[derive(Clone, Copy, Debug, PartialEq)]
#[cfg_attr(feature = "servo", derive(Deserialize, Serialize, HeapSizeOf))]
pub struct PinchZoomFactor(f32);

This comment has been minimized.

@mbrubeck

mbrubeck Feb 23, 2017

Contributor

Why a new type instead of ScaleFactor?

@mbrubeck
Copy link
Contributor

mbrubeck commented Feb 23, 2017

@bors-servo r+

Curious about the answer to the above question, but no changes required.

@bors-servo
Copy link
Contributor

bors-servo commented Feb 23, 2017

📌 Commit 30ff2f8 has been approved by mbrubeck

@bors-servo
Copy link
Contributor

bors-servo commented Feb 24, 2017

Testing commit 30ff2f8 with merge 1d13e6a...

bors-servo added a commit that referenced this pull request Feb 24, 2017
Introduce CSSPixel as a replacement for ViewportPx and PagePx.

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15700)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Feb 24, 2017

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev
Approved by: mbrubeck
Pushing 1d13e6a to master...

@bors-servo bors-servo merged commit 30ff2f8 into servo:master Feb 24, 2017
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
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
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.