Closed Bug 1251715 Opened 4 years ago Closed 4 years ago

use UniquePtr instead of ScopedDeletePtr in dom/media/

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

Details

Attachments

(1 file, 1 obsolete file)

UniquePtr is more standard than ScopedDeletePtr; using standard
constructs whenever possible is preferable.
Attachment #8724180 - Flags: review?(gsquelart)
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?
Attachment #8724180 - Flags: review?(gsquelart) → review+
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.
New patch with fixes for review comments.
Attachment #8724814 - Flags: review?(gsquelart)
Attachment #8724180 - Attachment is obsolete: true
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. :-)
Attachment #8724814 - Flags: review?(gsquelart) → review+
Priority: -- → P2
Relanded after an all-platform try run:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d665b3304c97
Flags: needinfo?(nfroyd)
https://hg.mozilla.org/mozilla-central/rev/bfd9d073c88e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.