[PresentationAPI] Implementing start/joinSession from WebPresentationClient to PresentationService.

[PresentationAPI] Implementing start/joinSession from WebPresentationClient to PresentationService.

Added PresentationSession Mojo interface with its client implementation, PresentationSessionDispatcher.
Depends on the Blink change: https://codereview.chromium.org/935103002

BUG=459005
Committed: https://crrev.com/f0dd974b186bbaf4d1e6c5805666f7c0d71ae795
Cr-Commit-Position: refs/heads/master@{#318550}

[whywhat, 2015-02-18 14:43:21]

avayvod@chromium.org changed reviewers:
+ mlamouri@chromium.org

[whywhat, 2015-02-18 14:43:22]

Please, take a first look for a sanity check.

[mlamouri (slow - plz ping), 2015-02-18 15:16:43]

First pass looking at the mojo interface. Let's figure out what to do there then
I will review the rest.

https://codereview.chromium.org/935083002/diff/1/content/common/presentation/...
File content/common/presentation/presentation_service.mojom (right):

https://codereview.chromium.org/935083002/diff/1/content/common/presentation/...
content/common/presentation/presentation_service.mojom:34: bool success,
Maybe the following would match style better:

StartSession(string presentation_url, string? presentation_id)
  => (bool success, PresentationSession? session, string? error, string? url,
string? id);

https://codereview.chromium.org/935083002/diff/1/content/common/presentation/...
content/common/presentation/presentation_service.mojom:38: string? id);
Maybe you could have something more generic here? Like a struct carrying some
data. For example url and id. Maybe event PresentationSession if they can carry
interfaces. The error should probably stay out of that struct. The struct would
be null if there is an error. I would remove the success boolean because it
should be easy for anyone to check the returned value in order to see if it was
a success.

https://codereview.chromium.org/935083002/diff/1/content/common/presentation/...
content/common/presentation/presentation_service.mojom:42: // (no user action
needed) rejoin the known session (i.e. when the page
s/the/a/ ?

https://codereview.chromium.org/935083002/diff/1/content/common/presentation/...
content/common/presentation/presentation_service.mojom:51: string? error);
ditto.

https://codereview.chromium.org/935083002/diff/1/content/common/presentation/...
File content/common/presentation/presentation_session.mojom (right):

https://codereview.chromium.org/935083002/diff/1/content/common/presentation/...
content/common/presentation/presentation_session.mojom:8: };
nit: could you tell `git cl` this is not a copy?

[blundell, 2015-02-18 15:35:15]

blundell@chromium.org changed reviewers:
+ blundell@chromium.org

[blundell, 2015-02-18 15:35:16]

https://codereview.chromium.org/935083002/diff/1/content/renderer/presentatio...
File content/renderer/presentation/presentation_dispatcher.h (right):

https://codereview.chromium.org/935083002/diff/1/content/renderer/presentatio...
content/renderer/presentation/presentation_dispatcher.h:47:
mojo::InterfaceRequest<presentation::PresentationSession> session_request,
Why is this called session_request? The idea is that this is a proxy to the
created session, right?

https://codereview.chromium.org/935083002/diff/1/content/renderer/presentatio...
content/renderer/presentation/presentation_dispatcher.h:60: typedef
std::vector<linked_ptr<PresentationSessionDispatcher>>
Why linked_ptr?

[whywhat, 2015-02-19 16:32:21]

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

[whywhat, 2015-02-19 16:32:22]

PTAL

https://codereview.chromium.org/935083002/diff/1/content/common/presentation/...
File content/common/presentation/presentation_service.mojom (right):

https://codereview.chromium.org/935083002/diff/1/content/common/presentation/...
content/common/presentation/presentation_service.mojom:34: bool success,
On 2015/02/18 15:16:43, Mounir Lamouri wrote:
> Maybe the following would match style better:
> 
> StartSession(string presentation_url, string? presentation_id)
>   => (bool success, PresentationSession? session, string? error, string? url,
> string? id);

Done.

https://codereview.chromium.org/935083002/diff/1/content/common/presentation/...
content/common/presentation/presentation_service.mojom:38: string? id);
On 2015/02/18 15:16:41, Mounir Lamouri wrote:
> Maybe you could have something more generic here? Like a struct carrying some
> data. For example url and id. Maybe event PresentationSession if they can
carry
> interfaces. The error should probably stay out of that struct. The struct
would
> be null if there is an error. I would remove the success boolean because it
> should be easy for anyone to check the returned value in order to see if it
was
> a success.

Done.

https://codereview.chromium.org/935083002/diff/1/content/common/presentation/...
content/common/presentation/presentation_service.mojom:42: // (no user action
needed) rejoin the known session (i.e. when the page
On 2015/02/18 15:16:42, Mounir Lamouri wrote:
> s/the/a/ ?

Done.

https://codereview.chromium.org/935083002/diff/1/content/common/presentation/...
content/common/presentation/presentation_service.mojom:51: string? error);
On 2015/02/18 15:16:43, Mounir Lamouri wrote:
> ditto.

Done.

https://codereview.chromium.org/935083002/diff/1/content/common/presentation/...
File content/common/presentation/presentation_session.mojom (right):

https://codereview.chromium.org/935083002/diff/1/content/common/presentation/...
content/common/presentation/presentation_session.mojom:8: };
On 2015/02/18 15:16:43, Mounir Lamouri wrote:
> nit: could you tell `git cl` this is not a copy?

Done.

https://codereview.chromium.org/935083002/diff/1/content/renderer/presentatio...
File content/renderer/presentation/presentation_dispatcher.h (right):

https://codereview.chromium.org/935083002/diff/1/content/renderer/presentatio...
content/renderer/presentation/presentation_dispatcher.h:47:
mojo::InterfaceRequest<presentation::PresentationSession> session_request,
On 2015/02/18 15:35:16, blundell wrote:
> Why is this called session_request? The idea is that this is a proxy to the
> created session, right?

I misunderstood the concept of passing a Object?& in Mojo. Fixed to be an
InterfacePtr.

https://codereview.chromium.org/935083002/diff/1/content/renderer/presentatio...
content/renderer/presentation/presentation_dispatcher.h:60: typedef
std::vector<linked_ptr<PresentationSessionDispatcher>>
On 2015/02/18 15:35:16, blundell wrote:
> Why linked_ptr?

Forgot about the existence of ScopedPtr I guess.

[mark a. foltz, 2015-02-19 22:59:57]

https://codereview.chromium.org/935083002/diff/40001/content/common/presentat...
File content/common/presentation/presentation_service.mojom (right):

https://codereview.chromium.org/935083002/diff/40001/content/common/presentat...
content/common/presentation/presentation_service.mojom:10: PresentationSession
session;
Are |url| and |id| properties of |session|?  Do we need the *Info wrapper
struct?

https://codereview.chromium.org/935083002/diff/40001/content/common/presentat...
File content/common/presentation/presentation_session.mojom (right):

https://codereview.chromium.org/935083002/diff/40001/content/common/presentat...
content/common/presentation/presentation_session.mojom:8: };
This will have postMessage/onMessage/close() eventually correct?

https://codereview.chromium.org/935083002/diff/40001/content/renderer/present...
File content/renderer/presentation/presentation_session_dispatcher.h (right):

https://codereview.chromium.org/935083002/diff/40001/content/renderer/present...
content/renderer/presentation/presentation_session_dispatcher.h:29: virtual
blink::WebString getUrl();
Should this be const?

https://codereview.chromium.org/935083002/diff/40001/content/renderer/present...
content/renderer/presentation/presentation_session_dispatcher.h:30: virtual
blink::WebString getId();
Ditto

[whywhat, 2015-02-19 23:22:18]

https://codereview.chromium.org/935083002/diff/40001/content/common/presentat...
File content/common/presentation/presentation_service.mojom (right):

https://codereview.chromium.org/935083002/diff/40001/content/common/presentat...
content/common/presentation/presentation_service.mojom:10: PresentationSession
session;
On 2015/02/19 22:59:57, mark a. foltz wrote:
> Are |url| and |id| properties of |session|?  Do we need the *Info wrapper
> struct?

[Mojo]PresentationSession is an interface meaning it can only contain async
methods, not sync properties like structs.
We could I guess add GetUrl/GetId to the interface and call it once we get
OnCreateSession callback before resolving the Promise - we need to have the id
as it's a readonly sync property of the [W3C]PresentationSession spec. I thought
it would be easier to pass
the values once in the OnCreateSession callback and wrapping it in a struct was
suggested by other reviewers.

