Closed Bug 1351200 Opened 7 years ago Closed 7 years ago

stylo: Gecko_nsStyleFont_GetBaseSize isn't thread-safe

Categories

(Core :: CSS Parsing and Computation, defect, P1)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: bholley, Assigned: manishearth)

References

(Depends on 1 open bug)

Details

Attachments

(3 files, 4 obsolete files)

Bug 1351200 - Part 1: stylo: Allow fetching prefs on servo traversal threads;
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1351200 - Part 2: stylo: Separate out caching code from font pref initialization code in GetFontPrefsForLang;
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1351200 - Part 3: stylo: Bypass cache when fetching font size prefs from Stylo;
59 bytes, text/x-review-board-request
emilio
: review+
Details 
Looks like this was added in bug 1341775.

I get this when forcing a parallel travesal on CI:

Assertion failure: NS_IsMainThread(), at /home/worker/workspace/build/src/modules/libpref/prefapi.cpp:784
#01: PREF_CopyCharPref [modules/libpref/prefapi.cpp:509]
#02: mozilla::Preferences::GetCString [modules/libpref/Preferences.cpp:1455]
#03: mozilla::StaticPresData::GetFontPrefsForLangHelper [layout/base/StaticPresData.cpp:140]
#04: Gecko_nsStyleFont_GetBaseSize [layout/base/nsPresContext.h:1234]
This is another thing that bug 1318187 would have caught.
Flags: needinfo?(manishearth)
I'm not sure what we should be doing here. Perhaps stuffing it inside a C++ side read write lock? There's no easy way to move this to the main thread because it can't happen before compute time.
Flags: needinfo?(manishearth)
GetFontPrefsForLangHelper doesn't seem to be thread-safe anyway, it maintains a single linked list for cache of LangGroupFontPrefs.

Probably we should delegate the request to the main thread, and have it return needed LangGroupFontPrefs. Traversing that linked list is probably thread-safe, so I guess we don't need to cache that in TLS.
Alternatively, we can probably change how Gecko handles it, and initialize a big table which includes all necessary information (probably in a hash table with language as the key), and use it as a shared immutable table for styling. That way we don't need to do any dynamic cache manipulation, nor read prefs from non-main-thread.

It seems to me by default we only have like ~30 languages listed in about:config, so it isn't too big to put into memory altogether. Actually, the current per-document linked-list cache setting may easily take more memory than a global table if there are many documents open.
Oh, hmm, actually StaticPresData is a shared singleton, so it is unbeatable on memory usage... but I guess we can still have all data read into this to avoid further mutation on it.

If I estimate correctly, all those data should only take ~35KiB memory in total, which seems to be cheap given it would be a per-process thing.

And doing so would also make us able to respond to changes on those prefs dynamically.

jfkthame, what do you think about the idea that we pre-fill the static font data cache to make it safe to access from multiple thread?
Flags: needinfo?(jfkthame)
I'm not too worried about the memory usage of pulling it all into a hash table; but what might be a concern is that this means we'd be parsing all those prefs on the startup path, ahead of first render, instead of reading them only as-needed. I wonder if that will be an issue, given how strongly we care about every millisecond of startup time...?
Flags: needinfo?(jfkthame)
<body style="font-family: fixed"><span lang="ar">text</span></body> is a testcase that crashes locally for me.
I think we should do the same thing here that we plan to do for font metrics: use a mutex on the FFI function, and cache the result in the ThreadLocalStyleContext so that we only need to make the call at most once per (worker thread, font) tuple.

This blocks running with the parallel traversal, which blocks bug 1318187, so this is high priority to fix.
Assignee: nobody → manishearth
The mutex makes this safe, but we'll still have that assert in libpref. I don't want to remove it since other consumers need to be protected from threadsafety issues as well. Should I duplicate the methods with non-asserty ones or something?

Also, is the main thread completely paused during parallel traversal or are there other events happening there that can touch prefs?
(In reply to Manish Goregaokar [:manishearth] from comment #10)
> The mutex makes this safe, but we'll still have that assert in libpref. I
> don't want to remove it since other consumers need to be protected from
> threadsafety issues as well. Should I duplicate the methods with non-asserty
> ones or something?

I guess you could MOZ_ASSERT(NS_IsMainThread() || ServoStyleSet::InServoTraversal());

> Also, is the main thread completely paused during parallel traversal or are
> there other events happening there that can touch prefs?

Yes, it's completely paused.
Okay, that works. I guess Rust has spoiled me when it comes to thread safety; was hoping to end up with something more watertight, but that seems okay I guess.
Wait, we went through some work to make the prefs-service main-thread only--can we please not start adding random mutexes and asserts simply because Stylo happens to need a pref?
It would probably be good to be a bit more strict with the assertion, and assert that, if we're in the Servo traversal, we're holding the stylo-pref-mutex.

(In reply to Nathan Froyd [:froydnj] from comment #13)
> Wait, we went through some work to make the prefs-service main-thread
> only--can we please not start adding random mutexes and asserts simply
> because Stylo happens to need a pref?

Can you clarify your objection here? We're not adding concurrent access to the prefs service, we're only adding sequentialized access when the main thread is is paused. As long as the pref service doesn't store things in TLS, and as long as we can swallow relaxing the assertion in the stylo case, it should be fine.

In case it wasn't clear, the mutex would be in the Stylo FFI function, not in the pref machinery itself.

This seems a lot better than the startup time implications of comment 6. But if we'd really rather do that and think it wouldn't be a big deal, I'm certainly fine with that too.
Comment on attachment 8854940 [details]
Bug 1351200 - Part 1: stylo: Allow fetching prefs on servo traversal threads;

https://reviewboard.mozilla.org/r/126860/#review129544

I agree with the assert, but not where it's placed, so it's entirely possible I'm misunderstanding something here.

::: modules/libpref/prefapi.cpp:787
(Diff revision 1)
>  }
>  
>  nsresult pref_HashPref(const char *key, PrefValue value, PrefType type, uint32_t flags)
>  {
>  #ifndef MOZ_B2G
> -    MOZ_ASSERT(NS_IsMainThread());
> +    MOZ_ASSERT(NS_IsMainThread() || mozilla::ServoStyleSet::IsInServoTraversal());

The commit message says "fetching prefs", but the function here permits adding prefs.  Surely we're not permitting Stylo threads to set prefs?

The original bug report says that we're hitting an assert coming through `PREF_CopyCharPref`, which calls `pref_HashTableLookup`, which also has a main thread assertion.  (But the assertion listed in the original report is in `pref_HashPref`, which seems nonsensical...)  I think we want the check there instead?
Attachment #8854940 - Flags: review?(nfroyd) → review-
My bad, I think that's due to messed up debuginfo?

I thought HashPref was called by the HT lookup (from its name), but that seems to be different.
Comment on attachment 8854940 [details]
Bug 1351200 - Part 1: stylo: Allow fetching prefs on servo traversal threads;

https://reviewboard.mozilla.org/r/126860/#review129548

This looks much better.  Did you test this forcing parallel traversal on?
Attachment #8854940 - Flags: review?(nfroyd) → review+
Plan to, build still running.
Works now.

One issue with the design for the provider is that for em/ch units we need a bit more things kept in it, like the pres context. However, because it's used in a trait as an associated type, it can't borrow from other things. I might store a `RefPtr<nsPresContext>` instead.
Comment on attachment 8854943 [details]
Bug 1351200 - Part 4: stylo: Store font metrics provider in thread local style context;

https://reviewboard.mozilla.org/r/126866/#review129566

::: servo/components/style/animation.rs:540
(Diff revision 3)
>  /// Updates a single animation and associated style based on the current time.
>  /// If `damage` is provided, inserts the appropriate restyle damage.
>  pub fn update_style_for_animation(context: &SharedStyleContext,
>                                    animation: &Animation,
> -                                  style: &mut Arc<ComputedValues>) {
> +                                  style: &mut Arc<ComputedValues>,
> +                                  font_metrics: &FontMetricsProvider) {

font_metrics is a confusing name, please call it font_metrics_provider, or something less confusing.

The metrics are the actual values that get returned by the provider, not the provider itself.

::: servo/components/style/context.rs:282
(Diff revision 3)
>      /// Statistics about the traversal.
>      pub statistics: TraversalStatistics,
>      /// Information related to the current element, non-None during processing.
>      current_element_info: Option<CurrentElementInfo>,
> +    /// Font metrics
> +    pub font_metrics: E::FontMetrics,

Similarly, font_metrics seems a really confusing name. Also, can we make it private and add an accessor instead?

::: servo/components/style/dom.rs:283
(Diff revision 3)
>  pub trait TElement : PartialEq + Debug + Sized + Copy + Clone + ElementExt + PresentationalHintsSynthetizer {
>      /// The concrete node type.
>      type ConcreteNode: TNode<ConcreteElement = Self>;
>  
> +    /// Type of the font metrics provider
> +    type FontMetrics: FontMetricsProvider;

This type really seems out of place, could we instead parameterize ThreadLocalStyleContext by FontMetricsProvider?

If not, why? And if it's too cumbersone, please leave a TODO in here, explaining why this design.

::: servo/components/style/font_metrics.rs:36
(Diff revision 3)
>      /// The font is not available.
>      NotAvailable,
>  }
>  
>  /// A trait used to represent something capable of providing us font metrics.
> -pub trait FontMetricsProvider: Send + Sync + fmt::Debug {
> +pub trait FontMetricsProvider: Send + fmt::Debug {

Let's make it `Sized` by default and remove the extra where clauses elsewhere?

::: servo/components/style/gecko/media_queries.rs:513
(Diff revision 3)
>              inherited_style: default_values,
>              layout_parent_style: default_values,
>              // This cloning business is kind of dumb.... It's because Context
>              // insists on having an actual ComputedValues inside itself.
>              style: default_values.clone(),
> -            font_metrics_provider: None,
> +            font_metrics_provider: &DummyProvider,

What should happen when someone adds a media query with `ch` units? Could we leave a `FIXME` here?

::: servo/components/style/gecko/wrapper.rs:432
(Diff revision 3)
>      }
>  }
>  
> +#[derive(Debug)]
> +/// Gecko font metrics provider
> +pub struct FontMetrics {

Please call this other thing that isn't `FontMetrics`. The font metrics are the font size/ascent/etc, not this, and I think this name is confusing. Ditto for the type parameter.

::: servo/components/style/gecko/wrapper.rs:433
(Diff revision 3)
>  }
>  
> +#[derive(Debug)]
> +/// Gecko font metrics provider
> +pub struct FontMetrics {
> +    /// Cache of font sizes

Cache of font-sizes to what? Please describe what this is doing.

::: servo/components/style/servo/media_queries.rs:189
(Diff revision 3)
>              inherited_style: default_values,
>              layout_parent_style: default_values,
>              // This cloning business is kind of dumb.... It's because Context
>              // insists on having an actual ComputedValues inside itself.
>              style: default_values.clone(),
> -            font_metrics_provider: None
> +            // will not be used

It could, please leave a similar `FIXME` here, though this is not relevant for stylo.

::: servo/components/style/values/computed/mod.rs:61
(Diff revision 3)
>      pub style: ComputedValues,
>  
>      /// A font metrics provider, used to access font metrics to implement
>      /// font-relative units.
>      ///
>      /// TODO(emilio): This should be required, see #14079.

Remove this `TODO`?

::: servo/components/style/viewport.rs:698
(Diff revision 3)
>              is_root_element: false,
>              device: device,
>              inherited_style: device.default_computed_values(),
>              layout_parent_style: device.default_computed_values(),
>              style: device.default_computed_values().clone(),
> -            font_metrics_provider: None, // TODO: Should have!
> +            font_metrics_provider: &DummyProvider, // TODO: Should have!

ditto.
Attachment #8854943 - Flags: review?(emilio+bugs)
Comment on attachment 8854944 [details]
Bug 1351200 - Part 5: stylo: Use font metrics provider as a cache for font size results;

https://reviewboard.mozilla.org/r/126868/#review129572

::: layout/style/ServoBindings.cpp:1535
(Diff revision 3)
>  FontSizePrefs
> -Gecko_GetBaseSize(nsIAtom* aLanguage, RawGeckoPresContextBorrowed aPresContext)
> +Gecko_GetBaseSize(nsIAtom* aLanguage)
>  {
> -  nsIAtom* lang =  aLanguage ? aLanguage : aPresContext->GetLanguageFromCharset();
>    LangGroupFontPrefs prefs;
> +  nsIAtom *langGroupAtom = StaticPresData::Get()->GetLangGroup(aLanguage);

nit: `nsIAtom*`.

::: servo/components/style/gecko/wrapper.rs:443
(Diff revision 3)
>  
>  impl FontMetrics {
>      /// Construct
>      pub fn new() -> Self {
>          FontMetrics {
>              font_size_cache: RefCell::new(Vec::new()),

Perhaps this should re-use the `LRUCache` infrastructure, what do you think?
Attachment #8854944 - Flags: review?(emilio+bugs)
Comment on attachment 8854944 [details]
Bug 1351200 - Part 5: stylo: Use font metrics provider as a cache for font size results;

https://reviewboard.mozilla.org/r/126868/#review129572

> Perhaps this should re-use the `LRUCache` infrastructure, what do you think?

That just gets us an eviction policy, which we don't need. This vec will hold at most 31 entries.
Comment on attachment 8854943 [details]
Bug 1351200 - Part 4: stylo: Store font metrics provider in thread local style context;

https://reviewboard.mozilla.org/r/126866/#review129566

> This type really seems out of place, could we instead parameterize ThreadLocalStyleContext by FontMetricsProvider?
> 
> If not, why? And if it's too cumbersone, please leave a TODO in here, explaining why this design.

Because it's cumbersome and makes all the style contexts have an extra parameter. I also feel that backend-specific stuff should either be part of the TElement/Tnode set of traits, or compile-time, having yet another different mechanism complicates things IMO.

> Let's make it `Sized` by default and remove the extra where clauses elsewhere?

No, that means you can't make trait objects out of it.

> What should happen when someone adds a media query with `ch` units? Could we leave a `FIXME` here?

Didn't occur to me, will put nondummy.
Attachment #8854944 - Attachment is obsolete: true
Comment on attachment 8854941 [details]
Bug 1351200 - Part 2: stylo: Separate out caching code from font pref initialization code in GetFontPrefsForLang;

https://reviewboard.mozilla.org/r/126862/#review129722

Bobby told me it was fine to steal review from here. This doesn't seem trivial, so I may need to take a closer look tomorrow, meanwhile, a few nits.

::: layout/base/StaticPresData.h:59
(Diff revision 3)
>      }
>      return n;
>    }
>  
> +  // Initialize this with the data for a given language
> +  void Initialize(nsIAtom *langGroupAtom);

nit: the star with the type. use `aFoo` for the argument.

::: layout/base/StaticPresData.cpp:251
(Diff revision 3)
> +  }
> +  return langGroupAtom;
> +}
> +
> +const LangGroupFontPrefs*
> +StaticPresData::GetFontPrefsForLangHelper(nsIAtom *aLanguage,

ditto
Comment on attachment 8854941 [details]
Bug 1351200 - Part 2: stylo: Separate out caching code from font pref initialization code in GetFontPrefsForLang;

https://reviewboard.mozilla.org/r/126862/#review129730

::: layout/base/StaticPresData.h:96
(Diff revision 3)
>     * This table maps border-width enums 'thin', 'medium', 'thick'
>     * to actual nscoord values.
>     */
>    const nscoord* GetBorderWidthTable() { return mBorderWidthTable; }
>  
> +  // Given a language, get the language group name

Let's be consistent with the rest of the comments and spacing and use `/**`-style comments here.

A bit more detail would be nice.

::: layout/base/StaticPresData.cpp:242
(Diff revision 3)
> +
> +nsIAtom*
> +StaticPresData::GetLangGroup(nsIAtom* aLanguage) const
> +{
> +  nsresult rv = NS_OK;
> +  nsIAtom *langGroupAtom = nullptr;

Also, star.

::: layout/base/StaticPresData.cpp:243
(Diff revision 3)
> +nsIAtom*
> +StaticPresData::GetLangGroup(nsIAtom* aLanguage) const
> +{
> +  nsresult rv = NS_OK;
> +  nsIAtom *langGroupAtom = nullptr;
> +  langGroupAtom = mLangService->GetLanguageGroup(aLanguage, &rv);

After looking with a bit more detail, this isn't thread-safe at all, and you call it from the parallel traversal.

There's a hashtable write here: http://searchfox.org/mozilla-central/rev/b8cce081200129a1307e2a682f121135b5ca1d19/intl/locale/nsLanguageAtomService.cpp#96

So this needs more work at least.
Attachment #8854941 - Flags: review-
Attachment #8854941 - Flags: review?(bobbyholley)
Comment on attachment 8854942 [details]
Bug 1351200 - Part 3: stylo: Bypass cache when fetching font size prefs from Stylo;

Emilio is taking these - thanks Emilio!
Attachment #8854942 - Flags: review?(bobbyholley)
Comment on attachment 8854943 [details]
Bug 1351200 - Part 4: stylo: Store font metrics provider in thread local style context;

https://reviewboard.mozilla.org/r/126866/#review129876

Clearing review until the previous patches are sorted out.
Attachment #8854943 - Flags: review?(emilio+bugs)
Blocks: 1318187
Comment on attachment 8854941 [details]
Bug 1351200 - Part 2: stylo: Separate out caching code from font pref initialization code in GetFontPrefsForLang;

https://reviewboard.mozilla.org/r/126862/#review130324

::: layout/base/StaticPresData.h:101
(Diff revision 5)
>    /**
> +   * Given a language, get the language group name, which can
> +   * be used as an argument to LangGroupFontPrefs::Initialize()
> +   *
> +   */
> +  nsIAtom* GetLangGroup(nsIAtom* aLanguage) const;

nit: Newline here.

::: layout/base/StaticPresData.cpp:261
(Diff revision 5)
> +  MOZ_ASSERT(mLangService);
> +  MOZ_ASSERT(aPrefs);
> +
> +  nsIAtom* langGroupAtom = GetLangGroup(aLanguage);
> +
> +  LangGroupFontPrefs *prefs = const_cast<LangGroupFontPrefs*>(aPrefs);

nit: Star.
Attachment #8854941 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8854942 [details]
Bug 1351200 - Part 3: stylo: Bypass cache when fetching font size prefs from Stylo;

https://reviewboard.mozilla.org/r/126864/#review129726

r- because of the atom lifetime issue.

::: layout/style/ServoBindings.cpp:1537
(Diff revision 3)
> -Gecko_nsStyleFont_GetBaseSize(const nsStyleFont* aFont, RawGeckoPresContextBorrowed aPresContext)
> +Gecko_GetBaseSize(nsIAtom* aLanguage, RawGeckoPresContextBorrowed aPresContext)
>  {
> -  return aPresContext->GetDefaultFont(aFont->mGenericID, aFont->mLanguage)->size;
> +  nsIAtom* lang =  aLanguage ? aLanguage : aPresContext->GetLanguageFromCharset();
> +  LangGroupFontPrefs prefs;
> +
> +  nsIAtom *langGroupAtom = StaticPresData::Get()->GetLangGroup(lang);

nit: star.

::: intl/locale/nsLanguageAtomService.h:33
(Diff revision 5)
>  
>    virtual nsIAtom* GetLocaleLanguage() override;
>  
>    virtual nsIAtom* GetLanguageGroup(nsIAtom *aLanguage,
>                                                  nsresult *aError) override;
> +  virtual nsIAtom* GetUncachedLanguageGroup(nsIAtom *aLanguage,

Stars (and if while you are here you update the indentation and star position in the method above it, that'd be great).

::: intl/locale/nsLanguageAtomService.cpp:82
(Diff revision 5)
> +  }
> +
> +  return retVal;
> +}
> +
> +nsIAtom*

This is now returning a potentially-destroyed atom pointer, if the lang group string doesn't exist in the atoms table, and the destruction of the atom happens to trigger an atom table GC:

```
nsCOMPtr<nsIAtom> atom = NS_Atomize(..);

return atom.get(); // atom is destroyed here.
```

This needs to return an already_Addrefed, or something like that.

::: intl/locale/nsLanguageAtomService.cpp:86
(Diff revision 5)
> +
> +nsIAtom*
> +nsLanguageAtomService::GetUncachedLanguageGroup(nsIAtom *aLanguage,
> +                                                nsresult *aError) const
> +{
> +  nsIAtom *retVal;

Star, here and in the method signatures.
Attachment #8854942 - Flags: review?(emilio+bugs) → review-
Comment on attachment 8854942 [details]
Bug 1351200 - Part 3: stylo: Bypass cache when fetching font size prefs from Stylo;

https://reviewboard.mozilla.org/r/126864/#review130522

Looks good, just a few minor issues and a question. I'd still want to take a final look to it before it lands.

::: intl/locale/nsLanguageAtomService.cpp:70
(Diff revision 6)
> -                                        nsresult *aError)
> +                                        nsresult* aError)
>  {
> -  nsIAtom *retVal;
> +  nsCOMPtr<nsIAtom> retVal;
> -  nsresult res = NS_OK;
>  
>    retVal = mLangToGroup.GetWeak(aLanguage);

This is now unnecessarily addreffing the pointer. Let's do:


```
nsIAtom* retVal = mLangToGroup.GetWeak(aLanguage);

if (!retVal) {
  nsCOMPtr<nsIAtom> uncached = GetUncachedLanguageGroup(aLanguage, aError);
  retVal = uncached.get();
  mLangToGroup.Put(aLanguage, Move(uncached));
}

return retVal;
```

::: layout/base/StaticPresData.cpp:254
(Diff revision 6)
>  
> +already_AddRefed<nsIAtom>
> +StaticPresData::GetUncachedLangGroup(nsIAtom* aLanguage) const
> +{
> +  nsresult rv = NS_OK;
> +  nsCOMPtr<nsIAtom> langGroupAtom = nullptr;

What's the point of not assigning it directly?

```
nsCOMPtr<nsIAtom> langGroupAtom =
  mLangService->GetUncachedLanguageGroup(aLanguage, &rv);
// ...
```

::: servo/components/style/properties/longhand/font.mako.rs:566
(Diff revision 6)
>                  static FONT_SIZE_FACTORS: [i32; 8] = [60, 75, 89, 100, 120, 150, 200, 300];
>  
>                  // XXXManishearth handle quirks mode
>  
> -                let base_size = unsafe {
> -                    Gecko_nsStyleFont_GetBaseSize(cx.style().get_font().gecko(),
> +                let base_sizes = unsafe {
> +                    Gecko_GetBaseSize(cx.style().get_font().gecko().mLanguage.raw(),

Do we guarantee that `mLanguage` is properly computed before `font-size`?
Attachment #8854942 - Flags: review?(emilio+bugs)
Comment on attachment 8854943 [details]
Bug 1351200 - Part 4: stylo: Store font metrics provider in thread local style context;

https://reviewboard.mozilla.org/r/126866/#review130524

It's quite sad we can't declare the providers from outside of style right now. I believe we should be able to, mind filing an issue for that, and dropping a link near the `create_device_for_product` functions?

::: servo/components/layout/animation.rs:148
(Diff revision 7)
>              for animation in animations.iter() {
>                  let old_style = fragment.style.clone();
>                  update_style_for_animation(&context.style_context,
>                                             animation,
> -                                           &mut fragment.style);
> +                                           &mut fragment.style,
> +                                           &DummyProvider);

nit: Perhaps we should alias `DummyProvider` to `ServoFontMetricsProvider` and use it where appropriate?

That'd differenciate between "we don't care about it" vs. "it's not implemented yet".

::: servo/components/style/animation.rs:415
(Diff revision 7)
>  
>  fn compute_style_for_animation_step(context: &SharedStyleContext,
>                                      step: &KeyframesStep,
>                                      previous_style: &ComputedValues,
> -                                    style_from_cascade: &ComputedValues)
> +                                    style_from_cascade: &ComputedValues,
> +                                    font_metrics_provider: &FontMetricsProvider)

This should have access to `StyleContext<E>`, instead of passing `SharedStyleContext` and the `FontMetricsProvider` separately, any reason it can't be done?

::: servo/components/style/animation.rs:665
(Diff revision 7)
>              // TODO: How could we optimise it? Is it such a big deal?
>              let from_style = compute_style_for_animation_step(context,
>                                                                last_keyframe,
>                                                                &**style,
> -                                                              &state.cascade_style);
> +                                                              &state.cascade_style,
> +                                                              font_metrics_provider);

ditto

::: servo/components/style/context.rs:281
(Diff revision 7)
>      pub tasks: Vec<SequentialTask<E>>,
>      /// Statistics about the traversal.
>      pub statistics: TraversalStatistics,
>      /// Information related to the current element, non-None during processing.
>      current_element_info: Option<CurrentElementInfo>,
> +    /// Font metrics

Please ellaborate a bit here: 

```
/// The struct used to compute and cache font metrics from style, to evaluate some font-relative units and font-size
```

For example?

::: servo/components/style/context.rs:285
(Diff revision 7)
>      current_element_info: Option<CurrentElementInfo>,
> +    /// Font metrics
> +    font_metrics_provider: E::FontMetricsProvider,
>  }
>  
> +

nit: stray newline.

::: servo/components/style/font_metrics.rs:57
(Diff revision 7)
> +    fn create_from(context: &SharedStyleContext) -> Self where Self: Sized;
> +}
> +
> +#[derive(Debug)]
> +/// Dummy font metrics provider, for use by Servo
> +/// and in cases where gecko doesn't need font metrics

nit: capitalize Gecko ;)

::: servo/components/style/font_metrics.rs:74
(Diff revision 7)
> +    ::gecko::wrapper::GeckoFontMetricsProvider::new()
> +}
> +
> +#[cfg(feature = "servo")]
> +/// Construct a font metrics provider for the current product
> +pub fn get_metrics_provider_for_product() -> DummyProvider {

Perhaps mention that this should be moved out of style? Seems really unfortunate to have this here, ideally the provider should just live in `ports/geckolib`.

::: servo/components/style/gecko/media_queries.rs:503
(Diff revision 7)
>                        self.feature.mRangeType == nsMediaFeature_RangeType::eMinMaxAllowed,
>                        "Whoops, wrong range");
>  
>          let default_values = device.default_computed_values();
>  
> +        let provider = get_metrics_provider_for_product();

This won't work for Servo, right? But I see why it's a pain to implement properly :(

::: servo/components/style/viewport.rs:700
(Diff revision 7)
>              is_root_element: false,
>              device: device,
>              inherited_style: device.default_computed_values(),
>              layout_parent_style: device.default_computed_values(),
>              style: device.default_computed_values().clone(),
> -            font_metrics_provider: None, // TODO: Should have!
> +            font_metrics_provider: &provider,

This is only used to resolve lengths, so this can be a `DummyProvider`, please add a comment to that effect.
Attachment #8854943 - Flags: review?(emilio+bugs)
Comment on attachment 8854942 [details]
Bug 1351200 - Part 3: stylo: Bypass cache when fetching font size prefs from Stylo;

https://reviewboard.mozilla.org/r/126864/#review130522

> Do we guarantee that `mLanguage` is properly computed before `font-size`?

I thought we did, because font-size and system fonts need this, but we don't. Fixing.
Comment on attachment 8854943 [details]
Bug 1351200 - Part 4: stylo: Store font metrics provider in thread local style context;

https://reviewboard.mozilla.org/r/126866/#review130524

> This should have access to `StyleContext<E>`, instead of passing `SharedStyleContext` and the `FontMetricsProvider` separately, any reason it can't be done?

Because it makes the function generic, mostly. I'd rather not have generic functions that don't actually need to be generic. It hurts compile times and it makes the code more opaque.

> Perhaps mention that this should be moved out of style? Seems really unfortunate to have this here, ideally the provider should just live in `ports/geckolib`.

No, it should absolutely live in style. ports/geckolib is bindings and bindings only. We need this function because we're using trait objects instead of just compile-time switching. Multiple locations in the style crate need a metrics provider.

> This won't work for Servo, right? But I see why it's a pain to implement properly :(

Why not? This should be fine.

> This is only used to resolve lengths, so this can be a `DummyProvider`, please add a comment to that effect.

Not when we add ex/ch support, which I plan on doing.
(In reply to Manish Goregaokar [:manishearth] from comment #65)
> Because it makes the function generic, mostly. I'd rather not have generic
> functions that don't actually need to be generic. It hurts compile times and
> it makes the code more opaque.

I'd say that making the function take an extra argument in exchange to that is not the best trade-off, nor makes it more opaque, but...

> Why not? This should be fine.

Do you really plan to move all the font code under style?

> Not when we add ex/ch support, which I plan on doing.

You're right here.
> Do you really plan to move all the font code under style?

Heh, didn't realize that. Left a comment to the effect that we should defer handling that till later.
Blocks: 1341724
Comment on attachment 8854942 [details]
Bug 1351200 - Part 3: stylo: Bypass cache when fetching font size prefs from Stylo;

https://reviewboard.mozilla.org/r/126864/#review130632

r=me with those.

::: intl/locale/nsLanguageAtomService.h:32
(Diff revision 7)
>      LookupCharSet(const nsACString& aCharSet) override;
>  
>    virtual nsIAtom* GetLocaleLanguage() override;
>  
> -  virtual nsIAtom* GetLanguageGroup(nsIAtom *aLanguage,
> +  virtual nsIAtom* GetLanguageGroup(nsIAtom* aLanguage,
> -                                                nsresult *aError) override;
> +                                    nsresult *aError) override;

nit: star

::: layout/style/ServoBindings.h:94
(Diff revision 7)
>    const uint8_t* mURLString;
>    uint32_t mURLStringLength;
>    mozilla::css::URLExtraData* mExtraData;
>  };
>  
> +class FontSizePrefs

Just us a `struct` instead of requiring all members `public`?

::: layout/style/ServoBindings.cpp:1540
(Diff revision 7)
> +  nsCOMPtr<nsIAtom> langGroupAtom = StaticPresData::Get()->GetUncachedLangGroup(aLanguage);
> +
> +  prefs.Initialize(langGroupAtom);
> +  FontSizePrefs sizes;
> +
> +  sizes.mDefaultVariableSize = prefs.mDefaultVariableFont.size;

Can we instead add an explicit constructor to `FontSizePrefs` that takes a `const LangGroupFontPrefs&`?

That way we can ensure we initialize all the fields, and feels less out of place.
Attachment #8854942 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8854943 [details]
Bug 1351200 - Part 4: stylo: Store font metrics provider in thread local style context;

https://reviewboard.mozilla.org/r/126866/#review130634

::: servo/components/style/font_metrics.rs:38
(Diff revision 8)
>  }
>  
> +/// Dummy font metrics provider, for use by Servo
> +/// and in cases where Gecko doesn't need font metrics
> +#[derive(Debug)]
> +pub struct DummyProvider;

Since we don't need `DummyProvider` for Gecko at all, let's call it `ServoMetricsProvider` directly and `cfg` it?

::: servo/components/style/font_metrics.rs:57
(Diff revision 8)
> +pub fn get_metrics_provider_for_product() -> ::gecko::wrapper::GeckoFontMetricsProvider {
> +    ::gecko::wrapper::GeckoFontMetricsProvider::new()
> +}
> +
> +#[cfg(feature = "servo")]
> +/// Construct a font metrics provider for the current product

Still needs the comment requested previously (a `TODO: This will need to get moved out of style to implement it properly on servo` would do it.
Attachment #8854943 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8855563 [details]
Bug 1351200 - Part 5: stylo: Use font metrics provider as a cache for font size results;

https://reviewboard.mozilla.org/r/127412/#review130636

::: servo/components/style/font_metrics.rs:56
(Diff revision 4)
> +    }
>  }
>  
> +// Servo's font metrics provider will probably not live in this crate, so this will
> +// have to be replaced with something else (perhaps a trait method on TElement)
> +// when we get there

Oh, you added it here, nevermind then. Please add TODO/FIXME or whatever if you don't mind, so it's easy to grep :)
Attachment #8855563 - Flags: review?(emilio+bugs) → review+
Attachment #8854965 - Attachment is obsolete: true
Attachment #8854943 - Attachment is obsolete: true
Attachment #8855563 - Attachment is obsolete: true
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/767bee78574c
Part 1: stylo: Allow fetching prefs on servo traversal threads; r=froydnj
https://hg.mozilla.org/integration/autoland/rev/792fb55377f5
Part 2: stylo: Separate out caching code from font pref initialization code in GetFontPrefsForLang; r=emilio
https://hg.mozilla.org/integration/autoland/rev/1cce9249b4a2
Part 3: stylo: Bypass cache when fetching font size prefs from Stylo; r=emilio
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7241e9e495e4
Part 1: stylo: Allow fetching prefs on servo traversal threads; r=froydnj
https://hg.mozilla.org/integration/autoland/rev/127354e97d5b
Part 2: stylo: Separate out caching code from font pref initialization code in GetFontPrefsForLang; r=emilio
https://hg.mozilla.org/integration/autoland/rev/d5ae39b31d34
Part 3: stylo: Bypass cache when fetching font size prefs from Stylo; r=emilio
Blocks: stylo
Depends on: 1474789
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: