Mojo-ify implementation of screen orientation locking/unlocking.

content/browser/screen_orientation/screen_orientation_impl.cc

Index: content/browser/screen_orientation/screen_orientation_impl.cc
diff --git a/content/browser/screen_orientation/screen_orientation_impl.cc b/content/browser/screen_orientation/screen_orientation_impl.cc
new file mode 100644
index 0000000000000000000000000000000000000000..5d58cad5a03b85cb9bc01527de5648006e458b53
--- /dev/null
+++ b/content/browser/screen_orientation/screen_orientation_impl.cc
@@ -0,0 +1,84 @@
+// Copyright 2016 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 "content/browser/screen_orientation/screen_orientation_impl.h"
+
+#include "base/bind.h"
+#include "content/public/browser/navigation_details.h"
+#include "content/public/browser/screen_orientation_provider.h"
+#include "content/public/browser/web_contents.h"
+
+namespace content {
+
+using typename ::blink::mojom::ScreenOrientationLockResult;
+
+ScreenOrientation::ScreenOrientation(WebContents* web_contents)
+    : web_contents_(web_contents),

[blundell, 2016/10/25 16:09:26]

I think you need to call the WCO constructor here and pass it |web_contents|,
right?

[lunalu1, 2016/11/03 23:15:25]

Done.

+      binding_(web_contents, this),
+      weak_factory_(this) {
+  provider_.reset(new ScreenOrientationProvider(web_contents));
+}
+
+ScreenOrientation::~ScreenOrientation() = default;
+
+void ScreenOrientation::LockOrientation(
+    blink::WebScreenOrientationLockType orientation,
+    const LockOrientationCallback& callback) {
+  // When a request comes in while another request was already outstanding.

[blundell, 2016/10/25 16:09:26]

nit: I don't think that we need this comment.

[lunalu1, 2016/11/03 23:15:25]

Done.

+  if (!on_result_callback_.is_null()) {
+    NotifyLockResult(ScreenOrientationLockResult::

[blundell, 2016/10/25 16:09:26]

is this formatting thanks to "git cl format"? 

[lunalu1, 2016/11/03 23:15:25]

Unfortunately yes. But I will make it less ugly. 

+                         SCREEN_ORIENTATION_LOCK_RESULT_ERROR_CANCELED);
+  }
+
+  on_result_callback_ = callback;
+
+  if (!provider_) {
+    NotifyLockResult(ScreenOrientationLockResult::
+                         SCREEN_ORIENTATION_LOCK_RESULT_ERROR_NOT_AVAILABLE);
+    return;
+  }
+
+  provider_->LockOrientation(
+      orientation, base::Bind(&ScreenOrientation::NotifyLockResult,
+                              weak_factory_.GetWeakPtr()));

[blundell, 2016/10/25 16:09:26]

This object owns |provider_|, correct? If so there's no need to pass a WeakPtr
here.

[lunalu1, 2016/11/03 23:15:25]

Done.

+}
+
+void ScreenOrientation::UnlockOrientation() {
+  // Cancel any pending lock request.
+  NotifyLockResult(ScreenOrientationLockResult::
+                       SCREEN_ORIENTATION_LOCK_RESULT_ERROR_CANCELED);
+
+  if (provider_)
+    provider_->UnlockOrientation();
+}
+
+void ScreenOrientation::DidNavigateMainFrame(
+    const LoadCommittedDetails& details,
+    const FrameNavigateParams& params) {
+  if (!provider_ || details.is_in_page)
+    return;
+  provider_->UnlockOrientation();
+}
+
+void ScreenOrientation::NotifyLockResult(
+    ScreenOrientationLockResult result) {
+  if (on_result_callback_.is_null())
+    return;
+  if (result ==
+          ScreenOrientationLockResult::SCREEN_ORIENTATION_LOCK_RESULT_SUCCESS &&
+      binding_.GetCurrentTargetFrame() != web_contents_->GetMainFrame())

[blundell, 2016/10/25 16:09:26]

This still seems strange to me. Can you go through the explanation again?

[Ken Rockot(use gerrit already), 2016/10/26 21:59:54]

This is correct. It's how WebContentsFrameBindingSet plumbs RenderFrameHost
context that used to be plumbed via WebContentsObserver::OnMessageReceived.

[blundell, 2016/10/26 23:07:43]

I still don't think this is correct. This callback is happening in response to
the orientation changing. It's not occurring (in general) during dispatch of an
incoming Mojo message.

The way that the old code worked here is that it saved the (RFH ID, RPH ID) of
the latest incoming lock request from the renderer and it used those to obtain
the correct RFH to send back the response on (cf.
ScreenOrientationDispatcherHostImpl::NotifyLockSuccess(int request_id)). What
this corresponds to in the new world is simply calling |on_result_callback_|,
since |on_result_callback_| is implicitly bound to the most recent renderer-side
caller.

In short, unless I'm missing something, lines 68-70 here should just be omitted.

[lunalu1, 2016/10/27 22:22:26]

Done.

[lunalu1, 2016/10/27 22:22:26]

Seems like Colin is right. With this 4 lines of code, the lock success callback
was never successfully returned. Without the 4 lines, the lock/unlock
orientation works as expected. (In the scenario of calling lock within an
iframe).

+    return;
+
+  on_result_callback_.Run(result);
+
+  // Reset the callback.
+  on_result_callback_ = LockOrientationCallback();
+}
+
+ScreenOrientationProvider*
+ScreenOrientation::GetScreenOrientationProvider() {
+  return provider_.get();
+}
+
+}  // namespace content