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

Add some factories for PrimitiveInfo #1708

Merged
merged 1 commit into from Sep 18, 2017
Merged

Add some factories for PrimitiveInfo #1708

merged 1 commit into from Sep 18, 2017

Conversation

@mrobinson
Copy link
Member

mrobinson commented Sep 15, 2017

This will make it easier to add new members in the future (for hit
testing). We also make local_clip a LocalClip instead of an
Option because this should never be None.


This change is Reviewable

@mrobinson mrobinson requested review from glennw and kvark Sep 15, 2017
Copy link
Member

kvark left a comment

Nice reduction in LOC! 👍

is_backface_visible: true,
};

let info = LayoutPrimitiveInfo::new_with_clip_rect(rect, bounds);

This comment has been minimized.

@kvark

kvark Sep 15, 2017

Member

should probably just be with_clip_rect?

This comment has been minimized.

@mrobinson

mrobinson Sep 18, 2017

Author Member

Okay. I'll make this change.

local_clip: Some(api::LocalClip::from(bounds)),
is_backface_visible: true,
};
let info = api::LayoutPrimitiveInfo::new((30, 30).by(500, 500));

This comment has been minimized.

@kvark

kvark Sep 15, 2017

Member

missing local clip?

This comment has been minimized.

@mrobinson

mrobinson Sep 18, 2017

Author Member

I'll fix this.

local_clip: Some(api::LocalClip::from(bounds)),
is_backface_visible: true,
};
let info = api::LayoutPrimitiveInfo::new((600, 600).by(200, 200));

This comment has been minimized.

@kvark

kvark Sep 15, 2017

Member

missing local clip?

This comment has been minimized.

@mrobinson

mrobinson Sep 18, 2017

Author Member

Ditto!

@bors-servo
Copy link
Contributor

bors-servo commented Sep 18, 2017

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

This will make it easier to add new members in the future (for hit
testing). We also make local_clip a LocalClip instead of an
Option<LocalClip> because this should never be None.
@mrobinson mrobinson force-pushed the mrobinson:cleanup branch from 6635e7e to 91a7b1f Sep 18, 2017
@mrobinson
Copy link
Member Author

mrobinson commented Sep 18, 2017

Okay. I've updated the branch with the changes and rebased it on top of the rustfmt changes.

@kvark
Copy link
Member

kvark commented Sep 18, 2017

Thanks @mrobinson , and sorry about the rebase pain!
@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Sep 18, 2017

📌 Commit 91a7b1f has been approved by kvark

bors-servo added a commit that referenced this pull request Sep 18, 2017
Add some factories for PrimitiveInfo

This will make it easier to add new members in the future (for hit
testing). We also make local_clip a LocalClip instead of an
Option<LocalClip> because this should never be None.

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

bors-servo commented Sep 18, 2017

Testing commit 91a7b1f with merge aabd093...

@bors-servo
Copy link
Contributor

bors-servo commented Sep 18, 2017

☀️ Test successful - status-travis
Approved by: kvark
Pushing aabd093 to master...

@bors-servo bors-servo merged commit 91a7b1f into servo:master Sep 18, 2017
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@staktrace
Copy link
Contributor

staktrace commented Sep 18, 2017

@mrobinson This broke the gecko build, please see https://bugzilla.mozilla.org/show_bug.cgi?id=1400216#c5 and provide a gecko-side patch. Thanks!

@mrobinson mrobinson deleted the mrobinson:cleanup branch Sep 19, 2017
@mrobinson
Copy link
Member Author

mrobinson commented Sep 19, 2017

@staktrace Thanks for the heads up. I've posted a patch to Bugzilla which should fix the build.

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.