1261279 - Consider integrating Ahem font into reftest rather than requiring adding @font-face rules in tests

Status: RESOLVED INVALID

(Testing :: Reftest, defect)

Description

[Xidorn Quan [:xidorn] UTC+10]

There are lots of tests across different CSS modules using Ahem font. CSSWG's test repo uses a <meta> to mark the dependency to this font, but in our tests, @font-face rule is required for Ahem font. This causes several issues:
1. for tests pulled from CSSWG's test repo, we need to insert an additional rule into the file (done by importing script, though);
2. for tests we submit to CSSWG's test repo, we have unnecessary @font-face inside the test files, and have several copy of Ahem font across the tree;
3. we need to add <http> to some of the items, which may slow down the test according to reftests/README.txt.

Given this, I suggest we should integrate Ahem to reftest itself somehow, so that we can close all issues above.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1261279 part 1 - Make StaticRefPtr accept already_AddRefed for operator=. r?froydnj

Review commit: https://reviewboard.mozilla.org/r/46979/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46979/

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1261279 part 2 - Add pref to load Ahem font internally. r?jfkthame

Review commit: https://reviewboard.mozilla.org/r/46981/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46981/

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1261279 part 3 - Enable Ahem font for all reftests. r?dbaron

Review commit: https://reviewboard.mozilla.org/r/46983/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46983/

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1261279 part 4 - Stop inserting font-face rule for imported reftests. r?dbaron

Review commit: https://reviewboard.mozilla.org/r/46985/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46985/

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1261279 part 5 - Change font-loading-api/multiple-sets test to ensure its relying on external Ahem font. r?jfkthame

Review commit: https://reviewboard.mozilla.org/r/46987/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46987/

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1261279 part 6 - Remove useless ahem rule for font-inflation/list-1 test. r?jfkthame

Review commit: https://reviewboard.mozilla.org/r/47289/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47289/

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1261279 part 7 - Remove font-face rules for Ahem font and some redundant in-tree Ahem font file in reftests. r?jfkthame

Review commit: https://reviewboard.mozilla.org/r/46989/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46989/

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Hmmmm, so it doesn't work with Android...

Comment 9

[Jonathan Kew [:jfkthame]]

While I can see how it makes things much more convenient, I'm a bit reluctant to add code to the platform (for all builds -- it's not like this is DEBUG-only or something) just for this.

Can we achieve a similar level of convenience by getting the reftest harness to insert a stylesheet (perhaps via userContent.css or similar) that simply includes a @font-face rule to load Ahem (from a data: url in the stylesheet itself, so that it loads synchronously and we don't need to worry about the path)... if we (in effect) insert

  @font-face { font-family: Ahem; src: url(data:font/opentype;base64,.....); }

into userContent.css for every reftest, then AFAICS any CSSWG tests using the ahem font should Just Work without needing to be hacked on import, run via http, etc.

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

That sounds like a promising and simpler way, thanks for the suggestion.

Comment 11

[(not currently active) Ted Mielczarek]

You can use nsIStyleSheetService to do this, like:
https://dxr.mozilla.org/mozilla-central/rev/67ac40fb8f680ea5e03805552187ba1b5e8392a1/layout/style/test/test_addSheet.html#34

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

One concern is that adding @font-face in user stylesheet would trigger the user font set feature for all reftests, which is not true for most pages in wild, which means our testing environment here does not exactly match the users'. This may or may not be an issue. Probably not a big deal.

If we think this matters, we can probably add a new "ahem" flag to reftests.

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

It seems adding the user sheet causes some high-frequent intermittent failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4586dcdd79dc , mainly box-shadow/611574-2.html and css-grid/grid-fragmentation-002.html, also some others.

Comment 14

[Mats Palmgren (inactive)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #12)
> One concern is that adding @font-face in user stylesheet would trigger the
> user font set feature for all reftests [...]

I'm worried that this might hide style update errors.  IIRC, loading
a user font triggers RebuildAllStyleData in some cases.

BTW, why can't we simply require that Ahem is installed as a system font
to run reftests? (it's small and public domain)

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Mats Palmgren (:mats) from comment #14)
> BTW, why can't we simply require that Ahem is installed as a system font
> to run reftests? (it's small and public domain)

I'm not sure. It's probably fine I guess.

It seems dbaron was objecting this in bug 423097. dbaron, what was your concern? Do you think it is fine to ask people to install Ahem font for running reftests? I guess it is possible for mach to detect the existence of the font.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Tests should be easy to run.  reftests need to be debuggable by any engineer working on Gecko, not just those focused on layout.  Adding requirements makes that harder.  It's definitely not OK to add requirements; "./mach reftest" should just work for somebody who has a build.

The tests should also be usable by ordinary people without going through exercises like installing fonts, especially given that the platform has downloadable fonts as a feature.  We'd look down on web sites that ask users to install a font, and we should act in the same way.  And if we're asking everybody developing Web sites to use a feature, we shouldn't be deciding that it's too hard and building something easier for ourselves; if it's hard, we should fix that in the platform.  (But I don't think it is.)

I'm also concerned about anything that would make reftests behave in a way that's different from any other Web page.  Having an extra sheet applied to all reftests would be bad; it could cause extra restyles for the font load that interfere with what's being tested.


It's not clear to me what your current plan is; there are a lot of comments with different ideas in the bug.

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

My initial trial was adding a round after normal font lookup, so that if no font is found, check the internal Ahem font. And the internal Ahem font was located by a pref, which is set by the script runs reftests. jfkthame felt reluctant to add code in platform for just testing, and suggested using user stylesheets in comment 9.

And then I wrote a patch to register stylesheets via nsIStyleSheetService as suggested by ted in comment 11, but that causes various failures on unrelated tests, and also it raises the issue that behavior of wider range of code could be affected after having a @font-face.

So I guess we probably want to either abandon this bug and live with the current situation described in comment 0, or retry the initial trial which adds some native code to lower the impact of the additional font?

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think the current situation is better than any of the proposed alternatives.

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

Personally, I'm not willing to add any w3c reftest uses Ahem with the current situation ._.

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Why not?  I think it's fine to add style sheets that load Ahem into the W3C's repository, and in fact I think the W3C test repository should switch all Ahem tests to do that.

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

Because that requires many redundant Ahem fonts in tree.

Comment 22

[Jeff Walden [:Waldo]]

Given the cost of disk space, and some likelihood that the VCS will coalesce the various Ahem instances' data into a single blob in the internal data store -- is it really that big a deal to have multiple copies?  Doesn't seem like it to me.

Comment 23

[Jet Villegas (inactive)]

I'm looking at bug 1264056 comment 4 and thinking we're getting test noise from intermittent font loading behavior. I imagine this behavior exists in the wild, so having a way to exercise our rendering with and without font load delays is a good thing. The trouble is that may be getting test coverage as intermittent oranges. It could be good to have better controls at the harness level to choose the default (though intermittently flaky) behavior, or force fonts to be loaded and ready.

Comment 24

[Mats Palmgren (inactive)]

We ship an internal font already:
http://searchfox.org/mozilla-central/source/browser/fonts/
It think adding Ahem.ttf there would load it as a system font (it's installed
in $OBJDIR/dist/bin/fonts/).  It's only ~12k so it doesn't seem like a big deal.

This would address dbaron's concern that ""./mach reftest" should just work
for somebody who has a build."

Comment 25

[Jonathan Kew [:jfkthame]]

If we did that for all builds as standard, it would make Ahem mysteriously appear in the font list (as exposed in Preferences) for users; I'm not terribly keen on that.

Also note that support for such "bundled" fonts is a build-time configuration option that we don't currently enable across all platforms (only those where we decided to ship a bundled emoji font).

Comment 26

[Mats Palmgren (inactive)]

> it would make Ahem mysteriously appear in the font list

I suspect it wouldn't be hard to filter it out.

Comment 27

[Jet Villegas (inactive)]

Maybe we can tighten up font load behavior instead of baking Ahem into Firefox. See:
https://github.com/w3c/csswg-drafts/issues/1082