Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1132903002:
[MediaRouter] Add implementation of PresentationServiceDelegate (Closed)
|
Created:
5 years, 7 months ago by haibinlu Modified:
5 years, 6 months ago CC:
chromium-reviews, posciak+watch_chromium.org, media-router+watch_chromium.org, wjia+watch_chromium.org, mcasas+watch_chromium.org, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds presentation_service_delegate_impl, which implements PresentationServiceDelegate to interface with the
Chrome Media Router. An instance of this class is associated with a WebContents. In addition to fulfilling Presentation requests, it also provide tab-level information that is required for creating browser-initiated sessions.
Renames the media source helper methods.
BUG=464226
Committed: https://crrev.com/571a4dfac35342be8d07cff886fb48349f546b36
Cr-Commit-Position: refs/heads/master@{#332320}
Patch Set 1 #
Total comments: 42
Patch Set 2 : #Patch Set 3 : #
Total comments: 63
Patch Set 4 : #
Total comments: 65
Patch Set 5 : Addresses Derek's comments #
Total comments: 18
Patch Set 6 : #
Total comments: 8
Patch Set 7 : #Patch Set 8 : #
Total comments: 128
Patch Set 9 : Addresses Wez's comments #
Total comments: 22
Patch Set 10 : Addresses Mark's comments #Patch Set 11 : #Messages
Total messages: 33 (5 generated)
haibinlu@chromium.org changed reviewers: + imcheng@chromium.org, mfoltz@chromium.org
To start, please write a proper change description and link a related BUG= with the MedaiRouter label :)
On 2015/05/08 21:41:08, mark a. foltz wrote: > To start, please write a proper change description and link a related BUG= with > the MedaiRouter label :) Done
Thanks, I had a hard time wrapping my head around the patch since there are several things going on in PresentationServiceDelegate. At a high level: (1) Why are there three different observers collaborating with the class? (2) It looks like we should factor out a helper class for dealing with the DPU/screen availability for a specific frame and that could simplify the bookkeeping quite a bit. (3) Does the render_process_id ever change for a WebContents? Maybe we could make it a member and clean up the APIs (4) IF there are helper functions that could be moved into the .cc and out of the class definition that would make its purpose clearer IMHO I will review the .cc + tests more closely once the API is organized a bit more https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... chrome/browser/media/router/presentation_service_delegate_impl.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. Copyright https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... chrome/browser/media/router/presentation_service_delegate_impl.cc:15: #include "chrome/browser/media/router/media_router_impl.h" Why does this class need to know about the MR implementation? https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... File chrome/browser/media/router/presentation_service_delegate_impl.h (right): https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... chrome/browser/media/router/presentation_service_delegate_impl.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. Update copyright https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... chrome/browser/media/router/presentation_service_delegate_impl.h:36: // Implementation of PresentationServiceDelegate that interfaces with the Suggest: ...that interfaces an instance of WebContents with... and omit the next sentence https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... chrome/browser/media/router/presentation_service_delegate_impl.h:42: : public content::WebContentsUserData<PresentationServiceDelegateImpl>, This doesn't follow the pattern in WebContentsUserData: public: // virtual ~FooTabHelper(); // // ... more public stuff here ... // private: // explicit FooTabHelper(content::WebContents* contents); // friend class content::WebContentsUserData<FooTabHelper>; // // ... more private stuff here ... // } with a private ctor and friend declaration for WCUD. https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... chrome/browser/media/router/presentation_service_delegate_impl.h:85: // Notifies this object that |route| has been created or joined outside of a Why does the delegate for a specific webcontents need to be notified about route creation elsewhere? https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... chrome/browser/media/router/presentation_service_delegate_impl.h:95: // Returns the host name (without the "www.") of the frame that contains What is this for? Why do we assume the frame has a host name? https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... chrome/browser/media/router/presentation_service_delegate_impl.h:99: const content::WebContents* web_contents() const { return web_contents_; } Do we need to expose the WebContents in the public API? https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... chrome/browser/media/router/presentation_service_delegate_impl.h:102: // tab. s/tab/WebContents/ https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... chrome/browser/media/router/presentation_service_delegate_impl.h:107: // Called when default media source for the corresponding tab has changed. s/tab/WebContents/ https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... chrome/browser/media/router/presentation_service_delegate_impl.h:111: virtual void OnDefaultMediaSourceChanged( It would be a cleaner API to have a separate observer method like OnDefaultMediaSourceRemoved(). https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... chrome/browser/media/router/presentation_service_delegate_impl.h:111: virtual void OnDefaultMediaSourceChanged( Do we need to pass a ptr to |this| or WebContents*? How does the caller know which WebContents has changed? https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... chrome/browser/media/router/presentation_service_delegate_impl.h:117: void AddDefaultMediaSourceObserver(DefaultMediaSourceObserver* observer); How are these observers different from the AddObserver/RemoveObserver above? Can we combine the APIs into a single observer interface (unless there are truly distinct use cases)? https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... chrome/browser/media/router/presentation_service_delegate_impl.h:138: // TODO(imcheng): Move this to PresentationServiceDelegate and use it in the Can we implement this TODO now? https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... chrome/browser/media/router/presentation_service_delegate_impl.h:140: typedef std::pair<int, int> RenderFrameHostId; Class members need to be declared after methods and functions. http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O... https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... chrome/browser/media/router/presentation_service_delegate_impl.h:146: static RenderFrameHostId GetRenderFrameHostId(content::RenderFrameHost* rfh); I usually prefer to declare helper functions for the .cc in an anonymous namespace there, instead of private static members of the class. This keeps the .h smaller and the class declaration free of helper function clutter. https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... chrome/browser/media/router/presentation_service_delegate_impl.h:158: std::string GetSourceHostForFrame(RenderFrameHostId rfh_id) const; Does this need to be an instance method or can it be a helper function in the .cc? https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... chrome/browser/media/router/presentation_service_delegate_impl.h:165: std::string GetOrGeneratePresentationId( Ditto https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... chrome/browser/media/router/presentation_service_delegate_impl.h:170: bool IsMainFrame(RenderFrameHostId rfh_id) const; Ditto https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... chrome/browser/media/router/presentation_service_delegate_impl.h:192: content::WebContents* web_contents_; This is unowned correct? https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... chrome/browser/media/router/presentation_service_delegate_impl.h:195: base::hash_map<RenderFrameHostId, Observer*> delegate_observers_; I'm confused here, why isn't this an ObserverList?
PTAL. I will update the unit test if the new design is ok. (1) Why are there three different observers collaborating with the class? It does not seem easy to combine the three observers. Two observers in the direction of MR --> web app: 1. PresentationServiceDelegate::Observer is one per frame, used for pushing sessions started by browser to PS impl. 2. PresentationScreenAvailabilityListener is one per (frame, presentation URL). One observer in the direction of web app --> MR: DefaultMediaSourceObserver is one per WebContents. (2) It looks like we should factor out a helper class for dealing with the DPU/screen availability for a specific frame and that could simplify the bookkeeping quite a bit. Done (3) Does the render_process_id ever change for a WebContents? Maybe we could make it a member and clean up the APIs WebContents may have multiple render processes under site-per-process model? (4) IF there are helper functions that could be moved into the .cc and out of the class definition that would make its purpose clearer Done. https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... chrome/browser/media/router/presentation_service_delegate_impl.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/05/11 07:36:21, mark a. foltz wrote: > Copyright Done. https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... chrome/browser/media/router/presentation_service_delegate_impl.cc:15: #include "chrome/browser/media/router/media_router_impl.h" On 2015/05/11 07:36:21, mark a. foltz wrote: > Why does this class need to know about the MR implementation? removed. https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... File chrome/browser/media/router/presentation_service_delegate_impl.h (right): https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... chrome/browser/media/router/presentation_service_delegate_impl.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/05/11 07:36:21, mark a. foltz wrote: > Update copyright Done. https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... chrome/browser/media/router/presentation_service_delegate_impl.h:36: // Implementation of PresentationServiceDelegate that interfaces with the On 2015/05/11 07:36:21, mark a. foltz wrote: > Suggest: > > ...that interfaces an instance of WebContents with... > > and omit the next sentence Done. https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... chrome/browser/media/router/presentation_service_delegate_impl.h:42: : public content::WebContentsUserData<PresentationServiceDelegateImpl>, On 2015/05/11 07:36:22, mark a. foltz wrote: > This doesn't follow the pattern in WebContentsUserData: > > public: > // virtual ~FooTabHelper(); > // // ... more public stuff here ... > // private: > // explicit FooTabHelper(content::WebContents* contents); > // friend class content::WebContentsUserData<FooTabHelper>; > // // ... more private stuff here ... > // } > > with a private ctor and friend declaration for WCUD. Done. https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... chrome/browser/media/router/presentation_service_delegate_impl.h:85: // Notifies this object that |route| has been created or joined outside of a On 2015/05/11 07:36:21, mark a. foltz wrote: > Why does the delegate for a specific webcontents need to be notified about route > creation elsewhere? This is for browser initiated sessions. https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... chrome/browser/media/router/presentation_service_delegate_impl.h:95: // Returns the host name (without the "www.") of the frame that contains On 2015/05/11 07:36:21, mark a. foltz wrote: > What is this for? Why do we assume the frame has a host name? it is used as domain name as in current extension. Discussed with Derek, he thinks the domain name should be from a frame that contains default presentation url. Our current exetnsion behavior is different: we always use the domain of the tab, e.g., play movie may uses an iframe to load their script from, say, gstatic.com, the iframe has the presentation URL, but we do not want to show "cast gstatic.com" because it is confusing to users. I sent out an email about this. https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... chrome/browser/media/router/presentation_service_delegate_impl.h:99: const content::WebContents* web_contents() const { return web_contents_; } On 2015/05/11 07:36:21, mark a. foltz wrote: > Do we need to expose the WebContents in the public API? removed https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... chrome/browser/media/router/presentation_service_delegate_impl.h:102: // tab. On 2015/05/11 07:36:22, mark a. foltz wrote: > s/tab/WebContents/ Done. https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... chrome/browser/media/router/presentation_service_delegate_impl.h:107: // Called when default media source for the corresponding tab has changed. On 2015/05/11 07:36:22, mark a. foltz wrote: > s/tab/WebContents/ Done. https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... chrome/browser/media/router/presentation_service_delegate_impl.h:111: virtual void OnDefaultMediaSourceChanged( On 2015/05/11 07:36:22, mark a. foltz wrote: > Do we need to pass a ptr to |this| or WebContents*? How does the caller know > which WebContents has changed? the call is from the webcontents https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... chrome/browser/media/router/presentation_service_delegate_impl.h:117: void AddDefaultMediaSourceObserver(DefaultMediaSourceObserver* observer); On 2015/05/11 07:36:21, mark a. foltz wrote: > How are these observers different from the AddObserver/RemoveObserver above? > > Can we combine the APIs into a single observer interface (unless there are truly > distinct use cases)? the observer above is one per WebContents, for WebContents to get sessions started by browser. This is for MR to register listener, so that it gets informed about default presentation url change. https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... chrome/browser/media/router/presentation_service_delegate_impl.h:138: // TODO(imcheng): Move this to PresentationServiceDelegate and use it in the On 2015/05/11 07:36:22, mark a. foltz wrote: > Can we implement this TODO now? Done. https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... chrome/browser/media/router/presentation_service_delegate_impl.h:140: typedef std::pair<int, int> RenderFrameHostId; On 2015/05/11 07:36:22, mark a. foltz wrote: > Class members need to be declared after methods and functions. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O... Acknowledged. https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... chrome/browser/media/router/presentation_service_delegate_impl.h:146: static RenderFrameHostId GetRenderFrameHostId(content::RenderFrameHost* rfh); On 2015/05/11 07:36:22, mark a. foltz wrote: > I usually prefer to declare helper functions for the .cc in an anonymous > namespace there, instead of private static members of the class. This keeps > the .h smaller and the class declaration free of helper function clutter. Done. https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... chrome/browser/media/router/presentation_service_delegate_impl.h:158: std::string GetSourceHostForFrame(RenderFrameHostId rfh_id) const; On 2015/05/11 07:36:22, mark a. foltz wrote: > Does this need to be an instance method or can it be a helper function in the > .cc? No. move to anonymous namespace. https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... chrome/browser/media/router/presentation_service_delegate_impl.h:165: std::string GetOrGeneratePresentationId( On 2015/05/11 07:36:21, mark a. foltz wrote: > Ditto need to access default_presentation_infos_ https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... chrome/browser/media/router/presentation_service_delegate_impl.h:170: bool IsMainFrame(RenderFrameHostId rfh_id) const; On 2015/05/11 07:36:22, mark a. foltz wrote: > Ditto Need to access web_contents_ https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... chrome/browser/media/router/presentation_service_delegate_impl.h:192: content::WebContents* web_contents_; On 2015/05/11 07:36:21, mark a. foltz wrote: > This is unowned correct? correct. https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... chrome/browser/media/router/presentation_service_delegate_impl.h:195: base::hash_map<RenderFrameHostId, Observer*> delegate_observers_; On 2015/05/11 07:36:21, mark a. foltz wrote: > I'm confused here, why isn't this an ObserverList? Need to index it by rfh_id. linear list works too.
Overall looks good, most comments are minor. Thanks for breaking out helper classes, it's a lot more understandable now. Since I will be out you may want Derek to pick up the rest of the review before passing on to a reviewer outside the team. Nit: Please reformat patch description to 80 cols. https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/media_router_impl_factory.h (right): https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/media_router_impl_factory.h:10: #include "chrome/browser/media/router/media_router_impl.h" This file is no longer necessary with Kevin's changes. https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/media_source_helper.cc (right): https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/media_source_helper.cc:53: DCHECK(IsValidPresentationUrl(source.id())); What behavior is intended in release builds? I would write this as: return IsValidPresentationUrl(source.id()) ? source.id() : ""; https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_frame.cc (right): https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_frame.cc:33: scoped_ptr<PresentationMediaSinksObserver> observer( make_scoped_ptr https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_frame.cc:68: delegate_observer_ = observer; This is a one-time operation correct? DCHECK(observer) DCHECK(!delegate_observer_) https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_frame.cc:81: ? ForTabMediaSource(SessionTabHelper::IdForTab(web_contents_)) We might want to define a media source specifically for a generic offscreen tab. In lieu of that can we use the generic tab source urn:x-org.chromium:media:source:tab-* ? https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_frame.h (right): https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_frame.h:23: class PresentationFrame { If this is used only by PSDI then I would declare it in that .h/.cc to keep the file count down. It looks like it could benefit from a separate unit test; otherwise you could just declare it in PSDI.cc https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_frame.h:28: ~PresentationFrame(); virtual https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_frame.h:30: // Return true if listener was added. s/Return/Returns/ here and below https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_frame.h:37: void SetDefaultPresentationInfo(const std::string& default_presentation_url, Add docstring https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_frame.h:42: void SetDelegateObserver(DelegateObserver* observer); Add docstrings https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_frame.h:51: // Not owned by this class. What guarantees the lifetime of this? https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_frame.h:60: content::WebContents* web_contents_; Ditto for this https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_frame_map.cc (right): https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_frame_map.cc:28: if (num_sink_observers_ >= kMaxNumSources) { I would rename this to num_screen_availability_listeners_. https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_frame_map.cc:33: auto frame = GetOrAddFrame(rfh_id); presentation_frame here and below (so as not to confuse with RenderFrameHost, etc.) https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_frame_map.cc:91: if (frame) { Omit { } on single-line if's https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_frame_map.h (right): https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_frame_map.h:22: // Helper class for PresentationServiceDelegateImpl to manage Similar comments to PresentationFrame: - If it is an implementation detail that does not need a unit test define+declare in PSDI.cc - Otherwise merge with PSDI.h/.cc https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_frame_map.h:31: ~PresentationFrameMap(); virtual https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_frame_map.h:33: bool AddScreenAvailabilityListener( Add docstrings to public method declarations https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_frame_map.h:57: base::ScopedPtrHashMap<RenderFrameHostId, scoped_ptr<PresentationFrame>> Add a comment explaining this map, i.e. "Maps a frame identifier to a PresentationFrame object for frames that are listening to screen availability." https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_frame_map.h:60: size_t num_sink_observers_; What is a sink observer in this context? Is it the same as a |listener| from the API above? Please add a comment explaining. https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_frame_map.h:62: // Does not own these two objects. Please comment about why this is okay. https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_helper.h (right): https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_helper.h:13: bool IsValidPresentationUrl(const std::string& url); Can we fold this into media_source_helper.h? https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:38: RenderFrameHostId GetRenderFrameHostId(RenderFrameHost* rfh) { It seems like this should be a ctor for RenderFrameHostId https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:48: std::string host = rfh->GetLastCommittedURL().host(); Is this guaranteed to be a valid URL? What about chrome://, chrome-extension:// or about: frames? Please have some sort of fallback as I'm not convinced there is always a valid host. https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:81: frame_map_.SetDelegateObserver(render_process_id, render_frame_id, NULL); Please add a method RemoveDelegateObserver() instead of passing NULL. https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:89: Extra newline https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:108: UpdateDefaultMediaSourceAndNotifyObservers(MediaSource(), std::string()); I would prefer an explicit method, RemoveDefaultMediaSource... https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:126: new_default_source = ForPresentationUrl(default_presentation_url); What about setting |default_presentation_id| on |new_default_source|? https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:127: std::string new_default_source_host(GetSourceHostForFrame(rfh_id)); Ah, this is not derived from new_default_source, but from the committed URL for the frame that set it. Can this be named something like new_default_presentation_frame_host (and the corresponding other variables updated)? https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:172: NOTIMPLEMENTED(); It seems like this can be implemented now: call into router_->RequestRoute() and bind a callback to OnRouteCreated(). https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:189: NOTIMPLEMENTED(); Can you elaborate on what is required here? What object will serve as the MediaRouter::Delegate? https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:207: } We need to instantiate a MediaRouter::Delegate for the newly returned route but we don't have the API to register it yet in MediaRouter. Can you check with imcheng@ to find out the status, and add a TODO/NOTE outlining the path forward? https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_service_delegate_impl.h (right): https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.h:148: std::string GetOrGeneratePresentationId( This is an odd method - it seems like it would be cleaner to have GetDefaultPresentationId(rfh_id) and GeneratePresentationId() and have the calling code implement the correct logic, according to whether we were starting presentation from a default URL/id or a URL/id passed with StartSession(). https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.h:153: bool IsMainFrame(RenderFrameHostId rfh_id) const; This is to determine whether the frame's DPU has been chosen for browser intitiated presentation? Can we call this IsFrameForDefaultPresentation()? https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.h:163: PresentationFrameMap frame_map_; Add docstrings for class members. https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.h:166: std::string default_source_host_; Can this be derived from default_source_? https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_service_delegate_impl_browsertest.cc (right): https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl_browsertest.cc:33: using content::WebContents; Declare all usings above https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl_browsertest.cc:36: class PresentationServiceDelegateImplBrowserTest : public InProcessBrowserTest { Why does this need to be a browser test? It seems like all the collaborators can be mocked out. https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl_browsertest.cc:57: // instead of MediaRouter in browser tests. Why? https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl_browsertest.cc:76: IN_PROC_BROWSER_TEST_F(PresentationServiceDelegateImplBrowserTest, Given this isn't even enabled can we remove this file from the patch? https://codereview.chromium.org/1132903002/diff/40001/content/public/browser/... File content/public/browser/presentation_service_delegate.h (right): https://codereview.chromium.org/1132903002/diff/40001/content/public/browser/... content/public/browser/presentation_service_delegate.h:50: typedef std::pair<int, int> RenderFrameHostId; using
https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/media_router_impl_factory.h (right): https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/media_router_impl_factory.h:10: #include "chrome/browser/media/router/media_router_impl.h" On 2015/05/14 22:20:44, mfoltz_ooo_until_5-26 wrote: > This file is no longer necessary with Kevin's changes. Done. https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/media_source_helper.cc (right): https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/media_source_helper.cc:53: DCHECK(IsValidPresentationUrl(source.id())); On 2015/05/14 22:20:44, mfoltz_ooo_until_5-26 wrote: > What behavior is intended in release builds? > > I would write this as: > > > return IsValidPresentationUrl(source.id()) ? source.id() : ""; Done. https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_frame.cc (right): https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_frame.cc:33: scoped_ptr<PresentationMediaSinksObserver> observer( On 2015/05/14 22:20:44, mfoltz_ooo_until_5-26 wrote: > make_scoped_ptr Done. https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_frame.h (right): https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_frame.h:23: class PresentationFrame { On 2015/05/14 22:20:45, mfoltz_ooo_until_5-26 wrote: > If this is used only by PSDI then I would declare it in that .h/.cc to keep the > file count down. > > It looks like it could benefit from a separate unit test; otherwise you could > just declare it in PSDI.cc Merged into PSDI.cc https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_frame.h:28: ~PresentationFrame(); On 2015/05/14 22:20:45, mark a. foltz wrote: > virtual Done. https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_frame.h:30: // Return true if listener was added. On 2015/05/14 22:20:45, mark a. foltz wrote: > s/Return/Returns/ here and below Done. https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_frame.h:37: void SetDefaultPresentationInfo(const std::string& default_presentation_url, On 2015/05/14 22:20:45, mfoltz_ooo_until_5-26 wrote: > Add docstring Done. https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_frame.h:42: void SetDelegateObserver(DelegateObserver* observer); On 2015/05/14 22:20:45, mfoltz_ooo_until_5-26 wrote: > Add docstrings Done. https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_frame_map.cc (right): https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_frame_map.cc:28: if (num_sink_observers_ >= kMaxNumSources) { On 2015/05/14 22:20:45, mark a. foltz wrote: > I would rename this to num_screen_availability_listeners_. Done. https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_frame_map.cc:33: auto frame = GetOrAddFrame(rfh_id); On 2015/05/14 22:20:45, mark a. foltz wrote: > presentation_frame here and below (so as not to confuse with RenderFrameHost, > etc.) Done. https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_frame_map.h (right): https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_frame_map.h:22: // Helper class for PresentationServiceDelegateImpl to manage On 2015/05/14 22:20:46, mfoltz_ooo_until_5-26 wrote: > Similar comments to PresentationFrame: > - If it is an implementation detail that does not need a unit test > define+declare in PSDI.cc > - Otherwise merge with PSDI.h/.cc Merged to PSDI.cc https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_frame_map.h:31: ~PresentationFrameMap(); On 2015/05/14 22:20:45, mark a. foltz wrote: > virtual Done. https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_frame_map.h:33: bool AddScreenAvailabilityListener( On 2015/05/14 22:20:45, mfoltz_ooo_until_5-26 wrote: > Add docstrings to public method declarations Done. https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_frame_map.h:57: base::ScopedPtrHashMap<RenderFrameHostId, scoped_ptr<PresentationFrame>> On 2015/05/14 22:20:45, mark a. foltz wrote: > Add a comment explaining this map, i.e. "Maps a frame identifier to a > PresentationFrame object for frames that are listening to screen availability." Done. https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_frame_map.h:60: size_t num_sink_observers_; On 2015/05/14 22:20:45, mark a. foltz wrote: > What is a sink observer in this context? Is it the same as a |listener| from > the API above? > > Please add a comment explaining. Removed. Calculated # of screen availability listener on-demand, the complexity is O(# of presentation frames). https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:38: RenderFrameHostId GetRenderFrameHostId(RenderFrameHost* rfh) { On 2015/05/14 22:20:46, mark a. foltz wrote: > It seems like this should be a ctor for RenderFrameHostId It is a typedef. typedef std::pair<int, int> RenderFrameHostId; https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:89: On 2015/05/14 22:20:46, mfoltz_ooo_until_5-26 wrote: > Extra newline Done. https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:172: NOTIMPLEMENTED(); On 2015/05/14 22:20:46, mfoltz_ooo_until_5-26 wrote: > It seems like this can be implemented now: call into router_->RequestRoute() and > bind a callback to OnRouteCreated(). router_ can not be not properly set till the next CL from Kevin. https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:189: NOTIMPLEMENTED(); On 2015/05/14 22:20:46, mfoltz_ooo_until_5-26 wrote: > Can you elaborate on what is required here? What object will serve as the > MediaRouter::Delegate? Same here. https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_service_delegate_impl.h (right): https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.h:148: std::string GetOrGeneratePresentationId( On 2015/05/14 22:20:46, mfoltz_ooo_until_5-26 wrote: > This is an odd method - it seems like it would be cleaner to have > > GetDefaultPresentationId(rfh_id) > > and > > GeneratePresentationId() > > and have the calling code implement the correct logic, according to whether we > were starting presentation from a default URL/id or a URL/id passed with > StartSession(). removed the method for now. will split when adding requestRoute https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.h:153: bool IsMainFrame(RenderFrameHostId rfh_id) const; On 2015/05/14 22:20:46, mfoltz_ooo_until_5-26 wrote: > This is to determine whether the frame's DPU has been chosen for browser > intitiated presentation? > Can we call this IsFrameForDefaultPresentation()? This is for checking the main frame. https://codereview.chromium.org/1132903002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.h:163: PresentationFrameMap frame_map_; On 2015/05/14 22:20:46, mfoltz_ooo_until_5-26 wrote: > Add docstrings for class members. Done.
imcheng@google.com changed reviewers: + imcheng@google.com
Did a first pass. Please rebase as there have been some gyp/gn changes. https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/BUILD.gn (right): https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/BUILD.gn:75: "presentation_service_delegate_impl_unittest.cc", You will need to rebase because the unit_tests target no longer exists. https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/media_source_helper.cc (right): https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/media_source_helper.cc:30: MediaSource ForCastAppMediaSource(const std::string& app_id) { This function has already been removed. https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:21: #include "chrome/browser/ui/webui/media_router/media_router_dialog_controller.h" This isn't used yet. Please remove it and other unused includes. https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:32: namespace { You should be able to nest this anonymous namespace into namespace media_router. https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:34: using RenderFrameHostId = typedef not needed if moved into media_router namespace. https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:66: virtual ~PresentationFrame(); Doesn't need to be virtual. https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:85: delegate_observer_ = observer; How about DCHECK(!delegate_observer_) before setting? https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:96: DelegateObserver* delegate_observer_; Move this to after web_contents_ https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:103: MediaRouter* router_; 1. Can these be declared in the same order as ctor arguments? 2. Make the pointers const. https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:109: : web_contents_(web_contents), router_(router), delegate_observer_(NULL) { s/NULL/nullptr https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:110: DCHECK(router_); DCHECK(web_contents_) ? https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:123: if (sink_observers_.contains(source.id())) A single add() will work: https://code.google.com/p/chromium/codesearch#chromium/src/base/containers/sc... https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:189: virtual ~PresentationFrameMap(); Does not need to be virtual. https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:196: // Returns true if listener was deleted. Newline before comments, also s/deleted/removed https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:201: // Returns true if there is a listener for |source_id| in the frame. Newline before comments, same for below https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:219: void Reset(int render_process_id, int render_frame_id); RenderFrameHostId rfh_id https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:229: presentation_frames_; It looks like we may have a potential memory leak here. How do we ensure PresentationFrame entries are removed from the map when they are no longer needed? https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:238: : web_contents_(web_contents), router_(router) { DCHECKs? https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:322: if (presentation_frame) { braces not needed for a single line. https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:329: auto presentation_frame = presentation_frames_.get(rfh_id); redundant get. https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:330: if (!presentation_frames_.contains(rfh_id)) { For this use case it looks like a single add() is sufficient -- add() does nothing if the key already exists. https://code.google.com/p/chromium/codesearch#chromium/src/base/containers/sc... https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:346: router_(NULL), s/NULL/nullptr, same for below https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:365: frame_map_->SetDelegateObserver(render_process_id, render_frame_id, NULL); nullptr https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:475: RenderFrameHost* main_frame = web_contents_->GetMainFrame(); Thinking about this some more, I think this is not quite correct. We need to iterate through all frames and call observer->OnDefaultPresentationStarted for each frame that has a matching default presentation URL. https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:514: bool PresentationServiceDelegateImpl::HasScreenAvailabilityListenerFor( is this used? https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_service_delegate_impl.h (right): https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.h:46: using RenderFrameHostId = Can this be moved to private? https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.h:104: MediaSource GetDefaultMediaSource() const; default_source() ? https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.h:129: static const size_t kMaxNumSources = 256; static const variables go at the top of public: block https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.h:150: // not have a persentation URL, default to tab mirroring. how about: returns the tab mirroring MediaSource. https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.h:154: // Returns |true| is frame |rfh_id| is the main frame of |web_contents_|. Returns |true| if ... https://codereview.chromium.org/1132903002/diff/60001/content/public/browser/... File content/public/browser/presentation_service_delegate.h (right): https://codereview.chromium.org/1132903002/diff/60001/content/public/browser/... content/public/browser/presentation_service_delegate.h:50: typedef std::pair<int, int> RenderFrameHostId; I think this can be left out for now. Because you'd also need to change the APIs below to take a RFHId.
PTAL https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/BUILD.gn (right): https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/BUILD.gn:75: "presentation_service_delegate_impl_unittest.cc", On 2015/05/18 20:54:06, imcheng wrote: > You will need to rebase because the unit_tests target no longer exists. Done. https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:21: #include "chrome/browser/ui/webui/media_router/media_router_dialog_controller.h" On 2015/05/18 20:54:07, imcheng wrote: > This isn't used yet. Please remove it and other unused includes. Done. https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:32: namespace { On 2015/05/18 20:54:07, imcheng wrote: > You should be able to nest this anonymous namespace into namespace media_router. Done. https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:34: using RenderFrameHostId = On 2015/05/18 20:54:06, imcheng wrote: > typedef not needed if moved into media_router namespace. Done. https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:66: virtual ~PresentationFrame(); On 2015/05/18 20:54:07, imcheng wrote: > Doesn't need to be virtual. Done. https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:85: delegate_observer_ = observer; On 2015/05/18 20:54:07, imcheng wrote: > How about DCHECK(!delegate_observer_) before setting? Done. https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:96: DelegateObserver* delegate_observer_; On 2015/05/18 20:54:07, imcheng wrote: > Move this to after web_contents_ Done. https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:103: MediaRouter* router_; On 2015/05/18 20:54:07, imcheng wrote: > 1. Can these be declared in the same order as ctor arguments? > 2. Make the pointers const. Done. https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:109: : web_contents_(web_contents), router_(router), delegate_observer_(NULL) { On 2015/05/18 20:54:07, imcheng wrote: > s/NULL/nullptr Done. https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:110: DCHECK(router_); On 2015/05/18 20:54:07, imcheng wrote: > DCHECK(web_contents_) ? Done. https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:123: if (sink_observers_.contains(source.id())) On 2015/05/18 20:54:06, imcheng wrote: > A single add() will work: > https://code.google.com/p/chromium/codesearch#chromium/src/base/containers/sc... can not remove it. because "new PresentationMediaSinksObserver" will auto add an observer to router_. https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:189: virtual ~PresentationFrameMap(); On 2015/05/18 20:54:07, imcheng wrote: > Does not need to be virtual. Done. https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:196: // Returns true if listener was deleted. On 2015/05/18 20:54:07, imcheng wrote: > Newline before comments, also s/deleted/removed Done. https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:201: // Returns true if there is a listener for |source_id| in the frame. On 2015/05/18 20:54:07, imcheng wrote: > Newline before comments, same for below Done. https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:219: void Reset(int render_process_id, int render_frame_id); On 2015/05/18 20:54:08, imcheng wrote: > RenderFrameHostId rfh_id Done. https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:229: presentation_frames_; On 2015/05/18 20:54:07, imcheng wrote: > It looks like we may have a potential memory leak here. How do we ensure > PresentationFrame entries are removed from the map when they are no longer > needed? As discussed, use removeObserver to remove the entry. will rename removeObserver to UnregisterPresentationFrame in a different CL. https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:238: : web_contents_(web_contents), router_(router) { On 2015/05/18 20:54:07, imcheng wrote: > DCHECKs? Done. https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:322: if (presentation_frame) { On 2015/05/18 20:54:06, imcheng wrote: > braces not needed for a single line. Done. https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:329: auto presentation_frame = presentation_frames_.get(rfh_id); On 2015/05/18 20:54:06, imcheng wrote: > redundant get. Done. https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:330: if (!presentation_frames_.contains(rfh_id)) { On 2015/05/18 20:54:07, imcheng wrote: > For this use case it looks like a single add() is sufficient -- add() does > nothing if the key already exists. > > https://code.google.com/p/chromium/codesearch#chromium/src/base/containers/sc... But it'd create a new PresentationFrameObject and release if the element already exists. https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:346: router_(NULL), On 2015/05/18 20:54:07, imcheng wrote: > s/NULL/nullptr, same for below Done. https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:365: frame_map_->SetDelegateObserver(render_process_id, render_frame_id, NULL); On 2015/05/18 20:54:06, imcheng wrote: > nullptr Done. https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:475: RenderFrameHost* main_frame = web_contents_->GetMainFrame(); On 2015/05/18 20:54:07, imcheng wrote: > Thinking about this some more, I think this is not quite correct. We need to > iterate through all frames and call observer->OnDefaultPresentationStarted for > each frame that has a matching default presentation URL. I am not sure this is the right behavior either. A client can sent arbitrary presentation URL; what presents it from getting access to the session if it purposely sets the same URL as the main frame? https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:514: bool PresentationServiceDelegateImpl::HasScreenAvailabilityListenerFor( On 2015/05/18 20:54:06, imcheng wrote: > is this used? Used in test, which was accessing private member directly. https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_service_delegate_impl.h (right): https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.h:46: using RenderFrameHostId = On 2015/05/18 20:54:08, imcheng wrote: > Can this be moved to private? Done. https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.h:104: MediaSource GetDefaultMediaSource() const; On 2015/05/18 20:54:08, imcheng wrote: > default_source() ? Done. https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.h:129: static const size_t kMaxNumSources = 256; On 2015/05/18 20:54:08, imcheng wrote: > static const variables go at the top of public: block Done. https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.h:150: // not have a persentation URL, default to tab mirroring. On 2015/05/18 20:54:08, imcheng wrote: > how about: returns the tab mirroring MediaSource. Done. https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.h:154: // Returns |true| is frame |rfh_id| is the main frame of |web_contents_|. On 2015/05/18 20:54:08, imcheng wrote: > Returns |true| if ... Done. https://codereview.chromium.org/1132903002/diff/60001/content/public/browser/... File content/public/browser/presentation_service_delegate.h (right): https://codereview.chromium.org/1132903002/diff/60001/content/public/browser/... content/public/browser/presentation_service_delegate.h:50: typedef std::pair<int, int> RenderFrameHostId; On 2015/05/18 20:54:08, imcheng wrote: > I think this can be left out for now. Because you'd also need to change the APIs > below to take a RFHId. removed back.
https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:123: if (sink_observers_.contains(source.id())) On 2015/05/18 23:40:47, haibinlu wrote: > On 2015/05/18 20:54:06, imcheng wrote: > > A single add() will work: > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/containers/sc... > > can not remove it. because "new PresentationMediaSinksObserver" will auto add an > observer to router_. How about: add an empty scoped_ptr to the map, and then if it succeeded, reset the pointer with new PresentationMediaSinksObserver. https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:330: if (!presentation_frames_.contains(rfh_id)) { On 2015/05/18 23:40:47, haibinlu wrote: > On 2015/05/18 20:54:07, imcheng wrote: > > For this use case it looks like a single add() is sufficient -- add() does > > nothing if the key already exists. > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/containers/sc... > > But it'd create a new PresentationFrameObject and release if the element already > exists. same comment as above. https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:475: RenderFrameHost* main_frame = web_contents_->GetMainFrame(); On 2015/05/18 23:40:46, haibinlu wrote: > On 2015/05/18 20:54:07, imcheng wrote: > > Thinking about this some more, I think this is not quite correct. We need to > > iterate through all frames and call observer->OnDefaultPresentationStarted for > > each frame that has a matching default presentation URL. > > I am not sure this is the right behavior either. A client can sent arbitrary > presentation URL; what presents it from getting access to the session if it > purposely sets the same URL as the main frame? Per offline chat. This is correct behavior (at least for session started via browser icon) since the browser icon uses the source on the main frame. So only the main frame should be notified in this case. We need additional support for handling autojoin behavior. https://codereview.chromium.org/1132903002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:514: bool PresentationServiceDelegateImpl::HasScreenAvailabilityListenerFor( On 2015/05/18 23:40:47, haibinlu wrote: > On 2015/05/18 20:54:06, imcheng wrote: > > is this used? > > Used in test, which was accessing private member directly. Can we use friend class and FRIEND_TEST_ALL_PREFIXES instead to access private members instead? https://codereview.chromium.org/1132903002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/1132903002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:60: explicit PresentationFrame(content::WebContents* web_contents, rm explicit https://codereview.chromium.org/1132903002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:117: Reset(); Why does Reset() need to be called? The dtor will take care of clearing containers and deleting scoped_ptrs. https://codereview.chromium.org/1132903002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:119: delegate_observer_->OnDelegateDestroyed(); OnDelegateDestroyed() should be called in PresesentationServiceDelegateImpl's dtor. I see that you had to explicitly set the delegate_observer_ to nullptr before erasing it because OnDelegateDestroyed() is here. You shouldn't have to do that. https://codereview.chromium.org/1132903002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:185: using RenderFrameHostId = std::pair<int, int>; I don't think you need this. You can just reuse the type alias defined in the .h. https://codereview.chromium.org/1132903002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:188: explicit PresentationFrameMap(content::WebContents* web_contents, rm explicit https://codereview.chromium.org/1132903002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:194: int render_process_id, Use RenderFrameHostId here and below? https://codereview.chromium.org/1132903002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:328: presentation_frame->set_delegate_observer(nullptr); not needed if you move the OnDelegateDestroyed() to PSDelegateImpl dtor. Then this can simplify into an erase() call. https://codereview.chromium.org/1132903002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:364: frame_map_(new PresentationFrameMap(web_contents, nullptr)), you might as well pass in router_ here if you are not DCHECKing against it. https://codereview.chromium.org/1132903002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_service_delegate_impl.h (right): https://codereview.chromium.org/1132903002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.h:49: static const size_t kMaxNumSources = 256; This should be defined in PresentationFrameMap since it's the only place that uses the constant.
PTAL https://codereview.chromium.org/1132903002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/1132903002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:60: explicit PresentationFrame(content::WebContents* web_contents, On 2015/05/19 20:03:29, imcheng wrote: > rm explicit Done. https://codereview.chromium.org/1132903002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:117: Reset(); On 2015/05/19 20:03:29, imcheng wrote: > Why does Reset() need to be called? The dtor will take care of clearing > containers and deleting scoped_ptrs. removed https://codereview.chromium.org/1132903002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:119: delegate_observer_->OnDelegateDestroyed(); On 2015/05/19 20:03:29, imcheng wrote: > OnDelegateDestroyed() should be called in PresesentationServiceDelegateImpl's > dtor. I see that you had to explicitly set the delegate_observer_ to nullptr > before erasing it because OnDelegateDestroyed() is here. You shouldn't have to > do that. Done. https://codereview.chromium.org/1132903002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:185: using RenderFrameHostId = std::pair<int, int>; On 2015/05/19 20:03:29, imcheng wrote: > I don't think you need this. You can just reuse the type alias defined in the > .h. Done. https://codereview.chromium.org/1132903002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:188: explicit PresentationFrameMap(content::WebContents* web_contents, On 2015/05/19 20:03:29, imcheng wrote: > rm explicit Done. https://codereview.chromium.org/1132903002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:194: int render_process_id, On 2015/05/19 20:03:29, imcheng wrote: > Use RenderFrameHostId here and below? Done. https://codereview.chromium.org/1132903002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:328: presentation_frame->set_delegate_observer(nullptr); On 2015/05/19 20:03:29, imcheng wrote: > not needed if you move the OnDelegateDestroyed() to PSDelegateImpl dtor. Then > this can simplify into an erase() call. Done. https://codereview.chromium.org/1132903002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:364: frame_map_(new PresentationFrameMap(web_contents, nullptr)), On 2015/05/19 20:03:29, imcheng wrote: > you might as well pass in router_ here if you are not DCHECKing against it. will update this once MediaRouterMojoImplFactory lands. https://codereview.chromium.org/1132903002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_service_delegate_impl.h (right): https://codereview.chromium.org/1132903002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.h:49: static const size_t kMaxNumSources = 256; On 2015/05/19 20:03:29, imcheng wrote: > This should be defined in PresentationFrameMap since it's the only place that > uses the constant. It is sort of public contract with the caller of this impl. plus, the unit test uses it too.
haibinlu@chromium.org changed reviewers: + wez@chromium.org
https://codereview.chromium.org/1132903002/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/1132903002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:366: frame_map_->RegisterPresenationFrame(rfh_id, observer); nit: rfh_id can be inlined https://codereview.chromium.org/1132903002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:372: frame_map_->UnregisterPresenationFrame(rfh_id); ditto https://codereview.chromium.org/1132903002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:381: return frame_map_->AddScreenAvailabilityListener(rfh_id, listener); ditto https://codereview.chromium.org/1132903002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:390: frame_map_->RemoveScreenAvailabilityListener(rfh_id, listener); ditto
PTAL https://codereview.chromium.org/1132903002/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/1132903002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:366: frame_map_->RegisterPresenationFrame(rfh_id, observer); On 2015/05/19 22:00:46, imcheng wrote: > nit: rfh_id can be inlined Done. https://codereview.chromium.org/1132903002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:372: frame_map_->UnregisterPresenationFrame(rfh_id); On 2015/05/19 22:00:46, imcheng wrote: > ditto Done. https://codereview.chromium.org/1132903002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:381: return frame_map_->AddScreenAvailabilityListener(rfh_id, listener); On 2015/05/19 22:00:46, imcheng wrote: > ditto Done. https://codereview.chromium.org/1132903002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:390: frame_map_->RemoveScreenAvailabilityListener(rfh_id, listener); On 2015/05/19 22:00:46, imcheng wrote: > ditto Done.
lgtm
Is this ready for external review?
On 2015/05/26 17:43:52, mark a. foltz wrote: > Is this ready for external review? Yes, it is. though your lgtm is enough
Ping!
https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... File chrome/browser/media/router/BUILD.gn (right): https://codereview.chromium.org/1132903002/diff/1/chrome/browser/media/router... chrome/browser/media/router/BUILD.gn:54: "presentation_service_delegate_impl.h", Where are the browser and unit-test .ccs added to the GN build? https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/media_source_helper.cc (right): https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_source_helper.cc:29: // PresentationServiceDelegateImpl::default_source is resolved. What does it mean for it to be "resolved..? Can you refer to the bug # for the relevant work, instead? https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_source_helper.cc:52: std::string GetPresentationUrl(const MediaSource& source) { As per naming comment above, assuming you choose MediaSourceForPresentationUrl(), this should become PresentationUrlForMediaSource() or PresentationUrlFromMediaSource(). https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/media_source_helper.h (right): https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_source_helper.h:24: // MediaSource types are mirroring-enabled. Please remove this comment and replace it with a cleanup bug! https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_source_helper.h:30: MediaSource ForPresentationUrl(const std::string& presentation_url); These should be named e.g. MediaSourceForTab(), MediaSourceForDesktop() etc - the current naming doesn't fit the style-guide, and the meanings are non-obvious. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_source_helper.h:41: // ForPresentationUrl, and must be valid. So what does this return if it's _not_ from a valid MediaSource, or from one that doesn't have a Presentation URL? https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/media_source_helper_unittest.cc (right): https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_source_helper_unittest.cc:47: GetPresentationUrl(ForPresentationUrl("http://example.com/"))); nit: Test the invalid-source case as well. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/mock_media_router.h (right): https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/mock_media_router.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. This and the .cc file comment change just change the Copyright year - if the comment has the incorrect year then please fix it in a separate CL. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:35: // Helper function to extract ID for a RFH. RFH->RenderFrameHost. Suggest: "Returns the unique identifier for the supplied RenderViewHost." https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:36: RenderFrameHostId GetRenderFrameHostId(RenderFrameHost* rfh) { nit: rfh->render_view_host https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:42: // Gets the host name associated with the frame given by |rfh_id|. I think you need to be more specific about what the "host name associated with the frame" is about - you're really pulling out the name of the host from which the frame's content was loaded, to display in the UI, I think? You might consider calling this GetDisplayNameForFrame, instead. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:43: std::string GetSourceHostForFrame(RenderFrameHostId rfh_id) { nit: render_view_host_id https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:44: RenderFrameHost* rfh = RenderFrameHost::FromID(rfh_id.first, rfh_id.second); nit: render_view_host https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:45: DCHECK(rfh); Don't DCHECK |rfh| here - you're dereferencing it immediately afterward, which will crash anyway if it's NULL! https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:54: // Helper class for PresentationServiceDelegateImpl to manage nit: No need for "Helper class..." intro - suggest "Used by PresentationServiceDelegateImpl to..." nit: what does "manage" mean - wording seems vague? https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:56: class PresentationFrame final { We rarely use "final" in Chromium code, AFAIK, and in this case this class is local to this .cc file (incidentally, why is the class not in the anonymous namespace, as well? https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:63: // Returns true if listener was added. It looks like these APIs mirror corresponding APIs in PresentationServiceDelegate interface, in which case I'd suggest just a block comment indicating that fact rather than individual comments on each method. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:75: // Returns the number of screen-availability listeners. nit: Why does Has*ForTest() take a |source_id| but this doesn't? https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:78: // Sets the default presentation URL and ID. nit: This comment adds nothing! https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:83: std::string GetDefaultPresentationId() const; nit: Blank line after this. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:90: void OnDelegateDestroyed(); nit: Add comments to explain these. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:100: // Does not own these objects. nit: Suggest "References to the owning WebContents, and the corresponding MediaRouter." Don't think you really need a comment on |delegate_observer_| since it's clear from the setter/getter above how it's owned. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:122: if (sink_observers_.contains(source.id())) This doesn't look like it'll behave as intended if two listeners A & B are added for the same Id - in that case you'll early-exit rather than adding B, but if the caller later tries to remove B then you will actually remove A, below? https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:127: router_, listener, source))).second; This is weird - why are you returning .second? How can that ever be null/false here? https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:132: MediaSource source(GetMediaSourceFromListener(listener)); Do you need to check that the registered listener for the id is the one we're being asked to remove? https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:170: : ""; nit: I think this would read more clearly as if(...) return ...; return ""; https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:176: // i.e. offscreeen tab rendering. typo: offscreen Do we have off-screen tab rendering hooked up? Looks like this will actually mirror the sending tab? https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:184: // PresentationFrames. nit: As above, clarify in this comment what it means to "manage" them? https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:185: class PresentationFrameMap final { Suggest PresentationFrameManager rather than Map - map implies behaviour akin to std::map(), whereas this class actually has boilerplate functions that route through to underlying PresentationFrames, and doesn't expose any actual map capabilities, just uses one internally. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:192: // Returns true if listener was added. See above re having a block comment to indicate that these more or less mirror the PresentationServiceDelegate interface? https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:194: const RenderFrameHostId& rfh_id, render_view_host_id here and elsewhere https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:218: void RegisterPresenationFrame(const RenderFrameHostId& rfh_id, typo: RegisterPresentationFrame, and Unregister Document these two APIs? Since these register a DelegateObserver for a RenderFrame, why are they called [Un]registerPresentationFrame? https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:225: void OnDelegateDestroyed(); OnDelegatesDestroyed(), since this applies to all PresentationFrames, not to a specific one. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:236: // Does not own these two objects. nit: See elsewhere re wording these comments. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:244: DCHECK(web_contents_); nit: DCHECK(router_)? https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:282: for (const auto& pf : presentation_frames_) { nit: pf -> frame https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:301: : ""; nit: May be clearer as if (...) return x; return y; https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:329: for (auto& kv : presentation_frames_) nit: kv -> frame https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:347: // TODO(haibinlu): Get router from MediaRouterMojoImplFactory once it lands. Add bug # for that change https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:352: router_(NULL), nullptr https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:357: frame_map_->OnDelegateDestroyed(); Given that |frame_map_| is about to be destroyed as well, could you move the OnDelegateDestroyed() to be dispatched from its destructor, rather than from here, and get rid of OnDelegatesDestroyed()? https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:410: // This is the main frame, that means tab-level default presentation nit: that -> which https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:411: // might have been updated. Suggest "... which means we may have switched between flinging and tab-level mirroring." https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:448: NOTIMPLEMENTED(); Is this and the below going to be implemented? If so then add bug # https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:479: if (default_source_.Equals(source)) { Suggest restructuring this to use early-exit if !Equals(), and similarly for the other if()s, to avoid all this indentation. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:486: // available from MediaRoute URN. Bug #? https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:487: observer->OnDefaultPresentationStarted(content::PresentationSessionInfo( Why not have a boilerplate OnDefaultPresentationStarted() method on the PresentationFrame[Map], so you don't need GetDelegateObserver? https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/presentation_service_delegate_impl.h (right): https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.h:29: } nit: This namespace is just long enough that I'd recommend adding the trailing comment. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.h:37: using RenderFrameHostId = std::pair<int, int>; Since this alias is used only internally in the PresentationServiceDelegateImpl, let's move it to the private section of the class. Why is this using rather than typedef? Doesn't seem to be any advantage to using, and typedef is more familiar syntax, surely? https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.h:40: // instance of WebContents with the Chrome Media Router. This comment doesn't really help me understand what this class does. In particular, be explicit about where this fits into the overall implementation - my guess is that the generic Presentation API glue in WebContents delegates to this interface to actually service requests, and that this implementation services them using the Media Router, for example - is that correct? https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.h:42: // provide tab-level information that is required for creating s/provide/provides https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.h:43: // browser-initiated sessions. Again, it's not clear what "provides tab-level information..." means - who does it provide that information to, and what does "tab-level information" even mean? https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.h:45: : public content::WebContentsUserData<PresentationServiceDelegateImpl>, Suggest clarifying why we need to derive from this - are instances essentially "user data" from WebContents' point of view, for example? Is that how ownership of the instance is managed - does the WebContents own it and tear it down when it goes out of scope? https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.h:49: static const int kMaxNumSources = 256; Where is this used by callers, if at all? If it's not then move it into private section. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.h:56: // is one per frame. Surely this detail is described in the PresentationServiceDelegate interface comments, so you don't need it here? https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.h:102: void OnRouteCreated(const MediaRoute& route); Does this need to be part of the class' public API? Rather than "Notifies this object...", suggest "Callback invoked when a |route| has been created or joined...", since it's not clear what it means for the function to "notify this". https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.h:106: MediaSource default_source() const { return default_source_; } This and the default-media-source-observer APIs: this class is the PresentationServiceDelegate implementation - who is it that will call these additional APIs that aren't available through the PresentationServiceDelegate interface? https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.h:128: base::WeakPtr<PresentationServiceDelegateImpl> GetWeakPtr(); What is this required for? https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.h:146: RemoveObserversOnNavigation); Do these tests actually need to access this class' internals? Looking at the tests, the only APIs they seem to use are the two *ForTest() ones, which means you can just make those public - PRESUBMIT will then catch anyone trying to (ab)use them in non-test code. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.h:154: bool IsMainFrame(const RenderFrameHostId& rfh_id) const; nit: rfh_id -> render_frame_host_id https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.h:156: // Updates tab-level default MediaSource and source host name. If either What is the "source host name"? Do you mean the source's URL, or something else? e.g. what if the source is a mirrored tab, or the desktop? https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.h:157: // changed, notify the observers. nit: Why is this method not SetDefault...()? https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.h:159: const MediaSource& new_default_source, nit: No need for |new_| prefix, since it's implicit in the purpose of this method. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.h:168: // Manages frames that are using presentation API. This comment doesn't really add anything - in what way does it "manage" them? Is it basically just a store for those details? What does it map from and to? https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.h:175: // this class. What does "not owned by this class" mean here? https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.h:178: // Does not own these objects. Suggest something like "References to the WebContents that owns this instance, and associated browser profile's MediaRouter instance." https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.h:182: // NOTE: All WeakPtrs must be invalidated before member variables. nit: You don't need this comment; it's documented with WeakPtrFactory. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc (right): https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc:68: RenderFrameHostId rfh_id(0, 0); render_view_host_id :) https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc:98: // The RFH entry should have been removed since all of its listeners have RenderViewHost!
PTAL Some method names of PresentationServiceDelegate need to be updated (will do in a separate CL since it update content code) AddObserver --> SetObserver since there is at most one observer per presentation frame. AddScreenAvailabilityListener --> SetScreenAvailabilityListener since there is at most one availability listener per frame. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/media_source_helper.cc (right): https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_source_helper.cc:29: // PresentationServiceDelegateImpl::default_source is resolved. On 2015/05/27 22:37:49, Wez wrote: > What does it mean for it to be "resolved..? Can you refer to the bug # for the > relevant work, instead? comment removed. outdated TODO. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_source_helper.cc:52: std::string GetPresentationUrl(const MediaSource& source) { On 2015/05/27 22:37:49, Wez wrote: > As per naming comment above, assuming you choose > MediaSourceForPresentationUrl(), this should become > PresentationUrlForMediaSource() or PresentationUrlFromMediaSource(). Done. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/media_source_helper.h (right): https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_source_helper.h:24: // MediaSource types are mirroring-enabled. On 2015/05/27 22:37:49, Wez wrote: > Please remove this comment and replace it with a cleanup bug! comment removed. They are not just for WebUI. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_source_helper.h:30: MediaSource ForPresentationUrl(const std::string& presentation_url); On 2015/05/27 22:37:49, Wez wrote: > These should be named e.g. MediaSourceForTab(), MediaSourceForDesktop() etc - > the current naming doesn't fit the style-guide, and the meanings are > non-obvious. Done. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_source_helper.h:41: // ForPresentationUrl, and must be valid. On 2015/05/27 22:37:49, Wez wrote: > So what does this return if it's _not_ from a valid MediaSource, or from one > that doesn't have a Presentation URL? Updated the comment. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/media_source_helper_unittest.cc (right): https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_source_helper_unittest.cc:47: GetPresentationUrl(ForPresentationUrl("http://example.com/"))); On 2015/05/27 22:37:49, Wez wrote: > nit: Test the invalid-source case as well. the first 2 cases are invalid ones. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/mock_media_router.h (right): https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/mock_media_router.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2015/05/27 22:37:49, Wez wrote: > This and the .cc file comment change just change the Copyright year - if the > comment has the incorrect year then please fix it in a separate CL. Done. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:35: // Helper function to extract ID for a RFH. On 2015/05/27 22:37:51, Wez wrote: > RFH->RenderFrameHost. > > Suggest: "Returns the unique identifier for the supplied RenderViewHost." Done. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:36: RenderFrameHostId GetRenderFrameHostId(RenderFrameHost* rfh) { On 2015/05/27 22:37:51, Wez wrote: > nit: rfh->render_view_host Done. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:42: // Gets the host name associated with the frame given by |rfh_id|. On 2015/05/27 22:37:50, Wez wrote: > I think you need to be more specific about what the "host name associated with > the frame" is about - you're really pulling out the name of the host from which > the frame's content was loaded, to display in the UI, I think? You might > consider calling this GetDisplayNameForFrame, instead. Done. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:43: std::string GetSourceHostForFrame(RenderFrameHostId rfh_id) { On 2015/05/27 22:37:50, Wez wrote: > nit: render_view_host_id Done. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:44: RenderFrameHost* rfh = RenderFrameHost::FromID(rfh_id.first, rfh_id.second); On 2015/05/27 22:37:50, Wez wrote: > nit: render_view_host Done. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:45: DCHECK(rfh); On 2015/05/27 22:37:51, Wez wrote: > Don't DCHECK |rfh| here - you're dereferencing it immediately afterward, which > will crash anyway if it's NULL! Done. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:54: // Helper class for PresentationServiceDelegateImpl to manage On 2015/05/27 22:37:51, Wez wrote: > nit: No need for "Helper class..." intro - suggest "Used by > PresentationServiceDelegateImpl to..." > > nit: what does "manage" mean - wording seems vague? Done. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:56: class PresentationFrame final { On 2015/05/27 22:37:49, Wez wrote: > We rarely use "final" in Chromium code, AFAIK, and in this case this class is > local to this .cc file (incidentally, why is the class not in the anonymous > namespace, as well? Removed "final". If it is in anonymous namespace, then we can reference it presentation_service_delegate_impl.h as a private member. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:63: // Returns true if listener was added. On 2015/05/27 22:37:50, Wez wrote: > It looks like these APIs mirror corresponding APIs in > PresentationServiceDelegate interface, in which case I'd suggest just a block > comment indicating that fact rather than individual comments on each method. Mostly mirror PresentationFrameManager API. Updated the comments. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:75: // Returns the number of screen-availability listeners. On 2015/05/27 22:37:50, Wez wrote: > nit: Why does Has*ForTest() take a |source_id| but this doesn't? method removed. Checked the presentation API, there is at most one listener per presentation frame. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:78: // Sets the default presentation URL and ID. On 2015/05/27 22:37:50, Wez wrote: > nit: This comment adds nothing! removed https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:83: std::string GetDefaultPresentationId() const; On 2015/05/27 22:37:50, Wez wrote: > nit: Blank line after this. Acknowledged. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:90: void OnDelegateDestroyed(); On 2015/05/27 22:37:49, Wez wrote: > nit: Add comments to explain these. method removed https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:100: // Does not own these objects. On 2015/05/27 22:37:50, Wez wrote: > nit: Suggest "References to the owning WebContents, and the corresponding > MediaRouter." > > Don't think you really need a comment on |delegate_observer_| since it's clear > from the setter/getter above how it's owned. Done. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:122: if (sink_observers_.contains(source.id())) On 2015/05/27 22:37:51, Wez wrote: > This doesn't look like it'll behave as intended if two listeners A & B are added > for the same Id - in that case you'll early-exit rather than adding B, but if > the caller later tries to remove B then you will actually remove A, below? method updated. Checked the presentation API, there is at most one listener per presentation frame. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:127: router_, listener, source))).second; On 2015/05/27 22:37:51, Wez wrote: > This is weird - why are you returning .second? How can that ever be null/false > here? Updated to 'Setter' https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:132: MediaSource source(GetMediaSourceFromListener(listener)); On 2015/05/27 22:37:50, Wez wrote: > Do you need to check that the registered listener for the id is the one we're > being asked to remove? Done. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:170: : ""; On 2015/05/27 22:37:49, Wez wrote: > nit: I think this would read more clearly as if(...) return ...; return ""; Done. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:176: // i.e. offscreeen tab rendering. On 2015/05/27 22:37:50, Wez wrote: > typo: offscreen > > Do we have off-screen tab rendering hooked up? Looks like this will actually > mirror the sending tab? Off-screen tab mirroring is not hooked up. Updated comments. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:184: // PresentationFrames. On 2015/05/27 22:37:50, Wez wrote: > nit: As above, clarify in this comment what it means to "manage" them? Done. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:185: class PresentationFrameMap final { On 2015/05/27 22:37:49, Wez wrote: > Suggest PresentationFrameManager rather than Map - map implies behaviour akin to > std::map(), whereas this class actually has boilerplate functions that route > through to underlying PresentationFrames, and doesn't expose any actual map > capabilities, just uses one internally. Done. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:192: // Returns true if listener was added. On 2015/05/27 22:37:51, Wez wrote: > See above re having a block comment to indicate that these more or less mirror > the PresentationServiceDelegate interface? Done. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:194: const RenderFrameHostId& rfh_id, On 2015/05/27 22:37:51, Wez wrote: > render_view_host_id here and elsewhere Done. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:218: void RegisterPresenationFrame(const RenderFrameHostId& rfh_id, On 2015/05/27 22:37:50, Wez wrote: > typo: RegisterPresentationFrame, and Unregister > > Document these two APIs? > > Since these register a DelegateObserver for a RenderFrame, why are they called > [Un]registerPresentationFrame? Registers a presentation frame and adds an observer. The lifetime of a PresentationFrame is RegisterPresentationFrame (delegate observer added) Reset Reset ... UnregisterPresentationFrame (delegate_observer.OnDelegateDestroyed invoked). https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:225: void OnDelegateDestroyed(); On 2015/05/27 22:37:51, Wez wrote: > OnDelegatesDestroyed(), since this applies to all PresentationFrames, not to a > specific one. The delegate means PresentationServiceDelegateImpl, only one. Updated the method names. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:236: // Does not own these two objects. On 2015/05/27 22:37:51, Wez wrote: > nit: See elsewhere re wording these comments. Done. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:244: DCHECK(web_contents_); On 2015/05/27 22:37:50, Wez wrote: > nit: DCHECK(router_)? cannot do this check now. Need to Derek's CL for MediaRouterMojoImplFactory to land, then we can get the right MediaRouter* from it. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:282: for (const auto& pf : presentation_frames_) { On 2015/05/27 22:37:50, Wez wrote: > nit: pf -> frame method updated. with one screen listener per frame, this is just numOfFrames. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:301: : ""; On 2015/05/27 22:37:49, Wez wrote: > nit: May be clearer as if (...) return x; return y; Done. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:329: for (auto& kv : presentation_frames_) On 2015/05/27 22:37:49, Wez wrote: > nit: kv -> frame Done. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:347: // TODO(haibinlu): Get router from MediaRouterMojoImplFactory once it lands. On 2015/05/27 22:37:49, Wez wrote: > Add bug # for that change Done. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:352: router_(NULL), On 2015/05/27 22:37:51, Wez wrote: > nullptr Done. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:357: frame_map_->OnDelegateDestroyed(); On 2015/05/27 22:37:50, Wez wrote: > Given that |frame_map_| is about to be destroyed as well, could you move the > OnDelegateDestroyed() to be dispatched from its destructor, rather than from > here, and get rid of OnDelegatesDestroyed()? Done. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:410: // This is the main frame, that means tab-level default presentation On 2015/05/27 22:37:50, Wez wrote: > nit: that -> which Done. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:411: // might have been updated. On 2015/05/27 22:37:50, Wez wrote: > Suggest "... which means we may have switched between flinging and tab-level > mirroring." This is caused by apps update its default presentation URL. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:448: NOTIMPLEMENTED(); On 2015/05/27 22:37:51, Wez wrote: > Is this and the below going to be implemented? If so then add bug # Done. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:479: if (default_source_.Equals(source)) { On 2015/05/27 22:37:51, Wez wrote: > Suggest restructuring this to use early-exit if !Equals(), and similarly for the > other if()s, to avoid all this indentation. Done. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:486: // available from MediaRoute URN. On 2015/05/27 22:37:51, Wez wrote: > Bug #? Done. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:487: observer->OnDefaultPresentationStarted(content::PresentationSessionInfo( On 2015/05/27 22:37:50, Wez wrote: > Why not have a boilerplate OnDefaultPresentationStarted() method on the > PresentationFrame[Map], so you don't need GetDelegateObserver? Done. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/presentation_service_delegate_impl.h (right): https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.h:29: } On 2015/05/27 22:37:53, Wez wrote: > nit: This namespace is just long enough that I'd recommend adding the trailing > comment. Done. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.h:37: using RenderFrameHostId = std::pair<int, int>; On 2015/05/27 22:37:52, Wez wrote: > Since this alias is used only internally in the PresentationServiceDelegateImpl, > let's move it to the private section of the class. > > Why is this using rather than typedef? Doesn't seem to be any advantage to > using, and typedef is more familiar syntax, surely? They are the same. mfoltz suggested using type aliases. Some private functions use "const RenderFrameHostId&" https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.h:40: // instance of WebContents with the Chrome Media Router. On 2015/05/27 22:37:52, Wez wrote: > This comment doesn't really help me understand what this class does. > > In particular, be explicit about where this fits into the overall implementation > - my guess is that the generic Presentation API glue in WebContents delegates to > this interface to actually service requests, and that this implementation > services them using the Media Router, for example - is that correct? Correct.Updated comments. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.h:42: // provide tab-level information that is required for creating On 2015/05/27 22:37:52, Wez wrote: > s/provide/provides Done. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.h:43: // browser-initiated sessions. On 2015/05/27 22:37:52, Wez wrote: > Again, it's not clear what "provides tab-level information..." means - who does > it provide that information to, and what does "tab-level information" even mean? Updated to "default presentation URL" https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.h:45: : public content::WebContentsUserData<PresentationServiceDelegateImpl>, On 2015/05/27 22:37:53, Wez wrote: > Suggest clarifying why we need to derive from this - are instances essentially > "user data" from WebContents' point of view, for example? Is that how ownership > of the instance is managed - does the WebContents own it and tear it down when > it goes out of scope? Done. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.h:49: static const int kMaxNumSources = 256; On 2015/05/27 22:37:52, Wez wrote: > Where is this used by callers, if at all? If it's not then move it into private > section. moved to private. it is used by unit tests. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.h:56: // is one per frame. On 2015/05/27 22:37:52, Wez wrote: > Surely this detail is described in the PresentationServiceDelegate interface > comments, so you don't need it here? removed https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.h:102: void OnRouteCreated(const MediaRoute& route); On 2015/05/27 22:37:53, Wez wrote: > Does this need to be part of the class' public API? > > Rather than "Notifies this object...", suggest "Callback invoked when a |route| > has been created or joined...", since it's not clear what it means for the > function to "notify this". It does. Callers to be added in another patch. When MR calls this method, PresentationServiceDelegate.Observer.OnDefaultPresentationStarted will be invoked. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.h:175: // this class. On 2015/05/27 22:37:52, Wez wrote: > What does "not owned by this class" mean here? Done. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.h:178: // Does not own these objects. On 2015/05/27 22:37:52, Wez wrote: > Suggest something like "References to the WebContents that owns this instance, > and associated browser profile's MediaRouter instance." Done. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.h:182: // NOTE: All WeakPtrs must be invalidated before member variables. On 2015/05/27 22:37:52, Wez wrote: > nit: You don't need this comment; it's documented with WeakPtrFactory. Done. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc (right): https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc:68: RenderFrameHostId rfh_id(0, 0); On 2015/05/27 22:37:53, Wez wrote: > render_view_host_id :) removed. https://codereview.chromium.org/1132903002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc:98: // The RFH entry should have been removed since all of its listeners have On 2015/05/27 22:37:53, Wez wrote: > RenderViewHost! removed
Please update the description to include the renaming of the media source helper methods. MOstly minor comments https://codereview.chromium.org/1132903002/diff/160001/chrome/browser/media/r... File chrome/browser/media/router/media_source_helper.h (right): https://codereview.chromium.org/1132903002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/media_source_helper.h:17: MediaSource MediaSourceForTab(int tab_id); Rather than creating longer camel-cased names that get harder to read, I would be okay with either: - Adding these as static factory methods in MediaSource, so they can be written as MediaSource::ForTab() - Declaring a source_helper namespace in media_router, so they are written as source_helper::ForTab() But I feel like we have already renamed these methods a few times and we are just bike-shedding at this point, so not worth spending further time on. https://codereview.chromium.org/1132903002/diff/160001/chrome/browser/media/r... File chrome/browser/media/router/presentation_helper.h (right): https://codereview.chromium.org/1132903002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/presentation_helper.h:13: bool IsValidPresentationUrl(const std::string& url); Is there a reason this isn't part of media_source_helper.{h,cc}? I guess this is not specific to the media router, only the Presentation API. https://codereview.chromium.org/1132903002/diff/160001/chrome/browser/media/r... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/1132903002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:68: // Mirror corresponding APIs in PresentationServiceDelegateImpl. Why doesn't this implement content::PresentationServiceDelegate? Is it because we aren't handling the messaging APIs? It would be more explicit to have an interface shared by PresentationServiceDelegateImpl, PresentationFrame and PresentationFrameManager to show they are all part of the same delegation chain. https://codereview.chromium.org/1132903002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:93: scoped_ptr<PresentationMediaSinksObserver> sink_observer_; nit: sinks_observer_ https://codereview.chromium.org/1132903002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:168: return default_presentation_info_->presentation_id; return default_presentation_info_ ? default_presentation_info_->presentation_id : ""; https://codereview.chromium.org/1132903002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:176: if (presentation_url.empty()) Ditto https://codereview.chromium.org/1132903002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:184: using DelegateObserver = content::PresentationServiceDelegate::Observer; Repeated definition. Move to file level? https://codereview.chromium.org/1132903002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:256: auto presentation_frame = GetOrAddFrame(render_frame_host_id); GetOrAddPresentationFrame? (We aren't creating a DOM frame here). https://codereview.chromium.org/1132903002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:333: // BUG=464205 https://codereview.chromium.org/1143603004/ has landed. Rebase and update this patch? https://codereview.chromium.org/1132903002/diff/160001/chrome/browser/media/r... File chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc (right): https://codereview.chromium.org/1132903002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc:38: class PresentationServiceDelegateImplTest Please ensure there is test coverage for: - Updating presentation url in main frame to non-empty value. - Updating presentation url in main frame to empty value. - Removing a frame and making sure the PresentationFrame gets cleaned up.
https://codereview.chromium.org/1132903002/diff/160001/chrome/browser/media/r... File chrome/browser/media/router/media_source_helper.h (right): https://codereview.chromium.org/1132903002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/media_source_helper.h:17: MediaSource MediaSourceForTab(int tab_id); On 2015/06/01 19:18:12, mark a. foltz wrote: > Rather than creating longer camel-cased names that get harder to read, I would > be okay with either: > > - Adding these as static factory methods in MediaSource, so they can be written > as MediaSource::ForTab() > > - Declaring a source_helper namespace in media_router, so they are written as > source_helper::ForTab() > > But I feel like we have already renamed these methods a few times and we are > just bike-shedding at this point, so not worth spending further time on. Acknowledged. https://codereview.chromium.org/1132903002/diff/160001/chrome/browser/media/r... File chrome/browser/media/router/presentation_helper.h (right): https://codereview.chromium.org/1132903002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/presentation_helper.h:13: bool IsValidPresentationUrl(const std::string& url); On 2015/06/01 19:18:12, mark a. foltz wrote: > Is there a reason this isn't part of media_source_helper.{h,cc}? I guess this > is not specific to the media router, only the Presentation API. merged into media_source_helper https://codereview.chromium.org/1132903002/diff/160001/chrome/browser/media/r... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/1132903002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:68: // Mirror corresponding APIs in PresentationServiceDelegateImpl. On 2015/06/01 19:18:12, mark a. foltz wrote: > Why doesn't this implement content::PresentationServiceDelegate? Is it because > we aren't handling the messaging APIs? > > It would be more explicit to have an interface shared by > PresentationServiceDelegateImpl, PresentationFrame and PresentationFrameManager > to show they are all part of the same delegation chain. PresentationFrame/Manager do not implement StartSession, JoinSession etc. https://codereview.chromium.org/1132903002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:93: scoped_ptr<PresentationMediaSinksObserver> sink_observer_; On 2015/06/01 19:18:12, mark a. foltz wrote: > nit: sinks_observer_ Done. https://codereview.chromium.org/1132903002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:168: return default_presentation_info_->presentation_id; On 2015/06/01 19:18:12, mark a. foltz wrote: > return default_presentation_info_ ? default_presentation_info_->presentation_id > : ""; wez@ prefers "if() return a; return b;" style. I am ok either way. https://codereview.chromium.org/1132903002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:176: if (presentation_url.empty()) On 2015/06/01 19:18:12, mark a. foltz wrote: > Ditto wez@ prefers "if() return a; return b;" style. I am ok either way. https://codereview.chromium.org/1132903002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:184: using DelegateObserver = content::PresentationServiceDelegate::Observer; On 2015/06/01 19:18:13, mark a. foltz wrote: > Repeated definition. Move to file level? Done. https://codereview.chromium.org/1132903002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:256: auto presentation_frame = GetOrAddFrame(render_frame_host_id); On 2015/06/01 19:18:12, mark a. foltz wrote: > GetOrAddPresentationFrame? (We aren't creating a DOM frame here). Done. https://codereview.chromium.org/1132903002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:333: // BUG=464205 On 2015/06/01 19:18:12, mark a. foltz wrote: > https://codereview.chromium.org/1143603004/ has landed. Rebase and update this > patch? Done. https://codereview.chromium.org/1132903002/diff/160001/chrome/browser/media/r... File chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc (right): https://codereview.chromium.org/1132903002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc:38: class PresentationServiceDelegateImplTest On 2015/06/01 19:18:13, mark a. foltz wrote: > Please ensure there is test coverage for: > - Updating presentation url in main frame to non-empty value. > - Updating presentation url in main frame to empty value. > - Removing a frame and making sure the PresentationFrame gets cleaned up. Done.
PTAL
lgtm, please rebase before landing. https://codereview.chromium.org/1132903002/diff/160001/chrome/browser/media/r... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/1132903002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:68: // Mirror corresponding APIs in PresentationServiceDelegateImpl. On 2015/06/01 21:44:58, haibinlu wrote: > On 2015/06/01 19:18:12, mark a. foltz wrote: > > Why doesn't this implement content::PresentationServiceDelegate? Is it > because > > we aren't handling the messaging APIs? > > > > It would be more explicit to have an interface shared by > > PresentationServiceDelegateImpl, PresentationFrame and > PresentationFrameManager > > to show they are all part of the same delegation chain. > > PresentationFrame/Manager do not implement StartSession, JoinSession etc. Acknowledged. https://codereview.chromium.org/1132903002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:168: return default_presentation_info_->presentation_id; On 2015/06/01 21:44:58, haibinlu wrote: > On 2015/06/01 19:18:12, mark a. foltz wrote: > > return default_presentation_info_ ? > default_presentation_info_->presentation_id > > : ""; > > wez@ prefers "if() return a; return b;" style. I am ok either way. Ok, we have different preferences on that front. Choose the style that's consistent with the rest of the code in this folder.
The CQ bit was checked by haibinlu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from imcheng@google.com, mfoltz@chromium.org Link to the patchset: https://codereview.chromium.org/1132903002/#ps200001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1132903002/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/571a4dfac35342be8d07cff886fb48349f546b36 Cr-Commit-Position: refs/heads/master@{#332320} |
