1441622 - Expose the OpenType Font Variations data to the fonts redux store

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P3)

Description

[Gabriel Luong [:gl] (ΦωΦ)]

This is to populate the fonts redux store with the OpenType Font Variations store so we can work on the UI in Bug 1441576.

Comment 1

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Bug 1441622 - Expose the OpenType Font Variations data to the fonts redux store.

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

Comment 2

[Code Review Bot [:reviewbot]]

Comment on attachment 8954559 [details]
Bug 1441622 - Expose the OpenType Font Variations data to the fonts redux store.

https://reviewboard.mozilla.org/r/223608/#review229694


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/inspector/fonts/fonts.js:198
(Diff revision 1)
>        return;
>      }
>  
>      let options = {
>        includePreviews: true,
> +      includeVariations: FONT_VARIATIONS_ENABLED,,

Error: Parsing error: unexpected token , [eslint: None]

::: devtools/client/inspector/fonts/types.js:34
(Diff revision 1)
> +
> +  // The value of the variation axis
> +  value: PropTypes.number,
> +};
> +
> +const fontVariationInstance = exports.fontVariationInstance = {

Error: 'fontvariationinstance' is assigned a value but never used. [eslint: no-unused-vars]

Comment 3

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8954559 [details]
Bug 1441622 - Expose the OpenType Font Variations data to the fonts redux store.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/223608/diff/1-2/

Comment 4

[Gabriel Luong [:gl] (ΦωΦ)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce2882aa0f512727aebefde011b2d1d0bcadf3c1

Comment 5

[Patrick Brosset <:pbro>]

Comment on attachment 8954559 [details]
Bug 1441622 - Expose the OpenType Font Variations data to the fonts redux store.

https://reviewboard.mozilla.org/r/223608/#review229794

::: devtools/client/inspector/fonts/fonts.js:198
(Diff revision 2)
>        return;
>      }
>  
>      let options = {
>        includePreviews: true,
> +      includeVariations: FONT_VARIATIONS_ENABLED,

We need to make this depend on whether the back-end even supports this option. Backward compatibility is a concern everytime we do a server-side protocol specification change.

Quite simply, assume this code lands in 61, and you then run a Firefox nightly 61 instance, but connect remotely to an Android device which, say, runs Firefox 57. 
In that case, the actor and spec changes you made won't even exist there on the server. And your request will fail.

In many cases, we can use the `actorHasMethod` to detect server-side capabilities, but not here, because you did not introduce a new method.
Instead, you can add a trait in the PageStyleActor (which already returns a traits property as part of its form).
And then use this here to know whether or not this option should be included.

Note that if the trait is missing, you should not just pass `includeVariations:false` in the options, you should completely omit the property altogether, because the old spec doesn't allow for it.

In fact, you can remove checking the pref at all here (`layout.css.font-variations.enabled`) because this is client-code, which may have variable fonts, while the server doesn't.

::: devtools/server/actors/styles.js:324
(Diff revision 2)
>            data: LongStringActor(this.conn, dataURL),
>            size: size
>          };
>        }
> +
> +      if (options.includeVariations) {

Here on the server, however, you should also check for the pref. Only return these things if the option is set *and* if the right pref is ON.

Comment 6

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8954559 [details]
Bug 1441622 - Expose the OpenType Font Variations data to the fonts redux store.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/223608/diff/2-3/

Comment 7

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8954559 [details]
Bug 1441622 - Expose the OpenType Font Variations data to the fonts redux store.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/223608/diff/3-4/

Comment 8

[Patrick Brosset <:pbro>]

Comment on attachment 8954559 [details]
Bug 1441622 - Expose the OpenType Font Variations data to the fonts redux store.

https://reviewboard.mozilla.org/r/223608/#review230188

::: devtools/server/actors/styles.js:125
(Diff revisions 2 - 4)
>          getAppliedCreatesStyleCache: true,
>          // Whether addNewRule accepts the editAuthored argument.
> -        authoredStyles: true
> +        authoredStyles: true,
> +        // Whether getAllUsedFontFaces/getUsedFontFaces accepts the includeVariations
> +        // argument.
> +        fontVariations: true,

I think this should depend on FONT_VARIATIONS_ENABLED.
This way the client won't even try to get the extra data if the pref is not ON on the server.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e915e01f05d
Expose the OpenType Font Variations data to the fonts redux store. r=pbro

Comment 10

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/2e915e01f05d