Last Comment Bug 663740 - migrate nsMathMLChar measuring and drawing from nsRenderingContext to gfx/thebes classes
: migrate nsMathMLChar measuring and drawing from nsRenderingContext to gfx/the...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: MathML (show other bugs)
: Trunk
: All All
: P5 normal with 1 vote (vote)
: mozilla30
Assigned To: Frédéric Wang (:fredw)
:
Mentors:
Depends on: 781494 973322 1194497
Blocks: 121148 407059
  Show dependency treegraph
 
Reported: 2011-06-12 21:18 PDT by Karl Tomlinson (:karlt)
Modified: 2015-08-13 15:32 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch V1 (10.31 KB, patch)
2012-08-21 14:15 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Patch V2 (43.02 KB, patch)
2012-10-08 10:20 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Patch V3 (44.44 KB, patch)
2012-10-11 07:14 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Patch V3 (rebased) (45.07 KB, patch)
2013-10-19 11:02 PDT, Khaled Hosny
no flags Details | Diff | Splinter Review
Part 1 - use gfx/thebes classes for measuring/drawing glyphs v3 (35.95 KB, patch)
2013-12-01 03:20 PST, Khaled Hosny
no flags Details | Diff | Splinter Review
patch - v4 (52.37 KB, patch)
2013-12-02 14:08 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Patch V5 (52.58 KB, patch)
2013-12-03 03:07 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Patch V6 (55.35 KB, patch)
2013-12-03 13:06 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Allow nsGlyphCode to store glyphID. (5.76 KB, patch)
2013-12-03 13:07 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Patch v7 (57.47 KB, patch)
2013-12-04 02:00 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Patch V8 (55.22 KB, patch)
2013-12-06 12:13 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Patch V9 (55.75 KB, patch)
2013-12-08 13:55 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Patch V10 (56.79 KB, patch)
2013-12-09 22:24 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Patch V11 (56.78 KB, patch)
2014-01-13 08:21 PST, Frédéric Wang (:fredw)
karlt: review-
Details | Diff | Splinter Review
Patch V12 (56.15 KB, patch)
2014-02-10 13:35 PST, Frédéric Wang (:fredw)
karlt: review+
Details | Diff | Splinter Review
Patch V13 (67.33 KB, patch)
2014-02-12 10:54 PST, Frédéric Wang (:fredw)
karlt: review-
karlt: feedback+
Details | Diff | Splinter Review
Patch V14 (70.62 KB, patch)
2014-02-12 22:58 PST, Frédéric Wang (:fredw)
karlt: review+
Details | Diff | Splinter Review

Description Karl Tomlinson (:karlt) 2011-06-12 21:18:24 PDT
Currently nsMathMLChar uses GetBoundingMetrics and DrawString methods
of the older nsRenderingContext.  These are high level methods that operate
with Unicode character input.  Most of the time, this works OK for our
mathfontUnicode.properties database of Unicode character-part support and
mathfontFONTNAME.properties database of PUA and supplementary font support of
other stretchy characters.

For bug 407059, however, we'll need methods based on glyph indices (instead
of Unicode characters).  For this, the newer gfxTextRun/gfxFont/gfxContext
classes would be appropriate.

The newer classes already support Unicode characters through gfxFontGroup, and
the nsRenderingContext methods above are simply wrappers around these newer
classes, so it would seem sensible to also use these classes (instead of
nsRenderingContext) in nsMathMLChar even for Unicode-based lookup.

The newer classes are also better for testing existence of a glyph in a
particular font, which is useful at least when using
mathfontUnicode.properties, because there all glyphs in characters built from
parts should come from the same font.

The gfxFont class provides methods for getting the font tables necessary for
bug 407059.
Comment 2 Frédéric Wang (:fredw) 2011-06-13 13:16:39 PDT
I think it would be best if the work on this bug is done on top of bug 407439, which already has a patch reviewed.

> // A table consists of "nsGlyphCode"s which are viewed either as Unicode
> // points or as direct glyph indices, depending on the type of the table.
> // XXX The latter is not yet supported.

Is the plan to make nsGlyphCode use only glyph indices or to allow a mix of Unicode points and glyph indices?

