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

No longer allow clipping items by complex clips #2512

Merged
merged 1 commit into from Mar 15, 2018

Conversation

mrobinson
Copy link
Member

@mrobinson mrobinson commented Mar 13, 2018

This is part of an API simplification which should hopefully lead to
less code complexity in the future.

Fixes #1742.


This change is Reviewable

@mrobinson mrobinson requested review from kvark and glennw March 13, 2018 09:15
@mrobinson
Copy link
Member Author

Gecko try job is here: #2512

@mrobinson mrobinson force-pushed the remove-complexclip-from-item branch from 153bb8b to ff8750b Compare March 13, 2018 09:39
@staktrace
Copy link
Contributor

@bors-servo
Copy link
Contributor

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

@mrobinson mrobinson force-pushed the remove-complexclip-from-item branch from ff8750b to 1828236 Compare March 13, 2018 17:25
@mrobinson
Copy link
Member Author

@staktrace Thanks for provided the correct link. I was in a bit of a rush this morning.

The branch should be rebased and ready to review now.

@glennw
Copy link
Member

glennw commented Mar 13, 2018

Is there a Gecko / Servo patch you could post here for the API update, that we can use while doing try runs in the interim?

@kvark
Copy link
Member

kvark commented Mar 13, 2018

Looks great! A few questions, otherwise 🚢 it


Reviewed 12 of 12 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


direct-composition/src/main_windows.rs, line 135 at r1 (raw file):

        let rect = euclid::TypedRect::new(euclid::TypedPoint2D::zero(), layout_size);
        builder.push_rect(
            &api::PrimitiveInfo::with_clip(

any reason we aren't doing push_clip_id with this complex clip?


wrench/src/rawtest.rs, line 606 at r1 (raw file):

        // Add a rounded 100x100 rectangle at 200,0.
        let rect = LayoutRect::new(LayoutPoint::new(200., 0.), LayoutSize::new(100., 100.));

any reason this is removed?


Comments from Reviewable

@mrobinson
Copy link
Member Author

Review status: all files reviewed at latest revision, 2 unresolved discussions.


direct-composition/src/main_windows.rs, line 135 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

any reason we aren't doing push_clip_id with this complex clip?

It looked like the rounded rectangle here wasn't important for the example, but I can add it back. I didn't want to make too many changes here because I don't have a Windows environment ready for testing. I can spend a little time getting the rendered content back to its original state though.


wrench/src/rawtest.rs, line 606 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

any reason this is removed?

I added this one to explicitly test items with their own rounded rectangle local clips. Since I removed that feature, I also removed the test for it. Directly below is the same test, but using separate clipping nodes.


Comments from Reviewable

@mrobinson mrobinson force-pushed the remove-complexclip-from-item branch from 1828236 to 55afba7 Compare March 14, 2018 09:29
This is part of an API simplification which should hopefully lead to
less code complexity in the future.

Fixes servo#1742.
@mrobinson mrobinson force-pushed the remove-complexclip-from-item branch from 55afba7 to d398096 Compare March 14, 2018 09:56
@mrobinson
Copy link
Member Author

@glennw I don't think there are any code changes necessary for Gecko (it always uses the PrimitiveInfo constructors) and here is a branch with the changes necessary for Servo: https://github.com/mrobinson/servo/tree/changes-for-localclip-changes. I haven't tested this one, but I'm fairly certain there is no change in behavior.

@kvark
Copy link
Member

kvark commented Mar 14, 2018

@mrobinson looks like this is ready to go?

@mrobinson
Copy link
Member Author

@kvark Yep. Is it okay to land it?

@kvark
Copy link
Member

kvark commented Mar 14, 2018

@mrobinson well, the try looks good, code is approved by me, and Gecko doesn't (seemingly?) appear to need any changes. Shipit!

@mrobinson
Copy link
Member Author

@kvark Thanks!

@bors-servo r=kvark

@bors-servo
Copy link
Contributor

📌 Commit d398096 has been approved by kvark

@bors-servo
Copy link
Contributor

⌛ Testing commit d398096 with merge 0291c44...

bors-servo pushed a commit that referenced this pull request Mar 14, 2018
No longer allow clipping items by complex clips

This is part of an API simplification which should hopefully lead to
less code complexity in the future.

Fixes #1742.

<!-- 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/2512)
<!-- Reviewable:end -->
@glennw glennw closed this Mar 15, 2018
@glennw glennw reopened this Mar 15, 2018
@glennw
Copy link
Member

glennw commented Mar 15, 2018

@bors-servo retry

@bors-servo
Copy link
Contributor

⌛ Testing commit d398096 with merge 45a3b7b...

bors-servo pushed a commit that referenced this pull request Mar 15, 2018
No longer allow clipping items by complex clips

This is part of an API simplification which should hopefully lead to
less code complexity in the future.

Fixes #1742.

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

💔 Test failed - status-travis

@glennw
Copy link
Member

glennw commented Mar 15, 2018

Blocked by #2522.

@mrobinson
Copy link
Member Author

@bors-servo retry

bors-servo pushed a commit that referenced this pull request Mar 15, 2018
No longer allow clipping items by complex clips

This is part of an API simplification which should hopefully lead to
less code complexity in the future.

Fixes #1742.

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

⌛ Testing commit d398096 with merge e0fbae7...

@bors-servo
Copy link
Contributor

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

@bors-servo bors-servo merged commit d398096 into servo:master Mar 15, 2018
@mrobinson mrobinson deleted the remove-complexclip-from-item branch March 15, 2018 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants