bluetooth: Disconnect if the page becomes hidden or is closed.

third_party/WebKit/Source/modules/bluetooth/BluetoothGATTRemoteServer.cpp

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "modules/bluetooth/BluetoothGATTRemoteServer.h"
 
 #include "bindings/core/v8/CallbackPromiseAdapter.h"
 #include "bindings/core/v8/ScriptPromise.h"
 #include "bindings/core/v8/ScriptPromiseResolver.h"
 #include "core/dom/DOMException.h"
+#include "core/dom/Document.h"
 #include "core/dom/ExceptionCode.h"
 #include "modules/bluetooth/BluetoothError.h"
 #include "modules/bluetooth/BluetoothGATTService.h"
 #include "modules/bluetooth/BluetoothSupplement.h"
 #include "modules/bluetooth/BluetoothUUID.h"
 #include "public/platform/modules/bluetooth/WebBluetooth.h"
 #include "wtf/OwnPtr.h"
 
 namespace blink {
+namespace {
 
-BluetoothGATTRemoteServer::BluetoothGATTRemoteServer(PassOwnPtr<WebBluetoothGATTRemoteServer> webGATT)
-    : m_webGATT(webGATT)
+bool isPageVisible(Page* page)

[haraken, 2016/01/23 02:02:02]

Can we add this helper function to Page? (i.e., Page::isVisible())

[ortuno, 2016/01/25 23:55:07]

done.

 {
+    return page->visibilityState() == PageVisibilityStateVisible;
 }
 
-BluetoothGATTRemoteServer* BluetoothGATTRemoteServer::take(ScriptPromiseResolver*, PassOwnPtr<WebBluetoothGATTRemoteServer> webGATT)
+} // namespace
+BluetoothGATTRemoteServer::BluetoothGATTRemoteServer(ExecutionContext* context, PassOwnPtr<WebBluetoothGATTRemoteServer> webGATT)
+    : ActiveDOMObject(context)
+    , PageLifecycleObserver(toDocument(context)->page())
+    , m_webGATT(webGATT)
+{
+    // See example in Source/platform/heap/ThreadState.h
+    ThreadState::current()->registerPreFinalizer(this);
+}
+
+BluetoothGATTRemoteServer* BluetoothGATTRemoteServer::take(ScriptPromiseResolver* resolver, PassOwnPtr<WebBluetoothGATTRemoteServer> webGATT)
 {
     ASSERT(webGATT);
-    return new BluetoothGATTRemoteServer(webGATT);
+    BluetoothGATTRemoteServer* server = new BluetoothGATTRemoteServer(resolver->executionContext(), webGATT);
+    if (!isPageVisible(server->page())) {
+        server->disconnectIfConnected();
+    }
+    server->suspendIfNeeded();
+    return server;
+}
+
+void BluetoothGATTRemoteServer::dispose()
+{
+    disconnectIfConnected();
+}
+
+void BluetoothGATTRemoteServer::stop()
+{
+    disconnectIfConnected();
+}
+
+void BluetoothGATTRemoteServer::pageVisibilityChanged()
+{
+    if (!isPageVisible(page())) {
+        disconnectIfConnected();
+    }
+}
+
+void BluetoothGATTRemoteServer::disconnectIfConnected()
+{
+    if (m_webGATT->connected) {
+        m_webGATT->connected = false;
+        WebBluetooth* webbluetooth = BluetoothSupplement::fromExecutionContext(executionContext());

[haraken, 2016/01/23 02:02:02]

I think we've discussed this before, but it's a bit nasty to have
BluetoothSupplement::fromExecutionContext and
BluetoothSupplement::fromScriptState. Given that BluetoothSupplement is a
supplement of a Frame, it should provide only
BluetoothSupplement::from(LocalFrame*).

The caller side (e.g., BluetoothGATTRemoteServer) should hold a Member<Frame>
and use BluetoothSupplement::from(m_frame) to retrieve the supplement.

(In the case of BluetoothGATTRemoteServer, it doesn't need to hold m_frame
because it can retrieve the frame from executionContext(). It can just provide
BluetoothGATTRemoteServer::frame() which returns the frame associated with
executionContext(). Then it can call BluetoothSupplement::from(frame())).

(You don't need to address this in this CL.)

[Jeffrey Yasskin, 2016/01/23 02:33:36]

FWIW, I disagree. There are uses of BluetoothSupplement in 5 different files,
none of which have a frame immediately available. They all have either a
ScriptState or an ExecutionContext, and accepting these in methods on
BluetoothSupplement allows us to centralize the non-trivial knowledge of how to
get from the object we have to the Frame. I don't want to duplicate this code.

+        webbluetooth->disconnect(m_webGATT->deviceId);
+    }
+}
+
+DEFINE_TRACE(BluetoothGATTRemoteServer)
+{
+    ActiveDOMObject::trace(visitor);
+    PageLifecycleObserver::trace(visitor);
 }
 
 void BluetoothGATTRemoteServer::disconnect(ScriptState* scriptState)
 {
     m_webGATT->connected = false;
     WebBluetooth* webbluetooth = BluetoothSupplement::fromScriptState(scriptState);

[haraken, 2016/01/23 02:02:02]

This is not really nice for the same reason (although it's not wrong).

We want to write this like BluetoothSupplement::from(frame())). Then you don't
need to pass in ScriptState.

You don't need to address this in this CL but consider removing
BluetoothSupplement::fromScriptState and
BluetoothSupplement::fromExecutionContext in a follow-up.

[ortuno, 2016/01/25 23:55:07]

Definitely in my TODO list. We used to be targeting workers that's why we
couldn't store the frame. Ever since we changed that I've been meaning to
refactor this pattern.

     webbluetooth->disconnect(m_webGATT->deviceId);
 }
 
 ScriptPromise BluetoothGATTRemoteServer::getPrimaryService(ScriptState* scriptState, const StringOrUnsignedLong& service, ExceptionState& exceptionState)
 {
     WebBluetooth* webbluetooth = BluetoothSupplement::fromScriptState(scriptState);
 
     String serviceUUID = BluetoothUUID::getService(service, exceptionState);
     if (exceptionState.hadException())
         return exceptionState.reject(scriptState);
 
     ScriptPromiseResolver* resolver = ScriptPromiseResolver::create(scriptState);
     ScriptPromise promise = resolver->promise();
     webbluetooth->getPrimaryService(m_webGATT->deviceId, serviceUUID, new CallbackPromiseAdapter<BluetoothGATTService, BluetoothError>(resolver));
 
     return promise;
 }
 
 } // namespace blink