servo: Merge #11109 - Make `text-align: justify` incremental layout safe (from kaksmet:justify-fix); r=mbrubeck
authorUlf Nilsson <kaksmet@gmail.com>
Tue, 10 May 2016 12:54:26 -0700
changeset 338758 bb7b9f713bf82f7dcb842c1dd587daa610372ca9
parent 338757 357c56149ac0650ba5085bc14460130dc0b7f88e
child 338759 18b20a707b80ee994d2b6d01f2d39fed83b2101b
push id31307
push usergszorc@mozilla.com
push dateSat, 04 Feb 2017 00:59:06 +0000
treeherdermozilla-central@94079d43835f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmbrubeck
servo: Merge #11109 - Make `text-align: justify` incremental layout safe (from kaksmet:justify-fix); r=mbrubeck Closes #10499 Closes #9057 Source-Repo: https://github.com/servo/servo Source-Revision: 069be91e8451d197c8f66790fbf53086f22150d7
servo/components/gfx/paint_context.rs
servo/components/gfx/text/glyph.rs
servo/components/gfx/text/text_run.rs
servo/components/layout/fragment.rs
servo/components/layout/inline.rs
servo/components/layout/webrender_helpers.rs
--- a/servo/components/gfx/paint_context.rs
+++ b/servo/components/gfx/paint_context.rs
@@ -1794,17 +1794,21 @@ impl ScaledFontExtensionMethods for Scal
                             else { AntialiasMode::None as u8 }
         };
 
         let mut origin = baseline_origin.clone();
         let mut azglyphs = Vec::with_capacity(range.length().to_usize());
 
         for slice in run.natural_word_slices_in_visual_order(range) {
             for glyph in slice.glyphs.iter_glyphs_for_byte_range(&slice.range) {
-                let glyph_advance = glyph.advance();
+                let glyph_advance = if glyph.char_is_space() {
+                    glyph.advance() + run.extra_word_spacing
+                } else {
+                    glyph.advance()
+                };
                 if !slice.glyphs.is_whitespace() {
                     let glyph_offset = glyph.offset().unwrap_or(Point2D::zero());
                     let azglyph = struct__AzGlyph {
                         mIndex: glyph.id() as u32,
                         mPosition: struct__AzPoint {
                             x: (origin.x + glyph_offset.x).to_f32_px(),
                             y: (origin.y + glyph_offset.y).to_f32_px(),
                         }
--- a/servo/components/gfx/text/glyph.rs
+++ b/servo/components/gfx/text/glyph.rs
@@ -66,23 +66,24 @@ impl GlyphEntry {
     }
 }
 
 /// The id of a particular glyph within a font
 pub type GlyphId = u32;
 
 // TODO: make this more type-safe.
 
-const FLAG_CHAR_IS_SPACE: u32      = 0x40000000;
-const FLAG_IS_SIMPLE_GLYPH: u32    = 0x80000000;
+const FLAG_CHAR_IS_SPACE: u32       = 0x40000000;
+const FLAG_CHAR_IS_SPACE_SHIFT: u32 = 30;
+const FLAG_IS_SIMPLE_GLYPH: u32     = 0x80000000;
 
 // glyph advance; in Au's.
-const GLYPH_ADVANCE_MASK: u32      = 0x3FFF0000;
-const GLYPH_ADVANCE_SHIFT: u32     = 16;
-const GLYPH_ID_MASK: u32           = 0x0000FFFF;
+const GLYPH_ADVANCE_MASK: u32       = 0x3FFF0000;
+const GLYPH_ADVANCE_SHIFT: u32      = 16;
+const GLYPH_ID_MASK: u32            = 0x0000FFFF;
 
 // Non-simple glyphs (more than one glyph per char; missing glyph,
 // newline, tab, large advance, or nonzero x/y offsets) may have one
 // or more detailed glyphs associated with them. They are stored in a
 // side array so that there is a 1:1 mapping of GlyphEntry to
 // unicode char.
 
 // The number of detailed glyphs for this char.
@@ -366,16 +367,25 @@ impl<'a> GlyphInfo<'a> {
     pub fn offset(self) -> Option<Point2D<Au>> {
         match self {
             GlyphInfo::Simple(_, _) => None,
             GlyphInfo::Detail(store, entry_i, detail_j) => {
                 Some(store.detail_store.detailed_glyph_with_index(entry_i, detail_j).offset)
             }
         }
     }
+
+    pub fn char_is_space(self) -> bool {
+        let (store, entry_i) = match self {
+            GlyphInfo::Simple(store, entry_i) => (store, entry_i),
+            GlyphInfo::Detail(store, entry_i, _) => (store, entry_i),
+        };
+
+        store.char_is_space(entry_i)
+    }
 }
 
 /// Stores the glyph data belonging to a text run.
 ///
 /// Simple glyphs are stored inline in the `entry_buffer`, detailed glyphs are
 /// stored as pointers into the `detail_store`.
 ///
 /// ~~~ignore
@@ -401,16 +411,18 @@ pub struct GlyphStore {
     /// transmute_entry_buffer_to_u32_buffer().
     entry_buffer: Vec<GlyphEntry>,
     /// A store of the detailed glyph data. Detailed glyphs contained in the
     /// `entry_buffer` point to locations in this data structure.
     detail_store: DetailedGlyphStore,
 
     /// A cache of the advance of the entire glyph store.
     total_advance: Au,
+    /// A cache of the number of spaces in the entire glyph store.
+    total_spaces: i32,
 
     /// Used to check if fast path should be used in glyph iteration.
     has_detailed_glyphs: bool,
     is_whitespace: bool,
     is_rtl: bool,
 }
 
 int_range_index! {
@@ -427,16 +439,17 @@ impl<'a> GlyphStore {
     /// Use the `add_*` methods to store glyph data.
     pub fn new(length: usize, is_whitespace: bool, is_rtl: bool) -> GlyphStore {
         assert!(length > 0);
 
         GlyphStore {
             entry_buffer: vec![GlyphEntry::initial(); length],
             detail_store: DetailedGlyphStore::new(),
             total_advance: Au(0),
+            total_spaces: 0,
             has_detailed_glyphs: false,
             is_whitespace: is_whitespace,
             is_rtl: is_rtl,
         }
     }
 
     #[inline]
     pub fn len(&self) -> ByteIndex {
@@ -445,30 +458,31 @@ impl<'a> GlyphStore {
 
     #[inline]
     pub fn is_whitespace(&self) -> bool {
         self.is_whitespace
     }
 
     pub fn finalize_changes(&mut self) {
         self.detail_store.ensure_sorted();
-        self.cache_total_advance()
+        self.cache_total_advance_and_spaces()
     }
 
     #[inline(never)]
-    fn cache_total_advance(&mut self) {
+    fn cache_total_advance_and_spaces(&mut self) {
         let mut total_advance = Au(0);
+        let mut total_spaces = 0;
         for glyph in self.iter_glyphs_for_byte_range(&Range::new(ByteIndex(0), self.len())) {
-            total_advance = total_advance + glyph.advance()
+            total_advance = total_advance + glyph.advance();
+            if glyph.char_is_space() {
+                total_spaces += 1;
+            }
         }
-        self.total_advance = total_advance
-    }
-
-    pub fn total_advance(&self) -> Au {
-        self.total_advance
+        self.total_advance = total_advance;
+        self.total_spaces = total_spaces;
     }
 
     /// Adds a single glyph.
     pub fn add_glyph_for_byte_index(&mut self,
                                     i: ByteIndex,
                                     character: char,
                                     data: &GlyphData) {
         let glyph_is_compressible = is_simple_glyph_id(data.id) &&
@@ -533,67 +547,85 @@ impl<'a> GlyphStore {
             store:       self,
             byte_index:  if self.is_rtl { range.end() } else { range.begin() - ByteIndex(1) },
             byte_range:  *range,
             glyph_range: None,
         }
     }
 
     #[inline]
-    pub fn advance_for_byte_range(&self, range: &Range<ByteIndex>) -> Au {
+    pub fn advance_for_byte_range(&self, range: &Range<ByteIndex>, extra_word_spacing: Au) -> Au {
         if range.begin() == ByteIndex(0) && range.end() == self.len() {
-            self.total_advance
+            self.total_advance + extra_word_spacing * self.total_spaces
         } else if !self.has_detailed_glyphs {
-            self.advance_for_byte_range_simple_glyphs(range)
+            self.advance_for_byte_range_simple_glyphs(range, extra_word_spacing)
         } else {
-            self.advance_for_byte_range_slow_path(range)
+            self.advance_for_byte_range_slow_path(range, extra_word_spacing)
         }
     }
 
     #[inline]
-    pub fn advance_for_byte_range_slow_path(&self, range: &Range<ByteIndex>) -> Au {
+    pub fn advance_for_byte_range_slow_path(&self, range: &Range<ByteIndex>, extra_word_spacing: Au) -> Au {
         self.iter_glyphs_for_byte_range(range)
-            .fold(Au(0), |advance, glyph| advance + glyph.advance())
+            .fold(Au(0), |advance, glyph| {
+                if glyph.char_is_space() {
+                    advance + glyph.advance() + extra_word_spacing
+                } else {
+                    advance + glyph.advance()
+                }
+            })
     }
 
     #[inline]
     #[cfg(any(target_arch = "x86_64", target_arch = "aarch64"))]
-    fn advance_for_byte_range_simple_glyphs(&self, range: &Range<ByteIndex>) -> Au {
-        let mask = u32x4::splat(GLYPH_ADVANCE_MASK);
+    fn advance_for_byte_range_simple_glyphs(&self, range: &Range<ByteIndex>, extra_word_spacing: Au) -> Au {
+        let advance_mask = u32x4::splat(GLYPH_ADVANCE_MASK);
+        let space_flag_mask = u32x4::splat(FLAG_CHAR_IS_SPACE);
         let mut simd_advance = u32x4::splat(0);
+        let mut simd_spaces = u32x4::splat(0);
         let begin = range.begin().to_usize();
         let len = range.length().to_usize();
         let num_simd_iterations = len / 4;
         let leftover_entries = range.end().to_usize() - (len - num_simd_iterations * 4);
         let buf = self.transmute_entry_buffer_to_u32_buffer();
 
         for i in 0..num_simd_iterations {
-            let mut v = u32x4::load(buf, begin + i * 4);
-            v = v & mask;
-            v = v >> GLYPH_ADVANCE_SHIFT;
-            simd_advance = simd_advance + v;
+            let v = u32x4::load(buf, begin + i * 4);
+            let advance = (v & advance_mask) >> GLYPH_ADVANCE_SHIFT;
+            let spaces = (v & space_flag_mask) >> FLAG_CHAR_IS_SPACE_SHIFT;
+            simd_advance = simd_advance + advance;
+            simd_spaces = simd_spaces + spaces;
         }
 
         let advance =
             (simd_advance.extract(0) +
              simd_advance.extract(1) +
              simd_advance.extract(2) +
              simd_advance.extract(3)) as i32;
-        let mut leftover = Au(0);
+        let spaces =
+            (simd_spaces.extract(0) +
+             simd_spaces.extract(1) +
+             simd_spaces.extract(2) +
+             simd_spaces.extract(3)) as i32;
+        let mut leftover_advance = Au(0);
+        let mut leftover_spaces = 0;
         for i in leftover_entries..range.end().to_usize() {
-            leftover = leftover + self.entry_buffer[i].advance();
+            leftover_advance = leftover_advance + self.entry_buffer[i].advance();
+            if self.entry_buffer[i].char_is_space() {
+                leftover_spaces += 1;
+            }
         }
-        Au(advance) + leftover
+        Au(advance) + leftover_advance + extra_word_spacing * (spaces + leftover_spaces)
     }
 
     /// When SIMD isn't available (non-x86_x64/aarch64), fallback to the slow path.
     #[inline]
     #[cfg(not(any(target_arch = "x86_64", target_arch = "aarch64")))]
-    fn advance_for_byte_range_simple_glyphs(&self, range: &Range<ByteIndex>) -> Au {
-        self.advance_for_byte_range_slow_path(range)
+    fn advance_for_byte_range_simple_glyphs(&self, range: &Range<ByteIndex>, extra_word_spacing: Au) -> Au {
+        self.advance_for_byte_range_slow_path(range, extra_word_spacing)
     }
 
     /// Used for SIMD.
     #[inline]
     #[cfg(any(target_arch = "x86_64", target_arch = "aarch64"))]
     fn transmute_entry_buffer_to_u32_buffer(&self) -> &[u32] {
         unsafe { mem::transmute(self.entry_buffer.as_slice()) }
     }
@@ -607,36 +639,16 @@ impl<'a> GlyphStore {
         let mut spaces = 0;
         for index in range.each_index() {
             if self.char_is_space(index) {
                 spaces += 1
             }
         }
         spaces
     }
-
-    pub fn distribute_extra_space_in_range(&mut self, range: &Range<ByteIndex>, space: f64) {
-        debug_assert!(space >= 0.0);
-        if range.is_empty() {
-            return
-        }
-        for index in range.each_index() {
-            // TODO(pcwalton): Handle spaces that are detailed glyphs -- these are uncommon but
-            // possible.
-            let entry = &mut self.entry_buffer[index.to_usize()];
-            if entry.is_simple() && entry.char_is_space() {
-                // FIXME(pcwalton): This can overflow for very large font-sizes.
-                let advance =
-                    ((entry.value & GLYPH_ADVANCE_MASK) >> GLYPH_ADVANCE_SHIFT) +
-                    Au::from_f64_px(space).0 as u32;
-                entry.value = (entry.value & !GLYPH_ADVANCE_MASK) |
-                    (advance << GLYPH_ADVANCE_SHIFT);
-            }
-        }
-    }
 }
 
 impl fmt::Debug for GlyphStore {
     fn fmt(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
         try!(write!(formatter, "GlyphStore:\n"));
         let mut detailed_buffer = self.detail_store.detail_buffer.iter();
         for entry in self.entry_buffer.iter() {
             if entry.is_simple() {
--- a/servo/components/gfx/text/text_run.rs
+++ b/servo/components/gfx/text/text_run.rs
@@ -28,16 +28,17 @@ pub struct TextRun {
     pub text: Arc<String>,
     pub font_template: Arc<FontTemplateData>,
     pub actual_pt_size: Au,
     pub font_metrics: FontMetrics,
     pub font_key: Option<webrender_traits::FontKey>,
     /// The glyph runs that make up this text run.
     pub glyphs: Arc<Vec<GlyphRun>>,
     pub bidi_level: u8,
+    pub extra_word_spacing: Au,
 }
 
 impl Drop for TextRun {
     fn drop(&mut self) {
         // Invalidate the glyph run cache if it was our text run that got freed.
         INDEX_OF_FIRST_GLYPH_RUN_CACHE.with(|index_of_first_glyph_run_cache| {
             if let Some((text_run_ptr, _, _)) = index_of_first_glyph_run_cache.get() {
                 if text_run_ptr == (self as *const TextRun) {
@@ -183,16 +184,17 @@ impl<'a> TextRun {
         TextRun {
             text: Arc::new(text),
             font_metrics: font.metrics.clone(),
             font_template: font.handle.template(),
             font_key: font.font_key,
             actual_pt_size: font.actual_pt_size,
             glyphs: Arc::new(glyphs),
             bidi_level: bidi_level,
+            extra_word_spacing: Au(0),
         }
     }
 
     pub fn break_and_shape(font: &mut Font, text: &str, options: &ShapingOptions)
                            -> Vec<GlyphRun> {
         let mut glyphs = vec!();
         let mut slice = 0..0;
 
@@ -242,29 +244,29 @@ impl<'a> TextRun {
         if range.is_empty() {
             return Au(0)
         }
 
         // TODO(Issue #199): alter advance direction for RTL
         // TODO(Issue #98): using inter-char and inter-word spacing settings when measuring text
         self.natural_word_slices_in_range(range)
             .fold(Au(0), |advance, slice| {
-                advance + slice.glyphs.advance_for_byte_range(&slice.range)
+                advance + slice.glyphs.advance_for_byte_range(&slice.range, self.extra_word_spacing)
             })
     }
 
     pub fn metrics_for_range(&self, range: &Range<ByteIndex>) -> RunMetrics {
         RunMetrics::new(self.advance_for_range(range),
                         self.font_metrics.ascent,
                         self.font_metrics.descent)
     }
 
     pub fn metrics_for_slice(&self, glyphs: &GlyphStore, slice_range: &Range<ByteIndex>)
                              -> RunMetrics {
-        RunMetrics::new(glyphs.advance_for_byte_range(slice_range),
+        RunMetrics::new(glyphs.advance_for_byte_range(slice_range, self.extra_word_spacing),
                         self.font_metrics.ascent,
                         self.font_metrics.descent)
     }
 
     pub fn min_width_for_range(&self, range: &Range<ByteIndex>) -> Au {
         debug!("iterating outer range {:?}", range);
         self.natural_word_slices_in_range(range).fold(Au(0), |max_piece_width, slice| {
             debug!("iterated on {:?}[{:?}]", slice.offset, slice.range);
--- a/servo/components/layout/fragment.rs
+++ b/servo/components/layout/fragment.rs
@@ -1746,33 +1746,33 @@ impl Fragment {
         }
         self.reset_text_range_and_inline_size();
         self.meld_with_next_inline_fragment(&next_fragment);
     }
 
     /// Restore any whitespace that was stripped from a text fragment, and recompute inline metrics
     /// if necessary.
     pub fn reset_text_range_and_inline_size(&mut self) {
-        match &mut self.specific {
-            &mut SpecificFragmentInfo::ScannedText(ref mut info) => {
-                // FIXME (mbrubeck): Do we need to restore leading too?
-                let range_end = info.range_end_including_stripped_whitespace;
-                if info.range.end() == range_end {
-                    return
-                }
-                info.range.extend_to(range_end);
-                info.content_size.inline = info.run.metrics_for_range(&info.range).advance_width;
-                self.border_box.size.inline = info.content_size.inline +
-                    self.border_padding.inline_start_end();
+        if let SpecificFragmentInfo::ScannedText(ref mut info) = self.specific {
+            if info.run.extra_word_spacing != Au(0) {
+                Arc::make_mut(&mut info.run).extra_word_spacing = Au(0);
             }
-            _ => {}
+
+            // FIXME (mbrubeck): Do we need to restore leading too?
+            let range_end = info.range_end_including_stripped_whitespace;
+            if info.range.end() == range_end {
+                return
+            }
+            info.range.extend_to(range_end);
+            info.content_size.inline = info.run.metrics_for_range(&info.range).advance_width;
+            self.border_box.size.inline = info.content_size.inline +
+                self.border_padding.inline_start_end();
         }
     }
 
-
     /// Assigns replaced inline-size, padding, and margins for this fragment only if it is replaced
     /// content per CSS 2.1 ยง 10.3.2.
     pub fn assign_replaced_inline_size_if_necessary(&mut self, container_inline_size: Au) {
         match self.specific {
             SpecificFragmentInfo::Generic |
             SpecificFragmentInfo::GeneratedContent(_) |
             SpecificFragmentInfo::Table |
             SpecificFragmentInfo::TableCell |
--- a/servo/components/layout/inline.rs
+++ b/servo/components/layout/inline.rs
@@ -956,59 +956,45 @@ impl InlineFlow {
                                 line: &Line,
                                 slack_inline_size: Au) {
         // Fast path.
         if slack_inline_size == Au(0) {
             return
         }
 
         // First, calculate the number of expansion opportunities (spaces, normally).
-        let mut expansion_opportunities = 0i32;
+        let mut expansion_opportunities = 0;
         for fragment_index in line.range.each_index() {
             let fragment = fragments.get(fragment_index.to_usize());
             let scanned_text_fragment_info = match fragment.specific {
                 SpecificFragmentInfo::ScannedText(ref info) if !info.range.is_empty() => info,
                 _ => continue
             };
             let fragment_range = scanned_text_fragment_info.range;
 
             for slice in scanned_text_fragment_info.run.character_slices_in_range(&fragment_range) {
-                expansion_opportunities += slice.glyphs.space_count_in_range(&slice.range) as i32
+                expansion_opportunities += slice.glyphs.space_count_in_range(&slice.range)
             }
         }
 
+        if expansion_opportunities == 0 {
+            return
+        }
+
         // Then distribute all the space across the expansion opportunities.
-        let space_per_expansion_opportunity = slack_inline_size.to_f64_px() /
-            (expansion_opportunities as f64);
+        let space_per_expansion_opportunity = slack_inline_size / expansion_opportunities as i32;
         for fragment_index in line.range.each_index() {
             let fragment = fragments.get_mut(fragment_index.to_usize());
             let mut scanned_text_fragment_info = match fragment.specific {
                 SpecificFragmentInfo::ScannedText(ref mut info) if !info.range.is_empty() => info,
                 _ => continue
             };
             let fragment_range = scanned_text_fragment_info.range;
-
-            // FIXME(pcwalton): This is an awful lot of uniqueness making. I don't see any easy way
-            // to get rid of it without regressing the performance of the non-justified case,
-            // though.
             let run = Arc::make_mut(&mut scanned_text_fragment_info.run);
-            {
-                let glyph_runs = Arc::make_mut(&mut run.glyphs);
-                for mut glyph_run in &mut *glyph_runs {
-                    let mut range = glyph_run.range.intersect(&fragment_range);
-                    if range.is_empty() {
-                        continue
-                    }
-                    range.shift_by(-glyph_run.range.begin());
-
-                    let glyph_store = Arc::make_mut(&mut glyph_run.glyph_store);
-                    glyph_store.distribute_extra_space_in_range(&range,
-                                                                space_per_expansion_opportunity);
-                }
-            }
+            run.extra_word_spacing = space_per_expansion_opportunity;
 
             // Recompute the fragment's border box size.
             let new_inline_size = run.advance_for_range(&fragment_range);
             let new_size = LogicalSize::new(fragment.style.writing_mode,
                                             new_inline_size,
                                             fragment.border_box.size.block);
             fragment.border_box = LogicalRect::from_point_size(fragment.style.writing_mode,
                                                                fragment.border_box.start,
--- a/servo/components/layout/webrender_helpers.rs
+++ b/servo/components/layout/webrender_helpers.rs
@@ -388,17 +388,21 @@ impl WebRenderDisplayItemConverter for D
                 }
             }
             DisplayItem::TextClass(ref item) => {
                 let mut origin = item.baseline_origin.clone();
                 let mut glyphs = vec!();
 
                 for slice in item.text_run.natural_word_slices_in_visual_order(&item.range) {
                     for glyph in slice.glyphs.iter_glyphs_for_byte_range(&slice.range) {
-                        let glyph_advance = glyph.advance();
+                        let glyph_advance = if glyph.char_is_space() {
+                            glyph.advance() + item.text_run.extra_word_spacing
+                        } else {
+                            glyph.advance()
+                        };
                         if !slice.glyphs.is_whitespace() {
                             let glyph_offset = glyph.offset().unwrap_or(Point2D::zero());
                             let glyph = webrender_traits::GlyphInstance {
                                 index: glyph.id(),
                                 x: (origin.x + glyph_offset.x).to_f32_px(),
                                 y: (origin.y + glyph_offset.y).to_f32_px(),
                             };
                             glyphs.push(glyph);