[Presentation API] Plumbing of |onstatechange| from the Mojo service to the public/platform

[Presentation API] Plumbing of |onstatechange| from the Mojo service to the public/platform

Removed PresentationSessionDispatcher since the session messages are going through the
PresentationService (to reduce the number of Mojo pipes used).

Depends on the Blink change: https://codereview.chromium.org/1020303004

BUG=459006
Committed: https://crrev.com/a3731aafcc2dc3b705a194f42e114169b0137d4d
Cr-Commit-Position: refs/heads/master@{#322162}

[whywhat, 2015-03-20 18:51:32]

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

[whywhat, 2015-03-20 18:52:00]

PTaL!

[Peter Beverloo, 2015-03-20 19:13:39]

lgtm

https://codereview.chromium.org/1024903003/diff/1/content/browser/presentatio...
File content/browser/presentation/presentation_service_impl.h (right):

https://codereview.chromium.org/1024903003/diff/1/content/browser/presentatio...
content/browser/presentation/presentation_service_impl.h:64:
presentation::PresentationSessionState)>;
nit: align with (

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

https://codereview.chromium.org/1024903003/diff/1/content/common/presentation...
content/common/presentation/presentation_service.mojom:83: // changed since the
last time the callback was called. Might
nit: advocating for 70-character line length limits rather than 80-character
ones? :)

https://codereview.chromium.org/1024903003/diff/1/content/renderer/presentati...
File content/renderer/presentation/presentation_dispatcher.cc (right):

https://codereview.chromium.org/1024903003/diff/1/content/renderer/presentati...
content/renderer/presentation/presentation_dispatcher.cc:44: default:
Are there more than two options in the generated enum?

If there aren't, please remove the default: case so that the compiler verifies
that all states are being handled.

https://codereview.chromium.org/1024903003/diff/1/content/renderer/presentati...
content/renderer/presentation/presentation_dispatcher.cc:203: new
PresentationSessionClient(session_info.Pass()),
It would be nice if you could document in the
WebPresentationController::didChangeSessionState API that it takes ownership of
the PresentationSessionClient object.

[mark a. foltz, 2015-03-20 21:22:43]

mfoltz@chromium.org changed reviewers:
+ imcheng@google.com, mfoltz@chromium.org

[mark a. foltz, 2015-03-20 21:22:44]

Would like Derek to take a look as well.

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

https://codereview.chromium.org/1024903003/diff/1/content/common/presentation...
content/common/presentation/presentation_service.mojom:85:
ListenForSessionStateChange()
GetSessionState() for consistency with GetScreenAvailability()?

[imcheng, 2015-03-20 23:01:45]

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

[imcheng, 2015-03-20 23:01:46]

https://codereview.chromium.org/1024903003/diff/1/content/renderer/presentati...
File content/renderer/presentation/presentation_dispatcher.cc (right):

https://codereview.chromium.org/1024903003/diff/1/content/renderer/presentati...
content/renderer/presentation/presentation_dispatcher.cc:202:
controller_->didChangeSessionState(
Does controller_ hold references to multiple PresentationSessionClients? How
does it handle multiple sessions in the same frame?

https://codereview.chromium.org/1024903003/diff/1/content/renderer/presentati...
content/renderer/presentation/presentation_dispatcher.cc:217:
&PresentationDispatcher::OnSessionStateChange,
How does PresentationServiceImpl know which session to listen to state changes
for?

[whywhat, 2015-03-24 19:20:58]

https://codereview.chromium.org/1024903003/diff/1/content/browser/presentatio...
File content/browser/presentation/presentation_service_impl.h (right):

https://codereview.chromium.org/1024903003/diff/1/content/browser/presentatio...
content/browser/presentation/presentation_service_impl.h:64:
presentation::PresentationSessionState)>;
On 2015/03/20 19:13:39, Peter Beverloo wrote:
> nit: align with (

Acknowledged.

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

https://codereview.chromium.org/1024903003/diff/1/content/common/presentation...
content/common/presentation/presentation_service.mojom:83: // changed since the
last time the callback was called. Might
On 2015/03/20 19:13:39, Peter Beverloo wrote:
> nit: advocating for 70-character line length limits rather than 80-character
> ones? :)

Copy paste from the doc about the Mojo interface. Docs are narrower than code.

https://codereview.chromium.org/1024903003/diff/1/content/common/presentation...
content/common/presentation/presentation_service.mojom:85:
ListenForSessionStateChange()
On 2015/03/20 21:22:43, mark a. foltz wrote:
> GetSessionState() for consistency with GetScreenAvailability()?

But this is consistent with ListenForDefaultSessionStart. Probably should change
GetScreenAvailability to ListenForScreenAvailability in a separate CL?

https://codereview.chromium.org/1024903003/diff/1/content/renderer/presentati...
File content/renderer/presentation/presentation_dispatcher.cc (right):

https://codereview.chromium.org/1024903003/diff/1/content/renderer/presentati...
content/renderer/presentation/presentation_dispatcher.cc:44: default:
On 2015/03/20 19:13:39, Peter Beverloo wrote:
> Are there more than two options in the generated enum?
> 
> If there aren't, please remove the default: case so that the compiler verifies
> that all states are being handled.

Done.

https://codereview.chromium.org/1024903003/diff/1/content/renderer/presentati...
content/renderer/presentation/presentation_dispatcher.cc:202:
controller_->didChangeSessionState(
On 2015/03/20 23:01:46, imcheng1 wrote:
> Does controller_ hold references to multiple PresentationSessionClients? How
> does it handle multiple sessions in the same frame?

Presentation has a set of PresentationSession objects (one per each JS object
returned to the frame). The PresentationSessionClient is just a way to pass the
message from content/ to blink/ and is destroyed as soon as PresentationSession
object is constructed.

https://codereview.chromium.org/1024903003/diff/1/content/renderer/presentati...
content/renderer/presentation/presentation_dispatcher.cc:203: new
PresentationSessionClient(session_info.Pass()),
On 2015/03/20 19:13:39, Peter Beverloo wrote:
> It would be nice if you could document in the
> WebPresentationController::didChangeSessionState API that it takes ownership
of
> the PresentationSessionClient object.

It would be nice to read this comment before submitting the Blink change.
Fingers crossed for it to fail to land :D

https://codereview.chromium.org/1024903003/diff/1/content/renderer/presentati...
content/renderer/presentation/presentation_dispatcher.cc:217:
&PresentationDispatcher::OnSessionStateChange,
On 2015/03/20 23:01:46, imcheng1 wrote:
> How does PresentationServiceImpl know which session to listen to state changes
> for?

PresentationServiceImpl should call the callback for any state change for any
session open from the frame. I need to call ListenForSessionStateChange again in
OnSessionStateChange though.

[mark a. foltz, 2015-03-24 22:53:35]

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

https://codereview.chromium.org/1024903003/diff/1/content/common/presentation...
content/common/presentation/presentation_service.mojom:85:
ListenForSessionStateChange()
On 2015/03/24 19:20:57, whywhat wrote:
> On 2015/03/20 21:22:43, mark a. foltz wrote:
> > GetSessionState() for consistency with GetScreenAvailability()?
> 
> But this is consistent with ListenForDefaultSessionStart. Probably should
change
> GetScreenAvailability to ListenForScreenAvailability in a separate CL?

SGTM

[whywhat, 2015-03-25 13:55:50]

The CQ bit was checked by avayvod@chromium.org

[whywhat, 2015-03-25 13:55:51]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org
Link to the patchset: https://codereview.chromium.org/1024903003/#ps20001
(title: "Call the Mojo method again from the callback")

[commit-bot: I haz the power, 2015-03-25 13:56:04]

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

[commit-bot: I haz the power, 2015-03-25 15:02:01]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-03-25 15:02:49]

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