Bug 137765 – Use std::unique_ptr instead of PassOwnPtr in MediaPlayerFoo classes

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Gyuyoung Kim]

SSIA

[Gyuyoung Kim]

Created attachment 239922 [details]
Patch

[Gyuyoung Kim]

Created attachment 239924 [details]
Patch

[Chris Dumez]

Comment on attachment 239924 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=239924&action=review

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:61
> +    explicit MediaPlayerPrivateGStreamer(MediaPlayer*);

I don't like that we have to make those constructors public. It is weird to have both a factory function and a public constructor. How about we don't use make_unique to avoid this issue?

[Gyuyoung Kim]

(In reply to comment #3)
> (From update of attachment 239924 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239924&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:61
> > +    explicit MediaPlayerPrivateGStreamer(MediaPlayer*);
> 
> I don't like that we have to make those constructors public. It is weird to have both a factory function and a public constructor. How about we don't use make_unique to avoid this issue?

Darin, what do you think about chris comment ?

[Gyuyoung Kim]

Created attachment 239994 [details]
Patch

[Gyuyoung Kim]

create() factory function is still needed by media engine register function pointer.

typedef void (*MediaEngineRegistrar)(CreateMediaEnginePlayer, MediaEngineSupportedTypes, MediaEngineSupportsType,
    MediaEngineGetSitesInMediaCache, MediaEngineClearMediaCache, MediaEngineClearMediaCacheForSite, MediaEngineSupportsKeySystem);

Thus my approach is that create() function is set as private, then it is only used by the register function. How about this change ?

[Chris Dumez]

Comment on attachment 239994 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=239994&action=review

> Source/WebCore/platform/graphics/MediaPlayer.h:576
> +    static std::unique_ptr<MediaPlayer> create(MediaPlayerClient& client)

I don't get it. Why does it make sense to make the constructor public but the factory function private? This seems reversed.

[Gyuyoung Kim]

(In reply to comment #7)
> Comment on attachment 239994 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=239994&action=review
> 
> > Source/WebCore/platform/graphics/MediaPlayer.h:576
> > +    static std::unique_ptr<MediaPlayer> create(MediaPlayerClient& client)
> 
> I don't get it. Why does it make sense to make the constructor public but
> the factory function private? This seems reversed.


Because Darin wanted to use std::make_unique<> instead of std::unique_ptr.
https://bugs.webkit.org/show_bug.cgi?id=137738#c3


If Darin is fine to use std::unique_ptr, I don't mind to use std::unique_ptr instead of std::make_unique.

[Chris Dumez]

(In reply to comment #8)
> (In reply to comment #7)
> > Comment on attachment 239994 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=239994&action=review
> > 
> > > Source/WebCore/platform/graphics/MediaPlayer.h:576
> > > +    static std::unique_ptr<MediaPlayer> create(MediaPlayerClient& client)
> > 
> > I don't get it. Why does it make sense to make the constructor public but
> > the factory function private? This seems reversed.
> 
> 
> Because Darin wanted to use std::make_unique<> instead of std::unique_ptr.
> https://bugs.webkit.org/show_bug.cgi?id=137738#c3
> 
> 
> If Darin is fine to use std::unique_ptr, I don't mind to use std::unique_ptr
> instead of std::make_unique.

The thing is that I am not sure Darin realized using make_unique here would mean making the constructors public because the patch he reviewed had the constructors private (only the post-review patch that landed had the constructors public).

Anyway, I'll defer to Darin but I am not a fan of making the constructors public.

[Gyuyoung Kim]

Created attachment 239998 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > Comment on attachment 239994 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=239994&action=review
> > > 
> > > > Source/WebCore/platform/graphics/MediaPlayer.h:576
> > > > +    static std::unique_ptr<MediaPlayer> create(MediaPlayerClient& client)
> > > 
> > > I don't get it. Why does it make sense to make the constructor public but
> > > the factory function private? This seems reversed.
> > 
> > 
> > Because Darin wanted to use std::make_unique<> instead of std::unique_ptr.
> > https://bugs.webkit.org/show_bug.cgi?id=137738#c3
> > 
> > 
> > If Darin is fine to use std::unique_ptr, I don't mind to use std::unique_ptr
> > instead of std::make_unique.
> 
> The thing is that I am not sure Darin realized using make_unique here would
> mean making the constructors public because the patch he reviewed had the
> constructors private (only the post-review patch that landed had the
> constructors public).
> 
> Anyway, I'll defer to Darin but I am not a fan of making the constructors
> public.

Chris, your comment makes more sense. I fix this patch using std::unique_ptr, then keep ctor as private. I wonder how does Darin think.

[Chris Dumez]

I talked to Andreas and he told me that he usually got rid of the factory functions altogether, made the constructors public and used make_unique<>() at call sites. I like this, what do you think?

At least it would be consistent with the classes that Andreas already ported a while back and we would get to use make_unique<>() without any inconsistency (i.e. like having both a public factory and a public constructor).

[Darin Adler]

(In reply to comment #12)
> I talked to Andreas and he told me that he usually got rid of the factory
> functions altogether, made the constructors public and used make_unique<>()
> at call sites. I like this, what do you think?
> 
> At least it would be consistent with the classes that Andreas already ported
> a while back and we would get to use make_unique<>() without any
> inconsistency (i.e. like having both a public factory and a public
> constructor).

I like this suggestion.

It’s actually Anders who strongly suggests using make_unique. We have often made constructors public so that we could use make_unique and avoid invoking new directly. I think it does indeed then make sense to get rid of the create function entirely.

[Gyuyoung Kim]

(In reply to comment #13)
> (In reply to comment #12)
> > I talked to Andreas and he told me that he usually got rid of the factory
> > functions altogether, made the constructors public and used make_unique<>()
> > at call sites. I like this, what do you think?
> > 
> > At least it would be consistent with the classes that Andreas already ported
> > a while back and we would get to use make_unique<>() without any
> > inconsistency (i.e. like having both a public factory and a public
> > constructor).
> 
> I like this suggestion.
> 
> It’s actually Anders who strongly suggests using make_unique. We have often
> made constructors public so that we could use make_unique and avoid invoking
> new directly. I think it does indeed then make sense to get rid of the
> create function entirely.

Ok, Darin, let me try to remove create() factory function here. It might be a little hard to remove it because MediaPlayer is using factory pattern to create child media player.

[Gyuyoung Kim]

Created attachment 240105 [details]
Patch for ews

[Gyuyoung Kim]

I remove all create() factory function in MediaPlayerFoo classes in order to use make_unique<>. Thus I delegate MediaPlayerFactory creation from MediaPlayerFoo to MediaPlayerFactory because MediaPlayerFactory has used the create() factory to create instance. 

Darin and Chris, how do you think about this approach ? If you guys have better idea, please let me know.

[Gyuyoung Kim]

(In reply to comment #16)
> I remove all create() factory function in MediaPlayerFoo classes in order to
> use make_unique<>. Thus I delegate MediaPlayerFactory creation from
> MediaPlayerFoo to MediaPlayerFactory because MediaPlayerFactory has used the
> create() factory to create instance. 
> 
> Darin and Chris, how do you think about this approach ? If you guys have
> better idea, please let me know.

Typo: I delegate MediaPlayerFactory creation from MediaPlayerFoo to MediaPlayerFactory

-> I delegate MediaPlayerFoo creation from "MediaPlayerFoo" factory to "MediaPlayerFactory"

[Gyuyoung Kim]

Created attachment 240202 [details]
WIP for mac port

[Gyuyoung Kim]

Created attachment 240235 [details]
Patch for mac ews

[Gyuyoung Kim]

Created attachment 240253 [details]
Patch for gtk

[Gyuyoung Kim]

Created attachment 240327 [details]
Patch for win

[Gyuyoung Kim]

Created attachment 240331 [details]
Patch for win

[Gyuyoung Kim]

Created attachment 240333 [details]
Patch for win

[Gyuyoung Kim]

Created attachment 240335 [details]
Patch

[Darin Adler]

I think we pushed you in a strange direction here. It seems fine to use factory functions rather than an enum to make these objects. The desire to use std::make_unique should not drive us to abandon that pattern entirely.

[Gyuyoung Kim]

(In reply to comment #25)
> I think we pushed you in a strange direction here. It seems fine to use
> factory functions rather than an enum to make these objects. The desire to
> use std::make_unique should not drive us to abandon that pattern entirely.

If so, we should keep create() factory function and public constructor as well. If we don't want to keep both, I think it is best to use std::unique_ptr in create() factory function. What do you think about it ?

[Gyuyoung Kim]

Created attachment 240588 [details]
Patch

[Gyuyoung Kim]

Created attachment 240590 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #26)
> (In reply to comment #25)
> > I think we pushed you in a strange direction here. It seems fine to use
> > factory functions rather than an enum to make these objects. The desire to
> > use std::make_unique should not drive us to abandon that pattern entirely.
> 
> If so, we should keep create() factory function and public constructor as
> well. If we don't want to keep both, I think it is best to use
> std::unique_ptr in create() factory function. What do you think about it ?

Darin, I modify this patch using std::unique_ptr for create() factory function. I think this is best for now.

[Gyuyoung Kim]

Created attachment 240593 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 240593 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=240593&action=review

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:389
> -PassOwnPtr<MediaPlayerPrivateInterface> MediaPlayerPrivateAVFoundationObjC::create(MediaPlayer* player)
> +std::unique_ptr<MediaPlayerPrivateInterface> MediaPlayerPrivateAVFoundationObjC::create(MediaPlayer* player)
>  { 
> -    return adoptPtr(new MediaPlayerPrivateAVFoundationObjC(player));
> +    return std::unique_ptr<MediaPlayerPrivateAVFoundationObjC>(new MediaPlayerPrivateAVFoundationObjC(player));
>  }

I thought we're removing these create functions?

[Gyuyoung Kim]

(In reply to comment #31)
> Comment on attachment 240593 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=240593&action=review
> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:389
> > -PassOwnPtr<MediaPlayerPrivateInterface> MediaPlayerPrivateAVFoundationObjC::create(MediaPlayer* player)
> > +std::unique_ptr<MediaPlayerPrivateInterface> MediaPlayerPrivateAVFoundationObjC::create(MediaPlayer* player)
> >  { 
> > -    return adoptPtr(new MediaPlayerPrivateAVFoundationObjC(player));
> > +    return std::unique_ptr<MediaPlayerPrivateAVFoundationObjC>(new MediaPlayerPrivateAVFoundationObjC(player));
> >  }
> 
> I thought we're removing these create functions?

In this case, we need to use create() because MediaPlayer is using factory pattern to make proper media player.

I already tried to remove create() in below patch. However, it looks this is too poor architecture.

https://bugs.webkit.org/attachment.cgi?id=240335&action=review

If you have good idea to replace the factory pattern without create() function, please let me know. I will try it.

[Gyuyoung Kim]

Darin, what do you think about this ?

[Darin Adler]

We have three separate goals:

1) Eliminate "new" entirely from WebKit code, long term; we want objects to go straight into the appropriate type of smart pointer. We use std::make_unique<X>(y) instead of using std::unique_ptr<X>(new X(y)) because of this.

2) Use the factory pattern for CreateMediaEnginePlayer and the callRegisterMediaEngine function.

3) Minimize the pattern where classes supply functions named "create" that wrap newly allocated object in smart pointers. That's because std::make_unique does the job even better, moving the new entirely out of WebKit code. We still need the "create" functions for RefCounted, though.

Given these goals I would suggest one of these patterns:

a) Make the constructors of these classes public, make the factory functions be local functions with internal linkage ("static") just above the various registerMediaEngine functions, have these factory functions call std::make_unique, and remove the unneeded static member functions named create.

b) Make the constructors of these classes public, make the factory functions be lambdas inside the registerMediaEngine functions, and have these lambdas call std::make_unique, and remove the unneeded static member functions named create.

While I understand that we need functions to pass to the registrar, I do not think they need to be static member functions named create, nor do I think we need to keep the constructors of these classes private.

[Gyuyoung Kim]

(In reply to comment #34)
> We have three separate goals:
> 
> 1) Eliminate "new" entirely from WebKit code, long term; we want objects to
> go straight into the appropriate type of smart pointer. We use
> std::make_unique<X>(y) instead of using std::unique_ptr<X>(new X(y)) because
> of this.
> 
> 2) Use the factory pattern for CreateMediaEnginePlayer and the
> callRegisterMediaEngine function.
> 
> 3) Minimize the pattern where classes supply functions named "create" that
> wrap newly allocated object in smart pointers. That's because
> std::make_unique does the job even better, moving the new entirely out of
> WebKit code. We still need the "create" functions for RefCounted, though.
> 
> Given these goals I would suggest one of these patterns:
> 
> a) Make the constructors of these classes public, make the factory functions
> be local functions with internal linkage ("static") just above the various
> registerMediaEngine functions, have these factory functions call
> std::make_unique, and remove the unneeded static member functions named
> create.
> 
> b) Make the constructors of these classes public, make the factory functions
> be lambdas inside the registerMediaEngine functions, and have these lambdas
> call std::make_unique, and remove the unneeded static member functions named
> create.
 
> While I understand that we need functions to pass to the registrar, I do not
> think they need to be static member functions named create, nor do I think
> we need to keep the constructors of these classes private.

Thank you for your detailed suggestion. Now I understand you want to remove create() factory function, and then we use std::make_unique<> for the registerMediaEngine() using internal linkage or lambdas. Let me try to apply your suggestions. It looks I tried to solve this issue with too simple approach.

[Gyuyoung Kim]

Created attachment 246713 [details]
Patch

[Gyuyoung Kim]

Created attachment 246720 [details]
Patch

[Gyuyoung Kim]

Created attachment 246728 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #34)

> Given these goals I would suggest one of these patterns:
> 
> a) Make the constructors of these classes public, make the factory functions
> be local functions with internal linkage ("static") just above the various
> registerMediaEngine functions, have these factory functions call
> std::make_unique, and remove the unneeded static member functions named
> create.
> 
> b) Make the constructors of these classes public, make the factory functions
> be lambdas inside the registerMediaEngine functions, and have these lambdas
> call std::make_unique, and remove the unneeded static member functions named
> create.
> 
> While I understand that we need functions to pass to the registrar, I do not
> think they need to be static member functions named create, nor do I think
> we need to keep the constructors of these classes private.

I want to use b) method though, I fail to support b) because lambdas needs to use a MediaPlayer argument in registerMediaEngine(). To use lambdas inside registerMediaEngine(), I think we need to support to access a MediaPlayer instance. So I upload a patch which is based on a) suggestion at the moment.

[Chris Dumez]

(In reply to comment #39)
> (In reply to comment #34)
> 
> > Given these goals I would suggest one of these patterns:
> > 
> > a) Make the constructors of these classes public, make the factory functions
> > be local functions with internal linkage ("static") just above the various
> > registerMediaEngine functions, have these factory functions call
> > std::make_unique, and remove the unneeded static member functions named
> > create.
> > 
> > b) Make the constructors of these classes public, make the factory functions
> > be lambdas inside the registerMediaEngine functions, and have these lambdas
> > call std::make_unique, and remove the unneeded static member functions named
> > create.
> > 
> > While I understand that we need functions to pass to the registrar, I do not
> > think they need to be static member functions named create, nor do I think
> > we need to keep the constructors of these classes private.
> 
> I want to use b) method though, I fail to support b) because lambdas needs
> to use a MediaPlayer argument in registerMediaEngine(). To use lambdas
> inside registerMediaEngine(), I think we need to support to access a
> MediaPlayer instance. So I upload a patch which is based on a) suggestion at
> the moment.

Could you clarify what doesn't work (it is not clear to me at least).

Why doesn't something like this work?
registrar([](MediaPlayer* player) { return std::make_unique<MediaPlayerPrivateAVFoundationCF>(player); }, getSupportedTypes, supportsType, 0, 0, 0, supportsKeySystem);

[Gyuyoung Kim]

Created attachment 246736 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #40)
> 
> Could you clarify what doesn't work (it is not clear to me at least).
> 
> Why doesn't something like this work?
> registrar([](MediaPlayer* player) { return
> std::make_unique<MediaPlayerPrivateAVFoundationCF>(player); },
> getSupportedTypes, supportsType, 0, 0, 0, supportsKeySystem);

I needed to modify "CreateMeidaEnginePlayer" in order to use lambdas as well. Thanks to your comment, I could update my patch again.

typedef std::function<std::unique_ptr<MediaPlayerPrivateInterface> (MediaPlayer*)> CreateMediaEnginePlayer;

[Gyuyoung Kim]

Darin and Chris, could you take a look this patch again ?

[WebKit Commit Bot]

Comment on attachment 246736 [details]
Patch

Rejecting attachment 246736 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 246736, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
tching file Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp
patching file Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.h
patching file Source/WebCore/platform/mock/mediasource/MockMediaPlayerMediaSource.cpp
patching file Source/WebCore/platform/mock/mediasource/MockMediaPlayerMediaSource.h

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Ryosuke Niwa']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.appspot.com/results/5137653796175872

[Gyuyoung Kim]

Created attachment 246944 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 246944 [details]
Patch for landing

Clearing flags on attachment: 246944

Committed r180406: <http://trac.webkit.org/changeset/180406>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.