+++ This bug was initially created as a clone of Bug #1449068 +++ These two rules currently store data in C++ side rather than Rust side, this is a problem because we need to maintain an extra descriptor-to-string table in both sides. (Note: descriptor lists cannot be merged anyway, but the map with string can.) Also currently we cannot strip the serialization code for specified value because of these rules. Once we change them, we should be able to remove majority of serialization code in nsCSSValue, which is quite a bit.

Per discussion with bholly on IRC, it is more helpful to port font-face rule first as it would help producing useful try run result than porting counter-style for now, so I'm going to start working on this. (Initially I wanted to port counter-style first because that is relative simple, only CounterStyleManager uses it, so I can take that bug to add some infrastructure code of the porting. But it's probably fine to work on this directly as well, although that may mean it would become a larger patch.)

Comment on attachment 8964488 [details] Bug 1449087 part 2 - Use Servo data to back @font-face rule. https://reviewboard.mozilla.org/r/233224/#review238716 Generally looks good. I need to take a bit more careful look because today I'm travelling, so I'll finish reviewing in the evening, but I'd rather drop these comments now. ::: commit-message-7a090:23 (Diff revision 3) > + > +There are some unfortunate bits from this change: > +* We lose style sheet for logging in FontFaceSet. This is probably not > + all that worse, because we wouldn't have that before either if the > + page doesn't use CSSOM to visit it. But we should figure out some > + approach to fix it anyway. File? ::: layout/style/FontFace.h:185 (Diff revision 3) > - */ > - bool ParseDescriptor(nsCSSFontDesc aDescID, const nsAString& aString, > - nsCSSValue& aResult); > - > // Helper function for the descriptor setter methods. > - void SetDescriptor(nsCSSFontDesc aFontDesc, > + bool SetDescriptor(nsCSSFontDesc aFontDesc, Document the return value? ::: layout/style/FontFace.h:268 (Diff revision 3) > // The values corresponding to the font face descriptors, if we are not > // a rule backed FontFace object. For rule backed objects, we use > // the descriptors stored in mRule. > - nsAutoPtr<mozilla::CSSFontFaceDescriptors> mDescriptors; > + // FIXME This should hold a unique ptr to just the descriptors inside, > + // so that we don't need to create a rule for it and don't need to > + // assign a fake line number and column number. Can you file a followup for this and reference it here? ::: layout/style/FontFace.cpp:195 (Diff revision 3) > > void > FontFace::InitializeSource(const StringOrArrayBufferOrArrayBufferView& aSource) > { > if (aSource.IsString()) { > - if (!ParseDescriptor(eCSSFontDesc_Src, > + ErrorResult rv; Ditto re. ErrorResult and this throwing. ::: layout/style/FontFace.cpp:574 (Diff revision 3) > const FontFaceDescriptors& aDescriptors) > { > MOZ_ASSERT(!HasRule()); > MOZ_ASSERT(!mDescriptors); > > - mDescriptors = new CSSFontFaceDescriptors; > + ErrorResult rv; Should this use IgnoredErrorResult? What happens if one of the setters actually throws, doesn't it assert, given the exception would remain there? Alternatively we could do `Reject(rv.StealNSResult())`, maybe asserting that it's a syntax error before. Also, I think we now have IgnoreErrors() so you can call this like: ``` if (!SetDescriptor(desc, aFamily, IgnoreErrors()) || // ... ``` ::: layout/style/FontFaceSet.cpp:1085 (Diff revision 3) > face->mReferrerPolicy = mozilla::net::RP_Unset; > } else { > aFontFace->GetDesc(eCSSFontDesc_Src, val); > unit = val.GetUnit(); > if (unit == eCSSUnit_Array) { > - nsCSSValue::Array* srcArr = val.GetArrayValue(); > + RefPtr<nsCSSValue::Array> srcArr = val.GetArrayValue(); This change doesn't seem related? ::: layout/style/ServoFontFaceRule.h:42 (Diff revision 3) > + friend class ServoFontFaceRule; > + > + explicit ServoFontFaceRuleDecl(already_AddRefed<RawServoFontFaceRule> aDecl) > + : mRawRule(Move(aDecl)) {} > + > + ~ServoFontFaceRuleDecl() {} nit: `= default`? ::: layout/style/ServoFontFaceRule.h:51 (Diff revision 3) > + > + RefPtr<RawServoFontFaceRule> mRawRule; > + > +private: > + // NOT TO BE IMPLEMENTED > + void* operator new(size_t size) CPP_THROW_NEW; nit: `= delete`? ::: layout/style/ServoFontFaceRule.h:89 (Diff revision 3) > +#ifdef DEBUG > + void List(FILE* out = stdout, int32_t aIndent = 0) const final; > +#endif > + > +private: > + virtual ~ServoFontFaceRule() {} nit: `= default`? ::: layout/style/ServoFontFaceRule.h:91 (Diff revision 3) > +#endif > + > +private: > + virtual ~ServoFontFaceRule() {} > + > + // For computing the offset of mDecls. nit: `mDecl`; ::: servo/components/style/stylesheets/font_face_rule.rs:12 (Diff revision 3) > -#[cfg(feature = "gecko")] > -pub use gecko::rules::FontFaceRule; > > impl FontFaceRule { > - #[cfg(feature = "servo")] > pub fn clone_conditionally_gecko_or_servo(&self) -> FontFaceRule { this can go away, right? ::: servo/ports/geckolib/glue.rs:2198 (Diff revision 3) > +pub extern "C" fn Servo_FontFaceRule_CreateEmpty() -> RawServoFontFaceRuleStrong { > + let global_style_data = &*GLOBAL_STYLE_DATA; > + // XXX This is not great. We should split FontFace descriptor data > + // from the rule, so that we don't need to create the rule like this > + // and the descriptor data itself can be hold in UniquePtr from the > + // Gecko side. Again, reference the followup from here.

