Closed Bug 1432552 Opened 6 years ago Closed 6 years ago

Extend the Inspector font-face API to expose variation-font named instances in addition to axes

Categories

(Core :: Layout: Text and Fonts, enhancement)

Product:
Core
Component:
Layout: Text and Fonts
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: jfkthame, Assigned: jfkthame)

References

(Blocks 1 open bug)

Details

(Whiteboard: [designer-tools])

Attachments

(4 files, 8 obsolete files)

patch 1 - Add a getVariationInstances() method to InspectorFontFace
8.63 KB, patch
Details | Diff | Splinter Review
patch 2 - Linux font back-end implementation of getVariationInstances
8.00 KB, patch
Details | Diff | Splinter Review
patch 3 - DirectWrite and macOS font back-end implementation of getVariationInstances
11.87 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
Update font-inspector API test to cover the getVariationInstances() method
11.83 KB, patch
Details | Diff | Splinter Review 
Bug 1323743 introduces basic layout inspector APIs to allow devtools to query the variation axes available in a given font.

We should extend this to also expose the font's collection of named instances (if present), so that devtools can present these to the user.
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
This follows on from bug 1323743, and extends the InspectorFontFace interface further to expose "named instances" of variation fonts.
Attachment #8945883 - Flags: review?(dholbert)
Attachment #8945883 - Flags: review?(bzbarsky)
Attachment #8944802 - Attachment is obsolete: true
Comment on attachment 8945883 [details] [diff] [review]
patch 1 - Add a getVariationInstances() method to InspectorFontFace

>+    InspectorVariationInstance& inst = *aResult.AppendElement();
....
>+      if (!inst.mValues.AppendElement(value, fallible_t())) {

Why is one append fallible and the other infallible?  This needs at least a comment explaining why the difference in treatment.

>+        break;

Shouldn't we throw OOM to JS when this happens?

r=me with that.
Attachment #8945883 - Flags: review?(bzbarsky) → review+
Comment on attachment 8945883 [details] [diff] [review]
patch 1 - Add a getVariationInstances() method to InspectorFontFace

Review of attachment 8945883 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits addressed, though feel free to request re-review on the updated patch if you'd like another look.

::: gfx/thebes/gfxFontVariations.h
@@ +26,5 @@
> +
> +struct gfxFontVariationInstance {
> +    nsString mName;
> +    nsTArray<gfxFontVariationValue> mValues;
> +};

Consider adding a brief comment above each of these "struct gfxFontVariationFooBar" structs (these ones and the previous ones)... That would be really helpful in remembering what their subtly-different usages are.

::: layout/inspector/InspectorFontFace.cpp
@@ +221,5 @@
> +  }
> +  AutoTArray<gfxFontVariationInstance,16> instances;
> +  mFontEntry->GetVariationInstances(instances);
> +  for (auto i : instances) {
> +    InspectorVariationInstance& inst = *aResult.AppendElement();

To reduce the need for possible-reallocation with each append here (and possibly to discover OOM up-front if you want to make this fallible per comment 9), you should probably preallocate the whole array up-front, like so:

  aResult.SetLength(instances.Length());

(possibly with fallible_t if you want it to be fallible)

Then you can rightly assume that each AppendElement will succeed.

(You should probably do this up in InspectorFontFace::GetVariationAxes(), too -- sorry for not thinking of this when reviewing that patch. Feel free to make that tweak as part of this patch, since it's relatively minimal.)

@@ +228,5 @@
> +      InspectorVariationValue value;
> +      value.mAxis.AppendPrintf("%c%c%c%c", (v.mAxis >> 24) & 0xff,
> +                                           (v.mAxis >> 16) & 0xff,
> +                                           (v.mAxis >> 8) & 0xff,
> +                                           v.mAxis & 0xff);

To avoid code-duplication, it might be worth factoring this out as a local helper so you can just call it like:
  AppendTagAsASCII(value.mAxis, v.mAxis);
or something, in the two places that do this.

(Up to you; 2 copies aren't so bad for this amount of code I suppose, particularly given that it's back-to-back & easy to visually compare.)

@@ +230,5 @@
> +                                           (v.mAxis >> 16) & 0xff,
> +                                           (v.mAxis >> 8) & 0xff,
> +                                           v.mAxis & 0xff);
> +      value.mValue = v.mValue;
> +      if (!inst.mValues.AppendElement(value, fallible_t())) {

Similarly, here, you should do "inst.mValues.SetLength(i.mValues.Length())" up-front (with fallible_t if needed).

Then you can handle failure earlier & assume that this AppendElement will succeed.
Attachment #8945883 - Flags: review?(dholbert) → review+
Sorry, when I said "SetLength" above, I really meant "SetCapacity":
https://dxr.mozilla.org/mozilla-central/source/xpcom/ds/nsTArray.h#1795-1800,1810-1811
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #9)
> Comment on attachment 8945883 [details] [diff] [review]
> patch 1 - Add a getVariationInstances() method to InspectorFontFace
> 
> >+    InspectorVariationInstance& inst = *aResult.AppendElement();
> ....
> >+      if (!inst.mValues.AppendElement(value, fallible_t())) {
> 
> Why is one append fallible and the other infallible?  This needs at least a
> comment explaining why the difference in treatment.

I'd have left it all infallible (these things won't get huge, as there are various 16-bit limitations on the size of things in the underlying font format), but the compiler doesn't seem to allow an infallible inst.mValues.AppendElement(). That'll be because mValues is declared (from webidl) as a sequence<...>, which in a FallibleTArray. (Or am I missing something -- can we make infallible calls on a fallible array?)
Comment on attachment 8945884 [details] [diff] [review]
patch 2 - Linux font back-end implementation of getVariationInstances

Review of attachment 8945884 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits addressed, though as above I'm happy to take another look if you like.

::: gfx/thebes/gfxFcPlatformFontList.cpp
@@ +385,5 @@
> +        // InitializeVarFuncs must have been called in order for mMMVar to be
> +        // non-null here, so we don't need to do it again.
> +        if (sDoneVar) {
> +            FT_Face face = GetFTFace();
> +            (*sDoneVar)(face->glyph->library, mMMVar);

Two things:

(1) Why are we using "GetFTFace()" here, vs. mFTFace in the sDoneVar(...) usage further down?  Could we just use mFTFace directly here?

(2) Elsewhere we have null-checks (er, "truthiness checks") for the "GetFTFace()" result before we use it.  I'm guessing you're skipping that here because mMMVar==true implies that GetFTFace()==truthy, right?  Consider adding a MOZ_ASSERT that face (or mFTFace per (1)) is truthy, before you dereference it, to make it clearer that this is an explicit justified assumption & not just a forgotten/inconsistent null-check.

@@ +1121,5 @@
> +            value.mAxis = mmVar->axis[j].tag;
> +            value.mValue = ns.coords[j] / 65536.0;
> +            inst.mValues.AppendElement(value);
> +        }
> +        aInstances.AppendElement(inst);

As with previous patch: since you know the array sizes up-front (before each AppendElement-in-a-loop pattern), you can avoid some allocation churn if you use SetCapacity to preallocate the correct size for each "inst.mValues" array, and for the aInstances output array.
Attachment #8945884 - Flags: review?(dholbert) → review+
> because mValues is declared (from webidl) as a sequence<...>, which in a FallibleTArray.

Ah, I see.  Then this is ok but might be worth a comment.
Comment on attachment 8945885 [details] [diff] [review]
patch 3 - DirectWrite and macOS font back-end implementation of getVariationInstances

Review of attachment 8945885 [details] [diff] [review]:
-----------------------------------------------------------------

I haven't really reviewed this one thoroughly yet - I think it'd benefit from some more comments in the new gfxFontUtils code, so I'll hold off on staring at it too hard, since a lot of it just makes no sense to me right now. And I'm hoping it'll make more sense once you've added some comments. :)

(I flagged a few specific places that could stand to be commented in my review feedback below, but those are by no means a complete list -- those are just the things I noticed before I punted on reviewing the rest. :D)

::: gfx/thebes/gfxFontUtils.cpp
@@ +1828,5 @@
> +        AutoSwap_PRUint16 instanceCount;
> +        AutoSwap_PRUint16 instanceSize;
> +    }; 
> + 
> +    struct AxisRecord { 

EOL whitespace on these last 3 lines.

@@ +1836,5 @@
> +        AutoSwap_PRInt32  maxValue;
> +        AutoSwap_PRUint16 flags;
> +        AutoSwap_PRUint16 axisNameID;
> +    }; 
> + 

EOL whitespace on these last 2 lines.

@@ +1841,5 @@
> +    struct InstanceRecord {
> +        AutoSwap_PRUint16 subfamilyNameID;
> +        AutoSwap_PRUint16 flags;
> +        AutoSwap_PRInt32  coordinates[1]; // variable-size array
> +    //  AutoSwap_PRUint16 postScriptNameID; // optional field

Why is this last field here (but commented out)?

If you've just got it here for completeness but it's (apparently) not needed, maybe add "(not needed for our purposes)" or something to that effect.  Or, just get rid of it?

@@ +1859,5 @@
> +
> +        operator hb_blob_t* () { return mBlob; }
> +
> +    private:
> +        hb_blob_t* mBlob;

Add "const" after the "*".

(Good practice for RAII stuff, to enforce/document that the wrapped value is locked in at construction-time and never changed no matter what, and hence the destructor's RAII-cleanup will indeed be operating on the same thing that was passed into the constructor.)

@@ +1872,5 @@
> +    const char* data = hb_blob_get_data(fvarTable, &len);
> +    if (len < sizeof(FvarHeader)) {
> +        return;
> +    }
> +    auto fvar = reinterpret_cast<const FvarHeader*>(data);

This reinterpret_cast, etc. all feels very fragile/dependent-on-harfbuzz-internals...  Is there any documentation that you can link to or reference in code-comments here?

(e.g. to back up the exact layout of your custom FvarHeader struct, for example)

Both for review & maintainability purposes, it'd be nice to have some code comments here explaining those sorts of things...

@@ +1886,5 @@
> +    if (axisCount == 0 ||
> +        axisSize != 20 ||
> +        instanceCount == 0 ||
> +        (instanceSize != axisCount * sizeof(int32_t) + 4 &&
> +         instanceSize != axisCount * sizeof(int32_t) + 6)) {

There are a lot of magic numbers here -- e.g. axisSize != 20, sizeof(int32_t) + 6, etc.  Can you add a few comments to hint at what these numbers represent?

::: gfx/thebes/gfxFontUtils.h
@@ +1002,5 @@
>                                      nsTArray<uint16_t> &aGlyphs,
>                                      nsTArray<mozilla::gfx::Color> &aColors);
>  
> +    static void
> +    GetVariationInstances(gfxFontEntry* aFontEntry,

Could you add a brief comment here, just saying that this is a helper that's used to help implement gfxFontEntry::GetVariationInstances overrides on some platforms? (or something like that)

(Right now it's a bit confusing, since this has the same name as the API that it's helping with, and since this mostly *is* the implementation of that API on some platforms but not others.  If someone finds a GetVariationInstances call and is trying to reason about which function(s) actually get called and why, it'd be helpful to have a brief comment here to orient them around how this particular version of the function fits into the picture [or doesn't, on Linux].)
Attachment #8945885 - Flags: review?(dholbert) → review-
Comment on attachment 8945886 [details] [diff] [review]
Update font-inspector API test to cover the getVariationInstances() method

Review of attachment 8945886 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on test tweaks with nits addressed.

::: layout/inspector/tests/chrome/test_fontVariationsAPI.xul
@@ +40,5 @@
> +  { tag: "wdth", name: "Width",  minValue: 0.62, maxValue: 1.3, defaultValue: 1.0 },
> +];
> +
> +function checkAxes(axes, reference) {
> +  is(axes.length, reference.length, "number of variation axes: " + axes.length);

The message here should just be "number of variation axes".  I don't think you want to hardcode the expected result in the prose of the error message, because it's redundant -- SimpleTest will already print it out for you if there's a failure.

(See https://dxr.mozilla.org/mozilla-central/rev/e22e2c9eb81686e958a9448ea3d1e8cd766743e2/testing/mochitest/tests/SimpleTest/SimpleTest.js#317 for the isfuzzy version of the error message -- and is() has a similar one).  IMO it makes the failure message more confusing if you repeat one of the numbers in the message (since it's not clear whether the repeated number is the observed value vs. the expected value).

This applies to every is(...) and isfuzzy(...) comparison in this test -- they can all have their message shortened a bit to avoid hardcoding in the observed value.

@@ +44,5 @@
> +  is(axes.length, reference.length, "number of variation axes: " + axes.length);
> +  for (var i = 0; i < axes.length; ++i) {
> +    var v = axes[i];
> +    var ref = reference[i];
> +    is(v.tag, ref.tag, "axis tag: " + v.tag);

nit: this will throw if we happen to have a scenario where we find more axes than we expect. (because we'll iterate up to some "i" which is out of bounds for the references array, and then ref = reference[i] will be undefined, and ref.tag will throw a TypeError).

To avoid throwing an error, we should probably iterate up to
  Math.min(axes.length, reference.length)
i.e.
  for (var i = 0; i < Math.min(axes.length, reference.length); ++i) {

This way, if the lengths are not equal, we'll still report a test-failure (from the "is(...)" comparison at the start of this function, and we'll still compare everything that we can meaningfully compare, without having to end the test prematurely & messily.

@@ +77,5 @@
> +  { name: "Wide Light",        values: [ { axis: "wght", value: 300 }, { axis: "wdth", value: 150 } ] },
> +  { name: "Wide Regular",      values: [ { axis: "wght", value: 400 }, { axis: "wdth", value: 150 } ] },
> +];
> +
> +const SkiaInstances = [ 

EOL whitespace.

@@ +94,5 @@
> +function checkInstances(instances, reference) {
> +  is(instances.length, reference.length,
> +     "number of variation instances: " + instances.length);
> +  instances.sort(function (a, b) {
> +    return a.name.localeCompare(b.name);

I don't know how much string-comparison differs between locales, but for fool-proofing, it might be wise to explicitly specify "en-US" or "en-GB" as the second argument here (the locale) so that the behavior is a bit more predictable & well-defined, regardless of the locale of the particular Firefox build that's being tested.

(It seems conceivable that there might exist some locale which might have its own sorting quirk that'd change the ordering of one of the instances here, either now or in the future. And we don't want this test be at the mercy of that sort of thing, since we're only sorting to make the hardcoded-expected-results comparison easier.)

@@ +96,5 @@
> +     "number of variation instances: " + instances.length);
> +  instances.sort(function (a, b) {
> +    return a.name.localeCompare(b.name);
> +  });
> +  for (var i = 0; i < instances.length; ++i) {

As above, you should iterate up to "Math.min(instances.length, reference.length)" here, to avoid walking off the end of the reference array and throwing an error.

@@ +102,5 @@
> +    var inst = instances[i];
> +    is(inst.name, ref.name, "instance name: " + inst.name);
> +    for (var j = 0; j < inst.values.length; ++j) {
> +      var v = inst.values[j];
> +      is(v.axis, ref.values[j].axis, "axis: " + v.axis);

Perhaps worth adding
  is(inst.values.length, ref.values.length, ...);
...before this loop, and iterating up to the min of those two lengths?
Attachment #8945886 - Flags: review?(dholbert) → review+
Attachment #8944803 - Attachment is obsolete: true
Attachment #8944804 - Attachment is obsolete: true
Attachment #8944805 - Attachment is obsolete: true
Updated for review comments, carrying over r=bz,dholbert.
Attachment #8945883 - Attachment is obsolete: true
Updated for review comments, carrying over r=dholbert.
Attachment #8945884 - Attachment is obsolete: true
Updated patch, now with comments. :)
Attachment #8946087 - Flags: review?(dholbert)
Attachment #8945885 - Attachment is obsolete: true
Updated for review comments, carrying over r=dholbert.
Attachment #8945886 - Attachment is obsolete: true
Comment on attachment 8946087 [details] [diff] [review]
patch 3 - DirectWrite and macOS font back-end implementation of getVariationInstances

Review of attachment 8946087 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for adding these comments! That makes this a lot easier to follow.

r=me with the following addressed:

::: gfx/thebes/gfxFontUtils.cpp
@@ +1904,5 @@
> +        return;
> +    }
> +    // Check that axis array will not exceed table size
> +    uint16_t axesOffset = fvar->axesArrayOffset;
> +    if (axesOffset + uint32_t(axisCount * axisSize) > len) {

You need to do the cast *before* the multiplication here -- i.e. you need to promote one of the factors to be a uint32_t so that the multiplication happens in 32-bit space, like so:
  uint32_t(axisCount) * axisSize

Without that change, I think the "axisCount * axisSize" operation could be performed in uint16_t space, and possibly overflow in that space, and then the possibly-overflowed 16-bit value will then be promoted to a 32-bit integer.

(Technically this probably "just works" as-is, because really, the factors will be automatically promoted to 'int' for the multiplication, since their type is smaller than int, according to https://stackoverflow.com/a/3496183/329541. But we probably shouldn't depend on that special case.)

@@ +1912,5 @@
> +    auto axes = reinterpret_cast<const AxisRecord*>(data + axesOffset);
> +    // Get address of instance array, and check it doesn't overflow table size.
> +    // https://www.microsoft.com/typography/otspec/fvar.htm#axisAndInstanceArrays
> +    auto instData = data + axesOffset + axisCount * axisSize;
> +    if (instData + instanceCount * instanceSize > data + len) {

s/instanceCount/uint32_t(instanceCount)/, for consistency with the similar "axisCount * axisSize" overflow check (with my review comment about needing to promote before multiplication)

@@ +1919,5 @@
> +    aInstances.SetCapacity(instanceCount);
> +    for (unsigned i = 0; i < instanceCount; ++i, instData += instanceSize) {
> +        // Typed pointer to the current instance record, to read its fields
> +        auto inst = reinterpret_cast<const InstanceRecord*>(instData);
> +        auto coords = &inst->coordinates[0];

Can you add a check somewhere to be sure that the upcoming "coords[j]" indexing isn't going to run out of bounds of our InstanceRecord (and ultimately, our 'data' array)? I don't think we have any checks to validate that the expected-size of that coords array actually fits where we expect it to fit (i.e. that instanceSize is really large enough to fit a "coordinates" array of size "axisCount")

Maybe something like:
 if (coords + axisCount > instData + instanceSize) {
   // coordinates array is larger than the instance that
   // it lives in! Bail out.
 }

(It's slightly wasteful to check this on each loop iteration, since we really only need to check it once. But maybe not a huge deal... the ergonomics are nice for having it here, since this is where we've got "coords" declared and right where we're about to start indexing into it, and it's good to know that that's safe right when we do it....)
Attachment #8946087 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #21)
> Can you add a check somewhere to be sure that the upcoming "coords[j]"
> indexing isn't going to run out of bounds of our InstanceRecord (and
> ultimately, our 'data' array)? I don't think we have any checks to validate
> that the expected-size of that coords array actually fits where we expect it
> to fit (i.e. that instanceSize is really large enough to fit a "coordinates"
> array of size "axisCount")

This is known to be safe because we validate the instanceSize field in the header:

>        // https://www.microsoft.com/typography/otspec/fvar.htm#instanceSize
>        (instanceSize != axisCount * sizeof(int32_t) + 4 &&
>         instanceSize != axisCount * sizeof(int32_t) + 6)) {
>        return;

and bail out if it isn't correct for the table's axisCount; so we know that instanceSize includes the axisCount elements of the coordinates array. And then we have checked that the array of InstanceRecords, given this instanceSize, doesn't exceed the actual table size:

>    if (instData + uint32_t(instanceCount) * instanceSize > data + len) {
>        return;

Hence, no need to check this on each loop interation, but I'll add a comment there for clarity.
Ah, makes sense. Thank you!
Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7ff43ee429b
patch 1 - Add a getVariationInstances() method to InspectorFontFace. r=dholbert,bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/33fb37dc9d54
patch 2 - Linux font back-end implementation of getVariationInstances. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc99d0e42468
patch 3 - DirectWrite and macOS font back-end implementation of getVariationInstances. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcf0b020d40d
Update font-inspector API test to cover the getVariationInstances() method. r=dholbert
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: