auto merge of #3697 : pcwalton/servo/get-layout-font-group, r=glennw

Seems to be a 38% layout win on a site I tested with a lot of text.

Other browser engines typically do not duplicate the information in the font style struct. `FontStyle` actually predates @SimonSapin's CSS selector matching library. It's time to get rid of it!

r? @glennw

6 files changed, 52 insertions, 87 deletions

diff --git a/components/gfx/font.rs b/components/gfx/font.rs
index b3e30b100fd..fe9445be107 100644
--- a/components/gfx/font.rs
+++ b/components/gfx/font.rs
@@ -8,7 +8,9 @@ use std::string;
 use std::rc::Rc;
 use std::cell::RefCell;
 use servo_util::cache::{Cache, HashCache};
-use style::computed_values::{font_weight, font_style, font_variant};
+use servo_util::smallvec::{SmallVec, SmallVec1};
+use style::computed_values::{font_variant, font_weight};
+use style::style_structs::Font as FontStyle;
 use sync::Arc;
 
 use servo_util::geometry::Au;
@@ -82,22 +84,6 @@ pub struct FontMetrics {
     pub line_gap:         Au,
 }
 
-// TODO(Issue #179): eventually this will be split into the specified
-// and used font styles.  specified contains uninterpreted CSS font
-// property values, while 'used' is attached to gfx::Font to descript
-// the instance's properties.
-//
-// For now, the cases are differentiated with a typedef
-#[deriving(Clone, PartialEq)]
-pub struct FontStyle {
-    pub pt_size: f64,
-    pub weight: font_weight::T,
-    pub style: font_style::T,
-    pub families: Vec<String>,
-    pub variant: font_variant::T,
-    // TODO(Issue #198): font-stretch, text-decoration, size-adjust
-}
-
 pub type SpecifiedFontStyle = FontStyle;
 pub type UsedFontStyle = FontStyle;
 
