Closed Bug 663740 Opened 13 years ago Closed 10 years ago

migrate nsMathMLChar measuring and drawing from nsRenderingContext to gfx/thebes classes

Categories

(Core :: MathML, defect, P5)

defect

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: karlt, Assigned: fredw)

References

Details

Attachments

(1 file, 16 obsolete files)

70.62 KB, patch
karlt
: review+
Details | Diff | Splinter Review
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.
Blocks: 407059
No longer blocks: 400938
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?
OS: Linux → All
Hardware: x86_64 → All
(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.
Feel free to remove the "Composite"/child char code too.
That might be a good first step.
Assignee: nobody → cazacu.daniell
Status: NEW → ASSIGNED
> 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".
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.
(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.
Assignee: cazacu.daniell → nobody
The use of gfxPlatform::ResolveFontName() is preventing the use of any mathfontNAME.properties files except for mathfontUnicode.properties with downloaded fonts.
Status: ASSIGNED → NEW
Blocks: 732830
Blocks: 121148
Assignee: nobody → francoiswang
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.
Status: NEW → ASSIGNED
No longer blocks: 732830
Assignee: francoiswang → nobody
Status: ASSIGNED → NEW
Depends on: 781494
Attached patch Patch V1 (obsolete) — Splinter Review
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...
Attachment #653943 - Flags: feedback?(karlt)
(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.
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.
Attachment #653943 - Flags: feedback?(karlt)
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.
Assignee: nobody → fred.wang
Status: NEW → ASSIGNED
Attached patch Patch V2 (obsolete) — Splinter Review
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.
Attachment #653943 - Attachment is obsolete: true
Attachment #669193 - Flags: feedback?(karlt)
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?
Attachment #669193 - Flags: feedback?(karlt)
Attached patch Patch V3 (obsolete) — Splinter Review
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?
Attachment #669193 - Attachment is obsolete: true
Assignee: fred.wang → nobody
Status: ASSIGNED → NEW
Priority: -- → P5
Whiteboard: [mentor=fredw][lang=c++]
Attached patch Patch V3 (rebased) (obsolete) — Splinter Review
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.
(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.
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.
Yet another rebase.
Attachment #819369 - Attachment is obsolete: true
Attachment #670382 - Attachment is obsolete: true
Attachment #8340749 - Attachment description: Patch V3 (rebased) → Part 1 - use gfx/thebes classes for measuring/drawing glyphs v3
Attachment #8340749 - Attachment filename: 0001-Bug-663740-migrate-nsMathMLChar-measuring-and-drawin.patch → 663740-1.patch
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.
Attached patch patch - v4 (obsolete) — Splinter Review
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?
Flags: needinfo?(karlt)
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.
Flags: needinfo?(karlt)
Attached patch Patch V5 (obsolete) — Splinter Review
Attachment #8340749 - Attachment is obsolete: true
Attachment #8341290 - Attachment is obsolete: true
Attached patch Patch V6 (obsolete) — Splinter Review
Attachment #8341645 - Attachment is obsolete: true
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().
Attached patch Patch v7 (obsolete) — Splinter Review
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.
Attachment #8341943 - Attachment is obsolete: true
Attachment #8341945 - Attachment is obsolete: true
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.
Attached patch Patch V8 (obsolete) — Splinter Review
Attachment #8342254 - Attachment is obsolete: true
Assignee: nobody → fred.wang
Attached patch Patch V9 (obsolete) — Splinter Review
Attachment #8343927 - Attachment is obsolete: true
Attachment #8344396 - Flags: review?(karlt)
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.
Attached patch Patch V10 (obsolete) — Splinter Review
Refreshing the patch to fix a warning-as-error + a crash due to the font not set on the nsRenderingContext.
Attachment #8344396 - Attachment is obsolete: true
Attachment #8344396 - Flags: review?(karlt)
Attachment #8345135 - Flags: review?(karlt)
Attached patch Patch V11 (obsolete) — Splinter Review
Refreshing patch.
Attachment #8345135 - Attachment is obsolete: true
Attachment #8345135 - Flags: review?(karlt)
Attachment #8359268 - Flags: review?(karlt)
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.
(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).
(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
(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 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
  };
Attachment #8359268 - Flags: review?(karlt) → review-
(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?
(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.
Attached patch Patch V12 (obsolete) — Splinter Review
Attachment #8359268 - Attachment is obsolete: true
Attachment #8373615 - Flags: feedback?(karlt)
Whiteboard: [mentor=fredw][lang=c++]
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.
Attachment #8373615 - Flags: feedback?(karlt) → review+
(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
Attached patch Patch V13 (obsolete) — Splinter Review
Attachment #8373615 - Attachment is obsolete: true
Attachment #8374986 - Flags: review?(karlt)
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.
Attachment #8374986 - Flags: review?(karlt)
Attachment #8374986 - Flags: review-
Attachment #8374986 - Flags: feedback+
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.
Attached patch Patch V14Splinter Review
Attachment #8374986 - Attachment is obsolete: true
Attachment #8375348 - Flags: review?(karlt)
Attachment #8375348 - Flags: review?(karlt) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ea9b012bfe3e
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Depends on: 973322
Depends on: 1194497
You need to log in before you can comment on or make changes to this bug.