[chrome.displaySource] Session notification improvements

[chrome.displaySource] Session notification improvements

This patch provides several fixes and improvements (simplifications)
to the session life cycle notification mechanism:
1) The 'startSession'/'terminateSession' call completion callbacks are
invoked when the session is actually started/terminated and in accordance
with 'onSessionStarted'/'onSessionTerminated' events propagating (and as
per documentation).
2) The notification from the required sink is filtered out in browser process
avoiding unneeded IPC calls
3) Methods in mojo interface are renamed in order to improve readability

BUG=242107
Committed: https://crrev.com/ef0d3aa8a2e82e6db356e112c236ef0f8a2a8918
Cr-Commit-Position: refs/heads/master@{#374929}

[Mikhail, 2016-02-05 14:43:33]

Patchset #1 (id:1) has been deleted

[Mikhail, 2016-02-05 14:44:45]

Description was changed from

==========
[chrome.displaySource] Session notification improvements

This patch provides several fixes and improvements (simplifications)
to the session life cycle notification mechanism:
1) The 'startSession'/'terminateSession' call completion callbacks are
invoked when the session is actually started/terminated and in accordance
with 'onSessionStarted'/'onSessionTerminated' events propagating (and as
per documentation).
2) The notification from the required sink is filtered out in browser process
avoiding unneeded IPC calls
3) Methods in mojo interface are renamed in order to improve readability

BUG=242107
==========

to

==========
[chrome.displaySource] Session notification improvements

This patch provides several fixes and improvements (simplifications)
to the session life cycle notification mechanism:
1) The 'startSession'/'terminateSession' call completion callbacks are
invoked when the session is actually started/terminated and in accordance
with 'onSessionStarted'/'onSessionTerminated' events propagating (and as
per documentation).
2) The notification from the required sink is filtered out in browser process
avoiding unneeded IPC calls
3) Methods in mojo interface are renamed in order to improve readability

BUG=242107
==========

[Mikhail, 2016-02-05 14:44:46]

mikhail.pozdnyakov@intel.com changed reviewers:
+ alexander.shalamov@intel.com, asargent@chromium.org

[Mikhail, 2016-02-05 14:44:58]

PTAL

[Mikhail, 2016-02-05 14:57:07]

Patchset #1 (id:20001) has been deleted

[shalamov, 2016-02-08 11:16:19]

On 2016/02/05 14:44:58, Mikhail wrote:
> PTAL

lgtm

[asargent_no_longer_on_chrome, 2016-02-09 00:07:56]

https://codereview.chromium.org/1674583002/diff/40001/extensions/renderer/dis...
File extensions/renderer/display_source_custom_bindings.cc (right):

https://codereview.chromium.org/1674583002/diff/40001/extensions/renderer/dis...
extensions/renderer/display_source_custom_bindings.cc:290:
CallSessionStartedCallback(sink_id);
One question that came to mind seeing this - does it seem likely that in actual
programs, you'd want both the standalone sessionStarted event *and* the callback
argument to startSession?

For something like the chrome.tabs API, we have a create method that lets you
create new browser tabs, and that does have a callback to let you know when the
tab is created, and there is also a standalone tabs.onCreated event, but it's
useful to have both because many tabs are created by things like user action
outside of the context of the tabs API. In your case, I'm wondering if it would
be common for anywhere in a program to care about sessions being started
*except* for the code that just asked to start one. If it is, that's fine, but
if not you might consider getting rid of the standalone event.

https://codereview.chromium.org/1674583002/diff/40001/extensions/renderer/res...
File extensions/renderer/resources/display_source_custom_bindings.js (right):

https://codereview.chromium.org/1674583002/diff/40001/extensions/renderer/res...
extensions/renderer/resources/display_source_custom_bindings.js:31:
natives.StartSession(sessionInfo, callbackWrapper);
Instead of having the callback wrapper handling split between JS and C++, where
the C++ code doesn't really do anything complicated, did you consider just doing
it entirely in JS? We essentially do that for most API requests that don't have
custom bindings - see the following functions in send_request.js :

-prepareRequest
-sendRequest
-handleResponse

Those just keep an array of information, including a reference to the callback
for later use, indexed by an integer request id that the C++ code assigns.

[Mikhail, 2016-02-09 14:26:42]

https://codereview.chromium.org/1674583002/diff/40001/extensions/renderer/dis...
File extensions/renderer/display_source_custom_bindings.cc (right):

https://codereview.chromium.org/1674583002/diff/40001/extensions/renderer/dis...
extensions/renderer/display_source_custom_bindings.cc:290:
CallSessionStartedCallback(sink_id);
On 2016/02/09 00:07:56, Antony Sargent wrote:
> One question that came to mind seeing this - does it seem likely that in
actual
> programs, you'd want both the standalone sessionStarted event *and* the
callback
> argument to startSession?
> 
> For something like the chrome.tabs API, we have a create method that lets you
> create new browser tabs, and that does have a callback to let you know when
the
> tab is created, and there is also a standalone tabs.onCreated event, but it's
> useful to have both because many tabs are created by things like user action
> outside of the context of the tabs API. In your case, I'm wondering if it
would
> be common for anywhere in a program to care about sessions being started
> *except* for the code that just asked to start one. If it is, that's fine, but
> if not you might consider getting rid of the standalone event.

Good point. Unlike session errors or termination, session start can only be
initiated from the API 'startSession' call, so 'sessionStarted' is duplicating
the method completion callback and can be dropped. However the
'chrome.displaySource' API proposal had been agreed. Would it be possible to
make such API changes now?

https://codereview.chromium.org/1674583002/diff/40001/extensions/renderer/res...
File extensions/renderer/resources/display_source_custom_bindings.js (right):

https://codereview.chromium.org/1674583002/diff/40001/extensions/renderer/res...
extensions/renderer/resources/display_source_custom_bindings.js:31:
natives.StartSession(sessionInfo, callbackWrapper);
On 2016/02/09 00:07:56, Antony Sargent wrote:
> Instead of having the callback wrapper handling split between JS and C++,
where
> the C++ code doesn't really do anything complicated, did you consider just
doing
> it entirely in JS? We essentially do that for most API requests that don't
have
> custom bindings - see the following functions in send_request.js :
> 
> -prepareRequest
> -sendRequest
> -handleResponse
> 
> Those just keep an array of information, including a reference to the callback
> for later use, indexed by an integer request id that the C++ code assigns. 

I should have a look at this, thanks!

[asargent_no_longer_on_chrome, 2016-02-09 17:04:05]

https://codereview.chromium.org/1674583002/diff/40001/extensions/renderer/dis...
File extensions/renderer/display_source_custom_bindings.cc (right):

https://codereview.chromium.org/1674583002/diff/40001/extensions/renderer/dis...
extensions/renderer/display_source_custom_bindings.cc:290:
CallSessionStartedCallback(sink_id);
On 2016/02/09 14:26:42, Mikhail wrote:
> On 2016/02/09 00:07:56, Antony Sargent wrote:
> > One question that came to mind seeing this - does it seem likely that in
> actual
> > programs, you'd want both the standalone sessionStarted event *and* the
> callback
> > argument to startSession?
> > 
> > For something like the chrome.tabs API, we have a create method that lets
you
> > create new browser tabs, and that does have a callback to let you know when
> the
> > tab is created, and there is also a standalone tabs.onCreated event, but
it's
> > useful to have both because many tabs are created by things like user action
> > outside of the context of the tabs API. In your case, I'm wondering if it
> would
> > be common for anywhere in a program to care about sessions being started
> > *except* for the code that just asked to start one. If it is, that's fine,
but
> > if not you might consider getting rid of the standalone event.
> 
> Good point. Unlike session errors or termination, session start can only be
> initiated from the API 'startSession' call, so 'sessionStarted' is duplicating
> the method completion callback and can be dropped. However the
> 'chrome.displaySource' API proposal had been agreed. Would it be possible to
> make such API changes now?

Since the API is still only available in dev channel and not officially launched
in stable, you are still free to make breaking changes if you want.

[Mikhail, 2016-02-10 15:58:51]

Thanks for having a look!

https://codereview.chromium.org/1674583002/diff/40001/extensions/renderer/dis...
File extensions/renderer/display_source_custom_bindings.cc (right):

https://codereview.chromium.org/1674583002/diff/40001/extensions/renderer/dis...
extensions/renderer/display_source_custom_bindings.cc:290:
CallSessionStartedCallback(sink_id);
On 2016/02/09 17:04:05, Antony Sargent wrote:
> Since the API is still only available in dev channel and not officially
launched
> in stable, you are still free to make breaking changes if you want. 
The 'chrome.displaySource' is removed now.

https://codereview.chromium.org/1674583002/diff/40001/extensions/renderer/res...
File extensions/renderer/resources/display_source_custom_bindings.js (right):

https://codereview.chromium.org/1674583002/diff/40001/extensions/renderer/res...
extensions/renderer/resources/display_source_custom_bindings.js:31:
natives.StartSession(sessionInfo, callbackWrapper);
On 2016/02/09 00:07:56, Antony Sargent wrote:
> Instead of having the callback wrapper handling split between JS and C++,
where
> the C++ code doesn't really do anything complicated, did you consider just
doing
> it entirely in JS? We essentially do that for most API requests that don't
have
> custom bindings - see the following functions in send_request.js :
> 
> -prepareRequest
> -sendRequest
> -handleResponse
> 
> Those just keep an array of information, including a reference to the callback
> for later use, indexed by an integer request id that the C++ code assigns. 

Done.

[shalamov, 2016-02-11 08:56:25]

https://codereview.chromium.org/1674583002/diff/60001/extensions/common/mojo/...
File extensions/common/mojo/wifi_display_session_service.mojom (right):

https://codereview.chromium.org/1674583002/diff/60001/extensions/common/mojo/...
extensions/common/mojo/wifi_display_session_service.mojom:13: Connect(int32
sink_id, int32 auth_method, string auth_data);
auth_method => idl enum?

https://codereview.chromium.org/1674583002/diff/60001/extensions/common/mojo/...
extensions/common/mojo/wifi_display_session_service.mojom:30: OnError(int32
type, string description);
type => enum that matches idl?

[Mikhail, 2016-02-11 14:42:58]

On 2016/02/11 08:56:25, shalamov wrote:
>
https://codereview.chromium.org/1674583002/diff/60001/extensions/common/mojo/...
> File extensions/common/mojo/wifi_display_session_service.mojom (right):
> 
>
https://codereview.chromium.org/1674583002/diff/60001/extensions/common/mojo/...
> extensions/common/mojo/wifi_display_session_service.mojom:13: Connect(int32
> sink_id, int32 auth_method, string auth_data);
> auth_method => idl enum?
> 
>
https://codereview.chromium.org/1674583002/diff/60001/extensions/common/mojo/...
> extensions/common/mojo/wifi_display_session_service.mojom:30: OnError(int32
> type, string description);
> type => enum that matches idl?

I would rather make a separate CL simplifying the error codes that the API
provides and it will include the changes which you've proposed (enums in .mojo).

[asargent_no_longer_on_chrome, 2016-02-11 17:14:00]

lgtm

https://codereview.chromium.org/1674583002/diff/60001/extensions/renderer/dis...
File extensions/renderer/display_source_custom_bindings.cc (right):

https://codereview.chromium.org/1674583002/diff/60001/extensions/renderer/dis...
extensions/renderer/display_source_custom_bindings.cc:299: static int sCallId;
Consider initializing to 0 just so that readers don't have to worry about
remembering/looking up details about in what cases variable zero-initialization
is guaranteed by the C++ standard and in which cases it's implementation
dependent. (I think in this case it's guaranteed)

[Mikhail, 2016-02-11 17:53:34]

The CQ bit was checked by mikhail.pozdnyakov@intel.com

[Mikhail, 2016-02-11 17:53:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
alexander.shalamov@intel.com, asargent@chromium.org
Link to the patchset: https://codereview.chromium.org/1674583002/#ps80001
(title: "Init sCallId")

[commit-bot: I haz the power, 2016-02-11 17:54:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1674583002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1674583002/80001

[commit-bot: I haz the power, 2016-02-11 19:42:30]

Description was changed from

==========
[chrome.displaySource] Session notification improvements

This patch provides several fixes and improvements (simplifications)
to the session life cycle notification mechanism:
1) The 'startSession'/'terminateSession' call completion callbacks are
invoked when the session is actually started/terminated and in accordance
with 'onSessionStarted'/'onSessionTerminated' events propagating (and as
per documentation).
2) The notification from the required sink is filtered out in browser process
avoiding unneeded IPC calls
3) Methods in mojo interface are renamed in order to improve readability

BUG=242107
==========

to

==========
[chrome.displaySource] Session notification improvements

This patch provides several fixes and improvements (simplifications)
to the session life cycle notification mechanism:
1) The 'startSession'/'terminateSession' call completion callbacks are
invoked when the session is actually started/terminated and in accordance
with 'onSessionStarted'/'onSessionTerminated' events propagating (and as
per documentation).
2) The notification from the required sink is filtered out in browser process
avoiding unneeded IPC calls
3) Methods in mojo interface are renamed in order to improve readability

BUG=242107
==========

[commit-bot: I haz the power, 2016-02-11 19:42:31]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2016-02-16 22:36:23]

Description was changed from

==========
[chrome.displaySource] Session notification improvements

This patch provides several fixes and improvements (simplifications)
to the session life cycle notification mechanism:
1) The 'startSession'/'terminateSession' call completion callbacks are
invoked when the session is actually started/terminated and in accordance
with 'onSessionStarted'/'onSessionTerminated' events propagating (and as
per documentation).
2) The notification from the required sink is filtered out in browser process
avoiding unneeded IPC calls
3) Methods in mojo interface are renamed in order to improve readability

BUG=242107
==========

to

==========
[chrome.displaySource] Session notification improvements

This patch provides several fixes and improvements (simplifications)
to the session life cycle notification mechanism:
1) The 'startSession'/'terminateSession' call completion callbacks are
invoked when the session is actually started/terminated and in accordance
with 'onSessionStarted'/'onSessionTerminated' events propagating (and as
per documentation).
2) The notification from the required sink is filtered out in browser process
avoiding unneeded IPC calls
3) Methods in mojo interface are renamed in order to improve readability

BUG=242107

Committed: https://crrev.com/ef0d3aa8a2e82e6db356e112c236ef0f8a2a8918
Cr-Commit-Position: refs/heads/master@{#374929}
==========

[commit-bot: I haz the power, 2016-02-16 22:36:24]

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