I assumed that id will not change during session lifetime. Url is not actually
needed by the spec but might be useful if we want to match sessions on the
renderer side.
Since we have an async

https://codereview.chromium.org/935083002/diff/40001/content/common/presentat...
File content/common/presentation/presentation_session.mojom (right):

https://codereview.chromium.org/935083002/diff/40001/content/common/presentat...
content/common/presentation/presentation_session.mojom:8: };
On 2015/02/19 22:59:57, mark a. foltz wrote:
> This will have postMessage/onMessage/close() eventually correct?

Sure. I filed two separate issues for these.

https://codereview.chromium.org/935083002/diff/40001/content/renderer/present...
File content/renderer/presentation/presentation_session_dispatcher.h (right):

https://codereview.chromium.org/935083002/diff/40001/content/renderer/present...
content/renderer/presentation/presentation_session_dispatcher.h:29: virtual
blink::WebString getUrl();
On 2015/02/19 22:59:57, mark a. foltz wrote:
> Should this be const?

Yes.

https://codereview.chromium.org/935083002/diff/40001/content/renderer/present...
content/renderer/presentation/presentation_session_dispatcher.h:30: virtual
blink::WebString getId();
On 2015/02/19 22:59:57, mark a. foltz wrote:
> Ditto

Acknowledged.

[blundell, 2015-02-20 07:16:27]

LGTM from the Mojo pov

[whywhat, 2015-02-20 18:22:14]

New patchsets have been uploaded after l-g-t-m from blundell@chromium.org

[mlamouri (slow - plz ping), 2015-02-23 14:07:39]

https://codereview.chromium.org/935083002/diff/60001/content/browser/presenta...
File content/browser/presentation/presentation_service_impl.h (right):

https://codereview.chromium.org/935083002/diff/60001/content/browser/presenta...
content/browser/presentation/presentation_service_impl.h:54: typedef
mojo::Callback<void(presentation::PresentationSessionInfoPtr,
What about using `using Foo = Bar;` C++11 FTW :)

https://codereview.chromium.org/935083002/diff/60001/content/common/presentat...
File content/common/presentation/presentation_service.mojom (right):

https://codereview.chromium.org/935083002/diff/60001/content/common/presentat...
content/common/presentation/presentation_service.mojom:16: int32 errorCode;
Could you have an enum called PresentationErroryType so the error codes actually
mean something and we can translate them and send them to Blink?

https://codereview.chromium.org/935083002/diff/60001/content/common/presentat...
content/common/presentation/presentation_service.mojom:46:
PresentationSessionInfo? sessionInfo, PresentationError? error);
nit: this looks more readable:
StartSession(string presentation_url, string? presentation_id)
  => (PresentationSessionInfo? sessionInfo, PresentationError? error);

This is also the style used in the permission service.

https://codereview.chromium.org/935083002/diff/60001/content/common/presentat...
content/common/presentation/presentation_service.mojom:53:
PresentationSessionInfo? sessionInfo, PresentationError? error);
ditto

https://codereview.chromium.org/935083002/diff/60001/content/renderer/present...
File content/renderer/presentation/presentation_dispatcher.cc (right):

https://codereview.chromium.org/935083002/diff/60001/content/renderer/present...
content/renderer/presentation/presentation_dispatcher.cc:58:
base::Unretained(callback)));
base::Owned(callback)

https://codereview.chromium.org/935083002/diff/60001/content/renderer/present...
content/renderer/presentation/presentation_dispatcher.cc:71:
base::Unretained(callback)));
ditto

https://codereview.chromium.org/935083002/diff/60001/content/renderer/present...
content/renderer/presentation/presentation_dispatcher.cc:93: error->errorCode,
You will need a switch to go from PresentationService error types to Blink error
types.

https://codereview.chromium.org/935083002/diff/60001/content/renderer/present...
content/renderer/presentation/presentation_dispatcher.cc:102:
callback->onSuccess(session_dispatcher);
PresentationDispatcher owns the callback. Please delete it.

https://codereview.chromium.org/935083002/diff/60001/content/renderer/present...
File content/renderer/presentation/presentation_dispatcher.h (right):

https://codereview.chromium.org/935083002/diff/60001/content/renderer/present...
content/renderer/presentation/presentation_dispatcher.h:55: typedef
ScopedVector<PresentationSessionDispatcher> PresentationSessions;
using PresentationSessions = ScopedVector<PresentationSessionDispatcher>;

https://codereview.chromium.org/935083002/diff/60001/content/renderer/present...
File content/renderer/presentation/presentation_session_dispatcher.cc (right):

https://codereview.chromium.org/935083002/diff/60001/content/renderer/present...
content/renderer/presentation/presentation_session_dispatcher.cc:24: return
blink::WebString::fromLatin1(session_info_->id);
Shouldn't that be ::fromUTF8?

https://codereview.chromium.org/935083002/diff/60001/content/shell/renderer/t...
File content/shell/renderer/test_runner/mock_presentation_client.cc (right):

https://codereview.chromium.org/935083002/diff/60001/content/shell/renderer/t...
content/shell/renderer/test_runner/mock_presentation_client.cc:57:
NOTIMPLEMENTED();
delete callback;

https://codereview.chromium.org/935083002/diff/60001/content/shell/renderer/t...
content/shell/renderer/test_runner/mock_presentation_client.cc:64:
NOTIMPLEMENTED();
delete callback;

[whywhat, 2015-02-25 14:12:42]

https://codereview.chromium.org/935083002/diff/60001/content/browser/presenta...
File content/browser/presentation/presentation_service_impl.h (right):

https://codereview.chromium.org/935083002/diff/60001/content/browser/presenta...
content/browser/presentation/presentation_service_impl.h:54: typedef
mojo::Callback<void(presentation::PresentationSessionInfoPtr,
On 2015/02/23 14:07:39, Mounir Lamouri wrote:
> What about using `using Foo = Bar;` C++11 FTW :)

Done.

https://codereview.chromium.org/935083002/diff/60001/content/common/presentat...
File content/common/presentation/presentation_service.mojom (right):

https://codereview.chromium.org/935083002/diff/60001/content/common/presentat...
content/common/presentation/presentation_service.mojom:16: int32 errorCode;
On 2015/02/23 14:07:39, Mounir Lamouri wrote:
> Could you have an enum called PresentationErroryType so the error codes
actually
> mean something and we can translate them and send them to Blink?

Done.

https://codereview.chromium.org/935083002/diff/60001/content/common/presentat...
content/common/presentation/presentation_service.mojom:46:
PresentationSessionInfo? sessionInfo, PresentationError? error);
On 2015/02/23 14:07:39, Mounir Lamouri wrote:
> nit: this looks more readable:
> StartSession(string presentation_url, string? presentation_id)
>   => (PresentationSessionInfo? sessionInfo, PresentationError? error);
> 
> This is also the style used in the permission service.

Done.

https://codereview.chromium.org/935083002/diff/60001/content/common/presentat...
content/common/presentation/presentation_service.mojom:53:
PresentationSessionInfo? sessionInfo, PresentationError? error);
On 2015/02/23 14:07:39, Mounir Lamouri wrote:
> ditto

Done.

https://codereview.chromium.org/935083002/diff/60001/content/renderer/present...
File content/renderer/presentation/presentation_dispatcher.cc (right):

https://codereview.chromium.org/935083002/diff/60001/content/renderer/present...
content/renderer/presentation/presentation_dispatcher.cc:58:
base::Unretained(callback)));
On 2015/02/23 14:07:39, Mounir Lamouri wrote:
> base::Owned(callback)

Done. Actually atm the callback owns/deletes itself but this seems to be
cleaner.

https://codereview.chromium.org/935083002/diff/60001/content/renderer/present...
content/renderer/presentation/presentation_dispatcher.cc:71:
base::Unretained(callback)));
On 2015/02/23 14:07:39, Mounir Lamouri wrote:
> ditto

Done.

https://codereview.chromium.org/935083002/diff/60001/content/renderer/present...
content/renderer/presentation/presentation_dispatcher.cc:93: error->errorCode,
On 2015/02/23 14:07:39, Mounir Lamouri wrote:
> You will need a switch to go from PresentationService error types to Blink
error
> types.

Done.

https://codereview.chromium.org/935083002/diff/60001/content/renderer/present...
content/renderer/presentation/presentation_dispatcher.cc:102:
callback->onSuccess(session_dispatcher);
On 2015/02/23 14:07:39, Mounir Lamouri wrote:
> PresentationDispatcher owns the callback. Please delete it.

As I understand, it will be deleted automatically if I use base::Owned?

https://codereview.chromium.org/935083002/diff/60001/content/renderer/present...
File content/renderer/presentation/presentation_dispatcher.h (right):

https://codereview.chromium.org/935083002/diff/60001/content/renderer/present...
content/renderer/presentation/presentation_dispatcher.h:55: typedef
ScopedVector<PresentationSessionDispatcher> PresentationSessions;
On 2015/02/23 14:07:39, Mounir Lamouri wrote:
> using PresentationSessions = ScopedVector<PresentationSessionDispatcher>;

Done.

https://codereview.chromium.org/935083002/diff/60001/content/renderer/present...
File content/renderer/presentation/presentation_session_dispatcher.cc (right):

https://codereview.chromium.org/935083002/diff/60001/content/renderer/present...
content/renderer/presentation/presentation_session_dispatcher.cc:24: return
blink::WebString::fromLatin1(session_info_->id);
On 2015/02/23 14:07:39, Mounir Lamouri wrote:
> Shouldn't that be ::fromUTF8?

I assumed the restrictions on the generated id are the same for site provided
id, but I guess the site could use utf8 characters for the id.

https://codereview.chromium.org/935083002/diff/60001/content/shell/renderer/t...
File content/shell/renderer/test_runner/mock_presentation_client.cc (right):

https://codereview.chromium.org/935083002/diff/60001/content/shell/renderer/t...
content/shell/renderer/test_runner/mock_presentation_client.cc:57:
NOTIMPLEMENTED();
On 2015/02/23 14:07:39, Mounir Lamouri wrote:
> delete callback;

Done.

https://codereview.chromium.org/935083002/diff/60001/content/shell/renderer/t...
content/shell/renderer/test_runner/mock_presentation_client.cc:64:
NOTIMPLEMENTED();
On 2015/02/23 14:07:39, Mounir Lamouri wrote:
> delete callback;

Done.

[mark a. foltz, 2015-02-26 00:13:42]

lgtm with a couple of suggestions for docstrings

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

https://codereview.chromium.org/935083002/diff/100001/content/common/presenta...
content/common/presentation/presentation_service.mojom:44: // succeeded (so the
session object is valid) or an error message if
I would say:

will return a non-null and valid PresentationSessionInfo if starting the session
succeeded, or null with a PresentationError if starting the session failed.

https://codereview.chromium.org/935083002/diff/100001/content/common/presenta...
content/common/presentation/presentation_service.mojom:56: // works the same as
for the method above. JoinSession is used to silently
JoinSession will join a known session (i.e. ...) silently and without user
action.

[whywhat, 2015-02-26 16:15:07]

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

https://codereview.chromium.org/935083002/diff/100001/content/common/presenta...
content/common/presentation/presentation_service.mojom:44: // succeeded (so the
session object is valid) or an error message if
On 2015/02/26 00:13:42, mark a. foltz wrote:
> I would say:
> 
> will return a non-null and valid PresentationSessionInfo if starting the
session
> succeeded, or null with a PresentationError if starting the session failed.

Done.

https://codereview.chromium.org/935083002/diff/100001/content/common/presenta...
content/common/presentation/presentation_service.mojom:56: // works the same as
for the method above. JoinSession is used to silently
On 2015/02/26 00:13:42, mark a. foltz wrote:
> JoinSession will join a known session (i.e. ...) silently and without user
> action.

Done.

[mlamouri (slow - plz ping), 2015-02-26 22:24:26]

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

https://codereview.chromium.org/935083002/diff/160001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.cc:87:
presentation_service_->JoinSession(
You don't have to apply that change but I wonder if we could do something like
this:
presentation::PresentationServicePtr& GetPresentationService() {
  if (!presentation_service_.get())
    render_frame()->GetServiceRegistry()->ConnectToRemoteService(
        &presentation_service_);

  return presentationt_service_;
}

Then you can do things like:
GetPresentationService()->JoinSession(...);

WDYT?

https://codereview.chromium.org/935083002/diff/160001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.cc:110: return;
How could that happen? If it's not expected, please add a DCHECK.

https://codereview.chromium.org/935083002/diff/160001/content/renderer/presen...
File content/renderer/presentation/presentation_dispatcher.h (right):

https://codereview.chromium.org/935083002/diff/160001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.h:56: PresentationSessions
presentation_sessions_;
You should probably rename that. Add "dispatcher" somewhere to make that less
confusing?

https://codereview.chromium.org/935083002/diff/160001/content/renderer/presen...
File content/renderer/presentation/presentation_session_client.h (right):

https://codereview.chromium.org/935083002/diff/160001/content/renderer/presen...
content/renderer/presentation/presentation_session_client.h:29:
PresentationSessionDispatcher* dispatcher_;
That's not right. You do not control the lifetime of that object so the
|dispatcher_| might be pointing to garbage.

https://codereview.chromium.org/935083002/diff/160001/content/renderer/presen...
File content/renderer/presentation/presentation_session_dispatcher.h (right):

https://codereview.chromium.org/935083002/diff/160001/content/renderer/presen...
content/renderer/presentation/presentation_session_dispatcher.h:11: #include
"third_party/WebKit/public/platform/modules/presentation/WebPresentationSessionClient.h"
not sure you need that.

[whywhat, 2015-02-27 14:44:46]

PTAL

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

https://codereview.chromium.org/935083002/diff/160001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.cc:87:
presentation_service_->JoinSession(
On 2015/02/26 22:24:26, Mounir Lamouri wrote:
> You don't have to apply that change but I wonder if we could do something like
> this:
> presentation::PresentationServicePtr& GetPresentationService() {
>   if (!presentation_service_.get())
>     render_frame()->GetServiceRegistry()->ConnectToRemoteService(
>         &presentation_service_);
> 
>   return presentationt_service_;
> }
> 
> Then you can do things like:
> GetPresentationService()->JoinSession(...);
> 
> WDYT?

Would look a little better but then you can't really return the pointer by value
and returning by reference is somewhat against the style guide. A smart pointer
would be nice  I guess but I'll leave it out of this CL.

https://codereview.chromium.org/935083002/diff/160001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.cc:110: return;
On 2015/02/26 22:24:26, Mounir Lamouri wrote:
> How could that happen? If it's not expected, please add a DCHECK.

Done.

https://codereview.chromium.org/935083002/diff/160001/content/renderer/presen...
File content/renderer/presentation/presentation_dispatcher.h (right):

https://codereview.chromium.org/935083002/diff/160001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.h:56: PresentationSessions
presentation_sessions_;
On 2015/02/26 22:24:26, Mounir Lamouri wrote:
> You should probably rename that. Add "dispatcher" somewhere to make that less
> confusing?

Done.

https://codereview.chromium.org/935083002/diff/160001/content/renderer/presen...
File content/renderer/presentation/presentation_session_client.h (right):

https://codereview.chromium.org/935083002/diff/160001/content/renderer/presen...
content/renderer/presentation/presentation_session_client.h:29:
PresentationSessionDispatcher* dispatcher_;
On 2015/02/26 22:24:26, Mounir Lamouri wrote:
> That's not right. You do not control the lifetime of that object so the
> |dispatcher_| might be pointing to garbage.

Done.

https://codereview.chromium.org/935083002/diff/160001/content/renderer/presen...
File content/renderer/presentation/presentation_session_dispatcher.h (right):

https://codereview.chromium.org/935083002/diff/160001/content/renderer/presen...
content/renderer/presentation/presentation_session_dispatcher.h:11: #include
"third_party/WebKit/public/platform/modules/presentation/WebPresentationSessionClient.h"
On 2015/02/26 22:24:26, Mounir Lamouri wrote:
> not sure you need that.

Done.

[mlamouri (slow - plz ping), 2015-02-27 18:41:51]

lgtm with comments applied

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

https://codereview.chromium.org/935083002/diff/180001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.cc:74:
ConnectToPresentationServiceIfNeeded();
nit: add an empty line, they don't cost anything and it increases readability :)

https://codereview.chromium.org/935083002/diff/180001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.cc:80:
base::Owned(callback)));
Could you have a comment explaining why using base::Unretained() and
base::Owned() is fine?

https://codereview.chromium.org/935083002/diff/180001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.cc:88:
ConnectToPresentationServiceIfNeeded();
ditto

https://codereview.chromium.org/935083002/diff/180001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.cc:94:
base::Owned(callback)));
ditto

https://codereview.chromium.org/935083002/diff/180001/content/renderer/presen...
File content/renderer/presentation/presentation_dispatcher.h (right):

https://codereview.chromium.org/935083002/diff/180001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.h:57:
PresentationSessionDispatchers presentation_session_dispatchers_;
This might be slightly sub-optimal when you will start looking for a session
dispatcher but it's okay for now.

https://codereview.chromium.org/935083002/diff/180001/content/renderer/presen...
File content/renderer/presentation/presentation_session_dispatcher.h (right):

https://codereview.chromium.org/935083002/diff/180001/content/renderer/presen...
content/renderer/presentation/presentation_session_dispatcher.h:22: const
mojo::String& getId() const;
style: GetURL() and GetID() (or GetId())

[whywhat, 2015-02-27 19:15:52]

avayvod@chromium.org changed reviewers:
+ avi@chromium.org

[whywhat, 2015-02-27 19:15:54]

+Avi for ownership LGTM (if it's needed for gn/gypi files).

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

https://codereview.chromium.org/935083002/diff/180001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.cc:74:
ConnectToPresentationServiceIfNeeded();
On 2015/02/27 18:41:51, Mounir Lamouri wrote:
> nit: add an empty line, they don't cost anything and it increases readability
:)

Done.

https://codereview.chromium.org/935083002/diff/180001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.cc:80:
base::Owned(callback)));
On 2015/02/27 18:41:51, Mounir Lamouri wrote:
> Could you have a comment explaining why using base::Unretained() and
> base::Owned() is fine?

Done.

https://codereview.chromium.org/935083002/diff/180001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.cc:80:
base::Owned(callback)));
On 2015/02/27 18:41:51, Mounir Lamouri wrote:
> Could you have a comment explaining why using base::Unretained() and
> base::Owned() is fine?

Done.

https://codereview.chromium.org/935083002/diff/180001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.cc:88:
ConnectToPresentationServiceIfNeeded();
On 2015/02/27 18:41:51, Mounir Lamouri wrote:
> ditto

Done.

https://codereview.chromium.org/935083002/diff/180001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.cc:94:
base::Owned(callback)));
On 2015/02/27 18:41:51, Mounir Lamouri wrote:
> ditto

Done.

https://codereview.chromium.org/935083002/diff/180001/content/renderer/presen...
File content/renderer/presentation/presentation_dispatcher.h (right):

https://codereview.chromium.org/935083002/diff/180001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.h:57:
PresentationSessionDispatchers presentation_session_dispatchers_;
On 2015/02/27 18:41:51, Mounir Lamouri wrote:
> This might be slightly sub-optimal when you will start looking for a session
> dispatcher but it's okay for now.

Left a TODO for later.

https://codereview.chromium.org/935083002/diff/180001/content/renderer/presen...
File content/renderer/presentation/presentation_session_dispatcher.h (right):

https://codereview.chromium.org/935083002/diff/180001/content/renderer/presen...
content/renderer/presentation/presentation_session_dispatcher.h:22: const
mojo::String& getId() const;
On 2015/02/27 18:41:51, Mounir Lamouri wrote:
> style: GetURL() and GetID() (or GetId())

Done.

[Avi (use Gerrit), 2015-02-27 19:20:38]

lgtm

stampity stamp

[whywhat, 2015-02-27 23:43:21]

The CQ bit was checked by avayvod@chromium.org

[whywhat, 2015-02-27 23:43:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
blundell@chromium.org, mfoltz@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/935083002/#ps200001
(title: "Fixed the last round of comments")

[commit-bot: I haz the power, 2015-02-27 23:45:00]

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

[commit-bot: I haz the power, 2015-02-27 23:48:59]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-02-27 23:49:37]

Patchset 11 (id:??) landed as
https://crrev.com/f0dd974b186bbaf4d1e6c5805666f7c0d71ae795
Cr-Commit-Position: refs/heads/master@{#318550}