Comment on attachment 8964488 [details] Bug 1449087 part 2 - Use Servo data to back @font-face rule. https://reviewboard.mozilla.org/r/233224/#review238716 > File? Filed bug 1450903. > Can you file a followup for this and reference it here? Filed bug 1450904. > This change doesn't seem related? This is important because val's content is going away in the loop, so the Array can become UAF. This wasn't a problem because GetDesc currently returns value from nsCSSValue hold elsewhere, so the array always has a reference in its original holder. This change makes the array generated dynamically, so it would get released once val is changed. I'll add some comment here.

There are quite a bit of serialization failures... and some of them needs fixing from cssparser :/

Comment on attachment 8964536 [details] Implement the correct ToCss for UrlSource. https://reviewboard.mozilla.org/r/233262/#review238820 ::: servo/components/style/font_face.rs:58 (Diff revision 1) > pub struct UrlSource { > /// The specified url. > pub url: SpecifiedUrl, > /// The format hints specified with the `format()` function. > - #[css(skip)] > pub format_hints: Vec<String>, I _think_ `#[css(function = "format", comma, string)]` should generate the code you want, maybe you need to wrap them in a struct to `derive` it, but not a big deal if it's non-trivial.

Comment on attachment 8964535 [details] Bug 1449087 part 1 - Upgrade cssparser to 0.23.4 for serialization fix. https://reviewboard.mozilla.org/r/233260/#review238822

Comment on attachment 8964488 [details] Bug 1449087 part 2 - Use Servo data to back @font-face rule. https://reviewboard.mozilla.org/r/233224/#review238824 Thanks for doing this! ::: layout/style/FontFace.cpp:615 (Diff revision 5) > > void > -FontFace::GetDesc(nsCSSFontDesc aDescID, > +FontFace::GetDesc(nsCSSFontDesc aDescID, nsString& aResult) const > - nsCSSPropertyID aPropID, > - nsString& aResult) const > { aResult.Truncate(), or assert it's empty otherwise? ::: layout/style/FontFaceSet.cpp:1086 (Diff revision 5) > } else { > aFontFace->GetDesc(eCSSFontDesc_Src, val); > unit = val.GetUnit(); > if (unit == eCSSUnit_Array) { > - nsCSSValue::Array* srcArr = val.GetArrayValue(); > + // Hold a strong reference because content of val is going away > + // in the loop below. Oh, I see, thank you for the comment :) ::: layout/style/ServoFontFaceRule.cpp:39 (Diff revision 5) > +NS_IMPL_RELEASE_USING_AGGREGATOR(ServoFontFaceRuleDecl, ContainingRule()) > + > +// helper for string GetPropertyValue and RemovePropertyValue > +void > +ServoFontFaceRuleDecl::GetPropertyValue(nsCSSFontDesc aFontDescID, > + nsAString & aResult) const nit: `nsAString&` (whitespace) ::: layout/style/ServoFontFaceRule.cpp:47 (Diff revision 5) > + Servo_FontFaceRule_GetDescriptorCssText(mRawRule, aFontDescID, &aResult); > +} > + > +void > +ServoFontFaceRuleDecl::GetCssText(nsAString& aCssText) > +{ Assert it's empty, or truncate? ::: layout/style/ServoFontFaceRule.cpp:62 (Diff revision 5) > +} > + > +NS_IMETHODIMP > +ServoFontFaceRuleDecl::GetPropertyValue(const nsAString& aPropName, > + nsAString& aResult) > +{ Assert it's empty / truncate. ::: layout/style/ServoFontFaceRule.cpp:86 (Diff revision 5) > + NS_ASSERTION(descID >= eCSSFontDesc_UNKNOWN && > + descID < eCSSFontDesc_COUNT, > + "LookupFontDesc returned value out of range"); > + > + if (descID == eCSSFontDesc_UNKNOWN) { > + aResult.Truncate(); Same. ::: layout/style/ServoFontFaceRule.cpp:225 (Diff revision 5) > + return CSSRuleBinding::FONT_FACE_RULE; > +} > + > +void > +ServoFontFaceRule::GetCssText(nsAString& aCssText) const > +{ Assert / Truncate.

Comment on attachment 8964536 [details] Implement the correct ToCss for UrlSource. https://reviewboard.mozilla.org/r/233262/#review238820 > I _think_ `#[css(function = "format", comma, string)]` should generate the code you want, maybe you need to wrap them in a struct to `derive` it, but not a big deal if it's non-trivial. Doesn't seem to work. It reports: > error: proc-macro derive panicked > failed to parse field attributes: Multiple errors: (Unexpected field `function`, Unexpected field `comma`, Unexpected field `string`) I'll just leave it as-is for now.

Pushed by xquan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/233075da24e3 part 1 - Upgrade cssparser to 0.23.4 for serialization fix. r=emilio https://hg.mozilla.org/integration/autoland/rev/8d23659e5494 part 2 - Use Servo data to back @font-face rule. r=emilio

https://hg.mozilla.org/mozilla-central/rev/233075da24e3 https://hg.mozilla.org/mozilla-central/rev/8d23659e5494