Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Error messages #36

Merged
merged 2 commits into from
Apr 19, 2020
Merged

Error messages #36

merged 2 commits into from
Apr 19, 2020

Conversation

orblivion
Copy link
Contributor

@orblivion orblivion commented Apr 17, 2020

Overview

  • Handle existing errors more accurately, including:
  • Handle a few more errors

The Most Interesting Part

So here's a couple very fun facts:

  • In some browsers (I suspect slightly old ones), NotAllowedError would mean either that the user disallowed access to the camera/mic, or the connection was insecure. As such, the only way I can think of distinguishing them is by checking for https (as we sortof were before I "fixed" it in a previous PR)
  • In other browsers (I suspect the most recent), NotAllowedError thankfully only means that the user disallowed access to the camera/mic. However, insecure connections are instead handled by hiding navigator.mediaDevices, navigator.getUserMedia, and possibly other things. The absence of those things used to be our way of testing for outdated browsers. So now, we need a way to distinguish between insecure and outdated, and the best way I can think of is to check for the existence of some other fields.

So we had to handle all that. The polyfill was already handling missing getUserMedia as an "unsupported" error, so I changed it to distinguish between the "unsupported" and "insecure" errors and return accordingly. I let the function that calls getUserMedia continue to handle all errors by giving an appropriate user message, but now I include the two potentially returned by the polyfill and I disambiguate NotAllowedError.


Question

As a separate matter, I wonder whether getUserMediaPolyfill is technically a polyfill anymore. By definition it looks like they're supposed to make older browsers behave like newer ones. Here, I'm making newer browsers behave like older ones, because it's just easier to handle errors this way. So the options I can think of:

  • Leave it as I wrote it if it's not a big deal
  • Rename the file
  • Move logic around so that it technically stays a polyfill.

return new Promise(function(resolve, reject) {
reject("getUserMedia is not supported in this browser.");
if (!(window.RTCPeerConnection || window.webkitRTCPeerConnection || window.mozRTCPeerConnection || window.RTCIceGatherer)) {
var e = new Error("getUserMedia is not supported in this browser.");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this text was ever getting seen. In fact, I think that we were returning this for SSL errors and accidentally correctly reporting them to the user as such.

@@ -22,7 +22,6 @@ var hooks = require("ep_etherpad-lite/static/js/pluginfw/hooks");

var rtc = (function() {
var isActive = false;
var isSupported = true;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a TODO to remove it (see below) and it's always set to true.

padcookie.setPref("rtcEnabled", true);
self.getUserMedia();
} else {
self.showNotSupported();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unless isSupported is getting set somewhere outside of this module, this function wasn't getting called.

@orblivion
Copy link
Contributor Author

Oh yeah, I also rebased master and I'm told I have to upgrade Etherpad-lite. I upgraded Etherpad-lite to the tip of develop and it still gives me that warning. Not sure what's up with that.

@orblivion
Copy link
Contributor Author

Oh, and according to the doc, even earlier versions of WebRTC used SecurityError instead of NotFoundError for insecure connections, but now SecurityError means something totally different 🤣 I could touch that if you want as well, but I figured I'd put it off since it's not entangled with the rest of this.

if (!(window.RTCPeerConnection || window.webkitRTCPeerConnection || window.mozRTCPeerConnection || window.RTCIceGatherer)) {
var e = new Error("getUserMedia is not supported in this browser.");
// I'd use NotSupportedError but I'm afraid they'll change the spec on us again
e.name = 'CustomNotSupportedError';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Best option I could think of at the time, but I'm open to other things. Perhaps just a better name.

@JohnMcLear JohnMcLear merged commit 00091f5 into ether:master Apr 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants