[Presentation API] fire onconnectionavailable and onconnect event asynchronously

[Presentation API] fire onconnectionavailable and onconnect event asynchronously

We would like to ensure following execution order when calling PresentationRequest.start():

1. resolve start() promise
2. fire PresentationRequest onconnectionavailable event
3. fire PresentationConnection onconnect event

To fire event after resolving promise, we need to dispatch events asynchronously

BUG=669213
Committed: https://crrev.com/4e188d7c91feaff452f8cf8c93ac48a4393b8e5b
Cr-Commit-Position: refs/heads/master@{#437598}

[zhaobin, 2016-12-02 19:52:00]

zhaobin@chromium.org changed reviewers:
+ imcheng@chromium.org, mfoltz@chromium.org, mlamouri@chromium.org

[mark a. foltz, 2016-12-03 01:36:36]

lgtm

[mlamouri (slow - plz ping), 2016-12-05 13:39:10]

https://codereview.chromium.org/2547143002/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/presentation/PresentationConnectionCallbacks.cpp
(right):

https://codereview.chromium.org/2547143002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/presentation/PresentationConnectionCallbacks.cpp:47:
wrapPersistent(connection)));
Any reason I'm missing why this is not in the ::take() from
PresentationConnection?

[imcheng, 2016-12-05 19:34:19]

I am a bit concerned about posting task to dispatch the onconnect event. Doesn't
that mean we can potentially get the state transition events out-of-order? To
fix this it seems we will have to post task for all state change events?

[zhaobin, 2016-12-05 22:39:42]

Changed to fire all state change events asynchronously.

mlamouri@ according to spec we would like to:

1. resolve presentationRequest.start().then() promise
2. fire onconnectionavailable event

If we fire event in ::take() or do not post an async task for state change
event, onconnectionavailable event will be fired first.

[mlamouri (slow - plz ping), 2016-12-06 09:53:08]

On 2016/12/05 at 22:39:42, zhaobin wrote:
> Changed to fire all state change events asynchronously.
> 
> mlamouri@ according to spec we would like to:
> 
> 1. resolve presentationRequest.start().then() promise
> 2. fire onconnectionavailable event
> 
> If we fire event in ::take() or do not post an async task for state change
event, onconnectionavailable event will be fired first.

That's fine. But could the event firing be done in ::take() instead of
implemented in the Callbacks code? It's a more streamline design to have
::take() implement the logic instead of Callbacks.

[zhaobin, 2016-12-06 19:58:47]

Resolving Mounir's comments: firing onConnectionAvailable event in ::take()
instead of in PresentationConnectionCallbacks.

[mark a. foltz, 2016-12-07 04:23:55]

https://codereview.chromium.org/2547143002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp
(right):

