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

Fixed scaling artefacts in paint worklets caused by zoom and hidpi #17499

Merged
merged 1 commit into from Jul 21, 2017

Conversation

@asajeffrey
Copy link
Member

asajeffrey commented Jun 23, 2017

This PR renders paint worklet canvases at the device pixel resolution, rather than the CSS pixel resolution.

It's a dependent PR, building on #17239, #17326 and #17364.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #17454
  • These changes do not require tests because we don't run reftests with zoom enabled

This change is Reviewable

@highfive
Copy link

highfive commented Jun 23, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/context.rs, components/style/servo/restyle_damage.rs, components/style/servo/media_queries.rs
  • @KiChjang: components/script/dom/webidls/PaintWorkletGlobalScope.webidl, components/script/dom/webidls/StylePropertyMapReadOnly.webidl, components/script_layout_interface/lib.rs, components/script/dom/testworkletglobalscope.rs, components/script/dom/mod.rs and 27 more
  • @fitzgen: components/script/dom/webidls/PaintWorkletGlobalScope.webidl, components/script/dom/webidls/StylePropertyMapReadOnly.webidl, components/script_layout_interface/lib.rs, components/script/dom/testworkletglobalscope.rs, components/script/dom/mod.rs and 27 more
  • @emilio: components/layout/display_list_builder.rs, components/layout/Cargo.toml, components/style/context.rs, components/style/servo/restyle_damage.rs, components/layout/context.rs and 2 more
@highfive
Copy link

highfive commented Jun 23, 2017

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@asajeffrey
Copy link
Member Author

asajeffrey commented Jun 23, 2017

r? @glennw

@highfive highfive assigned glennw and unassigned pcwalton Jun 23, 2017
@jdm jdm added the S-fails-travis label Jun 23, 2017
@jdm
Copy link
Member

jdm commented Jun 23, 2017

error: no method named `device_pixel_ratio` found for type `&gecko::media_queries::Device` in the current scope

   --> /home/travis/build/servo/servo/components/style/context.rs:150:31

    |

150 |         self.stylist.device().device_pixel_ratio()

    |                               ^^^^^^^^^^^^^^^^^^

error: aborting due to previous error

error: Could not compile `style`.
@asajeffrey
Copy link
Member Author

asajeffrey commented Jun 24, 2017

OK, I added a definition to style/gecko/media_queries.rs, but I'm not sure it's the right one...

    pub fn device_pixel_ratio(&self) -> ScaleFactor<f32, CSSPixel, DevicePixel> {
        let mut dppx = self.pres_context().mOverrideDPPX;
        if dppx <= 0.0 { dppx = 1.0; }
        ScaleFactor::new(dppx)
    }

@emilio or @bzbarsky does this look right?

@bzbarsky
Copy link
Contributor

bzbarsky commented Jun 24, 2017

No, that's not right. That's an override value if someone really needs to override the normal value and have the prescontext pretend like the override is what's going on. The normal value is nsPresContext::AppUnitsPerCSSPixel()/presContext->AppUnitsPerDevPixel() or so.

@asajeffrey
Copy link
Member Author

asajeffrey commented Jun 24, 2017

@bzbarsky: thanks!

@asajeffrey asajeffrey force-pushed the asajeffrey:script-paint-worklets-zoom branch from 335eb86 to 73c21e8 Jun 24, 2017
@asajeffrey
Copy link
Member Author

asajeffrey commented Jun 24, 2017

Slightly annoyingly, those functions aren't exported by our Gecko bindings. I think this is equivalent:

    pub fn device_pixel_ratio(&self) -> ScaleFactor<f32, CSSPixel, DevicePixel> {
        let au_per_dpx = self.pres_context().mCurAppUnitsPerDevPixel as f32;
        let au_per_px = AU_PER_PX as f32;
        ScaleFactor::new(au_per_px / au_per_dpx)
    }
@bzbarsky
Copy link
Contributor

bzbarsky commented Jun 24, 2017

Yes, that seems reasonable. But again, mOverrideDPPX overrides if it's > 0.

