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

Expose images to the BlobImageRenderer. #1018

Merged
merged 1 commit into from
Apr 26, 2017

Conversation

nical
Copy link
Contributor

@nical nical commented Mar 28, 2017

See issue #1017. The idea would be to do the same for fonts and paths.

r? @glennw


This change is Reviewable

@glennw
Copy link
Member

glennw commented Mar 28, 2017

This seems like a minimal change that doesn't affect other areas anyway. Just need to fix the compile errors on CI, then r=me.

@glennw glennw self-requested a review March 28, 2017 22:51
Copy link
Member

@glennw glennw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me once the CI compile error is fixed.

@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #1004) made this pull request unmergeable. Please resolve the merge conflicts.

@nical nical force-pushed the blob-ref-image branch 2 times, most recently from 8739743 to d2afb9b Compare March 29, 2017 16:08
@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #997) made this pull request unmergeable. Please resolve the merge conflicts.

@glennw
Copy link
Member

glennw commented Mar 30, 2017

@nical Needs another rebase.

@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #1160) made this pull request unmergeable. Please resolve the merge conflicts.

@nical
Copy link
Contributor Author

nical commented Apr 26, 2017

Can we land this? I have work in progress that I need to rebase on top of it.

@kvark
Copy link
Member

kvark commented Apr 26, 2017

@nical sure thing, but it's not mergable atm.

@nical
Copy link
Contributor Author

nical commented Apr 26, 2017

Woops I missed that. Rebased.

@@ -116,6 +116,40 @@ struct ImageResource {
dirty_rect: Option<DeviceUintRect>
}

struct ImageTemplates {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason for this to be a separate struct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason is the borrow checker. Without the separate struct the resource cache ends up being "entirely" borrowed when passed to the blob renderer which can't happen because we are already borrowing it partially (the blob renderer itself).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand. How does this change from the borrowck point of view if you just do type ImageTemplates = HashMap<...>?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as per IRC discussion, having a separate type allows us to implement ImageStore, which is needed to abstract away from BlobImageRenderer

@kvark
Copy link
Member

kvark commented Apr 26, 2017

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit a29dabc has been approved by kvark

@bors-servo
Copy link
Contributor

⌛ Testing commit a29dabc with merge fc9ee5b...

bors-servo pushed a commit that referenced this pull request Apr 26, 2017
Expose images to the BlobImageRenderer.

See issue #1017. The idea would be to do the same for fonts and paths.

r? @glennw

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

☀️ Test successful - status-travis
Approved by: kvark
Pushing fc9ee5b to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants