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

Support serializing NativeFontHandle on macOS #2122

Merged
merged 2 commits into from Dec 7, 2017
Merged

Conversation

@jrmuizel
Copy link
Contributor

jrmuizel commented Nov 28, 2017

This fixes #2051


This change is Reviewable

@bors-servo
Copy link
Contributor

bors-servo commented Nov 28, 2017

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

@jrmuizel jrmuizel force-pushed the jrmuizel:font-data branch from 83900ec to 2e795dc Nov 29, 2017
@glennw
Copy link
Member

glennw commented Nov 29, 2017

Looks like CI failures on Windows at least?

@jrmuizel jrmuizel force-pushed the jrmuizel:font-data branch from 2e795dc to 45d0716 Nov 29, 2017
@bors-servo
Copy link
Contributor

bors-servo commented Nov 29, 2017

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

@jrmuizel jrmuizel force-pushed the jrmuizel:font-data branch 3 times, most recently from 52d09ae to 9517fe9 Nov 30, 2017
@jrmuizel
Copy link
Contributor Author

jrmuizel commented Nov 30, 2017

I just realized this ends up writing out a new font every time it's used

@jrmuizel jrmuizel force-pushed the jrmuizel:font-data branch 2 times, most recently from c04fb74 to 0cc1218 Dec 5, 2017
@glennw
Copy link
Member

glennw commented Dec 5, 2017

@jrmuizel Is this ready for review now?

@jrmuizel
Copy link
Contributor Author

jrmuizel commented Dec 5, 2017

Yes.

fn calc_table_checksum<D: Read>(mut data: D) -> u32 {
let mut sum: u32 = 0;
loop {
if let Ok(x) = data.read_u32::<BigEndian>() {

This comment has been minimized.

@kvark

kvark Dec 6, 2017

Member

while let Ok(x) = ... {...}

}

fn max_pow_2_less_than(a: u16) -> u16
{

This comment has been minimized.

@kvark

kvark Dec 6, 2017

Member

nit: opening bracket

while (x << (shift + 1)) < a {
shift += 1;
}
return shift;

This comment has been minimized.

@kvark

kvark Dec 6, 2017

Member

nit: return

offset += 4 * 3;
offset += 4 * 4 * (count as u32);
for tag in tags.iter() {
if tag == 0x43464620 { // 'CFF '

This comment has been minimized.

@kvark

kvark Dec 6, 2017

Member

nit: could be const thing for clarity

@@ -217,25 +217,49 @@ fn write_sc(parent: &mut Table, sc: &StackingContext, properties: &SceneProperti
}

#[cfg(target_os = "windows")]
fn native_font_handle_to_yaml(handle: &NativeFontHandle, parent: &mut yaml_rust::yaml::Hash) {
fn native_font_handle_to_yaml(_rsrc: &mut ResourceGenerator,
handle: &NativeFontHandle,

This comment has been minimized.

@kvark

kvark Dec 6, 2017

Member

nit: formatting

handle: &NativeFontHandle,
parent: &mut yaml_rust::yaml::Hash,
path_opt: &mut Option<PathBuf>) {
let path = if let &mut Some(ref path) = path_opt {

This comment has been minimized.

@kvark

kvark Dec 6, 2017

Member

nit: use match *path_opt

}

impl ResourceGenerator {
fn next_rsrc_paths(&mut self, base: &str, ext: &str, ) -> (PathBuf, PathBuf) {

This comment has been minimized.

@kvark

kvark Dec 6, 2017

Member

nit: trailing comma?

jrmuizel added 2 commits Nov 27, 2017
This cleans up things a little and will make it easier for us to generate
files for native fonts because we won't have multiple mutable borrows on
YamlFrameWriter.
This ups the core-graphics dependency and adds the ability to
convert a CGFont into a font file. The code is based on the code in
https://searchfox.org/mozilla-central/rev/7a8c667bdd2a4a32746c9862356e199627c0896d/gfx/2d/ScaledFontMac.cpp#255

We also switch to CFNumber::from to avoid breakage with the latest core-foundation
@jrmuizel jrmuizel force-pushed the jrmuizel:font-data branch from 0cc1218 to 7bd05c0 Dec 6, 2017
@glennw
Copy link
Member

glennw commented Dec 7, 2017

I'm just going to assume that the code in cgfont_to_data makes sense. I guess it's based on some existing code? If so, it's probably worth adding a comment in the code referencing where it's ported from.

@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Dec 7, 2017

📌 Commit 7bd05c0 has been approved by glennw

bors-servo added a commit that referenced this pull request Dec 7, 2017
Support serializing NativeFontHandle on macOS

This fixes #2051

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

bors-servo commented Dec 7, 2017

Testing commit 7bd05c0 with merge f6d91e0...

@bors-servo
Copy link
Contributor

bors-servo commented Dec 7, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: glennw
Pushing f6d91e0 to master...

@bors-servo bors-servo merged commit 7bd05c0 into servo:master Dec 7, 2017
4 checks passed
4 checks passed
Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
bors-servo added a commit that referenced this pull request Dec 7, 2017
Force corefoundation 0.4.6 or higher since we use CFNumber::from whic…

…h didn't exist in 0.4.4

The use of CFNumber::from was added in #2122 and caused downstream gecko bustage since gecko is still using 0.4.4 in its Cargo.lock file. This change will force gecko to update to 0.4.6.

<!-- 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/2190)
<!-- 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.

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