Enabled use of FontFamily enum type #8420

craftytrickster · 2015-11-09T01:18:18Z

In addition to replacing loose strings with the FontFamily enum in font_cache_task.rs, I also centralized the add_generic_font calls into one single function. If centralizing into one function is not desired or if anything else needs to be changed, please let me know.

highfive · 2015-11-09T01:18:23Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @glennw (or someone else) soon.

SimonSapin · 2015-11-11T22:26:21Z

While the style changes look ok (modulo one issue), this doesn’t do the more important part of treating "serif" and serif differently in font code.

I really don’t know our font code at all, so I’d rather that someone who does suggests what to do here. @glennw maybe?

Reviewed 1 of 2 files at r1.
Review status: 1 of 2 files reviewed at latest revision, 4 unresolved discussions.

^{components/gfx/font_cache_task.rs, line 107 [r1] (raw file):}
Nit: remove vec! here to avoid allocating memory. (Use a bare […] array.)

^{components/gfx/font_cache_task.rs, line 119 [r1] (raw file):}
With the vec to array change above, replace these three lines with font (ref font_family, mapped_name) in &font_pairs {

^{components/style/properties.mako.rs, line 1722 [r1] (raw file):}
This method is suspicious: it makes e.g. FontFamily::Serif indistinguishable from FontFamily::FamilyName("serif") which is what this enum is about in the first place. I’d rather remove it, but I don’t know how to update the code that uses it.

^{components/style/properties.mako.rs, line 1735 [r1] (raw file):}
Please replace this with:

                    match *self {
                        FontFamily::FamilyName(ref name) => ::cssparser::serialize_string(name, dest),
                        FontFamily::Serif => dest.write_str("serif"),
                        FontFamily::SansSerif => dest.write_str("sans-serif"),
                        FontFamily::Cursive => dest.write_str("cursive"),
                        FontFamily::Fantasy => dest.write_str("fantasy"),
                        FontFamily::Monospace => dest.write_str("monospace"),
                    }

… which writes a quoted string for non-generic names.

Comments from the review on Reviewable.io

craftytrickster · 2015-11-12T00:22:50Z

^{components/gfx/font_cache_task.rs, line 107 [r1] (raw file):}
Good call. I've been writing too much JavaScript lately where you don't have to think about heap/stack allocations.

Comments from the review on Reviewable.io

glennw · 2015-11-12T05:02:40Z

@SimonSapin I may be missing the context here, but it sounds like the font cache should take the FontFamily enum as input in its public APIs (rather than a string as it does now). Once this is changed, the internal font cache code could be updated to distinguish between them, by only transforming the font name to a generic one when the specified type is not a FontFamily::FamilyName(Atom).

Does that sound right to you?

glennw · 2015-11-23T02:08:06Z

@SimonSapin ping

SimonSapin · 2015-11-23T14:01:02Z

@glennw I don’t know anything about our font code beyond parsing in the style crate, but what you describe sounds about right.

Fixing this should make tests/wpt/css-tests/css21_dev/html4/font-family-rule-012.htm pass, assuming #8374 also lands. That PR make AHEM_serif.ttf available to tests. The name field of that font is serif, so it should match font-family: "serif", but not font-family: serif which should be a UA-chosen default.

bors-servo · 2015-11-25T19:10:12Z

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

craftytrickster · 2015-11-25T19:13:11Z

Should I resolve the merge conflicts or should I close the PR?

jdm · 2015-11-25T19:18:47Z

There shouldn't be any need to close this PR. Any changes to address the previous comments and the merge conflicts can be force pushed to the same branch.

craftytrickster · 2015-11-29T21:08:26Z

Added correct email address to commits

glennw · 2015-12-03T06:18:05Z

components/gfx/font_cache_task.rs

                                                -> Option<Arc<FontTemplateData>> {

        let (response_chan, response_port) = ipc::channel().unwrap();
-        self.chan.send(Command::GetFontTemplate(family, desc, response_chan)).unwrap();
+        self.chan.send(Command::GetFontTemplate(family.name().to_owned(), desc, response_chan)).unwrap();


Since this converts the family to a string here, I don't see how it can differentiate between the enum value and 'serif' as a string when selecting which font to return?

When I did this a while ago, I originally tried to pass around the enum FontFamily version instead of using a string, but I ran into problems with some functions that are being called from another module that depend on using a string (they were unsafe blocks that depended on c-like strings). I will look around again to see if there is anything I can do, and I will post my findings.

craftytrickster · 2015-12-05T15:48:17Z

Please let me know if anything else is missing.

glennw · 2015-12-07T15:07:50Z

I ran the WPT tests on this PR and got 33 test failures - they seem to be related to our mozilla tests that use the css/ahem.css file.

craftytrickster · 2015-12-16T23:11:29Z

components/style/properties.mako.rs

+                        super::CURSIVE => FontFamily::Cursive,
+                        super::FANTASY => FontFamily::Fantasy,
+                        super::MONOSPACE => FontFamily::Monospace
+                        _ => FontFamily::FamilyName(input.clone())
                    }


Should I do this instead in order to avoid a clone?

pub fn from_atom(input: Atom) -> FontFamily { let option = match_ignore_ascii_case! { &input, super::SERIF => Some(FontFamily::Serif), super::SANS_SERIF => Some(FontFamily::SansSerif), super::CURSIVE => Some(FontFamily::Cursive), super::FANTASY => Some(FontFamily::Fantasy), super::MONOSPACE => Some(FontFamily::Monospace) _ => None }; match option { Some(family) => family, None => FontFamily::FamilyName(input) } }

Cloning an atom isn't very expensive (it's atomically reference counted), so it's probably fine. @SimonSapin is there a better way to do this while using that macro?

It's not expensive, but we have the opportunity to not clone here, so let's do it I guess?

craftytrickster · 2015-12-16T23:30:48Z

components/gfx/font_cache_task.rs

@@ -198,15 +218,15 @@ impl FontCache {
        for_each_available_family(|family_name| {
            let family_name = LowercaseString::new(&family_name);
            if !self.local_families.contains_key(&family_name) {


It seems inefficient to create a new LowercaseString, just for the purpose of performing a contains_key check. Is there a better way to do this?

It's only done when the local families are refreshed (which is only on application startup at the moment), so I wouldn't worry about it.

glennw · 2015-12-17T01:32:44Z

Thanks for the changes. One minor question for Simon to reply with, after that it looks good to me once squashed.

craftytrickster · 2015-12-17T02:48:31Z

Thank you for the review.

bors-servo · 2015-12-18T21:29:36Z

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

nox · 2015-12-30T18:32:13Z

@craftytrickster Fix that clone and I'll r=glennw.


        Enabled use of FontFamily enum type and replaced plain string paramet…

…ers with enum

craftytrickster · 2015-12-30T23:22:59Z

@nox thanks for checking in on this, I made the change as requested.

nox · 2015-12-30T23:25:34Z

@bors-servo r=glennw

Reviewed 2 of 4 files at r5, 3 of 3 files at r6.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

Comments from the review on Reviewable.io

bors-servo · 2015-12-30T23:25:35Z

📌 Commit d942bfb has been approved by glennw

bors-servo · 2015-12-31T10:25:33Z

⌛ Testing commit d942bfb with merge a1d7a33...


        Auto merge of #8420 - craftytrickster:8371/generic-font-family, r=glennw

Enabled use of FontFamily enum type #8371 In addition to replacing loose strings with the FontFamily enum in `font_cache_task.rs`, I also centralized the add_generic_font calls into one single function. If centralizing into one function is not desired or if anything else needs to be changed, please let me know.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8420)

bors-servo · 2015-12-31T10:26:16Z

⛄ The build was interrupted to prioritize another pull request.

bors-servo · 2015-12-31T11:19:49Z

⌛ Testing commit d942bfb with merge 66c8aa8...


        Auto merge of #8420 - craftytrickster:8371/generic-font-family, r=glennw

Enabled use of FontFamily enum type #8371 In addition to replacing loose strings with the FontFamily enum in `font_cache_task.rs`, I also centralized the add_generic_font calls into one single function. If centralizing into one function is not desired or if anything else needs to be changed, please let me know.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8420)

bors-servo · 2015-12-31T12:13:12Z

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt

highfive added the S-awaiting-review label Nov 9, 2015

jdm assigned SimonSapin Nov 11, 2015

SimonSapin assigned glennw and unassigned SimonSapin Nov 11, 2015

highfive added S-needs-rebase and removed S-awaiting-merge labels Nov 25, 2015

jdm removed the S-awaiting-review label Nov 25, 2015

craftytrickster force-pushed the craftytrickster:8371/generic-font-family branch from 54cc7b7 to c3cf40a Nov 26, 2015

highfive added S-awaiting-review and removed S-needs-rebase labels Nov 26, 2015

craftytrickster force-pushed the craftytrickster:8371/generic-font-family branch from 3a66914 to ac38845 Nov 29, 2015

glennw reviewed Dec 3, 2015
View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Dec 3, 2015

craftytrickster force-pushed the craftytrickster:8371/generic-font-family branch from ac38845 to 4b667af Dec 5, 2015

highfive added S-awaiting-review and removed S-needs-code-changes labels Dec 5, 2015

craftytrickster force-pushed the craftytrickster:8371/generic-font-family branch from 4b667af to 8c08b7b Dec 10, 2015

craftytrickster reviewed Dec 16, 2015
View changes

craftytrickster force-pushed the craftytrickster:8371/generic-font-family branch from 0b6fcf5 to e536883 Dec 17, 2015

highfive added the S-needs-rebase label Dec 18, 2015

craftytrickster force-pushed the craftytrickster:8371/generic-font-family branch from e536883 to d2dfe3d Dec 19, 2015

nox removed the S-needs-rebase label Dec 30, 2015

jdm added S-needs-code-changes and removed S-awaiting-review labels Dec 30, 2015

Enabled use of FontFamily enum type and replaced plain string paramet…

Loading status checks…

d942bfb

…ers with enum

craftytrickster force-pushed the craftytrickster:8371/generic-font-family branch from d2dfe3d to d942bfb Dec 30, 2015

highfive added S-awaiting-review and removed S-needs-code-changes labels Dec 30, 2015

highfive added S-awaiting-merge and removed S-awaiting-review labels Dec 30, 2015

bors-servo mentioned this pull request Dec 31, 2015

Add resize handler to global events and to macro to add event handler functions GetOnresize, SetOnresize #8006

Closed

bors-servo mentioned this pull request Dec 31, 2015

Implemementing context-based MIME type sniffing #8190

Merged

servo / servo

Enabled use of FontFamily enum type #8420

Enabled use of FontFamily enum type #8420

craftytrickster commented Nov 9, 2015

highfive commented Nov 9, 2015

SimonSapin commented Nov 11, 2015

craftytrickster commented Nov 12, 2015

glennw commented Nov 12, 2015

glennw commented Nov 23, 2015

SimonSapin commented Nov 23, 2015

bors-servo commented Nov 25, 2015

craftytrickster commented Nov 25, 2015

jdm commented Nov 25, 2015

craftytrickster commented Nov 29, 2015

This comment has been minimized.

This comment has been minimized.

craftytrickster commented Dec 5, 2015

glennw commented Dec 7, 2015

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

glennw commented Dec 17, 2015

craftytrickster commented Dec 17, 2015

bors-servo commented Dec 18, 2015

nox commented Dec 30, 2015

craftytrickster commented Dec 30, 2015

nox commented Dec 30, 2015

bors-servo commented Dec 30, 2015

bors-servo commented Dec 31, 2015

bors-servo commented Dec 31, 2015

bors-servo commented Dec 31, 2015

bors-servo commented Dec 31, 2015

servo / servo

Join GitHub today

Enabled use of FontFamily enum type #8420

Enabled use of FontFamily enum type #8420

Conversation

craftytrickster commented Nov 9, 2015

highfive commented Nov 9, 2015

SimonSapin commented Nov 11, 2015

craftytrickster commented Nov 12, 2015

glennw commented Nov 12, 2015

glennw commented Nov 23, 2015

SimonSapin commented Nov 23, 2015

bors-servo commented Nov 25, 2015

craftytrickster commented Nov 25, 2015

jdm commented Nov 25, 2015

craftytrickster commented Nov 29, 2015

This comment has been minimized.

glennw Dec 3, 2015

This comment has been minimized.

craftytrickster Dec 4, 2015

craftytrickster commented Dec 5, 2015

glennw commented Dec 7, 2015

This comment has been minimized.

craftytrickster Dec 16, 2015

This comment has been minimized.

glennw Dec 17, 2015

This comment has been minimized.

nox Dec 30, 2015

This comment has been minimized.

craftytrickster Dec 16, 2015

This comment has been minimized.

glennw Dec 17, 2015

glennw commented Dec 17, 2015

craftytrickster commented Dec 17, 2015

bors-servo commented Dec 18, 2015

nox commented Dec 30, 2015

craftytrickster commented Dec 30, 2015

nox commented Dec 30, 2015

bors-servo commented Dec 30, 2015

bors-servo commented Dec 31, 2015

bors-servo commented Dec 31, 2015

bors-servo commented Dec 31, 2015

bors-servo commented Dec 31, 2015