Closed Bug 1408303 Opened 8 years ago Closed 8 years ago

make FontFaceSet and FontFace use Servo for parsing font descriptors and properties

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: heycam, Assigned: boris)

References

Details

Attachments

(4 files, 1 obsolete file)

Bug 1408303 - Part 1: Use the Servo parser for font descriptors Web API.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1408303 - Part 2: Use the Servo parser for FontFaceSet Web API.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1408303 - Part 3: Move several Servo parsers into ServoCSSParser helper class.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Servo PR, #19491
41 bytes, text/x-github-pull-request
Details | Review
FontFaceSet and FontFace currently use nsCSSParser to parse font descriptors and properties. We should use Servo parsing code for this.
Priority: -- → P3
Assignee: nobody → boris.chiou
CSSFontFaceDescriptors [1] still uses nsCSSValue, so maybe we need to use a different type (Servo type?) to store the parsed result in CSSFontFaceDescriptors, and add some FFIs to retrieve the its content. [1] https://searchfox.org/mozilla-central/rev/c633ffa4c4611f202ca11270dcddb7b29edddff8/layout/style/nsCSSFontFaceRule.h#17-30
Status: NEW → ASSIGNED
Comment on attachment 8933591 [details] Bug 1408303 - Part 1: Store mIsServo into FontFace. https://reviewboard.mozilla.org/r/204538/#review210488 ::: layout/style/FontFace.h:284 (Diff revision 2) > + > + // Whether this FontFace is styled by Servo backend. > + bool mIsServo; Storing this is OK, although I think it would also be fine to just do mFontFaceSet->Document()->IsStyledByServo() whenever we need to know. ::: layout/style/FontFace.cpp:160 (Diff revision 2) > - nsCSSFontFaceRule* aRule) > + nsCSSFontFaceRule* aRule, > + bool aIsServo) > { > - RefPtr<FontFace> obj = new FontFace(aGlobal, aFontFaceSet); > + RefPtr<FontFace> obj = new FontFace(aGlobal, aFontFaceSet, aIsServo); Here, let's do aFontFaceSet->Document()->IsStyledByServo() instead of passing an argument in.
Attachment #8933591 - Flags: review?(cam) → review+
Comment on attachment 8933592 [details] Bug 1408303 - Part 1: Use the Servo parser for font descriptors Web API. https://reviewboard.mozilla.org/r/204540/#review210492 Nice macro use. :-) ::: servo/ports/geckolib/glue.rs:4710 (Diff revision 2) > + let mut parser = Parser::new(&mut input); > + let url_data = unsafe { RefPtr::from_ptr_ref(&data) }; > + let context = ParserContext::new( > + Origin::Author, > + url_data, > + Some(CssRuleType::Style), Maybe it makes no difference, but should this be CssRuleType::FontFace?
Attachment #8933592 - Flags: review?(cam) → review+
Comment on attachment 8933593 [details] Bug 1408303 - Part 2: Use the Servo parser for FontFaceSet Web API. https://reviewboard.mozilla.org/r/204542/#review210490 ::: layout/style/FontFaceSet.cpp:213 (Diff revision 2) > { > + if (mDocument->IsStyledByServo()) { > + nsCSSValue style; > + nsCSSValue stretch; > + nsCSSValue weight; > + RefPtr<URLExtraData> url = new URLExtraData(mDocument->GetDocumentURI(), Should this be mDocument->GetDocBaseURI()? ::: servo/ports/geckolib/glue.rs:4783 (Diff revision 2) > + let mut parser = Parser::new(&mut input); > + let url_data = unsafe { RefPtr::from_ptr_ref(&data) }; > + let context = ParserContext::new( > + Origin::Author, > + url_data, > + Some(CssRuleType::Style), And here, maybe.
Attachment #8933593 - Flags: review?(cam) → review+
Comment on attachment 8933594 [details] Bug 1408303 - Part 3: Move several Servo parsers into ServoCSSParser helper class. https://reviewboard.mozilla.org/r/204544/#review210494 ::: dom/canvas/CanvasRenderingContext2D.cpp:2824 (Diff revision 3) > - NS_ConvertUTF16toUTF8 value(aPropertyValue); > - > + ServoCSSParser::ParsingEnvironment env = > + ServoCSSParser::ParsingEnvironment(data, > - RefPtr<RawServoDeclarationBlock> servoDeclarations = > - Servo_ParseProperty(aProperty, > - &value, > - data, > - ParsingMode::Default, > - aDocument->GetCompatibilityMode(), > + aDocument->GetCompatibilityMode(), > - aDocument->CSSLoader()).Consume(); > + aDocument->CSSLoader()); ServoCSSParser::ParsingEnvironment env(data, ... so you don't need to write the type twice. ::: dom/smil/nsSMILCSSValueType.cpp:716 (Diff revision 3) > if (!doc) { > return result; > } > > // Parse property > - // FIXME this is using the wrong base uri (bug 1343919) > + auto env = ServoCSSParser::GetParsingEnvironment(doc); Nit: I would prefer the type name spelled out instead of "auto". (I think we tend to use auto only when the type name is already mentioned on the RHS, like with static_cast, and with more complicated to type nested class names, like with iterator classes.) ::: layout/style/ServoCSSParser.h:28 (Diff revision 3) > + > +using RawGeckoGfxMatrix4x4 = mozilla::gfx::Float[16]; > > namespace mozilla { > > +class ServoStyleSet; Maybe this isn't needed? (Or the #include in the cpp file?) ::: layout/style/ServoCSSParser.h:89 (Diff revision 3) > + nsCSSPropertyID aProperty, > + const nsAString& aValue, > + const ParsingEnvironment& aParsingEnvironment, > + ParsingMode aParsingMode = ParsingMode::Default); > + > + /* Nit: javadoc-style comments have two "*"s on this line. Although I don't actually know if we have tools that do read these javadoc-style comments...
Attachment #8933594 - Flags: review?(cam) → review+
Comment on attachment 8933591 [details] Bug 1408303 - Part 1: Store mIsServo into FontFace. https://reviewboard.mozilla.org/r/204538/#review210488 > Storing this is OK, although I think it would also be fine to just do mFontFaceSet->Document()->IsStyledByServo() whenever we need to know. You are right. Using mFontFaceSet to get the flag may be better, so I will update all of this by mFontFaceSet->Document()->IsStyledByServo()
Comment on attachment 8933593 [details] Bug 1408303 - Part 2: Use the Servo parser for FontFaceSet Web API. https://reviewboard.mozilla.org/r/204542/#review210490 > Should this be mDocument->GetDocBaseURI()? I'm not sure, because we still use mDocument->GetDocumentURI() as the base URI to nsCSSParser. I can update this for Stylo because this may be a bug.
Comment on attachment 8933593 [details] Bug 1408303 - Part 2: Use the Servo parser for FontFaceSet Web API. https://reviewboard.mozilla.org/r/204542/#review210490 > I'm not sure, because we still use mDocument->GetDocumentURI() as the base URI to nsCSSParser. I can update this for Stylo because this may be a bug. OK, then it can be a followup bug. (Maybe just as part of bug 1343919?)
Comment on attachment 8933593 [details] Bug 1408303 - Part 2: Use the Servo parser for FontFaceSet Web API. https://reviewboard.mozilla.org/r/204542/#review210490 > OK, then it can be a followup bug. (Maybe just as part of bug 1343919?) OK, I will add a comment here. Thanks.
Attachment #8933591 - Attachment is obsolete: true
Attached file Servo PR, #19491 —
Pushed by bchiou@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/de59a3003ed5 Part 1: Use the Servo parser for font descriptors Web API. r=heycam https://hg.mozilla.org/integration/autoland/rev/1305f336b9f3 Part 2: Use the Servo parser for FontFaceSet Web API. r=heycam https://hg.mozilla.org/integration/autoland/rev/1b5f770c5674 Part 3: Move several Servo parsers into ServoCSSParser helper class. r=heycam
https://hg.mozilla.org/mozilla-central/rev/de59a3003ed5 https://hg.mozilla.org/mozilla-central/rev/1305f336b9f3 https://hg.mozilla.org/mozilla-central/rev/1b5f770c5674
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: