1251714 - use UniquePtr instead of ScopedDeletePtr in media/

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P2)

Description

[Nathan Froyd [:froydnj]]

UniquePtr is more standard than ScopedDeletePtr; using standard
constructs whenever possible is preferable.

This patch merits a couple explanations:

- Where it made sense, I tried to convert:

    T* foo() {
      UniquePtr<T> x = ...;
      ...
      return x.release();
    }

  into:

    UniquePtr<T> foo()

  with corresponding changes inside |foo|'s body.

- The attentive reader will note that:

    auto x = MakeUnique<T>(...);

  is used sometimes and:

    UniquePtr<T> x(new T(...));

  is used sometimes.  I would prefer to use the former, but was stymied
  in several places due to protected constructors.  (MakeUnique doesn't
  have access to those protected constructors, natch.)

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: use UniquePtr instead of ScopedDeletePtr in media/

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: use UniquePtr instead of ScopedDeletePtr in media/

Update with some other patches I had lying about to fix new/free mismatch
issues.

Comment 3

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

Comment on attachment 8724800 [details] [diff] [review]
use UniquePtr instead of ScopedDeletePtr in media/

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

::: media/mtransport/transportflow.cpp
@@ +34,5 @@
>    // is maintained, it shouldn't be possible to access and
>    // destroy it simultaneously. The conversion to an nsAutoPtr
>    // ensures automatic destruction of the queue at exit of
>    // DestroyFinal.
> +  nsAutoPtr<std::deque<TransportLayer*> > layers_tmp(layers_.release());

Since you're touching it, "> >" -> ">>"

::: media/mtransport/transportflow.h
@@ +134,5 @@
>    static void ClearLayers(std::queue<TransportLayer *>* layers);
>  
>    std::string id_;
>    TransportLayer::State state_;
> +  UniquePtr<std::deque<TransportLayer *> > layers_;

ditto

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +512,5 @@
>      return;
>    }
>  
>    // Make a copy rather than cast away constness
> +  auto inner_data = MakeUnique<unsigned char[]>(len);

I presume this does the correct delete []

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1bbd0cd10f76

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This and bug 1251715 are backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/7e80b936414e for gtest crashes in media code: https://treeherder.mozilla.org/logviewer.html#?job_id=22754484&repo=mozilla-inbound

Comment 6

[Pulsebot]

Backout:
https://hg.mozilla.org/mozilla-central/rev/71fabdbb3567

Comment 7

[Nathan Froyd [:froydnj]]

Bleh.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/181f18c5d1ad

Comment 9

[Nathan Froyd [:froydnj]]

(In reply to Pulsebot from comment #8)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/181f18c5d1ad

I fixed the gtest issues by not using static UniquePtr instances; apparently some of the tests don't shut things down properly. =/

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a7c710ff0a6

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/181f18c5d1ad