Issue 2477573002: [Presentation API] (3rd) (1-UA) Split PresentationServiceDelegateImpl(PSDImpl)

zhaobin

Patchset #1 (id:1) has been deleted

4 years, 1 month ago (2016-11-03 18:36:11 UTC) #1

zhaobin

Description was changed from ========== [Presentation API] (1-UA) Split PresentationServiceDelegateImpl(PSDImpl) into ControllerPSDImpl and ReceiverPSDImpl (Splitting ...

4 years, 1 month ago (2016-11-03 18:39:25 UTC) #2

Description was changed from

==========
[Presentation API] (1-UA) Split PresentationServiceDelegateImpl(PSDImpl) into
ControllerPSDImpl and ReceiverPSDImpl

(Splitting of https://codereview.chromium.org/2379703002/)

In an offscreen presentation, we have controlling frame (page that starts a
PresentationRequest with url) and receiver frame (page corresponding to the url,
rendered as offscreen tab). They talk to MR extension and
OffscreenPresentationManager (OPM) in different ways. It makes sense to split
them.

- create ReceiverPSDImpl class
- PresentationServiceImpl (PSImpl) class takes controller_delegate_ and
receiver_delegate_ objects. For controlling frame, receiver_delegate_ = nullptr;
For receiver frame controller_delegate_ = nullptr
- Added PSImpl::SetPresentationConnection() - will be called in
PresentationDispatcher::OnSessionCreated(),
PresentationDispatcher::OnReceiverConnectionAvailable(); will call
OPM::RegisterOffscreenPresentationController()
- Added PSImpl::OnReceiverConnectionAvailable() - will be registered to OPM as
receiver callback

PSImpl.receiver_delegate_ is always nullptr now (make sure things wont break).
It will be set in offscreen_tab.cc in next CL.

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] (1-UA) Split PresentationServiceDelegateImpl(PSDImpl) into
ControllerPSDImpl and ReceiverPSDImpl

(Splitting of https://codereview.chromium.org/2379703002/)

In an offscreen presentation, we have controlling frame (page that starts a
PresentationRequest with url) and receiver frame (page corresponding to the url,
rendered as offscreen tab). They talk to MR extension and
OffscreenPresentationManager (OPM) in different ways. It makes sense to split
them.

- create ReceiverPSDImpl class
- PresentationServiceImpl (PSImpl) class takes controller_delegate_ and
receiver_delegate_ objects. For controlling frame, receiver_delegate_ = nullptr;
For receiver frame controller_delegate_ = nullptr
- Added PSImpl::SetPresentationConnection() - will be called in
PresentationDispatcher::OnSessionCreated(),
PresentationDispatcher::OnReceiverConnectionAvailable(); will call
OPM::RegisterOffscreenPresentationController()
- Added PSImpl::OnReceiverConnectionAvailable() - will be registered to OPM as
receiver callback

PSImpl.receiver_delegate_ is always nullptr now (make sure things wont break).
It will be set in offscreen_tab.cc in next CL.

More details in code comments and design doc (use chromium.org account):
https://docs.google.com/document/d/1XM3jhMJTQyhEC5PDAAJFNIaKh6UUEihqZDz_ztEe4...

BUG=525660
==========

zhaobin

zhaobin@chromium.org changed reviewers: + imcheng@chromium.org, mfoltz@chromium.org

4 years, 1 month ago (2016-11-03 18:39:25 UTC) #3

mark a. foltz

Mostly minor comments on style and documentation. I had a couple of suggestions on the ...

4 years, 1 month ago (2016-11-08 23:40:52 UTC) #5

zhaobin

https://codereview.chromium.org/2477573002/diff/20001/chrome/browser/chrome_content_browser_client.cc File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2477573002/diff/20001/chrome/browser/chrome_content_browser_client.cc#newcode3073 chrome/browser/chrome_content_browser_client.cc:3073: if (media_router::MediaRouterEnabled(web_contents->GetBrowserContext())) { On 2016/11/08 23:40:51, mark a. foltz ...

4 years, 1 month ago (2016-11-10 04:14:01 UTC) #6

https://codereview.chromium.org/2477573002/diff/20001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2477573002/diff/20001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:3073: if
(media_router::MediaRouterEnabled(web_contents->GetBrowserContext())) {
On 2016/11/08 23:40:51, mark a. foltz wrote:
> Please guard code that uses media_router:: with #if
defined(ENABLE_MEDIA_ROUTER)
> as above.

Done.

https://codereview.chromium.org/2477573002/diff/20001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:3079:
DCHECK(web_contents->GetBrowserContext()->IsOffTheRecord());
On 2016/11/08 23:40:51, mark a. foltz wrote:
> API design opinion:
> 
> This API is part of content/browser and it's a little weird to have a method
> that will DCHECK for some WebContents.  If the caller passes a WebContents*
that
> cannot be a receiver, I would just return nullptr instead of DCHECK(), and the
> caller can check the return value if it expects it to be valid.

Done.

https://codereview.chromium.org/2477573002/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/presentation_service_delegate_impl.cc (right):

https://codereview.chromium.org/2477573002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:929: auto
offscreen_presentation_manager = OffscreenPresentationManagerFactory::
On 2016/11/08 23:40:51, mark a. foltz wrote:
> Slight preference for auto offscreen_presentation_manager* const.  This makes
it
> clear that you are declaring a const pointer.

Done.

https://codereview.chromium.org/2477573002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:932:
session.presentation_id, GURL(session.presentation_url),
On 2016/11/08 23:40:51, mark a. foltz wrote:
> session.presentation_url is already a GURL.

Done.

https://codereview.chromium.org/2477573002/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/presentation_service_delegate_impl.h (right):

https://codereview.chromium.org/2477573002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.h:48: class
PresentationServiceDelegateImpl
On 2016/11/08 23:40:51, mark a. foltz wrote:
> Should we rename this to ControllerPresentationServiceDelegateImpl?  Bin,
Derek,
> WDYT?

Discussed with Derek, we think ControllerPresentationServiceDelegateImpl sounds
fine (too long though :))

https://codereview.chromium.org/2477573002/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc
(right):

https://codereview.chromium.org/2477573002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc:486:
content::PresentationSessionInfo session_info(GURL("presentation_url"),
On 2016/11/08 23:40:51, mark a. foltz wrote:
> Please use an obviously-valid URL like
http://www.example.com/presentation.html

Done.

https://codereview.chromium.org/2477573002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc:493:
EXPECT_EQ(size_t(1), presentation_manager->offscreen_presentations_.size());
On 2016/11/08 23:40:51, mark a. foltz wrote:
> It would be a more thorough test to inject a mock
> OffscreenPresentationController to verify that RegisterOffscreenPresentation
is
> called.   However there's a bit of setup required to do it the "standard" way.
> 
> The ChromeRenderViewTestHarness constructs a TestingProfile using
> CreateBrowserContext(), you can override that to inject a test factory that
> returns a mock using TestingProfile::Builder:
> 
>
https://cs.chromium.org/chromium/src/chrome/test/base/testing_profile.h?rcl=0...
> 
> The other way would be to add an API like
> OffscreenPresentationManagerFactory::SetInstanceForTest().  However that adds
> extra code to the release build.  Derek might have thoughts about this as well
> as he's written quite a few tests.
> 
> 
> 

Done.

https://codereview.chromium.org/2477573002/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/receiver_presentation_service_delegate_impl.cc
(right):

https://codereview.chromium.org/2477573002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/receiver_presentation_service_delegate_impl.cc:38:
void ReceiverPresentationServiceDelegateImpl::AddObserver(
On 2016/11/08 23:40:51, mark a. foltz wrote:
> Can AddObserver/RemoveObserver be moved into PresentationServiceDelegateBase? 
> The API only depends on content:: and the implementation should be identical
> across controller and receiver.

Done.

https://codereview.chromium.org/2477573002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/receiver_presentation_service_delegate_impl.cc:46:
observers_[rfh_id] = observer;
On 2016/11/08 23:40:51, mark a. foltz wrote:
> I am not sure why the map is needed.  Is it important that there is only one
> observer per render_frame_id/render_host_id?  When would there be more than
one
> observer registered for the same render frame host?  It seems like that would
be
> a bug given how it's used in PresentationServiceImpl.
> 
> Also, what guarantees that |observer| outlives |this| here?  I'm assuming it's
> because |observer| is implemented by PresentationServiceImpl which also owns
the
> delegate.

multiple PSImpl objects may reference to the same PSDImpl object. Need to keep a
map for all PSImpl referencing current PSDImpl object. Since PSImpl does not own
PSDImpl obj, PSDImpl obj might be destroyed earlier than PSImpl, and must notify
all PSImpl referencing it.

~PSImpl() will remove itself from PSDImpl's observer list. |observer| should
always be valid.

https://codereview.chromium.org/2477573002/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/receiver_presentation_service_delegate_impl.h
(right):

https://codereview.chromium.org/2477573002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/receiver_presentation_service_delegate_impl.h:31: //
Implements the receiver side of presentation API for offscreen presentations.
On 2016/11/08 23:40:52, mark a. foltz wrote:
> nit: Presentation

Done.

https://codereview.chromium.org/2477573002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/receiver_presentation_service_delegate_impl.h:32: //
Created under the offscreen tab WebContents when the tab is created for an
On 2016/11/08 23:40:52, mark a. foltz wrote:
> There's currently no "tab" or visual indicator in the tab strip for the
> offscreen presentation, so I would just say:
> 
> Created with the offscreen WebContents for an offscreen presentation.

Done.

https://codereview.chromium.org/2477573002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/receiver_presentation_service_delegate_impl.h:36: //
Only the main frame of the offscreen tab is allowed to make receiver
On 2016/11/08 23:40:52, mark a. foltz wrote:
> offscreen WebContents

Done.

https://codereview.chromium.org/2477573002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/receiver_presentation_service_delegate_impl.h:48: //
exists under |web_contents|.
On 2016/11/08 23:40:51, mark a. foltz wrote:
> Document that this does not take ownership of |web_contents|.

Done.

https://codereview.chromium.org/2477573002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/receiver_presentation_service_delegate_impl.h:75:
content::WebContents* const web_contents_;
On 2016/11/08 23:40:52, mark a. foltz wrote:
> There is a risk you will call into |web_contents_| during the WebContents
dtor. 
> Are you sure that WebContentsUserData are all destroyed before WebContents?

Did some manual tests, ~ReceiverPSDImpl() always happens before WebContents
dtor, e.g. call window.close() on receiver page. ReceiverPSDImpl is
CreatedForWebContents(), it seems to be a member of webContents, and WebContents
will take care of the destruction order (not sure though)?

https://codereview.chromium.org/2477573002/diff/20001/content/browser/present...
File content/browser/presentation/presentation_service_impl.cc (right):

https://codereview.chromium.org/2477573002/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:145: if (auto delegate
= GetPresentationServiceDelegate())
On 2016/11/08 23:40:52, mark a. foltz wrote:
> Prefer auto delegate* const here and elsewhere

Done.

https://codereview.chromium.org/2477573002/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:174: if
(impl->receiver_delegate_) {
On 2016/11/08 23:40:52, mark a. foltz wrote:
> When would |receiver_delegate_| be null?  It looks like
> ReceiverPresentationServiceDelegateImpl::CreateForWebContents will always
> succeed.
> 

Sorry for the confusion. It is set in offscreen_tab.cc in 5th code review

https://codereview.chromium.org/2471573005/diff/40001/chrome/browser/extensio...

When get, we use
media_router::ReceiverPresentationServiceDelegateImpl::FromWebContents(), and it
wont create new ReceiverPSDImpl.

https://codereview.chromium.org/2477573002/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:178: }
On 2016/11/08 23:40:52, mark a. foltz wrote:
> else { DCHECK(controller_delegate_); }

Code removed.

https://codereview.chromium.org/2477573002/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:398: DVLOG(2) <<
"SendSessionMessage"
On 2016/11/08 23:40:52, mark a. foltz wrote:
> Nit: Move this log below L406, where the message sending actually happens.

Prefer keeping it here as it makes debugging easier (If no SendSessionMessage
log, it means PSImpl::SendSessionMessage is not called. There is no need to
check conditions.)

https://codereview.chromium.org/2477573002/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:470: if
(receiver_delegate_)
On 2016/11/08 23:40:52, mark a. foltz wrote:
> Use a ternary return

Done.

https://codereview.chromium.org/2477573002/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:526: //
client_->OnReceiverConnectionAvailable(
On 2016/11/08 23:40:52, mark a. foltz wrote:
> Did you mean to comment this out?

Yes, client_->OnReceiverConnectionAvailable() is not defined in this CL
(splitting...). Remove it.

https://codereview.chromium.org/2477573002/diff/20001/content/browser/present...
File content/browser/presentation/presentation_service_impl.h (right):

https://codereview.chromium.org/2477573002/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl.h:156:
PresentationServiceImpl(
On 2016/11/08 23:40:52, mark a. foltz wrote:
> API design opinion:  
> 
> It would be better to make this ctor private and have two public static
factory
> methods that take the controller and receiver delegate, respectively.
> 
> CreateForPresentationController()
> CreateForPresentationReceiver()
> 
> This would prevent impossible combinations of arguments for controller and
> receiver delegates.

Done.

https://codereview.chromium.org/2477573002/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl.h:247: // receiver PSImpl
inits. Calls |client_| to create a new
On 2016/11/08 23:40:52, mark a. foltz wrote:
> the PresentationServiceImpl for the presentation receiver is initialized.

Done.

https://codereview.chromium.org/2477573002/diff/20001/content/browser/present...
File content/browser/presentation/presentation_service_impl_unittest.cc (right):

https://codereview.chromium.org/2477573002/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl_unittest.cc:724:
RegisterOffscreenPresentationConnectionRaw(_, _, _, _))
On 2016/11/08 23:40:52, mark a. foltz wrote:
> Can you verify the contents of |session| here?

Done.

https://codereview.chromium.org/2477573002/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl_unittest.cc:735:
&mock_delegate_,
On 2016/11/08 23:40:52, mark a. foltz wrote:
> Should the controller delegate be nullptr here?

Done.

https://codereview.chromium.org/2477573002/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl_unittest.cc:739:
RegisterReceiverConnectionAvailableCallback(_));
On 2016/11/08 23:40:52, mark a. foltz wrote:
> Nit: verify that the callback registered is not null.

Done.

https://codereview.chromium.org/2477573002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/modules/presentation/presentation.mojom
(right):

https://codereview.chromium.org/2477573002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/presentation/presentation.mojom:72:
/////////////Functions here are for the controller part of the API.///////////
On 2016/11/08 23:40:52, mark a. foltz wrote:
> Nit: add spaces between / and text to improve readability.

Done.

https://codereview.chromium.org/2477573002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/presentation/presentation.mojom:108:
// Called in StartSession's callback function for 1-UA presentations only. It
On 2016/11/08 23:40:52, mark a. foltz wrote:
> s/1-UA/offscreen/ for consistency with other documentation.

Done.

mark a. foltz

One more round, mostly cleaning up includes and a few minor fixes. https://codereview.chromium.org/2477573002/diff/80001/chrome/browser/media/router/offscreen_presentation_manager_factory.h File chrome/browser/media/router/offscreen_presentation_manager_factory.h ...

4 years, 1 month ago (2016-11-15 23:41:47 UTC) #7

mark a. foltz

> https://codereview.chromium.org/2477573002/diff/80001/content/browser/presentation/presentation_service_impl.cc > File content/browser/presentation/presentation_service_impl.cc (right): > > https://codereview.chromium.org/2477573002/diff/80001/content/browser/presentation/presentation_service_impl.cc#newcode475 > content/browser/presentation/presentation_service_impl.cc:475: ? static_cast<PresentationServiceDelegateBase*>(receiver_delegate_) > You ...

4 years, 1 month ago (2016-11-15 23:43:42 UTC) #8

zhaobin

https://codereview.chromium.org/2477573002/diff/80001/chrome/browser/media/router/offscreen_presentation_manager_factory.h File chrome/browser/media/router/offscreen_presentation_manager_factory.h (right): https://codereview.chromium.org/2477573002/diff/80001/chrome/browser/media/router/offscreen_presentation_manager_factory.h#newcode33 chrome/browser/media/router/offscreen_presentation_manager_factory.h:33: static OffscreenPresentationManagerFactory* GetInstance(); On 2016/11/15 23:41:46, mark a. foltz ...

4 years, 1 month ago (2016-11-16 18:02:42 UTC) #9

imcheng

Looks like this patch will need to be updated with the new design in 5th ...

4 years ago (2016-11-29 00:39:01 UTC) #10

zhaobin

https://codereview.chromium.org/2477573002/diff/100001/chrome/browser/media/router/offscreen_presentation_manager_factory.h File chrome/browser/media/router/offscreen_presentation_manager_factory.h (right): https://codereview.chromium.org/2477573002/diff/100001/chrome/browser/media/router/offscreen_presentation_manager_factory.h#newcode32 chrome/browser/media/router/offscreen_presentation_manager_factory.h:32: // For test use only On 2016/11/29 00:39:01, imcheng ...

4 years ago (2016-11-29 23:22:33 UTC) #11

mark a. foltz

LGTM with comments addressed Nice work! Make sure the patch description is up to date ...

4 years ago (2016-12-02 22:09:05 UTC) #12

zhaobin

https://codereview.chromium.org/2477573002/diff/140001/chrome/browser/chrome_content_browser_client.cc File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2477573002/diff/140001/chrome/browser/chrome_content_browser_client.cc#newcode3128 chrome/browser/chrome_content_browser_client.cc:3128: if (web_contents->GetBrowserContext()->IsOffTheRecord()) On 2016/12/02 22:09:05, mark a. foltz wrote: ...

4 years ago (2016-12-03 00:34:26 UTC) #13

zhaobin

zhaobin@chromium.org changed reviewers: + dcheng@chromium.org, jam@chromium.org

4 years ago (2016-12-13 22:15:38 UTC) #15

zhaobin

+dcheng for mojo related changes; +jam for content related changes.

4 years ago (2016-12-13 22:15:39 UTC) #16

jam

https://codereview.chromium.org/2477573002/diff/200001/content/public/browser/presentation_service_delegate.h File content/public/browser/presentation_service_delegate.h (right): https://codereview.chromium.org/2477573002/diff/200001/content/public/browser/presentation_service_delegate.h#newcode101 content/public/browser/presentation_service_delegate.h:101: std::map<RenderFrameHostID, Observer*> observers_; this is against the content api ...

4 years ago (2016-12-14 17:48:23 UTC) #17

zhaobin

https://codereview.chromium.org/2477573002/diff/200001/content/public/browser/presentation_service_delegate.h File content/public/browser/presentation_service_delegate.h (right): https://codereview.chromium.org/2477573002/diff/200001/content/public/browser/presentation_service_delegate.h#newcode101 content/public/browser/presentation_service_delegate.h:101: std::map<RenderFrameHostID, Observer*> observers_; On 2016/12/14 17:48:23, jam wrote: > ...

4 years ago (2016-12-15 02:55:59 UTC) #18

dcheng

https://codereview.chromium.org/2477573002/diff/220001/chrome/browser/chrome_content_browser_client.cc File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2477573002/diff/220001/chrome/browser/chrome_content_browser_client.cc#newcode3160 chrome/browser/chrome_content_browser_client.cc:3160: if (impl && web_contents->GetBrowserContext()->IsOffTheRecord()) I would expect the IsOffTheRecord() ...

3 years, 11 months ago (2017-01-06 11:54:22 UTC) #20

https://codereview.chromium.org/2477573002/diff/220001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2477573002/diff/220001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:3160: if (impl &&
web_contents->GetBrowserContext()->IsOffTheRecord())
I would expect the IsOffTheRecord() bit to a be a DCHECK(): we shouldn't be
creating it for a  non-OTR browser context at all, should we?

Which means this can just be simplified to:

auto* impl =
media_router::ReceiverPresentationServiceDelegateImpl::FromWebContents(web_contents);
DCHECK(web_contents->GetBrowserContext()->IsOffTheRecord());
return impl;

https://codereview.chromium.org/2477573002/diff/220001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/presentation/presentation.mojom
(right):

https://codereview.chromium.org/2477573002/diff/220001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/presentation.mojom:111:
PresentationConnection connection);
As far as I can tell, the goal is a 1:1 relationship of PresentationConnection
objects: we only need to associate a PresentationConnection in the renderer
starting the presentation with the PresentationConnection object in the
offscreen presentation. So my only big suggestion is to try to remove
SetTargetConnection() to simplify the exposed API in Mojo. Right now, this
method can be called at anytime, it's harder to understand how different
PresentationConnections can be related.

Is it possible to pass a PresentationConnection + PresentationConnection& pair
here? In C++, this would translate to a PresentationConnectionPtr and a
PresentationConnectionRequest.

In C++, in the renderer starting the presentation, we would create the proxy
connection:
  auto proxy = base::MakeUnique<PresentationConnectionProxy>();

then we would create PresentationConnectionRequest:
  auto remote_connection_request = proxy->MakeRemoteRequest();

and then we pass both the interface pointer to the created proxy, as well as the
interface request that the remote end will connect:

  service->SetPresentationConnection(info, proxy->Bind(),
      std::move(remote_connection_request));

On the browser side, the |remote_connection_request| is passed to the offscreen
presentation, which binds it to the PresentationConnection impl it creates,
completing the connection.

The MakeRemoteRequest() helper implementation would look like this:
PresentationConnectionRequest
PresentationConnectionProxy::MakeRemoteRequest() {
  DCHECK(!target_connection_)
      << "target_connection_ should only be bound once.";
  return mojo::MakeRequest(&target_connection_);
}

zhaobin

Removed SetTargetConnection() from PresentationConnection mojo interface according to dcheng's suggestion. Will update 4th and 5th ...

3 years, 11 months ago (2017-01-10 01:39:17 UTC) #22

Removed SetTargetConnection() from PresentationConnection mojo interface
according to dcheng's suggestion. Will update 4th and 5th patch accordingly.

https://codereview.chromium.org/2477573002/diff/220001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2477573002/diff/220001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:3160: if (impl &&
web_contents->GetBrowserContext()->IsOffTheRecord())
On 2017/01/06 11:54:22, dcheng wrote:
> I would expect the IsOffTheRecord() bit to a be a DCHECK(): we shouldn't be
> creating it for a  non-OTR browser context at all, should we?
> 
> Which means this can just be simplified to:
> 
> auto* impl =
>
media_router::ReceiverPresentationServiceDelegateImpl::FromWebContents(web_contents);
> DCHECK(web_contents->GetBrowserContext()->IsOffTheRecord());
> return impl;

Mark suggests not using DCHECK in patch set 1:

mark a. foltz 2016/11/08 23:40:51
API design opinion:

This API is part of content/browser and it's a little weird to have a method
that will DCHECK for some WebContents.  If the caller passes a WebContents* that
cannot be a receiver, I would just return nullptr instead of DCHECK(), and the
caller can check the return value if it expects it to be valid.

https://codereview.chromium.org/2477573002/diff/220001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/presentation/presentation.mojom
(right):

https://codereview.chromium.org/2477573002/diff/220001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/presentation.mojom:111:
PresentationConnection connection);
On 2017/01/06 11:54:22, dcheng wrote:
> As far as I can tell, the goal is a 1:1 relationship of PresentationConnection
> objects: we only need to associate a PresentationConnection in the renderer
> starting the presentation with the PresentationConnection object in the
> offscreen presentation. So my only big suggestion is to try to remove
> SetTargetConnection() to simplify the exposed API in Mojo. Right now, this
> method can be called at anytime, it's harder to understand how different
> PresentationConnections can be related.
> 
> Is it possible to pass a PresentationConnection + PresentationConnection& pair
> here? In C++, this would translate to a PresentationConnectionPtr and a
> PresentationConnectionRequest.
> 
> In C++, in the renderer starting the presentation, we would create the proxy
> connection:
>   auto proxy = base::MakeUnique<PresentationConnectionProxy>();
> 
> then we would create PresentationConnectionRequest:
>   auto remote_connection_request = proxy->MakeRemoteRequest();
> 
> and then we pass both the interface pointer to the created proxy, as well as
the
> interface request that the remote end will connect:
> 
>   service->SetPresentationConnection(info, proxy->Bind(),
>       std::move(remote_connection_request));
> 
> On the browser side, the |remote_connection_request| is passed to the
offscreen
> presentation, which binds it to the PresentationConnection impl it creates,
> completing the connection.
> 
> The MakeRemoteRequest() helper implementation would look like this:
> PresentationConnectionRequest
> PresentationConnectionProxy::MakeRemoteRequest() {
>   DCHECK(!target_connection_)
>       << "target_connection_ should only be bound once.";
>   return mojo::MakeRequest(&target_connection_);
> }

Done.

dcheng

https://codereview.chromium.org/2477573002/diff/220001/chrome/browser/chrome_content_browser_client.cc File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2477573002/diff/220001/chrome/browser/chrome_content_browser_client.cc#newcode3160 chrome/browser/chrome_content_browser_client.cc:3160: if (impl && web_contents->GetBrowserContext()->IsOffTheRecord()) On 2017/01/10 01:39:17, zhaobin wrote: ...

3 years, 11 months ago (2017-01-10 06:46:59 UTC) #23

https://codereview.chromium.org/2477573002/diff/220001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2477573002/diff/220001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:3160: if (impl &&
web_contents->GetBrowserContext()->IsOffTheRecord())
On 2017/01/10 01:39:17, zhaobin wrote:
> On 2017/01/06 11:54:22, dcheng wrote:
> > I would expect the IsOffTheRecord() bit to a be a DCHECK(): we shouldn't be
> > creating it for a  non-OTR browser context at all, should we?
> > 
> > Which means this can just be simplified to:
> > 
> > auto* impl =
> >
>
media_router::ReceiverPresentationServiceDelegateImpl::FromWebContents(web_contents);
> > DCHECK(web_contents->GetBrowserContext()->IsOffTheRecord());
> > return impl;
> 
> Mark suggests not using DCHECK in patch set 1:
> 
> mark a. foltz 2016/11/08 23:40:51
> API design opinion:
> 
> This API is part of content/browser and it's a little weird to have a method
> that will DCHECK for some WebContents.  If the caller passes a WebContents*
that
> cannot be a receiver, I would just return nullptr instead of DCHECK(), and the
> caller can check the return value if it expects it to be valid.

It's not really that weird, IMO. If we shouldn't have to handle "impossible
cases" (i.e. the impossible case is developer error), then it's better to DCHECK
rather than fail silently.

As an example, consider base::StringValue. base::StringValue expects to only
hold Unicode strings. If a non-UTF8 std::string is passed, we don't provide a
way to gracefully fail -- we just DCHECK, because the developer did something
wrong.

Unless there's a good reason to do otherwise, I think we should stick to that
pattern here.

https://codereview.chromium.org/2477573002/diff/280001/content/public/browser...
File content/public/browser/presentation_service_delegate.h (right):

https://codereview.chromium.org/2477573002/diff/280001/content/public/browser...
content/public/browser/presentation_service_delegate.h:225: // |connection|:
Pointer to controller's presentation connection,
Some of these comments need updates.

https://codereview.chromium.org/2477573002/diff/280001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/presentation/presentation.mojom
(right):

https://codereview.chromium.org/2477573002/diff/280001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/presentation.mojom:111:
PresentationConnection controller_conn_ptr,
Nit: conn => connection here and below. Same below as well.

https://codereview.chromium.org/2477573002/diff/280001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/presentation.mojom:173:
// Called on a presentation receiver when the first presentation connection is
What happens when subsequent connections are available? I'm just trying to
understand the significance of 'first' here.

zhaobin

https://codereview.chromium.org/2477573002/diff/220001/chrome/browser/chrome_content_browser_client.cc File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2477573002/diff/220001/chrome/browser/chrome_content_browser_client.cc#newcode3160 chrome/browser/chrome_content_browser_client.cc:3160: if (impl && web_contents->GetBrowserContext()->IsOffTheRecord()) On 2017/01/10 06:46:58, dcheng wrote: ...

3 years, 11 months ago (2017-01-10 21:49:54 UTC) #24

https://codereview.chromium.org/2477573002/diff/220001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2477573002/diff/220001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:3160: if (impl &&
web_contents->GetBrowserContext()->IsOffTheRecord())
On 2017/01/10 06:46:58, dcheng wrote:
> On 2017/01/10 01:39:17, zhaobin wrote:
> > On 2017/01/06 11:54:22, dcheng wrote:
> > > I would expect the IsOffTheRecord() bit to a be a DCHECK(): we shouldn't
be
> > > creating it for a  non-OTR browser context at all, should we?
> > > 
> > > Which means this can just be simplified to:
> > > 
> > > auto* impl =
> > >
> >
>
media_router::ReceiverPresentationServiceDelegateImpl::FromWebContents(web_contents);
> > > DCHECK(web_contents->GetBrowserContext()->IsOffTheRecord());
> > > return impl;
> > 
> > Mark suggests not using DCHECK in patch set 1:
> > 
> > mark a. foltz 2016/11/08 23:40:51
> > API design opinion:
> > 
> > This API is part of content/browser and it's a little weird to have a method
> > that will DCHECK for some WebContents.  If the caller passes a WebContents*
> that
> > cannot be a receiver, I would just return nullptr instead of DCHECK(), and
the
> > caller can check the return value if it expects it to be valid.
> 
> It's not really that weird, IMO. If we shouldn't have to handle "impossible
> cases" (i.e. the impossible case is developer error), then it's better to
DCHECK
> rather than fail silently.
> 
> As an example, consider base::StringValue. base::StringValue expects to only
> hold Unicode strings. If a non-UTF8 std::string is passed, we don't provide a
> way to gracefully fail -- we just DCHECK, because the developer did something
> wrong.
> 
> Unless there's a good reason to do otherwise, I think we should stick to that
> pattern here.

