1271483 - [Decoder Doctor] Add telemetry about notifications shown, clicks, and conversion rate

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P2)

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Add telemetry to judge the usefulness of Decoder Doctor (times notification is shown, clicked, and issue is fixed).

Suggested telemetry keywords:
https://docs.google.com/document/d/1BUHV-kQ3Kf9fC614CTjPxz_PaNerlNJerMQ8-t3e3lM/edit#heading=h.3h1imi7ehzv6

We should uplift to 48 if possible, where Decoder Doctor is already enabled for the Widevine-without-WMF-nor-Silverlight case.

Comment 1

[Benjamin Smedberg]

https://reviewboard.mozilla.org/r/55094/#review51770

I'm not sure what will happen when I submit this from reviewboard, but I'll fix up the flags to reflect a data-review-

::: toolkit/components/telemetry/Histograms.json:6440
(Diff revision 1)
>      "description": "The H.264 profile number (profile_idc) as extracted from the codecs parameter passed to HTMLMediaElement.canPlayType."
>    },
> +  "DECODER_DOCTOR_INFOBAR_STATS": {
> +    "alert_emails": ["[e-mail]"],
> +    "bug_numbers": [1271483],
> +    "expires_in_version": "58",

58 is probably too long for this. The standard period for an expiring measurement is 6 months/4 releases, with renewal always a possibility. Even a year would be 8 releases.

::: toolkit/components/telemetry/Histograms.json:6444
(Diff revision 1)
> +    "bug_numbers": [1271483],
> +    "expires_in_version": "58",
> +    "kind": "enumerated",
> +    "keyed": true,
> +    "n_values": 8,
> +    "description": "Decoder Doctor notifications, keyed by issue. 0=total shown, 1=shown per user, 2=total clicked, 3=clicked per user, 4=issue solved after infobar shown."

From this description, I really can't tell what this will record. This needs to be improved.

What exactly does this record as "per user"? And why do we need to record that separately rather than just aggregating per user? Does this reset or is it permanent?

You need to document the key values. How many possible key values are there? If it's an enumerated list, I think it would be far better to organize this along the other dimension:

each an enumerated histogram by problem type, not keyed:
DECODER_DOCTOR_INFOBAR_SHOWN
DECODER_DOCTOR_INFOBAR_CLICKED
DECODER_DOCTOR_INFOBAR_SOLVED_AFTER

Comment 2

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

https://reviewboard.mozilla.org/r/55100/#review51750

I will add a comment block explaining what's in parsedData.
If still unclear, please ping me.

> Can we call 'details' something more descriptive? What is it?

It's a dom.properties key, but is only used here to construct pref names, and as telemetry key.
Not really sure what else to call it, as it does represent a more-detailed description of the issue. Hopefully with the new comment block above it will make enough sense to keep 'details'.
(Or would you have suggestions?)

> You always falsy-check formatsInPref, so there seems to be no point in assigning it empty string.

Nice one, thank you.

> You can just call existing.filter() - filter() does not modify the array on which it is called.
> 
> ... in fact, this code doesn't make sense:
> 
> 1. You take a set of items, let's say the pref is "a,b,c".
> 2. You create an array, now you have ["a", "b", "c"]
> 3. You then create an identical array, the same way you created the first.
> 4. You filter out any items that are not in the first array. But you created the second array the same way as the first, so obviously all items in the second one are also in the first one, and after filtering your array will be empty...

Oops, the second array should be based on 'formats' (the new list of formats coming from parsedData).
So hopefully with step 3 now being 'you then create a array made from old&new items, say ["b", "d", "f"]', it makes more sense now, and I can keep filter() as is, to only keep the new items (["d", "f"]).

> Arrays are never falsy. You want to check newbies.length, I guess?

Should we fix Javascript to make empty arrays falsy? :-)

> Prevailing style is operator last, so:
> 
> ```
> (a &&
>  b) ||
> c
> ```
> 
> but this would IMO be easier to read as a ternary.

I was initially afraid a ternary would miss one case, but in fact it's fine, so ternary it is now.

> This if is lonely. Please change this to use:
> 
> ```
> } else if (formatsInPref) {
> ```
> 
> instead.

Done, with added comment to explain the test.

Comment 3

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

https://reviewboard.mozilla.org/r/55094/#review51770

> 58 is probably too long for this. The standard period for an expiring measurement is 6 months/4 releases, with renewal always a possibility. Even a year would be 8 releases.

Ok, will change to 53.

> From this description, I really can't tell what this will record. This needs to be improved.
> 
> What exactly does this record as "per user"? And why do we need to record that separately rather than just aggregating per user? Does this reset or is it permanent?
> 
> You need to document the key values. How many possible key values are there? If it's an enumerated list, I think it would be far better to organize this along the other dimension:
> 
> each an enumerated histogram by problem type, not keyed:
> DECODER_DOCTOR_INFOBAR_SHOWN
> DECODER_DOCTOR_INFOBAR_CLICKED
> DECODER_DOCTOR_INFOBAR_SOLVED_AFTER

I'll put more details in the description.

"Per user" means "first time shown/clicked per profile", and it is reset after the issue solved (which should hopefully be a one-time thing).

The key values are dom.properties keys that describe the issue, e.g. "MediaWMFNeeded", "MediaWidevineNoWMFNoSilverlight". More of these may be created in the future, so I can't build an enumerated list right now.
And for each issue, we want to know how many times we displayed the notification (first time per user=profile, and total times), how many times the "show me how" button was clicked, and then how many times the issue was resolved (which we would like to be as close to 'shown per user' as possible).

If all this still doesn't make sense, please ping me -- It's my first probe, so maybe *I* don't quite understand what is needed...

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756343 [details]
Bug 1271483 - p7. Telemetry histogram for DecDoc infobar stats - , f=bsmedberg

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

Comment 5

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756344 [details]
Bug 1271483 - p8. More details in DecoderDoctorNotification -

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

Comment 6

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756345 [details]
Bug 1271483 - p9. Report more details from DecDecDiagnostics -

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

Comment 7

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756346 [details]
Bug 1271483 - p10. Front-end handling of prefs&telemetry -

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

Comment 8

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756347 [details]
Bug 1271483 - p11. Implement comma-sep-string for-range iterator -

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

Comment 9

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756348 [details]
Bug 1271483 - p12. Use StringListContains to filter notifications -

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

Comment 10

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756349 [details]
Bug 1271483 - p13. Rework formats list -

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

Comment 11

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756350 [details]
Bug 1271483 - p14. Demagicify ReportStringId strings -

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

Comment 12

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756351 [details]
Bug 1271483 - p15. Check if issues have been solved -

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

Comment 13

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756343 [details]
Bug 1271483 - p7. Telemetry histogram for DecDoc infobar stats - , f=bsmedberg

