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

border: Get rid of BorderCornerClipSource. #3054

Merged
merged 1 commit into from Sep 13, 2018
Merged

border: Get rid of BorderCornerClipSource. #3054

merged 1 commit into from Sep 13, 2018

Conversation

@emilio
Copy link
Member

emilio commented Sep 13, 2018

Instead, just add functions to add the instances directly, avoiding the
intermediate allocation and deindenting a bunch of the code.

This should have no behavior change.


This change is Reviewable

@emilio
Copy link
Member Author

emilio commented Sep 13, 2018

r? @kvark or anyone else?

I'm sorry, github is terrible at flagging indentation changes. :(

@kvark
kvark approved these changes Sep 13, 2018
Copy link
Member

kvark left a comment

:lgtm:

a few notes, no blockers

Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @emilio)


webrender/src/border.rs, line 244 at r1 (raw file):

}

fn outer_and_clip_sign(

let's have it starting with a verb, since it's not an accessor method


webrender/src/border.rs, line 253 at r1 (raw file):

        BorderSegment::BottomRight => DeviceVector2D::new(1.0, 1.0),
        BorderSegment::BottomLeft => DeviceVector2D::new(0.0, 1.0),
        _ => unreachable!(),

panic would probably be more appropriate


webrender/src/border.rs, line 260 at r1 (raw file):

    );

    let clip_sign = DeviceVector2D::new(

nit: could use vector operations?


webrender/src/border.rs, line 282 at r1 (raw file):

        compute_half_dash(average_border_width, ellipse.total_arc_length);

    if num_half_dashes == 0 {

uuh, anticipating rebase issues in #3049

Instead, just add functions to add the instances directly, avoiding the
intermediate allocation and deindenting a bunch of the code.

This should have no behavior change.
@emilio emilio force-pushed the bye-clip-source branch from ed83d74 to 88353b7 Sep 13, 2018
@emilio
Copy link
Member Author

emilio commented Sep 13, 2018

nit: could use vector operations?

I couldn't do this: no implementation for {float} - euclid::TypedVector2D<f32, webrender_api::DevicePixel>

@emilio
Copy link
Member Author

emilio commented Sep 13, 2018

@bors-servo r=kvark

@bors-servo
Copy link
Contributor

bors-servo commented Sep 13, 2018

📌 Commit 88353b7 has been approved by kvark

@emilio
Copy link
Member Author

emilio commented Sep 13, 2018

(Rest of comments should be addressed)

@bors-servo
Copy link
Contributor

bors-servo commented Sep 13, 2018

Testing commit 88353b7 with merge 0f14252...

bors-servo added a commit that referenced this pull request Sep 13, 2018
border: Get rid of BorderCornerClipSource.

Instead, just add functions to add the instances directly, avoiding the
intermediate allocation and deindenting a bunch of the code.

This should have no behavior change.

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

bors-servo commented Sep 13, 2018

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

@bors-servo bors-servo merged commit 88353b7 into master Sep 13, 2018
5 of 6 checks passed
5 of 6 checks passed
code-review/reviewable 1 file, 4 discussions left (emilio, kvark)
Details
Taskcluster (pull_request) TaskGroup: success
Details
Taskcluster (push) TaskGroup: success
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@kvark kvark deleted the bye-clip-source branch Sep 13, 2018
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

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