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.

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": ["gsquelart@mozilla.com"], > + "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

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.

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 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 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 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 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 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 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 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 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 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 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...

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/

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

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

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/

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/

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/

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

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/

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/

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/

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/

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/

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/

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/

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 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 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 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 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 on attachment 8756338 [details] Bug 1271483 - p2. Fixed whitespace style - https://reviewboard.mozilla.org/r/55084/#review51948

Comment on attachment 8756339 [details] Bug 1271483 - p3. constify loop variable - https://reviewboard.mozilla.org/r/55086/#review51952

Comment on attachment 8756340 [details] Bug 1271483 - p4. Fixed console reporting for Widevine case - https://reviewboard.mozilla.org/r/55088/#review51954

Comment on attachment 8756341 [details] Bug 1271483 - p5. Remove 'canPlay', can be inferred otherwise - https://reviewboard.mozilla.org/r/55090/#review51960

Comment on attachment 8756342 [details] Bug 1271483 - p6. Separate unplayable formats by missing decoder - https://reviewboard.mozilla.org/r/55092/#review51964

Comment on attachment 8756343 [details] Bug 1271483 - p7. Telemetry histogram for DecDoc infobar stats - , f=bsmedberg https://reviewboard.mozilla.org/r/55094/#review51968

Comment on attachment 8756345 [details] Bug 1271483 - p9. Report more details from DecDecDiagnostics - https://reviewboard.mozilla.org/r/55098/#review51986

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 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 on attachment 8756349 [details] Bug 1271483 - p13. Rework formats list - https://reviewboard.mozilla.org/r/55106/#review52008

Comment on attachment 8756350 [details] Bug 1271483 - p14. Demagicify ReportStringId strings - https://reviewboard.mozilla.org/r/55108/#review52012

Comment on attachment 8756351 [details] Bug 1271483 - p15. Check if issues have been solved - https://reviewboard.mozilla.org/r/55110/#review52016

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.)

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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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?

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/

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 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).

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

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). :-)

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 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 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 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 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 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 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 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 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 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 on attachment 8756347 [details] Bug 1271483 - p11. Implement comma-sep-string for-range iterator - https://reviewboard.mozilla.org/r/55102/#review52346

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 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 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 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 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 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 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/

https://reviewboard.mozilla.org/r/55402/#review52348 Yep, much better, done! It was fully templated, so it all went into VideoUtils.h, not .cpp.

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".

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?

MozReview-Commit-ID: KB2r22Z3rss

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?

> 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.

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.

MozReview-Commit-ID: KB2r22Z3rss

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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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.

Pushed by gsquelart@mozilla.com: 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

Pushed by gsquelart@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/13bc59af8f20 p17. Fix chrome test to match new notification parameters - r=me

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

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

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 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.

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