Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add better support for AppOptions with multiple types (PR 18465 follow-up) #18480

Merged

Conversation

Snuffleupagus
Copy link
Collaborator

For options with varying types, see useSystemFonts, we're not sufficiently validating the type when setting a new value. This means that for an option that uses OptionKind.UNDEF_ALLOWED we'd allow any value, which is obviously not the intention.

Hence we instead introduce a new and optional type-field that allows specifying exactly which types are valid when multiple ones are supported.

Note: This obviously didn't occur to me until after PR #18465 landed, sorry about that!

@Snuffleupagus
Copy link
Collaborator Author

/botio-linux preview

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_preview from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/b79e4a2147287dc/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/b79e4a2147287dc/output.txt

Total script time: 1.04 mins

Published

Copy link
Contributor

@timvandermeij timvandermeij left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me, with one optional comment. Thank you for improving this!

web/app_options.js Outdated Show resolved Hide resolved
…w-up)

For options with varying types, see `useSystemFonts`, we're not sufficiently validating the type when setting a new value. This means that for an option that uses `OptionKind.UNDEF_ALLOWED` we'd allow *any* value, which is obviously not the intention.

Hence we instead introduce a new and *optional* `type`-field that allows specifying exactly which types are valid when multiple ones are supported.

*Note:* This obviously didn't occur to me until after PR 18465 landed, sorry about that!
@Snuffleupagus Snuffleupagus force-pushed the AppOptions-optional-type branch from 53ca512 to b3ea789 Compare July 22, 2024 15:59
@timvandermeij
Copy link
Contributor

/botio-linux preview

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://54.241.84.105:8877/44c9dc69565b48d/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/44c9dc69565b48d/output.txt

Total script time: 1.06 mins

Published

@timvandermeij timvandermeij merged commit 5184a38 into mozilla:master Jul 22, 2024
7 checks passed
@Snuffleupagus Snuffleupagus deleted the AppOptions-optional-type branch July 22, 2024 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants