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

Make the pinch-zoom API a frame message instead of a scene message. #3203

Merged
merged 2 commits into from Oct 16, 2018

Conversation

@staktrace
Copy link
Contributor

staktrace commented Oct 16, 2018

Fixes #3202.

There might be other issues that are exposed by this, not sure yet. But it's a start.


This change is Reviewable

@nical
Copy link
Collaborator

nical commented Oct 16, 2018

I think that it makes sense to move the pinch zoom to a frame message since DisplayListFlattener::create_frame_builder doesn't seem to access it. We should probably split the DocumentView struct into things that scene building needs and things that only matter for frame building.

@staktrace
Copy link
Contributor Author

staktrace commented Oct 16, 2018

Note that create_frame_builder does use the accumulated_scale_factor() here: https://searchfox.org/mozilla-central/rev/c9272ef398954288525e37196eada1e5a93d93bf/gfx/webrender/src/display_list_flattener.rs#212 which includes the pinch zoom value. I don't really understand the purpose of that though.

@nical
Copy link
Collaborator

nical commented Oct 16, 2018

D'oh! You are right I spoke too fast. This affects resolved_transform in SpatialNodeType::ReferenceFrame which itself looks like it isn't affecting scene building (I think?). the resolved_transform member doesn't seem to need to be persisted at all. I tried removing the member and adapting the code in SpatialNode::update_transform to use a variable on the stack instead of overwriting the removed member and it compiles.

@staktrace
Copy link
Contributor Author

staktrace commented Oct 16, 2018

Ah yes, I remember seeing that resolved_transform thing when I was working on the parallax bug. I think the only place it's used as a member variable is in print_node and I don't know how important it is there, seems like just debugging output.

@kvark
Copy link
Member

kvark commented Oct 16, 2018

I tried removing the member and adapting the code in SpatialNode::update_transform to use a variable on the stack instead of overwriting the removed member and it compiles.

Should we do it first (or as a part of this PR) then before merging?

@staktrace
Copy link
Contributor Author

staktrace commented Oct 16, 2018

Sure, if you're ok with me taking it out of the print_node code I can add it to this PR.

staktrace added 2 commits Oct 16, 2018
This member variable is written to immediately before being used,
in SpatialNode::update_transform, and so can just be turned into
a local variable. The only other place that uses this is just a debug
printout. Finally, since the member variable can be removed, we can
also remove code that exists solely to populate this member.
@staktrace staktrace force-pushed the staktrace:pinchzoom branch from 1d02be4 to 9c43de7 Oct 16, 2018
@staktrace
Copy link
Contributor Author

staktrace commented Oct 16, 2018

Patches updated ^

@kvark
kvark approved these changes Oct 16, 2018
Copy link
Member

kvark left a comment

:lgtm:

Reviewed 6 of 6 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@kvark
Copy link
Member

kvark commented Oct 16, 2018

@bors-servo delegate+
(leaving a chance for a try push, if needed)

@bors-servo
Copy link
Contributor

bors-servo commented Oct 16, 2018

✌️ @staktrace can now approve this pull request

@kvark
Copy link
Member

kvark commented Oct 16, 2018

Try isn't needed (as per IRC discussion).
@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Oct 16, 2018

📌 Commit 9c43de7 has been approved by kvark

@bors-servo
Copy link
Contributor

bors-servo commented Oct 16, 2018

Testing commit 9c43de7 with merge 9eda2df...

bors-servo added a commit that referenced this pull request Oct 16, 2018
Make the pinch-zoom API a frame message instead of a scene message.

Fixes #3202.

There might be other issues that are exposed by this, not sure yet. But it's a start.

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

bors-servo commented Oct 16, 2018

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: kvark
Pushing 9eda2df to master...

@bors-servo bors-servo merged commit 9c43de7 into servo:master Oct 16, 2018
4 checks passed
4 checks passed
Taskcluster (pull_request) TaskGroup: success
Details
code-review/reviewable 6 files reviewed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
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

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