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

Return the clip id from push_clip_node. #1333

Closed
wants to merge 1 commit into from
Closed

Conversation

@staktrace
Copy link
Contributor

staktrace commented Jun 2, 2017

In cases where the caller doesn't pass in an explicit clip id, it is
still useful for the caller to get back the generated clip id. This is
because the caller may later need to call push_clip_and_scroll_info with
the clip id that was generated. If this function doesn't return the clip
id the caller would basically have to inline this function and define
the clip manually, which seems much less convenient.


This change is Reviewable

In cases where the caller doesn't pass in an explicit clip id, it is
still useful for the caller to get back the generated clip id. This is
because the caller may later need to call push_clip_and_scroll_info with
the clip id that was generated. If this function doesn't return the clip
id the caller would basically have to inline this function and define
the clip manually, which seems much less convenient.
@staktrace
Copy link
Contributor Author

staktrace commented Jun 2, 2017

@kvark
Copy link
Member

kvark commented Jun 2, 2017

Technically, a breaking change. Unlikely to break anyone though.
@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Jun 2, 2017

📌 Commit e3becad has been approved by kvark

@bors-servo
Copy link
Contributor

bors-servo commented Jun 2, 2017

Testing commit e3becad with merge d1f4b54...

bors-servo added a commit that referenced this pull request Jun 2, 2017
Return the clip id from push_clip_node.

In cases where the caller doesn't pass in an explicit clip id, it is
still useful for the caller to get back the generated clip id. This is
because the caller may later need to call push_clip_and_scroll_info with
the clip id that was generated. If this function doesn't return the clip
id the caller would basically have to inline this function and define
the clip manually, which seems much less convenient.

<!-- 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/1333)
<!-- Reviewable:end -->
@mrobinson
Copy link
Member

mrobinson commented Jun 2, 2017

@staktrace Hrm. I originally thought of push_clip_node as a way to keep backward compatibility until everyone moved to define_clip and pushing it manually. Is there any way to Gecko can switch to this approach? Eventually, I think the clip stack might go away entirely.

@kvark
Copy link
Member

kvark commented Jun 2, 2017

@mrobinson if you don't feel good about this change, please r-

@staktrace
Copy link
Contributor Author

staktrace commented Jun 2, 2017

@mrobinson yeah we can stop using push_clip_node if that's the preferred way forward. I thought it was here to stay as a nice convenience function.

@staktrace
Copy link
Contributor Author

staktrace commented Jun 2, 2017

(Aside: is there a way to mark functions as deprecated in rust? If not even some inline comments to that effect would be good here)

@kvark
Copy link
Member

kvark commented Jun 2, 2017

@kvark
Copy link
Member

kvark commented Jun 2, 2017

@staktrace yeah,

#[deprecated]
fn foo() {}

fn main() {
    foo();
}
@kvark kvark closed this Jun 2, 2017
@mrobinson
Copy link
Member

mrobinson commented Jun 2, 2017

@kvark @staktrace In the end, I didn't r- only because the clipping API is moving a bit slowly in order to make sure that it meets the needs of Gecko. If the push_clip_node API is very useful, I am happy to adapt the to that style. On the other hand, if the define_clip approach works, then let's stick with that. 😄

@staktrace staktrace deleted the staktrace:clip branch Jun 2, 2017
@kvark
Copy link
Member

kvark commented Jun 2, 2017

@mrobinson ok, let's re-open if @staktrace has any issues with define_clip

@staktrace
Copy link
Contributor Author

staktrace commented Jun 2, 2017

Yeah, no problem with using define_clip. push_clip_node is pretty small and trivial to inline into gecko where needed.

@mrobinson
Copy link
Member

mrobinson commented Jun 2, 2017

@staktrace Thanks for the info. I'm tracking the removal here: #1335

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.