Also, should we move to the newer classes in one go or is it worth doing the work in several steps. For example starting to remove GetBoundingMetrics and DrawString?
Comment 3 Karl Tomlinson (:karlt) 2011-06-13 16:08:12 PDT
(In reply to comment #2)
> Is the plan to make nsGlyphCode use only glyph indices or to allow a mix of
> Unicode points and glyph indices?

For fonts where we continue to use mathfontFONTNAME.properties files we'll want to continue to lookup by Unicode point because glyph indices are likely to change between versions of fonts.  Glyph indices will only be useful when they come from information in that font (i.e. the CMAP or MATH table).

It may be possible to lookup Unicode points to get glyph indices store glyph indices in nsGlyphCode, but I don't know whether there is any advantage in doing that.  I wouldn't make that change unless there is a clear advantage in doing so.

> Also, should we move to the newer classes in one go or is it worth doing the
> work in several steps. For example starting to remove GetBoundingMetrics and
> DrawString?

As I see it, removing uses of nsRenderingContext's GetBoundingMetrics and DrawString will be pretty much all the work.  The public nsMathMLChar methods may well continue to have an nsRenderingContext parameter, but aRenderingContext->ThebesContext() will be used to get a gfxContext for drawing.

I'm not concerned about the "#ifdef SHOW_BORDERS" code.  That can either be removed or changed to use gfxContext.
Comment 4 Karl Tomlinson (:karlt) 2011-06-13 16:10:17 PDT
Feel free to remove the "Composite"/child char code too.
That might be a good first step.
Comment 5 Frédéric Wang (:fredw) 2011-07-01 14:41:37 PDT
> I'm not concerned about the "#ifdef SHOW_BORDERS" code.  That can either be
> removed or changed to use gfxContext.

I proposed to Daniel to create a first patch removing the "#ifdef SHOW_BORDERS".
Comment 6 Zack Weinberg (:zwol) 2011-08-27 09:31:06 PDT
I'm not going to mark a dependency, but whoever actually does this bug, please have a look at bug 651016 and its dependencies and see if some of that work makes sense to do as a side-effect.
Comment 7 Frédéric Wang (:fredw) 2012-02-04 03:26:43 PST
(In reply to Karl Tomlinson (:karlt) from comment #4)
> Feel free to remove the "Composite"/child char code too.
> That might be a good first step.

For people who want to work on this bug: MathJax fonts require composite char for (bug 701758 comment 4), so finally we might want to keep this code.
Comment 8 Karl Tomlinson (:karlt) 2012-02-28 14:14:08 PST
The use of gfxPlatform::ResolveFontName() is preventing the use of any mathfontNAME.properties files except for mathfontUnicode.properties with downloaded fonts.
Comment 9 Karl Tomlinson (:karlt) 2012-03-13 16:20:01 PDT
A key difference between the nsRenderingContext/nsFontMetrics API and
nsMathMLChar's needs is that nsRenderingContext's concept of a font is really
a CSS font specification, which might include several families and will fall
back to other families if none of the specified families match the particular
glyph.  nsMathMLChar wants to work with a particular font and know that it is
that font only that will be used.

gfxFont provides a font-specific interface.  A gfxFont can be obtained from a
gfxFontGroup (which corresponds to nsFontMetrics) via GetFontAt().
GetFontEntry()->FamilyName() will indicate whether the first font in the font
group actually matches the font family requested.  It looks like gfxTextRun
(which is not-necessarily font-specific) is still needed to draw an measure
text.
Comment 10 Frédéric Wang (:fredw) 2012-08-21 14:15:52 PDT
Created attachment 653943 [details] [diff] [review]
Patch V1

Just an experimental patch (may not be quite correct or may be subject to memory leak) to discuss the way to fix this bug. I think we can use such a DrawGlyph method (and similarly GetGlyphBoundingMetrics). Later the aGlyph parameter could be either a unicode character or a glyph index. I think that we'll still need DrawString and GetBoundingMetrics on mData (which may contain several characters) when "mDrawNormal" is true...
Comment 11 Karl Tomlinson (:karlt) 2012-08-23 18:51:15 PDT
(In reply to Frédéric Wang (:fredw) from comment #10)
> I think that
> we'll still need DrawString and GetBoundingMetrics on mData (which may
> contain several characters) when "mDrawNormal" is true...

I wonder whether there is much point in using nsMathMLChar if there are
multiple characters.  If not, perhaps GlyphCode::code could be copied from
nsMathMLChar::mString, to remove the need for an mDrawNormal case.
Comment 12 Karl Tomlinson (:karlt) 2012-08-23 18:53:09 PDT
A complication here is that the graphics team are working on a new API,
project name Azure, to replace gfxContext.  The new API is currently only
functional on some platforms.  It is close to being usable on all platforms,
but currently only canvas code is using the new API, and the API does not look
complete for other purposes.
http://mxr.mozilla.org/mozilla-central/source/gfx/2d/2D.h?force=1

At this stage, the new API provides only for drawing, not measuring text, so
canvas is still using gfxFontGroup for font selection and gfxTextRun for
laying out text.  It is then copying glyphs ids and positions across to the
newer GlyphBuffer format.  I assume at some stage there will be a better way
to setting up glyph info for the new API.
http://hg.mozilla.org/mozilla-central/annotate/3c39442f9f19/content/canvas/src/nsCanvasRenderingContext2DAzure.cpp#l3046

Given these changes are happening, I don't think there's much point
rearranging the code here too much if the only motivation is moving away from using nsRenderingContext to a different text drawing API.

That is not the only motivation.  This is what I see we want to gain:

1. A way to access font tables and get glyph ids.

   This is currently gfxFont, and glyph ids can be put into a gfxTextRun to
   Draw.  gfxTextRun is higher level than needed here because each string here
   will use only one font, but gfxTextRun is designed to fallback to other
   fonts for missing characters.

2. When using mathfontUnicode.properties, a way to check that the same
   font is used for each part.
  
   Currently, getting the gfxFonts from gfxTextRun will tell us that.

However, I expect some of this will change in the future, though I don't know
when.  It is at least months away.

Thinking about bug 407059, I think we are going to want to fetch glyph id's
during stretch and then store them, perhaps with positions, in the
nsMathMLChar for drawing.  The new gfx::GlyphBuffer looks most compatible with
this.

I'm beginning to think, despite what I said in comment 3, that it may be best
to store glyph ids and positions in the nsMathMLChar, during stretch, even
when using .properties files.  That way we won't need two different code paths
at drawing.  (If measuring of the parts can be shared too, even better.)  It
will also be more efficient.  By the time we've checked that each part is
using the same font, we'll have much of this information.
Comment 13 Frédéric Wang (:fredw) 2012-10-06 13:11:47 PDT
So if I understand correctly, the plan for this bug would be:

- Do not wait for the Azure API to be ready.
- Modify TryParts to verify that all the glyphs come from the same font.
- Modify the stretch code to directly convert the nsGlyphCode's to a gfx::GlyphBuffer (with glyph ids and perhaps positions) that we would store on nsMathMLChar.
- Modify the rendering code to display the glyphs in gfx::GlyphBuffer via a gfxTextRun.

Then in bug 407059 we could create a gfx::GlyphBuffer from the infos contained in the MATH table.

I'm taking this bug and will start to work on this as soon as I have more free time.
Comment 14 Frédéric Wang (:fredw) 2012-10-08 10:20:17 PDT
Created attachment 669193 [details] [diff] [review]
Patch V2

This patch removes the use of nsRenderingContext. It is still experimental, but I'm just asking feedback to be sure that the approach is correct. Some remarks:

1) the mDrawNormal that allowed several characters is actually used with "centered" operators (+, - etc), invisible operators and when the stretch is not necessary.  I removed it but I haven't really verified if everything still work correctly in that case (in particular these characters used the parent style context). I suspect that the "centered" and "invisible" operators are not really necessary with modern fonts and could be removed.

2) I haven't worked on the previous suggestions yet. I think parts should always come from the same font so it is probably possible to store the nsFontMetrics on the nsMathMLChar instead of recomputing it each time. The stretch code could also directly create a gfx::GlyphBuffer instead of using mGlyphTable, TopOf, etc. Regarding the positions of glyphs, I'm wondering what to do with these AppUnitsPerDevPixel conversions.

