Alan K [:ack] Reporter
	Description • 11 years ago

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.

	Adam Roach [:abr]
	Comment 1 • 11 years ago

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.

	Randell Jesup [:jesup] (needinfo me)
	Comment 2 • 11 years ago

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).

	Eric Rescorla (:ekr)
	Comment 3 • 11 years ago

(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.

	Adam Roach [:abr]
	Comment 4 • 11 years ago

(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.

	(not currently active) Ted Mielczarek
	Comment 5 • 11 years ago

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.

	(not currently active) Ted Mielczarek
	Comment 6 • 11 years ago

This passes the dom/media mochitests and the signalling C++ tests.

	(not currently active) Ted Mielczarek
	Comment 7 • 11 years ago

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.

	Randell Jesup [:jesup] (needinfo me)
	Comment 8 • 11 years ago

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

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 9 • 11 years ago

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

	(not currently active) Ted Mielczarek
	Comment 10 • 11 years ago

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.

	(not currently active) Ted Mielczarek
	Comment 11 • 11 years ago

I missed a few #ifdefs in the PeerConnection code. I think I'd like to land this patch instead since it's less churn.

	Randell Jesup [:jesup] (needinfo me)
	Comment 12 • 11 years ago

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

	Alan K [:ack] Reporter
	Comment 13 • 11 years ago

::: 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?

	(not currently active) Ted Mielczarek
	Comment 14 • 11 years ago

(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.

	Maire Reavy [:mreavy]
	Comment 15 • 9 years ago

Ted -- Is it useful to land these patches for the standalone or elsewhere?

	(not currently active) Ted Mielczarek
	Comment 16 • 9 years ago

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.