Closed Bug 1343442 Opened 7 years ago Closed 7 years ago

Analyze DecDoc decode errors&warnings and show user notification

Categories

(Core :: Audio/Video: Playback, enhancement)

Product:
Core
Component:
Audio/Video: Playback
Version:
53 Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: mozbugz, Assigned: mozbugz)

References

Details

Attachments

(10 files)

Bug 1343442 - Decode-error/warning webidl notification types and data -
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1343442 - MediaDecodeError/Warning user notification messages -
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1343442 - Allow MediaDecodeError/Warning user notifications in Nightly -
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1343442 - Move NotificationAndReportStringId closer to first use -
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1343442 - Provide appropriate params to console message -
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1343442 - Analyze eDecodeError/Warning issues -
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1343442 - Handle decode-error/warning in browser-media.js -
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1343442 - Refactored DecDoc browser tests for extensibility -
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1343442 - Re-enable browser_decoderDoctor.js on Mac -
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1343442 - decode-error/warning tests -
59 bytes, text/x-review-board-request
Gijs
: review+
Details 
Bug 1343161 is storing decode errors and warnings in DecoderDoctorDiagnostics.

This bug will implement the corresponding analysis, which will take the first error (or first warning if there are no errors), and display a drop-down notification, with a "Report Site Issue" button that will take the user to webcompat.com, with some relevant technical details already filled-in.

(We are dependent on https://github.com/webcompat/webcompat.com/issues/1356 and sub-issues, for the implementation of the extra parameters to be passed to webcompat.com.)

Because of privacy concerns, and to keep the volume of reports under control (for now), we will follow the same strategy as the main "Report Site Issue" button, which is only available on the Nightly channel (bug 1306114) and soon on the Dev channel (bug 1341438).
Comment on attachment 8852775 [details]
Bug 1343442 - MediaDecodeError/Warning user notification messages -

https://reviewboard.mozilla.org/r/124946/#review127540
Attachment #8852775 - Flags: review?(jyavenard) → review+
Comment on attachment 8852776 [details]
Bug 1343442 - Allow MediaDecodeError/Warning user notifications in Nightly -

https://reviewboard.mozilla.org/r/124948/#review127542
Attachment #8852776 - Flags: review?(jyavenard) → review+
Comment on attachment 8852777 [details]
Bug 1343442 - Move NotificationAndReportStringId closer to first use -

https://reviewboard.mozilla.org/r/124950/#review127544
Attachment #8852777 - Flags: review?(jyavenard) → review+
Comment on attachment 8852778 [details]
Bug 1343442 - Provide appropriate params to console message -

https://reviewboard.mozilla.org/r/124952/#review127546

::: dom/media/DecoderDoctorDiagnostics.cpp:331
(Diff revision 1)
>  }
>  
>  static void
>  ReportToConsole(nsIDocument* aDocument,
>                  const char* aConsoleStringId,
> -                const nsAString& aParams)
> +                nsTArray<const char16_t*>& aParams)

why not jse an array of nsAstring?
Attachment #8852778 - Flags: review?(jyavenard) → review+
Comment on attachment 8852779 [details]
Bug 1343442 - Analyze eDecodeError/Warning issues -

https://reviewboard.mozilla.org/r/124954/#review127554

::: commit-message-d24f3:1
(Diff revision 1)
> +Bug 1343442 - Analyze eDecodeError/Warning issues - r?jya

eDecodeError?
stray e?
Attachment #8852779 - Flags: review?(jyavenard) → review+
Comment on attachment 8852778 [details]
Bug 1343442 - Provide appropriate params to console message -

https://reviewboard.mozilla.org/r/124952/#review127546

> why not jse an array of nsAstring?

Because nsContentUtils::ReportToConsole takes a const char16_t**, and it was easy enough to create an array of char16_t* in the caller.
Passing an array of nsAStrings would mean extra work to create another array with just the pointers to pass to ReportToConsole.
Comment on attachment 8852779 [details]
Bug 1343442 - Analyze eDecodeError/Warning issues -

https://reviewboard.mozilla.org/r/124954/#review127554

> eDecodeError?
> stray e?

Nope, the 'e' is real: DecoderDoctorDiagnostics::eDecodeError is part of an enum generated from a webidl.
Comment on attachment 8852780 [details]
Bug 1343442 - Handle decode-error/warning in browser-media.js -

https://reviewboard.mozilla.org/r/124956/#review127600

The code looks solid but this could really do with a test. :-)

::: browser/base/content/browser-media.js:333
(Diff revision 1)
>              let baseURL = Services.urlFormatter.formatURLPref("app.support.baseURL");
>              openUILinkIn(baseURL + sumo, "tab");
>            }
>          });
>        }
> +      if (gDecoderDoctorHandler.getReportIssueButton(type)) {

Nit: this method name makes it sound like it returns a button element.

It feels like `shouldShowReportIssueButton()` would be clearer?

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

Nit: You can use:

`Services.prefs.getBoolPref(buttonClickedPref, false);`

It also looks like this was copied from one of the other buttons, so maybe we can pull out the logic in a small utility function?

::: browser/base/content/browser-media.js:347
(Diff revision 1)
> +              histogram.add(decoderDoctorReportId, TELEMETRY_DDSTAT_CLICKED_FIRST);
> +            }
> +            histogram.add(decoderDoctorReportId, TELEMETRY_DDSTAT_CLICKED);
> +
> +            openUILinkIn(
> +              "https://webcompat.com/issues/new?url=" +

Can we make the URL a pref? That would also help test this.

::: browser/base/content/browser-media.js:348
(Diff revision 1)
> +              encodeURIComponent(window.location.href) +
> +              "&details=" +
> +              encodeURIComponent("Technical Information:\n" + decodeIssue) +
> +              (resourceURL ? encodeURIComponent("\nResource: " + resourceURL) : "") +
> +              "&problem_type=video_bug&src=media-decode-error",
> +              "tab");

I think this would be simpler if you use the `URLSearchParams` constructor and append the relevant bits to it. That'll take care of the encoding and the nitty-gritty of inserting '?', '&' and '=' as necessary.
Attachment #8852780 - Flags: review?(gijskruitbosch+bugs) → review-
Comment on attachment 8852774 [details]
Bug 1343442 - Decode-error/warning webidl notification types and data -

https://reviewboard.mozilla.org/r/124944/#review128160

ok, so this is for chrome only.
Attachment #8852774 - Flags: review?(bugs) → review+
Comment on attachment 8852780 [details]
Bug 1343442 - Handle decode-error/warning in browser-media.js -

https://reviewboard.mozilla.org/r/124956/#review127600

Written! Had to rework other tests for more flexibility, hence the other new patches.

> Nit: this method name makes it sound like it returns a button element.
> 
> It feels like `shouldShowReportIssueButton()` would be clearer?

Renamed to getEndpointForReportIssueButton(), so it's providing useful information too.

> Nit: You can use:
> 
> `Services.prefs.getBoolPref(buttonClickedPref, false);`
> 
> It also looks like this was copied from one of the other buttons, so maybe we can pull out the logic in a small utility function?

Thanks for the tip.

But I haven't refactored the common code (yet).

> Can we make the URL a pref? That would also help test this.

Done, thanks for the idea, and it does help with testing.

> I think this would be simpler if you use the `URLSearchParams` constructor and append the relevant bits to it. That'll take care of the encoding and the nitty-gritty of inserting '?', '&' and '=' as necessary.

Done.
Webcompat had to be modified a bit to handle URLSearchParams' spaces-as-pluses encoding!
(See https://github.com/webcompat/webcompat.com/issues/1469 )
Comment on attachment 8852780 [details]
Bug 1343442 - Handle decode-error/warning in browser-media.js -

https://reviewboard.mozilla.org/r/124956/#review131316

::: browser/base/content/browser-media.js:280
(Diff revision 2)
>        return;
>      }
>  
>      // We keep the list of formats in prefs for the sake of the decoder itself,
>      // which reads it to determine when issues get solved for these formats.
>      // (Writing prefs from e10s content is now allowed.)

Is this comment supposed to say 'not' allowed?

::: browser/base/content/browser-media.js:288
(Diff revision 2)
> +                        Services.prefs.getPrefType(formatsPref) &&
>                          Services.prefs.getCharPref(formatsPref);

Could just use getCharPref(formatsPref, "") here, I think.
Attachment #8852780 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8856801 [details]
Bug 1343442 - Refactored DecDoc browser tests for extensibility -

https://reviewboard.mozilla.org/r/128722/#review131318

Close, but the tabChecker thing means I'd like to see this again. :-)

::: browser/base/content/test/general/browser_decoderDoctor.js:16
(Diff revision 1)
> +  if (data.type === undefined) {
> +    ok(data.type !== undefined, "Test implementation error: data.type must be provided");

We generally use `typeof` for this type of check, rather than strict equals (or strict not equals).

Inside the if, I think you can just ok(false, ...) because you already know the condition will be false given you're in the if() statement. :-)

::: browser/base/content/test/general/browser_decoderDoctor.js:20
(Diff revision 1)
> +  if (data.isSolved === undefined) {
> +    data.isSolved = false;
> +  }

Slightly more JS-y would be:

```js
data.isSolved = data.isSolved || false;
```

::: browser/base/content/test/general/browser_decoderDoctor.js:23
(Diff revision 1)
> +  if (data.decoderDoctorReportId === undefined) {
> +    data.decoderDoctorReportId = "testReportId";
> +  }

Similar here:

```js
data.decoderDoctorReportId = data.decoderDoctorReportId || "testReportId";
```

::: browser/base/content/test/general/browser_decoderDoctor.js:76
(Diff revision 1)
> +    if (!tabChecker) {
> +      ok(tabChecker,
> +         "Test implementation error: Missing option to check opened tab");
> +      return;
> +    }

It says `options` is optional, but this will fail the test if not provided, I think...

I think a bit earlier, rather than checking for options and options.sumo and then doing stuff, you could do:

```js
if (!options || !options.sumo) {
  return;
}
```

and then move the current tabChecker assignment etc. out of the if statement. In fact, then you probably don't need a separate function, you can just check the currentURI of the linked browser directly after the new tab has been opened.
Attachment #8856801 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8856802 [details]
Bug 1343442 - Re-enable browser_decoderDoctor.js on Mac -

https://reviewboard.mozilla.org/r/128724/#review131322
Attachment #8856802 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8856803 [details]
Bug 1343442 - decode-error/warning tests -

https://reviewboard.mozilla.org/r/128726/#review131328

::: browser/base/content/test/general/browser_decoderDoctor.js:76
(Diff revision 1)
>                             "fix-video-audio-problems-firefox-windows");
>        tabChecker = function(tab) {
>          is(tab.linkedBrowser.currentURI.spec, url,
>             "Expected '" + url + "' in new tab")
>        };
> +    } else if (options && options.webcompat) {

So, after the change in the first commit I reviewed, you can add ... !options.webcompat to the early return, and then use a simple if() after the new tab has been opened to do these extra checks.

::: browser/base/content/test/general/browser_decoderDoctor.js:81
(Diff revision 1)
> +    } else if (options && options.webcompat) {
> +      tabChecker = function(tab) {
> +        let urlString = tab.linkedBrowser.currentURI.spec;
> +        let endpoint = Services.prefs.getStringPref("media.decoder-doctor.new-issue-endpoint", "");
> +        ok(urlString.startsWith(endpoint),
> +           "Expected URL starting with '" + endpoint + "', got '" + urlString + "'")

Here and in some of the other assertions, please use template strings for clarity:

```js
ok(...,
   `Expected URL starting with '${endpoint}', got '${urlString}'`);
```

Is easier to read than the mix of quotes. :-)

::: browser/base/content/test/general/browser_decoderDoctor.js:82
(Diff revision 1)
> +        let url = new URL(urlString);
> +        let params = new URLSearchParams(url.search.substring(1));

Nit: let params = new URL(urlString).searchParams :-)
Attachment #8856803 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8856801 [details]
Bug 1343442 - Refactored DecDoc browser tests for extensibility -

