Closed Bug 1278647 Opened 4 years ago Closed 4 years ago

stylo: Add Servo bindings for setting font-family

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: mbrubeck, Assigned: mbrubeck)

References

Details

Attachments

(1 file)

No description provided.
Depends on https://github.com/servo/servo/pull/11667

Note: This involves calling from Rust to C++ in a loop (one call per font family name). We can't just create the FontFamilyName array entirely in Rust because it contains nsString fields.  One alternative would be a single FFI call that takes an array of atoms and enums, but this would require at least one new allocation on the Rust side.
Hmm, this builds but set_font_family is not actually getting called when I run Firefox.  Investigating...
(In reply to Matt Brubeck (:mbrubeck) from comment #2)
> Note: This involves calling from Rust to C++ in a loop (one call per font
> family name). We can't just create the FontFamilyName array entirely in Rust
> because it contains nsString fields.  One alternative would be a single FFI
> call that takes an array of atoms and enums, but this would require at least
> one new allocation on the Rust side.

As with the linear-gradient stuff, I think that's OK for now.  Hopefully we'll get a more comprehensive solution to dealing with nsTArrays from the rust side at some point.
Comment on attachment 8760880 [details]
Bug 1278647 [stylo] Add font family bindings for Servo

https://reviewboard.mozilla.org/r/58288/#review55226

r=me, feel free to re-request review if you need changes to fix your comment 3 issues.

::: gfx/thebes/gfxFontFamilyList.h:24
(Diff revision 1)
>   * type of font family name, either a name (e.g. Helvetica) or a
>   * generic (e.g. serif, sans-serif), with the ability to distinguish
>   * between unquoted and quoted names for serializaiton
>   */ 
>  
> -enum FontFamilyType {
> +enum FontFamilyType: uint32_t {

Nit: space before ":".

::: layout/style/ServoBindings.h:31
(Diff revision 1)
>  class nsIPrincipal;
>  class nsIURI;
> -namespace mozilla { namespace dom { class Element; } }
> +struct nsFont;
> +namespace mozilla {
> +  class FontFamilyList;
> +  enum FontFamilyType: uint32_t;

Nit: here too.

::: layout/style/ServoBindings.h:132
(Diff revision 1)
> +void Gecko_FontFamilyList_AppendNamed(FontFamilyList *aList, nsIAtom *aName);
> +void Gecko_FontFamilyList_AppendGeneric(FontFamilyList *list, FontFamilyType familyType);
> +void Gecko_CopyFontFamilyFrom(nsFont *dst, const nsFont *src);

Nit: "\*"s next to types.

::: layout/style/ServoBindings.cpp:291
(Diff revision 1)
>    NS_ConvertUTF8toUTF16 inStr(nsDependentCSubstring(aString, aLength));
>    return nsContentUtils::EqualsIgnoreASCIICase(atomStr, inStr);
>  }
>  
>  void
> +Gecko_FontFamilyList_AppendNamed(FontFamilyList *aList, nsIAtom *aName)

Nit: "\*"s next to types (and below).

::: layout/style/ServoBindings.cpp:293
(Diff revision 1)
> +  FontFamilyName family;
> +  aName->ToString(family.mName);
> +  aList->Append(family);

Might be worth commenting here that Servo doesn't store whether a font family name was quoted or not in its value, so we just assume unquoted for now.  (This might cause incorrect serialization of values in the CSSOM but otherwise shouldn't matter.)
Attachment #8760880 - Flags: review?(cam) → review+
Comment on attachment 8760880 [details]
Bug 1278647 [stylo] Add font family bindings for Servo

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58288/diff/1-2/
Comment on attachment 8760880 [details]
Bug 1278647 [stylo] Add font family bindings for Servo

Addressed review comments and added a FontFamilyList::Clear binding which was necessary to make the stylo code work correctly.
Attachment #8760880 - Flags: review+ → review?(cam)
Comment on attachment 8760880 [details]
Bug 1278647 [stylo] Add font family bindings for Servo

r=me on those changes.
Attachment #8760880 - Flags: review?(cam) → review+
Pushed by mbrubeck@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc5d3ca3be5a
[stylo] Add font family bindings for Servo r=heycam
https://hg.mozilla.org/mozilla-central/rev/cc5d3ca3be5a
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
> +void Gecko_FontFamilyList_Clear(FontFamilyList* aList);

Nit: I think things like this should be named Gecko_ClearFontFamilyList. That is to say, we should use idiomatic Gecko naming modulo the {Gecko,Servo}_ Prefix.
You need to log in before you can comment on or make changes to this bug.