Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds a new "connecting" state and per-state event handlers. #227

Merged
merged 7 commits into from Dec 21, 2015

Conversation

@mfoltzgoogle
Copy link
Contributor

commented Dec 15, 2015

This PR addresses the following issues:

Summary of changes:

  • Adds a new "connecting" state to the PresentationConnection (Issue #214)
  • Adds per-state event handlers onconnected, onclosed and onterminated to replace onstatechange
  • Adds a PresentationConnectionCloseEvent and PresentationConnectionCloseReason with details on why a connection was closed, including an error in the underlying message transport (Issue #149).
  • Refine example code and algorithms to clarify initial presentation states and ensure message sending happens at the right time. (Issue #198, Issue #216)
@mfoltzgoogle

This comment has been minimized.

Copy link
Contributor Author

commented Dec 16, 2015

This is ready for review. @sicking, @mounirlamouri, @avayvod and others, please take a look.

</dd>
<dd>
<code>closeReason</code>, the
<code>PresenentationConnectionClosedReason</code> describing why

This comment has been minimized.

Copy link
@tidoust

tidoust Dec 16, 2015

Contributor

Typo: "PresenentationConnectionClosedReason" should be "PresentationConnectionClosedReason"

@avayvod

This comment has been minimized.

Copy link
Contributor

commented Dec 16, 2015

The description mentions issue numbers inconsistently:

  • the list has issues #217, #216, #196, #149 while the summary below only references #216, #196, #149 (so no #217) and a new #214;
  • #149 doesn't seem to be related to per-state event handlers as the summary suggests though I might be wrong
is not <code>"connected"</code>, throw an
<code>InvalidStateError</code> exception.
<li>If the <a>state</a> property of
<code>presentationConnection</code> is not <code>connected</code>,

This comment has been minimized.

Copy link
@avayvod

avayvod Dec 16, 2015

Contributor

In #214 we speculate about allowing messaging to/from PresentationConnection in a "connecting" state. Do you plan to add this in a follow up PR? Without this difference from "disconnected" state, it's not clear why we just not resolve the start() promise with the "disconnected" state that changes to "connected" later on. All I can think of is it will be clearer for the Web developer whether to expect the transition to "connected" or not but it might not be good enough to justify adding a new state then.

This comment has been minimized.

Copy link
@mounirlamouri

mounirlamouri Dec 16, 2015

Member

I think an event based approach would be confused with a life cycle like disconnected -> connected -> closed -> disconnected because it could do:

myConnecton.onstatechange = function() {
  switch (this.state) {
    case 'connected':
      showRemoteControlUI();
      break;
    case 'closed':
      addButtonToJoin();
      break;
    case 'disconnected':
      hideRemoteControlUI();
      break;
  }
};

This comment has been minimized.

Copy link
@mfoltzgoogle

mfoltzgoogle Dec 18, 2015

Author Contributor

I think naming the state "connecting" is clearer than "disconnected" per consistency with other Web APIs (i.e. WebSockets) and the example code above, as well as in the PR.

<li>If the <a>state</a> property of <a>PresentationConnection</a>
is not <code>"connected"</code>, abort these steps.
<li>If the <a>state</a> property of
<code>presentationConnection</code> is not <code>connected</code>,

This comment has been minimized.

Copy link
@avayvod

avayvod Dec 16, 2015

Contributor

Ditto.

@mfoltzgoogle

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2015

@avayvod Re: #227 (comment)

Sorry about the poorly formatted description. I've updated it to fix the issues you pointed out.

@mfoltzgoogle

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2015

Unless there's further feedback, I'll do one more editorial pass to clean up anything minor with this PR (i.e. fixing TODOs) then plan on merging. As followups the following issues should be addressed:

  • #228 - GitHub API failing, which leads to ReSpec failures
  • #229 - Need algorithm for establishing a presentation connection when the previous connection(s) were closed
  • #231 - Wrong use of "Queue a task" and "in parallel" in algorithms
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.