Skip to content

Fix #374: Deprecate DOMError in favor of DOMException. #375

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Aug 12, 2019
Merged

Fix #374: Deprecate DOMError in favor of DOMException. #375

merged 2 commits into from
Aug 12, 2019

Conversation

floating-cat
Copy link
Contributor

Fix #374

floating-cat
floating-cat commented Aug 12, 2019
View reviewed changes
src/main/scala/org/scalajs/dom/experimental/webrtc/WebRTC.scala
* object, representing the generated offer.
*
* If the SDP generation process failed for any reason, the user agent must
* reject the returned promise with an DOMError object of type TBD as its
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method documentation came from WebRTC spec, but the latest spec for this method documentation changes a lot.
Because this comment mentioned DOMError, so I guess I need update this comment too, so I replace this method documentation with the MDN one. I am glad to revert this large documentation change if needed.

src/main/scala/org/scalajs/dom/experimental/webrtc/WebRTC.scala
* object, representing the generated answer.
*
* If the SDP generation process failed for any reason, the user agent must
* reject the returned promise with a DOMError object of type TBD.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above.

src/main/scala/org/scalajs/dom/experimental/mediastream/MediaStream.scala Outdated
@@ -570,8 +569,8 @@ object MediaDeviceInfo {
trait NavigatorMediaStream extends js.Object {
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 can't find this in MediaStream API. I am not sure whether the origin author added this trait by accident. I guess we can remove this trait completely?

src/main/scala/org/scalajs/dom/experimental/webrtc/package.scala
@@ -26,7 +27,7 @@ package object webrtc {
"Navigator.getUserMedia() is deprecated, use Navigator.mediaDevices.getUserMedia() instead.",
"2014-05-28")
def getUserMedia(constraints: MediaStreamConstraints,
success: js.Function1[MediaStream, Any],
error: js.Function1[DOMError, Any]): Unit = js.native
successCallback: js.Function1[MediaStream, Any],
Copy link
Contributor Author

@floating-cat floating-cat Aug 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I find this method argument names are incorrect[1], so I updated these two arugment names.

  1. https://w3c.github.io/mediacapture-main/#legacy-interface-extensions

sjrd
sjrd requested changes Aug 12, 2019
View reviewed changes
Copy link
Member

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

Thank you. I have a few comments.

@floating-cat
Update DOMException API
afc4a3f 
* Add new error sets.
* Reorder error sets in numeric order. So we can add new error sets
easily in the future.
@floating-cat
Copy link
Contributor Author

I have addressed your mentioned problems and rebased code.

sjrd
sjrd requested changes Aug 12, 2019
View reviewed changes
Copy link
Member

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

Thanks. All good, except one typo in the second commit message. It has:

Replace DOMError with DOMError.

But the second DOMError should obviously be DOMException.

After that it's good to merge.

@floating-cat
DOMError was removed from DOM spec
33d64c2 
* Replace DOMError with DOMException.
* Update documentation for old DOMError reference.
* Remove NavigatorMediaStream trait in MediaStream, because this facade
use DOMError but it doesn't exist in MediaStream API actually.

See:
https://dom.spec.whatwg.org/#domerror
https://bugs.chromium.org/p/chromium/issues/detail?id=460725
https://bugzilla.mozilla.org/show_bug.cgi?id=1120178
@floating-cat
Copy link
Contributor Author

Thanks. I have fixed the typo!

@sjrd sjrd changed the title DOMError was removed from DOM spec Update the DOMException API and deprecate DOMError in favor of DOMException. Aug 12, 2019
@sjrd sjrd changed the title Update the DOMException API and deprecate DOMError in favor of DOMException. Fix #374: Deprecate DOMError in favor of DOMException. Aug 12, 2019
@sjrd sjrd merged commit 3ad597c into scala-js:master Aug 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sjrd sjrd sjrd approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use DOMException instead of DOMError
2 participants
@floating-cat @sjrd