Last Comment Bug 416581 - style system should use better language information for choosing font preferences
: style system should use better language information for choosing font prefere...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- major with 1 vote (vote)
: mozilla13
Assigned To: Jonathan Watt [:jwatt] (back in October - email directly if necessary)
:
:
Mentors:
Depends on: 775797
Blocks: 91190 646882 806578 813705
  Show dependency treegraph
 
Reported: 2008-02-09 11:42 PST by David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
Modified: 2012-12-12 11:57 PST (History)
11 users (show)
roc: wanted‑next+
roc: blocking1.9-
roc: wanted1.9-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Move mLanguage from nsStyleVisibility to nsStyleFont (19.94 KB, patch)
2011-12-13 14:05 PST, Jonathan Watt [:jwatt] (back in October - email directly if necessary)
dbaron: review-
Details | Diff | Splinter Review
Move mLanguage from nsStyleVisibility to nsStyleFont (30.62 KB, patch)
2011-12-19 20:57 PST, Jonathan Watt [:jwatt] (back in October - email directly if necessary)
no flags Details | Diff | Splinter Review
Move mLanguage from nsStyleVisibility to nsStyleFont (31.15 KB, patch)
2011-12-19 21:23 PST, Jonathan Watt [:jwatt] (back in October - email directly if necessary)
dbaron: review+
Details | Diff | Splinter Review
Current WIP to move mLanguage from nsStyleVisibility to nsStyleFont (32.65 KB, patch)
2011-12-20 09:38 PST, Jonathan Watt [:jwatt] (back in October - email directly if necessary)
no flags Details | Diff | Splinter Review
Current WIP to move mLanguage from nsStyleVisibility to nsStyleFont (37.41 KB, patch)
2012-01-10 09:52 PST, Jonathan Watt [:jwatt] (back in October - email directly if necessary)
no flags Details | Diff | Splinter Review
Move mLanguage from nsStyleVisibility to nsStyleFont - passes Try (39.56 KB, patch)
2012-01-13 04:41 PST, Jonathan Watt [:jwatt] (back in October - email directly if necessary)
dbaron: review+
Details | Diff | Splinter Review
Part 2 - take lang group specific font size prefs into account (42.59 KB, patch)
2012-01-23 01:41 PST, Jonathan Watt [:jwatt] (back in October - email directly if necessary)
no flags Details | Diff | Splinter Review
Part 2 - take lang group specific font size prefs into account (43.91 KB, patch)
2012-01-23 11:28 PST, Jonathan Watt [:jwatt] (back in October - email directly if necessary)
no flags Details | Diff | Splinter Review
Part 2 - take lang group specific font size prefs into account (43.48 KB, patch)
2012-01-27 05:56 PST, Jonathan Watt [:jwatt] (back in October - email directly if necessary)
no flags Details | Diff | Splinter Review
Part 2 - take lang group specific font size prefs into account (43.34 KB, patch)
2012-01-31 05:49 PST, Jonathan Watt [:jwatt] (back in October - email directly if necessary)
dbaron: review+
Details | Diff | Splinter Review

Description David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2008-02-09 11:42:11 PST
We currently have language-group-specific font preferences (for default families and sizes), but we use that only on a per-document level, and based only on the character encoding of the document.

We could do a lot better here, and use the correct pref for the language.  (That said, it might mean users would see the effects of their different font prefs more often -- ones that they may well have no idea how to set.)

(Note that in some cases I think gfx uses the same preferences to get correct language-specific fallbacks.)
Comment 1 John Daggett (:jtd) 2008-02-14 21:57:36 PST
(In reply to comment #0)
> We currently have language-group-specific font preferences (for default
> families and sizes), but we use that only on a per-document level, and based
> only on the character encoding of the document.

At the rendering level this is specified by the gfxFontStyle attached to a font group.

> We could do a lot better here, and use the correct pref for the language. 
> (That said, it might mean users would see the effects of their different font
> prefs more often -- ones that they may well have no idea how to set.)

Hmmm, what's the "correct pref for the language" mean precisely?  This is a very tricky question, see bug 390900 for an interesting example of where things get confusing, there are some really weird interactions with generics and language setting.  And with CJK characters it's hard to know which pref setting to use if the charset is Unicode.

> (Note that in some cases I think gfx uses the same preferences to get correct
> language-specific fallbacks.)

This is part of the trickiness, in the fallback case we infer the lang of individual characters and use the pref font settings for that lang before doing a system-wide font search.

Comment 2 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2008-02-15 00:01:17 PST
At the very least, I was referring to picking up the information provided by the lang attribute, which is reflected in GetStyleVisibility()->mLangGroup.
Comment 3 John Daggett (:jtd) 2008-02-15 00:04:27 PST
Holy crap, we're not doing that already?!?  Yes, I completely agree, that needs to get fixed.
Comment 4 Simon Montagu :smontagu 2008-02-17 01:18:41 PST
Holy crap indeed. Is that a regression from Thebes? Do you have a test case? We certainly used to use the lang attribute.
Comment 5 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2008-02-17 11:26:12 PST
So I think *gfx* uses the lang attribute as part of font selection.  At least I think it does.  (Although for a while it didn't, but I think that regression was fixed.)

The issue is that we don't use the lang attribute to apply the user's preferences for default fonts -- at least when the style system applies them.  I think in some cases (looks like Windows and Mac, but not GTK) the gfx code may *reapply* the same preferences (or use them for something -- I'm not sure exactly what), but a lower priority than the style system doing it, since what gfx gets from layout is a concatenation of what the author specified and the user's preference.
Comment 6 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-03-31 09:37:08 PDT
So, to be clear, this definitely affects the language-specific font *size* preferences, and possibly also affects the serif vs. sans-serif default preference (depending on how the two things that read that pref interact).

Testcases for the font size pref issue are:
 * the FIXME in layout/style/test/test_bug401046.html
 * the fact that the same test fails on non-Western locales


Also, I think the first step for fixing this bug is to move mLanguage from nsStyleVisibility to nsStyleFont -- that'll fix a bunch of potential cross-struct-dependency issues.
Comment 7 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2011-12-13 14:05:28 PST
Created attachment 581424 [details] [diff] [review]
Move mLanguage from nsStyleVisibility to nsStyleFont

It's not clear to me exactly what I should be doing in nsStyleFont::CalcDifference since I don't have mVisible there, and it's not clear to me what should take priority when integrated with the existing code in that method.
Comment 8 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-12-13 14:54:55 PST
Comment on attachment 581424 [details] [diff] [review]
Move mLanguage from nsStyleVisibility to nsStyleFont

nsRuleNode.cpp:

  Remove the aStyleVisibility parameters to:
    CalcLengthWith
    CalcLengthCalcOps (mStyleVisibility too)
    GetMetricsFor

nsStyleStruct.cpp:

  The CalcDifference is really just weirdly optimized code:  basically,
  the logic is things like this:
    if (a != other.a)
      return reflow;
    if (b != other.b)
      return paint;
 etc.  However, what's confusing you is that there's a bunch of if-else
 nesting that uses == rather than !=.  You really just need to return
 NS_STYLE_HINT_REFLOW when the language is different, just like when
 the size is different.  It's probably clearer to make it:

  if (mSize != aOther.mSize ||
      mLanguage != aOther.mLanguage) {
    return NS_STYLE_HINT_REFLOW;
  }
  return CalcFontDifference(mFont, aOther.mFont);

  But you also need to leave the visibility-testing logic in
  nsStyleVisibility::CalcDifference.

r=dbaron with that, but I want to look at the revisions to
the CalcDifference methods
Comment 9 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2011-12-19 20:57:56 PST
Created attachment 583058 [details] [diff] [review]
Move mLanguage from nsStyleVisibility to nsStyleFont
Comment 10 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2011-12-19 21:20:35 PST
Comment on attachment 583058 [details] [diff] [review]
Move mLanguage from nsStyleVisibility to nsStyleFont

gah, forgot to do a final qref
Comment 11 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2011-12-19 21:23:23 PST
Created attachment 583063 [details] [diff] [review]
Move mLanguage from nsStyleVisibility to nsStyleFont

Probably a good idea to look over the entire interdiff BTW.

Obviously I'll remove the try flags from the commit message before pushing.
Comment 12 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-12-20 08:59:54 PST
Comment on attachment 583063 [details] [diff] [review]
Move mLanguage from nsStyleVisibility to nsStyleFont

Sorry, more things to remove in nsRuleNode that I missed the last time:

  param from SetFontSize
  param and member from SetFontSizeCalcOps
  languageVisibility variable in nsRuleNode::SetFont

  (and if you see more that I missed this time too, go ahead)

r=dbaron with those removed and the "try: ..." not in the commit message
Comment 13 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2011-12-20 09:38:19 PST
Created attachment 583202 [details] [diff] [review]
Current WIP to move mLanguage from nsStyleVisibility to nsStyleFont

Actually I've had to make a change in content/html/content/src/nsGenericHTMLElement.cpp and in layout/style/nsCSSPropList.h to fix Try crashes.

Even now I'm still getting failures in reftests/canvas/text-font-lang.html and reftests/text/auto-hyphenation-1.html, auto-hyphenation-1a.html and auto-hyphenation-5.html, and I'm having some trouble figuring out what I've missed/done wrong.

https://tbpl.mozilla.org/?tree=Try&rev=24c2dc528f8f

Once that's fixed I'll remove the extra stuff you've requested to be removed and post a new patch.
Comment 14 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-01-04 17:33:16 PST
The problem is basically that the 'lang' attribute doesn't make it through to the computed style for HTML canvas elements for some reason. I don't see anything wrong with my code, and I'm having a little trouble figuring out the problem using gdb. Can you think of anything obvious that I might have missed (hidden in a macro or something?) David? If not I'll keep working on getting to grips with how style is computed until I figure it out.
Comment 15 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-01-05 16:26:31 PST
I notice the following comment in the code:

const void*
nsRuleNode::ComputeVisibilityData(void* aStartStruct,
                                  const nsRuleData* aRuleData,
                                  nsStyleContext* aContext,
                                  nsRuleNode* aHighestNode,
                                  const RuleDetail aRuleDetail,
                                  const bool aCanStoreInRuleTree)
{
  COMPUTE_START_INHERITED(Visibility, (mPresContext),
                          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!


I'm not sure what that means, but I wonder if it's of concern now that mLanguage is on nsStyleFont, which does have lengths.
Comment 16 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-01-10 09:52:11 PST
Created attachment 587354 [details] [diff] [review]
Current WIP to move mLanguage from nsStyleVisibility to nsStyleFont
Comment 17 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-01-12 15:41:02 PST
(In reply to Jonathan Watt [:jwatt] from comment #14)
> The problem is basically that the 'lang' attribute doesn't make it through
> to the computed style...

What seems to be going wrong is this: when nsRuleNode::ComputeFontData() is called under WalkRuleTree(), things start out okay. At the beginning of the method the '-x-lang' (eCSSProperty__x_lang) property is correctly inherited into the new nsStyleFont object, 'font'. Further down though, the nsRuleNode::SetGenericFont() call overwrites this property when it should not.

I really don't understand why we're calling SetGenericFont() to do what it does - i.e. recalculate the cascade from the first ancestor that has a generic font family name (seems a bit quirks-mode'y). More to the point I was pretty surprising to discover that a setter includes it's own mini take on the whole cascade implementation. Anyway, I'll take it on faith that this is what we want to do.

Digging into SetGenericFont(), the issue seems to be that when we call nsRuleNode::SetFont() with the nsRuleData that's worked out from the style context with the '-x-lang' property set, it doesn't actually do anything for this property. Since the nsStyleFont that's being worked out under SetGenericFont is then used to reset the initial one in ComputeFontData, this is how the '-x-lang' property on the initial one unexpectedly goes from having the right value to having the wrong value.

So it seems that my patch just needs some code added to nsRuleNode::SetFont() to handle the '-x-lang' property. It's not clear to me at this point what that code should look like, but I'll try and figure it out tomorrow after some sleep.
Comment 18 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-01-12 15:44:00 PST
Oh, actually there was one other place that I stumbled across during my debugging that looked like it _might_ need some new code to handle lang - CheckFontCallback. Not sure about that one yet though.
Comment 19 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-01-13 04:41:30 PST
Created attachment 588375 [details] [diff] [review]
Move mLanguage from nsStyleVisibility to nsStyleFont - passes Try
Comment 20 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-01-13 04:52:32 PST
Other than the removal of nsStyleVisibility variables than are now unnecessary as requested in comment 12, there are only a few significant changes from the last r+'ed patch:

 * the change in nsGenericHTMLElement.cpp
 * the change in nsCSSPropList.h
 * an XXX comment in CheckFontCallback
 * the change to nsRuleNode::SetFont - not perfect, but I'm hoping you (David) can comment on what I should be doing there
Comment 21 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-01-19 19:58:43 PST
Comment on attachment 588375 [details] [diff] [review]
Move mLanguage from nsStyleVisibility to nsStyleFont - passes Try

In nsRuleNode.cpp, in CheckFontCallback, you have:

>+  // XXX check?: const nsCSSValue* langValue = aRuleData->ValueForLang();

Just remove the comment.  This function is for detecting special values that
act a lot like 'inherit' (in particular, require the same sort of disabling of
sharing that 'inherit' does).  lang doesn't have anything like that.


You don't need to duplicate the code between nsRuleNode::ComputeFontData 
and nsRuleNode::SetFont.  SetFont is a helper function for
ComputeFontData, so you're doing the same work twice, and in some cases 
even more than twice.  I think it should be *only* in ComputeFontData, 
but probably earlier in the function -- definitely before the calls to
SetFont and SetGenericFont -- and probably basically at the beginning:   
I'd say put it right after the 10-line comment after the
COMPUTE_START_INHERITED line.  You should, in fact, note in a comment 
that it's important that the language be set before any of the 
CalcLengthWith calls in SetFont (direct calls or calls via SetFontSize)
for the cases where |parentFont| is the same as |font|.

r=dbaron with that
Comment 22 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-01-23 01:37:43 PST
(In reply to David Baron [:dbaron] from comment #21)
> You don't need to duplicate the code between nsRuleNode::ComputeFontData 
> and nsRuleNode::SetFont.  SetFont is a helper function for
> ComputeFontData, so you're doing the same work twice, and in some cases 
> even more than twice.  I think it should be *only* in ComputeFontData

That would break inheritance of mLanguage when SetGenericFont is called. As mentioned in comment 17, SetGenericFont overwrites the nsStyleFont that ComputeFontData passes to it, so setting mLanguage in ComputeFontData before the SetGenericFont call is useless when it's called.

It looks like setting mLanguage at the top of SetFont is early enough to avoid any CalcLengthWith problems, so it seems best to remove the duplicate mLanguage setting code from ComputeFontData instead. (Also that passes Try.) Is that okay with you? You can see the patch I pushed to Try at: https://hg.mozilla.org/try/rev/de90114784f5
Comment 23 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-01-23 01:41:28 PST
Created attachment 590658 [details] [diff] [review]
Part 2 - take lang group specific font size prefs into account

You may with to change the method that's used to cache the prefs, but funtionally this fixes the bug.
Comment 24 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-01-23 11:28:08 PST
Created attachment 590801 [details] [diff] [review]
Part 2 - take lang group specific font size prefs into account

Tidy that up a little.
Comment 25 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-01-23 13:48:59 PST
(In reply to Jonathan Watt [:jwatt] from comment #22)
> It looks like setting mLanguage at the top of SetFont is early enough to
> avoid any CalcLengthWith problems, so it seems best to remove the duplicate
> mLanguage setting code from ComputeFontData instead. (Also that passes Try.)
> Is that okay with you? You can see the patch I pushed to Try at:
> https://hg.mozilla.org/try/rev/de90114784f5

Sounds good.
Comment 26 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-01-23 14:18:33 PST
For some reason the second patch seems to be causing layout/style/test/chrome/test_moz_document_rules.html to time out _after_ all the tests in that file have passed. It happens on all platforms on Try, but I can't reproduce locally. If anyone has any ideas...
Comment 27 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-01-24 05:12:54 PST
Pushed part 1 to m-i: https://hg.mozilla.org/integration/mozilla-inbound/rev/cfc432c9bfd1
Comment 28 Ed Morley [:emorley] 2012-01-25 07:21:32 PST
https://hg.mozilla.org/mozilla-central/rev/cfc432c9bfd1

Leaving open for the rest.
Comment 29 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-01-27 05:56:59 PST
Created attachment 592113 [details] [diff] [review]
Part 2 - take lang group specific font size prefs into account
Comment 30 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-01-27 07:03:36 PST
Seems to pass Try now BTW: https://tbpl.mozilla.org/?tree=Try&rev=a90db6ed7bc7
Comment 31 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-01-31 05:49:46 PST
Created attachment 593059 [details] [diff] [review]
Part 2 - take lang group specific font size prefs into account
Comment 32 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-02-13 15:48:16 PST
Comment on attachment 593059 [details] [diff] [review]
Part 2 - take lang group specific font size prefs into account

Make LangGroupFontPrefs::mLangGroup an nsCOMPtr<nsIAtom> and remove all
of the manual reference counting (i.e., the 2 occurrences of
NS_IF_RELEASE and 1 occurrence of NS_ADDREF that your patch adds).

+      NS_ABORT_IF_FALSE(++count < 35, "Lang group count exceeded!!!");

This shouldn't be an abort, given that if we add new lang groups
we might exceed it.  Make it an assertion or have a way of actually
detecting the number of lang groups at compile time (e.g., if they're
listed in a header file with constants).

Remove the "= nsnull" default to the parameter to
nsPresContext::MinFontSize.  It looks like it's unused (and if it's not,
then this bug isn't fully fixed).

>+  friend class nsAutoPtr<LangGroupFontPrefs>;

This seems odd.  Is it needed?  Can it be replaced by
|friend struct LangGroupFontPrefs|?

Don't give nsPresContext::GetFontPrefsForLang's parameter a default
value.  We want callers to pass the correct language (and all the ones
you've added do, so it's trivial to remove).

>   // See if we are in the chrome
>   // We only need to know this to determine if we have to use the
>   // document fonts (overriding the useDocumentFonts flag), or to
>   // determine if we have to override the minimum font-size constraint.
>-  if ((!useDocumentFonts || minimumFontSize > 0) && mPresContext->IsChrome()) {
>+  if (!useDocumentFonts && mPresContext->IsChrome()) {

Please fix up this comment to remove mention of min font size.


nsRuleNode::ComputeTextData:

  Hmm.  This is sort of bad.  Technically you need to move the
  "canStoreInRuleTree = false" along with the GetStyleFont line,
  since the cross-struct dependency is what means we can't store
  the result in the rule tree.  That's a bid of de-optimization,
  but I think it's probably best to just bite the bullet and do
  it.  Effectively it only means that 'line-height: 15px' is
  no better than 'line-height: 1.2em', so I'm not too worried.

nsStyleStruct.cpp:

  I think this is wrong, and hopefully even testably so (on pages where
  the document doesn't specify a font).  You should initialize mFont
  after calling Init(), and pass mLanguage.  And if you can, add
  a test, but don't spend too much time trying?

r=dbaron with those things fixed
Comment 33 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-02-13 18:39:51 PST
(In reply to David Baron [:dbaron] (busy through 12 Feb) from comment #32)
> Comment on attachment 593059 [details] [diff] [review]
> This shouldn't be an abort, given that if we add new lang groups
> we might exceed it.  Make it an assertion or have a way of actually
> detecting the number of lang groups at compile time (e.g., if they're
> listed in a header file with constants).

I believe the lang groups are built up from:

https://mxr.mozilla.org/mozilla-central/source/intl/locale/src/langGroups.properties?raw=1

Shall I add a LangGroupCount() method to nsILanguageAtomService (should be easy)?

> Remove the "= nsnull" default to the parameter to
> nsPresContext::MinFontSize.  It looks like it's unused (and if it's not,
> then this bug isn't fully fixed).

Currently no lang argument is passed to MinFontSize by:

  TransferZoomLevels (in nsDocument.cpp)
  DocumentViewerImpl::SetMinFontSize
  DocumentViewerImpl::GetMinFontSize

So having the nsnull default for them seemed okay. I could make it explicit though.

> >+  friend class nsAutoPtr<LangGroupFontPrefs>;
> 
> This seems odd.  Is it needed?  Can it be replaced by
> |friend struct LangGroupFontPrefs|?

It's needed to allow the nsAutoPtr<LangGroupFontPrefs> dtor to access LangGroupFontPrefs's dtor (LangGroupFontPrefs is protected, and one of its members is an nsAutoPtr).

> nsRuleNode::ComputeTextData:
> 
>   Hmm.  This is sort of bad.  Technically you need to move the
>   "canStoreInRuleTree = false" along with the GetStyleFont line,
>   since the cross-struct dependency is what means we can't store
>   the result in the rule tree.  That's a bid of de-optimization,
>   but I think it's probably best to just bite the bullet and do
>   it.  Effectively it only means that 'line-height: 15px' is
>   no better than 'line-height: 1.2em', so I'm not too worried.

I don't understand. Merely calling GetStyleFont means that canStoreInRuleTree must be set to false?

> nsStyleStruct.cpp:
> 
>   I think this is wrong, and hopefully even testably so (on pages where
>   the document doesn't specify a font).  You should initialize mFont
>   after calling Init(), and pass mLanguage.  And if you can, add
>   a test, but don't spend too much time trying?

Okay, I'll need to set nsStyleFont::mFont in nsRuleNode::SetFont() after setting mLanguage, then.
Comment 34 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-02-15 11:50:13 PST
(In reply to Jonathan Watt [:jwatt] from comment #33)
> (In reply to David Baron [:dbaron] (busy through 12 Feb) from comment #32)
> > Comment on attachment 593059 [details] [diff] [review]
> > This shouldn't be an abort, given that if we add new lang groups
> > we might exceed it.  Make it an assertion or have a way of actually
> > detecting the number of lang groups at compile time (e.g., if they're
> > listed in a header file with constants).
> 
> I believe the lang groups are built up from:
> 
> https://mxr.mozilla.org/mozilla-central/source/intl/locale/src/langGroups.
> properties?raw=1
> 
> Shall I add a LangGroupCount() method to nsILanguageAtomService (should be
> easy)?

Sounds fine.

> > Remove the "= nsnull" default to the parameter to
> > nsPresContext::MinFontSize.  It looks like it's unused (and if it's not,
> > then this bug isn't fully fixed).
> 
> Currently no lang argument is passed to MinFontSize by:
> 
>   TransferZoomLevels (in nsDocument.cpp)
>   DocumentViewerImpl::SetMinFontSize
>   DocumentViewerImpl::GetMinFontSize
> 
> So having the nsnull default for them seemed okay. I could make it explicit
> though.

Explicit would be better.

That said, what's the doc-specific min font size used by?  It seems like this would sort of break it.

> > nsRuleNode::ComputeTextData:
> > 
> >   Hmm.  This is sort of bad.  Technically you need to move the
> >   "canStoreInRuleTree = false" along with the GetStyleFont line,
> >   since the cross-struct dependency is what means we can't store
> >   the result in the rule tree.  That's a bid of de-optimization,
> >   but I think it's probably best to just bite the bullet and do
> >   it.  Effectively it only means that 'line-height: 15px' is
> >   no better than 'line-height: 1.2em', so I'm not too worried.
> 
> I don't understand. Merely calling GetStyleFont means that
> canStoreInRuleTree must be set to false?

Exactly.  Style data can be stored in the rule tree only when they're a function only of the style rules represented by that rule node (i.e., they're not a function of anything inherited from parent style contexts).  Since the contents of GetStyleFont could contain values coming from inheritance from a parent style context (and in fact likely do), it means that the resulting data can't be cached in the rule tree.
Comment 35 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-02-15 16:00:02 PST
(In reply to David Baron [:dbaron] from comment #34)
> (In reply to Jonathan Watt [:jwatt] from comment #33)
> > Currently no lang argument is passed to MinFontSize by:
> > 
> >   TransferZoomLevels (in nsDocument.cpp)
> >   DocumentViewerImpl::SetMinFontSize
> >   DocumentViewerImpl::GetMinFontSize
> > 
> > So having the nsnull default for them seemed okay. I could make it explicit
> > though.
> 
> Explicit would be better.
> 
> That said, what's the doc-specific min font size used by?  It seems like
> this would sort of break it.

It comes from bug 623820, which gave the ability for some code in mobile/xul/chrome/content/content.js to use the nsIMarkupDocumentViewer interface to set the mMinFontSize member of nsPresContext so that the use of text-zoom on mobile doesn't screw up rendering. So this is a document wide thing, and we need to have a way to ask nsPresContext::MinFontSize for this value in order for it to propagate correctly. So I think what the patch does now is correct.

I'll change it to pass an explicit nsnull though.

> > I don't understand. Merely calling GetStyleFont means that
> > canStoreInRuleTree must be set to false?
> 
> Exactly.  Style data can be stored in the rule tree only when they're a
> function only of the style rules represented by that rule node (i.e.,
> they're not a function of anything inherited from parent style contexts). 
> Since the contents of GetStyleFont could contain values coming from
> inheritance from a parent style context (and in fact likely do), it means
> that the resulting data can't be cached in the rule tree.

Just to be clear, this is a preexisting bug that I'm fixing then. Previously that min font size came from looking at the nsStyleFont - the patch just delays the point at which we looked at it.
Comment 36 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-02-15 16:18:14 PST
(In reply to Jonathan Watt [:jwatt] from comment #35)
> Just to be clear, this is a preexisting bug that I'm fixing then. Previously
> that min font size came from looking at the nsStyleFont - the patch just
> delays the point at which we looked at it.

Oh, sorry, I get it. It's only now that the min font size depends on the data in the nsStyleContext that the node can't be cached in the rule tree.
Comment 37 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-02-18 13:40:37 PST
Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/8bdb4c87ad78

(In reply to David Baron [:dbaron] from comment #32)
> nsStyleStruct.cpp:
> 
>   I think this is wrong, and hopefully even testably so (on pages where
>   the document doesn't specify a font).  You should initialize mFont
>   after calling Init(), and pass mLanguage.  And if you can, add
>   a test, but don't spend too much time trying?

As explained on IRC, passing nsnull as the language makes GetDefaultFont use the document's language, which is what we want for the default. I've added a comment noting that in the nsStyleStruct.cpp caller as requested by dbaron.

(In reply to David Baron [:dbaron] from comment #34)
> > Shall I add a LangGroupCount() method to nsILanguageAtomService (should be
> > easy)?
> 
> Sounds fine.

Actually, that turned out to be tricky, so I went the NS_ASSERTION route.
Comment 38 Ed Morley [:emorley] 2012-02-20 10:25:07 PST
https://hg.mozilla.org/mozilla-central/rev/8bdb4c87ad78

Note You need to log in before you can comment on or make changes to this bug.