3) I see memory leaks. I haven't verified but I suspect it is due to the nsFontMetrics.

4) Perhaps the gfxContext operations are not exactly the same as those that was done before. For example RTL or conversions involving AppUnitsPerDevPixel.
Comment 15 Frédéric Wang (:fredw) 2012-10-09 14:14:06 PDT
Two questions:

- What is the best way to get the glyph id nsGlyphCode::code? The functions in gfxFont take uint32 parameter but I'm note sure it is a good idea to convert the PRUnichar* to an integer. Otherwise we can do as in the canvas example mentioned in comment 12 and use a temporary gfxTextRun created from the glyph code.

- When we have a list of fonts (when we draw the character "normally" or when we use a variant from the unicode table) can we just set the glyph id to the unicode code point?
Comment 16 Frédéric Wang (:fredw) 2012-10-11 07:14:09 PDT
Created attachment 670382 [details] [diff] [review]
Patch V3

So here is an updated patch (fixing memory leaks, BTW). Ideally, I would like to do all the computation in the Stretch routine, so

  nsGlyphTable*      mGlyphTable;
  nsGlyphCode        mGlyph;
  nsString           mFamily;

could be replaced by

  nsRefPtr<nsFontMetrics> mFontMetrics;
  gfx::GlyphBuffer*       mGlyphBuffer;

where mGlyphBuffer contains one or more glyphs to draw and mFontMetrics is shared by all the glyphs. I'm not sure whether we should repeat the glue in mGlyphBuffer. If not, we can just use mGlyphBuffer[4] instead but in that case I don't think we can do all the positioning in the Stretch routine. Perhaps that's what I'm going to do to start with.

