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

Start implementing text in layout 2020 #24822

Merged
merged 6 commits into from Nov 25, 2019
Merged

Start implementing text in layout 2020 #24822

merged 6 commits into from Nov 25, 2019

Conversation

@nox
Copy link
Member

nox commented Nov 21, 2019

No description provided.

@highfive
Copy link

highfive commented Nov 21, 2019

Heads up! This PR modifies the following files:

  • @emilio: components/style/properties/longhands/inherited_text.mako.rs
@highfive
Copy link

highfive commented Nov 21, 2019

warning Warning warning

  • These commits modify style and gfx code, but no tests are modified. Please consider adding a test!
@nox nox force-pushed the fontcontext branch from 63a0a0f to 5e3597e Nov 21, 2019
@nox
Copy link
Member Author

nox commented Nov 21, 2019

Forgot to git add file, a classic.

@jdm
Copy link
Member

jdm commented Nov 21, 2019

./components/layout_2020/text.rs:1: incorrect license
@nox nox force-pushed the fontcontext branch from 5e3597e to 322c79d Nov 21, 2019
@nox
Copy link
Member Author

nox commented Nov 21, 2019

Fixed now, and I ran test-tidy again to make sure.

@nox nox force-pushed the fontcontext branch from 322c79d to ae56351 Nov 22, 2019
@nox
Copy link
Member Author

nox commented Nov 22, 2019

@highfive highfive assigned SimonSapin and unassigned asajeffrey Nov 22, 2019
@CYBAI
Copy link
Collaborator

CYBAI commented Nov 22, 2019

👀

Diff in /repo/components/layout_2020/flow/inline.rs at line 291:
 impl TextRun {
     fn layout(&self, layout_context: &LayoutContext, ifc: &mut InlineFormattingContextState) {
         use gfx::font::{ShapingFlags, ShapingOptions};
-        use style::values::generics::text::LineHeight;
         use style::computed_values::text_rendering::T as TextRendering;
         use style::computed_values::word_break::T as WordBreak;
+        use style::values::generics::text::LineHeight;
 
         let font_style = self.parent_style.clone_font();
         let inherited_text_style = self.parent_style.get_inherited_text();
Diff in /repo/components/layout_2020/flow/inline.rs at line 336:
                     &mut None,
                 );
 
-                (font.metrics.ascent, font.metrics.line_gap, font.font_key, glyph_runs)
+                (
+                    font.metrics.ascent,
+                    font.metrics.line_gap,
+                    font.font_key,
+                    glyph_runs,
+                )
             });
 
         let font_size = self.parent_style.get_font().font_size.size.0;
clang-format not installed. Skipping CPP formatting.
Run `./mach fmt` to fix the formatting
@nox
Copy link
Member Author

nox commented Nov 22, 2019

Oops. Fixed.

@nox nox force-pushed the fontcontext branch from ab555bc to 7a6b137 Nov 22, 2019
components/layout_2020/context.rs Outdated Show resolved Hide resolved
components/layout_2020/context.rs Outdated Show resolved Hide resolved
components/gfx/text/glyph.rs Show resolved Hide resolved
#[derive(Clone)]
pub struct ShapedSegment {
pub(crate) font_key: FontInstanceKey,
pub(crate) glyph_runs: Vec<Arc<GlyphStore>>,

This comment has been minimized.

@SimonSapin

SimonSapin Nov 22, 2019

Member

"Segment" and "run" are somewhat interchangeable in my mind. What’s the terminology here? What does it mean to have multiple runs per segment? Since there is only one font key per segment, I assume we also have multiple segments per DOM text node.

This comment has been minimized.

@SimonSapin

SimonSapin Nov 22, 2019

Member

multiple segments per DOM text node

Though maybe not yet if we don’t do font fallback? Either way, could we unify the segment and (glyph) run concepts with each other? (And maybe with flow::inline::TextRun?)

This comment has been minimized.

@nox

nox Nov 25, 2019

Author Member

I kinda want to remove ShapedSegment altogether, yeah.

This comment has been minimized.

@SimonSapin

SimonSapin Nov 25, 2019

Member

Removing an unnecessary abstraction sounds great! 👍

components/layout_2020/flow/inline.rs Outdated Show resolved Hide resolved
@nox nox force-pushed the fontcontext branch from 7a6b137 to bf0320c Nov 25, 2019
Copy link
Member Author

nox left a comment

I addressed all the comments. ShapedSegment and its containing module are both gone now.

components/gfx/text/glyph.rs Show resolved Hide resolved
components/layout_2020/context.rs Outdated Show resolved Hide resolved
components/layout_2020/context.rs Outdated Show resolved Hide resolved
components/layout_2020/flow/inline.rs Outdated Show resolved Hide resolved
#[derive(Clone)]
pub struct ShapedSegment {
pub(crate) font_key: FontInstanceKey,
pub(crate) glyph_runs: Vec<Arc<GlyphStore>>,

This comment has been minimized.

@nox

nox Nov 25, 2019

Author Member

I kinda want to remove ShapedSegment altogether, yeah.

@nox nox force-pushed the fontcontext branch from bf0320c to 9f8d659 Nov 25, 2019
#[derive(Clone)]
pub struct ShapedSegment {
pub(crate) font_key: FontInstanceKey,
pub(crate) glyph_runs: Vec<Arc<GlyphStore>>,

This comment has been minimized.

@SimonSapin

SimonSapin Nov 25, 2019

Member

Removing an unnecessary abstraction sounds great! 👍

@nox nox force-pushed the fontcontext branch from 9f8d659 to 85b2a4d Nov 25, 2019
@SimonSapin
Copy link
Member

SimonSapin commented Nov 25, 2019

@SimonSapin
Copy link
Member

SimonSapin commented Nov 25, 2019

@bors-servo
Copy link
Contributor

bors-servo commented Nov 25, 2019

Testing commit 85b2a4d with merge fa37d21...

bors-servo added a commit that referenced this pull request Nov 25, 2019
Start implementing text in layout 2020
@bors-servo
Copy link
Contributor

bors-servo commented Nov 25, 2019

💔 Test failed - status-taskcluster

@SimonSapin
Copy link
Member

SimonSapin commented Nov 25, 2019

@bors-servo retry

  ▶ CRASH [expected OK] /xhr/send-entity-body-document.htm
  │ 
  │ 
  │ 
  │ 
  │ Failed to receive a response from live font cache (thread LayoutThread PipelineId { namespace_id: PipelineNamespaceId(2), index: PipelineIndex(9) }, at components/gfx/font_cache_thread.rs:516)
  │ stack backtrace:
  │    0: servo::main::{{closure}}
  │    1: std::panicking::rust_panic_with_hook
  │              at src/libstd/panicking.rs:468
  │    2: std::panicking::begin_panic
  │    3: <gfx::font_cache_thread::FontCacheThread as gfx::font_context::FontSource>::get_font_instance
  │    4: gfx::font_context::FontContext<S>::font
  │    5: core::ops::function::impls::<impl core::ops::function::FnMut<A> for &mut F>::call_mut
  │    6: <core::iter::adapters::chain::Chain<A,B> as core::iter::traits::iterator::Iterator>::try_fold
  │    7: gfx::font::FontGroup::find_by_codepoint
  │    8: layout::text::TextRunScanner::scan_for_runs
  │    9: std::thread::local::LocalKey<T>::with
  │   10: layout::construct::FlowConstructor<ConcreteThreadSafeLayoutNode>::build_flow_for_block_starting_with_fragments
  │   11: layout::construct::FlowConstructor<ConcreteThreadSafeLayoutNode>::build_flow_for_block_like
  │   12: layout::construct::FlowConstructor<ConcreteThreadSafeLayoutNode>::build_flow_for_block
  │   13: <layout::construct::FlowConstructor<ConcreteThreadSafeLayoutNode> as layout::traversal::PostorderNodeMutTraversal<ConcreteThreadSafeLayoutNode>>::process
  │   14: style::traversal::DomTraversal::handle_postorder_traversal
  │   15: style::driver::traverse_dom
  │   16: profile_traits::time::profile
  │   17: layout_thread::LayoutThread::handle_reflow
  │   18: profile_traits::time::profile
  │   19: layout_thread::LayoutThread::handle_request_helper
  │   20: layout_thread::LayoutThread::start
  │   21: profile_traits::mem::ProfilerChan::run_with_memory_reporting
  │   22: std::sys_common::backtrace::__rust_begin_short_backtrace
  │   23: _ZN3std9panicking3try7do_call17h310e2d87e8030445E.llvm.4992153416396273530
  │   24: __rust_maybe_catch_panic
  │              at src/libpanic_unwind/lib.rs:81
  │   25: core::ops::function::FnOnce::call_once{{vtable.shim}}
  │   26: <alloc::boxed::Box<F> as core::ops::function::FnOnce<A>>::call_once
  │              at /rustc/1bd30ce2aac40c7698aa4a1b9520aa649ff2d1c5/src/liballoc/boxed.rs:942
@bors-servo
Copy link
Contributor

bors-servo commented Nov 25, 2019

Testing commit 85b2a4d with merge b69e414...

bors-servo added a commit that referenced this pull request Nov 25, 2019
Start implementing text in layout 2020
@bors-servo
Copy link
Contributor

bors-servo commented Nov 25, 2019

💔 Test failed - status-taskcluster

@CYBAI
Copy link
Collaborator

CYBAI commented Nov 25, 2019

@bors-servo
Copy link
Contributor

bors-servo commented Nov 25, 2019

Testing commit 85b2a4d with merge a562808...

bors-servo added a commit that referenced this pull request Nov 25, 2019
Start implementing text in layout 2020
@bors-servo
Copy link
Contributor

bors-servo commented Nov 25, 2019

☀️ Test successful - status-taskcluster
Approved by: SimonSapin
Pushing a562808 to master...

@bors-servo bors-servo merged commit 85b2a4d into master Nov 25, 2019
2 checks passed
2 checks passed
Community-TC (pull_request) TaskGroup: success
Details
homu Test successful
Details
@bors-servo bors-servo deleted the fontcontext branch Nov 25, 2019
@SimonSapin SimonSapin mentioned this pull request Dec 3, 2019
3 of 8 tasks complete
@SimonSapin SimonSapin added this to Done / resolved in Layout 2020 Dec 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Layout 2020
  
Merged / resolved
Linked issues

Successfully merging this pull request may close these issues.

None yet

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