Bug 137765

Created attachment 239922 [details] Patch

Created attachment 239924 [details] Patch

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?

(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 ?

Created attachment 239994 [details] Patch

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 ?

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.

(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.

(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.

Created attachment 239998 [details] Patch

(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.

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).

(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.

(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.

Created attachment 240105 [details] Patch for ews

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.

(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"

Created attachment 240202 [details] WIP for mac port

Created attachment 240235 [details] Patch for mac ews

Created attachment 240253 [details] Patch for gtk

Created attachment 240327 [details] Patch for win

Created attachment 240331 [details] Patch for win

Created attachment 240333 [details] Patch for win

Created attachment 240335 [details] Patch

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.

(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 ?

Created attachment 240588 [details] Patch

Created attachment 240590 [details] Patch

(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.

Created attachment 240593 [details] Patch

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 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.

Darin, what do you think about this ?

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.

(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.

Created attachment 246713 [details] Patch

Created attachment 246720 [details] Patch

Created attachment 246728 [details] Patch

(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.

(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);

Created attachment 246736 [details] Patch

(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;

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

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

Created attachment 246944 [details] Patch for landing

Comment on attachment 246944 [details] Patch for landing Clearing flags on attachment: 246944 Committed r180406: <http://trac.webkit.org/changeset/180406>

All reviewed patches have been landed. Closing bug.