1251715 - use UniquePtr instead of ScopedDeletePtr in dom/media/

Status: RESOLVED FIXED

(Core :: Audio/Video, defect, P2)

Description

[Nathan Froyd [:froydnj]]

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

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: use UniquePtr instead of ScopedDeletePtr in dom/media/

Comment 2

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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

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

r+ for patch as is, but:

- You didn't remove the ScopedDeletePtr in dom/media/VideoSegment.cpp#56 (though it's an array, are you doing these separately?)

- Small inconsistency, not a problem if you prefer to keep it:

::: dom/media/systemservices/MediaChild.cpp
@@ +45,5 @@
>  
>    if (XRE_GetProcessType() == GeckoProcessType_Default) {
>      // Avoid opening MediaManager in this case, since this is called by
>      // sanitize.js when cookies are cleared, which can happen on startup.
> +    UniquePtr<Parent<NonE10s>> tmpParent(new Parent<NonE10s>(true));

Why not |auto tmpParent = MakeUnique<...| as you've done in e.g., MediaManager.cpp#1397?

Comment 3

[Nathan Froyd [:froydnj]]

Thanks for the review!

(In reply to Gerald Squelart [:gerald] from comment #2)
> - You didn't remove the ScopedDeletePtr in dom/media/VideoSegment.cpp#56
> (though it's an array, are you doing these separately?)

I did...I just did it in a different patch in my queue because it was solving a different problem (new/free mismatch)!  Will fold it in.

> ::: dom/media/systemservices/MediaChild.cpp
> @@ +45,5 @@
> >  
> >    if (XRE_GetProcessType() == GeckoProcessType_Default) {
> >      // Avoid opening MediaManager in this case, since this is called by
> >      // sanitize.js when cookies are cleared, which can happen on startup.
> > +    UniquePtr<Parent<NonE10s>> tmpParent(new Parent<NonE10s>(true));
> 
> Why not |auto tmpParent = MakeUnique<...| as you've done in e.g.,
> MediaManager.cpp#1397?

I tried that, and it was giving me peculiar errors...but I just tried again, and it worked!  Must have been doing something dumb last time.

New patch coming.

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: use UniquePtr instead of ScopedDeletePtr in dom/media/

New patch with fixes for review comments.

Comment 5

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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

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

r+ with nit:

::: dom/media/systemservices/MediaChild.cpp
@@ +46,5 @@
>    if (XRE_GetProcessType() == GeckoProcessType_Default) {
>      // Avoid opening MediaManager in this case, since this is called by
>      // sanitize.js when cookies are cleared, which can happen on startup.
> +    //UniquePtr<Parent<NonE10s>> tmpParent(new Parent<NonE10s>(true));
> +    auto tmpParent = MakeUnique<Parent<NonE10s>>(true);

You forgot to remove the commented code. :-)

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/80b197c5608f

Comment 7

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

This and bug 1251714 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 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bfd9d073c88e

Comment 9

[Nathan Froyd [:froydnj]]

Relanded after an all-platform try run:

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

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bfd9d073c88e