See comment 21 (or reviewboard) for my responses to your first feedback.
Happy to discuss further if that is still not satisfactory...

Comment 14

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1271483 - p1. Optionally treat media.wmf.disabled as WMF failure for Decoder Doctor -

This can be used to test Decoder Doctor on Windows, but disabling WMF through
prefs, to trigger an Decoder Doctor infobar.

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

Comment 15

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1271483 - p2. Fixed whitespace style -

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

Comment 16

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1271483 - p3. constify loop variable -

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

Comment 17

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1271483 - p4. Fixed console reporting for Widevine case -

Message was not including the actual Key System at issue.

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

Comment 18

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1271483 - p5. Remove 'canPlay', can be inferred otherwise -

Instead of having yet-another variable, just infer playability from the lists
of playable&unplayable formats, which we need to look at anyway.

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

Comment 19

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1271483 - p6. Separate unplayable formats by missing decoder -

This helps determine how each format is affected by some issues. It will be
needed in later patches, to see when the issue get fixed (by noticing that
these formats become playable again).

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

Comment 20

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1271483 - p7. Telemetry histogram for DecDoc infobar stats - , f=bsmedberg

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

Comment 21

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1271483 - p8. More details in DecoderDoctorNotification -

Added some fields required by telemetry. Now with some documentation!
(Notifying code in p9, handling code in p10.)

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

Comment 22

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1271483 - p9. Report more details from DecDecDiagnostics -

Fill up extra details in notification: Web-console notification string id (will
be used as telemetry key), and whether the issue is present or has been solved.

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

Comment 23

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1271483 - p10. Front-end handling of prefs&telemetry -

When an issue is reported, save the unplayable formats/key-systesm in a pref,
keyed by the detailed issue string id, and report the infobar-shown telemetry.
More telemetry when the "Show me how" button is clicked.
And final telemetry (and clearing the prefs) when the issue is solved.

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

Comment 24

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1271483 - p11. Implement comma-sep-string for-range iterator -

Utility class&function that creates an iteratable range from a comma-separated
string, where each iteration will give a dependent substring (i.e., no string
copy happening).
This will help with going through lists of unplayable formats, to see if
issues have been solved.
If useful enough, this could later be published to a more public location
(e.g. mfbt or xpcom).

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

Comment 25

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1271483 - p12. Use StringListContains to filter notifications -

Using string-list iterator instead of bespoke code, to go through the decoder
doctor notifications-allowed pref.

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

Comment 26

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1271483 - p13. Rework formats list -

Renamed AppendToStringList to AppendToFormatsList, to distinguish from other
string-based lists.
Ensure that list items don't contain commas, as commas are used as separators,
and we don't want&need to introduce escaping.
Added FormatsListContains.

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

Comment 27

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1271483 - p14. Demagicify ReportStringId strings -

Combine notification type and web-console string id into structs, simpler to
pass around, and will be useful to go through them when checking for solved
issues.

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

Comment 28

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1271483 - p15. Check if issues have been solved -

To determine if an issue has been solved, go through the possible prefs that
would have been saved by the front-end, to see if any previously-unplayable
formats/key systems are now playable, in which case we notify the frontend, to
record the issue-solved telemetry.

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

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8756344 [details]
Bug 1271483 - p8. More details in DecoderDoctorNotification -

https://reviewboard.mozilla.org/r/55096/#review51746

This doesn't seem to be exposed to the web, so r+

Comment 30

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756343 [details]
Bug 1271483 - p7. Telemetry histogram for DecDoc infobar stats - , f=bsmedberg

Requesting approval for new telemetry probe: DECODER_DOCTOR_INFOBAR_STATS

This probe will help determine if Decoder Doctor is being useful, i.e., if notifications lead to clicks to the SUMO page, and eventually to resolutions of the reported media issues.
There is no sensitive or personal information, only a count of times notifications were shown (and a broad reason, e.g. "Windows Media Foundation not found"), "show me how" buttons clicked, and when a resolutions are found.

More context: https://docs.google.com/document/d/1BUHV-kQ3Kf9fC614CTjPxz_PaNerlNJerMQ8-t3e3lM/edit?pref=2&pli=1#heading=h.3h1imi7ehzv6

Comment 31

[:Gijs (he/him)]

Comment on attachment 8756346 [details]
Bug 1271483 - p10. Front-end handling of prefs&telemetry -

https://reviewboard.mozilla.org/r/55100/#review51750

There isn't really enough information in this patch for me to review this very well. What is this `parsedData` that comes in, and what messages indicate what, and what do we want to record in telemetry about it?

Either way, AFAICT the code here is broken - I don't think it ever writes anything to the prefs because the lists of 'existing' and the base for 'newbies' are the same, so then 'newbies' is empty... - so no r+. :-(

::: browser/base/content/browser-media.js:236
(Diff revision 1)
>        parsedData = JSON.parse(data);
>      } catch (ex) {
>        Cu.reportError("Malformed Decoder Doctor message with data: " + data);
>        return;
>      }
> -    let {type} = parsedData;
> +    let {type, isSolved, details, formats} = parsedData;

Can we call 'details' something more descriptive? What is it?

::: browser/base/content/browser-media.js:250
(Diff revision 1)
> +    let histogram =
> +      Services.telemetry.getKeyedHistogramById("DECODER_DOCTOR_INFOBAR_STATS");
> +
> +    let formatsInPref = (Services.prefs.getPrefType(formatsPref)
> +                         && Services.prefs.getCharPref(formatsPref))
> +                        || "";

You always falsy-check formatsInPref, so there seems to be no point in assigning it empty string.

::: browser/base/content/browser-media.js:264
(Diff revision 1)
> +        let newbies = formatsInPref.split(",")
> +                      .map(String.trim)
> +                      .filter(x => existing.indexOf(x) < 0);

You can just call existing.filter() - filter() does not modify the array on which it is called.

... in fact, this code doesn't make sense:

1. You take a set of items, let's say the pref is "a,b,c".
2. You create an array, now you have ["a", "b", "c"]
3. You then create an identical array, the same way you created the first.
4. You filter out any items that are not in the first array. But you created the second array the same way as the first, so obviously all items in the second one are also in the first one, and after filtering your array will be empty...

::: browser/base/content/browser-media.js:268
(Diff revision 1)
> +        // Keep given formats that were not already recorded.
> +        let newbies = formatsInPref.split(",")
> +                      .map(String.trim)
> +                      .filter(x => existing.indexOf(x) < 0);
> +        // And rewrite pref with the added new formats (if any).
> +        if (newbies) {

Arrays are never falsy. You want to check newbies.length, I guess?

::: browser/base/content/browser-media.js:269
(Diff revision 1)
> +          Services.prefs.setCharPref(formatsPref,
> +                                       existing.concat(newbies).join(", "));

Indenting is off here.

::: browser/base/content/browser-media.js:281
(Diff revision 1)
> +            let clickedInPref = (Services.prefs.getPrefType(buttonClickedPref)
> +                                 && Services.prefs.getIntPref(buttonClickedPref))
> +                                || 0;

Prevailing style is operator last, so:

```
(a &&
 b) ||
c
```

but this would IMO be easier to read as a ternary.

::: browser/base/content/browser-media.js:306
(Diff revision 1)
> +      if (formatsInPref) {
> +        Services.prefs.clearUserPref(formatsPref);
> +        Services.prefs.clearUserPref(buttonClickedPref);
> +        histogram.add(details, TELEMETRY_DDSTAT_SOLVED);
> +      }

This if is lonely. Please change this to use:

```
} else if (formatsInPref) {
```

instead.

Comment 32

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8756337 [details]
Bug 1271483 - p1. Optionally treat media.wmf.disabled as WMF failure for Decoder Doctor -

https://reviewboard.mozilla.org/r/55082/#review51946

Comment 33

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8756338 [details]
Bug 1271483 - p2. Fixed whitespace style -

https://reviewboard.mozilla.org/r/55084/#review51948

Comment 34

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8756339 [details]
Bug 1271483 - p3. constify loop variable -

https://reviewboard.mozilla.org/r/55086/#review51952

Comment 35

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8756340 [details]
Bug 1271483 - p4. Fixed console reporting for Widevine case -

https://reviewboard.mozilla.org/r/55088/#review51954

Comment 36

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8756341 [details]
Bug 1271483 - p5. Remove 'canPlay', can be inferred otherwise -

https://reviewboard.mozilla.org/r/55090/#review51960

Comment 37

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8756342 [details]
Bug 1271483 - p6. Separate unplayable formats by missing decoder -

https://reviewboard.mozilla.org/r/55092/#review51964

Comment 38

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8756343 [details]
Bug 1271483 - p7. Telemetry histogram for DecDoc infobar stats - , f=bsmedberg

https://reviewboard.mozilla.org/r/55094/#review51968

Comment 39

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8756345 [details]
Bug 1271483 - p9. Report more details from DecDecDiagnostics -

https://reviewboard.mozilla.org/r/55098/#review51986

Comment 40

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8756347 [details]
Bug 1271483 - p11. Implement comma-sep-string for-range iterator -

https://reviewboard.mozilla.org/r/55102/#review51988

Will need tests if we move this out. Arguably it should have a gtest anyway.

::: dom/media/DecoderDoctorDiagnostics.cpp:283
(Diff revision 2)
> +          return;
> +        }
> +        auto c = *p;
> +        if (c == CharType(',')) {
> +          // Comma -> Empty item -> Skip.
> +        } else if (c != CharType(' ')) {

In the general case, also consider tab chars, new lines, carriage returns as whitespace.

::: dom/media/DecoderDoctorDiagnostics.cpp:345
(Diff revision 2)
> +
> +template <typename ListString, typename ItemString>
> +static bool
> +StringListContains(const ListString& aList, const ItemString& aItem)
> +{
> +  for (const auto listItem : MakeStringListRange(aList)) {

Would making that "const auto& listItem" work? i.e. make it a const reference instead of a const copy, to avoid having to create a temporary copy of the string items.

Comment 41

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8756348 [details]
Bug 1271483 - p12. Use StringListContains to filter notifications -

https://reviewboard.mozilla.org/r/55104/#review52006

::: dom/media/DecoderDoctorDiagnostics.cpp:423
(Diff revision 2)
>    //                                  dom.properties) is one of them.
>    // - Nothing (missing or empty) -> Disable everything.
>    nsAdoptingCString filter =
>      Preferences::GetCString("media.decoder-doctor.notifications-allowed");
>    filter.StripWhitespace();
>    bool allowed = false;

Couldn't you make this if/else/if block just:

if (filter && (
    filter.EqualsLiteral("*") ||
    StringListContains(filter, aReportStringId))) {
  DispatchNotification(
    mDocument->GetInnerWindow(),
    aNotificationType, aIsSolved, aReportStringId, aParams);
}

I assume StringListContains handles an empty string as input? (Should be in its test!)

Comment 42

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8756349 [details]
Bug 1271483 - p13. Rework formats list -

https://reviewboard.mozilla.org/r/55106/#review52008

Comment 43

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8756350 [details]
Bug 1271483 - p14. Demagicify ReportStringId strings -

https://reviewboard.mozilla.org/r/55108/#review52012

Comment 44

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8756351 [details]
Bug 1271483 - p15. Check if issues have been solved -

https://reviewboard.mozilla.org/r/55110/#review52016

Comment 45

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

https://reviewboard.mozilla.org/r/55102/#review51988

Definitely needs tests when published, I'll see if I can produce some quickly before my PTO...

> In the general case, also consider tab chars, new lines, carriage returns as whitespace.

In the general case, I'll parameterize separators and whitespace (and default whitespace as you suggest).
I might actually separate this trimming from iterating over items, for a more composable design.

> Would making that "const auto& listItem" work? i.e. make it a const reference instead of a const copy, to avoid having to create a temporary copy of the string items.

Oops, the dog ate my ref! Thank you.
(In this particular case, it probably would have had low impact, as the dependent substrings returned by the iterator are low-weight; but better get it right of course.)

Comment 46

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

https://reviewboard.mozilla.org/r/55104/#review52006

> Couldn't you make this if/else/if block just:
> 
> if (filter && (
>     filter.EqualsLiteral("*") ||
>     StringListContains(filter, aReportStringId))) {
>   DispatchNotification(
>     mDocument->GetInnerWindow(),
>     aNotificationType, aIsSolved, aReportStringId, aParams);
> }
> 
> I assume StringListContains handles an empty string as input? (Should be in its test!)

Much simpler this way, thank you. In fact the first test is not even needed.
And yes StringListContains handles empty strings, I've manually tested it.

Comment 47

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756337 [details]
Bug 1271483 - p1. Optionally treat media.wmf.disabled as WMF failure for Decoder Doctor -

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

Comment 48

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756338 [details]
Bug 1271483 - p2. Fixed whitespace style -

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

Comment 49

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756339 [details]
Bug 1271483 - p3. constify loop variable -

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

Comment 50

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756340 [details]
Bug 1271483 - p4. Fixed console reporting for Widevine case -

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

Comment 51

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756341 [details]
Bug 1271483 - p5. Remove 'canPlay', can be inferred otherwise -

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

Comment 52

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756342 [details]
Bug 1271483 - p6. Separate unplayable formats by missing decoder -

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

Comment 53

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756343 [details]
Bug 1271483 - p7. Telemetry histogram for DecDoc infobar stats - , f=bsmedberg

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

Comment 54

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756344 [details]
Bug 1271483 - p8. More details in DecoderDoctorNotification -

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

Comment 55

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756345 [details]
Bug 1271483 - p9. Report more details from DecDecDiagnostics -

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

Comment 56

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756346 [details]
Bug 1271483 - p10. Front-end handling of prefs&telemetry -

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

Comment 57

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756347 [details]
Bug 1271483 - p11. Implement comma-sep-string for-range iterator -

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

Comment 58

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756348 [details]
Bug 1271483 - p12. Use StringListContains to filter notifications -

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

Comment 59

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756349 [details]
Bug 1271483 - p13. Rework formats list -

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

Comment 60

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756350 [details]
Bug 1271483 - p14. Demagicify ReportStringId strings -

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

Comment 61

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756351 [details]
Bug 1271483 - p15. Check if issues have been solved -

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

Comment 62

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756343 [details]
Bug 1271483 - p7. Telemetry histogram for DecDoc infobar stats - , f=bsmedberg

(Reviving f? after review updates)

See comment 21 (or reviewboard) for my responses to your first feedback.
Happy to discuss further if that is still not satisfactory...

Comment 63

[:Gijs (he/him)]

Comment on attachment 8756346 [details]
Bug 1271483 - p10. Front-end handling of prefs&telemetry -

https://reviewboard.mozilla.org/r/55100/#review52132

::: browser/base/content/browser-media.js:240
(Diff revision 3)
> +    // - 'details' is a more-detailed issue identifier, to be used as key for
> +    //   the telemetry (counting infobar displays, "show me how" buttons
> +    //   clicked, and resolutions) and for the prefs used to store at-issue
> +    //   formats.

Is there a list of what issues we support? I'm a bit surprised that there are apparently different ones, but the notification we show to the user is always based on 'type' and not 'details', but the pref is based on 'details'...

::: browser/base/content/browser-media.js:249
(Diff revision 3)
> +    // - 'formats' contains a comma-separated list of formats (or key systems)
> +    //   that suffer the issue. These are kept in a pref, which the backend
> +    //   uses to later find when an issue is resolved.
> +    // - 'isSolved' is true when the notification actually indicates the
> +    //   resolution of that issue, to be reported as telemetry.
> +    let {type, isSolved, details, formats} = parsedData;

Can't we make `formats` an actual array in the JSON, rather than the string that's really an array? Is that hard in the C++ dispatcher of this information or something?

::: browser/base/content/browser-media.js:256
(Diff revision 3)
> +    let formatsPref = "media.decoder-doctor." + details + ".formats";
> +    let buttonClickedPref = "media.decoder-doctor." + details + ".button-clicked";

I guess this means we're duplicating the information in `type` into `details`, otherwise these identifiers would be the same for different `type` things? That seems inefficient... can we include `type` in the pref name and ensure there's no duplication between `type` and `details`? (Conversely, if there isn't duplication, shouldn't `type` be included in the formats pref ?)

It would be easier to suggest different naming if I knew the current list of potential contents of the variable. That is, what are all the different values we use for "type" and "details" ?

In fact, I continue to find this confusing. Let's assume that type and details together identify something like "codec not found". Let's further suppose that this initially happens to some user on a website with theora, and later with h264.

Now the format pref for `media.decoder-doctor.codec-not-found.formats` or something is going to be:

"theora,h264"

right?

Then let's say that the user clicks the button to solve this issue for h264. Now we're going to send an "isSolved" notification, which will clear the pref completely, and so telemetry will be pretending that the issue isn't there for ogg theora, even though it never explicitly got 'solved'. Is that intentional? Can you elaborate on how that is more useful than tracking things for each codec individually?

I guess my understanding of the issue would also be helped by a test (that fakes the different types of data that we send to the browser frontend - I think Jared wrote one for the initial implementation?), which would also help with verifying this behaves as you expect. :-)

Sorry to be such a pain when reviewing this!

::: browser/base/content/browser-media.js:282
(Diff revision 3)
> +                      .map(String.trim)
> +                      .filter(x => existing.indexOf(x) < 0);
> +        // And rewrite pref with the added new formats (if any).
> +        if (newbies.length) {
> +          Services.prefs.setCharPref(formatsPref,
> +                                     existing.concat(newbies).join(", "));

Nit: might as well not have the space after the comma and get rid of the String.trim() everywhere, assuming we're the only people writing to this pref?

Comment 64

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1271483 - p16. gtest for list-string functions -

Because it is templated code inside a cpp, the test includes a copy of that
code.
When we make this more public, the test should then include the proper header
instead.

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

Comment 65

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

https://reviewboard.mozilla.org/r/55100/#review52132

> Is there a list of what issues we support? I'm a bit surprised that there are apparently different ones, but the notification we show to the user is always based on 'type' and not 'details', but the pref is based on 'details'...

Please see patch 14, starting at line 358: https://reviewboard.mozilla.org/r/55108/diff/3#0
It's the list of all possible type&details pairs.
(See comment below for more info.)

> Can't we make `formats` an actual array in the JSON, rather than the string that's really an array? Is that hard in the C++ dispatcher of this information or something?

Anything is possible of course, but since this list is going through strings at least twice (as JSON in the notification, and as string in the prefs), it seemed easier to me at the start to just work on strings everywhere.
It probably saves a bit of memory in C++ (one string object instead of many), and the processing needed to iterate through items would happen anyway when I would need to convert the pref string into an array of strings.

I'd be happy to revisit this in a separate bug if you agree.
(Full disclosure: I'm trying to land this this week before I go on PTO until mozlondon, hence my eagerness to get it through! But of course, if it's not acceptable code it should not land.)

> I guess this means we're duplicating the information in `type` into `details`, otherwise these identifiers would be the same for different `type` things? That seems inefficient... can we include `type` in the pref name and ensure there's no duplication between `type` and `details`? (Conversely, if there isn't duplication, shouldn't `type` be included in the formats pref ?)
> 
> It would be easier to suggest different naming if I knew the current list of potential contents of the variable. That is, what are all the different values we use for "type" and "details" ?
> 
> In fact, I continue to find this confusing. Let's assume that type and details together identify something like "codec not found". Let's further suppose that this initially happens to some user on a website with theora, and later with h264.
> 
> Now the format pref for `media.decoder-doctor.codec-not-found.formats` or something is going to be:
> 
> "theora,h264"
> 
> right?
> 
> Then let's say that the user clicks the button to solve this issue for h264. Now we're going to send an "isSolved" notification, which will clear the pref completely, and so telemetry will be pretending that the issue isn't there for ogg theora, even though it never explicitly got 'solved'. Is that intentional? Can you elaborate on how that is more useful than tracking things for each codec individually?
> 
> I guess my understanding of the issue would also be helped by a test (that fakes the different types of data that we send to the browser frontend - I think Jared wrote one for the initial implementation?), which would also help with verifying this behaves as you expect. :-)
> 
> Sorry to be such a pain when reviewing this!

You can see the list of all possible type&details pairs in patch 14, starting at line 358: https://reviewboard.mozilla.org/r/55108/diff/3#0

You will see for example that what comes as type 'Platform decoder not found' can have different 'details', e.g. MediaWMFNeeded or MediaWidevineNoWMFNoSilverlight.
Put another way, 'details' is a particular case that DecoderDoctor detects, but the type of issue and therefore its resolution is actually the same, and the user should only be shown that higher-level notification.

The distinction is necessary in DecoderDoctor, because the way we find out about MediaWidevineNoWMFNoSilverlight is when some encryption key system (e.g. "widevine") is requested, but MediaWMFNeeded happens when some format (e.g. "video/mp4") is checked for support.
But once again the underlying issue is the same: WMF is missing, so we only need to tell the user that.
However for telemetry, we *do* want to distinguish between these situations, if only for statistical purposes, to get a sense of what's happening in the field, and if we should make extra efforts in some situations.

When the user clicks on the "Learn how" button, we only redirect them to the SUMO page, there is no resolution yet. But we still want to tie the "clicked" telemetry to the "shown" one, once again keyed on details.

Later, when the user actually corrects the issue (outside of Firefox), DecoderDoctor (in C++) only detects the resolution when one of the previously-failing formats or key systems now works.
But to know that they were previously failing, we need to keep them somewhere.
And also for telemetry purposes, we want to tie the resolution with the "shown" statistics, and that's why we want to store the previously-failing formats in details-specific prefs.
(It doesn't matter that we're clearing other formats in a specific 'details' prefs, we just know that the issue was solved, we tell telemetry, and we forget about the issue.)
You can see that resolution detection in patch 15, starting at line 578: https://reviewboard.mozilla.org/r/55110/diff/3#0

I agree that a test would be great, but it's a bit difficult to put in place just yet, though I'm thinking of ways to do it... (Once again trying to beat my PTO, and start collecting some telemetry data.)
I did test this manually.

Does this explanation help you understand what I'm trying to do?
If yes, would you like to see more information in the code?
If not (yet), what can I do to help?

> Nit: might as well not have the space after the comma and get rid of the String.trim() everywhere, assuming we're the only people writing to this pref?

Removing that space would also require modifying C++ code. I'd prefer to do that in a separate bug if you don't mind.

Comment 66

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756346 [details]
Bug 1271483 - p10. Front-end handling of prefs&telemetry -

Gijs, please see comment 65 or review board for my responses to your review comments. I'd appreciate if you could have another look, see if my explanations are satisfactory (or not yet).

Comment 67

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Try with new gtest:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e643a75c815aafd617266cdd2b52e2c6bbecf7b

Comment 68

[:Gijs (he/him)]

Comment on attachment 8756346 [details]
Bug 1271483 - p10. Front-end handling of prefs&telemetry -

https://reviewboard.mozilla.org/r/55100/#review52152

::: browser/base/content/browser-media.js:240
(Diff revision 3)
> +    // - 'details' is a more-detailed issue identifier, to be used as key for
> +    //   the telemetry (counting infobar displays, "show me how" buttons
> +    //   clicked, and resolutions) and for the prefs used to store at-issue
> +    //   formats.

Having read the squashed diff a bit, can we just call details 'doctorReportId' or something like this?

::: browser/base/content/browser-media.js:256
(Diff revision 3)
> +    let formatsPref = "media.decoder-doctor." + details + ".formats";
> +    let buttonClickedPref = "media.decoder-doctor." + details + ".button-clicked";

(so mozreview just threw away ~6 parapgraphs I wrote here. If I am terse when rewriting them, I apologize. I filed bug 1275865.)

Yes, the explanation helps. I ended up just looking at the squashed diff as well, because on its own I continued to be confused by this code. The pattern seems to be like this:

1) C++ code finds broken things, sends notification
2) JS code writes pref, sends telemetry
3) C++ code might find working stuff, reads pref, sends notification
4) JS code sends more telemetry and clears the pref

