[Presentation API] Additional plumbing for PresentationServiceImpl.

[Presentation API] Additional plumbing for PresentationServiceImpl.

Implemented PSImpl::OnScreenAvailabilityListenerRemoved() by hooking
it up to PSDelegate.

Added the following functions to the PSDelegate interface:
(1) SetDefaultPresentationUrl
(2) StartSession
(3) JoinSession

Implemented (1), (2), (3) in PSImpl.

For (1), PSImpl contains logic to "transfer the callback" -- that is,
if default presentation URL changes after a device availability
listener was added, the listener will continue receiving device updates
for the new default presentation URL.

Note that because (2) and (3) takes callbacks back into PSImpl, and
that it could be possible for PSImpl to have been destroyed at the time
of callback invocation, the callbacks are bound using WeakPtr<PsImpl>.

Added C++ data types in content/public/browser that corresponds to
PresentationSessionInfo/PresentationError in mojom. Added mojo
converter templates + tests.

Also removed the PresentationSession field from PresentationSessionInfo
as PresentationSession is a mojo interface. It's done to separate
data structs from interfaces.

BUG=459001
Committed: https://crrev.com/271b9ef3cc86d5c038ff97b3a28ab437eeacbcc3
Cr-Commit-Position: refs/heads/master@{#321195}

[imcheng, 2015-03-10 00:01:25]

Patchset #4 (id:60001) has been deleted

[imcheng, 2015-03-10 00:03:46]

imcheng@chromium.org changed reviewers:
+ avayvod@chromium.org, avi@chromium.org, imcheng@chromium.org,
mfoltz@chromium.org

[imcheng, 2015-03-10 00:03:46]

Hi Avi, you helped review content/public/browser interfaces for Presentation API
before. I would appreciate it if you can do it again.

Anton / Mark, please review everything.

Thank you.

[Avi (use Gerrit), 2015-03-10 00:36:56]

API LGTM

[whywhat, 2015-03-10 17:05:05]

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

https://codereview.chromium.org/979413002/diff/100001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:76: // Presentation
URL.
BTW, are we going to support this situation for the 1UA case?
We don't have the DPU in the spec (yet) and certainly don't want to require it
from the websites for the availability to work.

https://codereview.chromium.org/979413002/diff/100001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:107: const std::string
presentation_url_str = default_presentation_url_;
It seems like you could eliminate this whole paragraph and just pass
default_presentation_url_ to the find() method (the empty string won't be found
anyway). You don't use presentation_url_str anywhere afterwards as far as I can
see.

https://codereview.chromium.org/979413002/diff/100001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:127: return;
VLOG?

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

https://codereview.chromium.org/979413002/diff/100001/content/browser/present...
content/browser/presentation/presentation_service_impl.h:183: // Additional
StartSession/JoinSession calls are ignored while this is
Is this a temporary limitation? What if the frame wants to start two
presentations (possibly with two different URLs/IDs but possibly to use two
screens)?

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

https://codereview.chromium.org/979413002/diff/100001/content/browser/present...
content/browser/presentation/presentation_service_impl_unittest.cc:371:
EXPECT_TRUE(Mock::VerifyAndClearExpectations(&mock_delegate_));
Shouldn't this be called (or even not called) after the second
SetDefaultPresentatilUrl call to verify that the delegate is actually called
only once?

https://codereview.chromium.org/979413002/diff/100001/content/browser/present...
content/browser/presentation/presentation_service_impl_unittest.cc:379:
EXPECT_CALL(mock_delegate_, SetDefaultPresentationUrl(_, _, Eq(url)))
I don't think you need this EXPECT_CALL here - you already test this
functionality in the first test case. No need to test it in every test case.

https://codereview.chromium.org/979413002/diff/100001/content/browser/present...
content/browser/presentation/presentation_service_impl_unittest.cc:383: // Same
URL as before; no-ops.
The comment seems to be irrelevant to the actual test.

https://codereview.chromium.org/979413002/diff/100001/content/browser/present...
content/browser/presentation/presentation_service_impl_unittest.cc:387:
.Times(1);
The test doesn't show there's any difference between setting a different
non-empty DPU vs an empty DPU. Is there anything else this test could check?

https://codereview.chromium.org/979413002/diff/100001/content/browser/present...
content/browser/presentation/presentation_service_impl_unittest.cc:403:
.WillOnce(SaveArg<4>(&success_cb));
Could you use InvokeArgument<4> here instead?

https://codereview.chromium.org/979413002/diff/100001/content/browser/present...
content/browser/presentation/presentation_service_impl_unittest.cc:463:
TEST_F(PresentationServiceImplTest, StartSessionInProgress) {
Perhaps add the same test case for JoinSession after JoinSession and
combinations (Join after Start and vice versa?) Could be in the same test case I
guess.

https://codereview.chromium.org/979413002/diff/100001/content/common/presenta...
File content/common/presentation/presentation_service.mojom (left):

https://codereview.chromium.org/979413002/diff/100001/content/common/presenta...
content/common/presentation/presentation_service.mojom:10: PresentationSession
session;
Why removing this?

[imcheng, 2015-03-10 18:31:16]

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

https://codereview.chromium.org/979413002/diff/100001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:76: // Presentation
URL.
On 2015/03/10 17:05:04, whywhat wrote:
> BTW, are we going to support this situation for the 1UA case?
> We don't have the DPU in the spec (yet) and certainly don't want to require it
> from the websites for the availability to work.

Maybe there's something I don't understand here.

When obtaining screen availability, we will need a presentation URL to work
with. It either comes from the DPU (if set) or provided as |presentation_url|
parameter in this function. If neither one exists, how would the media router
know how to filter out inapplicable devices? Also, PSDispatcher is currently
always passing in an empty string here. Does that need to be fixed?

https://codereview.chromium.org/979413002/diff/100001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:107: const std::string
presentation_url_str = default_presentation_url_;
On 2015/03/10 17:05:04, whywhat wrote:
> It seems like you could eliminate this whole paragraph and just pass
> default_presentation_url_ to the find() method (the empty string won't be
found
> anyway). You don't use presentation_url_str anywhere afterwards as far as I
can
> see.

Done. But shouldn't we support removing listener by presentation URL?

https://codereview.chromium.org/979413002/diff/100001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:127: return;
On 2015/03/10 17:05:04, whywhat wrote:
> VLOG?

Done.

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

https://codereview.chromium.org/979413002/diff/100001/content/browser/present...
content/browser/presentation/presentation_service_impl.h:183: // Additional
StartSession/JoinSession calls are ignored while this is
On 2015/03/10 17:05:04, whywhat wrote:
> Is this a temporary limitation? What if the frame wants to start two
> presentations (possibly with two different URLs/IDs but possibly to use two
> screens)?

For StartSession, we can only process one at a time since it opens up a dialog
in Chrome. There's other ways to deal with additional StartSession calls while
one is already in progress though -- perhaps we should queue up the StartSession
requests instead of ignoring?


For JoinSession, I think it's possible to have multiple of these in progress. We
can use a map to keep track of join requests. We can use some generated key to
make sure we match up the callbacks with the requests correctly. WDYT?

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

https://codereview.chromium.org/979413002/diff/100001/content/browser/present...
content/browser/presentation/presentation_service_impl_unittest.cc:371:
EXPECT_TRUE(Mock::VerifyAndClearExpectations(&mock_delegate_));
On 2015/03/10 17:05:04, whywhat wrote:
> Shouldn't this be called (or even not called) after the second
> SetDefaultPresentatilUrl call to verify that the delegate is actually called
> only once?

Of course, I added that after the second call.

https://codereview.chromium.org/979413002/diff/100001/content/browser/present...
content/browser/presentation/presentation_service_impl_unittest.cc:379:
EXPECT_CALL(mock_delegate_, SetDefaultPresentationUrl(_, _, Eq(url)))
On 2015/03/10 17:05:04, whywhat wrote:
> I don't think you need this EXPECT_CALL here - you already test this
> functionality in the first test case. No need to test it in every test case.

I need the EXPECT_CALL to satisfy the mock. Otherwise I get spammy warning
messages about unexpected calls.

https://codereview.chromium.org/979413002/diff/100001/content/browser/present...
content/browser/presentation/presentation_service_impl_unittest.cc:383: // Same
URL as before; no-ops.
On 2015/03/10 17:05:05, whywhat wrote:
> The comment seems to be irrelevant to the actual test.

Removed.

https://codereview.chromium.org/979413002/diff/100001/content/browser/present...
content/browser/presentation/presentation_service_impl_unittest.cc:387:
.Times(1);
On 2015/03/10 17:05:05, whywhat wrote:
> The test doesn't show there's any difference between setting a different
> non-empty DPU vs an empty DPU. Is there anything else this test could check?

How about checking default_presentation_url_ field is empty?

https://codereview.chromium.org/979413002/diff/100001/content/browser/present...
content/browser/presentation/presentation_service_impl_unittest.cc:403:
.WillOnce(SaveArg<4>(&success_cb));
On 2015/03/10 17:05:04, whywhat wrote:
> Could you use InvokeArgument<4> here instead?

I tried this and unfortunately it does not work. InvokeArgument requires the
argument to have a call operator, which does not exist in base::Callback.

We could define our own action template like this example:
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...

but it seems like overkill to me. Thoughts?

https://codereview.chromium.org/979413002/diff/100001/content/browser/present...
content/browser/presentation/presentation_service_impl_unittest.cc:463:
TEST_F(PresentationServiceImplTest, StartSessionInProgress) {
On 2015/03/10 17:05:04, whywhat wrote:
> Perhaps add the same test case for JoinSession after JoinSession and
> combinations (Join after Start and vice versa?) Could be in the same test case
I
> guess.

I can add a test for multiple join sessions in progress once it's clarified.

https://codereview.chromium.org/979413002/diff/100001/content/common/presenta...
File content/common/presentation/presentation_service.mojom (left):

https://codereview.chromium.org/979413002/diff/100001/content/common/presenta...
content/common/presentation/presentation_service.mojom:10: PresentationSession
session;
On 2015/03/10 17:05:05, whywhat wrote:
> Why removing this?

I don't think we should include an service interface as a field in the struct.
Instead shouldn't we define the PresentationSession methods to take a
PresentationSessionInfo struct?

[whywhat, 2015-03-10 23:20:23]

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

https://codereview.chromium.org/979413002/diff/100001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:76: // Presentation
URL.
On 2015/03/10 18:31:15, imcheng1 wrote:
> On 2015/03/10 17:05:04, whywhat wrote:
> > BTW, are we going to support this situation for the 1UA case?
> > We don't have the DPU in the spec (yet) and certainly don't want to require
it
> > from the websites for the availability to work.
> 
> Maybe there's something I don't understand here.
> 
> When obtaining screen availability, we will need a presentation URL to work
> with. It either comes from the DPU (if set) or provided as |presentation_url|
> parameter in this function. If neither one exists, how would the media router
> know how to filter out inapplicable devices? Also, PSDispatcher is currently
> always passing in an empty string here. Does that need to be fixed?

I believe that the absence of DPU from the spec means: search for universal
screens (e.g. wired/wireless monitors we could mirror the presentation to;
Presentation API remote screens that can render any URL without limitations). In
the code absence of DPU currently means - no screens available.

I'm working on a CL that would parse the <link> tags to determine DPU. We would
pass the same DPU to GetScreenAvailability() for now since the API doesn't have
a way to specify a URL for filtering when requesting screen availability.

https://codereview.chromium.org/979413002/diff/100001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:107: const std::string
presentation_url_str = default_presentation_url_;
On 2015/03/10 18:31:15, imcheng1 wrote:
> On 2015/03/10 17:05:04, whywhat wrote:
> > It seems like you could eliminate this whole paragraph and just pass
> > default_presentation_url_ to the find() method (the empty string won't be
> found
> > anyway). You don't use presentation_url_str anywhere afterwards as far as I
> can
> > see.
> 
> Done. But shouldn't we support removing listener by presentation URL?

Yes, but we don't pass a URL there yet. Let's fix it when we do pass it.
I assume your intention was something like:

void PresentationServiceImpl::OnScreenAvailabilityListenerRemoved(const
std::string& presentation_url) {
  if (presentation_url.empty())
    presentation_url = default_presentation_url_;

?
}

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

https://codereview.chromium.org/979413002/diff/100001/content/browser/present...
content/browser/presentation/presentation_service_impl_unittest.cc:387:
.Times(1);
On 2015/03/10 18:31:16, imcheng1 wrote:
> On 2015/03/10 17:05:05, whywhat wrote:
> > The test doesn't show there's any difference between setting a different
> > non-empty DPU vs an empty DPU. Is there anything else this test could check?
> 
> How about checking default_presentation_url_ field is empty?

Hm, in the non-empty case, the default_presentation_url_ would be non-empty,
right. So you would check it's the same value as was passed in both cases (which
is a good expectation to add I guess).

What's different in the implementation is juggling the callback for the DPU I
guess, so you could check there's no pending callback if DPU is set to empty,
but there's one if it's set to non-empty.

Although given the first comment discussion, it might not be true anymore.

https://codereview.chromium.org/979413002/diff/100001/content/browser/present...
content/browser/presentation/presentation_service_impl_unittest.cc:403:
.WillOnce(SaveArg<4>(&success_cb));
On 2015/03/10 18:31:16, imcheng1 wrote:
> On 2015/03/10 17:05:04, whywhat wrote:
> > Could you use InvokeArgument<4> here instead?
> 
> I tried this and unfortunately it does not work. InvokeArgument requires the
> argument to have a call operator, which does not exist in base::Callback.
> 
> We could define our own action template like this example:
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
> 
> but it seems like overkill to me. Thoughts?

I think you should leave it as it is then. Sorry my suggestion didn't work.

https://codereview.chromium.org/979413002/diff/100001/content/common/presenta...
File content/common/presentation/presentation_service.mojom (left):

https://codereview.chromium.org/979413002/diff/100001/content/common/presenta...
content/common/presentation/presentation_service.mojom:10: PresentationSession
session;
On 2015/03/10 18:31:16, imcheng1 wrote:
> On 2015/03/10 17:05:05, whywhat wrote:
> > Why removing this?
> 
> I don't think we should include an service interface as a field in the struct.
> Instead shouldn't we define the PresentationSession methods to take a
> PresentationSessionInfo struct?

Which PresentationSession do you mean? The JS interface?

[mark a. foltz, 2015-03-11 01:08:19]

A few initial comments - I will want to take another look.

Is there any opportunity for a screen availability event to get dropped while
the callbacks are being transferred?

I would like to see if there is a design that does not require transferring
callbacks - maybe by keying them by an identifier for the client/RPH instead of
the URL?

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

https://codereview.chromium.org/979413002/diff/140001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:29: VLOG(2) <<
"PresentationServiceImpl: "
Is VLOG() necessary for these messages, or would DVLOG() be sufficient?   They
look like they are for developer debugging.

https://codereview.chromium.org/979413002/diff/140001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:76: // Presentation
URL.
In this case we will be falling back on "1-UA" mode, i.e. offscreen tab
rendering.

Since the media source patch is landing soon, maybe we can define a default
media source for offscreen content to populate the screen availability query.

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

https://codereview.chromium.org/979413002/diff/140001/content/browser/present...
content/browser/presentation/presentation_service_impl.h:184: // set.
Normally I wouldn't expect overlapping calls, but not always.  What is the
behavior the client will see in this scenario:

Promise p1 = joinSession(URL).then(...)
Promise p2 = startSession(URL)

Will p2 never resolve, or reject...?

[mark a. foltz, 2015-03-11 23:55:57]

The tracking of state for each presentation URL seems a little complex.  Would a
simpler design do the following:

1) Track all state (including the URL itself) and the callbacks for each URL
passed to the service in a single struct, let's call it AvailabilityDelegate

2) Inject a factory from the caller to construct a new AD when the URL is set or
changed

3) If the url changes, delete the old AD and ask the factory to new up another
AD

What do you think?

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

https://codereview.chromium.org/979413002/diff/140001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:104: if (!delegate_)
When could this be null?  Can we enforce a non-null delegate_ in the ctor?

It seems like there are null-checks for delegate_ throughout the file, but it's
inconsistent.  It would be simpler not to have them at all.

https://codereview.chromium.org/979413002/diff/140001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:155:
start_or_join_session_cb_.reset(new NewSessionMojoCallback(callback));
All the two bound callbacks above do is invoke the start_or_join_session_cb_.

What if we just new'ed up a NewSessionMojoCallback and/or ErrorMojoCallback and
passed ownership to delegate_, who would run them directly (instead of tracking
them via a member in the PresentationServiceImpl)?

https://codereview.chromium.org/979413002/diff/140001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:175: 
extra newline

https://codereview.chromium.org/979413002/diff/140001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:183:
default_presentation_url_ = presentation_url;
Does this class also need to know the default_presentation_url_?

As I read this it's hard to know whose job it is to keep track - this object or
the delegate.

https://codereview.chromium.org/979413002/diff/140001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:213: // will continue
to work.
Are we sure that the callback that registered for the old URL wants to be
invoked for the new URL?

[imcheng, 2015-03-12 20:03:41]

Thanks for the reviews. I have left a question in the comments regarding the
behavior of listener + DPU. I have made changes according to most comments in
the patch set that I just uploaded. For the next patch set, I want to simplify
PSImpl so its model is consistent with PSDispatcher and also handle overlapping
Start/JoinSession calls.


> Is there any opportunity for a screen availability event to 
> get dropped while
> the callbacks are being transferred?

The event will not get dropped since the transfer happens on the same thread as
availability event firing from the media router.

> I would like to see if there is a design that does not 
> require transferring
> callbacks - maybe by keying them by an identifier for the 
> client/RPH instead of the URL?

Currently PSImpl is built to support multiple listeners for multiple
presentation URLs per frame. Since each presentation URL has their own
callbacks. A callback transfer is necessary when DPU changes.

But, I think the current availability listener model between PSDispatcher and
PSImpl is a little bit confusing -- PSDispatcher currently does not keep track
of callbacks per presentation URL and neither does the presentation API. So in
theory, we could avoid the transfer if PSImpl only kept one callback instead of
one callback per presentation URL. Such an arrangement would appear no different
to PSDispatcher. Either way, it would result in incorrect behavior if we
installed more than one listener in PSImpl.

If we plan to support listeners for multiple URLs in the future, perhaps we
should make PSDispatcher be aware of the different URLs. This requires change to
mojo availability callback to also return the presentation url. This requires
some more thought on the interaction between PSDispatcher and
WebPresentationController, since I assume WebPresentationController is currently
only bound to 1 URL and these two objects are 1:1.

> The tracking of state for each presentation URL seems a little complex.  Would
a
simpler design do the following:

> 1) Track all state (including the URL itself) and the callbacks for each URL
passed to the service in a single struct, let's call it AvailabilityDelegate

> 2) Inject a factory from the caller to construct a new AD when the URL is set
or
changed

> 3) If the url changes, delete the old AD and ask the factory to new up another
AD

> What do you think?


This is actually pretty similar to what I have right now (minus the factory). We
have ScreenAvailabilityContext to keep track of state per URL and PSImpl has a
map from Presentation URLs to ScreenAvailabilityContexts.

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

https://codereview.chromium.org/979413002/diff/100001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:76: // Presentation
URL.
On 2015/03/10 23:20:22, whywhat wrote:
> On 2015/03/10 18:31:15, imcheng1 wrote:
> > On 2015/03/10 17:05:04, whywhat wrote:
> > > BTW, are we going to support this situation for the 1UA case?
> > > We don't have the DPU in the spec (yet) and certainly don't want to
require
> it
> > > from the websites for the availability to work.
> > 
> > Maybe there's something I don't understand here.
> > 
> > When obtaining screen availability, we will need a presentation URL to work
> > with. It either comes from the DPU (if set) or provided as
|presentation_url|
> > parameter in this function. If neither one exists, how would the media
router
> > know how to filter out inapplicable devices? Also, PSDispatcher is currently
> > always passing in an empty string here. Does that need to be fixed?
> 
> I believe that the absence of DPU from the spec means: search for universal
> screens (e.g. wired/wireless monitors we could mirror the presentation to;
> Presentation API remote screens that can render any URL without limitations).
In
> the code absence of DPU currently means - no screens available.
> 
> I'm working on a CL that would parse the <link> tags to determine DPU. We
would
> pass the same DPU to GetScreenAvailability() for now since the API doesn't
have
> a way to specify a URL for filtering when requesting screen availability.

Ok, gotcha. We also talked about this briefly in the meeting today, to pass a
generic source to media router in the case of no DPU. I will think about how to
modify this code to make it work for that case.

https://codereview.chromium.org/979413002/diff/100001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:107: const std::string
presentation_url_str = default_presentation_url_;
On 2015/03/10 23:20:22, whywhat wrote:
> On 2015/03/10 18:31:15, imcheng1 wrote:
> > On 2015/03/10 17:05:04, whywhat wrote:
> > > It seems like you could eliminate this whole paragraph and just pass
> > > default_presentation_url_ to the find() method (the empty string won't be
> > found
> > > anyway). You don't use presentation_url_str anywhere afterwards as far as
I
> > can
> > > see.
> > 
> > Done. But shouldn't we support removing listener by presentation URL?
> 
> Yes, but we don't pass a URL there yet. Let's fix it when we do pass it.
> I assume your intention was something like:
> 
> void PresentationServiceImpl::OnScreenAvailabilityListenerRemoved(const
> std::string& presentation_url) {
>   if (presentation_url.empty())
>     presentation_url = default_presentation_url_;
> 
> ?
> }

I see you have added the param to the .mojom in your other patch. I will add the
change here as well and implement similar logic as GetScreenAvailability() here.

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

https://codereview.chromium.org/979413002/diff/100001/content/browser/present...
content/browser/presentation/presentation_service_impl_unittest.cc:387:
.Times(1);
On 2015/03/10 23:20:23, whywhat wrote:
> On 2015/03/10 18:31:16, imcheng1 wrote:
> > On 2015/03/10 17:05:05, whywhat wrote:
> > > The test doesn't show there's any difference between setting a different
> > > non-empty DPU vs an empty DPU. Is there anything else this test could
check?
> > 
> > How about checking default_presentation_url_ field is empty?
> 
> Hm, in the non-empty case, the default_presentation_url_ would be non-empty,
> right. So you would check it's the same value as was passed in both cases
(which
> is a good expectation to add I guess).
> 
> What's different in the implementation is juggling the callback for the DPU I
> guess, so you could check there's no pending callback if DPU is set to empty,
> but there's one if it's set to non-empty.
> 
> Although given the first comment discussion, it might not be true anymore.

Ok, I will add the assertions that you mentioned.

To clarify, given the way the device availability listeners are implemented in
Blink and the current proposed DPU spec, is it logically correct for the PSImpl
to transfer the callback when there's a listener already registered and the DPU
changes?

https://codereview.chromium.org/979413002/diff/100001/content/browser/present...
content/browser/presentation/presentation_service_impl_unittest.cc:403:
.WillOnce(SaveArg<4>(&success_cb));
On 2015/03/10 23:20:23, whywhat wrote:
> On 2015/03/10 18:31:16, imcheng1 wrote:
> > On 2015/03/10 17:05:04, whywhat wrote:
> > > Could you use InvokeArgument<4> here instead?
> > 
> > I tried this and unfortunately it does not work. InvokeArgument requires the
> > argument to have a call operator, which does not exist in base::Callback.
> > 
> > We could define our own action template like this example:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
> > 
> > but it seems like overkill to me. Thoughts?
> 
> I think you should leave it as it is then. Sorry my suggestion didn't work.

Acknowledged.

https://codereview.chromium.org/979413002/diff/100001/content/common/presenta...
File content/common/presentation/presentation_service.mojom (left):

https://codereview.chromium.org/979413002/diff/100001/content/common/presenta...
content/common/presentation/presentation_service.mojom:10: PresentationSession
session;
On 2015/03/10 23:20:23, whywhat wrote:
> On 2015/03/10 18:31:16, imcheng1 wrote:
> > On 2015/03/10 17:05:05, whywhat wrote:
> > > Why removing this?
> > 
> > I don't think we should include an service interface as a field in the
struct.
> > Instead shouldn't we define the PresentationSession methods to take a
> > PresentationSessionInfo struct?
> 
> Which PresentationSession do you mean? The JS interface?

I mean the (currently empty) mojo interface defined in
presentation_session.mojom.
I guess in the PresentationSession interface you can define methods that take a
PresentationSessionInfo object, like PostMessage(PresentationSessionInfo
session, string message) .... Maybe a renaming of these objects/interfaces are
in order? (PresentationSession -> PresentationSessionService,
PresentationSessionInfo -> PresentationSession)

Besides, I'm not sure if there's any benefit to putting a PresentationSession
interface in here. It also prevents me from converting from the
content/public/browser equivalent object to this object since there's no field
that corresponds to "session" here.

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

https://codereview.chromium.org/979413002/diff/140001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:29: VLOG(2) <<
"PresentationServiceImpl: "
On 2015/03/11 01:08:18, mark a. foltz wrote:
> Is VLOG() necessary for these messages, or would DVLOG() be sufficient?   They
> look like they are for developer debugging.

Converted to DVLOG().

https://codereview.chromium.org/979413002/diff/140001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:76: // Presentation
URL.
On 2015/03/11 01:08:18, mark a. foltz wrote:
> In this case we will be falling back on "1-UA" mode, i.e. offscreen tab
> rendering.
> 
> Since the media source patch is landing soon, maybe we can define a default
> media source for offscreen content to populate the screen availability query.

Ok. In that case, we can pass an empty string to PSDelegate which will then use
the default media source.

https://codereview.chromium.org/979413002/diff/140001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:104: if (!delegate_)
On 2015/03/11 23:55:57, mark a. foltz wrote:
> When could this be null?  Can we enforce a non-null delegate_ in the ctor?
> 
> It seems like there are null-checks for delegate_ throughout the file, but
it's
> inconsistent.  It would be simpler not to have them at all.

We should have these null checks because we don't expect all embedders to
implement PSDelegate and hook it up here. I did forget to add null checks in new
 functions defined, so I will add them back.

Another reason is that we don't assume the lifetime of PSDelegate interface.
It's possible for the PSDelegate to be destroyed before this class, in that
case, OnDelegateDestroyed() will be called and delegate_ will be set to nullptr.

https://codereview.chromium.org/979413002/diff/140001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:155:
start_or_join_session_cb_.reset(new NewSessionMojoCallback(callback));
On 2015/03/11 23:55:57, mark a. foltz wrote:
> All the two bound callbacks above do is invoke the start_or_join_session_cb_.
> 
> What if we just new'ed up a NewSessionMojoCallback and/or ErrorMojoCallback
and
> passed ownership to delegate_, who would run them directly (instead of
tracking
> them via a member in the PresentationServiceImpl)?

The problem is that embedders do not have access to content implementation.
Presentation service mojom is a content implementation detail and therefore
cannot be exposed to PSDelegate (which lives in content/public).

https://codereview.chromium.org/979413002/diff/140001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:175: 
On 2015/03/11 23:55:57, mark a. foltz wrote:
> extra newline

Done.

https://codereview.chromium.org/979413002/diff/140001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:183:
default_presentation_url_ = presentation_url;
On 2015/03/11 23:55:57, mark a. foltz wrote:
> Does this class also need to know the default_presentation_url_?
> 
> As I read this it's hard to know whose job it is to keep track - this object
or
> the delegate.

Yes, this class needs to know the DPU. Both this class and delegate holds a copy
of the current DPU. I thought letting the delegate hold the DPU and exposing a
GetDPU() in PSDelegate interface, but I think it's a pretty awkward interface
since PSImpl is the one that sets DPU on the delegate. And it's also strange to
let this class hold the DPU and expose GetDPU() in PSDelegate::Observer.
Additionally, I want to avoid introducing unnecessary interfaces to PSDelegate.

https://codereview.chromium.org/979413002/diff/140001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:213: // will continue
to work.
On 2015/03/11 23:55:57, mark a. foltz wrote:
> Are we sure that the callback that registered for the old URL wants to be
> invoked for the new URL?

That would depend on the blink implementation. I believe the callback is not
bound to any particular url so it should be safe to transfer. And then I also
want to verify that this transferring logic matches the behavior defined in the
spec. I asked the question to Anton regarding blink implementation in the other
comment.

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

https://codereview.chromium.org/979413002/diff/140001/content/browser/present...
content/browser/presentation/presentation_service_impl.h:184: // set.
On 2015/03/11 01:08:19, mark a. foltz wrote:
> Normally I wouldn't expect overlapping calls, but not always.  What is the
> behavior the client will see in this scenario:
> 
> Promise p1 = joinSession(URL).then(...)
> Promise p2 = startSession(URL)
> 
> Will p2 never resolve, or reject...?
> 

For JoinSession, we should be able to let them overlap. PSImpl would just need
to know how to match up the callback with each JoinSession request. We can
choose to not allow overlapping JoinSession for the same presentation id (i.e.
if PSImpl matches callbacks by presentation ID)

For StartSession, since it brings up a dialog, it does not make sense to have
overlapping calls. We can queue up excess StartSession calls if there's one in
progress. Once the first one resolves/rejects, we can process the next request
in the queue and bring up the dialog.

I can make this change in next patch set if there are no objections.

[imcheng, 2015-03-12 23:23:03]

Patchset #9 (id:180001) has been deleted

[imcheng, 2015-03-12 23:24:29]

Updated the patch.

- Added handling of overlapping Start/JoinSession requests. For StartSession we
will queue them up and execute one by one. For JoinSession, overlapping requests
are supported.
- Added handling of default_presentation_id and passing a copy of it to
PSDelegate.
- Added some comments to content/public interfaces regarding empty presentation
URL => 1-UA mode.

PTAL.

[mark a. foltz, 2015-03-13 01:39:13]

lgtm

Your explanation makes sense but I would like a short whiteboard session to
visualize it a bit better.

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

https://codereview.chromium.org/979413002/diff/200001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:31: <<
render_frame_host_->GetProcess()->GetID() << ", "
Indentation of this line and the one following needs to be fixed

https://codereview.chromium.org/979413002/diff/200001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:62: <<
render_frame_host_->GetProcess()->GetID() << ", "
Indentation

https://codereview.chromium.org/979413002/diff/200001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:74: auto it =
availability_contexts_.find(presentation_url_str);
Can you factor out a method GetOrCreaeteAvailabiltyContext to make this method
shorter and more readable?

https://codereview.chromium.org/979413002/diff/200001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:98: auto it =
availability_contexts_.find(presentation_url_str);
Factor out method GetAvailabilityContext(str)?

https://codereview.chromium.org/979413002/diff/200001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:102:
delegate_->RemoveScreenAvailabilityListener(
Is the delegate_ unique to this instance of the PresentationServiceImpl?  If so,
can it keep the RFH's process and routing ids as members instead of passing them
every time?

https://codereview.chromium.org/979413002/diff/200001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:117: if
(!start_session_in_progress_) {
Is this test the same as queued_start_session_requests_.empty() ?

https://codereview.chromium.org/979413002/diff/200001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:145: if
(queued_start_session_requests_.empty()) {
Ditto

https://codereview.chromium.org/979413002/diff/200001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:217: std::string
old_default_url(default_presentation_url_);
const std::string& ... .get() like you did above?

https://codereview.chromium.org/979413002/diff/200001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:250:
std::make_pair(context->GetPresentationUrl(), context)).first;
The method I suggested above to get or create the context could be used here too

[whywhat, 2015-03-13 17:44:35]

https://codereview.chromium.org/979413002/diff/100001/content/common/presenta...
File content/common/presentation/presentation_service.mojom (left):

https://codereview.chromium.org/979413002/diff/100001/content/common/presenta...
content/common/presentation/presentation_service.mojom:10: PresentationSession
session;
On 2015/03/12 20:03:41, imcheng1 wrote:
> On 2015/03/10 23:20:23, whywhat wrote:
> > On 2015/03/10 18:31:16, imcheng1 wrote:
> > > On 2015/03/10 17:05:05, whywhat wrote:
> > > > Why removing this?
> > > 
> > > I don't think we should include an service interface as a field in the
> struct.
> > > Instead shouldn't we define the PresentationSession methods to take a
> > > PresentationSessionInfo struct?
> > 
> > Which PresentationSession do you mean? The JS interface?
> 
> I mean the (currently empty) mojo interface defined in
> presentation_session.mojom.
> I guess in the PresentationSession interface you can define methods that take
a
> PresentationSessionInfo object, like PostMessage(PresentationSessionInfo
> session, string message) .... Maybe a renaming of these objects/interfaces are
> in order? (PresentationSession -> PresentationSessionService,
> PresentationSessionInfo -> PresentationSession)
> 
> Besides, I'm not sure if there's any benefit to putting a PresentationSession
> interface in here. It also prevents me from converting from the
> content/public/browser equivalent object to this object since there's no field
> that corresponds to "session" here.

Hm, the PresentationDispatcher would keep the PresentationSessionPtr as a form
of unique id to the session. Just a URL and ID is not enough if the frame starts
two sessions with the same URL and ID. This pointer to the remote interface
would be unique though. Otherwise I think we need to add some extra session_id
to the info so that PresentationService could distinguish between the sessions.
Then the PresentationSession methods could go into PresentationService and take
the session id as an extra argument (e.g. session.postMessage() would translate
into presentation_service_->PostMessage(session_id, message).

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

https://codereview.chromium.org/979413002/diff/200001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:117: if
(!start_session_in_progress_) {
On 2015/03/13 01:39:12, mark a. foltz wrote:
> Is this test the same as queued_start_session_requests_.empty() ?

I think one can have one session being started while the queue is empty.
I'd also like to get rid of the flag though: maybe queue every session in
StartSession/JoinSession and remove the front one in
OnStartOrJoinSessionCallback?

https://codereview.chromium.org/979413002/diff/200001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:161:
start_session_in_progress_ = true;
if called from HandleQueueStartSessionRequests, it's already true, right?

https://codereview.chromium.org/979413002/diff/200001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:180: if
(is_start_session) {
No need for braces for one line if branches.

https://codereview.chromium.org/979413002/diff/200001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:193:
HandleQueuedStartSessionRequests();
ditto

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

https://codereview.chromium.org/979413002/diff/200001/content/browser/present...
content/browser/presentation/presentation_service_impl.h:147: // the first one
in the queue and remove it from the queue.
nit: s/remove/removes

https://codereview.chromium.org/979413002/diff/200001/content/browser/present...
content/browser/presentation/presentation_service_impl.h:188:
ScreenAvailabilityMojoCallback* callback() const {
nit: I think since it's not a plain and simple "return callback_;" it should be
called GetCallback() or something like that.

https://codereview.chromium.org/979413002/diff/200001/content/renderer/presen...
File content/renderer/presentation/presentation_dispatcher.cc (right):

https://codereview.chromium.org/979413002/diff/200001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.cc:59:
DoUpdateAvailableChangeWatched(std::string(), watched);
note: string() should be replaced with GetPresentationURLFromFrame() once you
merge my CL

[imcheng, 2015-03-13 23:36:46]

https://codereview.chromium.org/979413002/diff/100001/content/common/presenta...
File content/common/presentation/presentation_service.mojom (left):

https://codereview.chromium.org/979413002/diff/100001/content/common/presenta...
content/common/presentation/presentation_service.mojom:10: PresentationSession
session;
On 2015/03/13 17:44:34, whywhat wrote:
> On 2015/03/12 20:03:41, imcheng1 wrote:
> > On 2015/03/10 23:20:23, whywhat wrote:
> > > On 2015/03/10 18:31:16, imcheng1 wrote:
> > > > On 2015/03/10 17:05:05, whywhat wrote:
> > > > > Why removing this?
> > > > 
> > > > I don't think we should include an service interface as a field in the
> > struct.
> > > > Instead shouldn't we define the PresentationSession methods to take a
> > > > PresentationSessionInfo struct?
> > > 
> > > Which PresentationSession do you mean? The JS interface?
> > 
> > I mean the (currently empty) mojo interface defined in
> > presentation_session.mojom.
> > I guess in the PresentationSession interface you can define methods that
take
> a
> > PresentationSessionInfo object, like PostMessage(PresentationSessionInfo
> > session, string message) .... Maybe a renaming of these objects/interfaces
are
> > in order? (PresentationSession -> PresentationSessionService,
> > PresentationSessionInfo -> PresentationSession)
> > 
> > Besides, I'm not sure if there's any benefit to putting a
PresentationSession
> > interface in here. It also prevents me from converting from the
> > content/public/browser equivalent object to this object since there's no
field
> > that corresponds to "session" here.
> 
> Hm, the PresentationDispatcher would keep the PresentationSessionPtr as a form
> of unique id to the session. Just a URL and ID is not enough if the frame
starts
> two sessions with the same URL and ID. This pointer to the remote interface
> would be unique though. Otherwise I think we need to add some extra session_id
> to the info so that PresentationService could distinguish between the
sessions.
> Then the PresentationSession methods could go into PresentationService and
take
> the session id as an extra argument (e.g. session.postMessage() would
translate
> into presentation_service_->PostMessage(session_id, message).

I want to understand this a little better. Who is responsible for mapping a
PresentationSession web object to a PresentationSessionPtr? Does Blink or
PresentationDispatcher maintain such a mapping?

Note that having PresentationSession web object map to a PresentationSessionPtr
means that mojo channels are created on a per-session basis rather than per
frame. I wonder if we can make the APIs for PresentationSession similar to
PresentationService, i.e. a per-frame service that can operate on a given handle
that identifies a PresentationSession object. That does mean we will have to
introduce a unique session id and make sure we can map a PresentationSession web
object to session id somewhere in the renderer.

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

https://codereview.chromium.org/979413002/diff/200001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:31: <<
render_frame_host_->GetProcess()->GetID() << ", "
On 2015/03/13 01:39:13, mark a. foltz wrote:
> Indentation of this line and the one following needs to be fixed

Done.

https://codereview.chromium.org/979413002/diff/200001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:62: <<
render_frame_host_->GetProcess()->GetID() << ", "
On 2015/03/13 01:39:13, mark a. foltz wrote:
> Indentation

Done.

https://codereview.chromium.org/979413002/diff/200001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:74: auto it =
availability_contexts_.find(presentation_url_str);
On 2015/03/13 01:39:13, mark a. foltz wrote:
> Can you factor out a method GetOrCreaeteAvailabiltyContext to make this method
> shorter and more readable?

Done.

https://codereview.chromium.org/979413002/diff/200001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:98: auto it =
availability_contexts_.find(presentation_url_str);
On 2015/03/13 01:39:13, mark a. foltz wrote:
> Factor out method GetAvailabilityContext(str)?

Actually, I need the iterator here since erase by iterator is more efficient.

https://codereview.chromium.org/979413002/diff/200001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:102:
delegate_->RemoveScreenAvailabilityListener(
On 2015/03/13 01:39:13, mark a. foltz wrote:
> Is the delegate_ unique to this instance of the PresentationServiceImpl?  If
so,
> can it keep the RFH's process and routing ids as members instead of passing
them
> every time?

It is not. It's actually per tab (see line 55-56) so delegate_ is shared for
instances of PresentationServiceImpl (per frame) in the same tab. Chatted
offline. We will keep it as it is for now.

https://codereview.chromium.org/979413002/diff/200001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:117: if
(!start_session_in_progress_) {
On 2015/03/13 17:44:35, whywhat wrote:
> On 2015/03/13 01:39:12, mark a. foltz wrote:
> > Is this test the same as queued_start_session_requests_.empty() ?
> 
> I think one can have one session being started while the queue is empty.
> I'd also like to get rid of the flag though: maybe queue every session in
> StartSession/JoinSession and remove the front one in
> OnStartOrJoinSessionCallback?

Ok, I will make the change as you suggest to get rid of the boolean. Though, in
a way this is more error prone, since we can potentially have a mismatch between
head of queue and the request just processed.

https://codereview.chromium.org/979413002/diff/200001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:145: if
(queued_start_session_requests_.empty()) {
On 2015/03/13 01:39:13, mark a. foltz wrote:
> Ditto

It wasn't the same because there could be a request in progress while queue is
empty.

https://codereview.chromium.org/979413002/diff/200001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:161:
start_session_in_progress_ = true;
On 2015/03/13 17:44:35, whywhat wrote:
> if called from HandleQueueStartSessionRequests, it's already true, right?

Yes.

https://codereview.chromium.org/979413002/diff/200001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:180: if
(is_start_session) {
On 2015/03/13 17:44:35, whywhat wrote:
> No need for braces for one line if branches.

Done.

https://codereview.chromium.org/979413002/diff/200001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:193:
HandleQueuedStartSessionRequests();
On 2015/03/13 17:44:35, whywhat wrote:
> ditto

Done.

https://codereview.chromium.org/979413002/diff/200001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:217: std::string
old_default_url(default_presentation_url_);
On 2015/03/13 01:39:13, mark a. foltz wrote:
> const std::string& ... .get() like you did above?   

Done.

https://codereview.chromium.org/979413002/diff/200001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:250:
std::make_pair(context->GetPresentationUrl(), context)).first;
On 2015/03/13 01:39:13, mark a. foltz wrote:
> The method I suggested above to get or create the context could be used here
too

Done.

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

https://codereview.chromium.org/979413002/diff/200001/content/browser/present...
content/browser/presentation/presentation_service_impl.h:147: // the first one
in the queue and remove it from the queue.
On 2015/03/13 17:44:35, whywhat wrote:
> nit: s/remove/removes

Done.

https://codereview.chromium.org/979413002/diff/200001/content/browser/present...
content/browser/presentation/presentation_service_impl.h:188:
ScreenAvailabilityMojoCallback* callback() const {
On 2015/03/13 17:44:35, whywhat wrote:
> nit: I think since it's not a plain and simple "return callback_;" it should
be
> called GetCallback() or something like that.

Done.

https://codereview.chromium.org/979413002/diff/200001/content/renderer/presen...
File content/renderer/presentation/presentation_dispatcher.cc (right):

https://codereview.chromium.org/979413002/diff/200001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.cc:59:
DoUpdateAvailableChangeWatched(std::string(), watched);
On 2015/03/13 17:44:35, whywhat wrote:
> note: string() should be replaced with GetPresentationURLFromFrame() once you
> merge my CL

Acknowledged.

[whywhat, 2015-03-15 13:48:58]

> I want to understand this a little better. Who is responsible for mapping a
> PresentationSession web object to a PresentationSessionPtr? Does Blink or
> PresentationDispatcher maintain such a mapping?

I think currently Blink gives presentation session url and id to
PresentationDispatcher that has a map from url/id pair to
PresentationSessionPtr. The idea was to avoid having unique session ids.
However, presentation url/id aren't necessarily unique so I think we need unique
session ids to track them. Do you think it still makes sense to have a separate
service for sessions then? If we add session methods to PresentationService, we
could only keep one Mojo channel for everything.

We might want to make session ids unique per browser and even between browser
runs (crashes), see this thread:
https://lists.w3.org/Archives/Public/public-secondscreen/2015Mar/0007.html

We would give the session id back to the Blink PresentationSession object and
that would be passed over to the PresentationService for every session method
call.

> 
> Note that having PresentationSession web object map to a
PresentationSessionPtr
> means that mojo channels are created on a per-session basis rather than per
> frame. I wonder if we can make the APIs for PresentationSession similar to
> PresentationService, i.e. a per-frame service that can operate on a given
handle
> that identifies a PresentationSession object. That does mean we will have to
> introduce a unique session id and make sure we can map a PresentationSession
web
> object to session id somewhere in the renderer.

[imcheng, 2015-03-16 17:51:38]

On 2015/03/15 13:48:58, whywhat wrote:
> > I want to understand this a little better. Who is responsible for mapping a
> > PresentationSession web object to a PresentationSessionPtr? Does Blink or
> > PresentationDispatcher maintain such a mapping?
> 
> I think currently Blink gives presentation session url and id to
> PresentationDispatcher that has a map from url/id pair to
> PresentationSessionPtr. The idea was to avoid having unique session ids.
> However, presentation url/id aren't necessarily unique so I think we need
unique
> session ids to track them. Do you think it still makes sense to have a
separate
> service for sessions then? If we add session methods to PresentationService,
we
> could only keep one Mojo channel for everything.
> 
> We might want to make session ids unique per browser and even between browser
> runs (crashes), see this thread:
> https://lists.w3.org/Archives/Public/public-secondscreen/2015Mar/0007.html
> 
> We would give the session id back to the Blink PresentationSession object and
> that would be passed over to the PresentationService for every session method
> call.
> 

I think it's a good idea to be more explicit about the uniqueness of the session
using session ids and have a centralized service/channel that takes the id for
session related APIs. I don't feel strongly about putting it in a separate
service, and I lean towards using the existing PresentationService to keep
things simple.

For IDs we can use something like base/guid.h to generate "sufficiently" unique
session ids in practice. These unique session ids should only be used between
PresentationSessionClient/PresentationDispatcher and PresentationServiceImpl,
i.e. only at mojo endpoints.


> > 
> > Note that having PresentationSession web object map to a
> PresentationSessionPtr
> > means that mojo channels are created on a per-session basis rather than per
> > frame. I wonder if we can make the APIs for PresentationSession similar to
> > PresentationService, i.e. a per-frame service that can operate on a given
> handle
> > that identifies a PresentationSession object. That does mean we will have to
> > introduce a unique session id and make sure we can map a PresentationSession
> web
> > object to session id somewhere in the renderer.

[whywhat, 2015-03-16 18:12:08]

On 2015/03/16 17:51:38, imcheng1 wrote:
> On 2015/03/15 13:48:58, whywhat wrote:
> > > I want to understand this a little better. Who is responsible for mapping
a
> > > PresentationSession web object to a PresentationSessionPtr? Does Blink or
> > > PresentationDispatcher maintain such a mapping?
> > 
> > I think currently Blink gives presentation session url and id to
> > PresentationDispatcher that has a map from url/id pair to
> > PresentationSessionPtr. The idea was to avoid having unique session ids.
> > However, presentation url/id aren't necessarily unique so I think we need
> unique
> > session ids to track them. Do you think it still makes sense to have a
> separate
> > service for sessions then? If we add session methods to PresentationService,
> we
> > could only keep one Mojo channel for everything.
> > 
> > We might want to make session ids unique per browser and even between
browser
> > runs (crashes), see this thread:
> > https://lists.w3.org/Archives/Public/public-secondscreen/2015Mar/0007.html
> > 
> > We would give the session id back to the Blink PresentationSession object
and
> > that would be passed over to the PresentationService for every session
method
> > call.
> > 
> 
> I think it's a good idea to be more explicit about the uniqueness of the
session
> using session ids and have a centralized service/channel that takes the id for
> session related APIs. I don't feel strongly about putting it in a separate
> service, and I lean towards using the existing PresentationService to keep
> things simple.
> 
> For IDs we can use something like base/guid.h to generate "sufficiently"
unique
> session ids in practice. These unique session ids should only be used between
> PresentationSessionClient/PresentationDispatcher and PresentationServiceImpl,
> i.e. only at mojo endpoints.

SGTM. Will you implement the IDs in this change?

> 
> 
> > > 
> > > Note that having PresentationSession web object map to a
> > PresentationSessionPtr
> > > means that mojo channels are created on a per-session basis rather than
per
> > > frame. I wonder if we can make the APIs for PresentationSession similar to
> > > PresentationService, i.e. a per-frame service that can operate on a given
> > handle
> > > that identifies a PresentationSession object. That does mean we will have
to
> > > introduce a unique session id and make sure we can map a
PresentationSession
> > web
> > > object to session id somewhere in the renderer.

[imcheng, 2015-03-16 19:38:09]

On 2015/03/16 18:12:08, whywhat wrote:
> On 2015/03/16 17:51:38, imcheng1 wrote:
> > On 2015/03/15 13:48:58, whywhat wrote:
> > > > I want to understand this a little better. Who is responsible for
mapping
> a
> > > > PresentationSession web object to a PresentationSessionPtr? Does Blink
or
> > > > PresentationDispatcher maintain such a mapping?
> > > 
> > > I think currently Blink gives presentation session url and id to
> > > PresentationDispatcher that has a map from url/id pair to
> > > PresentationSessionPtr. The idea was to avoid having unique session ids.
> > > However, presentation url/id aren't necessarily unique so I think we need
> > unique
> > > session ids to track them. Do you think it still makes sense to have a
> > separate
> > > service for sessions then? If we add session methods to
PresentationService,
> > we
> > > could only keep one Mojo channel for everything.
> > > 
> > > We might want to make session ids unique per browser and even between
> browser
> > > runs (crashes), see this thread:
> > > https://lists.w3.org/Archives/Public/public-secondscreen/2015Mar/0007.html
> > > 
> > > We would give the session id back to the Blink PresentationSession object
> and
> > > that would be passed over to the PresentationService for every session
> method
> > > call.
> > > 
> > 
> > I think it's a good idea to be more explicit about the uniqueness of the
> session
> > using session ids and have a centralized service/channel that takes the id
for
> > session related APIs. I don't feel strongly about putting it in a separate
> > service, and I lean towards using the existing PresentationService to keep
> > things simple.
> > 
> > For IDs we can use something like base/guid.h to generate "sufficiently"
> unique
> > session ids in practice. These unique session ids should only be used
between
> > PresentationSessionClient/PresentationDispatcher and
PresentationServiceImpl,
> > i.e. only at mojo endpoints.
> 
> SGTM. Will you implement the IDs in this change?
> 

Yes. I uploaded two patches; one is for rebasing, the other one is for adding
session_id. PTAL, thanks!


> > 
> > 
> > > > 
> > > > Note that having PresentationSession web object map to a
> > > PresentationSessionPtr
> > > > means that mojo channels are created on a per-session basis rather than
> per
> > > > frame. I wonder if we can make the APIs for PresentationSession similar
to
> > > > PresentationService, i.e. a per-frame service that can operate on a
given
> > > handle
> > > > that identifies a PresentationSession object. That does mean we will
have
> to
> > > > introduce a unique session id and make sure we can map a
> PresentationSession
> > > web
> > > > object to session id somewhere in the renderer.

[mark a. foltz, 2015-03-17 00:13:52]

On 2015/03/16 19:38:09, imcheng1 wrote:
> On 2015/03/16 18:12:08, whywhat wrote:
> > On 2015/03/16 17:51:38, imcheng1 wrote:
> > > On 2015/03/15 13:48:58, whywhat wrote:
> > > > > I want to understand this a little better. Who is responsible for
> mapping
> > a
> > > > > PresentationSession web object to a PresentationSessionPtr? Does Blink
> or
> > > > > PresentationDispatcher maintain such a mapping?
> > > > 
> > > > I think currently Blink gives presentation session url and id to
> > > > PresentationDispatcher that has a map from url/id pair to
> > > > PresentationSessionPtr. The idea was to avoid having unique session ids.
> > > > However, presentation url/id aren't necessarily unique so I think we
need
> > > unique
> > > > session ids to track them. Do you think it still makes sense to have a
> > > separate
> > > > service for sessions then? If we add session methods to
> PresentationService,
> > > we
> > > > could only keep one Mojo channel for everything.
> > > > 
> > > > We might want to make session ids unique per browser and even between
> > browser
> > > > runs (crashes), see this thread:
> > > >
https://lists.w3.org/Archives/Public/public-secondscreen/2015Mar/0007.html
> > > > 
> > > > We would give the session id back to the Blink PresentationSession
object
> > and
> > > > that would be passed over to the PresentationService for every session
> > method
> > > > call.
> > > > 
> > > 
> > > I think it's a good idea to be more explicit about the uniqueness of the
> > session
> > > using session ids and have a centralized service/channel that takes the id
> for
> > > session related APIs. I don't feel strongly about putting it in a separate
> > > service, and I lean towards using the existing PresentationService to keep
> > > things simple.
> > > 
> > > For IDs we can use something like base/guid.h to generate "sufficiently"
> > unique
> > > session ids in practice. These unique session ids should only be used
> between
> > > PresentationSessionClient/PresentationDispatcher and
> PresentationServiceImpl,
> > > i.e. only at mojo endpoints.
> > 
> > SGTM. Will you implement the IDs in this change?
> > 
> 
> Yes. I uploaded two patches; one is for rebasing, the other one is for adding
> session_id. PTAL, thanks!
> 
> 
> > > 
> > > 
> > > > > 
> > > > > Note that having PresentationSession web object map to a
> > > > PresentationSessionPtr
> > > > > means that mojo channels are created on a per-session basis rather
than
> > per
> > > > > frame. I wonder if we can make the APIs for PresentationSession
similar
> to
> > > > > PresentationService, i.e. a per-frame service that can operate on a
> given
> > > > handle
> > > > > that identifies a PresentationSession object. That does mean we will
> have
> > to
> > > > > introduce a unique session id and make sure we can map a
> > PresentationSession
> > > > web
> > > > > object to session id somewhere in the renderer.

not lgtm

Presentation URL and id should be unique within a given browser context.  This
is necessary for the joinSession() algorithm to work.  The semantics for
startSession() are undefined if there is an existing presentation with the same
URL and id, that is an issue that needs to be posted on github (related to
https://github.com/w3c/presentation-api/issues/39).

If I understand correctly there is one presentation service per browser context,
in that case I don't think we need an additional session id to distinguish them.

Am I missing something?

[imcheng, 2015-03-17 00:52:03]

Reverted the session_id patch set. Chatted offline with mark. This patch has
gotten pretty large so let's try to submit this for now and let's chat about the
session_id issue on wednesday.

[mark a. foltz, 2015-03-18 00:10:58]

lgtm % remaining comments (almost all on documentation)

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

https://codereview.chromium.org/979413002/diff/320001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:69: if (it ==
availability_contexts_.end()) {
Indentation

https://codereview.chromium.org/979413002/diff/320001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:95: return;
If the context can't be created, should we invoke the callback with 'false'?  Is
there any issue with leaving an orphaned and un-run Mojo callback?

https://codereview.chromium.org/979413002/diff/320001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:110:
delegate_->RemoveScreenAvailabilityListener(
Could we ever get into a state where the availability context was gone, but the
listener was still registered with the delegate?  That would leave an orphaned
listener.  What guarantees these are in sync?

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

https://codereview.chromium.org/979413002/diff/320001/content/browser/present...
content/browser/presentation/presentation_service_impl.h:73: class
CONTENT_EXPORT ScreenAvailabilityContext
Is CONTENT_EXPORT only for unit testing?

It is odd that this is a private class of PresentationServiceImpl yet is an
exported symbol.

https://codereview.chromium.org/979413002/diff/320001/content/browser/present...
content/browser/presentation/presentation_service_impl.h:92: // If callback
exists, it will be invoked with |available| and
Add a blank line before to help readability of the long docstring.

https://codereview.chromium.org/979413002/diff/320001/content/browser/present...
content/browser/presentation/presentation_service_impl.h:108: struct
CONTENT_EXPORT StartSessionRequest {
Similar comment about CONTENT_EXPORT.

https://codereview.chromium.org/979413002/diff/320001/content/browser/present...
content/browser/presentation/presentation_service_impl.h:189: // Callbacks from
the embedder as a result of StartSession/JoinSession.
I don't quite follow this comment - this is in content/browser so it is part of
Chrome (and depends on Mojo)? I don't think it can be a callback from a generic
embedder?

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

https://codereview.chromium.org/979413002/diff/320001/content/public/browser/...
content/public/browser/presentation_service_delegate.h:83: // This brings up a
screen list for the user to select a screen to
Actually that's up to the embedder :)

Maybe, "Typically, the embedder will allow the user to select a screen to show
the |presentation_url|."

https://codereview.chromium.org/979413002/diff/320001/content/public/browser/...
content/public/browser/presentation_service_delegate.h:86: //
|presentation_url|: URL of the presentation. If empty, fall back to
presentation_url should be mandatory for all calls to startSession.
It's optional for querying for screen availability.

https://codereview.chromium.org/979413002/diff/320001/content/public/browser/...
content/public/browser/presentation_service_delegate.h:106: //
|presentation_url|: URL of the presentation. If empty, fall back to
Also mandatory for joinSession().

[imcheng, 2015-03-18 00:56:53]

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

https://codereview.chromium.org/979413002/diff/320001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:69: if (it ==
availability_contexts_.end()) {
On 2015/03/18 00:10:58, mark a. foltz wrote:
> Indentation

Done.

https://codereview.chromium.org/979413002/diff/320001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:95: return;
On 2015/03/18 00:10:58, mark a. foltz wrote:
> If the context can't be created, should we invoke the callback with 'false'? 
Is
> there any issue with leaving an orphaned and un-run Mojo callback?

I don't think we should invoke the callback under error conditions. Invoking the
callback is a feedback to PresentationDispatcher to call this function again. If
context consistently fails to be created then we would have a mojo message
constantly being passed back and forth.

https://codereview.chromium.org/979413002/diff/320001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:110:
delegate_->RemoveScreenAvailabilityListener(
On 2015/03/18 00:10:58, mark a. foltz wrote:
> Could we ever get into a state where the availability context was gone, but
the
> listener was still registered with the delegate?  That would leave an orphaned
> listener.  What guarantees these are in sync?
> 

contexts get removed due to 2 reasons:
1) explicitly removed via OnScreenAvailabilityListenerRemoved - we have a
matching RemoveScreenAvailabilityListener() call to delegate to remove the
listener here.
2) removed as a result of frame navigation/destruction/etc., these are handled
by a Reset() call to delegate which should also remove all listeners for the
target frame.

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

https://codereview.chromium.org/979413002/diff/320001/content/browser/present...
content/browser/presentation/presentation_service_impl.h:73: class
CONTENT_EXPORT ScreenAvailabilityContext
On 2015/03/18 00:10:58, mark a. foltz wrote:
> Is CONTENT_EXPORT only for unit testing?
> 
> It is odd that this is a private class of PresentationServiceImpl yet is an
> exported symbol.

Yeah unfortunately this is needed otherwise unit tests won't link on component
builds.

https://codereview.chromium.org/979413002/diff/320001/content/browser/present...
content/browser/presentation/presentation_service_impl.h:92: // If callback
exists, it will be invoked with |available| and
On 2015/03/18 00:10:58, mark a. foltz wrote:
> Add a blank line before to help readability of the long docstring.

Done.

https://codereview.chromium.org/979413002/diff/320001/content/browser/present...
content/browser/presentation/presentation_service_impl.h:108: struct
CONTENT_EXPORT StartSessionRequest {
On 2015/03/18 00:10:58, mark a. foltz wrote:
> Similar comment about CONTENT_EXPORT.

Ditto

https://codereview.chromium.org/979413002/diff/320001/content/browser/present...
content/browser/presentation/presentation_service_impl.h:189: // Callbacks from
the embedder as a result of StartSession/JoinSession.
On 2015/03/18 00:10:58, mark a. foltz wrote:
> I don't quite follow this comment - this is in content/browser so it is part
of
> Chrome (and depends on Mojo)? I don't think it can be a callback from a
generic
> embedder?

I clarified the comments here.

What I mean is that these two functions from PSImpl (content code) are bound as
base::Callbacks and passed to the embedder via the PresentationServiceDelegate
interface to be invoked when the embedder knows whether session create/join
succeeded.

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

https://codereview.chromium.org/979413002/diff/320001/content/public/browser/...
content/public/browser/presentation_service_delegate.h:83: // This brings up a
screen list for the user to select a screen to
On 2015/03/18 00:10:58, mark a. foltz wrote:
> Actually that's up to the embedder :)
> 
> Maybe, "Typically, the embedder will allow the user to select a screen to show
> the |presentation_url|."

Done.

https://codereview.chromium.org/979413002/diff/320001/content/public/browser/...
content/public/browser/presentation_service_delegate.h:86: //
|presentation_url|: URL of the presentation. If empty, fall back to
On 2015/03/18 00:10:58, mark a. foltz wrote:
> presentation_url should be mandatory for all calls to startSession.
> It's optional for querying for screen availability.

Done.

https://codereview.chromium.org/979413002/diff/320001/content/public/browser/...
content/public/browser/presentation_service_delegate.h:106: //
|presentation_url|: URL of the presentation. If empty, fall back to
On 2015/03/18 00:10:58, mark a. foltz wrote:
> Also mandatory for joinSession().

Done.

[imcheng, 2015-03-18 19:23:27]

The CQ bit was checked by imcheng@chromium.org

[imcheng, 2015-03-18 19:23:28]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
mfoltz@chromium.org
Link to the patchset: https://codereview.chromium.org/979413002/#ps360001
(title: "rm presentation_session.mojom")

[commit-bot: I haz the power, 2015-03-18 19:23:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/979413002/360001

[commit-bot: I haz the power, 2015-03-18 19:43:28]

Committed patchset #17 (id:360001)

[commit-bot: I haz the power, 2015-03-18 19:44:29]

Patchset 17 (id:??) landed as
https://crrev.com/271b9ef3cc86d5c038ff97b3a28ab437eeacbcc3
Cr-Commit-Position: refs/heads/master@{#321195}