678671 - clean up APIs used to get font metrics in layout code

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[David Baron :dbaron:]

I have a series of patches to clean up the APIs that we use to get font metrics in layout code.  This has a few goals:

 * reduce the number of APIs, which both reduces brain-print and reduces the number of things I'll have to look for when working on bug 627842

 * make the code more consistent about passing the correct user font set and language, which:

   + improves behavior

   + makes nsFontCache more effective

I realized about 2/3 of the way through that I should probably have done some de-COM-tamination first, but oh well...


The individual patches are described in their (multiline) commit messages.

Comment 1

[David Baron :dbaron:]

Also, after this patch series, there are *no* font-metrics-getting APIs where the user font set or language are filled in with incorrect default values, which makes it harder to get them wrong.  There are just APIs that require them (nsDeviceContext::GetMetricsFor) and APIs that fill them in correctly (nsLayoutUtils::GetFontMetricsForStyleContext and nsLayoutUtils::GetFontMetricsForFrame).

Comment 2

[David Baron :dbaron:]

Attachment: patch 1: switch lots of callers to nsLayoutUtils::GetFontMetricsFor(Frame|StyleContext)

Comment 3

[David Baron :dbaron:]

Attachment: patch 2: remove nsLayoutUtils::SetFontFromStyle and replace its callers

Comment 4

[David Baron :dbaron:]

Attachment: patch 3: pass correct language from nsRuleNode

Comment 5

[David Baron :dbaron:]

Attachment: patch 4: remove the broken nsPresContext::GetMetricsFor and replace its last caller

Comment 6

[David Baron :dbaron:]

Attachment: patch 5: remove multiple-parameter (shortcut) nsRenderingContext::SetFont methods and replace last few callers

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 552808 [details] [diff] [review]
patch 3: pass correct language from nsRuleNode

Review of attachment 552808 [details] [diff] [review]:
-----------------------------------------------------------------

Seems to me that you could write a test for 'ch' and 'ex' depending on style. That's probably worth doing, especially since it would test your "IMPORTANT" invariant.

::: layout/style/nsRuleNode.cpp
@@ +4574,5 @@
>                            visibility, parentVisibility)
>  
> +  // IMPORTANT: No properties in this struct have lengths in them.  We
> +  // depend on this since CalcLengthWith can call GetStyleVisibility()
> +  // to get the language for resolving fonts!

That was lucky!

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 552809 [details] [diff] [review]
patch 4: remove the broken nsPresContext::GetMetricsFor and replace its last caller

Review of attachment 552809 [details] [diff] [review]:
-----------------------------------------------------------------

Actually, if we pass a real language for everything else, would it make sense to pass a real language atom here? You could use the frame's language. (Or we could just delete this code; when was the last time anyone used it?) That would enable some small cleanups by removing the null-language case in font-groups.

Comment 9

[David Baron :dbaron:]

in re comment 7:
>Seems to me that you could write a test for 'ch' and 'ex' depending on style. >That's probably worth doing, especially since it would test your "IMPORTANT" >invariant.

I'm not quite sure how to do this.  Since it depends on differences in default and generic fonts, any != reftest risks failing (and likely would in some real cases).

I suppose what I could do is write a crashtest-as-mochitest that loops through each CSS property and sets the value to '5ch' to make sure we don't break that invariant.

in re comment 8:
>Actually, if we pass a real language for everything else, would it make sense >to pass a real language atom here? You could use the frame's language. (Or we >could just delete this code; when was the last time anyone used it?) That would >enable some small cleanups by removing the null-language case in font-groups.

will do, though I might use the root frame throughout.  (Though I'm not sure if the real language is guaranteed non-null, but these followups are a project for another time and perhaps another person...)

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to David Baron [:dbaron] from comment #9)
> I'm not quite sure how to do this.  Since it depends on differences in
> default and generic fonts, any != reftest risks failing (and likely would in
> some real cases).

Yes, that's a tough one.

> in re comment 8:
> will do, though I might use the root frame throughout.  (Though I'm not sure
> if the real language is guaranteed non-null, but these followups are a
> project for another time and perhaps another person...)

Sure.

Comment 11

[David Baron :dbaron:]

Actually, it turns out patch 3 needs some work; the aStyleContext parameter that I use is actually optional, and fixing is a little bit tricky, though doable.

Comment 12

[David Baron :dbaron:]

Attachment: patch 3: pass correct language from nsRuleNode

This time passes reftests *and mochitests*, with the added test as well.

Comment 13

[David Baron :dbaron:]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dda49a4f9e4c
https://hg.mozilla.org/integration/mozilla-inbound/rev/f277fe9f70c3
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a378e08192f
https://hg.mozilla.org/integration/mozilla-inbound/rev/83291ec2e28e
https://hg.mozilla.org/integration/mozilla-inbound/rev/f982f2186319

I now realize doing the last sentence of bug 416581 comment 6 (moving mLanguage from nsStyleVisibility to nsStyleFont) would have made patch 3 a bit easier.  Oh well... another time.

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

http://hg.mozilla.org/mozilla-central/rev/dda49a4f9e4c
http://hg.mozilla.org/mozilla-central/rev/f277fe9f70c3
http://hg.mozilla.org/mozilla-central/rev/3a378e08192f
http://hg.mozilla.org/mozilla-central/rev/83291ec2e28e
http://hg.mozilla.org/mozilla-central/rev/f982f2186319