Initial straightforward conversion - somewhat wasteful in that each nsSVGGlyphFrame has a gfxFontStyle and gfxFontGroup.

+ifdef MOZ_ENABLE_GTK2 +DEFINES += -DMOZ_ENABLE_GTK2 +endif What's this for? I'm not sure what the ownership model for mFontStyle is. Your code appears to be confused about whether you own it or not. If you do own it, use nsAutoPtr. As I mentioned on IRC you'll need to eventually take clustering into account when drawing along a path. Probably should throw a comment in there for now. + gfxTextRun::Metrics metrics = + textRun->MeasureText(i, 1, PR_FALSE, nsnull); + float advance = metrics.mAdvanceWidth; If you only want the advance, GetAdvanceWidth is preferred. (Seems to happen in many places.) Also, you'll need to take clustering into effect here too. Basically, whenever you always pass "1" as the length you're doing something wrong. (Although it's fine to fix this later.) + gfxPoint pt = gfx->CurrentPoint() + metrics.mBoundingBox.pos; + gfx->Rectangle(gfxRect(pt, metrics.mBoundingBox.size)); Why not just metrics.mBoundingBox + pt? + return + gfxTextRunCache::GetCache()->GetOrMakeTextRun(aCtx, mFontGroup, + aText.get(), aText.Length(), + 1, //GetPresContext()->AppUnitsPerDevPixel(), + PR_FALSE, PR_FALSE, + aOwning); Passing 1 as AppUnitsPerDevPixel has the problem that all character advances will be rounded to the nearest pixel. Ultimately that's not good. Eventually it would be better to pass a larger value and then scale down the metrics you get. (Actual drawing will be scaled down for you by the textrun.) If we ever implement what I suggested on IRC about having SVG text create anonymous hidden blocks to carry out inline layout, then you'd use the textruns from the textframes in those blocks instead of creating your own.

(In reply to comment #2) > +ifdef MOZ_ENABLE_GTK2 > +DEFINES += -DMOZ_ENABLE_GTK2 > +endif > > What's this for? Overly generous copy of Makefile.in lines from gfx/src/thebes/Makefile.in. > I'm not sure what the ownership model for mFontStyle is. Your code appears to > be confused about whether you own it or not. If you do own it, use nsAutoPtr. We own it, and it needs to live for the life of the gfxFontGroup, according to Stuart. > As I mentioned on IRC you'll need to eventually take clustering into account > when drawing along a path. Probably should throw a comment in there for now. > > + gfxTextRun::Metrics metrics = > + textRun->MeasureText(i, 1, PR_FALSE, nsnull); > + float advance = metrics.mAdvanceWidth; > > If you only want the advance, GetAdvanceWidth is preferred. (Seems to happen in > many places.) Ok, I remember seeing that in the header file and thebes sources, not sure how I managed to miss using it. > Also, you'll need to take clustering into effect here too. > Basically, whenever you always pass "1" as the length you're doing something > wrong. (Although it's fine to fix this later.) > > + gfxPoint pt = gfx->CurrentPoint() + metrics.mBoundingBox.pos; > + gfx->Rectangle(gfxRect(pt, metrics.mBoundingBox.size)); > > Why not just metrics.mBoundingBox + pt? If memory serves, an intermediate step had something more complicated, and once reduced didn't get the final cleanup. > + return > + gfxTextRunCache::GetCache()->GetOrMakeTextRun(aCtx, mFontGroup, > + aText.get(), aText.Length(), > + 1, > //GetPresContext()->AppUnitsPerDevPixel(), > + PR_FALSE, PR_FALSE, > + aOwning); > > Passing 1 as AppUnitsPerDevPixel has the problem that all character advances > will be rounded to the nearest pixel. Ultimately that's not good. Eventually it > would be better to pass a larger value and then scale down the metrics you get. > (Actual drawing will be scaled down for you by the textrun.) Right, the 1 didn't seem right to me either, which is why I left the comment in to revisit. That was needed in order to have the text positioned in the right place when drawn with the coordinates used by SVG. If I pass in the conversion factor, then I need to scale coordinates both going into the draw methods and coming out of metric checks. > If we ever implement what I suggested on IRC about having SVG text create > anonymous hidden blocks to carry out inline layout, then you'd use the textruns > from the textframes in those blocks instead of creating your own.

> If I pass in the conversion factor, then I need to scale coordinates both going > into the draw methods and coming out of metric checks. Yes, that's correct. Sorry that's not so convenient but I hope you'll agree that it's a good idea to have everything optimized for layout where we are using appunits.

(In reply to comment #4) > > If I pass in the conversion factor, then I need to scale coordinates both going > > into the draw methods and coming out of metric checks. > > Yes, that's correct. Sorry that's not so convenient but I hope you'll agree > that it's a good idea to have everything optimized for layout where we are > using appunits. Sure, just wanted to check that there wasn't a third option before scattering conversions around the code.

A third option would be to add some new API or flag so that gfxTextRun could do any necessary conversions itself. However, as long as the possibility remains that we might use textruns via layout textframes --- which would require conversions to be moved out to SVG --- then we should probably avoid cluttering the gfxTextRun API with features we might end up not using.

Just leave the scale factor at 1 for now until we eventually figure out what the big plan is. We could probably even ship with 1 and text just won't look as nice as it possibly could (on Mac and Linux; AFAIK we can't even get subpixel glyph placement on Windows).

Comment on attachment 259449 [details] [diff] [review] switch svg text to thebes > NS_IMETHODIMP > nsSVGGlyphFrame::DidSetStyleContext() > { > nsSVGGlyphFrameBase::DidSetStyleContext(); > >+ // clear out old font >+ if (mFontStyle) >+ delete mFontStyle; no test required. >+#if defined(XP_WIN) >+ mFontGroup = new gfxWindowsFontGroup(font.name, mFontStyle); >+#elif defined(MOZ_ENABLE_PANGO) >+ mFontGroup = new gfxPangoFontGroup(font.name, mFontStyle); >+#elif defined(XP_MACOSX) >+ mFontGroup = new gfxAtsuiFontGroup(font.name, mFontStyle); >+#elif defined(XP_OS2) >+ mFontGroup = new gfxOS2FontGroup(font.name, mFontStyle); >+#else >+#error implement me >+#endif Shouldn't this be in thebes somewhere? Would make it less tricky for someone to add a new platform. > >- if (hasStroke) { >- SetupCairoStrokeGeometry(ctx); >- cairo_stroke_extents(ctx, &xmin, &ymin, &xmax, &ymax); >- nsSVGUtils::UserToDeviceBBox(ctx, &xmin, &ymin, &xmax, &ymax); >+ if (HasStroke()) { >+ SetupCairoStrokeGeometry(gfx); >+ extent = gfx->GetUserStrokeExtent(); >+ extent = gfx->UserToDevice(extent); > } else { leave as hasStroke, you still have this variable and use it elsewhere. >- return NS_NewSVGRect(_retval, xmin, ymin, xmax - xmin, ymax - ymin); >+ return NS_NewSVGRect(_retval, >+ rect.X(), rect.Y(), rect.Width(), rect.Height()); > } return NS_NewSVGRect(_retval, rect); At least two occurences.

Helps if you save the file before diffing...

Comment on attachment 259719 [details] [diff] [review] really fix comment >+ gfxTextRun::Metrics metrics = >+ ((gfxTextRun *)ctx)->MeasureText(charnum, 1, PR_FALSE, nsnull); Better to write a nsSVGAutoGlyphHelperContext::GetTextRun method rather than having an operator gfxTextRun * and explicitly call that since you are casting anyway.

Comment on attachment 259806 [details] [diff] [review] replace operator with accessor, update to tip > void >-nsSVGGlyphFrame::LoopCharacters(cairo_t *aCtx, const nsAString &aText, >+nsSVGGlyphFrame::LoopCharacters(gfxContext *aCtx, const nsString &aText, > const nsSVGCharacterPosition *aCP, >- void (*aFunc)(cairo_t *cr, const char *utf8)) >+ PRBool aDrawToPath) Maybe make drawtopath an enum so you know when looking at the callers what they are doing. > > nsAutoArrayPtr<nsSVGCharacterPosition> cp; > > nsSVGAutoGlyphHelperContext ctx(this, text, getter_Transfers(cp)); > >+ gfxContext *gfx = ctx; >+ gfxTextRun *textRun = ctx.GetTextRun(); >+ Do you need the gfx = ctx or can you use ctx directly? How about renaming the nsSVGAutoGlyphHelperContext variable gfx rather than ctx. >+ gfxMatrix rot; >+ rot.Rotate(cp[i].angle); >+ rot.Invert(); >+ gfxPoint pt = rot.Transform(gfxPoint(cp[i].x, cp[i].y)); >+ >+ gfx->Rotate(cp[i].angle); >+ textRun->DrawToPath(gfx, pt, i, 1, nsnull, nsnull); Move to a helper perhaps? Very similar code in more than one place. Probably with the if drawtopath test. > float s = sin(cp[charnum].angle); > float c = cos(cp[charnum].angle); > >- return NS_NewSVGPoint(_retval, >- cp[charnum].x + extent.x_advance * c - extent.y_advance * s, >- cp[charnum].y + extent.y_advance * c + extent.x_advance * s); >+ return NS_NewSVGPoint(_retval, >+ cp[charnum].x + advance * c, >+ cp[charnum].y + advance * s); > } > Could use sin and cos directly rather than storing in s and c as you are only using the result once now. //---------------------------------------------------------------------- > // > >-void nsSVGGlyphFrame::SelectFont(gfxContext *aContext) >-{ >- cairo_t *ctx = aContext->GetCairo(); >- >- const nsStyleFont* fontData = GetStyleFont(); >- nsFont font = fontData->mFont; >- >- // XXX eventually we will have to treat decorations separately from >- // fonts, because they can have a different color than the current >- // glyph. >- We seem to have lost the comment above. Do we get any text-decoration support though thebes (I see you pass it into the fontStyle) and if so does it meet SVG requirements? > class nsSVGAutoGlyphHelperContext; > friend class nsSVGAutoGlyphHelperContext; > > // A helper class to deal with temporary cairo contexts. > // It destroys the context when it goes out of scope. > class nsSVGAutoGlyphHelperContext Above comment no longer correct. Additionally: Would we be better having mX and mY and also nsSVGCharacterPosition x and y fields replaced by a gfxPoint? We seem to need to do conversions in various places which we might then avoid.

(In reply to comment #14) > > nsSVGAutoGlyphHelperContext ctx(this, text, getter_Transfers(cp)); > > > >+ gfxContext *gfx = ctx; > >+ gfxTextRun *textRun = ctx.GetTextRun(); > >+ > > Do you need the gfx = ctx or can you use ctx directly? How about renaming the > nsSVGAutoGlyphHelperContext variable gfx rather than ctx. Yes, I need that line because the compiler can't figure out to use the conversion operator when used as "ctx->Rectangle". This is why I used to have that ugly cast for getting the gfxTextRun*.

(In reply to comment #14) > >+ gfxMatrix rot; > >+ rot.Rotate(cp[i].angle); > >+ rot.Invert(); > >+ gfxPoint pt = rot.Transform(gfxPoint(cp[i].x, cp[i].y)); > >+ > >+ gfx->Rotate(cp[i].angle); > >+ textRun->DrawToPath(gfx, pt, i, 1, nsnull, nsnull); > > Move to a helper perhaps? Very similar code in more than one place. Probably > with the if drawtopath test. It's in two places, and with fairly different code around.

What about nsSVGCharacterPosition having a gfxPoint too rather than x and y?

Looks good now. I think that it would be clearer if you added an accessor for the gfxContext to nsSVGAutoGlyphHelperContext as the current operator access is pretty useless now except to write gfxContext *gfx = ctx which looks odd. I'd give you an r=longsonr with that.

Your other patch removes nsSVGFlattenedPath right? Do you want to update this patch?

(In reply to comment #23) > Your other patch removes nsSVGFlattenedPath right? Do you want to update this > patch? The two patches overlap; I'll update whichever goes second. The other patch is dependent on getting a review for adding flattening to thebes (bug 376927).

Another issue is that I'm about to change textrun caching so gfxTextRunCache.h will go away or at least change significantly. Can you remove your dependency on that, just call MakeTextRun on gfxFontGroup directly?

+ // XXX eventually we will have to treat decorations separately from + // fonts, because they can have a different color than the current + // glyph. The decorations passed to gfxFontStyle don't actually do anything. You can probably just remove this comment. At some point we should remove the decorations. Do you want to check the result of "new gfxFontStyle"? + rot.Invert(); + gfxPoint pt = rot.Transform(aCP[i].pos); + + aCtx->Rotate(aCP[i].angle); Why don't you do aCtx->SetMatrix(rot) before you invert it, instead of calling Rotate again? Saves some matrix math. + LoopCharacters(gfx, text, cp, STROKE); + gfx->IdentityMatrix(); + gfxRect rect = gfx->GetUserFillExtent(); This will be a very slow way to get the bbox. Why don't you do something similar to GetCoveredRegion and call MeasureText() and use mBoundingBox? nsSVGGlyphFrame::GetCharacterPosition seems to always advance from left to right. That seems wrong. Ditto for nsSVGGlyphFrame::GetStartPositionOfChar, nsSVGGlyphFrame::GetCharNumAtPosition and maybe others...

(In reply to comment #27) > + LoopCharacters(gfx, text, cp, STROKE); > + gfx->IdentityMatrix(); > + gfxRect rect = gfx->GetUserFillExtent(); > > This will be a very slow way to get the bbox. Why don't you do something > similar to GetCoveredRegion and call MeasureText() and use mBoundingBox? Per the specification, getBBox is supposed to return a tight bounding box. > nsSVGGlyphFrame::GetCharacterPosition seems to always advance from left to > right. That seems wrong. Ditto for nsSVGGlyphFrame::GetStartPositionOfChar, > nsSVGGlyphFrame::GetCharNumAtPosition and maybe others... Progression seems like the least of our problems dealing with rtol text currently.

> + // XXX docorations are ignored by gfxFontStyle - still need to implement s/docorations/decorations/

Checked in with comment typo fixed.

Backed out - seems to be causing the tests on qm-winxp01 to fail.

(In reply to comment #32) > Backed out - seems to be causing the tests on qm-winxp01 to fail. > ###!!! ASSERTION: GetDCFromSurface: Context target is not win32!: 'Error', file c:/firefox/mozilla/gfx/thebes/src/gfxWindowsFonts.cpp, line 82 ###!!! ASSERTION: GetDCFromSurface: Context target is not win32!: 'Error', file c:/firefox/mozilla/gfx/thebes/src/gfxWindowsFonts.cpp, line 82 ###!!! ASSERTION: No DC: 'aDC', file c:/firefox/mozilla/gfx/thebes/src/gfxWindowsFonts.cpp, line 1551 cairo_win32_scaled_font_select_font:SelectObject: The handle is invalid. from layout/reftests/object/svg.html

So you're passing an image surface here? I guess GetDCFromSurface should fall back to doing GetDC(NULL) or something.

Another nit I noticed compiling this in the brief time it was landed. > + float advance = textRun->GetAdvanceWidth(charnum, 1, nsnull); should s/float/gfxFloat/ I think the assignment of GetAdvanceWidth to float occurs twice (look for halfAdvance too).

(In reply to comment #35) > Another nit I noticed compiling this in the brief time it was landed. > > > + float advance = textRun->GetAdvanceWidth(charnum, 1, nsnull); > > should s/float/gfxFloat/ > > I think the assignment of GetAdvanceWidth to float occurs twice (look for > halfAdvance too). Is the compiler actually complaining about this? If so, there's more than just a couple places where we're doing implicit float/gfxFloat conversion currently.

(In reply to comment #34) > So you're passing an image surface here? > > I guess GetDCFromSurface should fall back to doing GetDC(NULL) or something. That would work, but properly releasing it is a bit of pain given how the gfxWindowsFont code is structured. The easier solution is to change nsSVGUtils::GetThebesComputationalSurface to use gfxPlatform::CreateOffscreenSurface.

> Is the compiler actually complaining about this? If so, there's more than just > a couple places where we're doing implicit float/gfxFloat conversion currently. > Just warnings. A gfxFloat is actually a double.

(In reply to comment #38) > > Is the compiler actually complaining about this? If so, there's more than just > > a couple places where we're doing implicit float/gfxFloat conversion currently. > > > > Just warnings. A gfxFloat is actually a double. gcc doesn't complain about that - the only things it warns about in nsSVGGlyphFrame is some longstanding virtual hidden methods.

Comment on attachment 261981 [details] [diff] [review] modified nsSVGUtils::GetThebesComputationalSurface, tested on all tier-1 platforms good idea!

Checked in again - tinderboxes happy this time around.