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

Remove redundant code, now that only local clips are segmented. #2888

Merged
merged 1 commit into from Jul 12, 2018

Conversation

@gw3583
Copy link
Collaborator

gw3583 commented Jul 12, 2018

As of #2859, this code path can never be executed. Removing it
also simplifies some of the code I'm working on for local space
picture rasterization.


This change is Reviewable

As of #2859, this code path can never be executed. Removing it
also simplifies some of the code I'm working on for local space
picture rasterization.
@mrobinson
Copy link
Member

mrobinson commented Jul 12, 2018

@bors-servo
Copy link
Contributor

bors-servo commented Jul 12, 2018

📌 Commit 9f011c9 has been approved by mrobinson

@bors-servo
Copy link
Contributor

bors-servo commented Jul 12, 2018

Testing commit 9f011c9 with merge e600bfe...

bors-servo added a commit that referenced this pull request Jul 12, 2018
Remove redundant code, now that only local clips are segmented.

As of #2859, this code path can never be executed. Removing it
also simplifies some of the code I'm working on for local space
picture rasterization.

<!-- 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/2888)
<!-- Reviewable:end -->
@gw3583
Copy link
Collaborator Author

gw3583 commented Jul 12, 2018

Holy cow, that's a lot of orange on the try run! @staktrace Is this a known issue on try runs right now, or did I somehow manage to break things?

@gw3583
Copy link
Collaborator Author

gw3583 commented Jul 12, 2018

A lot of the errors are:

'internal error: entered unreachable code: Got a non Clip ClipId for root reference frame.', gfx/webrender_bindings/src/bindings.rs:2444:14

The code I've changed here is clip related, but I can't see how it would cause that. Maybe the bindings are out of sync or something like that?

@gw3583
Copy link
Collaborator Author

gw3583 commented Jul 12, 2018

Ah, I think the following code in bindings.rs probably needs to be updated after #2871 and #2883.

#[no_mangle]
pub extern "C" fn wr_root_scroll_node_id() -> usize {
    // The PipelineId doesn't matter here, since we just want the numeric part of the id
    // produced for any given root reference frame.
    match ClipId::root_scroll_node(PipelineId(0, 0)) {
        ClipId::Clip(id, _) => id,
        _ => unreachable!("Got a non Clip ClipId for root reference frame."),
    }
}

Is that right @mrobinson ?

@mrobinson
Copy link
Member

mrobinson commented Jul 12, 2018

@gw3583 The changes necessary on the Gecko side for splitting the CipScrollTree were actually non-trivial. Do you mind applying the commit from my try job [1] for that PR and then trying the try job again?

  1. https://hg.mozilla.org/try/rev/33639cd81873baf1c23e6e061900ea3433e4fec1
@bors-servo
Copy link
Contributor

bors-servo commented Jul 12, 2018

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: mrobinson
Pushing e600bfe to master...

@bors-servo bors-servo merged commit 9f011c9 into servo:master Jul 12, 2018
3 checks passed
3 checks passed
Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@gw3583
Copy link
Collaborator Author

gw3583 commented Jul 12, 2018

@mrobinson Ah, sure - I'll apply that patch and do a new try run. Thanks!

@gw3583
Copy link
Collaborator Author

gw3583 commented Jul 12, 2018

@mrobinson
Copy link
Member

mrobinson commented Jul 13, 2018

The try job look pretty good to me. There are a couple crashes, but I'm not sure that they are related.

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.