Skip to content

/ webrender

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

Switch AlphaBatchKeyFlags to use bitflags! crate. #539

Closed
wants to merge 5 commits into from
Closed

Switch AlphaBatchKeyFlags to use bitflags! crate. #539

wants to merge 5 commits into from

Conversation

@moriturus
Copy link
Contributor

moriturus commented Nov 8, 2016 •
edited by larsbergstrom

#509

This change is Reviewable
@moriturus
Switch AlphaBatchKeyFlags to use bitflags! crate. #509
Loading status checks…
7ca8639
@emilio
emilio reviewed Nov 8, 2016
View changes
Copy link
Member

emilio left a comment

looks pretty good! :)
webrender/src/tiling.rs Outdated
pub flags AlphaBatchKeyFlags: u8 {
const AXIS_ALIGNED = 0b00000010,
const COMPLEX = 0b00000100,
const NEEDS_CLIPPLING = 0b00000001,

This comment has been minimized.

Sign in to view
@emilio

emilio Nov 8, 2016

Member

Maybe sorting them from smallest to largest?
webrender/src/tiling.rs Outdated

impl AlphaBatchKeyFlags {
fn new(transform_kind: TransformedRectKind,
needs_clipping: bool) -> AlphaBatchKeyFlags {

This comment has been minimized.

Sign in to view
@emilio

emilio Nov 8, 2016

Member

nit: align the arguments, please :)

This comment has been minimized.

Sign in to view
@emilio

emilio Nov 8, 2016

Member

Though now seems easier to just use the literals and or them as you've done below, so we might as well get rid of this function entirely.
@mstange
mstange reviewed Nov 8, 2016
View changes
webrender/src/tiling.rs Outdated
pub flags AlphaBatchKeyFlags: u8 {
const AXIS_ALIGNED = 0b00000010,
const COMPLEX = 0b00000100,
const NEEDS_CLIPPLING = 0b00000001,

This comment has been minimized.

Sign in to view
@mstange

mstange Nov 8, 2016

Contributor

There's an extra L in the word CLIPPLING. It should be CLIPPING.
changm and others added 2 commits Nov 8, 2016
@changm
add stride data as an optional field to upload textures
Loading status checks…
1ae6d0e
@moriturus
fix typo, sort AlphaBatchKeyFlags constants and remove AlphaBatchKeyF…
Loading status checks…
47d702b 
…lags::new
@glennw
glennw reviewed Nov 8, 2016
View changes
Copy link
Member

glennw left a comment

Thanks! Github says there are conflicts in this file, so it will need to be rebased. After rebasing and fixing the one remaining comment, we'll get this merged.
webrender/src/tiling.rs
pub flags AlphaBatchKeyFlags: u8 {
const NEEDS_CLIPPING = 0b00000001,
const AXIS_ALIGNED = 0b00000010,
const COMPLEX = 0b00000100,

This comment has been minimized.

Sign in to view
@glennw

glennw Nov 8, 2016

Member

I think we can remove the COMPLEX flag altogether, since it's mutually exclusive with AXIS_ALIGNED and isn't actually used.

This comment has been minimized.

Sign in to view
@moriturus

moriturus Nov 8, 2016

Author Contributor

https://github.com/moriturus/webrender/blob/6ae1e2a4e4d6c111d177b32d4f54b2273d95f63d/webrender/src/tiling.rs#L440-L443
TheCOMPLEX flag is used at above lines. Will TransformedRectKind::Complex never be used?

This comment has been minimized.

Sign in to view
@glennw

glennw Nov 8, 2016

Member

The COMPLEX flag is set there, but it's never actually tested against - the fn transform_kind() function just checks the valule of AXIS_ALIGNED (since the presence of one implies the absence of the other). Something seems to have gotten confused with the merge / rebase too - it looks like there is part of another commit in this PR now?
bors-servo and others added 2 commits Nov 8, 2016
@bors-servo
Auto merge of #538 - changm:stride, r=glennw
Loading status checks…
72d5c86 
add stride data as an optional field to upload textures

Thanks for the idea of using option! That worked quite well :)

<!-- 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/538)
<!-- Reviewable:end -->
@moriturus
Merge branch 'auto' into use_bitflags_crate
Loading status checks…
6ae1e2a 
# Conflicts:
#	webrender/src/tiling.rs
@moriturus moriturus closed this Nov 8, 2016
@moriturus moriturus deleted the moriturus:use_bitflags_crate branch Nov 8, 2016
@moriturus
Copy link
Contributor Author

moriturus commented Nov 8, 2016 •
edited

Oops, I accidentally remove this PR's branch. I will re-open my PR.
@moriturus moriturus mentioned this pull request Nov 8, 2016
Merged
bors-servo added a commit that referenced this pull request Nov 9, 2016
@bors-servo
Auto merge of #541 - moriturus:use_bitflags_crate, r=glennw
Loading status checks…
0ec2eb7 
Switch AlphaBatchKeyFlags to use bitflags! crate.

Sorry about my mistake on #539 then I re-open this PR.

<!-- 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/541)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mstange mstange

@emilio emilio

@glennw glennw

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
@moriturus @mstange @emilio @glennw @changm @bors-servo
You can’t perform that action at this time.