91478 - nsStyleFont::CalcDifference() too liberal

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect, P4)

Description

[Chris Waterson]

While investigating bug 90205, I discovered that nsStyleFont::CalcDifference()
will return NS_STYLE_HINT_REFLOW if the font's mFlags differ.

This seems a bit too liberal, because at least two of the flags
(NS_STYLE_FONT_[FACE|SIZE]_EXPLICIT) are write-only. (The only code that uses
them is some #if 0'd form control code -- see nsFormControlHelper.cpp:626;
perhaps we ought remove these flags altogether?)


The last flag (NS_STYLE_FONT_USE_FIXED) simply blasts the fixed font into the
normal font slot AFAICT. So...

Do we care if the flags differ at all? Could we just remove the flag comparison
code in CalcDifference() and let the normal font comparison routines do their thing?

Comment 1

[Chris Waterson]

Attachment: proposed change, diff -ub

Comment 2

[Marc Attinasi]

If the flag NS_STYLE_FONT_USE_FIXED changes, I think we need to reflow since the
layout will change to use or not use the fixed font, right? 

Other than that, I think you are probably right about the other flags (in other
words, we should remove the unused flags and restrict the flag comparison to
just the USE_FIXED flag - unless that turns out to be unnecessary too).

Comment 3

[Chris Waterson]

But at nsRuleNode.cpp:1527 (where we actually fill in the nsStyleFont's data),
we explicitly reset mFont to mFixedFont. (This is the only place that the
NS_STYLE_FONT_USE_FIXED is read.) With that in mind,
nsStyleFont::CalcFontDifference() should do the right thing, no?

Comment 4

[Marc Attinasi]

Yup, you're right. Since nsStyleFont::CalcDifference() computes the difference
between both fonts (fixed and non-fixed) it will correctly handle the swith made
in the ruleNode. 

Comment 5

[David Hyatt]

r/sr=hyatt

Comment 6

[Marc Attinasi]

r=attinasi

Comment 7

[rbs]

In other words, I can get rid of mFlags from nsStyleFont in my patch at that 
other bug 84398, since I don't use NS_STYLE_FONT_USE_FIXED anymore. A little 
more savings.

Comment 8

[Pierre Saslawsky]

Please get in touch with [e-mail] before removing these flags.

Comment 9

[Marc Attinasi]

Rod is on sabbatical - he won't be back for 5 more weeks. If he really objects,
he can always add back the dead, unused and ancient code (he #if 0'd it on Jan
25, 2000).

Comment 10

[Pierre Saslawsky]

Rod's on sabbatical? Cool! I'm going to re-enable the native widgets next week!

Comment 11

[Chris Waterson]

Changes checked in.

Comment 12

[rbs]

Adding this here for the record. As I was about to remove mFlags from
FONT_20011307_BRANCH, it occurred to me that one information carried
by these flags was to differentiate between the following two situations:

<span style="font-size:24pt">
  ...
  <span style="font-size:50%"> ... text at 12pt ... </span>
</span>

vs: (note: '50%' above could be something else, e.g., 'smaller' with a different 
parent font, and there could be an hierarchy of such elements)

<span style="font-size:24pt">
  ...
  <span style="font-size:12pt"> ... text at 12pt ... </span>
</span>

But the direct test on the final resolved font structs would tell if there is 
any difference and ensure that a reflow hint is returned. So I do not see why 
mFlags should be kept if it is write-only.

There is a subtelty as to whether the CalcLength()/CalPointSize() functions (the 
functions that maps the non-fixed values to the final fixed values) can lead to
edge cases that warrant having the test against mFlags in CalcFontDifference() 
somewhere in the StyleContext hierarchy. So far, I can't picture such edge 
cases that would not be detected by the direct test on the final resolved font 
structs. So off I go, I am going to remove mFlags in FONT_20011307_BRANCH 
because it is write-only there.

Comment 13

[R.K.Aa.]

would the crash at http://dhtmlkitchen.com/dhtml/dom/extendingDOM.html be
related to this?

Comment 14

[Madhur Bhatia]

since the code was checked in more than a year and a half ago, marking the bug
verified