Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2714783002:
[Presentation API] (browser side) Implement reconnect() for 1-UA mode (Closed)
|
Created:
3 years, 10 months ago by zhaobin Modified:
3 years, 9 months ago CC:
chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, media-router+watch_chromium.org, mlamouri+watch-content_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Presentation API] (browser side) Implement reconnect() for 1-UA mode
If PSDImpl::JoinSession() is called for offscreeen presentation, go to OPM instead of MRP to get media route.
BUG=688233
Review-Url: https://codereview.chromium.org/2714783002
Cr-Commit-Position: refs/heads/master@{#457870}
Committed: https://chromium.googlesource.com/chromium/src/+/f00e8b48a63cdac2e5d21fe63581bb488c3a355f
Patch Set 1 #Patch Set 2 : remove duplicate unit tests #
Total comments: 12
Patch Set 3 : resolve code review comments from Derek and Mark #
Total comments: 4
Patch Set 4 : resolve code review comments from Derek #
Total comments: 6
Patch Set 5 : resolve code review comments from Mark #
Messages
Total messages: 29 (17 generated)
zhaobin@chromium.org changed reviewers: + imcheng@chromium.org, mfoltz@chromium.org
https://codereview.chromium.org/2714783002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/offscreen_presentation_manager.h (right): https://codereview.chromium.org/2714783002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:201: std::unique_ptr<MediaRoute> route_; What does it mean to have an OffscreenPresentation without a route set? Is this possible because the object might have been created on the receiver side first? https://codereview.chromium.org/2714783002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/2714783002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:928: offscreen_presentation_manager->UnregisterOffscreenPresentationController( Do we currently change state on close() for 1-UA or is that a TODO?
https://codereview.chromium.org/2714783002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/offscreen_presentation_manager.h (right): https://codereview.chromium.org/2714783002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:201: std::unique_ptr<MediaRoute> route_; On 2017/02/24 01:37:46, imcheng wrote: > What does it mean to have an OffscreenPresentation without a route set? Is this > possible because the object might have been created on the receiver side first? Yes. RegisterReceiver() is invoked by receiver's PresentationDispatcher and PSImpl::SetClient(). RegisterRoute() is invoked by controller's PSDImpl::OnStartSessionSucceeded(). Order could be random? https://codereview.chromium.org/2714783002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/2714783002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:928: offscreen_presentation_manager->UnregisterOffscreenPresentationController( On 2017/02/24 01:37:46, imcheng wrote: > Do we currently change state on close() for 1-UA or is that a TODO? Have a different patch for change state on close(): https://codereview.chromium.org/2714693002/
https://codereview.chromium.org/2714783002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/offscreen_presentation_manager.h (right): https://codereview.chromium.org/2714783002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:201: std::unique_ptr<MediaRoute> route_; On 2017/02/24 02:35:33, zhaobin wrote: > On 2017/02/24 01:37:46, imcheng wrote: > > What does it mean to have an OffscreenPresentation without a route set? Is > this > > possible because the object might have been created on the receiver side > first? > > Yes. RegisterReceiver() is invoked by receiver's PresentationDispatcher and > PSImpl::SetClient(). RegisterRoute() is invoked by controller's > PSDImpl::OnStartSessionSucceeded(). Order could be random? Hmm, this almost feels as if we should have a presentation ID to MediaRoute mapping in MediaRouter instead. It could also help us solve the problem of deduplicating JoinRoute / ConnectRouteByRouteId. Mark, WDYT?
Looks good overall but I would lean towards having a presentation id -> MediaRoute mapping in the MR for tracking both 2UA and 1UA presentations, instead of one inside OffscreenPresentationManager. High level sketch: - The mapping could be kept in InternalMediaRoutesObserver inside MediaRouterBase - Either keep a std::map<> inside, or add presentation ID to MediaRoute and search the list kept by the observer - Watch out for "special" IDs passed by Cast If you agree, might be good to create the mapping in a separate patch, then land the reconnect() behavior on top of it (which should then be pretty simple). https://codereview.chromium.org/2714783002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/offscreen_presentation_manager.cc (right): https://codereview.chromium.org/2714783002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.cc:163: route_ = base::MakeUnique<MediaRoute>(route); Is it important that this take ownership of the passed Route object or can it make a copy? https://codereview.chromium.org/2714783002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/offscreen_presentation_manager.h (right): https://codereview.chromium.org/2714783002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:201: std::unique_ptr<MediaRoute> route_; On 2017/02/24 at 20:52:25, imcheng wrote: > On 2017/02/24 02:35:33, zhaobin wrote: > > On 2017/02/24 01:37:46, imcheng wrote: > > > What does it mean to have an OffscreenPresentation without a route set? Is > > this > > > possible because the object might have been created on the receiver side > > first? > > > > Yes. RegisterReceiver() is invoked by receiver's PresentationDispatcher and > > PSImpl::SetClient(). RegisterRoute() is invoked by controller's > > PSDImpl::OnStartSessionSucceeded(). Order could be random? > > Hmm, this almost feels as if we should have a presentation ID to MediaRoute mapping in MediaRouter instead. It could also help us solve the problem of deduplicating JoinRoute / ConnectRouteByRouteId. Mark, WDYT? What if we added a presentationId field to MediaRoute and a method in MR to look through MediaRoutes for a matching ID? https://codereview.chromium.org/2714783002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/2714783002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:896: presentation_urls[0], presentation_id, success_cb, Connection requires matching both presentation ID and presentation URL. So, you should check that the URL from the route's MediaSource matches one of the presentation URLs passed in, and pass the matching URL to OnJoinRouteResponse.
Close https://codereview.chromium.org/2731043002/ and keep route info in OffscreenPresentationManager instead. https://codereview.chromium.org/2714783002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/offscreen_presentation_manager.cc (right): https://codereview.chromium.org/2714783002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.cc:163: route_ = base::MakeUnique<MediaRoute>(route); On 2017/03/01 06:26:32, mark a. foltz wrote: > Is it important that this take ownership of the passed Route object or can it > make a copy? Done. https://codereview.chromium.org/2714783002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/2714783002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:896: presentation_urls[0], presentation_id, success_cb, On 2017/03/01 06:26:32, mark a. foltz wrote: > Connection requires matching both presentation ID and presentation URL. > > So, you should check that the URL from the route's MediaSource matches one of > the presentation URLs passed in, and pass the matching URL to > OnJoinRouteResponse. Done.
lgtm https://codereview.chromium.org/2714783002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/offscreen_presentation_manager.cc (right): https://codereview.chromium.org/2714783002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.cc:163: route_ = base::MakeUnique<MediaRoute>(route); On 2017/03/15 18:18:56, zhaobin wrote: > On 2017/03/01 06:26:32, mark a. foltz wrote: > > Is it important that this take ownership of the passed Route object or can it > > make a copy? > > Done. I thought this was a unique_ptr because there's a window of time when an OffscreenPresentation is not associated with a MediaRoute yet (if receiver side registered first). I don't think we should be default constructing MediaRoute. https://codereview.chromium.org/2714783002/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/2714783002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:884: DCHECK(route); Can this ever be nullptr in a race condition? If so we should return error instead of DCHECKing / crashing. https://codereview.chromium.org/2714783002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp (left): https://codereview.chromium.org/2714783002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp:444: NOTREACHED(); Why this change?
https://codereview.chromium.org/2714783002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/offscreen_presentation_manager.cc (right): https://codereview.chromium.org/2714783002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.cc:163: route_ = base::MakeUnique<MediaRoute>(route); On 2017/03/16 01:42:51, imcheng wrote: > On 2017/03/15 18:18:56, zhaobin wrote: > > On 2017/03/01 06:26:32, mark a. foltz wrote: > > > Is it important that this take ownership of the passed Route object or can > it > > > make a copy? > > > > Done. > > I thought this was a unique_ptr because there's a window of time when an > OffscreenPresentation is not associated with a MediaRoute yet (if receiver side > registered first). I don't think we should be default constructing MediaRoute. Done. https://codereview.chromium.org/2714783002/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/2714783002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:884: DCHECK(route); On 2017/03/16 01:42:51, imcheng wrote: > Can this ever be nullptr in a race condition? If so we should return error > instead of DCHECKing / crashing. Done. https://codereview.chromium.org/2714783002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp (left): https://codereview.chromium.org/2714783002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp:444: NOTREACHED(); On 2017/03/16 01:42:51, imcheng wrote: > Why this change? reconnect() on the same connection object will change 'closed' connection to 'connecting'.
The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM, minor comments only https://codereview.chromium.org/2714783002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/offscreen_presentation_manager.cc (right): https://codereview.chromium.org/2714783002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.cc:102: if (it == offscreen_presentations_.end()) Nit: slight preference for ternary return. https://codereview.chromium.org/2714783002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.cc:134: route_ = base::MakeUnique<MediaRoute>(route); I wonder if this would work better as a base::Optional<MediaRoute>? Then you could write route_ = route here. Also you're not really taking ownership of |route|, just making a copy at a later time. I don't feel strongly either way though. https://codereview.chromium.org/2714783002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc (right): https://codereview.chromium.org/2714783002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc:141: MockOffscreenPresentationManager* GetMockOffscreenPresentationManager() { Maybe return MockOffscreenPresentationManager& to simplify usage below.
The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run
https://codereview.chromium.org/2714783002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/offscreen_presentation_manager.cc (right): https://codereview.chromium.org/2714783002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.cc:102: if (it == offscreen_presentations_.end()) On 2017/03/16 23:02:17, mark a. foltz wrote: > Nit: slight preference for ternary return. Done. https://codereview.chromium.org/2714783002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.cc:134: route_ = base::MakeUnique<MediaRoute>(route); On 2017/03/16 23:02:17, mark a. foltz wrote: > I wonder if this would work better as a base::Optional<MediaRoute>? Then you > could write route_ = route here. Also you're not really taking ownership of > |route|, just making a copy at a later time. I don't feel strongly either way > though. Done. https://codereview.chromium.org/2714783002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc (right): https://codereview.chromium.org/2714783002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc:141: MockOffscreenPresentationManager* GetMockOffscreenPresentationManager() { On 2017/03/16 23:02:17, mark a. foltz wrote: > Maybe return MockOffscreenPresentationManager& to simplify usage below. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by zhaobin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from imcheng@chromium.org, mfoltz@chromium.org Link to the patchset: https://codereview.chromium.org/2714783002/#ps80001 (title: "resolve code review comments from Mark")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1489781006221240,
"parent_rev": "8c0b7bf620cb0ff2d74470b17800281e65405cdd", "commit_rev":
"f00e8b48a63cdac2e5d21fe63581bb488c3a355f"}
Message was sent while issue was closed.
Description was changed from ========== [Presentation API] (browser side) Implement reconnect() for 1-UA mode If PSDImpl::JoinSession() is called for offscreeen presentation, go to OPM instead of MRP to get media route. BUG=688233 ========== to ========== [Presentation API] (browser side) Implement reconnect() for 1-UA mode If PSDImpl::JoinSession() is called for offscreeen presentation, go to OPM instead of MRP to get media route. BUG=688233 Review-Url: https://codereview.chromium.org/2714783002 Cr-Commit-Position: refs/heads/master@{#457870} Committed: https://chromium.googlesource.com/chromium/src/+/f00e8b48a63cdac2e5d21fe63581... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/f00e8b48a63cdac2e5d21fe63581... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