https://reviewboard.mozilla.org/r/128722/#review131318

> We generally use `typeof` for this type of check, rather than strict equals (or strict not equals).
> 
> Inside the if, I think you can just ok(false, ...) because you already know the condition will be false given you're in the if() statement. :-)

Thank you for your patience with explaining js idioms, much appreciated!

For the `ok(condition, ...)` I thought it would be useful to show the unmet condition more visibly in the log. But I see you've used `ok(false, ...)` elsewhere, so I'll follow that.

> Similar here:
> 
> ```js
> data.decoderDoctorReportId = data.decoderDoctorReportId || "testReportId";
> ```

I agree with isSolved as we're expecting a boolean.

However decoderDoctorReportId could theoretically be given as an empty string, in which case we wouldn't want to override it with something else. So I'll keep the test, but use typeof instead.

> It says `options` is optional, but this will fail the test if not provided, I think...
> 
> I think a bit earlier, rather than checking for options and options.sumo and then doing stuff, you could do:
> 
> ```js
> if (!options || !options.sumo) {
>   return;
> }
> ```
> 
> and then move the current tabChecker assignment etc. out of the if statement. In fact, then you probably don't need a separate function, you can just check the currentURI of the linked browser directly after the new tab has been opened.

In fact the only "options" we now have are related to checking the new tab.
So I'll just make this parameter 'tabChecker', and each test can provide its own function to check the new tab; it's much more future-proof, and keeps test-specific logic closer to the test cases.
Comment on attachment 8856803 [details]
Bug 1343442 - decode-error/warning tests -

https://reviewboard.mozilla.org/r/128726/#review131328

> So, after the change in the first commit I reviewed, you can add ... !options.webcompat to the early return, and then use a simple if() after the new tab has been opened to do these extra checks.

After the big change in the "Refactored" patch, I only need to add a new tab-checker function to work with WebCompat. :-)
Comment on attachment 8856801 [details]
Bug 1343442 - Refactored DecDoc browser tests for extensibility -

https://reviewboard.mozilla.org/r/128722/#review131912

Nice, thanks!
Attachment #8856801 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/157c1aecdd3d
Decode-error/warning webidl notification types and data - r=smaug
https://hg.mozilla.org/integration/autoland/rev/5ff6265b06bf
MediaDecodeError/Warning user notification messages - r=jya
https://hg.mozilla.org/integration/autoland/rev/374eac537a5e
Allow MediaDecodeError/Warning user notifications in Nightly - r=jya
https://hg.mozilla.org/integration/autoland/rev/19baf99d6ad9
Move NotificationAndReportStringId closer to first use - r=jya
https://hg.mozilla.org/integration/autoland/rev/6c13a53b4cac
Provide appropriate params to console message - r=jya
https://hg.mozilla.org/integration/autoland/rev/33e712fad10c
Analyze eDecodeError/Warning issues - r=jya
https://hg.mozilla.org/integration/autoland/rev/c586bc55ed72
Refactored DecDoc browser tests for extensibility - r=Gijs
https://hg.mozilla.org/integration/autoland/rev/c61b9b0d946e
Re-enable browser_decoderDoctor.js on Mac - r=Gijs
https://hg.mozilla.org/integration/autoland/rev/fef911037b52
Handle decode-error/warning in browser-media.js - r=Gijs
https://hg.mozilla.org/integration/autoland/rev/ef27dccaf311
decode-error/warning tests - r=Gijs
Blocks: 1377338
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: