Bug 663740 - migrate nsMathMLChar measuring and drawing from nsRenderingContext to gfx/thebes classes
 Summary: migrate nsMathMLChar measuring and drawing from nsRenderingContext to gfx/the...
 Status: RESOLVED FIXED Core Components MathML (show other bugs) Trunk All All P5 normal with 1 vote (vote) mozilla30 Frédéric Wang (:fredw) 781494 973322 1194497 121148 407059 Show dependency tree / graph

Reported: 2011-06-12 21:18 PDT by Karl Tomlinson (:karlt)
Modified: 2015-08-13 15:32 PDT (History)
13 users (show)
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

 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. Frédéric Wang (:fredw) 2011-06-13 00:35:58 PDT nsMathMLChar: http://mxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLChar.h http://mxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLChar.cpp nsRenderingContext: http://mxr.mozilla.org/mozilla-central/source/gfx/src/nsRenderingContext.h http://mxr.mozilla.org/mozilla-central/source/gfx/src/nsRenderingContext.cpp gfxTextRun: http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxFont.h#1357 gfxFont: http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxFont.h#879 gfxContent: http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxContext.h#55 gfxFontGroup: http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxFont.h#2253 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? 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. 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. 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". 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. 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. 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. 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. 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... 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. 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. 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. 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. 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? 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 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? 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. 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. 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. 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. 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. 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? 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. Frédéric Wang (:fredw) 2013-12-03 03:07:15 PST Created attachment 8341645 [details] [diff] [review] Patch V5 Frédéric Wang (:fredw) 2013-12-03 13:06:18 PST Created attachment 8341943 [details] [diff] [review] Patch V6 Frédéric Wang (:fredw) 2013-12-03 13:07:11 PST Created attachment 8341945 [details] [diff] [review] Allow nsGlyphCode to store glyphID. 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(). 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. 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. Frédéric Wang (:fredw) 2013-12-06 12:13:56 PST Created attachment 8343927 [details] [diff] [review] Patch V8 Frédéric Wang (:fredw) 2013-12-08 13:55:23 PST Created attachment 8344396 [details] [diff] [review] Patch V9 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-12-08 14:57:28 PST See comments in 407059. 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. 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. Frédéric Wang (:fredw) 2014-01-13 08:21:01 PST Created attachment 8359268 [details] [diff] [review] Patch V11 Refreshing patch. 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. 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). 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 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(). 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* out parameter for SetFontFamily(). Probably better would be nsRefPtr*. 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 }; 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? 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 |·| still have finite size. Frédéric Wang (:fredw) 2014-02-10 13:35:13 PST Created attachment 8373615 [details] [diff] [review] Patch V12 Frédéric Wang (:fredw) 2014-02-11 13:26:36 PST https://tbpl.mozilla.org/?tree=Try&rev=77b03bcb483e 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 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. Frédéric Wang (:fredw) 2014-02-11 23:27:12 PST https://hg.mozilla.org/try/rev/e383b42c650a 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 Frédéric Wang (:fredw) 2014-02-12 09:42:04 PST https://tbpl.mozilla.org/?tree=Try&rev=6de2d73b4986 Frédéric Wang (:fredw) 2014-02-12 10:54:57 PST Created attachment 8374986 [details] [diff] [review] Patch V13 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. 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. Frédéric Wang (:fredw) 2014-02-12 22:58:40 PST Created attachment 8375348 [details] [diff] [review] Patch V14 Ryan VanderMeulen [:RyanVM] 2014-02-13 06:56:59 PST https://hg.mozilla.org/integration/mozilla-inbound/rev/ea9b012bfe3e Wes Kocher (:KWierso) 2014-02-13 17:01:00 PST https://hg.mozilla.org/mozilla-central/rev/ea9b012bfe3e Jacek Caban 2014-02-25 03:49:24 PST I landed a trivial mingw fixup: https://hg.mozilla.org/integration/mozilla-inbound/rev/5c4e8fca7cc5 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.