clean up APIs used to get font metrics in layout code

RESOLVED FIXED in mozilla8

Status

()

defect
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

(Blocks 1 bug)

Trunk
mozilla8
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 1 obsolete attachment)

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.
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 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!
Attachment #552808 - Flags: review?(roc) → review+
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.
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...)
(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.
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.
This time passes reftests *and mochitests*, with the added test as well.
Attachment #552808 - Attachment is obsolete: true
Attachment #552935 - Flags: review?(roc)
Depends on: 702043
You need to log in before you can comment on or make changes to this bug.