280443 - font-family should be stored as array of strings, not a string that requires parsing (escaped quotes)

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

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

font-family should be stored as an array of strings, not as a string that
requires parsing.  (I think media lists have the same problem, and the parsing
function that we've copied and pasted in various places has bugs:  see bug 235755.)

Right now the CSS parser parses the font-family correctly and then tries to
stuff all the information back into a string -- this isn't done fully correctly.

See the attached testcase, where I use \" to get quotes into the interior of a
single string, which when appended and reparsed appears to be multiple strings.

Comment 1

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

Attachment: testcase showing bug (requires one of Arial, Helvetica, or Verdana fonts present)

Comment 2

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

Note that we might consider storing it as an array of atoms, even.

Comment 3

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

"patch 1" in bug 481591 should make the CSS side of this a bit easier.

Comment 4

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

When fixing this, we should fix the "XXX" comment in http://hg.mozilla.org/mozilla-central/rev/41394e846c1b

Comment 5

[John Daggett (:jtd)]

Haven't looked at the patch yet but it needs to have a way to handle generics, i.e. "serif" and serif are a locally available font family called "serif" and serif without quotes refers to the user agent-defined serif font face.  But I completely agree, parsing font lists does not belong in gfx code

Comment 6

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

Yep.  Basically I'd want, instead of a string, a simple class that (1) allowed nsFont::EnumerateFamilies to continue working as it does today (except without the various reparsing/escaping bugs) and (2) let you do the same thing as EnumerateFamilies without a callback function by examining appropriate members.

But there's currently no patch.  Just figured you ought to be cc:ed on the bug.

Comment 7

[Kang-Hao (Kenny) Lu [:kennyluck]]

Attachment: patch 1, v1 - minimal change in /layout/, mostly on nsCSSParser.cpp. (WIP)

Comment 8

[Kang-Hao (Kenny) Lu [:kennyluck]]

Attachment: patch 2, v2 - for MathML @fontfamily and <font> @face. (WIP)

Comment 9

[Kang-Hao (Kenny) Lu [:kennyluck]]

The above two patches fix the test and bug 660397. I think it is the minimal patch that's required to fix this without breaking tests. Certain ugliness in this patch makes me feel that I should ask for feedback as early as possible.

There currently seem to be three locations where 'font-family' is directly written to nsFont.name without going through CSSParserImpl::ParseFamily:

1. <font>'s @face
2. system font (i.e., 'font: icon;' and so on) from nsRuleNode::SetFont.
3. MathML's @fontfamily

I'm not sure calling CSSParserImpl::ParseFamily from these locations is a good idea or not as I don't have a sense of modularity. They are currently using some ad-hoc parser+serializer: For 1., it current doesn't support quoted font family, which is probably not Web-compatible, but I doubt using the CSS parser (seems to be what's required by the HTML spec) is Web-compatible either (I'll do some testing on other browsers).

If this is accepted, it'll be pretty straightforward to fix bug 605231 and bug 748683.

 (In reply to John Daggett (:jtd) from comment #5)
> Haven't looked at the patch yet but it needs to have a way to handle
> generics, i.e. "serif" and serif are a locally available font family called
> "serif" and serif without quotes refers to the user agent-defined serif font
> face.

It seems that we already support this (CSS2.1 test suite font-family-rule-012 ~ 016). Do we have reftests about these?

(In reply to David Baron [:dbaron] from comment #6)
> Yep.  Basically I'd want, instead of a string, a simple class that (1)
> allowed nsFont::EnumerateFamilies to continue working as it does today
> (except without the various reparsing/escaping bugs) and (2) let you do the
> same thing as EnumerateFamilies without a callback function by examining
> appropriate members.

I am not sure if I understand the what simple class means. How is it going to look like? In any case, I am thinking about nuking nsFont::EnumerateFamilies. My patch already does (2), I think.


Going forward, here are my questions:

* can we safely nuke eCSSUnit_Families now?
* can we safely nuke nsFont::EnumerateFamilies, nsFont::GetGenericID and such?
* should we change nsFont.name to be an array?

Comment 10

[Kang-Hao (Kenny) Lu [:kennyluck]]

By the way, I never ever get all the tests run without failure on my local machine (Mac OSX), so I'll appreciate if someone can vouch for my access to the Try Server (bug 758178).

Comment 11

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

I'd think we'd probably want to keep eCSSUnit_Families and change what eCSSUnit_Families means we have in terms of a data structure (see comment 6), and I'd have expected that data structure to end up being part of nsFont and being used for eCSSUnit_Families.

It looks like these patches are changing only the storage of specified values and not of computed values, whereas my intent with the bug was to change both, and change them to the same thing.

Comment 12

[Kang-Hao (Kenny) Lu [:kennyluck]]

(In reply to David Baron [:dbaron] from comment #11)
> I'd think we'd probably want to keep eCSSUnit_Families and change what
> eCSSUnit_Families means we have in terms of a data structure (see comment
> 6), and I'd have expected that data structure to end up being part of nsFont
> and being used for eCSSUnit_Families.

I didn't really understand (1) of comment 6 (see comment 9) but am I correct what you are asking for is something similar to nscolor? While I think that's doable, if I understand correctly, nsFont currently passes the serialized font-family to the backend system so we have to either (1) change nsFont so that it passes structured data instead (2) move font-family serialization to nsFont. (2) is in particular messy as the serialization does escaping for double quotes and such (we probably should add a test with double quote in the font name).

> It looks like these patches are changing only the storage of specified
> values and not of computed values, whereas my intent with the bug was to
> change both, and change them to the same thing.

I can try it over the weekend but it seems to require a lot more code than my current patches.

Comment 13

[Kang-Hao (Kenny) Lu [:kennyluck]]

(In reply to Kang-Hao (Kenny) Lu [:kennyluck] from comment #12)
> if I understand correctly, nsFont currently passes the serialized font-family
> to the backend system so we have to either (1) change nsFont so that it passes
> structured data instead

It turns out this *backend* isn't as far as I thought. It's gfxFontGroup::ForEachFontInternal, which pretty much duplicates the codes in nsFont::EnumerateFamilies, with the exception of parsing the 'font-family'-related preferences. So this adds

4. 'font-family' preferences from gfxFontGroup::ForEachFontInternal

to comment 9.

So I think (1) is indeed doable. Various font-family passing functions would need to change it's signature, though, and there are many :(

Whether we should parse 1. ~ 4. using the CSS parser remains an issue. This kind of scenario seems to happen with color parsing before (bug 488649), but I haven't read through it.

Comment 14

[John Daggett (:jtd)]

I think it would be good idea to hold off on making changes to font-family parsing for a few days, as we're trying to resolve on clear wording to make the behavior clear.  This may or may not the impact the work here.

Comment 15

[Kang-Hao (Kenny) Lu [:kennyluck]]

Attachment: patch 1, v2 - create a new structure for storing 'familiy-name' and use it throught the code base. (WIP)

This patch compiles and runs on my machine, passes the test in the attachment as well as bug 660397, but I think it's still not an acceptable one. Requesting feedback/comment to see if I am on the right directions.

So I think this patch is closer to what's described in Comment 6 except that I nuked nsFont::EnumerateFamilies on the way of making changes to various codes relying on the fact that nsFont::name is a nsString. I can keep it but it seems to be quite pointless to do so. It is replaced with an iterator in my patch.

I sort of copied the pattern of the structure nsStyleQuotes for nsFont::Families. I don't know if there's serious performance regression with this approach. Any recommendation on how this can be profiled is much appreciated.

I try to keep all logic in various codes that access nsFont.name unchanged, but I'd like to note that nsRuleNode::SetFont appends an extra <generic-family> to every 'font-family' that's not a single <generic-family>, which seems quite weird to me. Can we (1) move this logic to /gfx/ (is nsPresContext accessible there?) or (2) don't append an extra <generic-family> when there's already one (e.g. "Times, serif" shouldn't get appended to "Times, serif, serif"). I noticed that there's a comment about this too (i.e., "// XXXldb Do we want to extract the generic for this if it's not only a generic?"), but I am not sure whether and how this changes the codepath used by nsRuleNode::SetGenericFont, where aFont.mGenericID is replied upon.

(In reply to John Daggett (:jtd) from comment #14)
> I think it would be good idea to hold off on making changes to font-family
> parsing for a few days, as we're trying to resolve on clear wording to make
> the behavior clear.  This may or may not the impact the work here.

What worries me instead is serialization as described in CSSOM and CSS 2.1, which says that the computed value of <font-family> is a string but this wasn't testable without window.getComputedStyle. IE doesn't treat Helvetica as "Helvetiva" and I am kind of afraid that changing this is breaking sites.

Comment 16

[Kang-Hao (Kenny) Lu [:kennyluck]]

Attachment: kenny's original patch, v3 - create a new structure for storing 'familiy-name' and use it throught the code base. (WIP)

Forgot to remove commented out function in gfxFont.cpp.

Comment 17

[John Daggett (:jtd)]

There's a separate bug 660397 for handling the escaping of quotes but since it
releates to serialization in general I thought I'd comment on this here.

There is by no means a standard way of serializing font family
names, current user agents each have slight variations in the 
serialized form.

Tested on OSX 10.8, except IE10 tested on Windows 8

Testcase:
  http://people.mozilla.org/~jdaggett/tests/font-family-serialization.html

Firefox Nightly (20121106)
  unquoted? preserve
  unquoted with spaces? preserve
  quoted? preserve
  leading escapes? not handled
  escaped quotes/commas/semi-colon's? not handled
  distinguish generics from quoted versions? yes, quoting preserved

Opera 12
  unquoted? double quoted
  unquoted with spaces? double quoted
  quoted? double quoted
  leading escapes? quoted
  escaped quotes/commas/semi-colon's? not handled
  distinguish generics from quoted versions? yes, double quoted

IE10
  unquoted? preserve
  unquoted with spaces? preserve
  quoted? double quoted
  leading escapes? not handled
  escaped quotes/commas/semi-colon's? not handled
  distinguish generics from quoted versions? yes, double quoted

Webkit Nightly (r133576)
  unquoted? preserve (if no spaces)
  unquoted with spaces? single quoted
  quoted? single quoted
  leading escapes? single quoted
  escaped quotes/commas/semi-colon's? handled, single quoted with escaped single quotes
  distinguish generics? no, unquoted (oops!)

Differences where the serialized form does not match the original form:

unquoted?
  font-family: unquoted;
  ==> font-family: "unquoted"; /* Opera */

unquoted with spaces?
  font-family: unquoted name;
  ==> font-family: "unquoted name"; /* Opera */
  ==> font-family: 'unquoted name'; /* Webkit */

quoted?
  font-family: 'quoted', "also quoted";
  ==> font-family: "quoted", "also quoted"; /* Opera, IE10 */
  ==> font-family: quoted, 'also quoted';   /* Webkit */

leading escapes?
  font-family: \35unquoted;
  ==> font-family: 5unquoted;   /* Firefox, IE10 - incorrect */
  ==> font-family: "5unquoted"; /* Opera */
  ==> font-family: '5unquoted'; /* Webkit */

escaped quotes/commas/semi-colon's?
  font-family: comma\,-unquoted
  ==> font-family: comma,unquoted;   /* Firefox, IE10 - incorrect */
  ==> font-family: "comma,unquoted"; /* Opera */
  ==> font-family: 'comma,unquoted'; /* Webkit */

distinguish generics from quoted versions?
  font-family: serif, 'serif';
  ==> font-family: serif, "serif";  /* Opera, IE10 */
  ==> font-family: serif, serif;    /* Webkit - incorrect */

The Webkit way of handling escapes by always quoting them seems reasonable to me, although double quoting them rather than single quoting makes more sense I think.

Comment 18

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

Comment on attachment 628041 [details] [diff] [review]
kenny's original patch, v3 - create a new structure for storing 'familiy-name' and use it throught the code base. (WIP)

Sorry for taking so long to respond here.

I like the idea here better, though I think this patch is trying to do too much refactoring in a single patch.  That's dangerous because it makes the patch harder to review and makes regression hunting harder.  In particular, the "last generic" concept (which actually seems like a change that would break stuff since we seem to use the first generic today), the introduction of Iteration in addition to enumeration, and the refactoring of DontUseDocumentFonts probably belong in separate patches.  I'd like to see this patch cover only the conversion of the storage from string to array and the reimplementation of nsFont::EnumerateFamilies (and equivalents, if there are other methods that do the same, which wouldn't surprise me in the current gfx code) on top of that array-like data structure.

But I'd be happy to see this move forward.  (Sorry again for not responding sooner.)

Comment 19

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

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #18)
> In particular,
> the "last generic" concept (which actually seems like a change that would
> break stuff since we seem to use the first generic today)

Er, sorry, never mind that -- I see what you're doing there now -- making a storage optimization for the common case.

> , the introduction
> of Iteration in addition to enumeration, and the refactoring of
> DontUseDocumentFonts probably belong in separate patches.

I still think these belong in separate patches -- though feel free to explain if I'm missing something here.

> I'd like to see
> this patch cover only the conversion of the storage from string to array and
> the reimplementation of nsFont::EnumerateFamilies (and equivalents, if there
> are other methods that do the same, which wouldn't surprise me in the
> current gfx code) on top of that array-like data structure.

The other method I was thinking of was probably gfxFontGroup::ForEachFontInternal (mentioned above).

Comment 20

[John Daggett (:jtd)]

Kenny, if you don't mind, I'm going to take a crack at streamlining this code.

Comment 21

[John Daggett (:jtd)]

Attachment: part 1, fontlist struct for font family lists (wip)

Comment 22

[John Daggett (:jtd)]

Attachment: part 2, parse font families into fontlist struct (wip)

This is an incremental patch that only implements the parsing and style portion. It still serializes the string into nsFont.name.  The next step is to switch nsFont to using the fontlist struct and eliminate instances of fontlist parsing in gfx and mathml code, along with nsFont::EnumerateFamilies.

Comment 23

[John Daggett (:jtd)]

Attachment: part 1 v2, fontlist struct for font family lists

Basic structure used to represent fontlists. Include ability to distinguish between quoted and unquoted names and a default generic to represent that trailing "serif" that is currently appended to all fontlists.

Comment 24

[John Daggett (:jtd)]

Attachment: part 2 v2, parse font families into fontlist struct

Modify the CSS parser to use fontlist structs instead of strings to store fontlists. To allow testing at this stage, code within nsRuleNode simply serializes the font family string into nsFont.name.

Comment 25

[John Daggett (:jtd)]

Attachment: part 3a v1, switch nsFont to use fontlist struct

Comment 26

[John Daggett (:jtd)]

Attachment: part 3b v1, workaround MathML fontlist manipulation

The MathML code in nsMathMLChar is doing some fairly wild fontlist manipulation. This patch simply does what is needed to make things work without doing the badly needed refactoring of this code (see bug 1009582).

Comment 27

[John Daggett (:jtd)]

Attachment: part 3c v1, rework the logic of ForEachFont

For non-Linux platforms, BuildFontList calls the ForEachFont to resolve the contents of the fontlist into an array of gfxFont objects, including generics.  I unraveled all the function pointer and typeless context usage and came up with something that's much easier to follow and understand I think.  It should mimic existing behavior.

In the Linux case, gfxPangoFontGroup is doing its own special thing with fontconfig.  I'm going to fix this is another patch, after testing to make sure all tests pass for Windows/OSX/mobile.  I'm also going to trim out ResolveFontName, which is basically unneeded, and move the substitution handling on Windows into FindFamily for the GDI & DirectWrite platform font handling classes.

Comment 28

[John Daggett (:jtd)]

In addition to the gfxPangoFontGroup fixups, I'm also going to put in code that properly handles font family names containing escaped idents. This is basically never needed in actual usage but I think it should be fairly simple to address.  Hopefully, I can figure out how to do this in the parser, and simply mark the name as requiring quotes, since this will be simpler to implement I think.

  /* unquoted names must be idents, so preceding numbers must be escaped */
  font-family: \1familyname, another family name;

This would end up serialized as:

  font-family: "1familyname",another family name;

Comment 29

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

Did you mean to request reviews on any of this, or is it not ready yet?

Comment 30

[John Daggett (:jtd)]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #29)
> Did you mean to request reviews on any of this, or is it not ready yet?

Nope, not just yet, working through test failures and reworking the font loading logic for the Linux/fontconfig case (e.g. gfxPangoFontGroup). Updated patches today or tomorrow.

Comment 31

[John Daggett (:jtd)]

Attachment: part 1 v3 - fontlist struct for font family lists

Fontlist struct for use with fontlists used in style, layout and gfx. Basic idea is to maintain the distinction between a named family (e.g. Arial) and generics (e.g. serif). This helps eliminate the re-parsing that's done in gfx and cleans up the font family name parsing withing style code.  Some of the serialization methods are needed for MathML, which is still doing some funky fontlist swizzling. Hopefully that can be eliminated in the future.

Comment 32

[John Daggett (:jtd)]

Attachment: part 2 v3 - parse font families into fontlist struct

Switch CSS parsing code to use the fontlist struct. When passing into nsFont structs the strings are serialized. Later patches eliminate this step. Also, later patches improve the escaping of family names.

Comment 33

[John Daggett (:jtd)]

Attachment: part 3a v2 - switch nsFont to use fontlist struct

Switch nsFont to use fontlist struct. The code on the patch will compile but won't be fully functional until later patches are applied.

Comment 34

[John Daggett (:jtd)]

Attachment: part 3c v3 - workaround MathML fontlist manipulation

Comment 35

[John Daggett (:jtd)]

Attachment: part 3b v1 - split apart fontlist parsing methods

Split apart simple parsing utility methods for dealing with pref-based fontlists.

Comment 36

[John Daggett (:jtd)]

Attachment: part 3d v2 - rework the fontlist iteration code in gfx

Simplify the code for initializing fontlists. Note that the next patch also does additional consolidation. The relatively complex structure of ForEachFontInternal makes it hard to follow the code flow and in some cases it hid the fact that certain methods are unnecessary (e.g. ResolveFontName).

Comment 37

[John Daggett (:jtd)]

Attachment: part 3e v2 - rework pango font group code to use fontlist struct

All platforms other than Linux use gfxFontGroup methods shared across platforms.  For now, rework the logic in gfxPangoFontGroup to use the new fontlist struct. The logic in the callback function FamilyCallback moves into the derived method FindPlatformFont.

Comment 38

[John Daggett (:jtd)]

Attachment: part 3f v1 - trim out ResolveFontName

The ResolveFontName method of gfxPlatform has effectively been replaced by the gfxPlatformFontList::FindFamily method. The existing logic was effectively calling FindFamily twice, once in ResolveFontName and then again to get the gfxFontFamily. On Linux it's never used. Moving the handling of substitute font handling for DirectWrite/GDI into FindFamily eliminates the need for this function.

Comment 39

[John Daggett (:jtd)]

Attachment: part 4 v2 - fixup escaping of font family names

Escape unquoted font family names properly. This will fix bug 660397.

Comment 40

[John Daggett (:jtd)]

(In reply to John Daggett (:jtd) from comment #34)
> Created attachment 8432298 [details] [diff] [review]
> part 3c v3 - workaround MathML fontlist manipulation

I should note here that I wasn't quite able to fix up all the reftest failures that resulted from using the new fontlist struct.  I tried to debug this a bit but the code is doing some relatively complicated and, I suspect, inefficient manipulation of the fontlist in an attempt to pick the ideal font for a given math character. For now, I'd like to just disable the failing tests and work on implementing this functionality in a more structured way, with gfx handling font fallback for math characters and the MathML only dealing with the actual layout operations. Bug 1009582 is intended to handle this.

Comment 41

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

Comment on attachment 8432295 [details] [diff] [review]
part 1 v3 - fontlist struct for font family lists

Review of attachment 8432295 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxFontFamilyList.h
@@ +11,5 @@
> +#include "nsString.h"
> +#include "nsTArray.h"
> +#include "mozilla/MemoryReporting.h"
> +
> +enum FontFamilyType {

Add a comment explaining what this is

@@ +30,5 @@
> +  eFamily_moz_variable,
> +  eFamily_moz_fixed
> +};
> +
> +struct FontFamilyName MOZ_FINAL {

Add a comment explaining what this is for

@@ +151,5 @@
> +operator==(const FontFamilyName& a, const FontFamilyName& b) {
> +    return a.mType == b.mType && a.mName == b.mName;
> +}
> +
> +class gfxFontFamilyList MOZ_FINAL {

Add a comment explaining what this is for.

Also, put everything in this file in the mozilla namespace and ditch the gfx prefix.

Comment 42

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

Comment on attachment 8432295 [details] [diff] [review]
part 1 v3 - fontlist struct for font family lists

Review of attachment 8432295 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxFontFamilyList.h
@@ +105,5 @@
> +        NS_ASSERTION(aType != eFamily_named && aType != eFamily_none,
> +                     "expected a generic font type");
> +        mName.Truncate();
> +        mType = aType;
> +    }

Actually instead of exposing this, how about making callers use the constructor version. That seems to lead to more idiomatic C++.

I'd kinda prefer a constructor instead of Assign(const nsString& aFamilyName, bool aQuoted) as well. Except instead of a bool parameter, use an enum with values QUOTED, UNQUOTED so call sites look nice. So,
    enum Quoted { UNQUOTED, QUOTED };
    FontFamilyName(const nsString& aFamilyName, Quoted aQuoted = UNQUOTED);

@@ +108,5 @@
> +        mType = aType;
> +    }
> +
> +    // helper method that converts generic names to the right enum value
> +    void Convert(const nsAString& aFamilyOrGenericName) {

Instead of an in-place conversion method, how about a static method that returns a new FontFamilyName?

Comment 43

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

Comment on attachment 8432295 [details] [diff] [review]
part 1 v3 - fontlist struct for font family lists

Review of attachment 8432295 [details] [diff] [review]:
-----------------------------------------------------------------

A few more questions :-)

::: gfx/thebes/gfxFontFamilyList.h
@@ +191,5 @@
> +    }
> +
> +    void Append(const FontFamilyName& aFamilyName) {
> +        mFontlist.AppendElement(aFamilyName);
> +    }

Wouldn't it be simpler to avoid all these overloads and just have a single Append method that takes a FontFamilyName?

@@ +208,5 @@
> +
> +    void Assign(const nsString& aFamilyName, bool aQuoted) {
> +        Clear();
> +        Append(aFamilyName, aQuoted);
> +    }

Wouldn't it be simpler to avoid having Assign and just have a single constructor taking a FontFamilyName?

@@ +217,5 @@
> +
> +    void SetGeneric(FontFamilyType aGenericType) {
> +        Clear();
> +        Append(aGenericType);
> +    }

Isn't this the same thing as Assign(FontFamilyType) above?

Comment 44

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

Comment on attachment 8432301 [details] [diff] [review]
part 3d v2 - rework the fontlist iteration code in gfx

Review of attachment 8432301 [details] [diff] [review]:
-----------------------------------------------------------------

Probably better to combine this patch with the patch that removes the code you moved here

::: gfx/thebes/gfxFont.cpp
@@ +5139,5 @@
>                          NS_ConvertUTF16toUTF8(families).get(),
> +                        (mFamilyList.GetDefaultFontType() == eFamily_serif ?
> +                         "serif" :
> +                         (mFamilyList.GetDefaultFontType() == eFamily_sans_serif ?
> +                          "sans-serif" : "none")),

Please please can we share this InitTextRun logging code?

Comment 45

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

Comment on attachment 8432302 [details] [diff] [review]
part 3e v2 - rework pango font group code to use fontlist struct

Review of attachment 8432302 [details] [diff] [review]:
-----------------------------------------------------------------

When you land this, you'll need to squash together the changesets that don't build independently.

Comment 46

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

Comment on attachment 8432303 [details] [diff] [review]
part 3f v1 - trim out ResolveFontName

Review of attachment 8432303 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxGDIFontList.cpp
@@ +837,5 @@
>  
>      return fe;
>  }
>  
> +gfxFontFamily* 

trailing whitespace

Comment 47

[John Daggett (:jtd)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #45)

> When you land this, you'll need to squash together the changesets that don't
> build independently.

Yeah, all the part3 patches will get squished to one patch on checkin.

Comment 48

[John Daggett (:jtd)]

Attachment: part 1 v3 - fontlist struct for font family lists

Updated based on review comments

Comment 49

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

Comment on attachment 8433097 [details] [diff] [review]
part 1 v3 - fontlist struct for font family lists

Review of attachment 8433097 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxFontFamilyList.h
@@ +15,5 @@
> +namespace mozilla {
> +
> +// type of font family name, either a name (e.g. Helvetica) or a
> +// generic (e.g. serif, sans-serif), with the ability to distinguish
> +// between unquoted and quoted names for serializaiton

use the /** ... */ doccomment style like our other classes/types do

Comment 50

[John Daggett (:jtd)]

Attachment: part 2 v4 - parse font families into fontlist struct

Rebased to take into account changes in part 1

Comment 51

[John Daggett (:jtd)]

Attachment: part 3c v4 - workaround MathML fontlist manipulation

Comment 52

[Frédéric Wang (:fredw)]

Comment on attachment 8433171 [details] [diff] [review]
part 3c v4 - workaround MathML fontlist manipulation

Review of attachment 8433171 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/reftests/mathml/reftest.list
@@ +152,5 @@
>  == whitespace-trim-2.html whitespace-trim-2-ref.html
>  == whitespace-trim-3.html whitespace-trim-3-ref.html
>  fails == whitespace-trim-4.html whitespace-trim-4-ref.html # Bug 787215
>  == whitespace-trim-5.html whitespace-trim-5-ref.html
> +random == opentype-stretchy.html opentype-stretchy-ref.html # disable on trunk for now

I didn't have time to check / test your patches, but I worry a bit that this one becomes random. The test is using a custom Web font and a font-family to force the math font, so it should not depend on what is installed on the machine and nor on the way the math fonts are picked.

Comment 53

[John Daggett (:jtd)]

Attachment: part 4 v3 - fixup escaping of font family names

Comment 54

[John Daggett (:jtd)]

(In reply to Frédéric Wang (:fredw) from comment #52)

> I didn't have time to check / test your patches, but I worry a bit that this
> one becomes random. The test is using a custom Web font and a font-family to
> force the math font, so it should not depend on what is installed on the
> machine and nor on the way the math fonts are picked.

I don't like it either but I wasn't able to track it down. The code here is attempting to replicate the functionality of the existing code but there's a *lot* of complexity in the code that's manipulating fontlists here that I think is probably unnecessary.  So I think we should temporarily mark this as random until we can track down what the problem is.  At some point we should also simplify all the fontlist manipulation that's being performed in this code, that's why I logged bug 1009582.

I did debug this for a day or so but ultimately gave up. I don't really understand what the stretchy reftest is testing so it's hard for me to grok what the correct result should be. I'd be happy to help track down the problem but right now I'm stuck because of my lack of knowledge of this code.

In the interests of getting this landed, I think we should just set the test to random for now.  Only a small portion of the test fails (the lower portion of the test) so another option would be to separate these into two tests and mark the lower portion one only as random.

Comment 55

[Frédéric Wang (:fredw)]

The test is verifying support for the stretchy features of the MATH table, that is whether the new OpenType MATH fonts will work. So if it is broken for Gecko 32 that it is a bit problematic to me because the plan is to ask people to move to these new fonts so that we can get rid of all the old codes and do the necessary optimization and clean up. If this is only changing the font selection behavior when MATH fonts are not installed, I don't really care (although perhaps Karl does)

I'd like to test your patches and see if the MathML code still works properly with the OpenType MATH fonts. I hope I'll have time to do that this week. Do you have a link to the reftest results? If that's only the LargeOpMinHeight square part, that's a less serious issue.

Alternatively, we could take it before the move to Aurora but we will have to backport the MathML patches to fix the failures. And I suspect the refactoring of the stretchy code will be a bit big.

Comment 56

[Frédéric Wang (:fredw)]

I don't see any obvious error in the patch. I don't know exactly the format difference between FontFamilyName/FontFamilyList VS the string font-family name and what are the conversions, so I can't really comment. I wanted to try the patches today to see how they affect the MathML support but I'm not able to build mozilla-central right now (and moreover the patches 1 & 2 didn't apply properly). I'll try again later.

I think the code could be simplified a bit if nsMathMLChar::StretchEnumContext::mFamilies becomes a FontFamilyList (I assume this is more or less equivalent to an array of FontFamilyName's) and similarly for the aFontFamily / aDefaultFamily parameters of

nsMathMLChar::StretchEnumContext::TryVariants
nsMathMLChar::StretchEnumContext::TryParts
nsMathMLChar::StretchEnumContext::EnumCallback
nsMathMLChar::SetFontFamily

What happens is that this family parameter is a singleton list in most cases, except when we have a generic font (or equivalently when we use the unicode table) for which we use the current CSS font-family + the fallbacks. Then the only places I see are:

* In NOISY_SEARCH, you can serialize the FontFamilyList or just ignored since that's not built anyway.

* In nsMathMLChar::StretchEnumContext::EnumCallback, aFamily should contain only one FontFamilyName when we arrive at the line "glyphTable = gGlyphTableList->GetGlyphTableFor(aFamily)" and so you can convert that FontFamilyName to a string using your AppendToString function.

* Similarly in nsMathMLChar::SetFontFamily, you will only need to check whether aFont.fontlist is a singleton containing "aGlyphTable->FontNameFor(aGlyphCode)" or whether it has the same FontFamilyName elements as the list aDefaultFamily. Then ConvertFamilies is no longer necessary: your new FontFamilyList will just be aDefaultFamily or the singleton containing "aGlyphTable->FontNameFor(aGlyphCode)". The nsGlyphTable::FontNameFor function returns the result of gfxFontEntry::FamilyName(), so I'm assuming you only need to call FontFamilyName::Convert on it.

Removing ConvertFamilies will avoid performance issues and possible bugs that we overlooked.

Comment 57

[John Daggett (:jtd)]

(In reply to Frédéric Wang (:fredw) from comment #55)
> I'd like to test your patches and see if the MathML code still works
> properly with the OpenType MATH fonts. I hope I'll have time to do that this
> week. Do you have a link to the reftest results? If that's only the
> LargeOpMinHeight square part, that's a less serious issue.

I think the simplest way to grab these patches is via my patch queue. I just updated this to the latest trunk code.

My patch queue for this bug is here:
ssh://hg.mozilla.org/users/jdaggett_mozilla.com/workinprogress

Latest tryserver run (with reftests marked as specified in patches):
https://tbpl.mozilla.org/?tree=Try&rev=a669b531bb7c

Comment 58

[John Daggett (:jtd)]

(In reply to Frédéric Wang (:fredw) from comment #56)
> I think the code could be simplified a bit if
> nsMathMLChar::StretchEnumContext::mFamilies becomes a FontFamilyList (I
> assume this is more or less equivalent to an array of FontFamilyName's) and
> similarly for the aFontFamily / aDefaultFamily parameters of
> 
> nsMathMLChar::StretchEnumContext::TryVariants
> nsMathMLChar::StretchEnumContext::TryParts
> nsMathMLChar::StretchEnumContext::EnumCallback
> nsMathMLChar::SetFontFamily

I started down that path and quickly got caught in trying to untangle all the sideeffects of this.  I'll take another crack at it and let you know what I come up with.

I'm somewhat hesitant to make big changes to this code because I think the fallback process should be streamlined. Doing more extensive morphing of the fontlist manipulation logic doesn't make sense if ultimately we should be instead pushing the fallback logic into gfx with sufficient richness that the MathML code can get what it needs out of it efficiently (i.e. not generating large numbers of font groups or textruns for a single given string of math characters).

Comment 59

[John Daggett (:jtd)]

Attachment: part 3c v5 - rework MathMLChar code to use fontlist struct

Updated based on review comments. Revised along the lines of comment 56, I eliminated the back-n-forth between fontlist structs and strings.  I needed to use the fontlist struct in nsGlyphTable but otherwise it wasn't too bad. I also chopped up the opentype-stretchy into two parts.  The lower of the two parts is the one that fails.  This appears to be related to the fallback behavior being ever-so-slightly different after implementing the use of a fontlist struct.

Comment 60

[John Daggett (:jtd)]

Example of inefficient MathMLChar textrun creation listed in bug 1009582, comment 5.

Comment 61

[Frédéric Wang (:fredw)]

Comment on attachment 8433861 [details] [diff] [review]
part 3c v5 - rework MathMLChar code to use fontlist struct

Review of attachment 8433861 [details] [diff] [review]:
-----------------------------------------------------------------

OK, that's essentially what I had in mind. (I didn't want to suggest changing the nsGlyphTable as I thought it would be too many modifications, but that was not that bad at the end). I don't know what NormalizeDefaultFont is doing?

Unfortunately, this seems to break the font selection for stretchy operators. If you go to

http://fred-wang.github.io/MathFonts/

you'll see that the operators (e.g. sums & integrals) always render the same whatever the font selected. The issue seems be due to the math fallback added. If you make empty the font.mathfont-family in about:config, you'll see that the selection is correctly done.

(I also see the bug with the version with local fonts at https://developer.mozilla.org/en-US/docs/Mozilla/MathML_Project/MathML_Torture_Test, so that's not related to web fonts)

Even when I empty the math fallback, I see the same issue with the old fonts "STIXGeneral" and "MathJax TeX". These are handled by the nsPropertiesTable and the name pass to LoadProperties should really match the mathfont*.properties files in layout/mathml/. nsPropertiesTable::ElementAt does not seem to print anything in the console in debug mode to say the properties file is loading...

::: layout/mathml/nsMathMLChar.cpp
@@ +1064,5 @@
>    const nscoord mTargetSize;
>    const uint32_t mStretchHint;
>    nsBoundingMetrics& mBoundingMetrics;
>    // Font families to search
> +  //const nsAString& mFamilies;

This one should be removed

Comment 62

[Frédéric Wang (:fredw)]

Comment on attachment 8433861 [details] [diff] [review]
part 3c v5 - rework MathMLChar code to use fontlist struct

Review of attachment 8433861 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/mathml/nsMathMLChar.cpp
@@ +1647,5 @@
> +    const nsTArray<FontFamilyName>& fontlist = font.fontlist.GetFontlist();
> +    uint32_t i, num = fontlist.Length();
> +    for (i = 0; i < num; i++) {
> +      const FontFamilyName& name = fontlist[i];
> +      StretchEnumContext::EnumCallback(name, name.IsGeneric(), &enumData);

OK, I just debugged this a bit. What's happening is that the old EnumerateFamilies function stops when StretchEnumContext::EnumCallback returns false, that is at the first font that has the appropriate glyphs to draw the stretchy char. With this change *all* the fonts are tested and so the last math font is always used.

So you only need to break when StretchEnumContext::EnumCallback returns false here. That seems to solve the main problem with the font selection for the fonts. I'll try to check what's happening with the nsPropertiesGlyph table now.

Comment 63

[Frédéric Wang (:fredw)]

(In reply to Frédéric Wang (:fredw) from comment #62)
> Comment on attachment 8433861 [details] [diff] [review]
> I'll try to check what's happening with the nsPropertiesGlyph table now.

So I tried to check the issue with the old fonts and I see too weird behaviors that I can't really explain:

1) For STIXGeneral, when doing openTypeTable = nsOpenTypeTable::Create(fontGroup->GetFontAt(0)) this returns an OpenType MATH table... but this font is not supposed to contain one table and indeed using Python fontforge.open("STIXGeneral-Regular.otf").math.exists()) returns false. So I don't understand how Gecko can ever find a table... If I try with "DejaVu Sans", the TryGetMathTable returns null as expected.

2) For MathJax_Main, when trying to set the font with nsMathMLChar::SetFontFamily, familyList indeed contains a single "MathJax_Main" element but firstFontList contains a single "MathJax_main" element (with the second M lowercase) so the equality firstFontList == familyList fails and the font is never set. Is it possible that FontFamilyName(firstFont->GetFontEntry()->FamilyName(), eUnquotedName) modifies the font name or something?

Comment 64

[Frédéric Wang (:fredw)]

Comment on attachment 8433861 [details] [diff] [review]
part 3c v5 - rework MathMLChar code to use fontlist struct

Review of attachment 8433861 [details] [diff] [review]:
-----------------------------------------------------------------

Please ignore comment 63... I had an old experimental MathML-fonts add-on installed that was redefining STIXGeneral and MathJax_Main...

Now, I'm able to make all the math fonts work as expected. Also, adding the break to the for loop seems enough to make the MathML tests pass: https://tbpl.mozilla.org/?tree=Try&rev=e4713e38a4e0

r=me with the review comments of comment 61 and comment 62 addressed and without the changes to the MathML tests.

Comment 65

[Frédéric Wang (:fredw)]

Comment on attachment 8433097 [details] [diff] [review]
part 1 v3 - fontlist struct for font family lists

Review of attachment 8433097 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxFontFamilyList.h
@@ +270,5 @@
> +        if (aIncludeDefault && mDefaultFontType != eFamily_none) {
> +            if (!aFamilyList.IsEmpty()) {
> +                aFamilyList.Append(',');
> +            }
> +            if (mDefaultFontType != eFamily_serif) {

Did you mean == here?

Comment 66

[Frédéric Wang (:fredw)]

Attachment: testcase MathML ; unquoted VS quoted (requires Asana Math)

Comment 67

[Frédéric Wang (:fredw)]

Comment on attachment 8433861 [details] [diff] [review]
part 3c v5 - rework MathMLChar code to use fontlist struct

Review of attachment 8433861 [details] [diff] [review]:
-----------------------------------------------------------------

After trying to work on bug 947654, I found another subtle bug. Attachment 8434213 [details] shows that the font-family of the stretchy char is ignored when the name is quoted. I think what's happening is that the comparaison firstFontList == familyList fails because firstFontList contains an unquoted font-family while familyList may contain a quoted font-family. A simple solution (not tested) would be to force the FontFamilyName passed to nsMathMLChar::StretchEnumContext::EnumCallback to be unquoted (either before the call or before constructing the local FontFamilyList family).

Comment 68

[John Daggett (:jtd)]

Attachment: part 5 - fixup problem with stretchy fallback

Force font family name in EnumCallback to be unquoted.

Comment 69

[John Daggett (:jtd)]

(In reply to Frédéric Wang (:fredw) from comment #65)
> Comment on attachment 8433097 [details] [diff] [review]
> ::: gfx/thebes/gfxFontFamilyList.h
> @@ +270,5 @@
> > +        if (aIncludeDefault && mDefaultFontType != eFamily_none) {
> > +            if (!aFamilyList.IsEmpty()) {
> > +                aFamilyList.Append(',');
> > +            }
> > +            if (mDefaultFontType != eFamily_serif) {
> 
> Did you mean == here?

Argh, yes.

Comment 70

[John Daggett (:jtd)]

Attachment: part 5a - fixup typo in comparison

Comment 71

[Cameron McCormack (:heycam)]

Comment on attachment 8433168 [details] [diff] [review]
part 2 v4 - parse font families into fontlist struct

Review of attachment 8433168 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with appropriate answers to some questions below and other comments addressed.

::: content/html/content/src/HTMLFontElement.cpp
@@ +64,5 @@
>        if (value && value->Type() == nsAttrValue::eString &&
>            !value->IsEmptyString()) {
> +        nsCSSParser parser;
> +        parser.ParseFontFamilyString(value->GetStringValue(),
> +                                     nullptr, 0, *family);

Looks like the behaviour here if we have an invalid font family list in the face="" attribute is changed, i.e. we will continue to look for a lower specificity rule to fill in a font-family value, rather than storing the invalid font family list that presumably will fall back to some default font when it's used.

I assume that's a good thing, now that it matches how size="" are treated.

::: layout/style/nsCSSParser.cpp
@@ +160,5 @@
>                           css::Declaration* aDeclaration,
>                           bool* aChanged,
>                           bool aIsImportant);
>  
> +  bool ParseFontFamilyString(const nsSubstring& aBuffer,

I'm not sure whether this should be called ParseFontFamilyListString.  That seems more accurate, but there are various places in nsCSSParser where we talk about "font family" instead of a "font family list".  (I guess in "ParseFontFamilyString" the "FontFamily" is really referring to the font-family property, whose value is a list.)

@@ +3340,5 @@
>      return false;
>    }
>  
>    // add family to rule
> +  const FontFamilyList *fontlist = fontlistValue.GetFontFamilyListValue();

"*" next to type.

@@ +9356,5 @@
>      // These four are similar to the properties of the same name,
>      // possibly with more restrictions on the values they can take.
>    case eCSSFontDesc_Family: {
> +    nsCSSValue value;
> +    if (!ParseFamily(value) ||

Why not call ParseOneFamily if we really only want one family name here?

@@ +11634,5 @@
> +static bool
> +AppendGeneric(nsCSSKeyword aKeyword, FontFamilyList *aFamilyList)
> +{
> +  switch (aKeyword) {
> +      case eCSSKeyword_serif:

Shift the cases back by two spaces.

@@ +11692,5 @@
> +          return true;
> +        }
> +        break;
> +      case eCSSKeyword__moz_use_system_font:
> +        if(!IsParsingCompoundProperty()) {

Space after "if".

@@ +11794,5 @@
> +      // reject generics
> +      if (single) {
> +        nsCSSKeyword keyword = nsCSSKeywords::LookupKeyword(family);
> +        switch (keyword) {
> +            case eCSSKeyword_serif:

Shift the cases back by two spaces.

@@ +11806,5 @@
> +              break;
> +        }
> +      }
> +
> +      cur.SetStringValue(family, eCSSUnit_Local_Font);

Is it OK that we don't store whether the font family inside the local() was quoted?

::: layout/style/nsCSSRules.cpp
@@ +1977,5 @@
>    aOutStr.Append(familyListStr);
>    aOutStr.AppendLiteral(" {\n");
>    FeatureValuesToString(aFeatureValues, valueTextStr);
>    aOutStr.Append(valueTextStr);
> +  aOutStr.AppendLiteral("}");

Why replace the Append('}') call?

::: layout/style/nsCSSValue.cpp
@@ +849,5 @@
>      GetStringValue(buffer);
>      if (unit == eCSSUnit_String) {
>        nsStyleUtil::AppendEscapedCSSString(buffer, aResult);
> +    }
> +    else {

"else {" on previous line.

::: layout/style/nsRuleNode.cpp
@@ +3335,5 @@
>  #endif
>      }
>    }
>  
>    // font-family: string list, enum, inherit

s/string/font family/

@@ +3904,5 @@
>    // the optimization with a non-null aStartStruct?
>    const nsCSSValue* familyValue = aRuleData->ValueForFontFamily();
> +  if (eCSSUnit_FontFamilyList == familyValue->GetUnit()) {
> +    font->mFont.name.Truncate();
> +    const FontFamilyList *fontlist = familyValue->GetFontFamilyListValue();

"*" next to type.

@@ +3947,5 @@
> +          generic = kGenericFont_monospace;
> +          break;
> +        case eFamily_moz_fixed:
> +          font->mFont.name.AssignLiteral("-moz-fixed");
> +          generic = kGenericFont_moz_fixed;

Missing break.

::: layout/style/nsStyleSet.cpp
@@ +1659,1 @@
>          nsAutoString silly(family);

(Is this nsAutoString really needed btw?)

::: layout/style/nsStyleUtil.cpp
@@ +149,5 @@
> +  nsAString& aResult)
> +{
> +  const nsTArray<FontFamilyName>& fontlist = aFamilyList.GetFontlist();
> +  uint32_t len = fontlist.Length();
> +  for (uint32_t i = 0; i < len; i++) {

nsTArray indices are now size_t rather than uint32_t, so please use that for len and i.

@@ +151,5 @@
> +  const nsTArray<FontFamilyName>& fontlist = aFamilyList.GetFontlist();
> +  uint32_t len = fontlist.Length();
> +  for (uint32_t i = 0; i < len; i++) {
> +    if (i != 0) {
> +      aResult.Append(',');

Any reason not to append ", "?

Comment 72

[Cameron McCormack (:heycam)]

Comment on attachment 8433185 [details] [diff] [review]
part 4 v3 - fixup escaping of font family names

Review of attachment 8433185 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsStyleUtil.cpp
@@ +142,5 @@
>    }
>    return true;
>  }
>  
> +// unquoted family names much be a sequence of idents

s/much/must/

@@ +156,5 @@
> +  aFamilyName.EndReading(p_end);
> +
> +   bool moreThanOne = false;
> +   while (p < p_end) {
> +      const char16_t *identStart = p;

Unindent this block by one space.

@@ +157,5 @@
> +
> +   bool moreThanOne = false;
> +   while (p < p_end) {
> +      const char16_t *identStart = p;
> +      while (++p != p_end && *p != kSpace)

I think writing "*p != ' '" should be OK here.

@@ +158,5 @@
> +   bool moreThanOne = false;
> +   while (p < p_end) {
> +      const char16_t *identStart = p;
> +      while (++p != p_end && *p != kSpace)
> +      /* nothing */ ;

Indent the empty statement by two spaces.

@@ +162,5 @@
> +      /* nothing */ ;
> +
> +      // pull out a single ident and clean out leading/trailing whitespace
> +      ident = Substring(identStart, p);
> +      ident.CompressWhitespace(true, true);

Why trim trailing spaces?  Don't we know that p is pointing just past a non-space character?  Also, why not advance identStart at the beginning of the loop while we have space characters?  That would let us have a pointer to the real start of the ident, and we could pass in the result of Substring directly to AppendEscapedCSSIdent, rather than having to copy and manipulate the string into |ident|.

@@ +165,5 @@
> +      ident = Substring(identStart, p);
> +      ident.CompressWhitespace(true, true);
> +
> +      if (!ident.IsEmpty()) {
> +          if (moreThanOne) {

Unindent this block by two spaces.

::: layout/style/test/test_font_family_parsing.html
@@ +54,5 @@
>    { namelist: "arial, helvetica, sans-serif" },
>    { namelist: "arial, helvetica, 'times' new roman, sans-serif", invalid: true },
>    { namelist: "arial, helvetica, \"times\" new roman, sans-serif", invalid: true },
>  
>    // bug 660397 - quotes contained within family names are not escaped

Can this comment be removed now (and the bug resolved) after these patches?

@@ +70,5 @@
>    /* escapes */
>    { namelist: "\\s imple", single: true },
>    { namelist: "\\073 imple", single: true },
>  
>    // bug 475216 - css serialization doesn't escape characters that need escaping

What about this one?

@@ -83,5 @@
> -  // { namelist: "\\,\\;", single: true },
> -  // { namelist: "\\{", single: true },
> -  // { namelist: "\\{\\;", single: true },
> -  // { namelist: "\\}", single: true },
> -  // { namelist: "\\}\\;", single: true },

Why do these tests with the escaped symbols not work?  Or are they invalid?

Comment 73

[John Daggett (:jtd)]

Attachment: part 4 v4 - fixup escaping of font family names

(In reply to Cameron McCormack (:heycam) from comment #72)

> @@ -83,5 @@
> > -  // { namelist: "\\,\\;", single: true },
> > -  // { namelist: "\\{", single: true },
> > -  // { namelist: "\\{\\;", single: true },
> > -  // { namelist: "\\}", single: true },
> > -  // { namelist: "\\}\\;", single: true },
> 
> Why do these tests with the escaped symbols not work?  Or are they invalid?

I don't think these are valid idents, so I don't think they are valid as part of a font family name. Other browsers spit out syntax errors for these also.

Comment 74

[John Daggett (:jtd)]

(In reply to Cameron McCormack (:heycam) from comment #71)

> ::: content/html/content/src/HTMLFontElement.cpp
> @@ +64,5 @@
> >        if (value && value->Type() == nsAttrValue::eString &&
> >            !value->IsEmptyString()) {
> > +        nsCSSParser parser;
> > +        parser.ParseFontFamilyString(value->GetStringValue(),
> > +                                     nullptr, 0, *family);
> 
> Looks like the behaviour here if we have an invalid font family list in the
> face="" attribute is changed, i.e. we will continue to look for a lower
> specificity rule to fill in a font-family value, rather than storing the
> invalid font family list that presumably will fall back to some default font
> when it's used.
> 
> I assume that's a good thing, now that it matches how size="" are treated.

Right, this is simply enforcing consistency. Can't really say the spec demands this or allows this since this is such an ancient attribute.

> ::: layout/style/nsCSSParser.cpp
> @@ +9356,5 @@
> >      // These four are similar to the properties of the same name,
> >      // possibly with more restrictions on the values they can take.
> >    case eCSSFontDesc_Family: {
> > +    nsCSSValue value;
> > +    if (!ParseFamily(value) ||
> 
> Why not call ParseOneFamily if we really only want one family name here?

ParseOneFamily is basically a low-level reading routine, it doesn't pull out the distinctions between keywords, generics or named families.  ParseFamily does that so it's easier to verify that the parsed name isn't a disallowed keyword (e.g. default or inherit) or a generic (e.g. serif).

> @@ +11806,5 @@
> > +              break;
> > +        }
> > +      }
> > +
> > +      cur.SetStringValue(family, eCSSUnit_Local_Font);
> 
> Is it OK that we don't store whether the font family inside the local() was
> quoted?

Hmmm, looking over the code I think it would probably make sense to use ParseFamily here also, for the same reasons outlined above.  But that's not really central to this bug, I'll create a follow-on bug for that. We need to preserve quoting of local font names, since there's a semantic difference between "serif" and serif.

> @@ +151,5 @@
> > +  const nsTArray<FontFamilyName>& fontlist = aFamilyList.GetFontlist();
> > +  uint32_t len = fontlist.Length();
> > +  for (uint32_t i = 0; i < len; i++) {
> > +    if (i != 0) {
> > +      aResult.Append(',');
> 
> Any reason not to append ", "?

Existing code doesn't include the space so I was just mimicing that.  Webkit adds a space after the comma and quotes family names containing spaces. The CSS OM spec has a definition of font family serialization but... It forces everything to be quoted. Only the old Opera browser ever implemented it.

Comment 75

[Cameron McCormack (:heycam)]

(In reply to John Daggett (:jtd) from comment #73)
> > @@ -83,5 @@
> > > -  // { namelist: "\\,\\;", single: true },
> > > -  // { namelist: "\\{", single: true },
> > > -  // { namelist: "\\{\\;", single: true },
> > > -  // { namelist: "\\}", single: true },
> > > -  // { namelist: "\\}\\;", single: true },
> > 
> > Why do these tests with the escaped symbols not work?  Or are they invalid?
> 
> I don't think these are valid idents, so I don't think they are valid as
> part of a font family name. Other browsers spit out syntax errors for these
> also.

I think they are, according to css-syntax.  See the "\" case in http://dev.w3.org/csswg/css-syntax/#consume-a-token.  Please file a followup for this (although I am surprised that doesn't work already).

Comment 76

[Cameron McCormack (:heycam)]

Comment on attachment 8435502 [details] [diff] [review]
part 4 v4 - fixup escaping of font family names

Review of attachment 8435502 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed.

::: layout/style/nsStyleUtil.cpp
@@ +147,5 @@
> +// so escape any parts that require escaping
> +static void
> +AppendUnquotedFamilyName(const nsAString& aFamilyName, nsAString& aResult)
> +{
> +  nsAutoString ident;

Please make this an nsDependentString so that we don't copy the identifier.

@@ +154,5 @@
> +  aFamilyName.EndReading(p_end);
> +
> +   bool moreThanOne = false;
> +   while (p < p_end) {
> +     const char16_t *identStart = p;

"*" next to type.

Comment 77

[John Daggett (:jtd)]

Pushed to inbound, with changes based on review comments, subparts of part 3 merged:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c079286e7288
https://hg.mozilla.org/integration/mozilla-inbound/rev/014f475ca0ec
https://hg.mozilla.org/integration/mozilla-inbound/rev/e39cfafa8517
https://hg.mozilla.org/integration/mozilla-inbound/rev/a73b48626bb5
https://hg.mozilla.org/integration/mozilla-inbound/rev/3af2e9f88905

Comment 78

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c079286e7288
https://hg.mozilla.org/mozilla-central/rev/014f475ca0ec
https://hg.mozilla.org/mozilla-central/rev/e39cfafa8517
https://hg.mozilla.org/mozilla-central/rev/a73b48626bb5
https://hg.mozilla.org/mozilla-central/rev/3af2e9f88905

Comment 79

[Philip Chee]

This caused a regression in Thunderbird and SeaMonkey:

Bug 1022481 - message pane: Monospace font setting is ignored instead the setting for proportional font is used if browser.display.use_document_fonts is set to 0