Pass lang-group into GetMetricsFor more consistently

REOPENED
Assigned to

Status

()

REOPENED
11 years ago
10 years ago

People

(Reporter: sharparrow1, Assigned: sharparrow1)

Tracking

Trunk
x86
Windows XP
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [not needed for 1.9])

Attachments

(2 attachments)

(Assignee)

Description

11 years ago
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.
(Assignee)

Comment 1

11 years ago
Created attachment 275069 [details] [diff] [review]
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.
Attachment #275069 - Flags: review?(roc)
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.
Attachment #275069 - Flags: superreview+
Attachment #275069 - Flags: review?(roc)
Attachment #275069 - Flags: review+
(Assignee)

Updated

11 years ago
Attachment #275069 - Flags: approval1.9?
(Assignee)

Comment 3

11 years ago
Created attachment 275296 [details] [diff] [review]
Patch

Here are the SVG changes (small patch, but I figure an SVG peer should look at it).
Attachment #275296 - Flags: superreview?(roc)
Attachment #275296 - Flags: review?(tor)
(Assignee)

Comment 4

11 years ago
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.

Updated

11 years ago
Attachment #275296 - Flags: review?(tor) → review+
Comment on attachment 275069 [details] [diff] [review]
General patch

a1.9=dbaron
Attachment #275069 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 6

11 years ago
Comment on attachment 275069 [details] [diff] [review]
General patch

Checked in "General patch".
Attachment #275296 - Flags: superreview?(roc) → superreview+
(Assignee)

Updated

11 years ago
Attachment #275296 - Flags: approval1.9?
Comment on attachment 275296 [details] [diff] [review]
Patch

a1.9=dbaron
Attachment #275296 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 8

11 years ago
SVG fix checked in.
These patches were both checked-in... resolving.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
I'm not sure if that means that this was finished, though.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → ASSIGNED

Comment 11

11 years ago
is this done? can someone mark it fixed if so?
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.
Attachment #275069 - Flags: approval1.9+
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.
Attachment #275296 - Flags: approval1.9+
Comment on attachment 275069 [details] [diff] [review]
General patch

Checked-in, as per comment #9.
Attachment #275069 - Flags: approval1.9?
Comment on attachment 275296 [details] [diff] [review]
Patch

Checked-in, as per comment #9.
Attachment #275296 - Flags: approval1.9?
Comment on attachment 275296 [details] [diff] [review]
Patch

Reset approval flag to + as it was already checked in.
Attachment #275296 - Flags: approval1.9? → approval1.9+
Comment on attachment 275069 [details] [diff] [review]
General patch

Reset approval flag to + as it was already checked in.
Attachment #275069 - Flags: approval1.9? → approval1.9+

Comment 18

10 years ago
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. 
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago10 years ago
Resolution: --- → INCOMPLETE

Comment 19

10 years ago
Change of plans...reopening, closing bug 433050, and adding whiteboard flag [not needed for 1.9] as suggested in irc. Sorry about the confusion.
Status: RESOLVED → REOPENED
Resolution: INCOMPLETE → ---
Whiteboard: [not needed for 1.9]
You need to log in before you can comment on or make changes to this bug.