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

Additions to CFURL and CFBundle #101

Merged
merged 4 commits into from May 23, 2017
Merged

Additions to CFURL and CFBundle #101

merged 4 commits into from May 23, 2017

Conversation

@mgoszcz2
Copy link
Contributor

mgoszcz2 commented May 22, 2017

I propose a few additions to CFBundle and CFURL APIs. Not sure how general they are, but they would be nice to have out of the box.

The CFURL::absolute_file_url test is a bit big, but it demonstrates the need for CFURL::absolute().

I think CFBundle::main_bundle() should return an Option since CFBundleGetMainBundle can return NULL but that is a breaking change and I decided not to include it.


This change is Reviewable

mgoszcz2 added 2 commits May 22, 2017
Also export CFURLCreateWithFileSystemPathRelativeToBase for testing
@jdm

This comment has been minimized.

Copy link

jdm commented on core-foundation/src/bundle.rs in 27cbbba May 22, 2017

executable

@jdm

This comment has been minimized.

Copy link

jdm commented on core-foundation/src/bundle.rs in 27cbbba May 22, 2017

Executable

@jdm
Copy link
Member

jdm commented May 22, 2017

Other than the spelling, this looks fine.

mgoszcz2 added 2 commits May 23, 2017
@jdm
Copy link
Member

jdm commented May 23, 2017

@bors-servo
Copy link
Contributor

bors-servo commented May 23, 2017

📌 Commit f279342 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented May 23, 2017

Testing commit f279342 with merge 18fadda...

bors-servo added a commit that referenced this pull request May 23, 2017
Additions to CFURL and CFBundle

I propose a few additions to CFBundle and CFURL APIs. Not sure how general they are, but they would be nice to have out of the box.

The `CFURL::absolute_file_url` test is a bit big, but it demonstrates the need for `CFURL::absolute()`.

I think `CFBundle::main_bundle()` should return an `Option` since `CFBundleGetMainBundle` can return NULL but that is a breaking change and I decided not to include it.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/core-foundation-rs/101)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented May 23, 2017

☀️ Test successful - status-travis
Approved by: jdm
Pushing 18fadda to master...

@bors-servo bors-servo merged commit f279342 into servo:master May 23, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@mgoszcz2
Copy link
Contributor Author

mgoszcz2 commented Jun 1, 2017

When will you be publishing the next version?

@jdm
Copy link
Member

jdm commented Jun 1, 2017

Could you make a PR that updates the minor version in Cargo.toml?

@jdm
Copy link
Member

jdm commented Jun 2, 2017

Published.

jdm pushed a commit that referenced this pull request Feb 1, 2018
Fix memory safety problem in `CGDataProvider::from_buffer()`.

The wrapper for that function made no attempt to ensure that the buffer
would stay alive, and as a result use-after-free could easily occur. The
fix is to require that the buffer be stuffed in an `Arc` box and to
teach Core Graphics how to release the `Arc` when it's done with it.

Related: #77. (This is basically a more minimal version of that PR. I had seen #77 but didn't realize that it was a memory safety problem! If you'd like to merge that PR instead, feel free.)

r? @jdm

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/core-graphics-rs/101)
<!-- Reviewable:end -->
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.