@@ -174,13 +160,13 @@ impl Font {
 }
 
 pub struct FontGroup {
-    pub fonts: Vec<Rc<RefCell<Font>>>,
+    pub fonts: SmallVec1<Rc<RefCell<Font>>>,
 }
 
 impl FontGroup {
-    pub fn new(fonts: Vec<Rc<RefCell<Font>>>) -> FontGroup {
+    pub fn new(fonts: SmallVec1<Rc<RefCell<Font>>>) -> FontGroup {
         FontGroup {
-            fonts: fonts
+            fonts: fonts,
         }
     }
 
@@ -188,7 +174,7 @@ impl FontGroup {
         assert!(self.fonts.len() > 0);
 
         // TODO(Issue #177): Actually fall back through the FontGroup when a font is unsuitable.
-        TextRun::new(&mut *self.fonts[0].borrow_mut(), text.clone())
+        TextRun::new(&mut *self.fonts.get(0).borrow_mut(), text.clone())
     }
 }
 
diff --git a/components/gfx/font_context.rs b/components/gfx/font_context.rs
index 261323b6df0..997a20e63f6 100644
--- a/components/gfx/font_context.rs
+++ b/components/gfx/font_context.rs
@@ -13,6 +13,7 @@ use platform::font_template::FontTemplateData;
 use font::FontHandleMethods;
 use platform::font::FontHandle;
 use servo_util::cache::HashCache;
+use servo_util::smallvec::{SmallVec, SmallVec1};
 
 use std::rc::Rc;
 use std::cell::RefCell;
@@ -122,18 +123,19 @@ impl FontContext {
         // TODO: The font context holds a strong ref to the cached fonts
         // so they will never be released. Find out a good time to drop them.
 
-        let desc = FontTemplateDescriptor::new(style.weight, style.style == font_style::italic);
-        let mut fonts: Vec<Rc<RefCell<Font>>> = vec!();
+        let desc = FontTemplateDescriptor::new(style.font_weight,
+                                               style.font_style == font_style::italic);
+        let mut fonts: SmallVec1<Rc<RefCell<Font>>> = SmallVec1::new();
 
-        for family in style.families.iter() {
+        for family in style.font_family.iter() {
             // GWTODO: Check on real pages if this is faster as Vec() or HashMap().
             let mut cache_hit = false;
             for cached_font_entry in self.layout_font_cache.iter() {
-                if cached_font_entry.family == *family {
+                if cached_font_entry.family.as_slice() == family.name() {
                     let cached_font = cached_font_entry.font.borrow();
                     if cached_font.descriptor == desc &&
-                       cached_font.requested_pt_size == style.pt_size &&
-                       cached_font.variant == style.variant {
+                       cached_font.requested_pt_size == style.font_size.to_subpx() &&
+                       cached_font.variant == style.font_variant {
                         fonts.push(cached_font_entry.font.clone());
                         cache_hit = true;
                         break;
@@ -142,16 +144,18 @@ impl FontContext {
             }
 
             if !cache_hit {
-                let font_template = self.font_cache_task.get_font_template(family.clone(), desc.clone());
+                let font_template = self.font_cache_task.get_font_template(family.name()
+                                                                                 .to_string(),
+                                                                           desc.clone());
                 match font_template {
                     Some(font_template) => {
                         let layout_font = self.create_layout_font(font_template,
                                                                   desc.clone(),
-                                                                  style.pt_size,
-                                                                  style.variant);
+                                                                  style.font_size.to_subpx(),
+                                                                  style.font_variant);
                         let layout_font = Rc::new(RefCell::new(layout_font));
                         self.layout_font_cache.push(LayoutFontCacheEntry {
-                            family: family.clone(),
+                            family: family.name().to_string(),
                             font: layout_font.clone(),
                         });
                         fonts.push(layout_font);
@@ -168,8 +172,8 @@ impl FontContext {
             for cached_font_entry in self.fallback_font_cache.iter() {
                 let cached_font = cached_font_entry.font.borrow();
                 if cached_font.descriptor == desc &&
-                            cached_font.requested_pt_size == style.pt_size &&
-                            cached_font.variant == style.variant {
+                            cached_font.requested_pt_size == style.font_size.to_subpx() &&
+                            cached_font.variant == style.font_variant {
                     fonts.push(cached_font_entry.font.clone());
                     cache_hit = true;
                     break;
@@ -180,8 +184,8 @@ impl FontContext {
                 let font_template = self.font_cache_task.get_last_resort_font_template(desc.clone());
                 let layout_font = self.create_layout_font(font_template,
                                                           desc.clone(),
-                                                          style.pt_size,
-                                                          style.variant);
+                                                          style.font_size.to_subpx(),
+                                                          style.font_variant);
                 let layout_font = Rc::new(RefCell::new(layout_font));
                 self.fallback_font_cache.push(FallbackFontCacheEntry {
                     font: layout_font.clone(),
diff --git a/components/layout/fragment.rs b/components/layout/fragment.rs
index 73afce62279..63992e37413 100644
--- a/components/layout/fragment.rs
+++ b/components/layout/fragment.rs
@@ -32,7 +32,6 @@ use gfx::display_list::{LineDisplayItemClass, OpaqueNode, PseudoDisplayItemClass
 use gfx::display_list::{SidewaysLeft, SidewaysRight, SolidColorDisplayItem};
 use gfx::display_list::{SolidColorDisplayItemClass, StackingLevel, TextDisplayItem};
 use gfx::display_list::{TextDisplayItemClass, Upright};
-use gfx::font::FontStyle;
 use gfx::text::glyph::CharIndex;
 use gfx::text::text_run::TextRun;
 use script_traits::UntrustedNodeAddress;
@@ -707,8 +706,8 @@ impl Fragment {
     }
 
     pub fn calculate_line_height(&self, layout_context: &LayoutContext) -> Au {
-        let font_style = text::computed_style_to_font_style(&*self.style);
-        let font_metrics = text::font_metrics_for_style(layout_context.font_context(), &font_style);
+        let font_style = self.style.get_font();
+        let font_metrics = text::font_metrics_for_style(layout_context.font_context(), font_style);
         text::line_height_from_style(&*self.style, &font_metrics)
     }
 
@@ -862,11 +861,6 @@ impl Fragment {
         }
     }
 
-    /// Converts this fragment's computed style to a font style used for rendering.
-    pub fn font_style(&self) -> FontStyle {
-        text::computed_style_to_font_style(self.style())
-    }
-
     #[inline(always)]
     pub fn style<'a>(&'a self) -> &'a ComputedValues {
         &*self.style
@@ -1834,9 +1828,9 @@ impl Fragment {
             InlineBlockFragment(ref info) => {
                 // See CSS 2.1 § 10.8.1.
                 let block_flow = info.flow_ref.deref().as_immutable_block();
-                let font_style = text::computed_style_to_font_style(&*self.style);
+                let font_style = self.style.get_font();
                 let font_metrics = text::font_metrics_for_style(layout_context.font_context(),
-                                                                &font_style);
+                                                                font_style);
                 InlineMetrics::from_block_height(&font_metrics,
                                                  block_flow.base.position.size.block +
                                                  block_flow.fragment.margin.block_start_end())
@@ -1872,7 +1866,7 @@ impl Fragment {
         match (&self.specific, &other.specific) {
             (&UnscannedTextFragment(_), &UnscannedTextFragment(_)) => {
                 // FIXME: Should probably use a whitelist of styles that can safely differ (#3165)
-                self.font_style() == other.font_style() &&
+                self.style().get_font() == other.style().get_font() &&
                     self.text_decoration() == other.text_decoration() &&
                     self.white_space() == other.white_space()
             }
diff --git a/components/layout/inline.rs b/components/layout/inline.rs
index 70b7ffa7d3e..3fd918502ed 100644
--- a/components/layout/inline.rs
+++ b/components/layout/inline.rs
@@ -930,8 +930,8 @@ impl InlineFlow {
             return (Au(0), Au(0))
         }
 
-        let font_style = text::computed_style_to_font_style(style);
-        let font_metrics = text::font_metrics_for_style(font_context, &font_style);
+        let font_style = style.get_font();
+        let font_metrics = text::font_metrics_for_style(font_context, font_style);
         let line_height = text::line_height_from_style(style, &font_metrics);
         let inline_metrics = InlineMetrics::from_font_metrics(&font_metrics, line_height);
 
@@ -944,8 +944,8 @@ impl InlineFlow {
             match frag.inline_context {
                 Some(ref inline_context) => {
                     for style in inline_context.styles.iter() {
-                        let font_style = text::computed_style_to_font_style(&**style);
-                        let font_metrics = text::font_metrics_for_style(font_context, &font_style);
+                        let font_style = style.get_font();
+                        let font_metrics = text::font_metrics_for_style(font_context, font_style);
                         let line_height = text::line_height_from_style(&**style, &font_metrics);
                         let inline_metrics = InlineMetrics::from_font_metrics(&font_metrics,
                                                                               line_height);
diff --git a/components/layout/text.rs b/components/layout/text.rs
index e50eb74a91e..be79be0c06d 100644
--- a/components/layout/text.rs
+++ b/components/layout/text.rs
@@ -9,7 +9,7 @@
 use flow::Flow;
 use fragment::{Fragment, ScannedTextFragment, ScannedTextFragmentInfo, UnscannedTextFragment};
 
-use gfx::font::{FontMetrics, FontStyle, RunMetrics};
+use gfx::font::{FontMetrics,RunMetrics};
 use gfx::font_context::FontContext;
 use gfx::text::glyph::CharIndex;
 use gfx::text::text_run::TextRun;
@@ -17,8 +17,10 @@ use gfx::text::util::{CompressWhitespaceNewline, transform_text, CompressNone};
 use servo_util::geometry::Au;
 use servo_util::logical_geometry::{LogicalSize, WritingMode};
 use servo_util::range::Range;
+use servo_util::smallvec::SmallVec;
 use style::ComputedValues;
-use style::computed_values::{font_family, line_height, text_orientation, white_space};
+use style::computed_values::{line_height, text_orientation, white_space};
+use style::style_structs::Font as FontStyle;
 use sync::Arc;
 
 struct NewLinePositions {
@@ -121,7 +123,7 @@ impl TextRunScanner {
                     _ => fail!("Expected an unscanned text fragment!"),
                 };
 
-                let font_style = old_fragment.font_style();
+                let font_style = old_fragment.style().get_font();
 
                 let compression = match old_fragment.white_space() {
                     white_space::normal | white_space::nowrap => CompressWhitespaceNewline,
@@ -141,7 +143,7 @@ impl TextRunScanner {
                     // TODO(#177): Text run creation must account for the renderability of text by
                     // font group fonts. This is probably achieved by creating the font group above
                     // and then letting `FontGroup` decide which `Font` to stick into the text run.
-                    let fontgroup = font_context.get_layout_font_group_for_style(&font_style);
+                    let fontgroup = font_context.get_layout_font_group_for_style(font_style);
                     let run = box fontgroup.create_textrun(
                         transformed_text.clone());
 
@@ -164,8 +166,8 @@ impl TextRunScanner {
                 // font group fonts. This is probably achieved by creating the font group above
                 // and then letting `FontGroup` decide which `Font` to stick into the text run.
                 let in_fragment = &in_fragments[self.clump.begin().to_uint()];
-                let font_style = in_fragment.font_style();
-                let fontgroup = font_context.get_layout_font_group_for_style(&font_style);
+                let font_style = in_fragment.style().get_font();
+                let fontgroup = font_context.get_layout_font_group_for_style(font_style);
 
                 let compression = match in_fragment.white_space() {
                     white_space::normal | white_space::nowrap => CompressWhitespaceNewline,
@@ -216,9 +218,8 @@ impl TextRunScanner {
                 // sequence. If no clump takes ownership, however, it will leak.
                 let clump = self.clump;
                 let run = if clump.length() != CharIndex(0) && run_str.len() > 0 {
-                    Some(Arc::new(box TextRun::new(
-                        &mut *fontgroup.fonts[0].borrow_mut(),
-                        run_str.to_string())))
+                    Some(Arc::new(box TextRun::new(&mut *fontgroup.fonts.get(0).borrow_mut(),
+                                                   run_str.to_string())))
                 } else {
                     None
                 };
@@ -287,34 +288,7 @@ fn bounding_box_for_run_metrics(metrics: &RunMetrics, writing_mode: WritingMode)
 pub fn font_metrics_for_style(font_context: &mut FontContext, font_style: &FontStyle)
                               -> FontMetrics {
     let fontgroup = font_context.get_layout_font_group_for_style(font_style);
-    fontgroup.fonts[0].borrow().metrics.clone()
-}
-
-/// Converts a computed style to a font style used for rendering.
-///
-/// FIXME(pcwalton): This should not be necessary; just make the font part of the style sharable
-/// with the display list somehow. (Perhaps we should use an ARC.)
-pub fn computed_style_to_font_style(style: &ComputedValues) -> FontStyle {
-    debug!("(font style) start");
-
-    // FIXME: Too much allocation here.
-    let mut font_families = style.get_font().font_family.iter().map(|family| {
-        match *family {
-            font_family::FamilyName(ref name) => (*name).clone(),
-        }
-    });
-    debug!("(font style) font families: `{:?}`", font_families);
-
-    let font_size = style.get_font().font_size.to_f64().unwrap() / 60.0;
-    debug!("(font style) font size: `{:f}px`", font_size);
-
-    FontStyle {
-        pt_size: font_size,
-        weight: style.get_font().font_weight,
-        style: style.get_font().font_style,
-        variant: style.get_font().font_variant,
-        families: font_families.collect(),
-    }
+    fontgroup.fonts.get(0).borrow().metrics.clone()
 }
 
 /// Returns the line block-size needed by the given computed style and font size.
diff --git a/components/style/properties/mod.rs.mako b/components/style/properties/mod.rs.mako
index c5ca7159893..e2c2bdcb7f3 100644
--- a/components/style/properties/mod.rs.mako
+++ b/components/style/properties/mod.rs.mako
@@ -774,6 +774,13 @@ pub mod longhands {
 //                Fantasy,
 //                Monospace,
             }
+            impl FontFamily {
+                pub fn name(&self) -> &str {
+                    match *self {
+                        FamilyName(ref name) => name.as_slice(),
+                    }
+                }
+            }
             pub type T = Vec<FontFamily>;
         }
         pub type SpecifiedValue = computed_value::T;