And orthogonally, the JS code keeps track of the button clicks (more on that below).

I think the correct way would have been to put the telemetry and pref-writing in the C++ code as well. There is nothing here for which we need the frontend, and in fact, if you're adding notification types for which there is no string in the localization files, we won't write prefs and the steps above break (see line 252 for the early return if block).

As it is, from the JS code's perspective, we could just as well use a bool pref instead of the string, the format concatenations etc. - the telemetry doesn't need the formats or the pref (beyond a 'is this the first time we're sending it for this `details` value' check, which could be a bool). We only need those because of the C++ code which also reads that pref, and determines if something was 'solved'.

This also means that if this code runs in e.g. Thunderbird or in <browser> elements that aren't in the main browser window, we won't write prefs and record telemetry. I don't think that really breaks anything beyond those two things, but it's something to keep in mind.

Though that all seems a lot simpler, it doesn't quite work that way, I suppose, because you can't write prefs (and maybe not telemetry either? Not sure...) from the content process.

IMO for the sake of separation of concerns, it would still be better if the "write this list of formats to the prefs and send telemetry" stuff lived in dom/media/ code, but I don't know how much work it is to do that. :-\

In the meantime, please add a comment here that we keep the list of formats in prefs for the sake of the decoder itself, which reads it to determine which issues got solved.

::: browser/base/content/browser-media.js:277
(Diff revision 3)
> +      } else {
> +        // Split existing formats into an array of strings.
> +        let existing = formatsInPref.split(",").map(String.trim);
> +        // Keep given formats that were not already recorded.
> +        let newbies = formats.split(",")
> +                      .map(String.trim)

Nit: this can go on the previous line.

::: browser/base/content/browser-media.js:278
(Diff revision 3)
> +        // Split existing formats into an array of strings.
> +        let existing = formatsInPref.split(",").map(String.trim);
> +        // Keep given formats that were not already recorded.
> +        let newbies = formats.split(",")
> +                      .map(String.trim)
> +                      .filter(x => existing.indexOf(x) < 0);

Nit: (x => existing.includes(x))

is easier to read.

::: browser/base/content/browser-media.js:282
(Diff revision 3)
> +                      .map(String.trim)
> +                      .filter(x => existing.indexOf(x) < 0);
> +        // And rewrite pref with the added new formats (if any).
> +        if (newbies.length) {
> +          Services.prefs.setCharPref(formatsPref,
> +                                     existing.concat(newbies).join(", "));

The C++ code depends on there being spaces between the items in this pref?? What happens if there aren't spaces? Failing to send telemetry is one thing, but crashing or not working is something else - prefs are easily user-modifiable in about:config, and something as innocent as spaces should not really influence the functioning of the code...

::: browser/base/content/browser-media.js:293
(Diff revision 3)
> +            let clickedInPref = Services.prefs.getPrefType(buttonClickedPref) ?
> +                                Services.prefs.getIntPref(buttonClickedPref) :
> +                                0;
> +            if (clickedInPref <= 0) {
> +              Services.prefs.setIntPref(buttonClickedPref, 1);
> +              histogram.add(details, TELEMETRY_DDSTAT_CLICKED_FIRST);
> +            } else {
> +              Services.prefs.setIntPref(buttonClickedPref, clickedInPref + 1);
> +            }
> +            histogram.add(details, TELEMETRY_DDSTAT_CLICKED);
> +

It seems we only need to know if this is the first click, so let's just make this pref a bool and then check if it's set or not. Then we can also avoid setting it again after that, and having the `< 0` check when the value is never going to be smaller than 0 (if set by this code). :-)

Comment 69

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

https://reviewboard.mozilla.org/r/55100/#review52152

> Having read the squashed diff a bit, can we just call details 'doctorReportId' or something like this?

For some reason I was intent on hiding its identify from the JS code. :-)
But fair enough, let's call it for what it really is, hopefully it might save some confusion next time.

