Closed Bug 910462 Opened 11 years ago Closed 4 years ago

Data channels should work in standalone C++ code

Categories

(Core :: WebRTC, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED INCOMPLETE
backlog parking-lot

People

(Reporter: akligman, Unassigned)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [games:p-])

Attachments

(4 obsolete files)

Data channels currently rely on the window objects, which means that standalone code is not able to make use of the implementation in mozilla-central.
Whiteboard: [games:p?]
Beyond Alan's use case above, being able to run datachannels without a window would also allow us to expand the signaling_unittests to run the datachannel SDP handling through its paces. This is currently a pretty big hole in our unit testing.
Better would be to have a window (or fake one up) for unit tests.  Also, unit tests are mostly useful for testing things that aren't visible or aren't tweakable in mochitests from JS.  DataChannels is pretty exposed to JS unless you want to do packet-level SCTP tests (purposeful drops and protocol failures/violations).
(In reply to Randell Jesup [:jesup] from comment #2)
> Better would be to have a window (or fake one up) for unit tests. 

Except that we also want to use this for non-unittest scenarios
that have nothing to do with a browser.

> Also,
> unit tests are mostly useful for testing things that aren't visible or
> aren't tweakable in mochitests from JS. 

I don't really agree with this philosophically. My objective is to
have C++ unit tests for everything PC and below.
(In reply to Randell Jesup [:jesup] from comment #2)
> DataChannels is pretty exposed to
> JS unless you want to do packet-level SCTP tests (purposeful drops and
> protocol failures/violations).

To be clear, I'm not saying we need this in order to unit test datachannels (although that is also true) -- I want to be able to test sipcc's handling of datachannel-related SDP, and I'd like to be able to do it without loading up a full-blown browser.
Attached patch hack nsDOMDataChannel (obsolete) — Splinter Review
So I did some hacking on the plane... This patch is pretty ugly, but it builds. I haven't tried running all the tests yet. This patch splits nsDOMDataChannel into a DataChannelBase class, and nsDOMDataChannel which does all the DOM stuff and has a DataChannelBase member that it defers to. There's a lot of redundant code in here, I'm not sure if that's really the best way to do things.
Assignee: ekr → ted
This passes the dom/media mochitests and the signalling C++ tests.
Attached patch hack nsDOMDataChannel (obsolete) — Splinter Review
I made this a little less bad. Talking to ekr today, he suggested that we
might alternately just #ifdef MOZILLA_INTERNAL_API the hell out of nsDOMDataChannel, which is a plausible alternative.
Attachment #813747 - Flags: review?(rjesup)
Attachment #813733 - Attachment is obsolete: true
Comment on attachment 813747 [details] [diff] [review]
hack nsDOMDataChannel

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

Looks good - Try build?

::: content/base/src/DataChannelBase.cpp
@@ +7,5 @@
> +#include "DataChannelBase.h"
> +
> +#ifdef MOZ_LOGGING
> +#define FORCE_PR_LOG
> +#endif

Not sure why the logging is set up this way...

@@ +142,5 @@
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +DataChannelBase::GetBinaryType(nsAString & aBinaryType)

nit: fix spaces around &

@@ +265,5 @@
> +             "Unknown state in DataChannelBase::Send");
> +
> +  int32_t sent;
> +  if (aMsgStream) {
> +    sent = mDataChannel->SendBinaryStream(aMsgStream, aMsgLength);

Please file a bug to remove msglength from SendBinaryStream() (and from DataChannelBase::Send())

::: content/base/src/DataChannelBase.h
@@ +42,5 @@
> +  enum DataChannelBinaryType {
> +    DC_BINARY_TYPE_ARRAYBUFFER,
> +    DC_BINARY_TYPE_BLOB,
> +  };
> +  DataChannelBinaryType DCBinaryType() const

trivial nit: add blank line after enum def
Attachment #813747 - Flags: review?(rjesup) → review+
Comment on attachment 813747 [details] [diff] [review]
hack nsDOMDataChannel

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

::: content/base/src/DataChannelBase.h
@@ +12,5 @@
> +#include "nsIDOMDataChannel.h"
> +
> +class nsIDOMBlob;
> +namespace mozilla {
> +  class ErrorResult;

Should not be indented

@@ +15,5 @@
> +namespace mozilla {
> +  class ErrorResult;
> +}
> +
> +class DataChannelBase : public nsIDOMDataChannel

New content code should be in the mozilla::dom namespace.

@@ +21,5 @@
> +public:
> +  DataChannelBase(already_AddRefed<mozilla::DataChannel> aDataChannel,
> +                  mozilla::DataChannelListener* aListener)
> +    : mDataChannel(aDataChannel),
> +      mBinaryType(DC_BINARY_TYPE_BLOB) {

{ on the next line

@@ +24,5 @@
> +    : mDataChannel(aDataChannel),
> +      mBinaryType(DC_BINARY_TYPE_BLOB) {
> +    MOZ_ASSERT(mDataChannel);
> +    if (aListener)
> +      mDataChannel->SetListener(aListener, nullptr);

{}

@@ +72,5 @@
> +protected:
> +  void Send(nsIInputStream* aMsgStream, const nsACString& aMsgString,
> +            uint32_t aMsgLength, bool aIsBinary, mozilla::ErrorResult& aRv);
> +
> +  // Owning reference

Clearly

::: content/base/src/moz.build
@@ +36,5 @@
>  ]
>  
>  if CONFIG['MOZ_WEBRTC']:
>      EXPORTS += [
> +        'DataChannelBase.h',

Export as mozilla/dom/DataChannelBase.h

::: content/base/src/nsDOMDataChannel.h
@@ +6,5 @@
>  
>  #ifndef nsDOMDataChannel_h
>  #define nsDOMDataChannel_h
>  
> +#include "DataChannelBase.h"

mozilla/dom/

::: media/mtransport/standalone/Makefile.in
@@ +7,5 @@
>  ifeq (WINNT,$(OS_TARGET))
>  VISIBILITY_FLAGS =
>  endif
>  
> +DEFINES = \

Move to moz.build, please. Why do we need those, though? Should we define those globally?

::: media/mtransport/standalone/moz.build
@@ +16,5 @@
>  
>  LIBRARY_NAME = 'mtransport_s'
>  
>  LOCAL_INCLUDES = [
> +    '$(DEPTH)/dist/include/mozilla/net',

Please don't do this. If you need something there, include it as mozilla/net/Foo.h

::: netwerk/sctp/datachannel/DataChannel.cpp
@@ +78,5 @@
>  
> +#ifndef MOZILLA_INTERNAL_API
> +
> +inline nsresult
> +NS_ReadInputStreamToString(nsIInputStream *aInputStream,

*, & to the left

@@ +83,5 @@
> +                           nsACString &aDest,
> +                           uint32_t aCount)
> +{
> +    if (!aDest.SetLength(aCount))
> +        return NS_ERROR_OUT_OF_MEMORY;

{}, but check if this can happen

@@ +85,5 @@
> +{
> +    if (!aDest.SetLength(aCount))
> +        return NS_ERROR_OUT_OF_MEMORY;
> +    void* dest = aDest.BeginWriting();
> +    return NS_ReadInputStreamToBuffer(aInputStream, &dest, aCount);

Two-space indentation
I know the patch here already has r+, but I had more plane time so I went back and tried out the #ifdef solution. I think it's more palatable in the end.
I missed a few #ifdefs in the PeerConnection code. I think I'd like to land this patch instead since it's less churn.
Attachment #815530 - Flags: review?(rjesup)
Attachment #814263 - Attachment is obsolete: true
Comment on attachment 815530 [details] [diff] [review]
ifdef nsDOMDataChannel to work with external linkage

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

r+ (in that this patch should work and cleans up a few things), but I'll note that while this allows abr's wish to write datachannel signaling unittests in C++, I think it's of only marginal help in making the DataChannel implementation 'liftable' to create node.js DataChannel support and would not support ack's original request.  So before checking this in, we need to decide if we want to go this route or another one.  I'll say that anything which helps to enable people to build DataChannels into non-browsers is a win for the feature as a whole.

::: content/base/src/nsDOMDataChannel.cpp
@@ +276,2 @@
>    NS_ABORT_IF_FALSE(NS_IsMainThread(), "Not running on main thread");
> +#endif

This is (to quote ekr) sad-making.... though it's merely a side-effect of the general problem of C++ unit tests not running their tests on mainthread.

@@ +442,5 @@
>    MOZ_ASSERT(NS_IsMainThread());
>    return DoOnMessageAvailable(aMessage, false);
> +#else
> +  return NS_OK;
> +#endif

This "works" as in compiles, but makes an nsDOMDataChannel object pretty pointless and untestable.

::: media/mtransport/standalone/moz.build
@@ +32,5 @@
>      '/media/mtransport/third_party/nrappkit/src/registry',
>      '/media/mtransport/third_party/nrappkit/src/share',
>      '/media/mtransport/third_party/nrappkit/src/stats',
>      '/media/mtransport/third_party/nrappkit/src/util/libekr',
> +    '/media/webrtc/trunk/third_party/libjingle/source/',

Remove libjingle, it went away long ago
Attachment #815530 - Flags: review?(rjesup) → review+
::: content/base/src/nsDOMDataChannel.cpp
@@ +276,2 @@
>    NS_ABORT_IF_FALSE(NS_IsMainThread(), "Not running on main thread");
> +#endif

I'm currently having this problem; No clear workaround. I'm trying to refactor things so that I have a "main thread". Thoughts?
(In reply to Randell Jesup [:jesup] from comment #12)
> Comment on attachment 815530 [details] [diff] [review]
> ifdef nsDOMDataChannel to work with external linkage
> 
> Review of attachment 815530 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ (in that this patch should work and cleans up a few things), but I'll
> note that while this allows abr's wish to write datachannel signaling
> unittests in C++, I think it's of only marginal help in making the
> DataChannel implementation 'liftable' to create node.js DataChannel support
> and would not support ack's original request.  So before checking this in,
> we need to decide if we want to go this route or another one.  I'll say that
> anything which helps to enable people to build DataChannels into
> non-browsers is a win for the feature as a whole.

Can you expound on this? This patch is pretty finely targeted at simply allowing the datachannel impl to be built without MOZILLA_INTERNAL_API.

> This "works" as in compiles, but makes an nsDOMDataChannel object pretty
> pointless and untestable.

Right, I have a followup patch that adds a very simple observer API to nsIDOMDataChannel. I figured this patch was ugly enough without introducing that.
Blocks: gecko-games
Whiteboard: [games:p?] → [games:p-]
Ted -- Is it useful to land these patches for the standalone or elsewhere?
backlog: --- → parking-lot
Flags: needinfo?(ted)
This was originally in support of node-webrtc, but ack wound up using non-Gecko code there, so I don't care about this anymore.
Flags: needinfo?(ted)
Assignee: ted → nobody
Attachment #813747 - Attachment is obsolete: true
Attachment #815530 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.