Closed Bug 1441622 Opened 3 years ago Closed 3 years ago

Expose the OpenType Font Variations data to the fonts redux store

Categories

(DevTools :: Inspector, enhancement, P3)

enhancement

Tracking

(firefox60 fixed)

RESOLVED FIXED
Firefox 60
Tracking Status
firefox60 --- fixed

People

(Reporter: gl, Assigned: gl)

References

Details

Attachments

(1 file)

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 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 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.
Attachment #8954559 - Flags: review?(pbrosset)
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.
Attachment #8954559 - Flags: review?(pbrosset) → review+
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e915e01f05d
Expose the OpenType Font Variations data to the fonts redux store. r=pbro
https://hg.mozilla.org/mozilla-central/rev/2e915e01f05d
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Product: Firefox → DevTools
Component: Inspector: Fonts → Inspector
You need to log in before you can comment on or make changes to this bug.