[Presentation API] Resolve PresentationRequest::reconnect() with existing presentation connection i…

third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp

Index: third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp
diff --git a/third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp b/third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp
index d26008add8582a518c4962370abd62d3271ca54a..8b37b1d3cb2efd2f857f7b26eb439cb7fde8b966 100644
--- a/third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp
+++ b/third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp
@@ -28,14 +28,18 @@ namespace blink {
 namespace {
 
 // TODO(mlamouri): refactor in one common place.
-WebPresentationClient* presentationClient(ExecutionContext* executionContext) {
+PresentationController* presentationController(
+    ExecutionContext* executionContext) {
   DCHECK(executionContext);
 
   Document* document = toDocument(executionContext);
   if (!document->frame())
     return nullptr;
-  PresentationController* controller =
-      PresentationController::from(*document->frame());
+  return PresentationController::from(*document->frame());
+}
+
+WebPresentationClient* presentationClient(ExecutionContext* executionContext) {
+  PresentationController* controller = presentationController(executionContext);
   return controller ? controller->client() : nullptr;
 }
 
@@ -146,6 +150,13 @@ ScriptPromise PresentationRequest::start(ScriptState* scriptState) {
 
 ScriptPromise PresentationRequest::reconnect(ScriptState* scriptState,
                                              const String& id) {
+  if (!getExecutionContext()) {

[mlamouri (slow - plz ping), 2017/01/03 12:02:11]

Why are you adding this check? can the ExecutionContext be null here? If I
understand correctly, the current assumes it's never null because
`presentationClient()` does a `DCHECK()` on the `executionContext`.

[zhaobin, 2017/01/03 20:12:07]

toDocument(getExecutionContext())->isSandboxed(...) uses getExecutionContext()
before presentationClient()?

haraken commented in patch set 1:

"Not related to this CL, getExecutionContext() returns null when the context is
detached from the frame."

Thought that he suggests to add a check for getExecutionContext().

+    return ScriptPromise::rejectWithDOMException(
+        scriptState,
+        DOMException::create(InvalidStateError,
+                             "The context is detached from the frame."));
+  }
+
   if (MixedContentChecker::isMixedContent(
           getExecutionContext()->getSecurityOrigin(), m_url)) {
     return rejectWithMixedContentException(scriptState, m_url.getString());
@@ -166,9 +177,23 @@ ScriptPromise PresentationRequest::reconnect(ScriptState* scriptState,
   // TODO(crbug.com/627655): Accept multiple URLs per PresentationRequest.
   WebVector<WebURL> presentationUrls(static_cast<size_t>(1U));
   presentationUrls[0] = m_url;
-  client->joinSession(
-      presentationUrls, id,
-      WTF::makeUnique<PresentationConnectionCallbacks>(resolver, this));
+
+  PresentationController* controller =
+      presentationController(getExecutionContext());
+  DCHECK(controller);
+
+  PresentationConnection* existingConnection =
+      controller->findExistingConnection(presentationUrls, id);
+  if (existingConnection) {
+    client->joinSession(
+        presentationUrls, id,
+        WTF::makeUnique<ExistingPresentationConnectionCallbacks>(
+            resolver, existingConnection));
+  } else {
+    client->joinSession(
+        presentationUrls, id,
+        WTF::makeUnique<PresentationConnectionCallbacks>(resolver, this));
+  }
   return resolver->promise();
 }