[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_) {
+    return ScriptPromise::Reject(
+        state, V8String(state->GetIsolate(),

[foolip, 2017/04/17 08:46:04]

To reject with a string is somewhat unusual I think. Please work out on the spec
side how it should be rejected, with TypeError or DOMException of some sort.

[Hzj_jie, 2017/04/18 02:26:08]

I am working with Gary to update the spec.

+                        "Last requestKeyLock() has not finished yet."));
+  }
+
+  if (!service_) {
+    LocalFrame* frame = GetSupplementable()->GetFrame();
+    if (!frame) {
+      return ScriptPromise::Reject(
+          state, V8String(state->GetIsolate(), "Current frame is detached."));
+    }
+    frame->GetInterfaceProvider()->GetInterface(mojo::MakeRequest(&service_));
+  }
+
+  if (!service_.get()) {
+    return ScriptPromise::Reject(
+        state, V8String(state->GetIsolate(), "Browser is not connecting."));

[foolip, 2017/04/17 08:46:05]

Ditto about this rejection. You will still need a description string, and
"Browser is not connecting" is hard for web developers to understand I'm afraid.
Is the cause here unknown, under what circumstances could we fail to create
service_?

[Hzj_jie, 2017/04/18 02:26:08]

I believe the spec won't cover the detail of exceptions here and the one above:
they are Chrome implementation detail. Maybe we can add a "Other implementation
limitations" error type in the spc and use similar DOMException with different
error strings.

For this case, I believe this could happen if the browser does not provide these
APIs.

[foolip, 2017/04/18 05:10:57]

When would this exception happen? It's possible it would be covered by a
spec-side statement like about the browsing context being discarded, or the
associated document not being fully active. This is the navigator object, and it
doesn't look like the spec fully defines when it's replaced, so there's no
concept of "active" that corresponds to the GetFrame() checks we use in
Navigator.cpp, so I've filed https://github.com/whatwg/html/issues/2545

If there is any way for a web page to get a promise that is rejected on this
line, then it's web observable and should correspond to the spec in some way. If
it turns out that this is blocked on adding some concepts to HTML, please file a
spec bug for keyboard lock that links to it, and add a TODO pointing to the
keyboard lock issue here.

[Hzj_jie, 2017/04/19 00:45:54]

Sorry, I mean the browser process does not provide these APIs, but the renderer
process requires them, which is a Chrome specific implementation detail.

TODO has been added.

[foolip, 2017/04/21 06:08:32]

I'd still like to understand exactly when this can happen. If it could be
subsumed by a broader "if x then reject" in the spec, then it would cease to be
a Chrome-specific behavior.

[Hzj_jie, 2017/04/22 01:20:24]

After discussion with Jamie, we believe here we should retry instead of return
an error to the web page.
But considering it's not a one-line change, and this change already took so long
time, I would prefer to do it in a separate change.

In the spec level, this should be covered by the statement of rejections,
something like, "User agent should only reject and discard the request in
following situations: 1. the request is not allowed by the user. 2. there is
already a pending request."

+  }
+
+  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 (!service_) {

[foolip, 2017/04/17 08:46:04]

Can you break out the shared code here into a private void EnsureService()?
(Name not important.)

[Hzj_jie, 2017/04/18 02:26:08]

Done.

+    LocalFrame* frame = GetSupplementable()->GetFrame();
+    if (!frame)
+      return;
+    frame->GetInterfaceProvider()->GetInterface(mojo::MakeRequest(&service_));
+  }
+
+  if (!service_.get())
+    return;
+
+  service_->CancelKeyLock(ConvertToBaseCallback(WTF::Bind([]() {})));

[foolip, 2017/04/17 08:46:05]

Why does service_->CancelKeyLock take a callback at all? Can you remove it if
it's not needed, or make it optional if it's needed somewhere else?

[Hzj_jie, 2017/04/18 02:26:08]

We have also discussed whether the cancelKeyLock() function should return a
Promise<void> to tell the web page once this operation is finished. But the
decision has not been made.

I do not think mojo supports optional callback.

[foolip, 2017/04/18 05:10:57]

Doesn't mojo support fire-and-forget operations, where you don't care about the
result?

[Hzj_jie, 2017/04/19 00:45:54]

Yes, the new change uses no-callback operation. But mojo does not support an
optional callback. i.e. =>? is not supported in mojo.

+}
+
+// static
+void NavigatorKeyboardLock::cancelKeyLock(Navigator& navigator) {
+  NavigatorKeyboardLock::From(navigator).cancelKeyLock();
+}
+
+void NavigatorKeyboardLock::LockRequestFinished(bool allowed,
+                                                const String& reason) {
+  DCHECK(request_keylock_resolver_);
+  if (allowed)
+    request_keylock_resolver_->Resolve();
+  else
+    request_keylock_resolver_->Reject(reason);

[foolip, 2017/04/17 08:46:04]

Ditto about rejecting with a string.

[Hzj_jie, 2017/04/18 02:26:08]

TODO has been added.

+  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