1616752 - Add checks to the UrlbarResult constructor for each result type

Status: RESOLVED FIXED

(Firefox :: Address Bar, enhancement, P3)

Description

[Drew Willcoxon :adw]

It's probably a good idea to add checks to the UrlbarResult constructor to reject results that have missing or incorrect properties according to their types. For example, tip results need a type property. See discussion here: https://phabricator.services.mozilla.com/D62256#inline-379647

Comment 1

[Drew Willcoxon :adw]

Another point that's related to this came up in the data review in bug 1608461 comment 42. For tips, we have the urlbar.tips keyed scalar telemetry. When a tip's main button and help button are picked, UrlbarInput automatically increments the keys <type>-picked and <type>-help, where <type> is the tip's payload.type [1]. So if we add a new tip type, we're automatically adding new keys to this scalar and therefore we need an expanded data collection review.

So two things, which we can probably do in this bug:

• We should enforce that all TIP results have a payload.type (i.e., throw an error if one isn't given)
• Wherever we end up enforcing that, we should also comment that adding new types automatically adds new keys to urlbar.tips and therefore requires expanded data collection review, as Tim requested in bug 1608461

[1] We should also probably automatically increment <type>-shown but we don't currently. The search tips and interventions providers manually do that. I'll file a bug.

Comment 2

[Drew Willcoxon :adw]

I filed bug 1617318.

Comment 3

[Drew Willcoxon :adw]

Attachment: Bug 1616752 - Part 2: Add payload schemas for each result type and validate payloads against them.

Comment 4

[Drew Willcoxon :adw]

Attachment: Bug 1616752 - Part 1: Improve JsonSchemaValidator.

Comment 5

[Drew Willcoxon :adw]

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

Comment 6

[Drew Willcoxon :adw]

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

Comment 7

[Drew Willcoxon :adw]

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

Comment 8

[Drew Willcoxon :adw]

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

Comment 9

[Drew Willcoxon :adw]

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/41590d1f096e
Part 1: Improve JsonSchemaValidator. r=mak,mythmon,mkaply
https://hg.mozilla.org/integration/autoland/rev/4f7300ad6e99
Part 2: Add payload schemas for each result type and validate payloads against them. r=mak

Comment 11

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/41590d1f096e
https://hg.mozilla.org/mozilla-central/rev/4f7300ad6e99