Closed Bug 1355721 Opened 7 years ago Closed 7 years ago

stylo: Implement @font-feature-values rule support

Categories

(Core :: CSS Parsing and Computation, enhancement, P2)

Product:
Core
Component:
CSS Parsing and Computation
Version:
53 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: xidorn, Assigned: canova)

References

(
URL
)

Details

Attachments

(2 files, 1 obsolete file)

Bug 1355721 - stylo: Implement font feature values lookup
59 bytes, text/x-review-board-request
xidorn
: review+
emilio
: review+
Details
Bug 1355721 - Change nsCSSFontFeatureValuesRule.h to follow style guideline
59 bytes, text/x-review-board-request
emilio
: review+
Details 
This is basically only used in style system with font-variant-alternates, so everything except the CSSOM interfaces should be implemented in Servo side.
Priority: -- → P2
Assignee: nobody → jeremychen
Status: NEW → ASSIGNED
Blocks: stylo-cssom, 1357724
Blocks: 1339656
Blocks: 1356087
No longer blocks: 1356087
Hi Jeremy, after bug 1365900, it seems big portion of the work is finished. Now we need to get these and store rule values inside nsFont when we convert font-variant-alternates into computed value. Do you mind if I take this too? :)
(In reply to Nazım Can Altınova [:canaltinova] from comment #1)
> Hi Jeremy, after bug 1365900, it seems big portion of the work is finished.
> Now we need to get these and store rule values inside nsFont when we convert
> font-variant-alternates into computed value. Do you mind if I take this too?
> :)

Nice work!!!! I'm kind of busy taking other Stylo bugs, so it would be very helpful if you can take over this one as well.

Assign to you, and appreciate your help. :)
Assignee: jeremychen → canaltinova
Priority: P2 → --
Priority: -- → P2
Hi Emilio,
This patch is not completely fine right now, I have a few nits to address and still need to rebase after https://github.com/servo/servo/pull/18037 but I wanted to get some feedback beforehand about setting this lookup.
So I moved the lookup method call to RebuildAllStyleData method in nsPresContext as we discussed. But I forgot one thing, we need to pass this lookup to `nsStyleFont.mFont.featureValueLookup` at some point. Gecko does this during nsRuleNode::SetFont. I had to do that also in computed value conversion(inside font.mako.rs) in servo side but because of the parallel traversal, we don't know which one will execute before. Generally first page load is successfull but after a refresh, it breaks. It would be wonderful if we can set the `nsStyleFont.mFont.featureValueLookup` inside nsPresContext. But I don't think we can do that. Do you know a way or place to set it without a problem?
(In reply to Nazım Can Altınova [:canaltinova] from comment #6)
> Hi Emilio,
> This patch is not completely fine right now, I have a few nits to address
> and still need to rebase after https://github.com/servo/servo/pull/18037 but
> I wanted to get some feedback beforehand about setting this lookup.
> So I moved the lookup method call to RebuildAllStyleData method in
> nsPresContext as we discussed. But I forgot one thing, we need to pass this
> lookup to `nsStyleFont.mFont.featureValueLookup` at some point. Gecko does
> this during nsRuleNode::SetFont. I had to do that also in computed value
> conversion(inside font.mako.rs) in servo side but because of the parallel
> traversal, we don't know which one will execute before. Generally first page
> load is successfull but after a refresh, it breaks. It would be wonderful if
> we can set the `nsStyleFont.mFont.featureValueLookup` inside nsPresContext.
> But I don't think we can do that. Do you know a way or place to set it
> without a problem?

This patch looks generally ok. This has nothing to do with the parallel traversal. This has to do with the fact that RebuildAllStyleData isn't called when you think it's called :).

In particular, it isn't called when CSS rules change, so if you happen to style the page for the first time before loading the stylesheet you care about, then you'll always see the initial value with no rules. I bet this also happens with Gecko, fwiw.

See the comment on [1] to see what RebuildAllStyleData is trying to do... I know it's an awful name :(.

Anyway, to test, you probably can do it on PresShell::RecordStylesheetChange[2]. But you probably want to move at least part of the state to the stylist, I think, to avoid rebuilding it unnecessarily. I don't know how we handle @font-face changes, but we probably want to do something similar. You can probably ask heycam about that, or I can look tomorrow or on monday.

I have a bunch of drive-by nits which I wrote while skimming over the patch btw :)

[1]: http://searchfox.org/mozilla-central/rev/4b79f3b23aebb4080ea85e94351fd4046116a957/layout/base/ServoRestyleManager.cpp#222
[2]: http://searchfox.org/mozilla-central/rev/4b79f3b23aebb4080ea85e94351fd4046116a957/layout/base/PresShell.cpp#4593
Comment on attachment 8896445 [details]
Bug 1355721 - stylo: Implement font feature values lookup

https://reviewboard.mozilla.org/r/167692/#review172902

::: layout/base/nsPresContext.h:1190
(Diff revision 1)
>    }
>  
>    nsBidi& GetBidiEngine();
>  
> +  // Fetch object for looking up font feature values
> +  void GetFontFeatureValuesLookup();

This really needs to be private, and instead of `Get`, let's call it `EnsureFontFeatureValuesLookup`.

Let's keep a `Get` method that just returns it.

::: layout/base/nsPresContext.cpp:3129
(Diff revision 1)
>  }
>  
> +void
> +nsPresContext::GetFontFeatureValuesLookup()
> +{
> +    StyleSetHandle styleSet = mShell->StyleSet();

nit: Indentation.

::: layout/base/nsPresContext.cpp:3135
(Diff revision 1)
> +    nsTArray<nsCSSFontFeatureValuesRule*> rules;
> +    styleSet->AppendFontFeatureValuesRules(rules);
> +
> +    mFontFeatureValuesLookup = new gfxFontFeatureValueSet();
> +
> +    uint32_t i, numRules = rules.Length();

Seems like if the rules are empty you could just avoid creating the object, right?

::: layout/base/nsPresContext.cpp:3135
(Diff revision 1)
> +    nsTArray<nsCSSFontFeatureValuesRule*> rules;
> +    styleSet->AppendFontFeatureValuesRules(rules);
> +
> +    mFontFeatureValuesLookup = new gfxFontFeatureValueSet();
> +
> +    uint32_t i, numRules = rules.Length();

nit: You don't use `i` (nor `f` below, for that matter). You could do:

```
for (nsCSSFontFeatureValuesRule* rule : mRules) {
```

And:

```
for (const FontFamilyName& name : familyList) {
```

Which is much nicer to read I think.
Attachment #8896445 - Flags: review?(emilio+bugs)
Comment on attachment 8896444 [details]
Bug 1355721 - Convert to lowercase the @font-feature-values declarations

https://reviewboard.mozilla.org/r/167690/#review172910

This looks fine.

Could you on another commit, and given we're moving it anyway, format the file so that it conforms to the style guide? (that is, only `override`, not `virtual`, etc...)?

Thanks!
Attachment #8896444 - Flags: review?(emilio+bugs)
Comment on attachment 8896444 [details]
Bug 1355721 - Convert to lowercase the @font-feature-values declarations

https://reviewboard.mozilla.org/r/167690/#review172914

Err, I intended to r+ this :)
Attachment #8896444 - Flags: review+
Latest try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=74f65456b032aa988eb9f8bb36b6e7e49c48839f