However, some contructions use parts from different fonts. For example \u21D1 = \u21D1@1\uFFFD\uFFFD\uE10E in STIXNonUnicode. I suspect that in most cases and in particular with OpenType fonts, all the parts come from the same fonts. But for the moment, it seems that we'll have to keep storing mGlyphTable and the font number of each glyph (nsGlyphCode::font) in mGlyphBuffer so that we can change the font used to draw each glyph (and in that case, maybe it's not relevant to store mFontMetrics on the nsMathMLChar). Or maybe we can also remove these constructions that rely on different fonts?
Comment 17 Khaled Hosny 2013-10-19 11:02:36 PDT
Created attachment 819369 [details] [diff] [review]
Patch V3 (rebased)

Here is a rebased version of Frédéric’s last patch.

I also looked into switching to using glyph id instead of characters, but there are several issues:

* gfx::GlyphBuffer() is internal to gfxFont.cpp, and in general I can’t find any glyph-based thebes API.
* An API to map characters to glyph indices is needed, there is gfxFont:GetGlyph() but it is not implemented for all font backends. The HarfBuzz shaper seems to have fallback code that I think we can move to a common place so that it is used when font backends don’t override it.
Comment 18 Karl Tomlinson (:karlt) 2013-10-22 19:46:05 PDT
(In reply to Khaled Hosny from comment #17)
> * gfx::GlyphBuffer() is internal to gfxFont.cpp, and in general I can’t find
> any glyph-based thebes API.

gfxShapedText gfxShapedWord and gfxTextRun are the public structures for storing glyph indices and positions.  Only gfxTextRun seems to support measuring the text.

If measuring is not required, then the GlyphBuffer in 2D.h may be an option.

> * An API to map characters to glyph indices is needed, there is
> gfxFont:GetGlyph() but it is not implemented for all font backends. The
> HarfBuzz shaper seems to have fallback code that I think we can move to a
> common place so that it is used when font backends don’t override it.

It may be necessary to add a generic gfxFont::GetGlyph() method for OpenType fonts that uses the CMAP table.

The alternative, I guess, is to use the APIs to shape a string of Unicode characters, with only a single character, and then retrieve the glyph id from the resulting gfxShapedText, or derived class.
Comment 19 Karl Tomlinson (:karlt) 2013-10-22 20:01:09 PDT
The gfxShapedText family of classes can be initialized from glyph ids and positions using SetGlyphs().  Perhaps CompressedGlyph::SetComplex() with DetailedGlyph will be necessary if there are vertical offsets, but it will be needed even if IsSimpleAdvance() returns false, so SetSimpleGlyph() is only an optimization, that is not necessary here.

With gfxTextRun, AddGlyphRun() is also required.

I think the idea was that there would be a single DetailedGlyph per unicode
character, but each DetailedGlyph can only contain glyphs from a single
gfxFont, so it may be necessary to add some bogus characters to the
gfxTextRun in some situations.  The aLength parameter for the gfxTextRun constructor would be at least the number of fonts required, or perhaps the number of DetailedGlyphs for the simplest implementation.
Comment 20 Khaled Hosny 2013-12-01 03:20:18 PST
Created attachment 8340749 [details] [diff] [review]
Part 1 - use gfx/thebes classes for measuring/drawing glyphs v3

Yet another rebase.
Comment 21 Frédéric Wang (:fredw) 2013-12-02 09:01:20 PST
So I came back to this today. As Karl said above, it's probably best to store the glyph ids and metrics on the mMathMLChar. The call to nsMathMLChar::MeasureGlyph in the stretch code will be replaced by something that checks whether a glyph exists and returns the ID + metrics. The nsMathMLChar::DrawGlyph and nsMathMLChar::MeasureGlyph in the draw code will just use the saved data. I'll try to write a new patch this week.
Comment 22 Frédéric Wang (:fredw) 2013-12-02 14:08:57 PST
Created attachment 8341290 [details] [diff] [review]
patch - v4

Here is a tentative patch to store the glyph ID and metrics on the frame. However, I'm not sure how to use the gfx* classes. 

Karl, can you have a quick look at nsMathMLChar::GetGlyphData and give more hints about how to extract the glyphID from the textrun?
Comment 23 Karl Tomlinson (:karlt) 2013-12-02 20:37:43 PST
See http://hg.mozilla.org/mozilla-central/annotate/c93cfe704487/gfx/thebes/gfxFont.cpp#l2455 for an example to follow that gets glyph ids from the run.
They are stored in a kind of compressed format, so check IsSimpleGlyph() first to find out where the id is.
Comment 24 Frédéric Wang (:fredw) 2013-12-03 03:07:15 PST
Created attachment 8341645 [details] [diff] [review]
Patch V5
Comment 25 Frédéric Wang (:fredw) 2013-12-03 13:06:18 PST
Created attachment 8341943 [details] [diff] [review]
Patch V6
Comment 26 Frédéric Wang (:fredw) 2013-12-03 13:07:11 PST
Created attachment 8341945 [details] [diff] [review]
Allow nsGlyphCode to store glyphID.
Comment 27 Frédéric Wang (:fredw) 2013-12-03 13:09:35 PST
The patches should now allow to handle glyph by glyph index. I see a weird rendering with the vertical bar when the STIX fonts are used, tough.

Some investigations done today:

- we will probably need to rewrite the code to allow the more general GlyphAssembly table.

- gfxFontEntry has some functions to access the Open Type tables like gfxFontEntry::GetFontTable. Probably a class similar to nsGlyphTable to retrieve info from the Open Type table should be implemented (perhaps two classes nsGlyphTableFromProperties and nsGlyphTableFromOpenTypeMath deriving from the same base class) 

- The XeTeX code is
  https://github.com/khaledhosny/xetex/blob/master/source/texk/web2c/xetexdir/MathTable.h
  https://github.com/khaledhosny/xetex/blob/master/source/texk/web2c/xetexdir/XeTeXOTMath.cpp
  https://github.com/khaledhosny/xetex/blob/master/source/texk/web2c/xetexdir/XeTeXOTMath.h
  https://github.com/khaledhosny/xetex/blob/master/source/texk/web2c/xetexdir/XeTeXswap.h (it seems that mozilla::NativeEndian can do that)
  I'm wondering if it's worth implementing the logic on the gfx/thebes side, like what is done for gfxSVGGlyphs.

- I think the appropriate place to plug the Open Type search is in StretchEnumContext::EnumCallback ; do the SetFontFamily call before setting glyphTable and then try to get a glyphTable from context->mChar->mFontMetrics->GetThebesFontGroup()->GetFontAt(0)->GetFontEntry().
Comment 28 Frédéric Wang (:fredw) 2013-12-04 02:00:38 PST
Created attachment 8342254 [details] [diff] [review]
Patch v7

I've merged the patch that allows nsGlyphCode to store either a glyph index or a Unicode code point. To fix the rendering bug mentioned above, I kept drawing by Unicode code point for the mathfontUnicode.properties. For the other tables, a conversion from Unicode code point to glyph index is performed. This is probably not really useful at the moment, but at least it allows to check that the draw-by-glyph-index code is working as expected.
Comment 29 Frédéric Wang (:fredw) 2013-12-04 11:03:17 PST
For the record, I've started some of the work indicated in comment 27 today. I'll keep the latest version of the patches on my GitHub MozillaCentralPatches repository.
Comment 30 Frédéric Wang (:fredw) 2013-12-06 12:13:56 PST
Created attachment 8343927 [details] [diff] [review]
Patch V8
Comment 31 Frédéric Wang (:fredw) 2013-12-08 13:55:23 PST
Created attachment 8344396 [details] [diff] [review]
Patch V9
Comment 32 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-12-08 14:57:28 PST
See comments in 407059.
Comment 33 Frédéric Wang (:fredw) 2013-12-09 10:23:59 PST
Comment on attachment 8344396 [details] [diff] [review]
Patch V9

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

> -      aRenderingContext.SetFont(fm);

aRenderingContext.SetFont(mFontMetrics); should still be called before the Stretch/MaxWidth exit. At least the msqrt code assumes the font metrics to be set by the stretchy code. I'll update the patch later.
Comment 34 Frédéric Wang (:fredw) 2013-12-09 22:24:20 PST
Created attachment 8345135 [details] [diff] [review]
Patch V10

Refreshing the patch to fix a warning-as-error + a crash due to the font not set on the nsRenderingContext.
Comment 35 Frédéric Wang (:fredw) 2014-01-13 08:21:01 PST
Created attachment 8359268 [details] [diff] [review]
Patch V11

Refreshing patch.
Comment 36 Karl Tomlinson (:karlt) 2014-01-31 00:05:24 PST
Looks like storing the metrics between stretch and draw is working out well.

I think we should be able to share more code in getting the bounding metrics,
but I'll to look at what you have in mind for IsGlyphID GlyphCodes.
At a minimum we should be able to share the code converting from bounding box
to bounding metrics, or perhaps mBmData could be bounding boxes instead of
bounding metrics because individual widths are not necessary (and maybe that
can be a separate future improvement).

I'll have to finish this review next week. 

(In reply to Frédéric Wang (:fredw) from comment #33)
> aRenderingContext.SetFont(mFontMetrics); should still be called before the
> Stretch/MaxWidth exit. At least the msqrt code assumes the font metrics to
> be set by the stretchy code. I'll update the patch later.

Which code is assuming that?
It may be better to change that code.
Comment 37 Frédéric Wang (:fredw) 2014-01-31 00:20:03 PST
(In reply to Karl Tomlinson (:karlt) from comment #36)
> Looks like storing the metrics between stretch and draw is working out well.
> 
> I think we should be able to share more code in getting the bounding metrics,
> but I'll to look at what you have in mind for IsGlyphID GlyphCodes.
> At a minimum we should be able to share the code converting from bounding box
> to bounding metrics, or perhaps mBmData could be bounding boxes instead of
> bounding metrics because individual widths are not necessary (and maybe that
> can be a separate future improvement).
> 
> I'll have to finish this review next week. 
> 

The latest version of the patch for bug 407059 is:
https://github.com/fred-wang/MozillaCentralPatches/blob/master/407059-2.diff

I also have a WIP patch for the mirroring that changes a bit things:
https://github.com/fred-wang/MozillaCentralPatches/blob/master/945183.diff

> (In reply to Frédéric Wang (:fredw) from comment #33)
> > aRenderingContext.SetFont(mFontMetrics); should still be called before the
> > Stretch/MaxWidth exit. At least the msqrt code assumes the font metrics to
> > be set by the stretchy code. I'll update the patch later.
> 
> Which code is assuming that?
> It may be better to change that code.

I don't remember exactly and can see that from the code. I think it was in nsMathMLmenclose, where a crash happens in some tests without that (removing aRenderingContext.SetFont(mFontMetrics) should allow to remember that).
Comment 38 Frédéric Wang (:fredw) 2014-02-03 10:22:49 PST
(In reply to Frédéric Wang (:fredw) from comment #37)
> > Which code is assuming that?
> > It may be better to change that code.
> 
> I don't remember exactly and can see that from the code. I think it was in
> nsMathMLmenclose, where a crash happens in some tests without that (removing
> aRenderingContext.SetFont(mFontMetrics) should allow to remember that).

https://tbpl.mozilla.org/?tree=Try&rev=c2f154c787de
So I think that was at least https://hg.mozilla.org/mozilla-central/file/067123df4f77/layout/mathml/nsMathMLmrootFrame.cpp#l378
Comment 39 Karl Tomlinson (:karlt) 2014-02-06 18:47:11 PST
(In reply to Frédéric Wang (:fredw) from comment #38)
> So I think that was at least
> https://hg.mozilla.org/mozilla-central/file/067123df4f77/layout/mathml/
> nsMathMLmrootFrame.cpp#l378

I wonder whether that is using a different nsFontMetrics than Reflow().
I don't think it was intentional that it got the metrics from the nsMathMLChar.

I think the GetIntrinsicWidthMetrics was getting the font metrics from the
rendering context because that looked similar to what Reflow() did before
https://hg.mozilla.org/mozilla-central/rev/dda49a4f9e4c#l13.1

But still, it looks like I wrote that code wrong, because it was meant to
match Reflow().
Comment 40 Karl Tomlinson (:karlt) 2014-02-06 18:50:58 PST
Comment on attachment 8359268 [details] [diff] [review]
Patch V11

I would like to avoid having mFontMetrics on the nsMathMLChar.  This is
essentially being used as a way to pass a parameter between SetFontFamily() or
GetMetricsFor() and MakeTextRun(), but it would be better to be explicit about
that.

This can be done by using an nsRefPtr<nsFontMetrics>* out parameter for
SetFontFamily().  Probably better would be nsRefPtr<gfxFontGroup>*.  The only
additional thing that nsFontMetrics provides over gfxFontGroup is
AppUnitsPerDevPixel() but that is available from an nsPresContext or
nsDeviceContext.

If mGlyphs on the nsMathMLChar is replaced with a gfxTextRun for each part,
then the draw methods will not need to use SetFontFamily, the nsMathMLChar
will not need to keep mGlyphTable nor mFamily, and PaintForeground can use the
same code for DRAW_NORMAL and DRAW_VARIANT.

If a MakeTextRun method is added to the glyph table, other code won't need to
know about the glyph storage, and nsGlyphCodes can be opaque pointers or ids
(that the table can handle).  Even OpenType MATH glyph tables should be able
to construct the gfxTextRun so that the same measuring code can be used for
all text runs.

It should be fine to pass a null property provider AFAICS.

Some code can be shared by having a helper function to measure text of a
gfxTextRun and return nsBoundingMetrics.

>+    aPresContext->DeviceContext()->GetMetricsFor(font,
>+                                                 mStyleContext->StyleFont()->
>+                                                 mLanguage,
>+                                                 mStyleContext->PresContext()->
>+                                                 GetUserFontSet(),
>+                                                 mStyleContext->PresContext()->
>+                                                 GetTextPerfMetrics(),
>+                                                 *getter_AddRefs(fm));

This has fewer line breaks, making it easier to read:

    aPresContext->DeviceContext()->
      GetMetricsFor(font,
                    mStyleContext->StyleFont()->mLanguage,
                    mStyleContext->PresContext()->GetUserFontSet(),
                    mStyleContext->PresContext()->GetTextPerfMetrics(),
                    *getter_AddRefs(fm));

but I assume mStyleContext->PresContext() is aPresContext. 

> // Update the font and rendering context if there is a family change

Please update this comment.

>-    mDrawNormal = !glyphFound;
>+    if (!glyphFound) mDraw = DRAW_NORMAL;

This is no longer necessary, I assume because Stretch() sets DRAW_NORMAL.

>-    mCtx.IntersectClip(aRect);

>+    mThebesContext->NewPath();
>+    gfxRect clip(NSAppUnitsToFloatPixels(aRect.x, aAppUnitsPerGfxUnit),
>+                 NSAppUnitsToFloatPixels(aRect.y, aAppUnitsPerGfxUnit),
>+                 NSAppUnitsToFloatPixels(aRect.width, aAppUnitsPerGfxUnit),
>+                 NSAppUnitsToFloatPixels(aRect.height, aAppUnitsPerGfxUnit));

nsLayoutUtils::RectToGfxRect()

Similarly in PaintRule.

>+    mThebesContext->Rectangle(clip);

Use SnappedRectangle(clip) as having clean edges hid the joins in the parts.

>+PaintRule(gfxContext* aThebesContext,
>+          int32_t     aAppUnitsPerGfxUnit,
>+          nsRect&     aRect)
>+{
>+  if (!aRect.IsEmpty()) {

I don't think this test is necessary.

>+    aThebesContext->Rectangle(rect, true);

SnappedRectangle() is more explicit than the boolean parameter. 

>-        if (!ch.Exists())
>-          continue; // no middle

It looks like this may be a change in behavior.
ComputeSizeFromParts() ignores the size of glue, so that a missing middle does
not need anything to be drawn.  However storing the size of the glue for
middle in mBmData and not skipping the middle in drawing will force drawing of
glue for the middle part.

>+    return (other.font == font && other.IsGlyphID() == IsGlyphID() &&
>+            ((IsGlyphID() && other.glyphID == glyphID) ||
>+             (!IsGlyphID() && other.code[0] == code[0] &&
>+              other.code[1] == code[1])));

No need for "other.IsGlyphID() == IsGlyphID()" as this is covered by comparing
the font members.

>+  typedef enum {
>+    DRAW_NORMAL, DRAW_VARIANT, DRAW_PARTS
>+  } DrawingMethod;

In C++, this is usually

  enum DrawingMethod {
    DRAW_NORMAL, DRAW_VARIANT, DRAW_PARTS
  };
Comment 41 Frédéric Wang (:fredw) 2014-02-07 10:10:16 PST
(In reply to Karl Tomlinson (:karlt) from comment #40)
> >-        if (!ch.Exists())
> >-          continue; // no middle
> 
> It looks like this may be a change in behavior.
> ComputeSizeFromParts() ignores the size of glue, so that a missing middle
> does
> not need anything to be drawn.  However storing the size of the glue for
> middle in mBmData and not skipping the middle in drawing will force drawing
> of
> glue for the middle part.

Yes, I think that was the most convenient way to handle this without changing to much code (eventually this should be rewritten to support arbitrary number of part). I'm not such how many constructions will be affected... What do you suggest?
Comment 42 Karl Tomlinson (:karlt) 2014-02-09 12:04:03 PST
(In reply to Frédéric Wang (:fredw) from comment #41)
> What do you suggest?

I guess the simplest approach would be to leave missing parts empty (instead of setting them to glue) and leave their bounding metrics at zero.

Just check that the bars in
<mrow><mo>&VerticalLine;</mo><mi>&CenterDot;</mi><mo>&VerticalLine;</mo></mrow>
still have finite size.
Comment 43 Frédéric Wang (:fredw) 2014-02-10 13:35:13 PST
Created attachment 8373615 [details] [diff] [review]
Patch V12
Comment 44 Frédéric Wang (:fredw) 2014-02-11 13:26:36 PST
https://tbpl.mozilla.org/?tree=Try&rev=77b03bcb483e
Comment 45 Karl Tomlinson (:karlt) 2014-02-11 18:28:49 PST
Comment on attachment 8373615 [details] [diff] [review]
Patch V12

This all looks great, thanks.  I think this should be landed with just a couple of minor changes below.

>+      mGlyphs[0]->Draw(thebesContext, gfxPoint(r.x, r.y + mUnscaledAscent),

This can be gfxPoint(0.0, mUnscaledAscent), because the transform has been
applied to make r.TopLeft() zero, and I think that would be clearer.

>+nsMathMLChar::PaintVertically(nsPresContext* aPresContext,
>+                              gfxContext*    aThebesContext,
>+                              nsFont&        aFont,
>+                              nsGlyphTable*  aGlyphTable,
>+                              nsRect&        aRect)
> {
>   // Get the device pixel size in the vertical direction.
>   // (This makes no effort to optimize for non-translation transformations.)
>   nscoord oneDevPixel = aPresContext->AppUnitsPerDevPixel();
>+  nsRefPtr<gfxFontGroup> fontGroup;

This variable is no longer needed.  Similarly in PaintHorizontally.

>-  if (! family.Equals(aFont.name)) {
>+  if (!*aFontGroup || !family.Equals(aFont.name)) {

I wonder what required this.
It looks sensible and probably means some other aFont.name code can be
removed, but leave that for another time.
Comment 46 Frédéric Wang (:fredw) 2014-02-11 23:27:12 PST
https://hg.mozilla.org/try/rev/e383b42c650a
Comment 47 Frédéric Wang (:fredw) 2014-02-12 01:48:24 PST
(In reply to Frédéric Wang (:fredw) from comment #46)
> https://hg.mozilla.org/try/rev/e383b42c650a
I've modified semantics-1-ref.xhtml and that seems to make the test pass again now.

I checked the painting code and indeed the remaining use of the Glyph Table was really only for the glue, which we don't need to handle specially anymore. So I've cleanup the code even further and now the painting only works on the textruns. I have also simplified the GlyphTable API and that will be more convenient for the changes with the MATH table:
https://tbpl.mozilla.org/?tree=Try&rev=3b372e6747b4
Comment 48 Frédéric Wang (:fredw) 2014-02-12 09:42:04 PST
https://tbpl.mozilla.org/?tree=Try&rev=6de2d73b4986
Comment 49 Frédéric Wang (:fredw) 2014-02-12 10:54:57 PST
Created attachment 8374986 [details] [diff] [review]
Patch V13
Comment 50 Karl Tomlinson (:karlt) 2014-02-12 13:05:27 PST
Comment on attachment 8374986 [details] [diff] [review]
Patch V13

Removing all this code is excellent.  Usually it is easier to review changes in separate patches, but this wasn't too bad as interdiff happened to work ok.

The only issue I see is from this code now having additional effect on the missing middle glyph.

>     // _cairo_scaled_font_glyph_device_extents rounds outwards to the nearest
>     // pixel, so the bm values can include 1 row of faint pixels on each edge.
>     // Don't rely on this pixel as it can look like a gap.
>     start[i] = dy - bm.ascent + oneDevPixel; // top join
>     end[i] = dy + bm.descent - oneDevPixel; // bottom join

The middle missing glyph has zero metrics, and so the oneDevPixel will make the glues on either side overlap, which can make the join visible.

Can you handle the missing glyph specially or avoid adding oneDevPixel when bm.ascent + bm.descent < 2 * oneDevPixel, please?
Similarly for horizontally.
Comment 51 Karl Tomlinson (:karlt) 2014-02-12 13:21:07 PST
I think the semantics-1 failure at
https://tbpl.mozilla.org/?tree=Try&rev=77b03bcb483e
is likely due to different bounding metrics in the operator built from
horizontal parts.

Now that the missing parts are not filled with glue their metrics have ascent
and descent of zero.  For U+00AF, descent should be negative.

Can you adjust this code to compute ascent and descent only from existing
glyphs (or non-empty metrics), please?
http://hg.mozilla.org/mozilla-central/annotate/d8d8fa98ee7d/layout/mathml/nsMathMLChar.cpp#l1208

Having an initial loop to find the first non-empty glyph may be the simplest
approach.
Comment 52 Frédéric Wang (:fredw) 2014-02-12 22:58:40 PST
Created attachment 8375348 [details] [diff] [review]
Patch V14
Comment 53 Ryan VanderMeulen [:RyanVM] 2014-02-13 06:56:59 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea9b012bfe3e
Comment 54 Wes Kocher (:KWierso) 2014-02-13 17:01:00 PST
https://hg.mozilla.org/mozilla-central/rev/ea9b012bfe3e
Comment 55 Jacek Caban 2014-02-25 03:49:24 PST
I landed a trivial mingw fixup:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c4e8fca7cc5
Comment 56 Ryan VanderMeulen [:RyanVM] 2014-02-25 12:16:17 PST
https://hg.mozilla.org/mozilla-central/rev/5c4e8fca7cc5

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