https://codereview.chromium.org/2547143002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp:470:
void PresentationConnection::dispatchEventAsync(Event* event) {
Can this be combined into dispatchStateChangeEvent?

https://codereview.chromium.org/2547143002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationConnection.h
(right):

https://codereview.chromium.org/2547143002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationConnection.h:117: //
Internal helper function to dispatch connect event asynchronously.
...dispatch state change events...

[mlamouri (slow - plz ping), 2016-12-07 14:45:35]

lgtm with mfoltz comments applied

[zhaobin, 2016-12-07 19:30:44]

https://codereview.chromium.org/2547143002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp
(right):

https://codereview.chromium.org/2547143002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp:470:
void PresentationConnection::dispatchEventAsync(Event* event) {
On 2016/12/07 04:23:55, mark a. foltz wrote:
> Can this be combined into dispatchStateChangeEvent?

Not sure how to combine them neatly. postTask expects void return type, while
dispatchEvent returns DispatchEventResult. We need some function wrapper to
convert return type.

https://codereview.chromium.org/2547143002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationConnection.h
(right):

https://codereview.chromium.org/2547143002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationConnection.h:117: //
Internal helper function to dispatch connect event asynchronously.
On 2016/12/07 04:23:55, mark a. foltz wrote:
> ...dispatch state change events...

Done.

[mark a. foltz, 2016-12-07 21:32:44]

lgtm

https://codereview.chromium.org/2547143002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp
(right):

https://codereview.chromium.org/2547143002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp:470:
void PresentationConnection::dispatchEventAsync(Event* event) {
On 2016/12/07 at 19:30:43, zhaobin wrote:
> On 2016/12/07 04:23:55, mark a. foltz wrote:
> > Can this be combined into dispatchStateChangeEvent?
> 
> Not sure how to combine them neatly. postTask expects void return type, while
dispatchEvent returns DispatchEventResult. We need some function wrapper to
convert return type.

Ah, got it.  Slight preference to call this method dispatchStateChangeEvent and
to name the method below dispatchEventAsync.

It would also be possible to write a static utility function void
dispatchEventAsync(EventTarget*, Event*) to be used with PostTask here and above
for connectionavailable, but I don't feel strongly.

[zhaobin, 2016-12-08 00:16:46]

https://codereview.chromium.org/2547143002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp
(right):

https://codereview.chromium.org/2547143002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp:470:
void PresentationConnection::dispatchEventAsync(Event* event) {
On 2016/12/07 21:32:44, mark a. foltz wrote:
> On 2016/12/07 at 19:30:43, zhaobin wrote:
> > On 2016/12/07 04:23:55, mark a. foltz wrote:
> > > Can this be combined into dispatchStateChangeEvent?
> > 
> > Not sure how to combine them neatly. postTask expects void return type,
while
> dispatchEvent returns DispatchEventResult. We need some function wrapper to
> convert return type.
> 
> Ah, got it.  Slight preference to call this method dispatchStateChangeEvent
and
> to name the method below dispatchEventAsync.
> 
> It would also be possible to write a static utility function void
> dispatchEventAsync(EventTarget*, Event*) to be used with PostTask here and
above
> for connectionavailable, but I don't feel strongly.

Done.

[zhaobin, 2016-12-09 02:11:27]

The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 02:11:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mark a. foltz, 2016-12-09 05:20:50]

lgtm 

Nice :)

[commit-bot: I haz the power, 2016-12-09 05:32:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-09 05:32:37]

Dry run: This issue passed the CQ dry run.

[zhaobin, 2016-12-09 18:38:54]

The CQ bit was checked by zhaobin@chromium.org

[zhaobin, 2016-12-09 18:38:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2547143002/#ps80001
(title: "resolve code review comments from Mark")

[commit-bot: I haz the power, 2016-12-09 18:39:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-09 18:49:08]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481308734465020,
"parent_rev": "8434bba588e285d8a2d0f8442cb6e0ae39c3c20f", "commit_rev":
"385ba88d0b5c58d21b59e6bffe109cd5359f18ce"}

[commit-bot: I haz the power, 2016-12-09 18:49:34]

Description was changed from

==========
[Presentation API] fire onconnectionavailable and onconnect event asynchronously

We would like to ensure following execution order when calling
PresentationRequest.start():

1. resolve start() promise
2. fire PresentationRequest onconnectionavailable event
3. fire PresentationConnection onconnect event

To fire event after resolving promise, we need to dispatch events asynchronously

BUG=669213
==========

to

==========
[Presentation API] fire onconnectionavailable and onconnect event asynchronously

We would like to ensure following execution order when calling
PresentationRequest.start():

1. resolve start() promise
2. fire PresentationRequest onconnectionavailable event
3. fire PresentationConnection onconnect event

To fire event after resolving promise, we need to dispatch events asynchronously

BUG=669213

Review-Url: https://codereview.chromium.org/2547143002
==========

[commit-bot: I haz the power, 2016-12-09 18:49:35]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-12 14:36:35]

Description was changed from

==========
[Presentation API] fire onconnectionavailable and onconnect event asynchronously

We would like to ensure following execution order when calling
PresentationRequest.start():

1. resolve start() promise
2. fire PresentationRequest onconnectionavailable event
3. fire PresentationConnection onconnect event

To fire event after resolving promise, we need to dispatch events asynchronously

BUG=669213

Review-Url: https://codereview.chromium.org/2547143002
==========

to

==========
[Presentation API] fire onconnectionavailable and onconnect event asynchronously

We would like to ensure following execution order when calling
PresentationRequest.start():

1. resolve start() promise
2. fire PresentationRequest onconnectionavailable event
3. fire PresentationConnection onconnect event

To fire event after resolving promise, we need to dispatch events asynchronously

BUG=669213

Committed: https://crrev.com/4e188d7c91feaff452f8cf8c93ac48a4393b8e5b
Cr-Commit-Position: refs/heads/master@{#437598}
==========

[commit-bot: I haz the power, 2016-12-12 14:36:37]

Patchset 5 (id:??) landed as
https://crrev.com/4e188d7c91feaff452f8cf8c93ac48a4393b8e5b
Cr-Commit-Position: refs/heads/master@{#437598}