Done.

https://codereview.chromium.org/2477573002/diff/280001/content/public/browser...
File content/public/browser/presentation_service_delegate.h (right):

https://codereview.chromium.org/2477573002/diff/280001/content/public/browser...
content/public/browser/presentation_service_delegate.h:225: // |connection|:
Pointer to controller's presentation connection,
On 2017/01/10 06:46:58, dcheng wrote:
> Some of these comments need updates.

Done.

https://codereview.chromium.org/2477573002/diff/280001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/presentation/presentation.mojom
(right):

https://codereview.chromium.org/2477573002/diff/280001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/presentation.mojom:111:
PresentationConnection controller_conn_ptr,
On 2017/01/10 06:46:59, dcheng wrote:
> Nit: conn => connection here and below. Same below as well.

Done.

https://codereview.chromium.org/2477573002/diff/280001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/presentation.mojom:173:
// Called on a presentation receiver when the first presentation connection is
On 2017/01/10 06:46:58, dcheng wrote:
> What happens when subsequent connections are available? I'm just trying to
> understand the significance of 'first' here.

Done.

Outdated comments. It is invoked for all presentation connections on controlling
page.

mark a. foltz

https://codereview.chromium.org/2477573002/diff/220001/third_party/WebKit/public/platform/modules/presentation/presentation.mojom File third_party/WebKit/public/platform/modules/presentation/presentation.mojom (right): https://codereview.chromium.org/2477573002/diff/220001/third_party/WebKit/public/platform/modules/presentation/presentation.mojom#newcode111 third_party/WebKit/public/platform/modules/presentation/presentation.mojom:111: PresentationConnection connection); On 2017/01/06 at 11:54:22, dcheng wrote: > ...

3 years, 11 months ago (2017-01-10 23:05:34 UTC) #25

https://codereview.chromium.org/2477573002/diff/220001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/presentation/presentation.mojom
(right):

https://codereview.chromium.org/2477573002/diff/220001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/presentation.mojom:111:
PresentationConnection connection);
On 2017/01/06 at 11:54:22, dcheng wrote:
> As far as I can tell, the goal is a 1:1 relationship of PresentationConnection
objects: we only need to associate a PresentationConnection in the renderer
starting the presentation with the PresentationConnection object in the
offscreen presentation. So my only big suggestion is to try to remove
SetTargetConnection() to simplify the exposed API in Mojo. Right now, this
method can be called at anytime, it's harder to understand how different
PresentationConnections can be related.
> 
> Is it possible to pass a PresentationConnection + PresentationConnection& pair
here? In C++, this would translate to a PresentationConnectionPtr and a
PresentationConnectionRequest.
> 
> In C++, in the renderer starting the presentation, we would create the proxy
connection:
>   auto proxy = base::MakeUnique<PresentationConnectionProxy>();
> 
> then we would create PresentationConnectionRequest:
>   auto remote_connection_request = proxy->MakeRemoteRequest();
> 
> and then we pass both the interface pointer to the created proxy, as well as
the interface request that the remote end will connect:
> 
>   service->SetPresentationConnection(info, proxy->Bind(),
>       std::move(remote_connection_request));
> 
> On the browser side, the |remote_connection_request| is passed to the
offscreen presentation, which binds it to the PresentationConnection impl it
creates, completing the connection.
> 
> The MakeRemoteRequest() helper implementation would look like this:
> PresentationConnectionRequest
> PresentationConnectionProxy::MakeRemoteRequest() {
>   DCHECK(!target_connection_)
>       << "target_connection_ should only be bound once.";
>   return mojo::MakeRequest(&target_connection_);
> }

I agree it would be slightly better to have an API where it's clear that
connection passing happens once.  However it comes at the expense of adding a
connection request struct to the API, which is really more general than we need
to implement offscreen presentations.

I think there are other ways to solve this.  For example PresentationSessionInfo
could have an optional PresentationConnectionPtr for the target connection
(which could be set on the callback to StartSession, instead of through a
separate method).  However, I don't think additional bike shedding is going to
help much, and effort is better spent on documenting the current solution.

https://codereview.chromium.org/2477573002/diff/300001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2477573002/diff/300001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:3163:
DCHECK(web_contents->GetBrowserContext()->IsOffTheRecord());
If you're converting this back to a DCHECK, then please document very clearly in
content_browser_client.h that only WebContents created as offscreen
presentations should be passed to this API (i.e. the comment you have above).

In effect, it's a private API for our offscreen presentation implementation,
just happens to be exposed in content_browser_client.h.

zhaobin

https://codereview.chromium.org/2477573002/diff/300001/chrome/browser/chrome_content_browser_client.cc File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2477573002/diff/300001/chrome/browser/chrome_content_browser_client.cc#newcode3163 chrome/browser/chrome_content_browser_client.cc:3163: DCHECK(web_contents->GetBrowserContext()->IsOffTheRecord()); On 2017/01/10 23:05:34, mark a. foltz wrote: > ...

3 years, 11 months ago (2017-01-11 03:13:37 UTC) #26

dcheng

mojo lgtm https://codereview.chromium.org/2477573002/diff/220001/third_party/WebKit/public/platform/modules/presentation/presentation.mojom File third_party/WebKit/public/platform/modules/presentation/presentation.mojom (right): https://codereview.chromium.org/2477573002/diff/220001/third_party/WebKit/public/platform/modules/presentation/presentation.mojom#newcode111 third_party/WebKit/public/platform/modules/presentation/presentation.mojom:111: PresentationConnection connection); On 2017/01/10 23:05:33, mark a. ...

3 years, 11 months ago (2017-01-11 09:22:18 UTC) #27

mojo lgtm

https://codereview.chromium.org/2477573002/diff/220001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/presentation/presentation.mojom
(right):

https://codereview.chromium.org/2477573002/diff/220001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/presentation.mojom:111:
PresentationConnection connection);
On 2017/01/10 23:05:33, mark a. foltz wrote:
> On 2017/01/06 at 11:54:22, dcheng wrote:
> > As far as I can tell, the goal is a 1:1 relationship of
PresentationConnection
> objects: we only need to associate a PresentationConnection in the renderer
> starting the presentation with the PresentationConnection object in the
> offscreen presentation. So my only big suggestion is to try to remove
> SetTargetConnection() to simplify the exposed API in Mojo. Right now, this
> method can be called at anytime, it's harder to understand how different
> PresentationConnections can be related.
> > 
> > Is it possible to pass a PresentationConnection + PresentationConnection&
pair
> here? In C++, this would translate to a PresentationConnectionPtr and a
> PresentationConnectionRequest.
> > 
> > In C++, in the renderer starting the presentation, we would create the proxy
> connection:
> >   auto proxy = base::MakeUnique<PresentationConnectionProxy>();
> > 
> > then we would create PresentationConnectionRequest:
> >   auto remote_connection_request = proxy->MakeRemoteRequest();
> > 
> > and then we pass both the interface pointer to the created proxy, as well as
> the interface request that the remote end will connect:
> > 
> >   service->SetPresentationConnection(info, proxy->Bind(),
> >       std::move(remote_connection_request));
> > 
> > On the browser side, the |remote_connection_request| is passed to the
> offscreen presentation, which binds it to the PresentationConnection impl it
> creates, completing the connection.
> > 
> > The MakeRemoteRequest() helper implementation would look like this:
> > PresentationConnectionRequest
> > PresentationConnectionProxy::MakeRemoteRequest() {
> >   DCHECK(!target_connection_)
> >       << "target_connection_ should only be bound once.";
> >   return mojo::MakeRequest(&target_connection_);
> > }
> 
> I agree it would be slightly better to have an API where it's clear that
> connection passing happens once.  However it comes at the expense of adding a
> connection request struct to the API, which is really more general than we
need
> to implement offscreen presentations.
> 
> I think there are other ways to solve this.  For example
PresentationSessionInfo
> could have an optional PresentationConnectionPtr for the target connection
> (which could be set on the callback to StartSession, instead of through a
> separate method).  However, I don't think additional bike shedding is going to
> help much, and effort is better spent on documenting the current solution.
> 

I'm not familiar enough with how the presentation mojo interfaces are setup.
This suggestion does sound better to me, but I'm OK with this CL as-is as well.
If you don't mind CCing me in followups, I'm a bit curious how this will end up
looking.

zhaobin

The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run

3 years, 11 months ago (2017-01-11 19:29:55 UTC) #28