Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2905613003:
Extract media code from RenderFrameImpl (Closed)
|
Created:
3 years, 6 months ago by chcunningham Modified:
3 years, 6 months ago CC:
chfremer+watch_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, posciak+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionExtract media code from RenderFrameImpl
RenderFrameImpl has grown to contain nearly 500 lines of media plumbing.
This CL extracts much of this into MediaFactory.
This is done in anticipation of adding a MediaCapabilities API to the
RenderFrame.
BUG=695264, 588408
Review-Url: https://codereview.chromium.org/2905613003
Cr-Commit-Position: refs/heads/master@{#475659}
Committed: https://chromium.googlesource.com/chromium/src/+/86f025e31d8d3676fea772a36eb9ecece61877ff
Patch Set 1 #
Total comments: 6
Patch Set 2 : Rebase and small fixes #
Total comments: 2
Patch Set 3 : Feedback #Patch Set 4 : Remove #error debugging #Patch Set 5 : Dont cache remote interfaces ptr #Patch Set 6 : MediaFactory, cleaner responsibilities. Mojo fix #
Total comments: 18
Patch Set 7 : Rebase #Patch Set 8 : Feedback #
Total comments: 6
Patch Set 9 : Feedback #
Messages
Total messages: 70 (48 generated)
The CQ bit was checked by chcunningham@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
chcunningham@chromium.org changed reviewers: + tguilbert@chromium.org
Hey Thomas, PTAL. This mostly copy paste from RenderFrameImpl -> RenderMediaHelper. There are a few media things that linger in RenderFrameImpl: 1. UserMediaClient stuff. I left this here because its very simple and RenderFrameImpl has a public API for returning the UserMediaClient, so it seemed a bit much to add a layer to the call stack. 2. RequestOverlayRoutingToken and related methods. These rely on RenderFrame to be the IPC Sender/Receiver. They're pretty simple and moving them feels over-engineered.
dalecurtis@chromium.org changed reviewers: + xhwang@chromium.org
+xhwang since he's most familiar with a lot of this render frame code.
Thanks for doing this!!! Actually there's a dedicated bug for this: https://bugs.chromium.org/p/chromium/issues/detail?id=588408 The code looks good % nits. But it seems you need to rebase :) https://codereview.chromium.org/2905613003/diff/1/content/renderer/media/rend... File content/renderer/media/render_media_helper.h (right): https://codereview.chromium.org/2905613003/diff/1/content/renderer/media/rend... content/renderer/media/render_media_helper.h:71: class RenderMediaHelper { naming bikeshedding: We are already in content/renderer, so can we just name it MediaHelper? We do have RenderMediaClient and RenderMediaLog, but they are subclass of MediaClient and MediaLog so we have to add something to the dereived class. https://codereview.chromium.org/2905613003/diff/1/content/renderer/media/rend... content/renderer/media/render_media_helper.h:132: #if defined(ENABLE_MOJO_MEDIA) nit: you need rebase :) https://codereview.chromium.org/2905613003/diff/1/content/renderer/render_fra... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2905613003/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:1061: base::Unretained(this)))); Given we always create |media_helper_| during construction, does it make sense to have a RenderMediaHelper member directly, instead of using a unique_ptr?
The CQ bit was checked by chcunningham@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Extract media code from RenderFrameImpl RenderFrameImpl has grown to contain nearly 500 lines of media plumbing. This CL extracts almost all of this into RenderMediaHelper and hides the details behind 4 public APIs. This is done in anticipation of adding a MediaCapabilities API to the RenderFrame. BUG=695264 ========== to ========== Extract media code from RenderFrameImpl RenderFrameImpl has grown to contain nearly 500 lines of media plumbing. This CL extracts almost all of this into RenderMediaHelper and hides the details behind 4 public APIs. This is done in anticipation of adding a MediaCapabilities API to the RenderFrame. BUG=695264,588408 ==========
LGTM https://codereview.chromium.org/2905613003/diff/20001/content/renderer/media/... File content/renderer/media/render_media_helper.h (right): https://codereview.chromium.org/2905613003/diff/20001/content/renderer/media/... content/renderer/media/render_media_helper.h:152: // communicating with the real media player and sessions in the This comment is out of date. The RendererMediaPlayerManager is only used in the WebMediaPlayerCast scenario at the moment. I think the following would be more accurate: Manages media players and sessions in this render frame for communicating with the real media player and sessions in the browser process. NOTE: This currently only being used in the case where we are casting. See also WebMediaPlayerCast (renderer side) and RemoteMediaPlayerManager (browser side).
Thanks guys. https://codereview.chromium.org/2905613003/diff/1/content/renderer/media/rend... File content/renderer/media/render_media_helper.h (right): https://codereview.chromium.org/2905613003/diff/1/content/renderer/media/rend... content/renderer/media/render_media_helper.h:71: class RenderMediaHelper { On 2017/05/24 23:41:28, xhwang wrote: > naming bikeshedding: We are already in content/renderer, so can we just name it > MediaHelper? We do have RenderMediaClient and RenderMediaLog, but they are > subclass of MediaClient and MediaLog so we have to add something to the dereived > class. Done. https://codereview.chromium.org/2905613003/diff/1/content/renderer/media/rend... content/renderer/media/render_media_helper.h:132: #if defined(ENABLE_MOJO_MEDIA) On 2017/05/24 23:41:28, xhwang wrote: > nit: you need rebase :) Done. https://codereview.chromium.org/2905613003/diff/1/content/renderer/render_fra... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2905613003/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:1061: base::Unretained(this)))); On 2017/05/24 23:41:28, xhwang wrote: > Given we always create |media_helper_| during construction, does it make sense > to have a RenderMediaHelper member directly, instead of using a unique_ptr? Done. https://codereview.chromium.org/2905613003/diff/20001/content/renderer/media/... File content/renderer/media/render_media_helper.h (right): https://codereview.chromium.org/2905613003/diff/20001/content/renderer/media/... content/renderer/media/render_media_helper.h:152: // communicating with the real media player and sessions in the On 2017/05/25 00:35:39, tguilbert wrote: > This comment is out of date. The RendererMediaPlayerManager is only used in the > WebMediaPlayerCast scenario at the moment. I think the following would be more > accurate: > > Manages media players and sessions in this render frame for communicating with > the real media player and sessions in the browser process. > NOTE: This currently only being used in the case where we are casting. See also > WebMediaPlayerCast (renderer side) and RemoteMediaPlayerManager (browser side). Done.
chcunningham@chromium.org changed reviewers: + creis@chromium.org
+creis for content
chcunningham@chromium.org changed reviewers: + jochen@chromium.org - creis@chromium.org
-creis (Didn't see the overloaded) +jochen
The CQ bit was checked by chcunningham@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by chcunningham@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by chcunningham@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
more bikeshedding: since this "helper" actually has a lot of Create*() and Get*() methods, does it make sense to call it MediaFactory? Ignore me if this doesn't make sense to me. jochen: FYI, after this CL, media/ OWNERS will start to own this file, though this really is part of RenderFrameImpl. Let us know if this is a concern. One options is to keep this in the same folder as render_frame_impl.* and name it RenderFrameMediaFactory. But I am not sure whether it's worth it.
On 2017/05/25 22:08:57, xhwang wrote: > more bikeshedding: since this "helper" actually has a lot of Create*() and > Get*() methods, does it make sense to call it MediaFactory? Ignore me if this > doesn't make sense to me. Ok, but you only get one more rename ;). Are you sure about this one? Will wait on jochen to reply on your other comment before proceeding.
On 2017/05/25 22:46:44, chcunningham wrote: > On 2017/05/25 22:08:57, xhwang wrote: > > more bikeshedding: since this "helper" actually has a lot of Create*() and > > Get*() methods, does it make sense to call it MediaFactory? Ignore me if this > > doesn't make sense to me. > > Ok, but you only get one more rename ;). Are you sure about this one? Will wait > on jochen to reply on your other comment before proceeding. After thinking more, void CheckIfAudioSinkExistsAndIsAuthorized(...) is very non-factory sounding. Maybe "helper" is a more general name.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2017/05/25 22:52:50, chcunningham wrote: > On 2017/05/25 22:46:44, chcunningham wrote: > > On 2017/05/25 22:08:57, xhwang wrote: > > > more bikeshedding: since this "helper" actually has a lot of Create*() and > > > Get*() methods, does it make sense to call it MediaFactory? Ignore me if > this > > > doesn't make sense to me. > > > > Ok, but you only get one more rename ;). Are you sure about this one? Will > wait > > on jochen to reply on your other comment before proceeding. > > After thinking more, void CheckIfAudioSinkExistsAndIsAuthorized(...) is very > non-factory sounding. Maybe "helper" is a more general name. Sure. It's totally up to you at this point :)
The CQ bit was checked by chcunningham@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
stuff outside of media/ lgtm
While you're here, did you see any functions which might benefit from a small unittest? I.e. even things such as calling with a bunch of null values to ensure a constructed object, etc. FWIW, almost any time you end up with a file or class that ends in helper or util, it's a smell that your code layout is not optimal. It's possible it's unavoidable here, but I defer to you and xhwang@ to sort it out :)
On 2017/05/26 16:47:23, DaleCurtis wrote: > While you're here, did you see any functions which might benefit from a small > unittest? I.e. even things such as calling with a bunch of null values to ensure > a constructed object, etc. > > FWIW, almost any time you end up with a file or class that ends in helper or > util, it's a smell that your code layout is not optimal. It's possible it's > unavoidable here, but I defer to you and xhwang@ to sort it out :) Yeah, I also don't like a helper that contains "everything media". But I understand it's hard to draw the line perfectly. Another option is to only pull the most large/complex logical blocks into the helper, e.g. creation of MediaPlayer and EncryptedMediaClient. And leave everything else in RFI. That'll probably make it easier to understand what this class does, and/or to add unittests for it. We can even put the creation of MediaPlayer and EncryptedMediaClient in two separate helpers, e.g MediaPlayerFactory and EncryptedMediaClientFactory, to further separate things, since they are really not related to each other. But I am not sure how the new MediaCapabilities APIs will look like. So I am not sure whether it makes sense.
The CQ bit was checked by chcunningham@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
xhwang and I chatted offline. Latest PS removes CheckAudioSink... and GetMediaPermissions from the helper. This leaves only the EncryptedClient and MediaPlayer methods, so its now closer to a true factory. Renamed it MediaFactory. I don't see functions that would benefit from a "small" unit test. Unit tests would seem to need some mocking/faking and the value prop isn't great for the work required. I vote we call this yak shaved.
The CQ bit was checked by chcunningham@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! still lgtm with nits https://codereview.chromium.org/2905613003/diff/100001/content/renderer/media... File content/renderer/media/media_factory.h (right): https://codereview.chromium.org/2905613003/diff/100001/content/renderer/media... content/renderer/media/media_factory.h:25: using media::RequestRoutingTokenCallback; "aliases declared in a header file are part of that header's public API". Is this what we want? https://google.github.io/styleguide/cppguide.html#Aliases https://codereview.chromium.org/2905613003/diff/100001/content/renderer/media... content/renderer/media/media_factory.h:76: const RequestRoutingTokenCallback request_routing_token_cb); You can pass the callback by value, and use std::move. Or pass by const-ref. We pretty much never pass callbacks by const-value: https://cs.chromium.org/chromium/src/docs/callback.md?rcl=8ae2026e07808d63a17... https://codereview.chromium.org/2905613003/diff/100001/content/renderer/media... content/renderer/media/media_factory.h:79: void SetupMojo(); Add a comment, e.g. when this is expected to be called? https://codereview.chromium.org/2905613003/diff/100001/content/renderer/media... content/renderer/media/media_factory.h:88: blink::WebEncryptedMediaClient* EncryptedMediaClient(); These two are not overloads and we should have some comments. https://codereview.chromium.org/2905613003/diff/100001/content/renderer/media... content/renderer/media/media_factory.h:133: // The render frame we're helping. RenderFrameImpl creates and destroys this s/creates and destroys/owns? https://codereview.chromium.org/2905613003/diff/100001/content/renderer/media... content/renderer/media/media_factory.h:141: service_manager::InterfaceProvider* remote_interfaces_; Please document the lifetime of this raw pointer and the following raw pointers. It seems you dropped some of the old comments. https://codereview.chromium.org/2905613003/diff/100001/content/renderer/media... content/renderer/media/media_factory.h:141: service_manager::InterfaceProvider* remote_interfaces_; nit: service_manager::InterfaceProvider* remote_interfaces_ = nullptr; so we don't need to worry about initializing them in the init list. https://codereview.chromium.org/2905613003/diff/100001/content/renderer/media... content/renderer/media/media_factory.h:149: RendererMediaPlayerManager* media_player_manager_; I am surprised we are still keeping this :( https://codereview.chromium.org/2905613003/diff/100001/content/renderer/media... content/renderer/media/media_factory.h:184: } // namespace media s/media/content
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Thanks! https://codereview.chromium.org/2905613003/diff/100001/content/renderer/media... File content/renderer/media/media_factory.h (right): https://codereview.chromium.org/2905613003/diff/100001/content/renderer/media... content/renderer/media/media_factory.h:25: using media::RequestRoutingTokenCallback; On 2017/05/26 22:45:36, xhwang wrote: > "aliases declared in a header file are part of that header's public API". Is > this what we want? > > https://google.github.io/styleguide/cppguide.html#Aliases No. Just dropped it. https://codereview.chromium.org/2905613003/diff/100001/content/renderer/media... content/renderer/media/media_factory.h:76: const RequestRoutingTokenCallback request_routing_token_cb); On 2017/05/26 22:45:36, xhwang wrote: > You can pass the callback by value, and use std::move. Or pass by const-ref. We > pretty much never pass callbacks by const-value: > > https://cs.chromium.org/chromium/src/docs/callback.md?rcl=8ae2026e07808d63a17... Done. Using move. https://codereview.chromium.org/2905613003/diff/100001/content/renderer/media... content/renderer/media/media_factory.h:79: void SetupMojo(); On 2017/05/26 22:45:37, xhwang wrote: > Add a comment, e.g. when this is expected to be called? Done. https://codereview.chromium.org/2905613003/diff/100001/content/renderer/media... content/renderer/media/media_factory.h:88: blink::WebEncryptedMediaClient* EncryptedMediaClient(); On 2017/05/26 22:45:36, xhwang wrote: > These two are not overloads and we should have some comments. Done. https://codereview.chromium.org/2905613003/diff/100001/content/renderer/media... content/renderer/media/media_factory.h:133: // The render frame we're helping. RenderFrameImpl creates and destroys this On 2017/05/26 22:45:36, xhwang wrote: > s/creates and destroys/owns? Done. https://codereview.chromium.org/2905613003/diff/100001/content/renderer/media... content/renderer/media/media_factory.h:141: service_manager::InterfaceProvider* remote_interfaces_; On 2017/05/26 22:45:37, xhwang wrote: > Please document the lifetime of this raw pointer and the following raw pointers. > It seems you dropped some of the old comments. Done. https://codereview.chromium.org/2905613003/diff/100001/content/renderer/media... content/renderer/media/media_factory.h:141: service_manager::InterfaceProvider* remote_interfaces_; On 2017/05/26 22:45:36, xhwang wrote: > nit: > > service_manager::InterfaceProvider* remote_interfaces_ = nullptr; > > so we don't need to worry about initializing them in the init list. Done. https://codereview.chromium.org/2905613003/diff/100001/content/renderer/media... content/renderer/media/media_factory.h:149: RendererMediaPlayerManager* media_player_manager_; On 2017/05/26 22:45:36, xhwang wrote: > I am surprised we are still keeping this :( Its usage is shrinking! https://codereview.chromium.org/2905613003/diff/100001/content/renderer/media... content/renderer/media/media_factory.h:184: } // namespace media On 2017/05/26 22:45:36, xhwang wrote: > s/media/content Done.
The CQ bit was checked by chcunningham@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2905613003/diff/140001/content/renderer/media... File content/renderer/media/media_factory.cc (right): https://codereview.chromium.org/2905613003/diff/140001/content/renderer/media... content/renderer/media/media_factory.cc:88: media_player_delegate_(nullptr) { You don't need these any more. https://codereview.chromium.org/2905613003/diff/140001/content/renderer/media... File content/renderer/media/media_factory.h (right): https://codereview.chromium.org/2905613003/diff/140001/content/renderer/media... content/renderer/media/media_factory.h:99: // client who's lifetime is tied to this Factory (same as the RenderFrame). nit: whose https://codereview.chromium.org/2905613003/diff/140001/content/renderer/media... content/renderer/media/media_factory.h:155: service_manager::InterfaceProvider* remote_interfaces_; = nullptr?
https://codereview.chromium.org/2905613003/diff/140001/content/renderer/media... File content/renderer/media/media_factory.cc (right): https://codereview.chromium.org/2905613003/diff/140001/content/renderer/media... content/renderer/media/media_factory.cc:88: media_player_delegate_(nullptr) { On 2017/05/27 00:25:52, xhwang wrote: > You don't need these any more. Woops. Done https://codereview.chromium.org/2905613003/diff/140001/content/renderer/media... File content/renderer/media/media_factory.h (right): https://codereview.chromium.org/2905613003/diff/140001/content/renderer/media... content/renderer/media/media_factory.h:99: // client who's lifetime is tied to this Factory (same as the RenderFrame). On 2017/05/27 00:25:52, xhwang wrote: > nit: whose Done. https://codereview.chromium.org/2905613003/diff/140001/content/renderer/media... content/renderer/media/media_factory.h:155: service_manager::InterfaceProvider* remote_interfaces_; On 2017/05/27 00:25:52, xhwang wrote: > = nullptr? Done.
The CQ bit was checked by chcunningham@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Extract media code from RenderFrameImpl RenderFrameImpl has grown to contain nearly 500 lines of media plumbing. This CL extracts almost all of this into RenderMediaHelper and hides the details behind 4 public APIs. This is done in anticipation of adding a MediaCapabilities API to the RenderFrame. BUG=695264,588408 ========== to ========== Extract media code from RenderFrameImpl RenderFrameImpl has grown to contain nearly 500 lines of media plumbing. This CL extracts much of this into MediaFactory. This is done in anticipation of adding a MediaCapabilities API to the RenderFrame. BUG=695264,588408 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by chcunningham@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tguilbert@chromium.org, jochen@chromium.org Link to the patchset: https://codereview.chromium.org/2905613003/#ps160001 (title: "Feedback")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1496169667655220,
"parent_rev": "c0934409a5ad0453174431362d47fff9e6e68e13", "commit_rev":
"86f025e31d8d3676fea772a36eb9ecece61877ff"}
Message was sent while issue was closed.
Description was changed from ========== Extract media code from RenderFrameImpl RenderFrameImpl has grown to contain nearly 500 lines of media plumbing. This CL extracts much of this into MediaFactory. This is done in anticipation of adding a MediaCapabilities API to the RenderFrame. BUG=695264,588408 ========== to ========== Extract media code from RenderFrameImpl RenderFrameImpl has grown to contain nearly 500 lines of media plumbing. This CL extracts much of this into MediaFactory. This is done in anticipation of adding a MediaCapabilities API to the RenderFrame. BUG=695264,588408 Review-Url: https://codereview.chromium.org/2905613003 Cr-Commit-Position: refs/heads/master@{#475659} Committed: https://chromium.googlesource.com/chromium/src/+/86f025e31d8d3676fea772a36eb9... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/86f025e31d8d3676fea772a36eb9... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
