Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
795193003:
Introduce MojoRendererFactory::ServiceProvider. (Closed)
|
Created:
6 years ago by xhwang Modified:
6 years ago Reviewers:
jamesr CC:
chromium-reviews, feature-media-reviews_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce MojoRendererFactory::ServiceProvider.
Currently MojoRendererFactory takes a ServiceProviderPtr. This works in
HTMLViewer. However, in RenderFrameImpl, we need to use
GetServiceRegistry()->ConnectToRemoteService() to get the MediaRenderer service.
This CL makes it possible to use MojoRendererFactory in both places.
BUG=431776
Committed: https://crrev.com/bd57385e3c623d0db22159dc686a392f24440d24
Cr-Commit-Position: refs/heads/master@{#308537}
Patch Set 1 #
Total comments: 2
Patch Set 2 : #Patch Set 3 : Callback -> Interface per discussion. #Patch Set 4 : git cl format #
Total comments: 14
Patch Set 5 : comments addressed #
Messages
Total messages: 18 (2 generated)
xhwang@chromium.org changed reviewers: + jamesr@chromium.org
PTAL
https://codereview.chromium.org/795193003/diff/1/mojo/services/html_viewer/we... File mojo/services/html_viewer/webmediaplayer_factory.cc (right): https://codereview.chromium.org/795193003/diff/1/mojo/services/html_viewer/we... mojo/services/html_viewer/webmediaplayer_factory.cc:49: err, I'll add #if !defined(OS_ANDROID) here...
https://codereview.chromium.org/795193003/diff/1/mojo/services/html_viewer/we... File mojo/services/html_viewer/webmediaplayer_factory.cc (right): https://codereview.chromium.org/795193003/diff/1/mojo/services/html_viewer/we... mojo/services/html_viewer/webmediaplayer_factory.cc:49: On 2014/12/13 01:06:31, xhwang wrote: > err, I'll add #if !defined(OS_ANDROID) here... Done.
Why is this a callback instead of an interface? What is the expected lifetime of the callback?
On 2014/12/15 21:14:08, jamesr wrote: > Why is this a callback instead of an interface? What is the expected lifetime of > the callback? The callback has the same lifetime as MojoRendererFactory, which has the same lifetime as WebMediaPlayerImpl (the media element). I choose to use a callback because it's simpler. Here's another case where I need this callback: https://codereview.chromium.org/712463004/diff/20001/content/renderer/render_...
Since it's a factory interface I would expect it to be able to create multiple instances, but since the callback takes a pass-only type I believe it can only run once. Is that correct? I'm not really sure how the callback makes things simpler. It's pretty confusing to reason about the bound state of the callback, imo.
On 2014/12/15 21:40:06, jamesr wrote: > Since it's a factory interface I would expect it to be able to create multiple > instances, but since the callback takes a pass-only type I believe it can only > run once. Is that correct? > > I'm not really sure how the callback makes things simpler. It's pretty > confusing to reason about the bound state of the callback, imo. The callback definition implies that it can be called multiple times: typedef base::Callback<void(mojo::InterfacePtr<mojo::MediaRenderer>* ptr)> ConnectToServiceCB; Then it's the responsibility of the code that binds something into this callback to make sure the callback can be called multiple times. In WebMediaPlayerFactory's case, the binded callback owns the ServiceProviderPtr. Since MojoRendererFactory owns the callback, it owns the ServiceProviderPtr indirectly. This is how we can actually call the callback multiple times. The callback is just a small glue code to make sure MojoRendererFactory works for both ServiceProviderPtr here, and ServiceRegistry in content code: https://codereview.chromium.org/712463004/diff/20001/content/renderer/render_... If we use an interface instead of a callback for this glue code, we'll end up with one interface (with only one method) and two different implementations for that interface. I felt that's too much for what I am trying to do here. That said, I don't have strong opinions on this. If you still feel an interface is the way to go. I can definitely do that.
On 2014/12/15 22:00:22, xhwang wrote: > On 2014/12/15 21:40:06, jamesr wrote: > > Since it's a factory interface I would expect it to be able to create multiple > > instances, but since the callback takes a pass-only type I believe it can only > > run once. Is that correct? > > > > I'm not really sure how the callback makes things simpler. It's pretty > > confusing to reason about the bound state of the callback, imo. > > The callback definition implies that it can be called multiple times: > typedef base::Callback<void(mojo::InterfacePtr<mojo::MediaRenderer>* ptr)> > ConnectToServiceCB; > > Then it's the responsibility of the code that binds something into this callback > to make sure the callback can be called multiple times. In > WebMediaPlayerFactory's case, the binded callback owns the ServiceProviderPtr. > Since MojoRendererFactory owns the callback, it owns the ServiceProviderPtr > indirectly. This is how we can actually call the callback multiple times. I don't think that's what will actually happen. The ServiceProviderPtr is passed to the callback, so the first time the callback is invoked the parameter will have ownership. At the end of the invocation the parameter will be destroyed. The callback can't be run again since the ServiceProviderPtr is not valid. If you wanted to run multiple times you would need to somehow transfer ownership of the ServiceProviderPtr from the parameter 'back' to the callback, but that's impossible. (You should have some tests for this, btw) > > The callback is just a small glue code to make sure MojoRendererFactory works > for both ServiceProviderPtr here, and ServiceRegistry in content code: > https://codereview.chromium.org/712463004/diff/20001/content/renderer/render_... > If we use an interface instead of a callback for this glue code, we'll end up > with one interface (with only one method) and two different implementations for > that interface. I felt that's too much for what I am trying to do here. > > That said, I don't have strong opinions on this. If you still feel an interface > is the way to go. I can definitely do that. I think if this was an interface the ownership of things like ServiceProviderPtr would be a lot clearer.
On 2014/12/15 22:22:46, jamesr wrote: > On 2014/12/15 22:00:22, xhwang wrote: > > On 2014/12/15 21:40:06, jamesr wrote: > > > Since it's a factory interface I would expect it to be able to create > multiple > > > instances, but since the callback takes a pass-only type I believe it can > only > > > run once. Is that correct? > > > > > > I'm not really sure how the callback makes things simpler. It's pretty > > > confusing to reason about the bound state of the callback, imo. > > > > The callback definition implies that it can be called multiple times: > > typedef base::Callback<void(mojo::InterfacePtr<mojo::MediaRenderer>* ptr)> > > ConnectToServiceCB; > > > > Then it's the responsibility of the code that binds something into this > callback > > to make sure the callback can be called multiple times. In > > WebMediaPlayerFactory's case, the binded callback owns the ServiceProviderPtr. > > Since MojoRendererFactory owns the callback, it owns the ServiceProviderPtr > > indirectly. This is how we can actually call the callback multiple times. > > I don't think that's what will actually happen. The ServiceProviderPtr is > passed to the callback, so the first time the callback is invoked the parameter > will have ownership. At the end of the invocation the parameter will be > destroyed. The callback can't be run again since the ServiceProviderPtr is not > valid. If you wanted to run multiple times you would need to somehow transfer > ownership of the ServiceProviderPtr from the parameter 'back' to the callback, > but that's impossible. (You should have some tests for this, btw) > > > > > The callback is just a small glue code to make sure MojoRendererFactory works > > for both ServiceProviderPtr here, and ServiceRegistry in content code: > > > https://codereview.chromium.org/712463004/diff/20001/content/renderer/render_... > > If we use an interface instead of a callback for this glue code, we'll end up > > with one interface (with only one method) and two different implementations > for > > that interface. I felt that's too much for what I am trying to do here. > > > > That said, I don't have strong opinions on this. If you still feel an > interface > > is the way to go. I can definitely do that. > > I think if this was an interface the ownership of things like ServiceProviderPtr > would be a lot clearer. You are right. I didn't pay attention to this comment: https://code.google.com/p/chromium/codesearch#chromium/src/base/bind_helpers.... I'll work on the interface solution. Thanks for the insights!
PTAL again, thanks!
lgtm https://codereview.chromium.org/795193003/diff/60001/media/mojo/services/mojo... File media/mojo/services/mojo_renderer_factory.h (right): https://codereview.chromium.org/795193003/diff/60001/media/mojo/services/mojo... media/mojo/services/mojo_renderer_factory.h:18: class ServiceProvider { comment? https://codereview.chromium.org/795193003/diff/60001/media/mojo/services/mojo... media/mojo/services/mojo_renderer_factory.h:20: ServiceProvider(){}; no ;, spacing seems odd. did you run 'git cl format'? https://codereview.chromium.org/795193003/diff/60001/media/mojo/services/mojo... media/mojo/services/mojo_renderer_factory.h:29: MojoRendererFactory(scoped_ptr<ServiceProvider> service_provider); explicit https://codereview.chromium.org/795193003/diff/60001/mojo/services/html_viewe... File mojo/services/html_viewer/webmediaplayer_factory.cc (right): https://codereview.chromium.org/795193003/diff/60001/mojo/services/html_viewe... mojo/services/html_viewer/webmediaplayer_factory.cc:34: RendererServiceProvider(ServiceProviderPtr service_provider_ptr); explicit https://codereview.chromium.org/795193003/diff/60001/mojo/services/html_viewe... mojo/services/html_viewer/webmediaplayer_factory.cc:37: mojo::InterfacePtr<mojo::MediaRenderer>* media_renderer_ptr) final; this code is in namespace mojo, you can drop all these 'mojo::'s https://codereview.chromium.org/795193003/diff/60001/mojo/services/html_viewe... mojo/services/html_viewer/webmediaplayer_factory.cc:45: RendererServiceProvider::RendererServiceProvider( i would just inline these https://codereview.chromium.org/795193003/diff/60001/mojo/services/html_viewe... mojo/services/html_viewer/webmediaplayer_factory.cc:99: scoped_ptr<media::MojoRendererFactory::ServiceProvider>( you don't need to spell this type out again. just say "make_scoped_ptr(new ....)" (this works now because rvalue to scoped_ptr<Derived> can be converted to rvalues of scoped_ptr<Base>)
comments addressed
https://codereview.chromium.org/795193003/diff/60001/media/mojo/services/mojo... File media/mojo/services/mojo_renderer_factory.h (right): https://codereview.chromium.org/795193003/diff/60001/media/mojo/services/mojo... media/mojo/services/mojo_renderer_factory.h:18: class ServiceProvider { On 2014/12/16 01:47:37, jamesr wrote: > comment? Done. https://codereview.chromium.org/795193003/diff/60001/media/mojo/services/mojo... media/mojo/services/mojo_renderer_factory.h:20: ServiceProvider(){}; On 2014/12/16 01:47:37, jamesr wrote: > no ;, spacing seems odd. did you run 'git cl format'? Yep, done by git cl format. Fixed manually. https://codereview.chromium.org/795193003/diff/60001/media/mojo/services/mojo... media/mojo/services/mojo_renderer_factory.h:29: MojoRendererFactory(scoped_ptr<ServiceProvider> service_provider); On 2014/12/16 01:47:37, jamesr wrote: > explicit Done. https://codereview.chromium.org/795193003/diff/60001/mojo/services/html_viewe... File mojo/services/html_viewer/webmediaplayer_factory.cc (right): https://codereview.chromium.org/795193003/diff/60001/mojo/services/html_viewe... mojo/services/html_viewer/webmediaplayer_factory.cc:34: RendererServiceProvider(ServiceProviderPtr service_provider_ptr); On 2014/12/16 01:47:37, jamesr wrote: > explicit Done. https://codereview.chromium.org/795193003/diff/60001/mojo/services/html_viewe... mojo/services/html_viewer/webmediaplayer_factory.cc:37: mojo::InterfacePtr<mojo::MediaRenderer>* media_renderer_ptr) final; On 2014/12/16 01:47:37, jamesr wrote: > this code is in namespace mojo, you can drop all these 'mojo::'s Done. https://codereview.chromium.org/795193003/diff/60001/mojo/services/html_viewe... mojo/services/html_viewer/webmediaplayer_factory.cc:45: RendererServiceProvider::RendererServiceProvider( On 2014/12/16 01:47:37, jamesr wrote: > i would just inline these Done. https://codereview.chromium.org/795193003/diff/60001/mojo/services/html_viewe... mojo/services/html_viewer/webmediaplayer_factory.cc:99: scoped_ptr<media::MojoRendererFactory::ServiceProvider>( On 2014/12/16 01:47:37, jamesr wrote: > you don't need to spell this type out again. just say "make_scoped_ptr(new > ....)" > > (this works now because rvalue to scoped_ptr<Derived> can be converted to > rvalues of scoped_ptr<Base>) Good to know. Thanks!
The CQ bit was checked by xhwang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/795193003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/bd57385e3c623d0db22159dc686a392f24440d24 Cr-Commit-Position: refs/heads/master@{#308537} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
