Currently calls GetAdvanceOfChar for each character. Why not return all of the positions in one go?

Is the idl correct? Surely there is a better way to indicate I want a float ** argument, I just don't know what it is.

Comment on attachment 228311 [details] [diff] [review] patch > /** > * Get the advance of an individual glyph. > */ >- float getAdvanceOfChar(in unsigned long charnum); >+ void getCharacterAdvance(out adv advance); Update the comment. Would naming this getCharacterAdvances suggest the purpose clearer to the reader? >+ nsresult rv = mSource->GetCharacterPosition(getter_Transfers(cp)); >+ NS_ENSURE_SUCCESS(rv, rv); This and similar changes in the patch - resist the temptation to do unrelated cleanups. > NS_IMETHODIMP >-nsSVGCairoGlyphMetrics::GetAdvanceOfChar(PRUint32 charnum, float *advance) >+nsSVGCairoGlyphMetrics::GetCharacterAdvance(float **advance) > { > cairo_text_extents_t extent; > > nsAutoString text; > mSource->GetCharacterData(text); >+ PRUint32 strLength = text.Length(); >+ >+ *advance = new float[strLength]; Check that this succeeded before putting data into it.

Got fed up with how complicated the glyph metrics code is and moved it into nsSVGGlyphFrame.cpp. This solves the GetAdvanceOfChar problem by removing the function altogether and calling the cairo functions inline. I'm not sure whether to close this bug (as WONTFIX or INVALID?) and open another called move nsSVGCairoGlyphMetrics into nsSVGGlyphFrame instead or just rename this bug. I don't need to make the idl changes any more as the idl files are gone also there's no memory allocated for the advances so nothing to go wrong there.

(In reply to comment #4) > Created an attachment (id=228437) [edit] You have the metric utility methods all calling SelectFont(mCT), and actually missed one call in GetCharNumAtPosition. This could be moved to DidSetStyleContext, but you would have to change text containers to call DidSetStyleContext on their glyph child frames, as the dominant baseline is in SVGReset so won't trigger the nsSVGGlyphFrame without assistance. If you're removing a state flag (thank you, by the way, those things are valuable), could you shift the rest down so it's always clear how many we have left?

> You have the metric utility methods all calling SelectFont(mCT), and actually > missed one call in GetCharNumAtPosition. This could be moved to > DidSetStyleContext, but you would have to change text containers to call > DidSetStyleContext on their glyph child frames, as the dominant baseline is in > SVGReset so won't trigger the nsSVGGlyphFrame without assistance. Added SelectFont to GetCharNumAtPosition. I'd rather do the DidSetStyleContext change under a follow-up. Would you like me to raise one? Alternatively we could choose to save memory by removing the cached cairo context and creating one each time any of the methods is called as we do in various functions in nsSVGCairoGlyphGeometry. In that case we couldn't cache the font either. > If you're removing a state flag (thank you, by the way, those things are > valuable), could you shift the rest down so it's always clear how many we have > left? Done.

Comment on attachment 228659 [details] [diff] [review] address review comments >@@ -876,31 +1138,17 @@ NS_IMETHODIMP_(void) > nsSVGGlyphFrame::NotifyMetricsSuspended() > { > // do nothing > } > > NS_IMETHODIMP_(void) > nsSVGGlyphFrame::NotifyMetricsUnsuspended() > { >- [giblets] >+ // do nothing > } Is there any reason for leaving in the NotifyMetrics{Un}suspended methods?

> Is there any reason for leaving in the NotifyMetrics{Un}suspended methods? No reason, just didn't realise they were no longer useful in any way. Gone in this patch.

Comment on attachment 228807 [details] [diff] [review] address additional review comments I look forward to seeing the fragment tree removal finish this cleanup.

I think caching the cairo context in the frame is a big overoptimization. In fact I'm not sure it's an optimization at all. If we must cache, can't we just cache one, perhaps in nsSVGUtils or in the renderer, next to the cached computational surface?

> I think caching the cairo context in the frame is a big overoptimization. In > fact I'm not sure it's an optimization at all. If we must cache, can't we just > cache one, perhaps in nsSVGUtils or in the renderer, next to the cached > computational surface? I was going to do that as a separate fix as the previous patch just copied the code from nsSVGCairoGlyphMetrics.cpp. If you want it done all in one go though, here it is. We gain a little on the swings from reducing the number of SelectFont calls.

Comment on attachment 228931 [details] [diff] [review] remove cairo context caching GetTextRendering appears to not be used, you can remove it. I think you should have a helper function next to SelectFont that creates the computational cairo context and selects the font into it, then returns it. + cairo_new_path(ctx); + cairo_destroy(ctx); + return -1; } What is the point of calling cairo_new_path just before cairo_destroy? We do this in several places. I know you just copied the code, but still.

Asking for sr again to check that you're happy with the helper class implementation (see below). > GetTextRendering appears to not be used, you can remove it. Done. We only implemented text quality for GDI+. I guess we can put this function back again if/when we actually implement this for cairo. > I think you should have a helper function next to SelectFont that creates the > computational cairo context and selects the font into it, then returns it. Created a helper class nsSVGAutoCairoContext that handles cairo_destroy too. Is this in the right place? I think it could be quite generally useful as non glyph methods sometimes want temporary cairo context (although without the SelectFont call). I could put this nsSVGUtils.h or in files all on its own. > + cairo_new_path(ctx); > + cairo_destroy(ctx); > + return -1; > } > What is the point of calling cairo_new_path just before cairo_destroy? We do > this in several places. I know you just copied the code, but still. Actually I didn't copy the code, I specifically added in the cairo_new_path calls. You get a memory leak if the path is not cleared before the context is destroyed. I think you can use a path even after you have finished with the context. See bug 336663 also. Would you like me to add in a comment along the lines of bug 336663?

> Created a helper class nsSVGAutoCairoContext that handles cairo_destroy too. The thing is, we already have something like that, it's the Thebes gfxContext. That's why I didn't suggest this helper class. At some point we really need to use the Thebes C++ classes. However, once I've read the code for your helper I'm probably OK with it. > Actually I didn't copy the code, I specifically added in the cairo_new_path > calls. You get a memory leak if the path is not cleared before the context is > destroyed. Sounds like a cairo bug. > I think you can use a path even after you have finished with the context. How? > See bug 336663 also. That's a different issue. We certainly need to make sure we leave the context in a consistent state if someone else might use it later. That's not a concern here.

I looked further into the memory usage. It does come back down so I was wrong in thinking there was a leak. I think you get more churn on memory allocated by cairo as it is not deallocated in the reverse order to allocation without the cairo_new_path so it goes up more if you stress it. I have removed the additional cairo_new_path calls in this version as they aren't required.

Comment on attachment 229112 [details] [diff] [review] further changes for sr comments nsSVGAutoCairoContext should be named something more specific. I suggest nsSVGAutoGlyphHelperContext.

Checked in