[System-Keyboard-Lock] Forward navigator functions to RenderFrameHost

third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp

+// Copyright 2017 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/keyboard_lock/NavigatorKeyboardLock.h"
+
+#include "bindings/core/v8/ScriptPromise.h"
+#include "bindings/core/v8/ScriptPromiseResolver.h"
+#include "bindings/core/v8/V8Binding.h"
+#include "core/frame/LocalFrame.h"
+#include "platform/heap/Persistent.h"
+#include "platform/wtf/Assertions.h"
+#include "platform/wtf/Functional.h"
+#include "public/platform/InterfaceProvider.h"
+
+namespace blink {
+
+NavigatorKeyboardLock::NavigatorKeyboardLock(Navigator& navigator)
+    : Supplement<Navigator>(navigator) {}
+
+NavigatorKeyboardLock& NavigatorKeyboardLock::From(Navigator& navigator) {
+  NavigatorKeyboardLock* supplement = static_cast<NavigatorKeyboardLock*>(
+      Supplement<Navigator>::From(navigator, SupplementName()));
+  if (!supplement) {
+    supplement = new NavigatorKeyboardLock(navigator);
+    ProvideTo(navigator, SupplementName(), supplement);
+  }
+  return *supplement;
+}
+
+// static
+ScriptPromise NavigatorKeyboardLock::requestKeyLock(
+    ScriptState* state,
+    Navigator& navigator,
+    const Vector<String>& keycodes) {
+  return NavigatorKeyboardLock::From(navigator).requestKeyLock(state, keycodes);
+}
+
+ScriptPromise NavigatorKeyboardLock::requestKeyLock(
+    ScriptState* state,
+    const Vector<String>& keycodes) {
+  DCHECK(state);
+  if (request_keylock_resolver_) {
+    // TODO(zijiehe): Reject with a DOMException once it has been defined in the
+    // spec. See https://github.com/garykac/system-keyboard-lock/issues/18.
+    return ScriptPromise::Reject(
+        state, V8String(state->GetIsolate(),
+                        "Last requestKeyLock() has not finished yet."));
+  }
+
+  String error_message;
+  if (!EnsureServiceConnected(&error_message)) {

[dcheng, 2017/04/26 13:27:17]

The only possible failure is if the frame is detached. That check should
probably just be a separate helper, e.g.

if (RejectPromiseIfDetached(state))
  return;

[Hzj_jie, 2017/04/26 22:05:56]

This comment is conflict with what Kentaro suggested.
I tend to agree if GetInterface() won't return nullptr, we should inline the
EnsureServiceConnected() function directly into requestKeyLock() and
cancelKeyLock() functions. But before I had a chance to talk with Daniel
directly, I would prefer to keep it as this to avoid any troubles caused by
misunderstanding.

[dcheng, 2017/04/27 02:53:10]

Oh I see; I missed that there's no promise being rejected for cancelKeyLock. I
agree with haraken@ then.

[Hzj_jie, 2017/04/28 00:10:31]

Since this change has been lasted for almost one month already, I would prefer
to submit this one first, and do the following upgrade in a new change.
1. Change the return type of IPC function RequestKeyLock() into an enumeration.
2. Remove the NavigatorKeyboardLock::EnsureServiceConnected() function.
3. Store the pending operations in NavigatorKeyboardLock, and retry after
connection has been generated.

[dcheng, 2017/04/28 02:24:49]

I'm OK with deferring 1 and 3, but 2 is easy enough to do and contained to this
CL, so please address 2.

[Hzj_jie, 2017/04/28 04:53:01]

Done.

+    // TODO(zijiehe): Retrying instead of rejecting the request if browser is
+    // not connected.
+    return ScriptPromise::Reject(state,
+                                 V8String(state->GetIsolate(), error_message));
+  }
+
+  request_keylock_resolver_ = ScriptPromiseResolver::Create(state);
+  service_->RequestKeyLock(
+      keycodes,
+      ConvertToBaseCallback(WTF::Bind(
+          &NavigatorKeyboardLock::LockRequestFinished, WrapPersistent(this))));
+  return request_keylock_resolver_->Promise();
+}
+
+void NavigatorKeyboardLock::cancelKeyLock() {
+  if (!EnsureServiceConnected(nullptr))
+    return;
+
+  service_->CancelKeyLock();
+}
+
+// static
+void NavigatorKeyboardLock::cancelKeyLock(Navigator& navigator) {
+  NavigatorKeyboardLock::From(navigator).cancelKeyLock();
+}
+
+bool NavigatorKeyboardLock::EnsureServiceConnected(String* error_message) {

[haraken, 2017/04/26 06:56:03]

I'd inline this method into the caller sites because once you want to handle the
exception properly, you anyway need to inline the logic in the caller sites
(e.g., requestKeyLock() rejects promises, cancelKeyLock() returns with silence
etc).

[Hzj_jie, 2017/04/26 22:05:56]

Please see my reply in Daniel's comment. I would keep this function as-is for
now, and talk with Daniel about the GetInterface() function once he returned.

+  if (!service_) {
+    LocalFrame* frame = GetSupplementable()->GetFrame();
+    if (!frame) {
+      if (error_message) {
+        *error_message = "Current frame is detached.";
+      }
+      return false;
+    }
+    frame->GetInterfaceProvider()->GetInterface(mojo::MakeRequest(&service_));
+  }
+
+  if (!service_.get()) {

[dcheng, 2017/04/26 13:27:17]

This block is unnecessary.

[Hzj_jie, 2017/04/26 22:05:56]

Since Daniel is OoO, and my understanding is GetInterface may still return an
empty service pointer, I would keep this check to make it safer. We can update
this logic once I talked with Daniel about the detail.

[dcheng, 2017/04/27 02:53:10]

As discussed offline, this should work fine assuming the static configuration is
done correctly (i.e. this should be a DCHECK if it's checked at all). It seems
there might be an issue with the manifest config still though, so let's see what
the trybots say.

[Hzj_jie, 2017/04/28 00:10:31]

Done.

+    if (error_message) {
+      *error_message = "Browser is not connecting.";
+    }
+    return false;
+  }
+
+  return true;
+}
+
+void NavigatorKeyboardLock::LockRequestFinished(bool allowed,
+                                                const String& reason) {
+  DCHECK(request_keylock_resolver_);
+  // TODO(zijiehe): Reject with a DOMException once it has been defined in the
+  // spec.
+  if (allowed)
+    request_keylock_resolver_->Resolve();
+  else
+    request_keylock_resolver_->Reject(reason);
+  request_keylock_resolver_ = nullptr;
+}
+
+// static
+const char* NavigatorKeyboardLock::SupplementName() {
+  return "NavigatorKeyboardLock";
+}
+
+DEFINE_TRACE(NavigatorKeyboardLock) {
+  visitor->Trace(request_keylock_resolver_);
+  Supplement<Navigator>::Trace(visitor);
+}
+
+}  // namespace blink