@asajeffrey asajeffrey force-pushed the asajeffrey:script-paint-worklets-zoom branch from 73c21e8 to ee387a5 Jun 24, 2017
@asajeffrey
Copy link
Member Author

asajeffrey commented Jun 24, 2017

Good point. Thanks!

@bors-servo
Copy link
Contributor

bors-servo commented Jun 30, 2017

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

@asajeffrey
Copy link
Member Author

asajeffrey commented Jul 17, 2017

@glennw This PR's dependencies have been merged, so it's ready for review!

@bors-servo
Copy link
Contributor

bors-servo commented Jul 18, 2017

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

@jdm
Copy link
Member

jdm commented Jul 18, 2017

error[E0432]: unresolved import `script_traits::DevicePixel`

  --> /home/travis/build/servo/servo/ports/glutin/window.rs:27:21

   |

27 | use script_traits::{DevicePixel, LoadData, TouchEventType, TouchpadPressurePhase};

   |                     ^^^^^^^^^^^ no `DevicePixel` in the root

error[E0412]: cannot find type `IpcSender` in this scope

    --> /home/travis/build/servo/servo/ports/glutin/window.rs:1304:60

     |

1304 |     fn allow_navigation(&self, _: ServoUrl, response_chan: IpcSender<bool>) {

     |                                                            ^^^^^^^^^ not found in this scope

     |

help: possible candidate is found in another module, you can import it into scope

     |

36   | use servo::ipc_channel::ipc::IpcSender;

     |

error: aborting due to 2 previous errors

error: Could not compile `glutin_app`.
@glennw
Copy link
Member

glennw commented Jul 19, 2017

@asajeffrey r=me on the changes - it'd be good if we could add a reftest either here or as a follow up though (I'm not sure if the reftest harness allows setting dpi ratio though).

@asajeffrey asajeffrey force-pushed the asajeffrey:script-paint-worklets-zoom branch from 8eb8de3 to c9c569a Jul 20, 2017
@asajeffrey
Copy link
Member Author

asajeffrey commented Jul 20, 2017

Squashed. @bors-servo: r=glennw

@bors-servo
Copy link
Contributor

bors-servo commented Jul 20, 2017

📌 Commit c9c569a has been approved by glennw

@bors-servo
Copy link
Contributor

bors-servo commented Jul 20, 2017

Testing commit c9c569a with merge 91a157d...

bors-servo added a commit that referenced this pull request Jul 20, 2017
Fixed scaling artefacts in paint worklets caused by zoom and hidpi

<!-- Please describe your changes on the following line: -->

This PR renders paint worklet canvases at the device pixel resolution, rather than the CSS pixel resolution.

It's a dependent PR, building on #17239, #17326 and #17364.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #17454
- [X] These changes do not require tests because we don't run reftests with zoom enabled

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/17499)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jul 20, 2017

💔 Test failed - linux-dev

@asajeffrey asajeffrey force-pushed the asajeffrey:script-paint-worklets-zoom branch from c9c569a to caa3585 Jul 20, 2017
@asajeffrey
Copy link
Member Author

asajeffrey commented Jul 20, 2017

Got build-cef to compile. @bors-servo r=glennw

@bors-servo
Copy link
Contributor

bors-servo commented Jul 20, 2017

📌 Commit caa3585 has been approved by glennw

@bors-servo
Copy link
Contributor

bors-servo commented Jul 21, 2017

Testing commit caa3585 with merge 9fcbeb3...

bors-servo added a commit that referenced this pull request Jul 21, 2017
Fixed scaling artefacts in paint worklets caused by zoom and hidpi

<!-- Please describe your changes on the following line: -->

This PR renders paint worklet canvases at the device pixel resolution, rather than the CSS pixel resolution.

It's a dependent PR, building on #17239, #17326 and #17364.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #17454
- [X] These changes do not require tests because we don't run reftests with zoom enabled

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/17499)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jul 21, 2017

@bors-servo bors-servo merged commit caa3585 into servo:master Jul 21, 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.

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