Closed Bug 868778 Opened 6 years ago Closed 6 years ago

Move DataChannel to WebIDL

Categories

(Core :: WebRTC, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla24

People

(Reporter: Ms2ger, Assigned: Ms2ger)

References

(Blocks 1 open bug)

Details

(Whiteboard: [WebRTC][blocking-webrtc-])

Attachments

(3 files)

No description provided.
Whiteboard: [WebRTC][blocking-webrtc-]
I want that file name...
Attachment #747993 - Flags: review?(rjesup)
I removed the XPIDL send() because it was too much work to keep it; left the rest of the XPIDL interface because people seemed to QI to it, at least.
Attachment #747995 - Flags: review?(mounir)
Attachment #747993 - Flags: review?(rjesup) → review+
Attachment #747994 - Flags: review?(rjesup) → review+
Comment on attachment 747995 [details] [diff] [review]
Part c: Move to WebIDL

Review of attachment 747995 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me but I would feel more comfortable to get someone who knows that code to check this patch too.

::: content/base/src/nsDOMDataChannel.cpp
@@ +56,5 @@
>    mDataChannel->SetListener(nullptr, nullptr);
>    mDataChannel->Close();
>  }
>  
> +

nit: no need to add this blank line

::: content/base/src/nsDOMDataChannel.h
@@ +7,5 @@
>  #ifndef nsDOMDataChannel_h
>  #define nsDOMDataChannel_h
>  
> +#include "mozilla/dom/DataChannelBinding.h"
> +#include "mozilla/dom/TypedArray.h"

It's not obvious why you need those #include. Could you double-check that they are actually needed?
Attachment #747995 - Flags: review?(rjesup)
Attachment #747995 - Flags: review?(mounir)
Attachment #747995 - Flags: review+
(In reply to Mounir Lamouri (:mounir) from comment #4)
> Comment on attachment 747995 [details] [diff] [review]
> Part c: Move to WebIDL
> 
> Review of attachment 747995 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me but I would feel more comfortable to get someone who knows
> that code to check this patch too.
> 
> ::: content/base/src/nsDOMDataChannel.cpp
> @@ +56,5 @@
> >    mDataChannel->SetListener(nullptr, nullptr);
> >    mDataChannel->Close();
> >  }
> >  
> > +
> 
> nit: no need to add this blank line
> 
> ::: content/base/src/nsDOMDataChannel.h
> @@ +7,5 @@
> >  #ifndef nsDOMDataChannel_h
> >  #define nsDOMDataChannel_h
> >  
> > +#include "mozilla/dom/DataChannelBinding.h"
> > +#include "mozilla/dom/TypedArray.h"
> 
> It's not obvious why you need those #include. Could you double-check that
> they are actually needed?

You can't forward declare enums (for DataChannelBinding.h) or typedefs (for TypedArray.h).
Attachment #747995 - Flags: review?(rjesup) → review+
https://hg.mozilla.org/mozilla-central/rev/7197c7cc8a30
https://hg.mozilla.org/mozilla-central/rev/60c386548d5e
https://hg.mozilla.org/mozilla-central/rev/b94e996b05fd

And because I forgot SetIsDOMBinding() and we apparently don't have any tests for send():

https://hg.mozilla.org/mozilla-central/rev/ca7f8131a8d4
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Keywords: verifyme
Did a good amount of regression testing on this today with my test app for doing remote data channel communication - lgtm on nightly.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.