Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2471573005:
[Presentation API] (5th) (1-UA) integrate controller and receiver side for 1-UA messaging (Closed)
|
Created:
4 years, 1 month ago by zhaobin Modified:
3 years, 8 months ago CC:
Aaron Boodman, abarth-chromium, blink-reviews, blink-reviews-api_chromium.org, chromium-apps-reviews_chromium.org, chromium-reviews, darin (slow to review), darin-cc_chromium.org, dglazkov+blink, extensions-reviews_chromium.org, jam, miu+watch_chromium.org, mlamouri+watch-content_chromium.org, qsr+mojo_chromium.org, takumif, viettrungluu+watch_chromium.org, xjz+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Presentation API] (5th)(1-UA) integrate controller and receiver side for 1-UA messaging
This CL actually turns on 1-UA messaging:
- When presentation starts, a new route is created and returned to MR. In PresentationDispatcher, we send controller_connection_proxy and receiver_connection_request back to browser via PresentationService::SetPresentationConnection()
For 1-UA:
- In PresentationServiceDelegateImpl::ConnectToPresentation(), we register proxy and request as a controller connection entry to OPM. If there has been a receiver_call_back, invoke it;
- When offscreen tab is rendered, we register a receiverPSDImpl, which registers a receiver_call_back to OPM. If there is any pending controller connection entry, invoke receiver_call_back for each controller connection entry;
- In receiver_call_back (PresentationDispatcher::OnReceiverConnectionAvailable() function), we create a receiver_connection_proxy object, bind it to receiver_connection_request (input parameter) and connect it with controller_connection_proxy (input parameter). Message sending between controller Blink connection and receiver Blink connection may happen afterwards.
For 2-UA:
- In PresentationServiceDelegateImpl::ConnectToPresentation(), we create a browser_connection_proxy object, bind it to receiver_connection_request (input parameter) and connect it with controller_connection_proxy (input parameter). Message sending from controller Blink connection to media router may happen afterwards.
More details in code comments and design doc (use chromium.org account): https://docs.google.com/document/d/1XM3jhMJTQyhEC5PDAAJFNIaKh6UUEihqZDz_ztEe4Co/edit#heading=h.hadpx5oi0gml
Patch set 4 updates:
- added BrowserPresentationConnectionProxy class
- Added PresentationFrame::IsOffscreen() and PresentationFrame::OnBrowserConnectionAvailable()
- PresentationConnection::OnMessage() takes a callback
- PresentationDispatcher::SendXXX() takes PresentationConnectionProxy as parameter
BUG=525660, 513859
Review-Url: https://codereview.chromium.org/2471573005
Cr-Original-Commit-Position: refs/heads/master@{#448299}
Committed: https://chromium.googlesource.com/chromium/src/+/4b94a1fe3f58aacad2c46bbfd84aa780469e15be
Review-Url: https://codereview.chromium.org/2471573005
Cr-Commit-Position: refs/heads/master@{#448509}
Committed: https://chromium.googlesource.com/chromium/src/+/42407c7feb218491f72b05732fa83b4679144da4
Patch Set 1 #Patch Set 2 : call OPM.unregisterOPMController() in PresentationFrame::Reset() #Patch Set 3 : rebase #
Total comments: 5
Patch Set 4 : split renderer related changes into 4th patch #
Total comments: 14
Patch Set 5 : resolve code review comments from Mark #Patch Set 6 : merge and refactor #
Total comments: 27
Patch Set 7 : resolve code review comments from Mark #Patch Set 8 : merge with issue 2471263003 #
Total comments: 20
Patch Set 9 : resolve code review comments from Mark #
Total comments: 17
Patch Set 10 : add unit test for browser_presentation_connection_proxy #
Total comments: 34
Patch Set 11 : resolve code review comments from Derek #
Total comments: 6
Patch Set 12 : resolve code review comments from Derek #
Total comments: 6
Patch Set 13 : resolve code review comments from miu #Patch Set 14 : merge with master #Patch Set 15 : Fix PresentationDispatcherTest failures #
Total comments: 2
Patch Set 16 : merge with master and fix unit tests #Patch Set 17 : fix presentation-reconnect.html crash #Patch Set 18 : merge with master #Messages
Total messages: 75 (47 generated)
Patchset #1 (id:1) has been deleted
zhaobin@chromium.org changed reviewers: + imcheng@chromium.org, mfoltz@chromium.org
Overall looks good, however, I'd like to finish crbug.com/632623 which has been on my plate for a while, to implement a typemap (automatic type conversion) between Blink types like WebString/WebVector<uint8_t> and PresentationSessionMessage. This eliminates manual conversion code and is more secure. https://codereview.chromium.org/2471573005/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/tab_capture/offscreen_tab.cc (right): https://codereview.chromium.org/2471573005/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/tab_capture/offscreen_tab.cc:9: #include <utility> Are these missing #includes from existing code, or can they be removed? https://codereview.chromium.org/2471573005/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/2471573005/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:252: pid_route_id.first, render_frame_host_id_); Side comment: If we could ensure 1 route == 1 presentation, we could (perhaps) consolidate presentation ids and media route ids. Worth discussing when we have a moment. https://codereview.chromium.org/2471573005/diff/60001/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/2471573005/diff/60001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:79: blink::mojom::SessionMessagePtr ToMojoTextMessage( We should really implement Mojo type traits for session messages before landing this so we're not adding new manual type conversion code. https://codereview.chromium.org/2471573005/diff/60001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:91: size_t length) { We'd have to implement a converter between blink::mojom::SessionMessage and WebVector<uint_8> or some single type.
Description was changed from ========== [Presentation API] (5th)(1-UA) integrate controller and receiver side for 1-UA messaging This CL actually turns on 1-UA messaging: - When offscreen presentation starts, a new route is created and returned to MR. In PresentationDispatcher, we register a controller_connection_proxy to OPM. If there has been a receiver_call_back, invoke it. - When offscreen tab is rendered, we register a receiverPSDImpl, which registers a receiver_call_back to OPM. If there is any pending controller proxy, invoke receiver_call_back for each controller_proxy. - In receiver_call_back (PresentationDispatcher::OnReceiverConnectionAvailable() function), we set up a connection between controller_connection_proxy and receiver_connection_proxy. Message sending may happen afterwards. More details in code comments and design doc (use chromium.org account): https://docs.google.com/document/d/1XM3jhMJTQyhEC5PDAAJFNIaKh6UUEihqZDz_ztEe4... BUG=525660 ========== to ========== [Presentation API] (5th)(1-UA) integrate controller and receiver side for 1-UA messaging This CL actually turns on 1-UA messaging: - When offscreen presentation starts, a new route is created and returned to MR. In PresentationDispatcher, we register a controller_connection_proxy to OPM. If there has been a receiver_call_back, invoke it. - When offscreen tab is rendered, we register a receiverPSDImpl, which registers a receiver_call_back to OPM. If there is any pending controller proxy, invoke receiver_call_back for each controller_proxy. - In receiver_call_back (PresentationDispatcher::OnReceiverConnectionAvailable() function), we set up a connection between controller_connection_proxy and receiver_connection_proxy. Message sending may happen afterwards. More details in code comments and design doc (use chromium.org account): https://docs.google.com/document/d/1XM3jhMJTQyhEC5PDAAJFNIaKh6UUEihqZDz_ztEe4... Patch set 4 updates: - added BrowserPresentationConnectionProxy class - Added PresentationFrame::IsOffscreen() and PresentationFrame::OnBrowserConnectionAvailable() - PresentationConnection::OnMessage() takes a callback - PresentationDispatcher::SendXXX() takes PresentationConnectionProxy as parameter BUG=525660 ==========
Unifying message sending logic between 1-UA and 2-UA
previous 1-UA (Send msg from controller/receiver to receiver/controller)
controller blink PresentationConnection.send()
-> controller PresentationConnectionProxy.send()
--> (mojo call)
-> receiver PresentationConnectionProxy.onMessage()
-> receiver blink PresentationConnection.onMessage()
After:
controller blink PresentationConnection.send()
-> controller PresentationDispatcher.send()
-> controller PresentationConnectionProxy.send()
--> (mojo call)
-> receiver PresentationConnectionProxy.onMessage()
-> receiver blink PresentationConnection.onMessage()
Previous 2-UA (Send message from Page to MediaRouter)
controller blink PresentationConnection.send()
-> controller PresentationDispatcher.send()
-> controller PSImpl.send()
-> controller PSDImpl.send()
-> MediaRouter.send()
After:
controller blink PresentationConnection.send()
-> controller PresentationDispatcher.send()
-> controller PresentationConnectionProxy.send()
--> (mojo call)
-> BrowserPresentationConnectionProxy.onMessage()
-> MediaRouter.send()
// No change if send message from MediaRouter to Page (Will refactor later)
controller side is the same for both 1-UA and 2-UA now:
controller blink PresentationConnection.send()
-> controller PresentationDispatcher.send()
-> controller PresentationConnectionProxy.send()
https://codereview.chromium.org/2471573005/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/tab_capture/offscreen_tab.cc (right):
https://codereview.chromium.org/2471573005/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/tab_capture/offscreen_tab.cc:9: #include <utility>
On 2016/11/16 01:25:37, mark a. foltz wrote:
> Are these missing #includes from existing code, or can they be removed?
Done.
Patchset #4 (id:80001) has been deleted
Cool, it looks like with this flow the content side doesn't need to know whether a presentation connection is offscreen so this is definitely better than before. It looks like it still requires signaling from PresentationDispatcher to PSDImpl finish setting up the mojo connection - could you please remind me if we have already considered setting up the connection in controller PDispatcher / receiver PDispatcher (1UA) or PDispatcher / PSImpl (2UA) to avoid the round trip signaling? https://codereview.chromium.org/2471573005/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/2471573005/diff/100001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:333: session_messages_observers_.insert(std::make_pair( It looks like we should be able to invoke one of the connections in browser_connection_proxies_ instead of this :)
Overall looks okay. There are a lot of changes to the messaging flow in PSDImpl I'll need to examine more closely. Also, I'd really like to finish the type mapping before landing this, to remove the message conversion code in the proxy and dispatcher classes. https://codereview.chromium.org/2471573005/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/browser_presentation_connection_proxy.cc (right): https://codereview.chromium.org/2471573005/diff/100001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.cc:23: // set |data|, or size too large). Rewrap comment. https://codereview.chromium.org/2471573005/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/browser_presentation_connection_proxy.h (right): https://codereview.chromium.org/2471573005/diff/100001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.h:23: // side PresentationConnection is always 'connected'. Wrap comments @ 80 cols https://codereview.chromium.org/2471573005/diff/100001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.h:33: // --> (mojo call to browser sid PresentationConnection) typo in 'sid' https://codereview.chromium.org/2471573005/diff/100001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.h:37: // Instance of this class is only created for non-offscreen presentations. We should probably decide on consistent terminology: the spec uses 1-UA and 2-UA to indicate whether the presentation is rendered locally or remotely, however, those terms make the most sense in the context of the spec. Maybe "locally rendered" for offscreen vs. "remotely rendered" for non-offscreen? Derek, WDYT? https://codereview.chromium.org/2471573005/diff/100001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.h:60: MediaRouter* router_; I'm assuming both of these pointers are unowned. What controls lifetime of |this|? https://codereview.chromium.org/2471573005/diff/100001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.h:65: std::unique_ptr<OnMessageCallback> on_message_callback_; By default, callbacks are copyable, and I'm not aware of a strong reason to pass ownership of them. So I don't think this needs to be a unique_ptr. https://cs.chromium.org/chromium/src/base/callback_forward.h?rcl=0&l=26
Patchset #6 (id:140001) has been deleted
https://codereview.chromium.org/2471573005/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/browser_presentation_connection_proxy.cc (right): https://codereview.chromium.org/2471573005/diff/100001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.cc:23: // set |data|, or size too large). On 2016/12/02 23:06:48, mark a. foltz wrote: > Rewrap comment. Done. https://codereview.chromium.org/2471573005/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/browser_presentation_connection_proxy.h (right): https://codereview.chromium.org/2471573005/diff/100001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.h:23: // side PresentationConnection is always 'connected'. On 2016/12/02 23:06:48, mark a. foltz wrote: > Wrap comments @ 80 cols Done. https://codereview.chromium.org/2471573005/diff/100001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.h:33: // --> (mojo call to browser sid PresentationConnection) On 2016/12/02 23:06:48, mark a. foltz wrote: > typo in 'sid' Done. https://codereview.chromium.org/2471573005/diff/100001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.h:37: // Instance of this class is only created for non-offscreen presentations. On 2016/12/02 23:06:48, mark a. foltz wrote: > We should probably decide on consistent terminology: the spec uses 1-UA and 2-UA > to indicate whether the presentation is rendered locally or remotely, however, > those terms make the most sense in the context of the spec. > > Maybe "locally rendered" for offscreen vs. "remotely rendered" for > non-offscreen? > > Derek, WDYT? Done. https://codereview.chromium.org/2471573005/diff/100001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.h:60: MediaRouter* router_; On 2016/12/02 23:06:48, mark a. foltz wrote: > I'm assuming both of these pointers are unowned. What controls lifetime of > |this|? Done. PresentationFrame owns BrowserPresentationConnectionProxy object. https://codereview.chromium.org/2471573005/diff/100001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.h:65: std::unique_ptr<OnMessageCallback> on_message_callback_; On 2016/12/02 23:06:48, mark a. foltz wrote: > By default, callbacks are copyable, and I'm not aware of a strong reason to pass > ownership of them. So I don't think this needs to be a unique_ptr. > > https://cs.chromium.org/chromium/src/base/callback_forward.h?rcl=0&l=26 Done. https://codereview.chromium.org/2471573005/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/2471573005/diff/100001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:333: session_messages_observers_.insert(std::make_pair( On 2016/11/29 00:33:22, imcheng wrote: > It looks like we should be able to invoke one of the connections in > browser_connection_proxies_ instead of this :) Yes, we can do a lot of refactoring after introducing BrowserConnectionProxy. Will do code cleanup and refactoring later in seperate patches.
A few comments TBD: PresentationServiceDelegateImpl unittest https://codereview.chromium.org/2471573005/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/tab_capture/offscreen_tab.cc (right): https://codereview.chromium.org/2471573005/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/tab_capture/offscreen_tab.cc:122: DVLOG(1) << " Register with ReceiverPresentationServiceDelegateImpl, id=" Nit: I've seen [id] used in other log messages. Can you check other files and use a consistent format. It might be helpful to label these [presentation_id] to be more specific. https://codereview.chromium.org/2471573005/diff/160001/chrome/browser/media/r... File chrome/browser/media/router/browser_presentation_connection_proxy.cc (right): https://codereview.chromium.org/2471573005/diff/160001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.cc:13: std::unique_ptr<content::PresentationConnectionMessage> This will go away once we finish typemaps for presentation.mojom. Can you add a TODO(crbug.com/632623) in case this lands in advance? https://codereview.chromium.org/2471573005/diff/160001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.cc:37: if (!input->data || input->message || It's really unfortunate that we have to duplicate the validation logic here and in PresentationServiceImpl. https://codereview.chromium.org/2471573005/diff/160001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.cc:69: DVLOG(2) << __FUNCTION__; Are these function-only log statements needed permanently (or only for temporary debugging)? https://codereview.chromium.org/2471573005/diff/160001/chrome/browser/media/r... File chrome/browser/media/router/browser_presentation_connection_proxy.h (right): https://codereview.chromium.org/2471573005/diff/160001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2017 here and elsewhere in new files :) https://codereview.chromium.org/2471573005/diff/160001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.h:38: // instance of this class will also be destroyed. How does the lifetime of this class relate to the lifetime of the media route? The route could be terminated while the frame is still alive. https://codereview.chromium.org/2471573005/diff/160001/chrome/browser/media/r... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/2471573005/diff/160001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:171: content::PresentationConnectionPtr source_conn, source_connection https://codereview.chromium.org/2471573005/diff/160001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:172: content::PresentationConnectionRequest target_conn_request); target_connection_request https://codereview.chromium.org/2471573005/diff/160001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:387: browser_connection_proxies_.insert(std::move(proxy)); Where are entries removed from browser_connection_proxies_? https://codereview.chromium.org/2471573005/diff/160001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:552: if (it != presentation_frames_.end()) If there is no target presentation frame for the connection, what ensures the connections are cleaned up? https://codereview.chromium.org/2471573005/diff/160001/content/renderer/prese... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/2471573005/diff/160001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:229: static_cast<const PresentationConnectionProxy*>(request->connection_proxy) Is the static_cast necessary? I don't see where request->connection_proxy is declared. https://codereview.chromium.org/2471573005/diff/160001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:410: auto* controller_connection_proxy = This block of code is duplicated with L437-442. Please factor out a common method. https://codereview.chromium.org/2471573005/diff/160001/content/renderer/prese... File content/renderer/presentation/presentation_dispatcher_unittest.cc (right): https://codereview.chromium.org/2471573005/diff/160001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:264: TEST_F(PresentationDispatcherTest, TestStartSession) { Are there expectations covering that the proxies are bound to connections on StartSession and onDefaultSessionStarted? https://codereview.chromium.org/2471573005/diff/160001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:387: TEST_F(PresentationDispatcherTest, TestSendBlobData) { Does there need to be a test case added for OnReceiverConnectionAvailable (proxies are created and bound to connections)?
https://codereview.chromium.org/2471573005/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/tab_capture/offscreen_tab.cc (right): https://codereview.chromium.org/2471573005/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/tab_capture/offscreen_tab.cc:122: DVLOG(1) << " Register with ReceiverPresentationServiceDelegateImpl, id=" On 2017/01/23 22:29:34, mark a. foltz wrote: > Nit: I've seen [id] used in other log messages. Can you check other files and > use a consistent format. > > It might be helpful to label these [presentation_id] to be more specific. Done. https://codereview.chromium.org/2471573005/diff/160001/chrome/browser/media/r... File chrome/browser/media/router/browser_presentation_connection_proxy.cc (right): https://codereview.chromium.org/2471573005/diff/160001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.cc:13: std::unique_ptr<content::PresentationConnectionMessage> On 2017/01/23 22:29:34, mark a. foltz wrote: > This will go away once we finish typemaps for presentation.mojom. > Can you add a TODO(crbug.com/632623) in case this lands in advance? Done. https://codereview.chromium.org/2471573005/diff/160001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.cc:69: DVLOG(2) << __FUNCTION__; On 2017/01/23 22:29:34, mark a. foltz wrote: > Are these function-only log statements needed permanently (or only for temporary > debugging)? Just for debugging. Removed. https://codereview.chromium.org/2471573005/diff/160001/chrome/browser/media/r... File chrome/browser/media/router/browser_presentation_connection_proxy.h (right): https://codereview.chromium.org/2471573005/diff/160001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/01/23 22:29:34, mark a. foltz wrote: > 2017 here and elsewhere in new files :) Done. https://codereview.chromium.org/2471573005/diff/160001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.h:38: // instance of this class will also be destroyed. On 2017/01/23 22:29:34, mark a. foltz wrote: > How does the lifetime of this class relate to the lifetime of the media route? > The route could be terminated while the frame is still alive. Object will be destroyed if |route_| is closed or terminated. https://codereview.chromium.org/2471573005/diff/160001/chrome/browser/media/r... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/2471573005/diff/160001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:171: content::PresentationConnectionPtr source_conn, On 2017/01/23 22:29:34, mark a. foltz wrote: > source_connection Done. https://codereview.chromium.org/2471573005/diff/160001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:172: content::PresentationConnectionRequest target_conn_request); On 2017/01/23 22:29:34, mark a. foltz wrote: > target_connection_request Done. https://codereview.chromium.org/2471573005/diff/160001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:387: browser_connection_proxies_.insert(std::move(proxy)); On 2017/01/23 22:29:34, mark a. foltz wrote: > Where are entries removed from browser_connection_proxies_? Should be removed in PresentationFrame::RemoveConnection() and PresentationFrame::Reset(). https://codereview.chromium.org/2471573005/diff/160001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:552: if (it != presentation_frames_.end()) On 2017/01/23 22:29:34, mark a. foltz wrote: > If there is no target presentation frame for the connection, what ensures the > connections are cleaned up? Done. https://codereview.chromium.org/2471573005/diff/160001/content/renderer/prese... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/2471573005/diff/160001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:229: static_cast<const PresentationConnectionProxy*>(request->connection_proxy) On 2017/01/23 22:29:34, mark a. foltz wrote: > Is the static_cast necessary? I don't see where request->connection_proxy is > declared. It is declared in 4th patch as WebPresentationConnection*. https://codereview.chromium.org/2471263003/diff/280001/content/renderer/prese... https://codereview.chromium.org/2471573005/diff/160001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:410: auto* controller_connection_proxy = On 2017/01/23 22:29:34, mark a. foltz wrote: > This block of code is duplicated with L437-442. Please factor out a common > method. Done. https://codereview.chromium.org/2471573005/diff/160001/content/renderer/prese... File content/renderer/presentation/presentation_dispatcher_unittest.cc (right): https://codereview.chromium.org/2471573005/diff/160001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:264: TEST_F(PresentationDispatcherTest, TestStartSession) { On 2017/01/23 22:29:34, mark a. foltz wrote: > Are there expectations covering that the proxies are bound to connections on > StartSession and onDefaultSessionStarted? Done. https://codereview.chromium.org/2471573005/diff/160001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:387: TEST_F(PresentationDispatcherTest, TestSendBlobData) { On 2017/01/23 22:29:34, mark a. foltz wrote: > Does there need to be a test case added for OnReceiverConnectionAvailable > (proxies are created and bound to connections)? Done.
Overall looks good, a few comments. I didn't take a close look at the tests yet. https://codereview.chromium.org/2471573005/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/browser_presentation_connection_proxy.cc (right): https://codereview.chromium.org/2471573005/diff/200001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.cc:16: GetPresentationConnectionMessage(blink::mojom::ConnectionMessagePtr input) { PresentationConnectionMessageFromMojo https://codereview.chromium.org/2471573005/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/browser_presentation_connection_proxy.h (right): https://codereview.chromium.org/2471573005/diff/200001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.h:21: // Message received on this class is further routed to media router. State of nit: Media Router https://codereview.chromium.org/2471573005/diff/200001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.h:28: // Send message from page to media router: s/page/render frame/ https://codereview.chromium.org/2471573005/diff/200001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.h:38: // |route_| is closed or terminated, instance of this class will be destroyed. What if the PresentationConnection is closed? https://codereview.chromium.org/2471573005/diff/200001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.h:63: MediaRoute* route_; Can this be const? https://codereview.chromium.org/2471573005/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/2471573005/diff/200001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:171: content::PresentationConnectionPtr source_connection, I think you'll have to revert this to use the blink::mojom namespace since I won't be moving the mojom to content/. Really sorry for the confusion :-/ https://codereview.chromium.org/2471573005/diff/200001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:341: DVLOG(2) << "ListenForSessionMessages: do not listen for offscreen " Do we expect this to happen at all, or is this a bug? If a bug, I would turn this into a DCHECK(). https://codereview.chromium.org/2471573005/diff/200001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:370: DCHECK(base::ContainsKey(presentation_id_to_route_, presentation_id)); Where is presentation_id being passed from? If it is from the renderer then this should return false and not DCHECK. https://codereview.chromium.org/2471573005/diff/200001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:380: DCHECK(base::ContainsKey(presentation_id_to_route_, session.presentation_id)); Similar comment about presentation_id. https://codereview.chromium.org/2471573005/diff/200001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:381: auto it = presentation_id_to_route_.find(session.presentation_id); Can this be const auto?
https://codereview.chromium.org/2471573005/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/browser_presentation_connection_proxy.cc (right): https://codereview.chromium.org/2471573005/diff/200001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.cc:16: GetPresentationConnectionMessage(blink::mojom::ConnectionMessagePtr input) { On 2017/01/27 00:44:21, mark a. foltz wrote: > PresentationConnectionMessageFromMojo Done. https://codereview.chromium.org/2471573005/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/browser_presentation_connection_proxy.h (right): https://codereview.chromium.org/2471573005/diff/200001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.h:21: // Message received on this class is further routed to media router. State of On 2017/01/27 00:44:21, mark a. foltz wrote: > nit: Media Router Done. https://codereview.chromium.org/2471573005/diff/200001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.h:28: // Send message from page to media router: On 2017/01/27 00:44:21, mark a. foltz wrote: > s/page/render frame/ Done. https://codereview.chromium.org/2471573005/diff/200001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.h:38: // |route_| is closed or terminated, instance of this class will be destroyed. On 2017/01/27 00:44:21, mark a. foltz wrote: > What if the PresentationConnection is closed? If controller connection.close(), PSDImpl will detach route and destory BrowserConnectionProxy; It is similar as |route_| closed? Connection's target_connection_ will be corrupted at this point. We are not using target_connection_ beyond this point though. We may defer handle this neatly when implementing reconnect()? https://codereview.chromium.org/2471573005/diff/200001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.h:63: MediaRoute* route_; On 2017/01/27 00:44:21, mark a. foltz wrote: > Can this be const? Done. https://codereview.chromium.org/2471573005/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/2471573005/diff/200001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:171: content::PresentationConnectionPtr source_connection, On 2017/01/27 00:44:21, mark a. foltz wrote: > I think you'll have to revert this to use the blink::mojom namespace since I > won't be moving the mojom to content/. Really sorry for the confusion :-/ Have a using in presentation_service_delegate.h :) using PresentationConnectionPtr = blink::mojom::PresentationConnectionPtr; https://codereview.chromium.org/2471573005/diff/200001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:341: DVLOG(2) << "ListenForSessionMessages: do not listen for offscreen " On 2017/01/27 00:44:21, mark a. foltz wrote: > Do we expect this to happen at all, or is this a bug? If a bug, I would turn > this into a DCHECK(). Yes, it is a normal code path (log for debugging). We do not have is_offscreen flag in PresentationDispatcher::onSessionCreated() so we just call ListenForConnectionMessages() for both 1-ua and 2-ua. When it goes down here, we can decide if 1-ua do nothing; if 2-ua listen to message. https://codereview.chromium.org/2471573005/diff/200001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:370: DCHECK(base::ContainsKey(presentation_id_to_route_, presentation_id)); On 2017/01/27 00:44:21, mark a. foltz wrote: > Where is presentation_id being passed from? If it is from the renderer then > this should return false and not DCHECK. Done. https://codereview.chromium.org/2471573005/diff/200001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:380: DCHECK(base::ContainsKey(presentation_id_to_route_, session.presentation_id)); On 2017/01/27 00:44:21, mark a. foltz wrote: > Similar comment about presentation_id. Done. https://codereview.chromium.org/2471573005/diff/200001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:381: auto it = presentation_id_to_route_.find(session.presentation_id); On 2017/01/27 00:44:21, mark a. foltz wrote: > Can this be const auto? Done.
A few more comments, mostly minor style issues. Tests look good. Can you write a small unit test for browser_presentation_connection_proxy? https://codereview.chromium.org/2471573005/diff/220001/chrome/browser/media/r... File chrome/browser/media/router/browser_presentation_connection_proxy.cc (right): https://codereview.chromium.org/2471573005/diff/220001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.cc:71: blink::mojom::PresentationConnectionRequest target_conn_request) { target_request (prefer not using abbreviations in parameter names) https://codereview.chromium.org/2471573005/diff/220001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.cc:85: blink::mojom::ConnectionMessagePtr session_message, The parameter names here don't match the declaration. https://codereview.chromium.org/2471573005/diff/220001/chrome/browser/media/r... File chrome/browser/media/router/browser_presentation_connection_proxy.h (right): https://codereview.chromium.org/2471573005/diff/220001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.h:48: void Bind(blink::mojom::PresentationConnectionRequest); Outside of third_party/WebKit we the Media Router code has always been putting parameter names (even when obvious). Omitting parameter names is allowed by the style guide and I'm fine with it. But I think it should be done consistently throughout a class definition. So if you want to do it here, suggest omitting |router|, |route|, |state| and |callback| in this header as well. https://codereview.chromium.org/2471573005/diff/220001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.h:58: content::PresentationSessionInfo session_info_; The PresentationSessionInfo passed here only seems to be used in a debug logging statement. Is it necessary for this class? If you need to keep it, then please declare as const. https://codereview.chromium.org/2471573005/diff/220001/chrome/browser/media/r... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/2471573005/diff/220001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:375: auto it = presentation_id_to_route_.find(presentation_id); Nit: const auto https://codereview.chromium.org/2471573005/diff/220001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:397: auto route_id = it->second.media_route_id(); Can this be inlined below? https://codereview.chromium.org/2471573005/diff/220001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:564: return presentation_frame->OnBrowserConnectionAvailable( This is returning a value but the function is declared void. https://codereview.chromium.org/2471573005/diff/220001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:1019: frame_manager_->OnBrowserConnectionAvailable( I feel like we need to handle a presentation_id that does not match a valid presentation. I guess this function is a no-op in that case, and we log at L371/385. But IMO it would be clearer to check presentation_id_to_route_ before handing off to the offscreen/remote code. It's possible a route is terminated before this function is called, so I don't think we can distinguish a misbehaving renderer from a late call to this function. WDYT Derek?
https://codereview.chromium.org/2471573005/diff/220001/chrome/browser/media/r... File chrome/browser/media/router/browser_presentation_connection_proxy.h (right): https://codereview.chromium.org/2471573005/diff/220001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.h:48: void Bind(blink::mojom::PresentationConnectionRequest); On 2017/01/27 at 18:30:05, mark a. foltz wrote: > Outside of third_party/WebKit we the Media Router code has always been putting parameter names (even when obvious). > > Omitting parameter names is allowed by the style guide and I'm fine with it. But I think it should be done consistently throughout a class definition. So if you want to do it here, suggest omitting |router|, |route|, |state| and |callback| in this header as well. Slight correction. Parameter names can be omitted if they are unused. Then they should be commented out instead. Here's the full style guide entry. https://google.github.io/styleguide/cppguide.html#Function_Declarations_and_D...
https://codereview.chromium.org/2471573005/diff/220001/chrome/browser/media/r... File chrome/browser/media/router/browser_presentation_connection_proxy.cc (right): https://codereview.chromium.org/2471573005/diff/220001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.cc:71: blink::mojom::PresentationConnectionRequest target_conn_request) { On 2017/01/27 18:30:05, mark a. foltz wrote: > target_request (prefer not using abbreviations in parameter names) Done. https://codereview.chromium.org/2471573005/diff/220001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.cc:85: blink::mojom::ConnectionMessagePtr session_message, On 2017/01/27 18:30:05, mark a. foltz wrote: > The parameter names here don't match the declaration. Done. https://codereview.chromium.org/2471573005/diff/220001/chrome/browser/media/r... File chrome/browser/media/router/browser_presentation_connection_proxy.h (right): https://codereview.chromium.org/2471573005/diff/220001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.h:48: void Bind(blink::mojom::PresentationConnectionRequest); On 2017/01/27 18:30:05, mark a. foltz wrote: > Outside of third_party/WebKit we the Media Router code has always been putting > parameter names (even when obvious). > > Omitting parameter names is allowed by the style guide and I'm fine with it. But > I think it should be done consistently throughout a class definition. So if you > want to do it here, suggest omitting |router|, |route|, |state| and |callback| > in this header as well. Adding them back. https://codereview.chromium.org/2471573005/diff/220001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.h:48: void Bind(blink::mojom::PresentationConnectionRequest); On 2017/01/27 18:52:01, mark a. foltz wrote: > On 2017/01/27 at 18:30:05, mark a. foltz wrote: > > Outside of third_party/WebKit we the Media Router code has always been putting > parameter names (even when obvious). > > > > Omitting parameter names is allowed by the style guide and I'm fine with it. > But I think it should be done consistently throughout a class definition. So if > you want to do it here, suggest omitting |router|, |route|, |state| and > |callback| in this header as well. > > Slight correction. Parameter names can be omitted if they are unused. Then > they should be commented out instead. > > Here's the full style guide entry. > > https://google.github.io/styleguide/cppguide.html#Function_Declarations_and_D... Acknowledged. https://codereview.chromium.org/2471573005/diff/220001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.h:58: content::PresentationSessionInfo session_info_; On 2017/01/27 18:30:05, mark a. foltz wrote: > The PresentationSessionInfo passed here only seems to be used in a debug logging > statement. Is it necessary for this class? > > If you need to keep it, then please declare as const. Removed. https://codereview.chromium.org/2471573005/diff/220001/chrome/browser/media/r... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/2471573005/diff/220001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:375: auto it = presentation_id_to_route_.find(presentation_id); On 2017/01/27 18:30:06, mark a. foltz wrote: > Nit: const auto Done. https://codereview.chromium.org/2471573005/diff/220001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:397: auto route_id = it->second.media_route_id(); On 2017/01/27 18:30:05, mark a. foltz wrote: > Can this be inlined below? Done. https://codereview.chromium.org/2471573005/diff/220001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:564: return presentation_frame->OnBrowserConnectionAvailable( On 2017/01/27 18:30:06, mark a. foltz wrote: > This is returning a value but the function is declared void. Done.
Patchset #11 (id:260001) has been deleted
https://codereview.chromium.org/2471573005/diff/240001/chrome/browser/media/r... File chrome/browser/media/router/browser_presentation_connection_proxy.cc (right): https://codereview.chromium.org/2471573005/diff/240001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.cc:89: if (!router_) { Why would this be nullptr? Maybe there should be a CHECK in ctor? https://codereview.chromium.org/2471573005/diff/240001/chrome/browser/media/r... File chrome/browser/media/router/browser_presentation_connection_proxy.h (right): https://codereview.chromium.org/2471573005/diff/240001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.h:48: void Bind( Maybe name this BindToRequest or something similar? I assume this can only be called once? Would it be possible to supply the interface request and the connection ptr in the same call? See comment in PSDImpl https://codereview.chromium.org/2471573005/diff/240001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.h:60: blink::mojom::PresentationConnectionState state) override {} Add a TODO to implement this, or a comment why this is empty. https://codereview.chromium.org/2471573005/diff/240001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.h:64: MediaRouter* router_; MediaRouter* const https://codereview.chromium.org/2471573005/diff/240001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.h:66: const MediaRoute* route_; Who owns the MediaRoute? Since it is just a data object, why not just make a copy here? Or maybe just MediaRoute::Id? https://codereview.chromium.org/2471573005/diff/240001/chrome/browser/media/r... File chrome/browser/media/router/browser_presentation_connection_proxy_unittest.cc (right): https://codereview.chromium.org/2471573005/diff/240001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy_unittest.cc:18: using OnMessageCallback = BrowserPresentationConnectionProxy::OnMessageCallback; or just: using BrowserPresentationConnectionProxy::OnMessageCallback; https://codereview.chromium.org/2471573005/diff/240001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy_unittest.cc:20: class MockOnMessageCallback { You can replace this using MockCallback: https://cs.chromium.org/chromium/src/base/test/mock_callback.h?q=mockcallback... https://codereview.chromium.org/2471573005/diff/240001/chrome/browser/media/r... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/2471573005/diff/240001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:168: bool IsOffscreenPresentation(const std::string& presentation_id); Can this be const? https://codereview.chromium.org/2471573005/diff/240001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:280: offscreen_presentation_manager->UnregisterOffscreenPresentationController( Should this first check if the route is an offscreen presentation? https://codereview.chromium.org/2471573005/diff/240001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:370: if (!base::ContainsKey(presentation_id_to_route_, presentation_id)) { Move the find() call here to avoid double lookup. https://codereview.chromium.org/2471573005/diff/240001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:397: proxy->Bind(std::move(target_connection_request)); You can probably combine these two calls to proxy into an Init() method? Or even into the constructor? https://codereview.chromium.org/2471573005/diff/240001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:463: const std::string& presentation_id); const https://codereview.chromium.org/2471573005/diff/240001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:557: return false; nit: insert empty line above https://codereview.chromium.org/2471573005/diff/240001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:1011: if (frame_manager_->IsOffscreenPresentation(render_frame_host_id, It seems a bit odd that we are checking for offscreen presentation at the PSDImpl level, but then doing the same for ListenForSessionMessages at the PresentationFrame level. Can the logic be moved to PresentationFrame to be consistent? With the state transition to "connected" happening in Blink, I wonder if there's some room for simplifying the setup sequence from PresentationDispatcher to here. It looks like we can combine ListenForConnectionMessages with ConnectToPresentation. ====== Chatted offline after writing the message above. Bin tried to do this but it seems we still have some timing issues with state change vs. message arriving from extension due to them using different mojo message pipes to talk to renderer. We can fix this by having messages from extension go through PresentationConnectionProxy, but this will probably make sense in a separate patch. https://codereview.chromium.org/2471573005/diff/240001/chrome/browser/media/r... File chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc (right): https://codereview.chromium.org/2471573005/diff/240001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc:105: class MockPresentationSessionStartedCallback { Use MockCallback https://codereview.chromium.org/2471573005/diff/240001/content/renderer/prese... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/2471573005/diff/240001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:228: static_cast<const PresentationConnectionProxy*>(request->connection_proxy) Hmm... it seems a bit odd that we have to do this static_cast here. Chatted with you offline, this is something we can remove when we implement per-connection message queueing. So it's fine for now but please add TODO if you haven't already. https://codereview.chromium.org/2471573005/diff/240001/content/renderer/prese... File content/renderer/presentation/test_presentation_connection.h (right): https://codereview.chromium.org/2471573005/diff/240001/content/renderer/prese... content/renderer/presentation/test_presentation_connection.h:28: std::unique_ptr<blink::WebPresentationConnectionProxy> proxy_; This should be private. Add a proxy() getter method instead.
https://codereview.chromium.org/2471573005/diff/240001/chrome/browser/media/r... File chrome/browser/media/router/browser_presentation_connection_proxy.cc (right): https://codereview.chromium.org/2471573005/diff/240001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.cc:89: if (!router_) { On 2017/01/31 01:53:24, imcheng wrote: > Why would this be nullptr? Maybe there should be a CHECK in ctor? Done. https://codereview.chromium.org/2471573005/diff/240001/chrome/browser/media/r... File chrome/browser/media/router/browser_presentation_connection_proxy.h (right): https://codereview.chromium.org/2471573005/diff/240001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.h:48: void Bind( On 2017/01/31 01:53:24, imcheng wrote: > Maybe name this BindToRequest or something similar? I assume this can only be > called once? > > Would it be possible to supply the interface request and the connection ptr in > the same call? See comment in PSDImpl Done. https://codereview.chromium.org/2471573005/diff/240001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.h:60: blink::mojom::PresentationConnectionState state) override {} On 2017/01/31 01:53:24, imcheng wrote: > Add a TODO to implement this, or a comment why this is empty. Done. https://codereview.chromium.org/2471573005/diff/240001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.h:64: MediaRouter* router_; On 2017/01/31 01:53:25, imcheng wrote: > MediaRouter* const Done. https://codereview.chromium.org/2471573005/diff/240001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy.h:66: const MediaRoute* route_; On 2017/01/31 01:53:25, imcheng wrote: > Who owns the MediaRoute? Since it is just a data object, why not just make a > copy here? Or maybe just MediaRoute::Id? Done. https://codereview.chromium.org/2471573005/diff/240001/chrome/browser/media/r... File chrome/browser/media/router/browser_presentation_connection_proxy_unittest.cc (right): https://codereview.chromium.org/2471573005/diff/240001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy_unittest.cc:18: using OnMessageCallback = BrowserPresentationConnectionProxy::OnMessageCallback; On 2017/01/31 01:53:25, imcheng wrote: > or just: > > using BrowserPresentationConnectionProxy::OnMessageCallback; Done. https://codereview.chromium.org/2471573005/diff/240001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy_unittest.cc:20: class MockOnMessageCallback { On 2017/01/31 01:53:25, imcheng wrote: > You can replace this using MockCallback: > https://cs.chromium.org/chromium/src/base/test/mock_callback.h?q=mockcallback... Done. https://codereview.chromium.org/2471573005/diff/240001/chrome/browser/media/r... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/2471573005/diff/240001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:168: bool IsOffscreenPresentation(const std::string& presentation_id); On 2017/01/31 01:53:25, imcheng wrote: > Can this be const? Done. https://codereview.chromium.org/2471573005/diff/240001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:280: offscreen_presentation_manager->UnregisterOffscreenPresentationController( On 2017/01/31 01:53:25, imcheng wrote: > Should this first check if the route is an offscreen presentation? Done. https://codereview.chromium.org/2471573005/diff/240001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:370: if (!base::ContainsKey(presentation_id_to_route_, presentation_id)) { On 2017/01/31 01:53:25, imcheng wrote: > Move the find() call here to avoid double lookup. Done. https://codereview.chromium.org/2471573005/diff/240001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:397: proxy->Bind(std::move(target_connection_request)); On 2017/01/31 01:53:25, imcheng wrote: > You can probably combine these two calls to proxy into an Init() method? Or even > into the constructor? Done. https://codereview.chromium.org/2471573005/diff/240001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:463: const std::string& presentation_id); On 2017/01/31 01:53:25, imcheng wrote: > const Done. https://codereview.chromium.org/2471573005/diff/240001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:557: return false; On 2017/01/31 01:53:25, imcheng wrote: > nit: insert empty line above Done. https://codereview.chromium.org/2471573005/diff/240001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:1011: if (frame_manager_->IsOffscreenPresentation(render_frame_host_id, On 2017/01/31 01:53:25, imcheng wrote: > It seems a bit odd that we are checking for offscreen presentation at the > PSDImpl level, but then doing the same for ListenForSessionMessages at the > PresentationFrame level. Can the logic be moved to PresentationFrame to be > consistent? > > With the state transition to "connected" happening in Blink, I wonder if there's > some room for simplifying the setup sequence from PresentationDispatcher to > here. It looks like we can combine ListenForConnectionMessages with > ConnectToPresentation. > > ====== > > Chatted offline after writing the message above. Bin tried to do this but it > seems we still have some timing issues with state change vs. message arriving > from extension due to them using different mojo message pipes to talk to > renderer. We can fix this by having messages from extension go through > PresentationConnectionProxy, but this will probably make sense in a separate > patch. Moved it into PresentationFrame. Removing ListenForSessionMessages is tracked by crbug.com/687011. https://codereview.chromium.org/2471573005/diff/240001/chrome/browser/media/r... File chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc (right): https://codereview.chromium.org/2471573005/diff/240001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc:105: class MockPresentationSessionStartedCallback { On 2017/01/31 01:53:25, imcheng wrote: > Use MockCallback Done. https://codereview.chromium.org/2471573005/diff/240001/content/renderer/prese... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/2471573005/diff/240001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:228: static_cast<const PresentationConnectionProxy*>(request->connection_proxy) On 2017/01/31 01:53:25, imcheng wrote: > Hmm... it seems a bit odd that we have to do this static_cast here. Chatted with > you offline, this is something we can remove when we implement per-connection > message queueing. So it's fine for now but please add TODO if you haven't > already. Done. https://codereview.chromium.org/2471573005/diff/240001/content/renderer/prese... File content/renderer/presentation/test_presentation_connection.h (right): https://codereview.chromium.org/2471573005/diff/240001/content/renderer/prese... content/renderer/presentation/test_presentation_connection.h:28: std::unique_ptr<blink::WebPresentationConnectionProxy> proxy_; On 2017/01/31 01:53:25, imcheng wrote: > This should be private. Add a proxy() getter method instead. Done.
lgtm https://codereview.chromium.org/2471573005/diff/280001/chrome/browser/media/r... File chrome/browser/media/router/browser_presentation_connection_proxy_unittest.cc (right): https://codereview.chromium.org/2471573005/diff/280001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy_unittest.cc:21: class MockOnMessageCallback { This can be removed now https://codereview.chromium.org/2471573005/diff/280001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy_unittest.cc:93: base::MockCallback<base::Callback<void(bool success)>> sucess param not needed. https://codereview.chromium.org/2471573005/diff/280001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy_unittest.cc:111: base::MockCallback<base::Callback<void(bool success)>> sucess param not needed.
zhaobin@chromium.org changed reviewers: + jam@chromium.org, miu@chromium.org
+miu@ to review chrome/browser/extensions/api/tab_capture/offscreen_tab.cc +jam@ to review content/public/browser/presentation_service_delegate.h https://codereview.chromium.org/2471573005/diff/280001/chrome/browser/media/r... File chrome/browser/media/router/browser_presentation_connection_proxy_unittest.cc (right): https://codereview.chromium.org/2471573005/diff/280001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy_unittest.cc:21: class MockOnMessageCallback { On 2017/01/31 22:22:48, imcheng wrote: > This can be removed now Done. https://codereview.chromium.org/2471573005/diff/280001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy_unittest.cc:93: base::MockCallback<base::Callback<void(bool success)>> On 2017/01/31 22:22:48, imcheng wrote: > sucess param not needed. Done. https://codereview.chromium.org/2471573005/diff/280001/chrome/browser/media/r... chrome/browser/media/router/browser_presentation_connection_proxy_unittest.cc:111: base::MockCallback<base::Callback<void(bool success)>> On 2017/01/31 22:22:48, imcheng wrote: > sucess param not needed. Done.
lgtm
lgtm % a couple small things and one large thing to consider: https://codereview.chromium.org/2471573005/diff/300001/chrome/browser/extensi... File chrome/browser/extensions/api/tab_capture/offscreen_tab.cc (left): https://codereview.chromium.org/2471573005/diff/300001/chrome/browser/extensi... chrome/browser/extensions/api/tab_capture/offscreen_tab.cc:121: // the PresentationRouter. http://crbug.com/513859 Please include 513859 on the BUG= line in this change description. https://codereview.chromium.org/2471573005/diff/300001/chrome/browser/extensi... File chrome/browser/extensions/api/tab_capture/offscreen_tab.cc (right): https://codereview.chromium.org/2471573005/diff/300001/chrome/browser/extensi... chrome/browser/extensions/api/tab_capture/offscreen_tab.cc:124: // Register the offscreen tab as the receiver of the offscreen presentation. I think a more-accurate comment here would be something like: Create a ReceiverPSDImpl associated with the offscreen tab's WebContents. The new instance will set up the necessary infrastructure to allow controlling peers the ability to connect to the offscreen tab. https://codereview.chromium.org/2471573005/diff/300001/chrome/browser/extensi... chrome/browser/extensions/api/tab_capture/offscreen_tab.cc:125: media_router::ReceiverPresentationServiceDelegateImpl::CreateForWebContents( I dove into the call points from here, and I wonder if there's a potential bug in assumptions made about the Profiles (BrowserContexts): 1. The offscreen tab will be created from the tab capture extension API. The extension that launches the tab will be using whatever Profile the MR Extension was loaded under. 2. The offscreen tab will, of course, create its own incognito Profile based on the extension's Profile. 3. Code downstream of here will then create an OffscreenPresentationManagerFactory using the BrowserContextKeyedServiceFactory. IIUC, the BCKSF will do a backwards-lookup from the offscreen tab's incognito Profile to get/create a factory that will be associated with the Profile of the MR Extension. 4. Therefore, will pages in other Profiles that want to start 1UA presentations be unable to access the OffscreenPresentationManagerFactory to connect to the offscreen tab? I suggest you make sure to test using the presentation API 1UA functionality in multiple-profile scenarios (e.g., browser started-up on one profile, but you can switch to another profile and start a controlling page that starts a presentation). A browsertest might be appropriate here.
Description was changed from ========== [Presentation API] (5th)(1-UA) integrate controller and receiver side for 1-UA messaging This CL actually turns on 1-UA messaging: - When offscreen presentation starts, a new route is created and returned to MR. In PresentationDispatcher, we register a controller_connection_proxy to OPM. If there has been a receiver_call_back, invoke it. - When offscreen tab is rendered, we register a receiverPSDImpl, which registers a receiver_call_back to OPM. If there is any pending controller proxy, invoke receiver_call_back for each controller_proxy. - In receiver_call_back (PresentationDispatcher::OnReceiverConnectionAvailable() function), we set up a connection between controller_connection_proxy and receiver_connection_proxy. Message sending may happen afterwards. More details in code comments and design doc (use chromium.org account): https://docs.google.com/document/d/1XM3jhMJTQyhEC5PDAAJFNIaKh6UUEihqZDz_ztEe4... Patch set 4 updates: - added BrowserPresentationConnectionProxy class - Added PresentationFrame::IsOffscreen() and PresentationFrame::OnBrowserConnectionAvailable() - PresentationConnection::OnMessage() takes a callback - PresentationDispatcher::SendXXX() takes PresentationConnectionProxy as parameter BUG=525660 ========== to ========== [Presentation API] (5th)(1-UA) integrate controller and receiver side for 1-UA messaging This CL actually turns on 1-UA messaging: - When offscreen presentation starts, a new route is created and returned to MR. In PresentationDispatcher, we register a controller_connection_proxy to OPM. If there has been a receiver_call_back, invoke it. - When offscreen tab is rendered, we register a receiverPSDImpl, which registers a receiver_call_back to OPM. If there is any pending controller proxy, invoke receiver_call_back for each controller_proxy. - In receiver_call_back (PresentationDispatcher::OnReceiverConnectionAvailable() function), we set up a connection between controller_connection_proxy and receiver_connection_proxy. Message sending may happen afterwards. More details in code comments and design doc (use chromium.org account): https://docs.google.com/document/d/1XM3jhMJTQyhEC5PDAAJFNIaKh6UUEihqZDz_ztEe4... Patch set 4 updates: - added BrowserPresentationConnectionProxy class - Added PresentationFrame::IsOffscreen() and PresentationFrame::OnBrowserConnectionAvailable() - PresentationConnection::OnMessage() takes a callback - PresentationDispatcher::SendXXX() takes PresentationConnectionProxy as parameter BUG=525660,513859 ==========
The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run
Patchset #13 (id:320001) has been deleted
https://codereview.chromium.org/2471573005/diff/300001/chrome/browser/extensi... File chrome/browser/extensions/api/tab_capture/offscreen_tab.cc (left): https://codereview.chromium.org/2471573005/diff/300001/chrome/browser/extensi... chrome/browser/extensions/api/tab_capture/offscreen_tab.cc:121: // the PresentationRouter. http://crbug.com/513859 On 2017/02/02 21:17:34, miu wrote: > Please include 513859 on the BUG= line in this change description. Done. https://codereview.chromium.org/2471573005/diff/300001/chrome/browser/extensi... File chrome/browser/extensions/api/tab_capture/offscreen_tab.cc (right): https://codereview.chromium.org/2471573005/diff/300001/chrome/browser/extensi... chrome/browser/extensions/api/tab_capture/offscreen_tab.cc:124: // Register the offscreen tab as the receiver of the offscreen presentation. On 2017/02/02 21:17:34, miu wrote: > I think a more-accurate comment here would be something like: > > Create a ReceiverPSDImpl associated with the offscreen tab's WebContents. The > new instance will set up the necessary infrastructure to allow controlling peers > the ability to connect to the offscreen tab. Done. https://codereview.chromium.org/2471573005/diff/300001/chrome/browser/extensi... chrome/browser/extensions/api/tab_capture/offscreen_tab.cc:125: media_router::ReceiverPresentationServiceDelegateImpl::CreateForWebContents( On 2017/02/02 21:17:35, miu wrote: > I dove into the call points from here, and I wonder if there's a potential bug > in assumptions made about the Profiles (BrowserContexts): > > 1. The offscreen tab will be created from the tab capture extension API. The > extension that launches the tab will be using whatever Profile the MR Extension > was loaded under. > > 2. The offscreen tab will, of course, create its own incognito Profile based on > the extension's Profile. > > 3. Code downstream of here will then create an > OffscreenPresentationManagerFactory using the BrowserContextKeyedServiceFactory. > IIUC, the BCKSF will do a backwards-lookup from the offscreen tab's incognito > Profile to get/create a factory that will be associated with the Profile of the > MR Extension. > > 4. Therefore, will pages in other Profiles that want to start 1UA presentations > be unable to access the OffscreenPresentationManagerFactory to connect to the > offscreen tab? > > I suggest you make sure to test using the presentation API 1UA functionality in > multiple-profile scenarios (e.g., browser started-up on one profile, but you can > switch to another profile and start a controlling page that starts a > presentation). A browsertest might be appropriate here. reconnect() for 1-ua has not been implemented yet. Created crbug.com/688233 to track this issue.
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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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 Nice work :) A couple of minor changes will be needed to presentation_dispatcher to rebase with the typemaps patch that just landed. https://codereview.chromium.org/2471573005/diff/380001/content/renderer/prese... File content/renderer/presentation/presentation_dispatcher.h (right): https://codereview.chromium.org/2471573005/diff/380001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.h:189: blink::mojom::PresentationSessionInfoPtr session_info, content::PresentationSessionInfo& when merging with typemap patch
Just a reminder, review the patch description and make sure it fits the latest patchset before landing. Thanks!
Description was changed from ========== [Presentation API] (5th)(1-UA) integrate controller and receiver side for 1-UA messaging This CL actually turns on 1-UA messaging: - When offscreen presentation starts, a new route is created and returned to MR. In PresentationDispatcher, we register a controller_connection_proxy to OPM. If there has been a receiver_call_back, invoke it. - When offscreen tab is rendered, we register a receiverPSDImpl, which registers a receiver_call_back to OPM. If there is any pending controller proxy, invoke receiver_call_back for each controller_proxy. - In receiver_call_back (PresentationDispatcher::OnReceiverConnectionAvailable() function), we set up a connection between controller_connection_proxy and receiver_connection_proxy. Message sending may happen afterwards. More details in code comments and design doc (use chromium.org account): https://docs.google.com/document/d/1XM3jhMJTQyhEC5PDAAJFNIaKh6UUEihqZDz_ztEe4... Patch set 4 updates: - added BrowserPresentationConnectionProxy class - Added PresentationFrame::IsOffscreen() and PresentationFrame::OnBrowserConnectionAvailable() - PresentationConnection::OnMessage() takes a callback - PresentationDispatcher::SendXXX() takes PresentationConnectionProxy as parameter BUG=525660,513859 ========== to ========== [Presentation API] (5th)(1-UA) integrate controller and receiver side for 1-UA messaging This CL actually turns on 1-UA messaging: - When presentation starts, a new route is created and returned to MR. In PresentationDispatcher, we send controller_connection_proxy and receiver_connection_request back to browser via PresentationService::SetPresentationConnection() For 1-UA: - In PresentationServiceDelegateImpl::ConnectToPresentation(), we register proxy and request as a controller connection entry to OPM. If there has been a receiver_call_back, invoke it; - When offscreen tab is rendered, we register a receiverPSDImpl, which registers a receiver_call_back to OPM. If there is any pending controller connection entry, invoke receiver_call_back for each controller connection entry; - In receiver_call_back (PresentationDispatcher::OnReceiverConnectionAvailable() function), we create a receiver_connection_proxy object, bind it to receiver_connection_request (input parameter) and connect it with controller_connection_proxy (input parameter). Message sending between controller Blink connection and receiver Blink connection may happen afterwards. For 2-UA: - In PresentationServiceDelegateImpl::ConnectToPresentation(), we create a browser_connection_proxy object, bind it to receiver_connection_request (input parameter) and connect it with controller_connection_proxy (input parameter). Message sending from controller Blink connection to media router may happen afterwards. More details in code comments and design doc (use chromium.org account): https://docs.google.com/document/d/1XM3jhMJTQyhEC5PDAAJFNIaKh6UUEihqZDz_ztEe4... Patch set 4 updates: - added BrowserPresentationConnectionProxy class - Added PresentationFrame::IsOffscreen() and PresentationFrame::OnBrowserConnectionAvailable() - PresentationConnection::OnMessage() takes a callback - PresentationDispatcher::SendXXX() takes PresentationConnectionProxy as parameter BUG=525660,513859 ==========
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: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
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: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_TIMED_OUT, no build URL) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
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.
https://codereview.chromium.org/2471573005/diff/380001/content/renderer/prese... File content/renderer/presentation/presentation_dispatcher.h (right): https://codereview.chromium.org/2471573005/diff/380001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.h:189: blink::mojom::PresentationSessionInfoPtr session_info, On 2017/02/04 00:07:24, mark a. foltz wrote: > content::PresentationSessionInfo& when merging with typemap patch Done.
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, jam@chromium.org, miu@chromium.org, mfoltz@chromium.org Link to the patchset: https://codereview.chromium.org/2471573005/#ps400001 (title: "merge with master and fix unit tests")
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": 400001, "attempt_start_ts": 1486403608956900,
"parent_rev": "df8192adf5dabd4b94e9c7c6a4b6593aa1c3bc65", "commit_rev":
"4b94a1fe3f58aacad2c46bbfd84aa780469e15be"}
Message was sent while issue was closed.
Description was changed from ========== [Presentation API] (5th)(1-UA) integrate controller and receiver side for 1-UA messaging This CL actually turns on 1-UA messaging: - When presentation starts, a new route is created and returned to MR. In PresentationDispatcher, we send controller_connection_proxy and receiver_connection_request back to browser via PresentationService::SetPresentationConnection() For 1-UA: - In PresentationServiceDelegateImpl::ConnectToPresentation(), we register proxy and request as a controller connection entry to OPM. If there has been a receiver_call_back, invoke it; - When offscreen tab is rendered, we register a receiverPSDImpl, which registers a receiver_call_back to OPM. If there is any pending controller connection entry, invoke receiver_call_back for each controller connection entry; - In receiver_call_back (PresentationDispatcher::OnReceiverConnectionAvailable() function), we create a receiver_connection_proxy object, bind it to receiver_connection_request (input parameter) and connect it with controller_connection_proxy (input parameter). Message sending between controller Blink connection and receiver Blink connection may happen afterwards. For 2-UA: - In PresentationServiceDelegateImpl::ConnectToPresentation(), we create a browser_connection_proxy object, bind it to receiver_connection_request (input parameter) and connect it with controller_connection_proxy (input parameter). Message sending from controller Blink connection to media router may happen afterwards. More details in code comments and design doc (use chromium.org account): https://docs.google.com/document/d/1XM3jhMJTQyhEC5PDAAJFNIaKh6UUEihqZDz_ztEe4... Patch set 4 updates: - added BrowserPresentationConnectionProxy class - Added PresentationFrame::IsOffscreen() and PresentationFrame::OnBrowserConnectionAvailable() - PresentationConnection::OnMessage() takes a callback - PresentationDispatcher::SendXXX() takes PresentationConnectionProxy as parameter BUG=525660,513859 ========== to ========== [Presentation API] (5th)(1-UA) integrate controller and receiver side for 1-UA messaging This CL actually turns on 1-UA messaging: - When presentation starts, a new route is created and returned to MR. In PresentationDispatcher, we send controller_connection_proxy and receiver_connection_request back to browser via PresentationService::SetPresentationConnection() For 1-UA: - In PresentationServiceDelegateImpl::ConnectToPresentation(), we register proxy and request as a controller connection entry to OPM. If there has been a receiver_call_back, invoke it; - When offscreen tab is rendered, we register a receiverPSDImpl, which registers a receiver_call_back to OPM. If there is any pending controller connection entry, invoke receiver_call_back for each controller connection entry; - In receiver_call_back (PresentationDispatcher::OnReceiverConnectionAvailable() function), we create a receiver_connection_proxy object, bind it to receiver_connection_request (input parameter) and connect it with controller_connection_proxy (input parameter). Message sending between controller Blink connection and receiver Blink connection may happen afterwards. For 2-UA: - In PresentationServiceDelegateImpl::ConnectToPresentation(), we create a browser_connection_proxy object, bind it to receiver_connection_request (input parameter) and connect it with controller_connection_proxy (input parameter). Message sending from controller Blink connection to media router may happen afterwards. More details in code comments and design doc (use chromium.org account): https://docs.google.com/document/d/1XM3jhMJTQyhEC5PDAAJFNIaKh6UUEihqZDz_ztEe4... Patch set 4 updates: - added BrowserPresentationConnectionProxy class - Added PresentationFrame::IsOffscreen() and PresentationFrame::OnBrowserConnectionAvailable() - PresentationConnection::OnMessage() takes a callback - PresentationDispatcher::SendXXX() takes PresentationConnectionProxy as parameter BUG=525660,513859 Review-Url: https://codereview.chromium.org/2471573005 Cr-Commit-Position: refs/heads/master@{#448299} Committed: https://chromium.googlesource.com/chromium/src/+/4b94a1fe3f58aacad2c46bbfd84a... ==========
Message was sent while issue was closed.
Committed patchset #16 (id:400001) as https://chromium.googlesource.com/chromium/src/+/4b94a1fe3f58aacad2c46bbfd84a...
Message was sent while issue was closed.
A revert of this CL (patchset #16 id:400001) has been created in https://codereview.chromium.org/2674273003/ by joedow@chromium.org. The reason for reverting is: Reverting as this is a suspect CL causing a presentation webkit test to fail: https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linu... Debug output: 10:33:07.330 27811 renderer crash, pid = None, error_line = #CRASHED - renderer 10:33:07.331 27811 killed pid 18285 10:33:07.331 27811 worker/2 presentation/presentation-reconnect.html crashed, (stderr lines): 10:33:07.332 27811 [18285:18329:0206/103306.890280:1569541808:WARNING:url_request_job_manager.cc(90)] Failed to map: layout-test-mojom://content/shell/renderer/layout_test/frame_interface_registry 10:33:07.332 27811 [18285:18329:0206/103306.890393:1569541918:WARNING:url_request_job_manager.cc(90)] Failed to map: layout-test-mojom://content/shell/renderer/layout_test/interface_registry 10:33:07.332 27811 [18285:18329:0206/103306.891027:1569542552:WARNING:url_request_job_manager.cc(90)] Failed to map: layout-test-mojom://Mojo Helpers 10:33:07.332 27811 [1:1:0206/103306.896454:1569552070:ERROR:mojo_context_state.cc(194)] Failed to fetch source for module "content/shell/renderer/layout_test/frame_interface_registry" 10:33:07.332 27811 [1:1:0206/103306.900643:1569552511:ERROR:mojo_context_state.cc(194)] Failed to fetch source for module "content/shell/renderer/layout_test/interface_registry" 10:33:07.332 27811 [1:1:0206/103306.901223:1569553276:ERROR:mojo_context_state.cc(194)] Failed to fetch source for module "Mojo Helpers" 10:33:07.332 27811 [18285:18329:0206/103306.932287:1569583816:WARNING:crash_handler_host_linux.cc(324)] Could not translate tid, attempt = 1 retry ... 10:33:07.333 27725 [42777/48739] presentation/presentation-reconnect.html failed unexpectedly (renderer crashed).
Message was sent while issue was closed.
Description was changed from ========== [Presentation API] (5th)(1-UA) integrate controller and receiver side for 1-UA messaging This CL actually turns on 1-UA messaging: - When presentation starts, a new route is created and returned to MR. In PresentationDispatcher, we send controller_connection_proxy and receiver_connection_request back to browser via PresentationService::SetPresentationConnection() For 1-UA: - In PresentationServiceDelegateImpl::ConnectToPresentation(), we register proxy and request as a controller connection entry to OPM. If there has been a receiver_call_back, invoke it; - When offscreen tab is rendered, we register a receiverPSDImpl, which registers a receiver_call_back to OPM. If there is any pending controller connection entry, invoke receiver_call_back for each controller connection entry; - In receiver_call_back (PresentationDispatcher::OnReceiverConnectionAvailable() function), we create a receiver_connection_proxy object, bind it to receiver_connection_request (input parameter) and connect it with controller_connection_proxy (input parameter). Message sending between controller Blink connection and receiver Blink connection may happen afterwards. For 2-UA: - In PresentationServiceDelegateImpl::ConnectToPresentation(), we create a browser_connection_proxy object, bind it to receiver_connection_request (input parameter) and connect it with controller_connection_proxy (input parameter). Message sending from controller Blink connection to media router may happen afterwards. More details in code comments and design doc (use chromium.org account): https://docs.google.com/document/d/1XM3jhMJTQyhEC5PDAAJFNIaKh6UUEihqZDz_ztEe4... Patch set 4 updates: - added BrowserPresentationConnectionProxy class - Added PresentationFrame::IsOffscreen() and PresentationFrame::OnBrowserConnectionAvailable() - PresentationConnection::OnMessage() takes a callback - PresentationDispatcher::SendXXX() takes PresentationConnectionProxy as parameter BUG=525660,513859 Review-Url: https://codereview.chromium.org/2471573005 Cr-Commit-Position: refs/heads/master@{#448299} Committed: https://chromium.googlesource.com/chromium/src/+/4b94a1fe3f58aacad2c46bbfd84a... ========== to ========== [Presentation API] (5th)(1-UA) integrate controller and receiver side for 1-UA messaging This CL actually turns on 1-UA messaging: - When presentation starts, a new route is created and returned to MR. In PresentationDispatcher, we send controller_connection_proxy and receiver_connection_request back to browser via PresentationService::SetPresentationConnection() For 1-UA: - In PresentationServiceDelegateImpl::ConnectToPresentation(), we register proxy and request as a controller connection entry to OPM. If there has been a receiver_call_back, invoke it; - When offscreen tab is rendered, we register a receiverPSDImpl, which registers a receiver_call_back to OPM. If there is any pending controller connection entry, invoke receiver_call_back for each controller connection entry; - In receiver_call_back (PresentationDispatcher::OnReceiverConnectionAvailable() function), we create a receiver_connection_proxy object, bind it to receiver_connection_request (input parameter) and connect it with controller_connection_proxy (input parameter). Message sending between controller Blink connection and receiver Blink connection may happen afterwards. For 2-UA: - In PresentationServiceDelegateImpl::ConnectToPresentation(), we create a browser_connection_proxy object, bind it to receiver_connection_request (input parameter) and connect it with controller_connection_proxy (input parameter). Message sending from controller Blink connection to media router may happen afterwards. More details in code comments and design doc (use chromium.org account): https://docs.google.com/document/d/1XM3jhMJTQyhEC5PDAAJFNIaKh6UUEihqZDz_ztEe4... Patch set 4 updates: - added BrowserPresentationConnectionProxy class - Added PresentationFrame::IsOffscreen() and PresentationFrame::OnBrowserConnectionAvailable() - PresentationConnection::OnMessage() takes a callback - PresentationDispatcher::SendXXX() takes PresentationConnectionProxy as parameter BUG=525660,513859 Review-Url: https://codereview.chromium.org/2471573005 Cr-Commit-Position: refs/heads/master@{#448299} Committed: https://chromium.googlesource.com/chromium/src/+/4b94a1fe3f58aacad2c46bbfd84a... ==========
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 mfoltz@chromium.org, imcheng@chromium.org, jam@chromium.org, miu@chromium.org Link to the patchset: https://codereview.chromium.org/2471573005/#ps440001 (title: "merge with master")
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": 440001, "attempt_start_ts": 1486432025241320,
"parent_rev": "35b4a8553a6b68f24297006c39168f5ff4aa0468", "commit_rev":
"42407c7feb218491f72b05732fa83b4679144da4"}
Message was sent while issue was closed.
Description was changed from ========== [Presentation API] (5th)(1-UA) integrate controller and receiver side for 1-UA messaging This CL actually turns on 1-UA messaging: - When presentation starts, a new route is created and returned to MR. In PresentationDispatcher, we send controller_connection_proxy and receiver_connection_request back to browser via PresentationService::SetPresentationConnection() For 1-UA: - In PresentationServiceDelegateImpl::ConnectToPresentation(), we register proxy and request as a controller connection entry to OPM. If there has been a receiver_call_back, invoke it; - When offscreen tab is rendered, we register a receiverPSDImpl, which registers a receiver_call_back to OPM. If there is any pending controller connection entry, invoke receiver_call_back for each controller connection entry; - In receiver_call_back (PresentationDispatcher::OnReceiverConnectionAvailable() function), we create a receiver_connection_proxy object, bind it to receiver_connection_request (input parameter) and connect it with controller_connection_proxy (input parameter). Message sending between controller Blink connection and receiver Blink connection may happen afterwards. For 2-UA: - In PresentationServiceDelegateImpl::ConnectToPresentation(), we create a browser_connection_proxy object, bind it to receiver_connection_request (input parameter) and connect it with controller_connection_proxy (input parameter). Message sending from controller Blink connection to media router may happen afterwards. More details in code comments and design doc (use chromium.org account): https://docs.google.com/document/d/1XM3jhMJTQyhEC5PDAAJFNIaKh6UUEihqZDz_ztEe4... Patch set 4 updates: - added BrowserPresentationConnectionProxy class - Added PresentationFrame::IsOffscreen() and PresentationFrame::OnBrowserConnectionAvailable() - PresentationConnection::OnMessage() takes a callback - PresentationDispatcher::SendXXX() takes PresentationConnectionProxy as parameter BUG=525660,513859 Review-Url: https://codereview.chromium.org/2471573005 Cr-Commit-Position: refs/heads/master@{#448299} Committed: https://chromium.googlesource.com/chromium/src/+/4b94a1fe3f58aacad2c46bbfd84a... ========== to ========== [Presentation API] (5th)(1-UA) integrate controller and receiver side for 1-UA messaging This CL actually turns on 1-UA messaging: - When presentation starts, a new route is created and returned to MR. In PresentationDispatcher, we send controller_connection_proxy and receiver_connection_request back to browser via PresentationService::SetPresentationConnection() For 1-UA: - In PresentationServiceDelegateImpl::ConnectToPresentation(), we register proxy and request as a controller connection entry to OPM. If there has been a receiver_call_back, invoke it; - When offscreen tab is rendered, we register a receiverPSDImpl, which registers a receiver_call_back to OPM. If there is any pending controller connection entry, invoke receiver_call_back for each controller connection entry; - In receiver_call_back (PresentationDispatcher::OnReceiverConnectionAvailable() function), we create a receiver_connection_proxy object, bind it to receiver_connection_request (input parameter) and connect it with controller_connection_proxy (input parameter). Message sending between controller Blink connection and receiver Blink connection may happen afterwards. For 2-UA: - In PresentationServiceDelegateImpl::ConnectToPresentation(), we create a browser_connection_proxy object, bind it to receiver_connection_request (input parameter) and connect it with controller_connection_proxy (input parameter). Message sending from controller Blink connection to media router may happen afterwards. More details in code comments and design doc (use chromium.org account): https://docs.google.com/document/d/1XM3jhMJTQyhEC5PDAAJFNIaKh6UUEihqZDz_ztEe4... Patch set 4 updates: - added BrowserPresentationConnectionProxy class - Added PresentationFrame::IsOffscreen() and PresentationFrame::OnBrowserConnectionAvailable() - PresentationConnection::OnMessage() takes a callback - PresentationDispatcher::SendXXX() takes PresentationConnectionProxy as parameter BUG=525660,513859 Review-Url: https://codereview.chromium.org/2471573005 Cr-Original-Commit-Position: refs/heads/master@{#448299} Committed: https://chromium.googlesource.com/chromium/src/+/4b94a1fe3f58aacad2c46bbfd84a... Review-Url: https://codereview.chromium.org/2471573005 Cr-Commit-Position: refs/heads/master@{#448509} Committed: https://chromium.googlesource.com/chromium/src/+/42407c7feb218491f72b05732fa8... ==========
Message was sent while issue was closed.
Committed patchset #18 (id:440001) as https://chromium.googlesource.com/chromium/src/+/42407c7feb218491f72b05732fa8... |
