Manish Goregaokar [:manishearth] Assignee
	Description • 7 years ago

Gecko populates the nsFont with GetDefaultFont each time the generic font changes[1]. We should do the same, at least for the font family list (We handle the font size separately anyway).

Observably, this will make "font-family: -moz-fixed" (the font family of <code>/<tt> and others) serialize to "monospace" in computed value form. Gecko also serializes the specified form to "-moz-fixed" while we do it as "monospace", I'm not sure which is better.


WPT: /editing/run/fontname.html


 [1]: http://searchfox.org/mozilla-central/source/layout/style/nsRuleNode.cpp#4186

	Cameron McCormack (:heycam)
	Comment 6 • 7 years ago
mozreview-review

Comment on attachment 8875080 [details]
Bug 1370734 - Part 1: stylo: Refactor generic font handling into a method;

https://reviewboard.mozilla.org/r/146434/#review150526

::: servo/components/style/properties/longhand/font.mako.rs:300
(Diff revision 2)
> +                if values.len() == 1 {
> +                    if let FontFamily::Generic(ref name) = values[0] {
> +                        return Some(FontFamily::generic(name).1);
> +                    }
> +                }
> +            };

Nit: no semicolon

	Cameron McCormack (:heycam)
	Comment 7 • 7 years ago
mozreview-review

Comment on attachment 8875081 [details]
Bug 1370734 - Part 2: stylo: Prefill default font when a single generic is set;

https://reviewboard.mozilla.org/r/146446/#review150528

::: layout/style/ServoBindings.cpp:92
(Diff revision 3)
>  static RWLock* sServoLangFontPrefsLock = nullptr;
>  
>  
>  static
>  const nsFont*
> -ThreadSafeGetDefaultFontHelper(const nsPresContext* aPresContext, nsIAtom* aLanguage)
> +ThreadSafeGetDefaultFontHelper(const nsPresContext* aPresContext, nsIAtom* aLanguage, uint8_t genericID)

Nit: aGenericID

::: servo/components/style/properties/properties.mako.rs:2774
(Diff revision 3)
> +                                //
> +                                // We instead skip cascading font-family in that case.
> +                                //
> +                                // In case of the language changing, we wish for a specified font-
> +                                // family to override this, so we do not skip cascading then.
> +                                _skip_font_family = true;

Nit: looks a bit weird having a _-prefixed variable name which we then use (though I see why, given the conditional compilation).  What about doing |font_family = None;| instead of this?

::: servo/components/style/properties/properties.mako.rs:2783
(Diff revision 3)
> +                        context.mutate_style().mutate_font().gecko_mut().mGenericID = generic;
> +                        let pres_context = context.device.pres_context;
> +                        unsafe {
> +                            bindings::Gecko_nsStyleFont_PrefillDefaultForGeneric(context.mutate_style().mutate_font().gecko_mut(),
> +                                                                                 &*pres_context,
> +                                                                                 generic);

NIt: Can you do

  let gecko_font = context.mutate_style().mutate_font().gecko_mut();

and work on that, since you need it twice.

::: servo/components/style/properties/properties.mako.rs:2817
(Diff revision 3)
> -                                                 &mut context,
> +                                                     &mut context,
> -                                                 &mut cacheable,
> +                                                     &mut cacheable,
> -                                                 &mut cascade_info,
> +                                                     &mut cascade_info,
> -                                                 error_reporter);
> +                                                     error_reporter);
> -                % if product == "gecko":
> +                    % if product == "gecko":
> -                    context.style.mutate_font().fixup_none_generic(context.device);
> +                        unsafe { context.style.mutate_font().fixup_none_generic(context.device); }

Don't need this unsafe block?

	Manish Goregaokar [:manishearth] Assignee
	Comment 8 • 7 years ago
mozreview-review-reply

Comment on attachment 8875081 [details]
Bug 1370734 - Part 2: stylo: Prefill default font when a single generic is set;

https://reviewboard.mozilla.org/r/146446/#review150528

> Nit: looks a bit weird having a _-prefixed variable name which we then use (though I see why, given the conditional compilation).  What about doing |font_family = None;| instead of this?

No, because we also check if the family was recalculated when we decide to recascade font-size

> Don't need this unsafe block?

heh I'd changed it initially so that you pass in the default font (so it was unsafe since that's a raw ptr) but I changed it back.

	Manish Goregaokar [:manishearth] Assignee
	Comment 11 • 7 years ago

https://github.com/servo/servo/pull/17211

	Pulsebot
	Comment 12 • 7 years ago

Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9c914937a358
Part 2: stylo: Prefill default font when a single generic is set; r=heycam

	Pulsebot
	Comment 13 • 7 years ago

Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2e5cb33a39c0
Whitelist outparam of Gecko_nsStyleFont_PrefillDefaultForGeneric ; r=bustage

	Pulsebot
	Comment 14 • 7 years ago

Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/38740950a317
Whitelist ThreadSafeGetDefaultFontHelper; r=bustage

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 15 • 7 years ago

That last followup somehow increased the number of write hazards from 5 (out of 4 allowed) to 6. I'm going to bump the limit to six for now to green things up, but this will need to be taken care of soon so we can drop that back to 4.

	Pulsebot
	Comment 16 • 7 years ago

Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5ba76b85e0c9
Temporarily increase the number of allowed write hazards until issues can be sorted out a=me

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 17 • 7 years ago

If it helps, here's the hazard output at various points today:

After manishearth's latest followup (bringing the number of hazards up to 6 out of 4 allowed): https://pastebin.mozilla.org/9023807


Before that latest followup, but after the first one (number of hazards was 5 out of 4 allowed): https://pastebin.mozilla.org/9023808

Before anything from this bug landed to autoland (number of hazards was 4 out of 4 allowed, hazard build job was passing): https://pastebin.mozilla.org/9023809

	Carsten Book [:Tomcat]
	Comment 18 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/9c914937a358
https://hg.mozilla.org/mozilla-central/rev/2e5cb33a39c0
https://hg.mozilla.org/mozilla-central/rev/38740950a317
https://hg.mozilla.org/mozilla-central/rev/5ba76b85e0c9

	Pulsebot
	Comment 19 • 7 years ago

Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a2159eb5765c
Fix hazards ; r=bustage

	Manish Goregaokar [:manishearth] Assignee
	Comment 20 • 7 years ago

Brought it down to 3.

	Carsten Book [:Tomcat]
	Comment 21 • 7 years ago
bugherder

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