RESOLVED FIXED 137765
Use std::unique_ptr instead of PassOwnPtr in MediaPlayerFoo classes
https://bugs.webkit.org/show_bug.cgi?id=137765
Summary Use std::unique_ptr instead of PassOwnPtr in MediaPlayerFoo classes
Gyuyoung Kim
Reported 2014-10-15 20:26:43 PDT
SSIA
Attachments
Patch (7.96 KB, patch)
2014-10-15 20:29 PDT, Gyuyoung Kim
no flags
Patch (10.33 KB, patch)
2014-10-15 20:44 PDT, Gyuyoung Kim
no flags
Patch (13.18 KB, patch)
2014-10-16 21:11 PDT, Gyuyoung Kim
no flags
Patch (14.81 KB, patch)
2014-10-16 22:20 PDT, Gyuyoung Kim
no flags
Patch for ews (33.82 KB, patch)
2014-10-20 03:28 PDT, Gyuyoung Kim
no flags
WIP for mac port (38.64 KB, patch)
2014-10-21 07:32 PDT, Gyuyoung Kim
no flags
Patch for mac ews (40.01 KB, patch)
2014-10-21 18:26 PDT, Gyuyoung Kim
no flags
Patch for gtk (42.10 KB, patch)
2014-10-22 00:01 PDT, Gyuyoung Kim
no flags
Patch for win (41.58 KB, patch)
2014-10-22 21:49 PDT, Gyuyoung Kim
no flags
Patch for win (41.56 KB, patch)
2014-10-22 22:57 PDT, Gyuyoung Kim
no flags
Patch for win (41.56 KB, patch)
2014-10-22 23:08 PDT, Gyuyoung Kim
no flags
Patch (41.66 KB, patch)
2014-10-23 00:46 PDT, Gyuyoung Kim
no flags
Patch (24.23 KB, patch)
2014-10-28 19:14 PDT, Gyuyoung Kim
no flags
Patch (25.78 KB, patch)
2014-10-28 20:34 PDT, Gyuyoung Kim
no flags
Patch (25.74 KB, patch)
2014-10-28 21:38 PDT, Gyuyoung Kim
no flags
Patch (27.30 KB, patch)
2015-02-16 18:30 PST, Gyuyoung Kim
no flags
Patch (26.03 KB, patch)
2015-02-16 20:30 PST, Gyuyoung Kim
no flags
Patch (26.07 KB, patch)
2015-02-16 22:20 PST, Gyuyoung Kim
no flags
Patch (27.46 KB, patch)
2015-02-17 01:13 PST, Gyuyoung Kim
no flags
Patch for landing (26.12 KB, patch)
2015-02-20 01:20 PST, Gyuyoung Kim
no flags
Gyuyoung Kim
Comment 1 2014-10-15 20:29:56 PDT
Gyuyoung Kim
Comment 2 2014-10-15 20:44:28 PDT
Chris Dumez
Comment 3 2014-10-15 21:41:10 PDT
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
Comment 4 2014-10-15 21:45:14 PDT
(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
Comment 5 2014-10-16 21:11:59 PDT
Gyuyoung Kim
Comment 6 2014-10-16 21:14:03 PDT
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 7 2014-10-16 21:22:53 PDT
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
Comment 8 2014-10-16 21:29:12 PDT
(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
Comment 9 2014-10-16 21:36:50 PDT
(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
Comment 10 2014-10-16 22:20:25 PDT
Gyuyoung Kim
Comment 11 2014-10-16 22:23:31 PDT
(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
Comment 12 2014-10-17 13:27:01 PDT
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
Comment 13 2014-10-17 22:47:40 PDT
(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
Comment 14 2014-10-19 21:30:17 PDT
(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
Comment 15 2014-10-20 03:28:21 PDT
Created attachment 240105 [details] Patch for ews
Gyuyoung Kim
Comment 16 2014-10-20 03:32:12 PDT
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
Comment 17 2014-10-20 03:36:51 PDT
(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
Comment 18 2014-10-21 07:32:40 PDT
Created attachment 240202 [details] WIP for mac port
Gyuyoung Kim
Comment 19 2014-10-21 18:26:34 PDT
Created attachment 240235 [details] Patch for mac ews
Gyuyoung Kim
Comment 20 2014-10-22 00:01:45 PDT
Created attachment 240253 [details] Patch for gtk
Gyuyoung Kim
Comment 21 2014-10-22 21:49:01 PDT
Created attachment 240327 [details] Patch for win
Gyuyoung Kim
Comment 22 2014-10-22 22:57:54 PDT
Created attachment 240331 [details] Patch for win
Gyuyoung Kim
Comment 23 2014-10-22 23:08:15 PDT
Created attachment 240333 [details] Patch for win
Gyuyoung Kim
Comment 24 2014-10-23 00:46:52 PDT
Darin Adler
Comment 25 2014-10-28 17:28:48 PDT
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
Comment 26 2014-10-28 17:52:16 PDT
(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
Comment 27 2014-10-28 19:14:05 PDT
Gyuyoung Kim
Comment 28 2014-10-28 20:34:18 PDT
Gyuyoung Kim
Comment 29 2014-10-28 20:35:11 PDT
(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
Comment 30 2014-10-28 21:38:08 PDT
Ryosuke Niwa
Comment 31 2014-10-28 22:45:03 PDT
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
Comment 32 2014-10-28 23:26:28 PDT
(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
Comment 33 2014-10-30 18:49:00 PDT
Darin, what do you think about this ?
Darin Adler
Comment 34 2014-10-30 19:56:32 PDT
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
Comment 35 2014-10-30 22:07:16 PDT
(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
Comment 36 2015-02-16 18:30:29 PST
Gyuyoung Kim
Comment 37 2015-02-16 20:30:18 PST
Gyuyoung Kim
Comment 38 2015-02-16 22:20:24 PST
Gyuyoung Kim
Comment 39 2015-02-16 23:16:18 PST
(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
Comment 40 2015-02-16 23:28:10 PST
(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
Comment 41 2015-02-17 01:13:12 PST
Gyuyoung Kim
Comment 42 2015-02-17 01:50:39 PST
(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
Comment 43 2015-02-18 03:55:58 PST
Darin and Chris, could you take a look this patch again ?
WebKit Commit Bot
Comment 44 2015-02-19 21:41:15 PST
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
Comment 45 2015-02-20 01:20:24 PST
Created attachment 246944 [details] Patch for landing
WebKit Commit Bot
Comment 46 2015-02-20 02:10:48 PST
Comment on attachment 246944 [details] Patch for landing Clearing flags on attachment: 246944 Committed r180406: <http://trac.webkit.org/changeset/180406>
WebKit Commit Bot
Comment 47 2015-02-20 02:10:57 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.