390740 - Pass lang-group into GetMetricsFor more consistently

Status: REOPENED

(Core :: Layout, defect)

Description

[Eli Friedman]

Getting the right font metrics for a font depends on getting the right lang-group; otherwise, we're likely to pick the wrong font, especially for CJK languages.

The way I'm going about this is calling the helpers in nsLayoutUtils more consistently; these helpers do the right thing.

Comment 1

[Eli Friedman]

Attachment: General patch

I also have some changes in my tree to cover SVG, MathML, and style/; I'll wait for review on this before I post them, though.

Comment 2

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

Comment on attachment 275069 [details] [diff] [review]
General patch

These helper functions should probably return already_Addrefed<nsIFontMetrics>... or even, eventally, already_Addrefed<gfxFontMetrics>. But not necessary now.

Comment 3

[Eli Friedman]

Attachment: Patch

Here are the SVG changes (small patch, but I figure an SVG peer should look at it).

Comment 4

[Eli Friedman]

I guess I should comment on the risk for this patch: it should be very low-risk, because the helper methods do basically the same thing as the current code, except that they pass in the right lang-group.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 275069 [details] [diff] [review]
General patch

a1.9=dbaron

Comment 6

[Eli Friedman]

Comment on attachment 275069 [details] [diff] [review]
General patch

Checked in "General patch".

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 275296 [details] [diff] [review]
Patch

a1.9=dbaron

Comment 8

[Eli Friedman]

SVG fix checked in.

Comment 9

[Reed Loden [:reed]]

These patches were both checked-in... resolving.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm not sure if that means that this was finished, though.

Comment 11

[Stuart Parmenter]

is this done? can someone mark it fixed if so?

Comment 12

[Damon Sicore (:damons)]

Comment on attachment 275069 [details] [diff] [review]
General patch

Resetting all approval1.9+ flags on bugs that have not been checked in by Oct 22 11:59 PM PDT.  Please re-request approval if needed.

Comment 13

[Damon Sicore (:damons)]

Comment on attachment 275296 [details] [diff] [review]
Patch

Resetting all approval1.9+ flags on bugs that have not been checked in by Oct 22 11:59 PM PDT.  Please re-request approval if needed.

Comment 14

[Reed Loden [:reed]]

Comment on attachment 275069 [details] [diff] [review]
General patch

Checked-in, as per comment #9.

Comment 15

[Reed Loden [:reed]]

Comment on attachment 275296 [details] [diff] [review]
Patch

Checked-in, as per comment #9.

Comment 16

[Damon Sicore (:damons)]

Comment on attachment 275296 [details] [diff] [review]
Patch

Reset approval flag to + as it was already checked in.

Comment 17

[Damon Sicore (:damons)]

Comment on attachment 275069 [details] [diff] [review]
General patch

Reset approval flag to + as it was already checked in.

Comment 18

[cajbir (:cajbir)]

This landed, but there is some question about whether it is fixed (see comments 10 and 11). We want to get it off the list
for "isn't closed but has approved patch for FF3", so I'm closing this and I've
created the followup bug 433050. 

Comment 19

[cajbir (:cajbir)]

Change of plans...reopening, closing bug 433050, and adding whiteboard flag [not needed for 1.9] as suggested in irc. Sorry about the confusion.

Comment 20

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.