1278647 - stylo: Add Servo bindings for setting font-family

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Comment 1

[Matt Brubeck (:mbrubeck)]

Attachment: Bug 1278647 [stylo] Add font family bindings for Servo

Review commit: https://reviewboard.mozilla.org/r/58288/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58288/

Comment 2

[Matt Brubeck (:mbrubeck)]

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.

Comment 3

[Matt Brubeck (:mbrubeck)]

Hmm, this builds but set_font_family is not actually getting called when I run Firefox.  Investigating...

Comment 4

[Cameron McCormack (:heycam)]

(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 5

[Cameron McCormack (:heycam)]

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.)

Comment 6

[Matt Brubeck (:mbrubeck)]

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 7

[Matt Brubeck (:mbrubeck)]

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.

Comment 8

[Cameron McCormack (:heycam)]

Comment on attachment 8760880 [details]
Bug 1278647 [stylo] Add font family bindings for Servo

r=me on those changes.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc5d3ca3be5a
[stylo] Add font family bindings for Servo r=heycam

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cc5d3ca3be5a

Comment 11

[Bobby Holley (:bholley)]

> +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.