808432 - Adding async handling for sending through the transport

Status: NEW

(Core :: WebRTC: Networking, defect, P3)

Comment 1

[Adam Roach [:abr]]

Attachment: Adding async handling for sending through the transport

Comment 2

[Eric Rescorla (:ekr)]

Comment on attachment 678158 [details] [diff] [review]
Adding async handling for sending through the transport

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

::: media/mtransport/test/transport_unittests.cpp
@@ +289,4 @@
>    }
>  
> +  void SetAsync(bool f) { 
> +    send_async_ = f;

Remove trailing whitespace.

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 678158 [details] [diff] [review]
Adding async handling for sending through the transport

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

::: media/mtransport/test/transport_unittests.cpp
@@ +276,5 @@
>  
>    TransportResult SendPacket(const unsigned char* data, size_t len) {
> +    if (send_async_) {
> +      return flow_->SendPacket(data, len, TF_ASYNC);
> +    } else {

Minor style: 
  if (foo) {
    return bar;
  }
  return xyz

@@ +278,5 @@
> +    if (send_async_) {
> +      return flow_->SendPacket(data, len, TF_ASYNC);
> +    } else {
> +      TransportResult ret;
> +      

trailing whitespace

::: media/mtransport/transportflow.h
@@ +53,5 @@
>    // at the time. This way you don't need to do top()->Foo().
>    TransportLayer::State state(); // Current state
> +
> +  // See transportlayer.h for flags
> +  TransportResult SendPacket(const unsigned char *data, size_t len, 

trailing space

Comment 4

[Eric Rescorla (:ekr)]

Comment on attachment 678158 [details] [diff] [review]
Adding async handling for sending through the transport

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

lgtm with changes below.

::: media/mtransport/test/transport_unittests.cpp
@@ +97,5 @@
>  class TransportTestPeer : public sigslot::has_slots<> {
>   public:
>    TransportTestPeer(nsCOMPtr<nsIEventTarget> target, std::string name)
> +      : send_async_(false),
> +        name_(name), target_(target),

These can all go on one line.

::: media/mtransport/transportlayer.cpp
@@ +64,5 @@
> +
> +  unsigned char *data_copy = new unsigned char[len];
> +  memcpy(data_copy, data, len);
> +
> +  target_->Dispatch(WrapRunnable(this, &TransportLayer::SendPacketInt, 

Please add a comment that we always return success.

Comment 5

[Adam Roach [:abr]]

Attachment: Added flags field to TransportFlow. Defined and implemented a TF_ASYNC flag to indicate that data should be sent out asynchronously (iff we're not sending from the socket thread).

Comment 6

[Matthew N. [:MattN]]

Randell, is this waiting on something to land?

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 678537 [details] [diff] [review]
Added flags field to TransportFlow. Defined and implemented a TF_ASYNC flag to indicate that data should be sent out asynchronously (iff we're not sending from the socket thread).

Carrying forward r+ (myself and ekr)

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 678537 [details] [diff] [review]
Added flags field to TransportFlow. Defined and implemented a TF_ASYNC flag to indicate that data should be sent out asynchronously (iff we're not sending from the socket thread).

Cancelling checkin? until I have code to make use of this feature in DataChannels, and reassigning the bug to myself to do so.

Comment 9

[Bulk Bug Changes for mreavy's org]

Mass change P2->P3 to align with new Mozilla triage process.

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

Not sure we have need of this.