Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1126923002:
Add Media Router Mojo impl code. (Closed)
|
Created:
5 years, 7 months ago by Kevin M Modified:
5 years, 7 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, qsr+mojo_chromium.org, Aaron Boodman, posciak+watch_chromium.org, ben+mojo_chromium.org, viettrungluu+watch_chromium.org, mcasas+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, media-router+watch_chromium.org, darin (slow to review), wjia+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd Media Router Mojo impl code.
This code bridges calls from the Media Router to the Media Router
Provider Manager. CL comprises logic for marshaling data across
internal and Mojo structs, awareness of the MRPM extension's
suspend state, and a BrowserContextKeyedService for constructing and
managing Mojo service objects.
Also included are unit tests, supporting mock classes, and some trivial
miscellaneous name changes.
R=mfoltz@chromium.org
BUG=464205
Patch Set 1 #Patch Set 2 : Fixed licenses, added some more comments. #Patch Set 3 : Split off type converters library into its own CL. #Patch Set 4 : Remove type converters from CL. #Patch Set 5 : Fix the patch contents. #
Total comments: 52
Patch Set 6 : Sync with master. #Patch Set 7 : Addressed code review feedback. #Patch Set 8 : Removed duplicate entries from media_router_tests.gypi. #
Total comments: 28
Patch Set 9 : Rebase #Patch Set 10 : Addressed code review feedback. #
Total comments: 2
Patch Set 11 : Redundant forward decl removed. #
Total comments: 105
Messages
Total messages: 23 (6 generated)
tracylsnow1gir2boys@gmail.com changed reviewers: + tracylsnow1gir2boys@gmail.com
lgtm
mfoltz@chromium.org changed reviewers: - tracylsnow1gir2boys@gmail.com
I didn't review the tests or the test support classes yet. media_router_mojo_impl is pretty hairy and I feel like it could be split into a few nested or separate classes to more clearly separate/encapsulate responsibilities: - Managing wakeup of extension and queueing requests - Instantiating and binding mojo interface (maybe this could be handled in the factory)? - Handling requests from Media Router clients - Handling responses/messages from the component extension What is the relationship between media_router_impl and media_router_mojo_impl? https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/media_router.h (right): https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router.h:73: // Adds the IssuesObserver |observer|. Why are there two APIs for callers to be notified about Issues? IssueObserver and MediaRouter::Delegate::OnIssue Can we eliminate one? https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router.h:83: friend class MediaRouterImpl; The implementation should not be friended by the interface (how does this not create a circular dependency). Is there some API that needs to be protected: ? https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router.h:85: // The following APIs are called by the friend observer classes I think I would prefer listing the classes specifically. https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/media_router_mojo_impl.cc (right): https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_mojo_impl.cc:34: callback.Run(nullptr, (!error_text.get().empty() ? error_text.get() extra () https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_mojo_impl.cc:75: impl->Bind(request.Pass()); What does this do? https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_mojo_impl.cc:76: impl->MonitorExtension(extension_id, context); What if extension_id is already being monitored or there is already a binding to request? Can we make this idempotent? Why does this need to be public? https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_mojo_impl.cc:188: // Lazily create an observer list for the media source and add |observer| This doesn't look lazy - you're newing one up at L194 https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_mojo_impl.cc:198: DLOG(FATAL) << "Redundant RegisterMediaSinksObserver call detected."; Log instance id? https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_mojo_impl.cc:265: NOTIMPLEMENTED(); // TODO(kmarshall): To be landed in a separate CL. I think that's implied by NOTIMPLEMENTED() :) Or add a TODO at the top of the file to implement *all* of the NOTIMPLEMENTED methods. https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/media_router_mojo_impl.h (right): https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_mojo_impl.h:41: class MediaRouterMojoImpl : public MediaRouter, What is the relationship between this class and MediaRouterImpl? Are they two distinct implementations? https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_mojo_impl.h:42: public interfaces::MediaRouterObserver, It seems odd for a class to both implement an interface and observe an instance of it. https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_mojo_impl.h:52: // Used for querying suspension state. Wrap to previous line and use a four space continued indent. https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_mojo_impl.h:53: // |context|: The BrowserContext which owns the extension process. extra whitespace https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_mojo_impl.h:54: // |request|: The Mojo connection request used for binding. ditto https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_mojo_impl.h:55: static void BindToRequest( When would a caller invoke this method? Is this a one-time initialization of this object? https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_mojo_impl.h:64: // For incoming messages from the extension. It seems like ProvideMediaRouter is injecting an instance of the Mojo API, not responding to an extension message? https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_mojo_impl.h:66: interfaces::MediaRouterPtr mrpm, |mrpm| is too short- expand into a hacker_style parameter https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_mojo_impl.h:87: bool RegisterMediaSinksObserver(MediaSinksObserver* observer) override; Are the methods from here on below also resulting in messages to the extension? https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_mojo_impl.h:120: // Pending requests queued to be executed once MRPM becomes ready. Member variables need to go after method declarations http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O... https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_mojo_impl.h:149: interfaces::MediaRouterPtr mrpm_; Please expand into a proper variable name. https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_mojo_impl.h:151: // ID of the MRPM extension. Used for managing its suspend/wake state s/MRPM/component/ ? https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_mojo_impl.h:155: // Provides querying and waking functionality for an extension's suspend ...for the component extension's... https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_mojo_impl.h:160: // Used by the MRPM to determine if its persistent cache is stale. ... the component extension ... https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/media_router_mojo_impl_factory.cc (right): https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_mojo_impl_factory.cc:27: MediaRouterMojoImplFactory* MediaRouterMojoImplFactory::GetInstance() { Does this need to be a separate function, or just inline into GetApiForBrowserContext? https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/media_router_tests.gypi (right): https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_tests.gypi:12: 'issue_manager_unittest.cc', I don't see all of these files in this patch https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/media_router_type_converters.h (right): https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_type_converters.h:3: // found in the LICENSE file. I thought this already landed. Rebase?
https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/media_router.h (right): https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router.h:73: // Adds the IssuesObserver |observer|. On 2015/05/08 00:58:37, mark a. foltz wrote: > Why are there two APIs for callers to be notified about Issues? > > IssueObserver and MediaRouter::Delegate::OnIssue > > Can we eliminate one? Yes, OnIssue is redundant. Removed. https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router.h:83: friend class MediaRouterImpl; On 2015/05/08 00:58:37, mark a. foltz wrote: > The implementation should not be friended by the interface (how does this not > create a circular dependency). > > Is there some API that needs to be protected: ? Removed. The friend relationship existed to allow MediaRouterMojoImpl to allow MediaRouterImpl to pass calls to MediaRouterImpl::Register*Observer to the MediaRouterMojoImpl object. It's no longer necessary now that MediaRouterImpl is gone. https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router.h:85: // The following APIs are called by the friend observer classes On 2015/05/08 00:58:37, mark a. foltz wrote: > I think I would prefer listing the classes specifically. Done. https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/media_router_mojo_impl.cc (right): https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_mojo_impl.cc:34: callback.Run(nullptr, (!error_text.get().empty() ? error_text.get() On 2015/05/08 00:58:37, mark a. foltz wrote: > extra () Done. https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_mojo_impl.cc:75: impl->Bind(request.Pass()); On 2015/05/08 00:58:37, mark a. foltz wrote: > What does this do? Binds the object "impl" to a Mojo interface request. Messages sent from the extension via the Mojo MessagePipe will be unpacked and dispatched to |this| object. https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_mojo_impl.cc:76: impl->MonitorExtension(extension_id, context); On 2015/05/08 00:58:37, mark a. foltz wrote: > What if extension_id is already being monitored or there is already a binding to > request? Can we make this idempotent? > > Why does this need to be public? It is idempotent for a given extension ID and context. I removed a DCHECK that enforced single assignment for extension_id, which is incompatible for hot installations of a beta extension... This is public so that it can be called from chrome_mojo_service_registration. Would you prefer a friend relationship for a narrower scope? https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_mojo_impl.cc:188: // Lazily create an observer list for the media source and add |observer| On 2015/05/08 00:58:37, mark a. foltz wrote: > This doesn't look lazy - you're newing one up at L194 Done. https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_mojo_impl.cc:198: DLOG(FATAL) << "Redundant RegisterMediaSinksObserver call detected."; On 2015/05/08 00:58:38, mark a. foltz wrote: > Log instance id? Done. https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_mojo_impl.cc:265: NOTIMPLEMENTED(); // TODO(kmarshall): To be landed in a separate CL. On 2015/05/08 00:58:37, mark a. foltz wrote: > I think that's implied by NOTIMPLEMENTED() :) > > Or add a TODO at the top of the file to implement *all* of the NOTIMPLEMENTED > methods. Done. https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/media_router_mojo_impl.h (right): https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_mojo_impl.h:41: class MediaRouterMojoImpl : public MediaRouter, On 2015/05/08 00:58:38, mark a. foltz wrote: > What is the relationship between this class and MediaRouterImpl? Are they two > distinct implementations? Please revisit. https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_mojo_impl.h:42: public interfaces::MediaRouterObserver, On 2015/05/08 00:58:38, mark a. foltz wrote: > It seems odd for a class to both implement an interface and observe an instance > of it. This reflects a choice made during the upstreaming review for media_router.mojom. https://codereview.chromium.org/1055403006/ , see feedback item #8 for the rationale. We can revisit this if you think it's confusing. https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_mojo_impl.h:52: // Used for querying suspension state. On 2015/05/08 00:58:38, mark a. foltz wrote: > Wrap to previous line and use a four space continued indent. Done. https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_mojo_impl.h:53: // |context|: The BrowserContext which owns the extension process. On 2015/05/08 00:58:38, mark a. foltz wrote: > extra whitespace Done. https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_mojo_impl.h:54: // |request|: The Mojo connection request used for binding. On 2015/05/08 00:58:38, mark a. foltz wrote: > ditto Done. https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_mojo_impl.h:55: static void BindToRequest( On 2015/05/08 00:58:38, mark a. foltz wrote: > When would a caller invoke this method? Is this a one-time initialization of > this object? Correct - it's called by the Mojo module registry. Added documentation to that effect. https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_mojo_impl.h:64: // For incoming messages from the extension. On 2015/05/08 00:58:38, mark a. foltz wrote: > It seems like ProvideMediaRouter is injecting an instance of the Mojo API, not > responding to an extension message? It's registering a the extension's Mojo "MediaRouter" interface, which mirrors media_router::MediaRouter's commands. https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_mojo_impl.h:66: interfaces::MediaRouterPtr mrpm, On 2015/05/08 00:58:38, mark a. foltz wrote: > |mrpm| is too short- expand into a hacker_style parameter Done. https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_mojo_impl.h:87: bool RegisterMediaSinksObserver(MediaSinksObserver* observer) override; On 2015/05/08 00:58:38, mark a. foltz wrote: > Are the methods from here on below also resulting in messages to the extension? Yes, upon first registration and last unregistration. Added documentation. https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_mojo_impl.h:120: // Pending requests queued to be executed once MRPM becomes ready. On 2015/05/08 00:58:38, mark a. foltz wrote: > Member variables need to go after method declarations > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O... Done. https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_mojo_impl.h:149: interfaces::MediaRouterPtr mrpm_; On 2015/05/08 00:58:38, mark a. foltz wrote: > Please expand into a proper variable name. Done. https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_mojo_impl.h:151: // ID of the MRPM extension. Used for managing its suspend/wake state On 2015/05/08 00:58:38, mark a. foltz wrote: > s/MRPM/component/ ? Done. https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_mojo_impl.h:155: // Provides querying and waking functionality for an extension's suspend On 2015/05/08 00:58:38, mark a. foltz wrote: > ...for the component extension's... Done. https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_mojo_impl.h:160: // Used by the MRPM to determine if its persistent cache is stale. On 2015/05/08 00:58:38, mark a. foltz wrote: > ... the component extension ... Done. https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/media_router_mojo_impl_factory.cc (right): https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_mojo_impl_factory.cc:27: MediaRouterMojoImplFactory* MediaRouterMojoImplFactory::GetInstance() { On 2015/05/08 00:58:38, mark a. foltz wrote: > Does this need to be a separate function, or just inline into > GetApiForBrowserContext? Done. https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/media_router_tests.gypi (right): https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_tests.gypi:12: 'issue_manager_unittest.cc', On 2015/05/08 00:58:38, mark a. foltz wrote: > I don't see all of these files in this patch Duplicate entries from a mainline merge. Removed them. https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/media_router_type_converters.h (right): https://codereview.chromium.org/1126923002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_type_converters.h:3: // found in the LICENSE file. On 2015/05/08 00:58:39, mark a. foltz wrote: > I thought this already landed. Rebase? Done.
kmarshall@chromium.org changed reviewers: + imcheng@chromium.org
Adding imcheng to the reviewers list. Once he LGTM's the patch, he will take over the upstreaming review with xhwang et al.
imcheng@google.com changed reviewers: + imcheng@google.com
Please rebase, thanks. https://codereview.chromium.org/1126923002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/media_router.h (right): https://codereview.chromium.org/1126923002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_router.h:57: virtual void ClearIssue(const Issue::IssueId& issue_id) = 0; The name Issue::IssueId is inconsistent with the other ids. Is it possible to have simply IssueId (which is already in media_router namespace)? https://codereview.chromium.org/1126923002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_router.h:69: // Adds the IssuesObserver |observer|. These two functions should go into private. https://codereview.chromium.org/1126923002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/media_router_impl.h (right): https://codereview.chromium.org/1126923002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_router_impl.h:58: void ClearIssue(const std::string& issue_id) override; IssueId https://codereview.chromium.org/1126923002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/media_router_mojo_impl.cc (right): https://codereview.chromium.org/1126923002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:103: ExecutePendingRequests(); Consider inlining this since there's only 1 caller. https://codereview.chromium.org/1126923002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:202: return true; return false instead? https://codereview.chromium.org/1126923002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:208: if (!observer_list->might_have_observers()) { Because we don't have caching anymore, we should always be calling DoStartObservingMediaSinks whenever a sink is added. Otherwise sinks other than the first will not receive results. https://codereview.chromium.org/1126923002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:246: // TODO(kmarshall): add result caching and max limits. ditto on caching https://codereview.chromium.org/1126923002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:257: if (!routes_observers_.HasObserver(observer)) { curly brace not needed for a single line https://codereview.chromium.org/1126923002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:366: request_cb.Run(); I wonder if we can add a DCHECK(mojo_media_router_); here and remove it from all of the Do* functions. Also I don't think we need DCHECK(thread_checker_.CalledOnValidThread()); in the Do* functions as the callers already have that DCHECK. https://codereview.chromium.org/1126923002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:374: if (event_page_tracker_->IsEventPageSuspended(mrpm_extension_id_)) { In what conditions will that happen? Should it be DCHECK instead? https://codereview.chromium.org/1126923002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:380: for (const auto& next_request : pending_requests_) { curly braces not needed https://codereview.chromium.org/1126923002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/media_router_mojo_impl.h (right): https://codereview.chromium.org/1126923002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:46: MediaRouterMojoImpl(); Move this to private and document that one should use MediaRouterMojoImplFactory::GetApiForBrowserContext to obtain an instance. https://codereview.chromium.org/1126923002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:141: std::map<MediaSourceId, linked_ptr<ObserverList<MediaSinksObserver>>> How about ScopedPtrHashMap? https://codereview.chromium.org/1126923002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/test_helper.h (right): https://codereview.chromium.org/1126923002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/test_helper.h:65: class MockApiDelegate : public MediaRouterMojoImpl::Delegate { This doesn't exist yet.
https://codereview.chromium.org/1126923002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/media_router.h (right): https://codereview.chromium.org/1126923002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_router.h:57: virtual void ClearIssue(const Issue::IssueId& issue_id) = 0; On 2015/05/14 21:30:13, imcheng wrote: > The name Issue::IssueId is inconsistent with the other ids. Is it possible to > have simply IssueId (which is already in media_router namespace)? Making the type alias a child of Issue was suggested to us during an upstreaming code review. I agree that consistency is important. I'll send out a separate review that nests RouteId et al. under their respective classes. https://codereview.chromium.org/1126923002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_router.h:69: // Adds the IssuesObserver |observer|. On 2015/05/14 21:30:13, imcheng wrote: > These two functions should go into private. Done. https://codereview.chromium.org/1126923002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/media_router_impl.h (right): https://codereview.chromium.org/1126923002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_router_impl.h:58: void ClearIssue(const std::string& issue_id) override; On 2015/05/14 21:30:13, imcheng wrote: > IssueId N/A, media_router_impl is gone https://codereview.chromium.org/1126923002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/media_router_mojo_impl.cc (right): https://codereview.chromium.org/1126923002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:103: ExecutePendingRequests(); On 2015/05/14 21:30:13, imcheng wrote: > Consider inlining this since there's only 1 caller. I added an "inline" keyword. I think that organizing it under its own function adds clarity to the code structure. Do you still think I need to inline the code itself? https://codereview.chromium.org/1126923002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:202: return true; On 2015/05/14 21:30:13, imcheng wrote: > return false instead? Done https://codereview.chromium.org/1126923002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:208: if (!observer_list->might_have_observers()) { On 2015/05/14 21:30:13, imcheng wrote: > Because we don't have caching anymore, we should always be calling > DoStartObservingMediaSinks whenever a sink is added. Otherwise sinks other than > the first will not receive results. Done. https://codereview.chromium.org/1126923002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:246: // TODO(kmarshall): add result caching and max limits. On 2015/05/14 21:30:13, imcheng wrote: > ditto on caching Done. https://codereview.chromium.org/1126923002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:257: if (!routes_observers_.HasObserver(observer)) { On 2015/05/14 21:30:13, imcheng wrote: > curly brace not needed for a single line Done. https://codereview.chromium.org/1126923002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:366: request_cb.Run(); On 2015/05/14 21:30:13, imcheng wrote: > I wonder if we can add a DCHECK(mojo_media_router_); here and remove it from all > of the Do* functions. Also I don't think we need > DCHECK(thread_checker_.CalledOnValidThread()); in the Do* functions as the > callers already have that DCHECK. No need to have a DCHECK on mojo_media_router_ because the conditional already enforces that condition. Removed thread checker & mojo_media_router_ checks from Do* fns. https://codereview.chromium.org/1126923002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:374: if (event_page_tracker_->IsEventPageSuspended(mrpm_extension_id_)) { On 2015/05/14 21:30:13, imcheng wrote: > In what conditions will that happen? Should it be DCHECK instead? If the extension shuts down immediately after ProvideMediaRouter is called. Not likely to occur but it's worth having a check, anyways. It's not costly to verify, so I don't think we need to demote it to a debug-only check. https://codereview.chromium.org/1126923002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:380: for (const auto& next_request : pending_requests_) { On 2015/05/14 21:30:13, imcheng wrote: > curly braces not needed Done. https://codereview.chromium.org/1126923002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/media_router_mojo_impl.h (right): https://codereview.chromium.org/1126923002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:46: MediaRouterMojoImpl(); On 2015/05/14 21:30:13, imcheng wrote: > Move this to private and document that one should use > MediaRouterMojoImplFactory::GetApiForBrowserContext to obtain an instance. Done. https://codereview.chromium.org/1126923002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:141: std::map<MediaSourceId, linked_ptr<ObserverList<MediaSinksObserver>>> On 2015/05/14 21:30:13, imcheng wrote: > How about ScopedPtrHashMap? Didn't know about this, done. https://codereview.chromium.org/1126923002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/test_helper.h (right): https://codereview.chromium.org/1126923002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/test_helper.h:65: class MockApiDelegate : public MediaRouterMojoImpl::Delegate { On 2015/05/14 21:30:13, imcheng wrote: > This doesn't exist yet. Done.
lgtm https://codereview.chromium.org/1126923002/diff/180001/chrome/browser/media/r... File chrome/browser/media/router/media_router_mojo_impl.h (right): https://codereview.chromium.org/1126923002/diff/180001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:36: class IssuesObserver; these fwd declarations not needed?
marshallk@google.com changed reviewers: + marshallk@google.com
Thanks Derek. Mark, would you mind having another look before I move on with the upstreaming reviews? Thanks. https://codereview.chromium.org/1126923002/diff/180001/chrome/browser/media/r... File chrome/browser/media/router/media_router_mojo_impl.h (right): https://codereview.chromium.org/1126923002/diff/180001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:36: class IssuesObserver; On 2015/05/14 23:00:44, imcheng wrote: > these fwd declarations not needed? Done.
Thanks Derek. Mark, would you mind having another look before I move on with the upstreaming reviews? Thanks.
I won't have time to re-review before my trip. Suggest pinging wez@ or one of the media team reviewers to move this forward in the meantime.
kmarshall@chromium.org changed reviewers: + wez@chromium.org - mfoltz@chromium.org
R+=wez Derek will take over the CL for the media team review phase, but I want to make sure the code that I hand over is good quality first.
nit: CL description paragraph from "CL comprises" would be more readable as a bullet-list.
I've not reviewed the tests as yet, but here are some code comments. :) https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/media_router.h (right): https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router.h:109: nit: Unnecessary blank line? https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/media_router_mojo_impl.cc (right): https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:44: LOG(ERROR) << "An error encountered while waking the event page."; Shouldn't we do fun stuff like dispatch errors for any pending_requests_, and make sure no new requests are queued, in this case? Do we have a test-case for wake failure? :) https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:58: DCHECK(event_page_tracker_for_test); nit: Suggest a blank line between pre-condition DCHECKs and the rest of the function body, here and below - helps break up the code to be more readable at a glance. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:88: DCHECK(thread_checker_.CalledOnValidThread()); What happens to pendign requests at this point? https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:107: NOTIMPLEMENTED(); is this something that will be implemented, ever? If so then add a comment and bug #? If not then should this be NOTREACHED() - i.e. getting here is really bad and indicates an error somewhere else? https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:112: NOTIMPLEMENTED(); See above. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:154: // MediaRouter methods. nit: Remove this comment? It doesn't really add anything - I can see what this method is about by looking at the header. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:199: << "Redundant RegisterMediaSinksObserver call detected."; s/Redundant/Duplicate Does this indicate a coding error in Chrome? If so then consider NOTREACHED(), and get rid of the return-code from this function. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:206: observer_list->AddObserver(observer); nit: In principle you should add the observer _before_ you start observing media sinks, so that any notifications about them don't appear between you starting observing, and registering the observer. I realise this is single-threaded, so that's not an issue here, but I'd order things in what is _logically_ the right order here, in case the code changes! Do you still need to start observing if you are already observing for a particular source_id? If so then add a comment to clarify why. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:218: return; Does reaching here indicate a coding error? If so then NOTREACHED()! Or you could just not do these checks, and let RemoveObserver() trap the problem and crash us. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:223: // observing sinks for it. nit: Suggest clarifying here that might_have_observers() is reliable here (i.e. won't return true even though there are still observers) because we are definitely not iterating over it right now - I'm assuming it's guaranteed that we never UnregisterMediaSinksObserver from within an observer callback, though. ;) https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:235: DLOG(FATAL) << "Redundant RegisterMediaRoutesObserver() call detected."; See above re these never-reached error conditions. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:236: return true; Why return true here? https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:257: NOTIMPLEMENTED(); Is there abug # for adding these? https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:313: << ")"; Does this get used anywhere...? https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:322: event_page_tracker_ = extensions::ProcessManager::Get(context); This doesn't seem to be doing anything to actually request extension state-change notifications? https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:325: void MediaRouterMojoImpl::RunOrDefer(const base::Closure& request_cb) { This isn't the request callback, it's the request to run - suggest just call it |request| https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:333: base::Bind(&EventPageWakeComplete))) { Seems strange that we don't care about the wake compeltion notification; I'd expect that we at least trap it and handle failure, and potentially start a timer to ensure that we cope with the extension starting but then failing to connect via Mojo. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/media_router_mojo_impl.h (right): https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:28: } // namespace content nit: This namespace is sufficiently short that you can omit the comment. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:39: // Provider Manager and the Media Router. Would it be clearer to describe this as the Mojo service exposing MediaRouter functionality to the Media Router Provider Manager? https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:58: // mojo::ErrorHandler You mean "mojo::ErrorHandler interface."? https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:61: // interfaces::MediaRouter implementation. This class doesn't seem to inherit from interfaces::MediaRouter? https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:62: // Registers a Mojo MediaRouter proxy object from the extension. Why does this method deserve a comment, but none of the On*() ones do? Isn't the purpose of ProvideMediaRouter() defined in the interfaces::MediaRouter interface? https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:75: // For messages sent to the extension. ? https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:86: // and last unregistration. nit: Blank line above this comment, to make it more clearly a distinct block of methods to which the comment applies? https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:99: DeferredBindingAndSuspension); What's the DeferredBindingAndSuspension thing about? https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:102: // MediaRouterMojoImplFactory::GetApiForBrowserContext. Suggest "Standard constructor, used by ..." https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:105: // Used for testing. Suggest "Constructor used only for testing." Consider instead having a static method CreateForTest() - the ForTest() in the name will cause presubmit checks to fail if anyone mistakenly uses the method in production code. You could then safely leave that method in the public API section and avoid some unnecessary friending, potentially. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:119: content::BrowserContext* context); Clarify the role of |context| in this API; I assume it's a bare reference - what's it used for? https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:125: // Runs pending requests in |pending_requests_|. Suggest "Runs the closures queued in |pending_requests_|." https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:126: inline void ExecutePendingRequests(); Remove inline from here. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:145: // Multimap of sinks observer objects, keyed by media source ID. nit: This comment doesn't scan; you mean it's a Multi-map of MediaSink observers keyed my MediaSource Id. In any case, this comment describes what I can tell my reading the code - what it doesn't say is why we need this member. e.g. "Multimap of observers registered via StartObservingMediaSinks." https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:150: // List of route observer objects. Similarly, this comment doesn't tell me what this method is for - suggest either removing it or updating the comment to provide that info. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:154: // The binding is released when binding_ is deleted. Again, this comment doesn't describe _why_ this member exists - what is it for? If I had to guess it looks like it tells Mojo the service implementation to route requests for that interface to? But does it bind an individual client request, or cope with multiple clients, or what? https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:157: // Mojo proxy object for the Provider Manager. Is there a possibility that this may be "null" if the extension isn't currently running, for example? https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:158: interfaces::MediaRouterPtr mojo_media_router_; I'm confused... why is the interface to the Provider Manager called MediaRouter[Ptr]? Shouldn't that be the name for the MediaRouter interface that the Provider Manager uses...? https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:160: // ID of the component extension. Used for managing its suspend/wake state nit: ID->Id. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:165: // suspend state. Suggest: "Allows the extension to be monitor for suspend, and woken." Also suggest clarifying who owns the EventPageTracker, e.g. is this a cached pointer to a singleton? https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:170: // is stale. Probably needs elaborating upon; suggest something like: "GUID unique to each browser run. Component extension uses this to detect when its persisted state was written by an older browser instance, and is therefore stale." https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/media_router_mojo_impl_factory.cc (right): https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl_factory.cc:26: return Singleton<MediaRouterMojoImplFactory>::get(); I'm not familiar with this pattern; is this the preferred mechanism over base::LazyInstance? I notice that the Singleton header cautions against using it unless you absolutely have to... ;) https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl_factory.cc:33: DependsOn(extensions::ProcessManagerFactory::GetInstance()); nit: Consider documenting why we depend on this, since we don't seem to actually call into it anywhere here! e.g. does the Impl() depend on it? https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl_factory.cc:46: // This means MediaRouterImpl is available in incongnito contexts as well. typo: Incognito This comment doesn't make clear what "this" is - are you saying that returning the supplied |context| somehow makes it available to Incognito..? https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/media_router_mojo_impl_factory.h (right): https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl_factory.h:13: } // namespace content nit: No need for this since the namespace is teeny weeny https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl_factory.h:20: // BrowserContext, creating one lazily if needed. nit: Can you clean up this comment, plz? :) https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl_factory.h:25: static MediaRouterMojoImplFactory* GetInstance(); If there are no non-static methods on MediaRouterMojoImplFactory then why do we need there to be an instance of it? And if we need an instance somewhere internal, why do we need to expose it anywhere, since no-one can call it? https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl_factory.h:33: // BrowserContextKeyedBaseFactory ... interface. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/mock_media_router.cc (right): https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/mock_media_router.cc:10: } nit: } can appear immediately after { here https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/test_helper.cc (right): https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/test_helper.cc:14: } nit: } can appear immediately after { since this is a single-line declaration. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/test_helper.h (right): https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/test_helper.h:20: // Custom GMock matchers nit: This comment doesn't seem to add anything? If you want to keep it, it needs a terminating period! https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/test_helper.h:23: // .Equals() operator. nit: Neither this nor the comment on SequenceEquals seems to add any information that isn't obvious from the naming/context. Are these matchers intended to be used with a specific type of object, though, that has an Equals() method?
Comments addressed in https://codereview.chromium.org/1143603004/ https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/media_router.h (right): https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router.h:109: On 2015/05/20 23:37:47, Wez wrote: > nit: Unnecessary blank line? Done. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/media_router_mojo_impl.cc (right): https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:44: LOG(ERROR) << "An error encountered while waking the event page."; On 2015/05/20 23:37:48, Wez wrote: > Shouldn't we do fun stuff like dispatch errors for any pending_requests_, and > make sure no new requests are queued, in this case? > > Do we have a test-case for wake failure? :) Filed a tracking bug for handling failure. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:58: DCHECK(event_page_tracker_for_test); On 2015/05/20 23:37:48, Wez wrote: > nit: Suggest a blank line between pre-condition DCHECKs and the rest of the > function body, here and below - helps break up the code to be more readable at a > glance. Done. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:88: DCHECK(thread_checker_.CalledOnValidThread()); On 2015/05/20 23:37:48, Wez wrote: > What happens to pendign requests at this point? There's an edge case here where this can happen between Bind() and ProvideMediaRouter(), in that case we should probably flush the pending requests with failure. Added a tracking bug for it. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:107: NOTIMPLEMENTED(); On 2015/05/20 23:37:47, Wez wrote: > is this something that will be implemented, ever? If so then add a comment and > bug #? If not then should this be NOTREACHED() - i.e. getting here is really bad > and indicates an error somewhere else? Yes, this will be implemented eventually. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:112: NOTIMPLEMENTED(); On 2015/05/20 23:37:48, Wez wrote: > See above. Ditto. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:154: // MediaRouter methods. On 2015/05/20 23:37:48, Wez wrote: > nit: Remove this comment? It doesn't really add anything - I can see what this > method is about by looking at the header. Done. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:199: << "Redundant RegisterMediaSinksObserver call detected."; On 2015/05/20 23:37:48, Wez wrote: > s/Redundant/Duplicate > > Does this indicate a coding error in Chrome? If so then consider NOTREACHED(), > and get rid of the return-code from this function. How about a DCHECK? That is what we used to have in MediaRouterImpl before it is removed. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:206: observer_list->AddObserver(observer); On 2015/05/20 23:37:47, Wez wrote: > nit: In principle you should add the observer _before_ you start observing media > sinks, so that any notifications about them don't appear between you starting > observing, and registering the observer. I realise this is single-threaded, so > that's not an issue here, but I'd order things in what is _logically_ the right > order here, in case the code changes! > > Do you still need to start observing if you are already observing for a > particular source_id? If so then add a comment to clarify why. Good point. Swapped the order. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:218: return; On 2015/05/20 23:37:47, Wez wrote: > Does reaching here indicate a coding error? If so then NOTREACHED()! Or you > could just not do these checks, and let RemoveObserver() trap the problem and > crash us. Done. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:223: // observing sinks for it. On 2015/05/20 23:37:48, Wez wrote: > nit: Suggest clarifying here that might_have_observers() is reliable here (i.e. > won't return true even though there are still observers) because we are > definitely not iterating over it right now - I'm assuming it's guaranteed that > we never UnregisterMediaSinksObserver from within an observer callback, though. > ;) Done. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:235: DLOG(FATAL) << "Redundant RegisterMediaRoutesObserver() call detected."; On 2015/05/20 23:37:48, Wez wrote: > See above re these never-reached error conditions. Done. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:236: return true; On 2015/05/20 23:37:48, Wez wrote: > Why return true here? removed return value. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:257: NOTIMPLEMENTED(); On 2015/05/20 23:37:48, Wez wrote: > Is there abug # for adding these? Done. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:313: << ")"; On 2015/05/20 23:37:48, Wez wrote: > Does this get used anywhere...? Changed below to use this function. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:322: event_page_tracker_ = extensions::ProcessManager::Get(context); On 2015/05/20 23:37:48, Wez wrote: > This doesn't seem to be doing anything to actually request extension > state-change notifications? Yeah, we changed from a push model to a pull model at some point, so the name is no longer accurate. How about moving these into Bind() and renaming it BindAndInitExtensionInfo()? https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:325: void MediaRouterMojoImpl::RunOrDefer(const base::Closure& request_cb) { On 2015/05/20 23:37:48, Wez wrote: > This isn't the request callback, it's the request to run - suggest just call it > |request| Done. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:333: base::Bind(&EventPageWakeComplete))) { On 2015/05/20 23:37:48, Wez wrote: > Seems strange that we don't care about the wake compeltion notification; I'd > expect that we at least trap it and handle failure, and potentially start a > timer to ensure that we cope with the extension starting but then failing to > connect via Mojo Yes, I agree. But these adds complexity to this class, so I will file a TODO for now. We can work on it after the upstreaming is complete. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/media_router_mojo_impl.h (right): https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:28: } // namespace content On 2015/05/20 23:37:49, Wez wrote: > nit: This namespace is sufficiently short that you can omit the comment. Done. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:39: // Provider Manager and the Media Router. On 2015/05/20 23:37:49, Wez wrote: > Would it be clearer to describe this as the Mojo service exposing MediaRouter > functionality to the Media Router Provider Manager? Done. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:58: // mojo::ErrorHandler On 2015/05/20 23:37:49, Wez wrote: > You mean "mojo::ErrorHandler interface."? Changed to "mojo::ErrorHandler implementation." https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:61: // interfaces::MediaRouter implementation. On 2015/05/20 23:37:48, Wez wrote: > This class doesn't seem to inherit from interfaces::MediaRouter? Done. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:62: // Registers a Mojo MediaRouter proxy object from the extension. On 2015/05/20 23:37:49, Wez wrote: > Why does this method deserve a comment, but none of the On*() ones do? Isn't the > purpose of ProvideMediaRouter() defined in the interfaces::MediaRouter > interface? removed. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:75: // For messages sent to the extension. On 2015/05/20 23:37:48, Wez wrote: > ? Removed. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:86: // and last unregistration. On 2015/05/20 23:37:49, Wez wrote: > nit: Blank line above this comment, to make it more clearly a distinct block of > methods to which the comment applies? Removed since it is no longer true. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:99: DeferredBindingAndSuspension); On 2015/05/20 23:37:49, Wez wrote: > What's the DeferredBindingAndSuspension thing about? It's a test that's disabled until "issues with extension system teardown are addressed." I am not too familiar with the details, but it looks like a test that tries to call MediaRouterMojoImpl from the browser before it's hooked up to the component extension. Filed a bug to reenable it. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:102: // MediaRouterMojoImplFactory::GetApiForBrowserContext. On 2015/05/20 23:37:49, Wez wrote: > Suggest "Standard constructor, used by ..." Done. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:105: // Used for testing. On 2015/05/20 23:37:48, Wez wrote: > Suggest "Constructor used only for testing." > > Consider instead having a static method CreateForTest() - the ForTest() in the > name will cause presubmit checks to fail if anyone mistakenly uses the method in > production code. You could then safely leave that method in the public API > section and avoid some unnecessary friending, potentially. Changed comment. The MediaRouterMojoTest setup code need to access to private methods, so I will leave it for now. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:119: content::BrowserContext* context); On 2015/05/20 23:37:48, Wez wrote: > Clarify the role of |context| in this API; I assume it's a bare reference - > what's it used for? Done. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:125: // Runs pending requests in |pending_requests_|. On 2015/05/20 23:37:49, Wez wrote: > Suggest "Runs the closures queued in |pending_requests_|." Done. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:126: inline void ExecutePendingRequests(); On 2015/05/20 23:37:48, Wez wrote: > Remove inline from here. Done. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:145: // Multimap of sinks observer objects, keyed by media source ID. On 2015/05/20 23:37:49, Wez wrote: > nit: This comment doesn't scan; you mean it's a Multi-map of MediaSink observers > keyed my MediaSource Id. > > In any case, this comment describes what I can tell my reading the code - what > it doesn't say is why we need this member. e.g. "Multimap of observers > registered via StartObservingMediaSinks." Removed comment. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:150: // List of route observer objects. On 2015/05/20 23:37:49, Wez wrote: > Similarly, this comment doesn't tell me what this method is for - suggest either > removing it or updating the comment to provide that info. Done. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:154: // The binding is released when binding_ is deleted. On 2015/05/20 23:37:49, Wez wrote: > Again, this comment doesn't describe _why_ this member exists - what is it for? > > If I had to guess it looks like it tells Mojo the service implementation to > route requests for that interface to? But does it bind an individual client > request, or cope with multiple clients, or what? Done. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:157: // Mojo proxy object for the Provider Manager. On 2015/05/20 23:37:49, Wez wrote: > Is there a possibility that this may be "null" if the extension isn't currently > running, for example? Yes. Clarified in comments. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:158: interfaces::MediaRouterPtr mojo_media_router_; On 2015/05/20 23:37:49, Wez wrote: > I'm confused... why is the interface to the Provider Manager called > MediaRouter[Ptr]? Shouldn't that be the name for the MediaRouter interface that > the Provider Manager uses...? There was a discussion with Kevin and xhwang@ on the naming. The idea, I think, is that the provider manager is a mojo implementation of the MediaRouter. This class is an implementation that delegates the calls to it. Please see: https://codereview.chromium.org/1055403006/diff/40001/chrome/browser/media/ro... https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:160: // ID of the component extension. Used for managing its suspend/wake state On 2015/05/20 23:37:49, Wez wrote: > nit: ID->Id. Done. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:165: // suspend state. On 2015/05/20 23:37:49, Wez wrote: > Suggest: "Allows the extension to be monitor for suspend, and woken." > > Also suggest clarifying who owns the EventPageTracker, e.g. is this a cached > pointer to a singleton? Done. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:170: // is stale. On 2015/05/20 23:37:49, Wez wrote: > Probably needs elaborating upon; suggest something like: > "GUID unique to each browser run. Component extension uses this to detect when > its persisted state was written by an older browser instance, and is therefore > stale." Done. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/media_router_mojo_impl_factory.cc (right): https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl_factory.cc:26: return Singleton<MediaRouterMojoImplFactory>::get(); On 2015/05/20 23:37:49, Wez wrote: > I'm not familiar with this pattern; is this the preferred mechanism over > base::LazyInstance? I notice that the Singleton header cautions against using it > unless you absolutely have to... ;) Done. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl_factory.cc:33: DependsOn(extensions::ProcessManagerFactory::GetInstance()); On 2015/05/20 23:37:49, Wez wrote: > nit: Consider documenting why we depend on this, since we don't seem to actually > call into it anywhere here! e.g. does the Impl() depend on it? Done. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl_factory.cc:46: // This means MediaRouterImpl is available in incongnito contexts as well. On 2015/05/20 23:37:50, Wez wrote: > typo: Incognito > > This comment doesn't make clear what "this" is - are you saying that returning > the supplied |context| somehow makes it available to Incognito..? I tried to clarified the comment. It just means we don't redirect incognito context to the original context. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/media_router_mojo_impl_factory.h (right): https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl_factory.h:13: } // namespace content On 2015/05/20 23:37:50, Wez wrote: > nit: No need for this since the namespace is teeny weeny Done. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl_factory.h:20: // BrowserContext, creating one lazily if needed. On 2015/05/20 23:37:50, Wez wrote: > nit: Can you clean up this comment, plz? :) Done. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl_factory.h:25: static MediaRouterMojoImplFactory* GetInstance(); On 2015/05/20 23:37:50, Wez wrote: > If there are no non-static methods on MediaRouterMojoImplFactory then why do we > need there to be an instance of it? And if we need an instance somewhere > internal, why do we need to expose it anywhere, since no-one can call it? Removed this function and inlined it in GetApiForBrowserContext. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl_factory.h:33: // BrowserContextKeyedBaseFactory On 2015/05/20 23:37:50, Wez wrote: > ... interface. Done. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/mock_media_router.cc (right): https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/mock_media_router.cc:10: } On 2015/05/20 23:37:50, Wez wrote: > nit: } can appear immediately after { here Done. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/test_helper.cc (right): https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/test_helper.cc:14: } On 2015/05/20 23:37:50, Wez wrote: > nit: } can appear immediately after { since this is a single-line declaration. Actually, this is due to `git cl format`. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/test_helper.h (right): https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/test_helper.h:20: // Custom GMock matchers On 2015/05/20 23:37:50, Wez wrote: > nit: This comment doesn't seem to add anything? If you want to keep it, it needs > a terminating period! Removed. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/test_helper.h:23: // .Equals() operator. On 2015/05/20 23:37:50, Wez wrote: > nit: Neither this nor the comment on SequenceEquals seems to add any information > that isn't obvious from the naming/context. > > Are these matchers intended to be used with a specific type of object, though, > that has an Equals() method? Ok, I replaced the comment with description of objects these matchers are intended for.
Message was sent while issue was closed.
https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/media_router_mojo_impl.cc (right): https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:322: event_page_tracker_ = extensions::ProcessManager::Get(context); On 2015/05/21 19:28:50, imcheng wrote: > On 2015/05/20 23:37:48, Wez wrote: > > This doesn't seem to be doing anything to actually request extension > > state-change notifications? > > Yeah, we changed from a push model to a pull model at some point, so the name is > no longer accurate. How about moving these into Bind() and renaming it > BindAndInitExtensionInfo()? Bind could certainly use a re-name to something more helpful - I'm drawing a blank on how to describe it better, though. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:333: base::Bind(&EventPageWakeComplete))) { On 2015/05/21 19:28:50, imcheng wrote: > On 2015/05/20 23:37:48, Wez wrote: > > Seems strange that we don't care about the wake compeltion notification; I'd > > expect that we at least trap it and handle failure, and potentially start a > > timer to ensure that we cope with the extension starting but then failing to > > connect via Mojo > > Yes, I agree. But these adds complexity to this class, so I will file a TODO for > now. We can work on it after the upstreaming is complete. Acknowledged. https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/media_router_mojo_impl.h (right): https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:158: interfaces::MediaRouterPtr mojo_media_router_; On 2015/05/21 19:28:51, imcheng wrote: > On 2015/05/20 23:37:49, Wez wrote: > > I'm confused... why is the interface to the Provider Manager called > > MediaRouter[Ptr]? Shouldn't that be the name for the MediaRouter interface > that > > the Provider Manager uses...? > > There was a discussion with Kevin and xhwang@ on the naming. The idea, I think, > is that the provider manager is a mojo implementation of the MediaRouter. This > class is an implementation that delegates the calls to it. > > Please see: > https://codereview.chromium.org/1055403006/diff/40001/chrome/browser/media/ro... That doesn't sound right. The MediaRouter is the component in Chrome, and the MediaRouteProviderManager is the extension-side component that the MediaRouteProviders integrate with. Or is the ideal that the MediaRouteProviders all talk via Mojo, through a MediaRouter interface, to the MediaRouteProviderManager, which then talks to the C++ MediaRouter?
Message was sent while issue was closed.
https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/media_router_mojo_impl.cc (right): https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:322: event_page_tracker_ = extensions::ProcessManager::Get(context); On 2015/05/21 23:32:47, Wez wrote: > On 2015/05/21 19:28:50, imcheng wrote: > > On 2015/05/20 23:37:48, Wez wrote: > > > This doesn't seem to be doing anything to actually request extension > > > state-change notifications? > > > > Yeah, we changed from a push model to a pull model at some point, so the name > is > > no longer accurate. How about moving these into Bind() and renaming it > > BindAndInitExtensionInfo()? > > Bind could certainly use a re-name to something more helpful - I'm drawing a > blank on how to describe it better, though. I ended up keeping the Bind() and extension logic separate since MediaRouterMojoTest setup code only requires the former. How about calling it BindToMojoRequest? https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/media_router_mojo_impl.h (right): https://codereview.chromium.org/1126923002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:158: interfaces::MediaRouterPtr mojo_media_router_; On 2015/05/21 23:32:47, Wez wrote: > On 2015/05/21 19:28:51, imcheng wrote: > > On 2015/05/20 23:37:49, Wez wrote: > > > I'm confused... why is the interface to the Provider Manager called > > > MediaRouter[Ptr]? Shouldn't that be the name for the MediaRouter interface > > that > > > the Provider Manager uses...? > > > > There was a discussion with Kevin and xhwang@ on the naming. The idea, I > think, > > is that the provider manager is a mojo implementation of the MediaRouter. This > > class is an implementation that delegates the calls to it. > > > > Please see: > > > https://codereview.chromium.org/1055403006/diff/40001/chrome/browser/media/ro... > > That doesn't sound right. The MediaRouter is the component in Chrome, and the > MediaRouteProviderManager is the extension-side component that the > MediaRouteProviders integrate with. > > Or is the ideal that the MediaRouteProviders all talk via Mojo, through a > MediaRouter interface, to the MediaRouteProviderManager, which then talks to the > C++ MediaRouter? Hmm. I think it makes sense to me. From Chrome's perspective, it does not need to know about provider manager (this means we should update the comments here to omit 'Provider Manager'). All it knows as that there is a component extension that acts as a MediaRouter because it implements provides an implementation of the MediaRouter interface. We are using MediaRouter to describe the set of APIs for routing media, and not just the component in Chrome. That might be the source of confusion? |
