Closed Bug 1370734 Opened 2 years ago Closed 2 years ago

stylo: Generic font should copy default font

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: manishearth, Assigned: manishearth)

References

(Depends on 1 open bug)

Details

Attachments

(2 files)

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
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
Attachment #8875080 - Flags: review?(cam) → 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?
Attachment #8875081 - Flags: review?(cam) → 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

> 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.
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
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2e5cb33a39c0
Whitelist outparam of Gecko_nsStyleFont_PrefillDefaultForGeneric ; r=bustage
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/38740950a317
Whitelist ThreadSafeGetDefaultFontHelper; r=bustage
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.
Flags: needinfo?(manishearth)
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
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
Brought it down to 3.
Flags: needinfo?(manishearth)
Depends on: 1511675
You need to log in before you can comment on or make changes to this bug.