> (so mozreview just threw away ~6 parapgraphs I wrote here. If I am terse when rewriting them, I apologize. I filed bug 1275865.)
> 
> Yes, the explanation helps. I ended up just looking at the squashed diff as well, because on its own I continued to be confused by this code. The pattern seems to be like this:
> 
> 1) C++ code finds broken things, sends notification
> 2) JS code writes pref, sends telemetry
> 3) C++ code might find working stuff, reads pref, sends notification
> 4) JS code sends more telemetry and clears the pref
> 
> And orthogonally, the JS code keeps track of the button clicks (more on that below).
> 
> I think the correct way would have been to put the telemetry and pref-writing in the C++ code as well. There is nothing here for which we need the frontend, and in fact, if you're adding notification types for which there is no string in the localization files, we won't write prefs and the steps above break (see line 252 for the early return if block).
> 
> As it is, from the JS code's perspective, we could just as well use a bool pref instead of the string, the format concatenations etc. - the telemetry doesn't need the formats or the pref (beyond a 'is this the first time we're sending it for this `details` value' check, which could be a bool). We only need those because of the C++ code which also reads that pref, and determines if something was 'solved'.
> 
> This also means that if this code runs in e.g. Thunderbird or in <browser> elements that aren't in the main browser window, we won't write prefs and record telemetry. I don't think that really breaks anything beyond those two things, but it's something to keep in mind.
> 
> Though that all seems a lot simpler, it doesn't quite work that way, I suppose, because you can't write prefs (and maybe not telemetry either? Not sure...) from the content process.
> 
> IMO for the sake of separation of concerns, it would still be better if the "write this list of formats to the prefs and send telemetry" stuff lived in dom/media/ code, but I don't know how much work it is to do that. :-\
> 
> In the meantime, please add a comment here that we keep the list of formats in prefs for the sake of the decoder itself, which reads it to determine which issues got solved.

Pref-writing from content is the main reason I had to do this trick, yes. I'll make this clearer.

> Nit: (x => existing.includes(x))
> 
> is easier to read.

I didn't know about 'includes()', thank you.
Oh, I see it's quite new!

> The C++ code depends on there being spaces between the items in this pref?? What happens if there aren't spaces? Failing to send telemetry is one thing, but crashing or not working is something else - prefs are easily user-modifiable in about:config, and something as innocent as spaces should not really influence the functioning of the code...

The C++ code does not *depend* on spaces, but it adds them.
Also I've written a bunch of code that deals with them, and I wouldn't want to throw that away! (But I now use it for another human-written pref, so it's needed anyway.)
So I think we might as well keep them, it's more readable -- The .formats prefs are indeed read by code, but also by developers (i.e., me).

> It seems we only need to know if this is the first click, so let's just make this pref a bool and then check if it's set or not. Then we can also avoid setting it again after that, and having the `< 0` check when the value is never going to be smaller than 0 (if set by this code). :-)

Initially I had planned to keep count of clicks, to attach to the final "solved" telemetry (giving us an idea of how it takes the user to get the hint!)
But you're right, we can simplify this one now.

Comment 70

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756344 [details]
Bug 1271483 - p8. More details in DecoderDoctorNotification -

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

Comment 71

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756345 [details]
Bug 1271483 - p9. Report more details from DecDecDiagnostics -

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

Comment 72

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756346 [details]
Bug 1271483 - p10. Front-end handling of prefs&telemetry -

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

Comment 73

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756347 [details]
Bug 1271483 - p11. Implement comma-sep-string for-range iterator -

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

Comment 74

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756348 [details]
Bug 1271483 - p12. Use StringListContains to filter notifications -

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

Comment 75

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756349 [details]
Bug 1271483 - p13. Rework formats list -

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

Comment 76

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756350 [details]
Bug 1271483 - p14. Demagicify ReportStringId strings -

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

Comment 77

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756351 [details]
Bug 1271483 - p15. Check if issues have been solved -

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

Comment 78

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756783 [details]
Bug 1271483 - p16. gtest for list-string functions -

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

Comment 79

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8756347 [details]
Bug 1271483 - p11. Implement comma-sep-string for-range iterator -

https://reviewboard.mozilla.org/r/55102/#review52346

Comment 80

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8756783 [details]
Bug 1271483 - p16. gtest for list-string functions -

https://reviewboard.mozilla.org/r/55402/#review52348

Would be better to put the function declaration in VideoUtils.h and definition in VideoUtils.cpp, so that there's only one copy of the code. Otherwise if you have two copies, it's easy to make changes to one and forget to update the other.

Comment 81

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756347 [details]
Bug 1271483 - p11. Implement comma-sep-string for-range iterator -

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

Comment 82

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756348 [details]
Bug 1271483 - p12. Use StringListContains to filter notifications -

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

Comment 83

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756349 [details]
Bug 1271483 - p13. Rework formats list -

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

Comment 84

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756350 [details]
Bug 1271483 - p14. Demagicify ReportStringId strings -

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

Comment 85

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756351 [details]
Bug 1271483 - p15. Check if issues have been solved -

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

Comment 86

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756783 [details]
Bug 1271483 - p16. gtest for list-string functions -

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

Comment 87

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

https://reviewboard.mozilla.org/r/55402/#review52348

Yep, much better, done!
It was fully templated, so it all went into VideoUtils.h, not .cpp.

Comment 88

[Benjamin Smedberg]

https://reviewboard.mozilla.org/r/55094/#review53646

::: toolkit/components/telemetry/Histograms.json:6444
(Diff revision 3)
> +    "bug_numbers": [1271483],
> +    "expires_in_version": "53",
> +    "kind": "enumerated",
> +    "keyed": true,
> +    "n_values": 8,
> +    "description": "Decoder Doctor notification events, keyed by type of issue. For each issue, we record: 0=total number of times the notification is shown, 1=number of times it is first shown to a user, 2=total number of times 'Learn how' clicked, 3=first clicked per user, 4=issue solved after infobar has been shown at least once."

If there are five enumeration values being recorded, why is n_values 8? Do you expect to add more events in the future?

This description still doesn't say what keys may contain and how they will be constructed. That is still my biggest concern here. In the bug comments you mention dom.properties keys, but that's not included here and if I'm looking at telemetry.mozilla.org or the auto-generated docs, I want to be able to figure out what the key values may contain.

I'm going to nitpick the description: it should be written from the perspective of what exactly is recorded. You're not actually recording the "number of times" the notification is shown, for example. Please reword to something like: "0=recorded each time the notification is shown 1=recorded the first time the notification is shown in each profile...". Finally, don't use the word "user" because that is overloaded and confusing. Since you're using prefs to record this, you should use "profile".

Comment 89

[Chris Pearce [:cpearce (Not reading bugmail)]]

I'll get this across the line, since gerald is on PTO and thus not supposed to be working.

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #88)
> https://reviewboard.mozilla.org/r/55094/#review53646
> 
> ::: toolkit/components/telemetry/Histograms.json:6444
> (Diff revision 3)
> > +    "bug_numbers": [1271483],
> > +    "expires_in_version": "53",
> > +    "kind": "enumerated",
> > +    "keyed": true,
> > +    "n_values": 8,
> > +    "description": "Decoder Doctor notification events, keyed by type of issue. For each issue, we record: 0=total number of times the notification is shown, 1=number of times it is first shown to a user, 2=total number of times 'Learn how' clicked, 3=first clicked per user, 4=issue solved after infobar has been shown at least once."
> 
> If there are five enumeration values being recorded, why is n_values 8? Do
> you expect to add more events in the future?

I can imagine we may want to add more enum values to this probe if we evolve how Decoder Doctor prompts the user.

Vladan had suggested to us when reviewing previous probes that we should include extra free slots in enumerated probes, because telemetry probes can't have enum valued added later on.

We can limit it to 5 if you like, but it seems leaving some free slots in the enum adds future agility at not much cost?

Comment 90

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: p7. Telemetry histogram for DecDoc infobar stats -

MozReview-Commit-ID: KB2r22Z3rss

Comment 91

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

To add to Chris' reply:

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #88)
> https://reviewboard.mozilla.org/r/55094/#review53646
> [...]
> This description still doesn't say what keys may contain and how they will
> be constructed. That is still my biggest concern here. In the bug comments
> you mention dom.properties keys, but that's not included here and if I'm
> looking at telemetry.mozilla.org or the auto-generated docs, I want to be
> able to figure out what the key values may contain.

The currently-expected key values will be:
- MediaWMFNeeded
- MediaWidevineNoWMFNoSilverlight
And possibly in the near future, if enabled by pref:
- MediaPlatformDecoderNotFound

They happen to be dom.properties keys, but I don't think that's important here (Maybe I shouldn't have mentioned that?)
They just specify specific issues that Decoder Doctor catches.
I would hope their name is clear enough that we don't need to explain them further in the probe description?

Comment 92

[Benjamin Smedberg]

> The currently-expected key values will be:
> - MediaWMFNeeded
> - MediaWidevineNoWMFNoSilverlight
> And possibly in the near future, if enabled by pref:
> - MediaPlatformDecoderNotFound

Can we just list those in the histogram description, then? And as you add new doctor rules, update the histogram description?

Or, if you have a list of these keys somewhere else in the source tree, just reference that file?

It's important that people reading this be able to figure out, reading only the docs and not looking at the data, what is going to be sent.

Comment 93

[Chris Pearce [:cpearce (Not reading bugmail)]]

I will update Histograms.json to identify the keys of the histogram as localized string names from dom.properties, and direct people to dom.properties for verbose description of the condition being reported on.

Comment 94

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: p7. Telemetry histogram for DecDoc infobar stats -

MozReview-Commit-ID: KB2r22Z3rss

Comment 95

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756337 [details]
Bug 1271483 - p1. Optionally treat media.wmf.disabled as WMF failure for Decoder Doctor -

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

Comment 96

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756338 [details]
Bug 1271483 - p2. Fixed whitespace style -

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

Comment 97

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756339 [details]
Bug 1271483 - p3. constify loop variable -

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

Comment 98

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756340 [details]
Bug 1271483 - p4. Fixed console reporting for Widevine case -

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

Comment 99

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756341 [details]
Bug 1271483 - p5. Remove 'canPlay', can be inferred otherwise -

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

Comment 100

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756342 [details]
Bug 1271483 - p6. Separate unplayable formats by missing decoder -

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

Comment 101

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756343 [details]
Bug 1271483 - p7. Telemetry histogram for DecDoc infobar stats - , f=bsmedberg

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

Comment 102

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756344 [details]
Bug 1271483 - p8. More details in DecoderDoctorNotification -

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

Comment 103

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756345 [details]
Bug 1271483 - p9. Report more details from DecDecDiagnostics -

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

Comment 104

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756346 [details]
Bug 1271483 - p10. Front-end handling of prefs&telemetry -

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

Comment 105

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756347 [details]
Bug 1271483 - p11. Implement comma-sep-string for-range iterator -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55102/diff/5-6/

Comment 106

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756348 [details]
Bug 1271483 - p12. Use StringListContains to filter notifications -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55104/diff/5-6/

Comment 107

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756349 [details]
Bug 1271483 - p13. Rework formats list -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55106/diff/5-6/

Comment 108

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756350 [details]
Bug 1271483 - p14. Demagicify ReportStringId strings -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55108/diff/5-6/

Comment 109

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756351 [details]
Bug 1271483 - p15. Check if issues have been solved -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55110/diff/5-6/

Comment 110

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756783 [details]
Bug 1271483 - p16. gtest for list-string functions -

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

Comment 111

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756343 [details]
Bug 1271483 - p7. Telemetry histogram for DecDoc infobar stats - , f=bsmedberg

https://reviewboard.mozilla.org/r/55094/#review57004

Carrying r+

Comment 112

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756343 [details]
Bug 1271483 - p7. Telemetry histogram for DecDoc infobar stats - , f=bsmedberg

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

Comment 113

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756344 [details]
Bug 1271483 - p8. More details in DecoderDoctorNotification -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55096/diff/5-6/

Comment 114

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756345 [details]
Bug 1271483 - p9. Report more details from DecDecDiagnostics -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55098/diff/5-6/

Comment 115

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756346 [details]
Bug 1271483 - p10. Front-end handling of prefs&telemetry -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55100/diff/5-6/

Comment 116

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756347 [details]
Bug 1271483 - p11. Implement comma-sep-string for-range iterator -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55102/diff/6-7/

Comment 117

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756348 [details]
Bug 1271483 - p12. Use StringListContains to filter notifications -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55104/diff/6-7/

Comment 118

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756349 [details]
Bug 1271483 - p13. Rework formats list -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55106/diff/6-7/

Comment 119

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756350 [details]
Bug 1271483 - p14. Demagicify ReportStringId strings -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55108/diff/6-7/

Comment 120

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756351 [details]
Bug 1271483 - p15. Check if issues have been solved -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55110/diff/6-7/

Comment 121

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756783 [details]
Bug 1271483 - p16. gtest for list-string functions -

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

Comment 122

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8761890 [details] [diff] [review]
p7. Telemetry histogram for DecDoc infobar stats -

Moved r+'d patch into mozreview, marking it as f=bsmedberg there.

Comment 123

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/59ecc73662cc
p1. Optionally treat media.wmf.disabled as WMF failure for Decoder Doctor - r=cpearce
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f59435519a3
p2. Fixed whitespace style - r=cpearce
https://hg.mozilla.org/integration/mozilla-inbound/rev/f156a01bcf57
p3. constify loop variable - r=cpearce
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed7dc9a4cbe6
p4. Fixed console reporting for Widevine case - r=cpearce
https://hg.mozilla.org/integration/mozilla-inbound/rev/32ec54498aee
p5. Remove 'canPlay', can be inferred otherwise - r=cpearce
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb46c7802ff4
p6. Separate unplayable formats by missing decoder - r=cpearce
https://hg.mozilla.org/integration/mozilla-inbound/rev/55ea04d784cc
p7. Telemetry histogram for DecDoc infobar stats - r=cpearce, f=bsmedberg
https://hg.mozilla.org/integration/mozilla-inbound/rev/95bef5e7a918
p8. More details in DecoderDoctorNotification - r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7a336a48fa2
p9. Report more details from DecDecDiagnostics - r=cpearce
https://hg.mozilla.org/integration/mozilla-inbound/rev/60ea602c56cd
p10. Front-end handling of prefs&telemetry - r=gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/93bc3bb0c1be
p11. Implement comma-sep-string for-range iterator - r=cpearce
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbfd03f4dd55
p12. Use StringListContains to filter notifications - r=cpearce
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bc76d3f7827
p13. Rework formats list - r=cpearce
https://hg.mozilla.org/integration/mozilla-inbound/rev/d780f0ca5243
p14. Demagicify ReportStringId strings - r=cpearce
https://hg.mozilla.org/integration/mozilla-inbound/rev/575198d83ce7
p15. Check if issues have been solved - r=cpearce
https://hg.mozilla.org/integration/mozilla-inbound/rev/da005aa1d83c
p16. gtest for list-string functions - r=cpearce

Comment 124

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/13bc59af8f20
p17. Fix chrome test to match new notification parameters - r=me

Comment 125

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/59ecc73662cc
https://hg.mozilla.org/mozilla-central/rev/9f59435519a3
https://hg.mozilla.org/mozilla-central/rev/f156a01bcf57
https://hg.mozilla.org/mozilla-central/rev/ed7dc9a4cbe6
https://hg.mozilla.org/mozilla-central/rev/32ec54498aee
https://hg.mozilla.org/mozilla-central/rev/cb46c7802ff4
https://hg.mozilla.org/mozilla-central/rev/55ea04d784cc
https://hg.mozilla.org/mozilla-central/rev/95bef5e7a918
https://hg.mozilla.org/mozilla-central/rev/e7a336a48fa2
https://hg.mozilla.org/mozilla-central/rev/60ea602c56cd
https://hg.mozilla.org/mozilla-central/rev/93bc3bb0c1be
https://hg.mozilla.org/mozilla-central/rev/bbfd03f4dd55
https://hg.mozilla.org/mozilla-central/rev/2bc76d3f7827
https://hg.mozilla.org/mozilla-central/rev/d780f0ca5243
https://hg.mozilla.org/mozilla-central/rev/575198d83ce7
https://hg.mozilla.org/mozilla-central/rev/da005aa1d83c
https://hg.mozilla.org/mozilla-central/rev/13bc59af8f20

Comment 126

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1271483-p17.patch

Attaching follow-up patch (that landed shortly after parts 1-16), a small fix to a unit test.

Comment 127

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8756337 [details]
Bug 1271483 - p1. Optionally treat media.wmf.disabled as WMF failure for Decoder Doctor -

Aurora approval is requested for all 17 patches.

Approval Request Comment
[Feature/regressing bug #]: Decoder Doctor notifications about missing WMF libraries.

[User impact if declined]: We would not know the amount of WMF-missing issues in the field. If high, there could be other things we may want to do to help our many users with these issues; if low, we could spend our limited resources on more important issues instead.

[Describe test coverage new/current, TreeHerder]: In Nightly for more than a week, telemetry has started trickling in, showing it works; Aurora try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=11ca6ce0623ea4e57f377594f45bc18330574d59

[Risks and why]: Some risk associated with new code; but it just adds some processing&messaging to existing code, it shouldn't use much more resources itself; added unit tests for the heaviest part. Tested locally, with tries, and got proof (by telemetry) that it works in Nightly with no issues so far.

[String/UUID change made/needed]: None

Comment 128

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8756337 [details]
Bug 1271483 - p1. Optionally treat media.wmf.disabled as WMF failure for Decoder Doctor -

We still have a month left for aurora 49 to find and fix any regressions, so let's go for it.

Comment 129

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8634f32770f9
https://hg.mozilla.org/releases/mozilla-aurora/rev/6fa9ce655422
https://hg.mozilla.org/releases/mozilla-aurora/rev/cebc765f65c5
https://hg.mozilla.org/releases/mozilla-aurora/rev/a1b956fd336a
https://hg.mozilla.org/releases/mozilla-aurora/rev/41617bfea7c8
https://hg.mozilla.org/releases/mozilla-aurora/rev/812548e44545
https://hg.mozilla.org/releases/mozilla-aurora/rev/a3b704b37e5f
https://hg.mozilla.org/releases/mozilla-aurora/rev/0e22674d58c0
https://hg.mozilla.org/releases/mozilla-aurora/rev/0794b4026370
https://hg.mozilla.org/releases/mozilla-aurora/rev/83def5aed3da
https://hg.mozilla.org/releases/mozilla-aurora/rev/5d717e3c331f
https://hg.mozilla.org/releases/mozilla-aurora/rev/c99836f4707d
https://hg.mozilla.org/releases/mozilla-aurora/rev/e40406f3f9e9
https://hg.mozilla.org/releases/mozilla-aurora/rev/32ffb9c06c7b
https://hg.mozilla.org/releases/mozilla-aurora/rev/9d1c99c89f49
https://hg.mozilla.org/releases/mozilla-aurora/rev/31e95261ecde
https://hg.mozilla.org/releases/mozilla-aurora/rev/01f2dde06c5f