So this looks working currently. I had to convert an assertion into an early return to do that. 
Normally, we have 2 main entries to `nsDocument::EndUpdate` method. One is `StyleSheet::SetComplete -> XULDocument::EndUpdate` and the other one is `SVGSVGElement::BindToTree -> nsDocument::EnsureOnDemandBuiltInUASheet`. So the second one is for svg elements. Because of the second one, I was hitting to this assertion http://searchfox.org/mozilla-central/source/layout/style/nsStyleSet.cpp#2291. The normal html files don't hit to that assertion, just svg files(previous try run with assertion https://treeherder.mozilla.org/#/jobs?repo=try&revision=726571869416ba2cf2a0913c5ebca9bb80ce70dc). We don't need to worry about @font-feature-values rule inside svg files. So just an early return seems reasonable here. What do you think?
Comment on attachment 8896847 [details]
Bug 1355721 - Change nsCSSFontFeatureValuesRule.h to follow style guideline

https://reviewboard.mozilla.org/r/168138/#review173876
Attachment #8896847 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8896445 [details]
Bug 1355721 - stylo: Implement font feature values lookup

https://reviewboard.mozilla.org/r/167692/#review173878

So, this looks pretty good, but I don't think the `mBatching == 0` changes is correct. My suggestion is below, but I'd check with Cameron too to see if that looks acceptable to him.

::: gfx/thebes/gfxFontFeatures.h:53
(Diff revision 3)
>      return (a.alternate == b.alternate) && (a.value == b.value);
>  }
>  
>  class gfxFontFeatureValueSet final {
>  public:
> -    NS_INLINE_DECL_REFCOUNTING(gfxFontFeatureValueSet)
> +    NS_INLINE_DECL_THREADSAFE_REFCOUNTING(gfxFontFeatureValueSet)

This shouldn't be needed, right?

::: layout/base/PresShell.cpp:2549
(Diff revision 3)
>    if (aUpdateType & UPDATE_STYLE) {
>      mStyleSet->EndUpdate();
>      if (mStyleSet->StyleSheetsHaveChanged()) {
>        RestyleForCSSRuleChanges();
> +      if (mPresContext) {
> +        mPresContext->EnsureFontFeatureValuesLookup();

Let's move this to RestyleForCSSRuleChanges.

::: layout/base/nsPresContext.h:1189
(Diff revision 3)
>      mHasWarnedAboutTooLargeDashedOrDottedRadius = true;
>    }
>  
>    nsBidi& GetBidiEngine();
>  
> +  RefPtr<gfxFontFeatureValueSet> GetFontFeatureValuesLookup() const {

This doesn't need to return a copy of the refptr, right? Let's just return `gfxFontFeatureValueSet*`.

::: layout/base/nsPresContext.cpp:3139
(Diff revision 3)
> +  for (nsCSSFontFeatureValuesRule* rule : rules) {
> +    const nsTArray<FontFamilyName>& familyList = rule->GetFamilyList().GetFontlist();
> +    const nsTArray<gfxFontFeatureValueSet::FeatureValues>&
> +      featureValues = rule->GetFeatureValues();
> +
> +    for (FontFamilyName f : familyList) {

Let's use `const FontFamilyName&`, to avoid copying the string that it contains unnecessarily.

::: layout/style/nsStyleSet.cpp:2290
(Diff revision 3)
>  bool
>  nsStyleSet::AppendFontFeatureValuesRules(
>                                   nsTArray<nsCSSFontFeatureValuesRule*>& aArray)
>  {
>    NS_ENSURE_FALSE(mInShutdown, false);
> -  NS_ASSERTION(mBatching == 0, "rule processors out of date");
> +  if (mBatching != 0) {

I don't think this is ok. For some reason we're nesting styleset updates. I think what needs to happen is that RestyleForCSSRuleChanges should only be called for the last of these updates.

Currently `nsIPresShell::mUpdateCount` is `DEBUG`-only, but that can change... Probably I'd check with Cam first?

::: servo/components/style/stylist.rs:170
(Diff revision 3)
>          }
>      }
>  }
>  
>  #[cfg(feature = "gecko")]
>  impl PerOriginExtraStyleData {

This needs a rebase.
Comment on attachment 8896445 [details]
Bug 1355721 - stylo: Implement font feature values lookup

https://reviewboard.mozilla.org/r/167692/#review173886
Attachment #8896445 - Flags: review?(emilio+bugs)
Cameron, what do you think about the nsStyleSet bits in comment 16, and comment 18?
(see comment 20, whoops)
Flags: needinfo?(cam)
Blocks: 1389187
So, what we do for counter styles is that, we flush it in PresShell::DoFlushPendingNotifications() (via calling FlushCounterStyles()), just before we start restyling.

When the stylesheet is changed, nsIPresShell::RestyleForCSSRuleChanges() calls RebuildCounterStyles() to mark the content dirty, and dispatch a runnable. That runnable would be competing with the notification flushing. Whoever runs first would update the counter styles, and the other one would just do nothing.

It seems to me this matches what font set does as well. (Actually, handling of counter styles mostly follows how font set is handled.)

I think the same approach applies to font feature set as well.

You can create a RebuildFontFeatureSet() which simply destroy the existing font feature set and dispatch a runnable, and call it from RestyleForCSSRuleChanges(). Then create a FlushFontFeatureSet() to actually create the set, and call it from DoFlushPendingNotifications().

That should solve this problem, I suppose.

But I think the existence of this mechanism is not really for avoiding nested style update, but for laziness. That said, the script can apply multiple changes onto stylesheets, while we don't want to rebuild everything every time.
Attachment #8896445 - Flags: review?(emilio+bugs) → review?(xidorn+moz)
Comment on attachment 8896445 [details]
Bug 1355721 - stylo: Implement font feature values lookup

https://reviewboard.mozilla.org/r/167692/#review175986

::: servo/components/style/properties/helpers.mako.rs:373
(Diff revision 5)
>                                       &specified_value,
>                                       computed,
>                                   );
>                              % else:
> +                                % if product == "gecko" and property.ident == "font_variant_alternates":
> +                                    longhands::font_variant_alternates::font_feature_values_lookup(context);

Why can't this be in `{set/copy/reset}_font_variant_alternates` instead?

I'd prefer no more special-casing.

::: servo/components/style/stylesheets/font_feature_values_rule.rs:53
(Diff revision 5)
>          dest.write_str(";")
>      }
>  }
>  
> +/// A trait for @font-feature-values rule to gecko values conversion.
> +pub trait DeclToGeckoValues {

This trait name is unfortunate...

::: servo/components/style/stylesheets/font_feature_values_rule.rs:120
(Diff revision 5)
>      }
>  }
>  
> +impl DeclToGeckoValues for PairValues {
> +    fn set_to_gecko(&self, array: &mut nsTArray<c_uint>) {
> +        let len = match self.1 {

nit: can use `if self.1.is_some() { 2 } else { 1 }`.

::: servo/components/style/stylesheets/font_feature_values_rule.rs:127
(Diff revision 5)
> +            None => 1,
> +        };
> +
> +        unsafe { array.set_len(len); }
> +        array[0] = self.0 as c_uint;
> +        match self.1 {

nit: can use if let.
Comment on attachment 8896445 [details]
Bug 1355721 - stylo: Implement font feature values lookup

https://reviewboard.mozilla.org/r/167692/#review176040

::: servo/components/style/properties/longhand/font.mako.rs:1476
(Diff revision 6)
> +        #[cfg(feature = "gecko")]
> +        #[derive(Debug, Clone, PartialEq)]
> +        // todo implement to_css
> +        pub struct T {
> +            pub value: super::VariantAlternatesList,
> +            pub pres_context: Option<*const nsPresContext>,

I had to do that optinal because we don't have the Context or nsPresContext inside copy function. I had to set None in that function.
Comment on attachment 8896445 [details]
Bug 1355721 - stylo: Implement font feature values lookup

https://reviewboard.mozilla.org/r/167692/#review175986

> This trait name is unfortunate...

Yes, but that was the best name that comes to my mind :(
Comment on attachment 8896445 [details]
Bug 1355721 - stylo: Implement font feature values lookup

https://reviewboard.mozilla.org/r/167692/#review176112

::: layout/base/PresShell.cpp:4586
(Diff revision 6)
> +  if (mPresContext) {
> +    mPresContext->RebuildFontFeatureValues();
> +  }

Why check `mPresContext` again? Just put this into the previous if-block should be fine.

::: layout/style/ServoStyleSet.cpp:1347
(Diff revision 6)
> +bool
> +ServoStyleSet::AppendFontFeatureValuesRules(
> +                                 nsTArray<nsCSSFontFeatureValuesRule*>& aArray)
> +{
> +  UpdateStylistIfNeeded();
> +  Servo_StyleSet_GetFontFeatureValuesRule(mRawSet.get(), &aArray);
> +  return true;
> +}

This looks wrong to me. Who owns the rules you create here? Aren't you simply leaking all the font-feature rules for each font feature set flush?

Basically this interface looks unfortunate to me. We are allocating rules temporarily just for adding them to font feature set.

Can you change this method to simply return a font feature set so that we don't need to create those short-lived rules?
Attachment #8896445 - Flags: review-
Changed servo side to pass directly `gfxFontFeatureValueSet` instead. This way is more clear. Removed the first commit about that moves `nsCSSFontFeatureValuesRule` declaration to `nsCSSFontFeatureValuesRule.h` because it's not needed anymore. Also I had to rebase, that made harder to see the changes between revisions. Sorry about that. Doing one more try run to see if everything is ok.
Comment on attachment 8896445 [details]
Bug 1355721 - stylo: Implement font feature values lookup

https://reviewboard.mozilla.org/r/167692/#review176526

::: gfx/thebes/gfxFontFeatures.h
(Diff revision 7)
> -private:
> -    // Private destructor, to discourage deletion outside of Release():
> -    ~gfxFontFeatureValueSet() {}

Why do you make the structs and fields public?

::: layout/base/nsPresContext.h:1246
(Diff revision 7)
> +  // whether font feature values lookup object needs initialization
> +  RefPtr<gfxFontFeatureValueSet> mFontFeatureValuesLookup;

I don't think this should be public. And this should be put to some place with other fields, not some random position in the class.

Also it is the lookup set itself, not "whether ...". A "whether ..." comment usually indicates a bool variable, doesn't it?

::: layout/base/nsPresContext.cpp:3134
(Diff revision 7)
> +    nsTArray<nsCSSFontFeatureValuesRule*> rules;
> +    styleSet->AsGecko()->AppendFontFeatureValuesRules(rules);
> +
> +    if (rules.Length() != 0) {
> +      mFontFeatureValuesLookup = new gfxFontFeatureValueSet();
> +      for (nsCSSFontFeatureValuesRule* rule : rules) {
> +        const nsTArray<FontFamilyName>& familyList = rule->GetFamilyList().GetFontlist();
> +        const nsTArray<gfxFontFeatureValueSet::FeatureValues>&
> +          featureValues = rule->GetFeatureValues();
> +
> +        for (const FontFamilyName& f : familyList) {
> +          mFontFeatureValuesLookup->AddFontFeatureValues(f.mName,
> +                                                         featureValues);
> +        }
> +      }
> +    } else if (mFontFeatureValuesLookup != nullptr) {
> +      mFontFeatureValuesLookup = nullptr;
> +    }

I think you can move this whole logic into `nsStyleSet::GetFontFeatureValueSet` so that StyleSetHandle exposes a unified API for Gecko and Stylo.

::: servo/components/style/stylesheets/font_feature_values_rule.rs:327
(Diff revision 7)
> +                                    let entry = Gecko_CreateFeatureValueHashEntry(
> +                                        dest,
> +                                        family.name.to_ascii_lowercase().as_ptr(),
> +                                        structs::$gecko_enum,
> +                                        val.name.as_ptr()
> +                                    );
> +                                    val.value.set_to_gecko(&mut (*entry).mValues);

Instead of exposing all the details of gfxFontFeatureSet and doing this, can we just construct an `nsTArray<gfxFontFeatureValueSet::FeatureValues>` and pass it to `gfxFontFeatureValueSet::AddFontFeatureValues`?

::: servo/ports/geckolib/glue.rs:3510
(Diff revision 7)
> +    let len: u32 = data.extra_style_data
> +     .iter_origins()
> +     .map(|(d, _)| d.font_feature_values.len() as u32)
> +     .sum();
> +
> +    // We should return false if there is no font feautures value.
> +    if len == 0 {
> +      return false;
> +    }

You don't really need the length.

Something like `let has_rule = xxx.iter_origins().any(|(d, _)| d.font_feature_values.len() != 0)` should be enough. Probably not a big deal, though.

::: servo/ports/geckolib/glue.rs:3522
(Diff revision 7)
> +      return false;
> +    }
> +
> +    let font_feature_values_iter = data.extra_style_data
> +        .iter_origins_rev()
> +        .flat_map(|(d, o)| d.font_feature_values.iter().zip(iter::repeat(o)));

It doesn't seem to me that we need the origin in the loop below. I think it should be fine to just return the iterator of `font_feature_values`.
Attachment #8896445 - Flags: review?(xidorn+moz)
Comment on attachment 8896445 [details]
Bug 1355721 - stylo: Implement font feature values lookup

https://reviewboard.mozilla.org/r/167692/#review176532

::: gfx/thebes/gfxFontFeatures.h
(Diff revision 7)
> -private:
> -    // Private destructor, to discourage deletion outside of Release():
> -    ~gfxFontFeatureValueSet() {}

To be able to create them inside `ServoBindings.cpp`, they need to be public.

::: layout/base/nsPresContext.h:1246
(Diff revision 7)
> +  // whether font feature values lookup object needs initialization
> +  RefPtr<gfxFontFeatureValueSet> mFontFeatureValuesLookup;

Yes, this comment was on its previous place and I just coppied both. But it looks like this is wrong.

Making it protected.

::: servo/components/style/stylesheets/font_feature_values_rule.rs:327
(Diff revision 7)
> +                                    let entry = Gecko_CreateFeatureValueHashEntry(
> +                                        dest,
> +                                        family.name.to_ascii_lowercase().as_ptr(),
> +                                        structs::$gecko_enum,
> +                                        val.name.as_ptr()
> +                                    );
> +                                    val.value.set_to_gecko(&mut (*entry).mValues);

I thought we talked about this yesterday and decided to create this on servo side to get rid of additional intermediate `FeatureValues`s? Using AddFontFeatureValues will cause looping this values twice. Also directly passing values to  `gfxFontFeatureValueSet`, reduced and simplified the bindings.
Comment on attachment 8896445 [details]
Bug 1355721 - stylo: Implement font feature values lookup

https://reviewboard.mozilla.org/r/167692/#review176532

> I thought we talked about this yesterday and decided to create this on servo side to get rid of additional intermediate `FeatureValues`s? Using AddFontFeatureValues will cause looping this values twice. Also directly passing values to  `gfxFontFeatureValueSet`, reduced and simplified the bindings.

What I was thinking is to avoid iterating rules multiple times (which was how this was done in the previous approach), not the values. I think it is better to respect existing public API of `gfxFontFeatureValueSet` as far as we cannot operate the hashtable directly anyway. Exposing the internal doesn't seem like a good idea. If I didn't make it clear yesterday, sorry about that.
Ok, I think this is the best solution. It doesn't expose the private members and also minimizes intermediate objects. We don't even need to create an intermediate nsTArray. I tried various solutions bot none of them was better. WDYT?
Flags: needinfo?(cam)
Comment on attachment 8896445 [details]
Bug 1355721 - stylo: Implement font feature values lookup

https://reviewboard.mozilla.org/r/167692/#review177112

Haven't finished reviewing the Servo part, but I don't have much time at the moment, so post the comment for reviewed part for now...

::: gfx/thebes/gfxFontFeatures.h:81
(Diff revision 8)
>      void
>      AddFontFeatureValues(const nsAString& aFamily,
>                  const nsTArray<gfxFontFeatureValueSet::FeatureValues>& aValues);
>  
> +    nsTArray<uint32_t>*
> +    AppendFeatureValueHashEntry(nsAutoString family,

Please use `const nsAString&` here. It should be able to accept all kinds of non-"C"-prefixed strings.

You can read https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Internal_strings for more details about string types in Gecko.

::: gfx/thebes/gfxFontFeatures.h:81
(Diff revision 8)
> +    AppendFeatureValueHashEntry(nsAutoString family,
> +                                 nsString name,
> +                                 uint32_t alternate);

Parameter names need to be prefixed by `a`, e.g. `aFamily`, `aName`, `aAlternate`.

::: gfx/thebes/gfxFontFeatures.h:82
(Diff revision 8)
>      AddFontFeatureValues(const nsAString& aFamily,
>                  const nsTArray<gfxFontFeatureValueSet::FeatureValues>& aValues);
>  
> +    nsTArray<uint32_t>*
> +    AppendFeatureValueHashEntry(nsAutoString family,
> +                                 nsString name,

`const nsAString&` here as well.

::: layout/base/nsPresContext.h:1246
(Diff revision 8)
>    void UpdateCharSet(NotNull<const Encoding*> aCharSet);
>  
>    static bool NotifyDidPaintSubdocumentCallback(nsIDocument* aDocument, void* aData);
>  
>  public:
> +

Seems to be an unrelated change.

::: layout/base/nsPresContext.cpp:3127
(Diff revision 8)
> +void
> +nsPresContext::EnsureFontFeatureValuesLookup()
> +{
> +  StyleSetHandle styleSet = mShell->StyleSet();
> +  mFontFeatureValuesLookup = styleSet->GetFontFeatureValueSet();
> +}

I guess you can just inline this function into `FlushFontFeatureValues` directly.

::: layout/base/nsPresContext.cpp:3150
(Diff revision 8)
> +  if (!mShell) {
> +    return; // we've been torn down
> +  }

Either this or the `if (mShell)` condition below is useless.

::: layout/style/ServoBindings.h:305
(Diff revision 8)
>  // font_id is LookAndFeel::FontID
>  void Gecko_nsFont_InitSystem(nsFont* dst, int32_t font_id,
>                               const nsStyleFont* font, RawGeckoPresContextBorrowed pres_context);
>  void Gecko_nsFont_Destroy(nsFont* dst);
>  
> +nsTArrayBorrowed_uint32_t Gecko_AppendFeatureValueHashEntry(gfxFontFeatureValueSet* value_set,

Seems to have exceeded 80 chars... but this file doesn't really follow that style strictly...

Anyway, either move the parameter to the next line, or make the second line align to the first parameter (after the paren).

::: layout/style/ServoBindings.h:306
(Diff revision 8)
>  void Gecko_nsFont_InitSystem(nsFont* dst, int32_t font_id,
>                               const nsStyleFont* font, RawGeckoPresContextBorrowed pres_context);
>  void Gecko_nsFont_Destroy(nsFont* dst);
>  
> +nsTArrayBorrowed_uint32_t Gecko_AppendFeatureValueHashEntry(gfxFontFeatureValueSet* value_set,
> +  nsIAtom* atom, uint32_t alternate, nsIAtom* name);

`atom` is a weird name. This is for font family, right? Probably better calling it `family` to make things clear.

::: layout/style/ServoBindings.cpp:1339
(Diff revision 8)
> +Gecko_AppendFeatureValueHashEntry(gfxFontFeatureValueSet* aFontFeatureValues,
> +                                  nsIAtom* aAtom, uint32_t aAlternate, nsIAtom* aName)
> +{
> +  nsString string = nsDependentAtomString(aAtom);
> +  nsAutoString family(string);
> +  return aFontFeatureValues->AppendFeatureValueHashEntry(family,

Once you make the parameter use `const nsAString&` type, you should be able to use `nsDependentAtomString` for family as well.

::: servo/components/style/properties/gecko.mako.rs:2563
(Diff revision 8)
> +        if let Some(pres_context) = v.pres_context {
> +            unsafe {
> +                Gecko_nsFont_SetFontFeatureValuesLookup(&mut self.gecko.mFont,
> +                    pres_context);
> +            }
> +        }

I... don't think storing a pointer to pres context is the right thing to do.

You can do change like https://github.com/servo/servo/commit/08c12062c7e94096c8d31703afb86309c5abc6da#diff-3c176cae60dd5fe7b6f7c611905937a4 to have a `&Device` passed in for this property, so that you can use the pres context.

::: servo/components/style/properties/longhand/font.mako.rs:1352
(Diff revision 8)
> -    <%self:simple_system_boilerplate name="font_variant_alternates"></%self:simple_system_boilerplate>
> +    <%
> +    if product != "gecko":
> +        simple_system_boilerplate("font_variant_alternates")
> +    %>
> +
> +    #[cfg(feature = "gecko")]
> +    impl SpecifiedValue {
> +        pub fn system_font(f: SystemFont) -> Self {
> +            SpecifiedValue::System(f)
> +        }
> +        pub fn get_system(&self) -> Option<SystemFont> {
> +            if let SpecifiedValue::System(s) = *self {
> +                Some(s)
> +            } else {
> +                None
> +            }
> +        }
> +    }
> +
> +    #[cfg(feature = "gecko")]
> +    impl ToComputedValue for SpecifiedValue {
> +        type ComputedValue = computed_value::T;
> +
> +        fn to_computed_value(&self, _context: &Context) -> computed_value::T {
> +            let value = match *self {
> +                SpecifiedValue::Value(ref v) => v.clone(),
> +                SpecifiedValue::System(_) => {
> +                    <%self:nongecko_unreachable>
> +                        _context.cached_system_font.as_ref()
> +                            .unwrap().font_variant_alternates.value.clone()
> +                    </%self:nongecko_unreachable>
> +                }
> +            };
> +
> +            computed_value::T {
> +                value: value,
> +                pres_context: Some(_context.device().pres_context())
> +            }
> +        }
> +
> +        fn from_computed_value(other: &computed_value::T) -> Self {
> +            SpecifiedValue::Value(other.value.clone())
> +        }
> +    }

I believe you can remove most, if not all, of changes here if you remove `pres_context` from the computed value.

::: servo/components/style/stylesheets/font_feature_values_rule.rs:54
(Diff revision 8)
>  
> +/// A trait for @font-feature-values rule to gecko values conversion.
> +#[cfg(feature = "gecko")]
> +pub trait DeclToGeckoValues {
> +    /// Sets the equivalent of declaration to gecko `nsTArray<c_uint>` array.
> +    fn set_to_gecko(&self, array: &mut nsTArray<c_uint>);

Shouldn't this be `nsTArray<u32>`?
Attachment #8896445 - Flags: review?(xidorn+moz)
Comment on attachment 8896445 [details]
Bug 1355721 - stylo: Implement font feature values lookup

https://reviewboard.mozilla.org/r/167692/#review177112

> I... don't think storing a pointer to pres context is the right thing to do.
> 
> You can do change like https://github.com/servo/servo/commit/08c12062c7e94096c8d31703afb86309c5abc6da#diff-3c176cae60dd5fe7b6f7c611905937a4 to have a `&Device` passed in for this property, so that you can use the pres context.

That code was like this prevously but Emilio didn't like it and asked to store a pointer to pres context instead. We all should speak if we want to change this back..

> I believe you can remove most, if not all, of changes here if you remove `pres_context` from the computed value.

Same. We need this change if we want to keep a pointer to pres context.
Comment on attachment 8896445 [details]
Bug 1355721 - stylo: Implement font feature values lookup

https://reviewboard.mozilla.org/r/167692/#review177144

::: layout/style/ServoStyleSet.cpp:1354
(Diff revision 8)
> +  RefPtr<gfxFontFeatureValueSet> set = new gfxFontFeatureValueSet();
> +  bool result = Servo_StyleSet_GetFontFeatureValueSet(mRawSet.get(), set.get());

I kinda prefer that we move the construction of `gfxFontFeatureValueSet` into Servo side (with a binding funtion back to Gecko to new it, though), so that we can avoid allocating it when we don't have any `@font-feature-values` rule (which is the common case).

But that could be an optimization in a separate bug if you prefer.
Comment on attachment 8896445 [details]
Bug 1355721 - stylo: Implement font feature values lookup

https://reviewboard.mozilla.org/r/167692/#review177144

> I kinda prefer that we move the construction of `gfxFontFeatureValueSet` into Servo side (with a binding funtion back to Gecko to new it, though), so that we can avoid allocating it when we don't have any `@font-feature-values` rule (which is the common case).
> 
> But that could be an optimization in a separate bug if you prefer.

Yes, I would also prefer doing this in a separate bug.
Comment on attachment 8896445 [details]
Bug 1355721 - stylo: Implement font feature values lookup

https://reviewboard.mozilla.org/r/167692/#review177152

::: gfx/thebes/gfxFontFeatures.cpp:55
(Diff revision 9)
>          const FeatureValues& fv = aValues.ElementAt(i);
>          uint32_t alternate = fv.alternate;
>          uint32_t j, numValues = fv.valuelist.Length();
>          for (j = 0; j < numValues; j++) {
>              const ValueList& v = fv.valuelist.ElementAt(j);
> -            nsAutoString name(v.name);
> +            auto array = AppendFeatureValueHashEntry(family, v.name, alternate);

Probably at least write `auto*` here...

::: layout/style/nsStyleSet.cpp:2303
(Diff revision 9)
> +  } else {
> +    return nullptr;
> +  }

This `else` block isn't needed. The previous `if` block returns, so just a `return nullptr;` at the end would be enough.

Alternatively, you can do something like
```c++
if (rules.IsEmpty()) {
  return nullptr;
}
// the rest
```

::: servo/components/style/stylesheets/font_feature_values_rule.rs:82
(Diff revision 9)
>  }
>  
> +impl DeclToGeckoValues for SingleValue {
> +    #[cfg(feature = "gecko")]
> +    fn set_to_gecko(&self, array: &mut nsTArray<u32>) {
> +        unsafe { array.set_len(1); }

You can use `set_len_pod` here. Probably not a big deal, though.

::: servo/components/style/stylesheets/font_feature_values_rule.rs:125
(Diff revision 9)
> +impl DeclToGeckoValues for PairValues {
> +    #[cfg(feature = "gecko")]
> +    fn set_to_gecko(&self, array: &mut nsTArray<u32>) {
> +        let len = if self.1.is_some() { 2 } else { 1 };
> +
> +        unsafe { array.set_len(len); }

Same here.

::: servo/components/style/stylesheets/font_feature_values_rule.rs:178
(Diff revision 9)
>  }
>  
> +impl DeclToGeckoValues for VectorValues {
> +    #[cfg(feature = "gecko")]
> +    fn set_to_gecko(&self, array: &mut nsTArray<u32>) {
> +        unsafe { array.set_len(self.0.len() as u32); }

and here.

::: servo/components/style/stylesheets/font_feature_values_rule.rs:179
(Diff revision 9)
> +        for (i, value) in self.0.iter().enumerate() {
> +            array[i] = *value as u32;
> +        }

`for (dest, value) in array.iter_mut().zip(self.0.iter())` should be better.

::: servo/components/style/stylesheets/font_feature_values_rule.rs:322
(Diff revision 9)
> +                        if self.$ident.len() > 0 {
> +                            unsafe {
> +                                for val in self.$ident.iter() {
> +                                    let mut array = Gecko_AppendFeatureValueHashEntry(
> +                                        dest,
> +                                        family.name.to_ascii_lowercase().as_ptr(),
> +                                        structs::$gecko_enum,
> +                                        val.name.as_ptr()
> +                                    );
> +                                    val.value.set_to_gecko(&mut array);
> +                                }
> +                            }
> +                        }

This code is a lot deeper than necessary...

Could you do
```rust
if self.$ident.is_empty() {
  continue;
}
```
to deindent one level?

And only wrap the binding call with `unsafe`?

::: servo/components/style/stylesheets/font_feature_values_rule.rs:327
(Diff revision 9)
> +                        if self.$ident.len() > 0 {
> +                            unsafe {
> +                                for val in self.$ident.iter() {
> +                                    let mut array = Gecko_AppendFeatureValueHashEntry(
> +                                        dest,
> +                                        family.name.to_ascii_lowercase().as_ptr(),

It seems to me it is better to do the conversion at the beginning of the `for` loop (instead of inside the inner `$()*` loop).
Attachment #8896445 - Flags: review?(xidorn+moz)
Comment on attachment 8896445 [details]
Bug 1355721 - stylo: Implement font feature values lookup

https://reviewboard.mozilla.org/r/167692/#review177160

::: servo/components/style/properties/gecko.mako.rs:2564
(Diff revision 10)
> +        unsafe {
> +            Gecko_nsFont_SetFontFeatureValuesLookup(&mut self.gecko.mFont, device.pres_context());
> +        }

It seems to me that it is not always necessary to set it.
Comment on attachment 8896445 [details]
Bug 1355721 - stylo: Implement font feature values lookup

https://reviewboard.mozilla.org/r/167692/#review177166

r=me with the comment addressed.

::: servo/components/style/stylesheets/font_feature_values_rule.rs:321
(Diff revision 11)
> +
> +            /// Convert to Gecko gfxFontFeatureValueSet.
> +            #[cfg(feature = "gecko")]
> +            pub fn set_at_rules(&self, dest: *mut gfxFontFeatureValueSet) {
> +                for ref family in self.family_names.iter() {
> +                    let family = family.name.to_ascii_lowercase().as_ptr();

Hmm, I believe this is unsafe. The pointer returned can become dangling.

`to_ascii_lowercase()` may create a new Atom, and nothing holds it after this statement, while we still have a pointer to it, so it is possible that the new Atom is released before we use it.

It should be better to do something like:
```rust
let family = family.name.to_ascii_lowercase();
// ...
family.as_ptr()
```
Attachment #8896445 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8896445 [details]
Bug 1355721 - stylo: Implement font feature values lookup

https://reviewboard.mozilla.org/r/167692/#review177368

::: gfx/thebes/gfxFontFeatures.h:81
(Diff revision 11)
>      void
>      AddFontFeatureValues(const nsAString& aFamily,
>                  const nsTArray<gfxFontFeatureValueSet::FeatureValues>& aValues);
>  
> +    nsTArray<uint32_t>*
> +    AppendFeatureValueHashEntry(const nsAString& aFamily,

This needs docs, what does this return? Can it be null?

::: gfx/thebes/gfxFontFeatures.cpp:72
(Diff revision 11)
> +    nsAutoString name(aName);
> -            ToLowerCase(name);
> +    ToLowerCase(name);
> -            FeatureValueHashKey key(family, alternate, name);
> +    FeatureValueHashKey key(aFamily, aAlternate, name);
> -            FeatureValueHashEntry *entry = mFontFeatureValues.PutEntry(key);
> +    FeatureValueHashEntry *entry = mFontFeatureValues.PutEntry(key);
> -            entry->mKey = key;
> +    entry->mKey = key;
> -            entry->mValues = v.featureSelectors;
> +    return &entry->mValues;

This can probably return a reference.

::: layout/base/nsPresContext.cpp:3135
(Diff revision 11)
> +  if (!mShell) {
> +    return; // we've been torn down
> +  }
> +
> +  if (mFontFeatureValuesDirty) {
> +    StyleSetHandle styleSet = mShell->StyleSet();

(I wouldn't bother creating a local variable for this, but it's fine).

::: layout/style/ServoBindings.h:112
(Diff revision 11)
>    struct nsTArrayBorrowed_##type_ {                                 \
>      nsTArray<type_>* mArray;                                        \
>      MOZ_IMPLICIT nsTArrayBorrowed_##type_(nsTArray<type_>* aArray)  \
>        : mArray(aArray) {}                                           \
>    }
> +DEFINE_ARRAY_TYPE_FOR(uint32_t);

Let's stop using this. You should be able to use `nsTArray` directly, don't you? Indeed I had a patch to remove the `uintptr_t` hack, but I just forgot to submit it :)

::: layout/style/ServoBindings.cpp:1333
(Diff revision 11)
>  Gecko_nsFont_Destroy(nsFont* aDest)
>  {
>    aDest->~nsFont();
>  }
>  
> +nsTArrayBorrowed_uint32_t

Yeah, please just use `nsTArray<uint32_t>*`.

::: layout/style/ServoStyleSet.h:377
(Diff revision 11)
>    bool AppendFontFaceRules(nsTArray<nsFontFaceRuleContainer>& aArray);
>  
>    nsCSSCounterStyleRule* CounterStyleRuleForName(nsIAtom* aName);
>  
> +  // Get all the currently-active font feature values set.
> +  already_AddRefed<gfxFontFeatureValueSet> GetFontFeatureValueSet();

I wouldn't call this `GetFontFeatureValueSet`. This looks cheap, but then turns out it isn't... What about `BuildFontFeatureValueSet`?

::: layout/style/ServoStyleSet.cpp:1355
(Diff revision 11)
> +already_AddRefed<gfxFontFeatureValueSet>
> +ServoStyleSet::GetFontFeatureValueSet()
> +{
> +  UpdateStylistIfNeeded();
> +  RefPtr<gfxFontFeatureValueSet> set = new gfxFontFeatureValueSet();
> +  bool result = Servo_StyleSet_GetFontFeatureValueSet(mRawSet.get(), set.get());

Please use a more descriptive variable name than `result`.

::: servo/components/style/properties/gecko.mako.rs:2532
(Diff revision 11)
>              Gecko_ClearAlternateValues(&mut self.gecko.mFont, v.len());
>          }
>  
>          if v.0.is_empty() {
>              self.gecko.mFont.variantAlternates = NS_FONT_VARIANT_ALTERNATES_NORMAL as u16;
> +            return;

Why don't you need to reset the font feature values lookup pointer to null here?

Looks to me like it should. Otherwise you could end up with incoherent state when resetting this property from something that have alternates.

::: servo/components/style/stylesheets/font_feature_values_rule.rs:32
(Diff revision 11)
>  /// This struct can take 3 value types.
>  /// - `SingleValue` is to keep just one unsigned integer value.
>  /// - `PairValues` is to keep one or two unsigned integer values.
>  /// - `VectorValues` is to keep a list of unsigned integer values.
>  #[derive(Clone, Debug, PartialEq)]
> -pub struct FFVDeclaration<T> {
> +pub struct FFVDeclaration<T: DeclToGeckoValues> {

if you remove the bound in the struct, you can remove most of them that don't needed it, I think.

::: servo/components/style/stylesheets/font_feature_values_rule.rs:50
(Diff revision 11)
>      }
>  }
>  
> +/// A trait for @font-feature-values rule to gecko values conversion.
> +#[cfg(feature = "gecko")]
> +pub trait DeclToGeckoValues {

Let's rename it to something more specific to font-feature-values.

What about `ToGeckoFontFeatureValues`?

::: servo/components/style/stylesheets/font_feature_values_rule.rs:52
(Diff revision 11)
>  
> +/// A trait for @font-feature-values rule to gecko values conversion.
> +#[cfg(feature = "gecko")]
> +pub trait DeclToGeckoValues {
> +    /// Sets the equivalent of declaration to gecko `nsTArray<u32>` array.
> +    fn set_to_gecko(&self, array: &mut nsTArray<u32>);

And let's call this `to_gecko_font_feature_values`?

::: servo/components/style/stylesheets/font_feature_values_rule.rs:321
(Diff revision 11)
> +
> +            /// Convert to Gecko gfxFontFeatureValueSet.
> +            #[cfg(feature = "gecko")]
> +            pub fn set_at_rules(&self, dest: *mut gfxFontFeatureValueSet) {
> +                for ref family in self.family_names.iter() {
> +                    let family = family.name.to_ascii_lowercase().as_ptr();

This doesn't seem fixed.

::: servo/ports/geckolib/glue.rs:3510
(Diff revision 11)
> +    let data = PerDocumentStyleData::from_ffi(raw_data).borrow();
> +
> +    let global_style_data = &*GLOBAL_STYLE_DATA;
> +    let guard = global_style_data.shared_lock.read();
> +
> +    let has_rule = data.extra_style_data

Why do we need to iterate over them here? Seems to me that you just need something like:

```
let mut any_rule = false;

// ...

for src in font_feature_values_iter {
    any_rule = true;
    // ...
}

return any_rule;

::: servo/ports/geckolib/glue.rs:3512
(Diff revision 11)
> +    let global_style_data = &*GLOBAL_STYLE_DATA;
> +    let guard = global_style_data.shared_lock.read();
> +
> +    let has_rule = data.extra_style_data
> +        .iter_origins()
> +        .any(|(d, _)| d.font_feature_values.len() != 0);

(This should also use `is_empty`, but per the above comment I don't think it's needed).
Attachment #8896445 - Flags: review?(emilio+bugs)
Comment on attachment 8896445 [details]
Bug 1355721 - stylo: Implement font feature values lookup

https://reviewboard.mozilla.org/r/167692/#review177368

> Let's stop using this. You should be able to use `nsTArray` directly, don't you? Indeed I had a patch to remove the `uintptr_t` hack, but I just forgot to submit it :)

I suggested him to do this. Have we required clang 4.0 now? IIRC if we still allow 3.9, we have to use this hack, don't we?
(In reply to Xidorn Quan [:xidorn] UTC+10 (PTO, back Aug 28) from comment #60)
> Comment on attachment 8896445 [details]
> Bug 1355721 - stylo: Implement font feature values lookup
> 
> https://reviewboard.mozilla.org/r/167692/#review177368
> 
> > Let's stop using this. You should be able to use `nsTArray` directly, don't you? Indeed I had a patch to remove the `uintptr_t` hack, but I just forgot to submit it :)
> 
> I suggested him to do this. Have we required clang 4.0 now? IIRC if we still
> allow 3.9, we have to use this hack, don't we?

I thought this was 3.8 only... And I'm not sure if any of the bindgen changes would've fixed this.

Also clang 4.0 has been released long ago (indeed, clang 5.0 is about to be released).

I'd really prefer if we dropped this hack.
As far as the hack still exists, we probably shouldn't risk dropping it in this bug. You can probably open another bug for dropping the hack and see if that works. I guess that would be fine.
(In reply to Xidorn Quan [:xidorn] UTC+10 (PTO, back Aug 28) from comment #62)
> As far as the hack still exists, we probably shouldn't risk dropping it in
> this bug. You can probably open another bug for dropping the hack and see if
> that works. I guess that would be fine.

I guess that's fair. Ignore that comment then Nazim :)
Comment on attachment 8896445 [details]
Bug 1355721 - stylo: Implement font feature values lookup

https://reviewboard.mozilla.org/r/167692/#review177368

> This can probably return a reference.

Hm, there is no conversion from reference to `nsTArrayBorrowed_uint32_t`. So I think it's better to stick with pointer.

> This doesn't seem fixed.

Yeah, I fixed it on my local but didn't publish the last review comment since I was going to remove servo side anyway. But since there is more reviews I can push all of them.
Comment on attachment 8896445 [details]
Bug 1355721 - stylo: Implement font feature values lookup

https://reviewboard.mozilla.org/r/167692/#review177558

::: layout/style/ServoStyleSet.cpp:1355
(Diff revision 12)
> +already_AddRefed<gfxFontFeatureValueSet>
> +ServoStyleSet::BuildFontFeatureValueSet()
> +{
> +  UpdateStylistIfNeeded();
> +  RefPtr<gfxFontFeatureValueSet> set = new gfxFontFeatureValueSet();
> +  bool setFilled = Servo_StyleSet_GetFontFeatureValueSet(mRawSet.get(), set.get());

What about `setHasAnyRules`?

Also, I'd really change the name of the function for the same reason... `Servo_StyleSet_BuildFontFeatureValueSet`?

::: servo/components/style/stylesheets/font_feature_values_rule.rs:349
(Diff revision 12)
>              }
>          }
>  
>          /// Updates with new value if same `ident` exists, otherwise pushes to the vector.
> -        fn update_or_push<T>(vec: &mut Vec<FFVDeclaration<T>>, element: FFVDeclaration<T>) {
> +        fn update_or_push<T>(vec: &mut Vec<FFVDeclaration<T>>,
> +                             element: FFVDeclaration<T>) {

This change seems unneeded?

::: servo/components/style/stylist.rs:425
(Diff revision 12)
>                  }
>                  #[cfg(feature = "gecko")]
> +                CssRule::FontFeatureValues(ref rule) => {
> +                    _extra_data
> +                        .borrow_mut_for_origin(&origin)
> +                        .add_font_feature_values(&rule);

nit: no need for `&rule`, you're already matching by `ref`. While at it you can also remove the ones above and below.

::: servo/ports/geckolib/glue.rs:3503
(Diff revision 12)
>          rule.read_with(&guard).get()
>      }).unwrap_or(ptr::null_mut())
>  }
>  
>  #[no_mangle]
> +pub extern "C" fn Servo_StyleSet_GetFontFeatureValueSet(raw_data: RawServoStyleSetBorrowed,

nit: I'd stop using visual indentation, since it's deprectated, and it looks specially bad with such a long argument type names.

```
pub extern "C" fn Servo_StyleSet_GetFontFeatureValueSet(
    raw_data: RawServoStyleSetBorrowed,
    set: *mut gfxFontFeatureValueSet
) -> bool {
```
Attachment #8896445 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8896445 [details]
Bug 1355721 - stylo: Implement font feature values lookup

https://reviewboard.mozilla.org/r/167692/#review177558

> nit: no need for `&rule`, you're already matching by `ref`. While at it you can also remove the ones above and below.

The other branches were like this so I did this for consistency. Removing from others too then.

Btw, I think we can add a tidy rule for that. It seems pretty common misuse.
Attachment #8896444 - Attachment is obsolete: true
Oh, we had to add the new reset binding to the list inside analyzeHeapWrites.js as well.
Pushed by canaltinova@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2791eac8d760
stylo: Implement font feature values lookup r=emilio,xidorn
https://hg.mozilla.org/integration/autoland/rev/b749fc196a50
Change nsCSSFontFeatureValuesRule.h to follow style guideline r=emilio
Backout by gszorc@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e28de8d5a107
Backed out changesets b749fc196a50, 2791eac8d760 for build bustage
This is backed out because of windows build problem. Fixed it and opening review request again. Also included back the servo changes but will remove again after a review. You can see the latest try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=deb6539136cc5321fcf2deeafd7e4053c3e6d9da
It's a bit hard to see the diff since rebase and servo side changes. This is the changes: https://hg.mozilla.org/try/rev/d669d5b82f1933ad3570f1b563a0c7fffe4e59bd
Pushed by canaltinova@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/02083faed1c7
stylo: Implement font feature values lookup r=emilio,xidorn
https://hg.mozilla.org/integration/autoland/rev/98a629415892
Change nsCSSFontFeatureValuesRule.h to follow style guideline r=emilio
Pushed by canaltinova@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f570921adc79
stylo: Update test expectations for font feature values lookup r=me
Pushed by canaltinova@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/599e60ddca1c
stylo: Adjust non-ascii chars in ini file to properly match r=me
Pushed by canaltinova@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5a041e9d9f1e
stylo: Mark osx only test failure as pass r=me
Depends on: 1394233
Depends on: 1424878
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: