Marks session as connected upon StartSession success

Marks session as connected upon StartSession success

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197868

[haibinlu, 2015-06-24 18:43:47]

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

[haibinlu, 2015-06-25 17:28:16]

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

[whywhat, 2015-06-25 17:33:45]

lgtm with a note
implementation wise, dispatching an event doesn't seem to do much for an empty
set of listeners

https://codereview.chromium.org/1209473003/diff/1/Source/modules/presentation...
File Source/modules/presentation/PresentationSessionClientCallbacks.cpp (right):

https://codereview.chromium.org/1209473003/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationSessionClientCallbacks.cpp:43:
session->didChangeState(WebPresentationSessionState::Connected);
Seems wasteful to dispatch an event to just created and not yet returned to the
page session.
Maybe it's better to pass the initial state with WebPresentationSessionClient?
Or you could just set the session's state to Connected in the constructor.

[haibinlu, 2015-06-25 18:45:36]

PTAL

https://codereview.chromium.org/1209473003/diff/1/Source/modules/presentation...
File Source/modules/presentation/PresentationSessionClientCallbacks.cpp (right):

https://codereview.chromium.org/1209473003/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationSessionClientCallbacks.cpp:43:
session->didChangeState(WebPresentationSessionState::Connected);
On 2015/06/25 17:33:45, whywhat wrote:
> Seems wasteful to dispatch an event to just created and not yet returned to
the
> page session.
> Maybe it's better to pass the initial state with WebPresentationSessionClient?
> Or you could just set the session's state to Connected in the constructor.

Make sense. Pass in the initial state.

[whywhat, 2015-06-25 19:57:53]

https://codereview.chromium.org/1209473003/diff/20001/Source/modules/presenta...
File Source/modules/presentation/PresentationSession.h (right):

https://codereview.chromium.org/1209473003/diff/20001/Source/modules/presenta...
Source/modules/presentation/PresentationSession.h:34: static
PresentationSession* take(WebPresentationSessionClient*, Presentation*,
WebPresentationSessionState);
I'd prefer the state be passed as part of the WebPresentationSessionClient. Why
is it different from URL and id and is specified separately? I don't think the
Callback class is what should determine that the session is connected.
I understand that it's even more changes throughout though.

[haibinlu, 2015-06-25 20:26:49]

It takes quite a bit of change to make WebPresentationSessionClient to have
state. Can we simply set a new PresentationSession to Connected state? Right
now, it is the case: a new session is created only when Start/JoinSession
succeeds.

[whywhat, 2015-06-25 21:26:09]

Yes, that was my alternative suggestion in the first reply: default to
Connected in the ctor
On 25 Jun 2015 9:26 pm, <haibinlu@chromium.org> wrote:

> It takes quite a bit of change to make WebPresentationSessionClient to have
> state. Can we simply set a new PresentationSession to Connected state?
> Right
> now, it is the case: a new session is created only when Start/JoinSession
> succeeds.
>
> https://codereview.chromium.org/1209473003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[haibinlu, 2015-06-25 22:07:58]

The CQ bit was checked by haibinlu@chromium.org

[haibinlu, 2015-06-25 22:07:59]

The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org
Link to the patchset: https://codereview.chromium.org/1209473003/#ps60001
(title: " ")

[commit-bot: I haz the power, 2015-06-25 22:08:06]

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

[commit-bot: I haz the power, 2015-06-25 23:55:24]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197868