1315270 - convert gfxFontGroup::MakeTextRun and supporting machinery to use Range<const T> instead of const T*, length pairs

Status: NEW

(Core :: Graphics: Text, defect, P3)

Description

[Nathan Froyd [:froydnj]]

mozilla::Range<T>:

https://dxr.mozilla.org/mozilla-central/source/mfbt/Range.h

(or GSL's span<T>, https://github.com/Microsoft/GSL/blob/master/gsl/span) was designed to be used in place of passing T*, length arguments everywhere.  mozilla::Range<T> is fairly limited compared to gsl::span, though, and as part of experimenting with how to extend it and experimenting with more Range<T> usage in Gecko, I thought I'd try a small-to-medium scale conversion project.  gfxFontGroup::MakeTextRun seems like a decent place to try this:

1. It touches a decent amount of code, so you're not just changing one or two functions, and you get some sense of the sort of patterns Range might need to extended with;
2. At the same time , you don't have to reformat the *entire* codebase or anything like that;
3. MakeTextRun has a fair number of parameters, and lowering that number seems like it'd be an improvement.

One concern is that, AIUI, text run creation performance is important, and debug builds would introduce bounds checks via Range::operator[], making debug builds slower than they are today.  We could probably work around that if necessary.

Jonathan, what do you think about this?  Does it seem like a reasonable thing to do, or should I look elsewhere?

Comment 1

[Jonathan Kew [:jfkthame]]

Hmmm, I'm not sure. On the one hand, I like the idea of consolidating various (pointer, length) pairs, and smart pointers that can validate they're not being abused are generally a good thing; but on the other hand, [1] makes it sound like there could be a performance risk here even for non-debug builds. We do care a lot about text-rendering performance.

(I'm conscious that even if we do this initially just in MakeTextRun and its immediately-related code, over time we're likely to extend that throughout the textrun/font code that handles measurement and rendering of ranges of text, and so any perf impact could grow. A tiny amount of overhead, insignificant when it only relates to textrun _creation_, might become more of a concern once it extends to measurement (e.g. for line-breaking) as well, then to every painting operation....)

I'm more concerned about any impact -- even if tiny -- on non-debug builds than I am about additional assertion checks in debug builds, though I suppose if that gets really extreme it could make them harder to use for debugging.

So... in principle I think this seems OK, but I might change that opinion in the event that we can detect any perf impact.

[1] https://dxr.mozilla.org/mozilla-central/rev/86f702229e32c6119d092e86431afee576f033a1/mfbt/RangedPtr.h#32-36

Comment 2

[Julien Cristau [:jcristau]]

Too late for firefox 52, mass-wontfix.