Do not fallback to FocusedOrMainFrame() in FocusedLocalFrameInWidget()

third_party/WebKit/Source/web/WebViewImpl.cpp

Index: third_party/WebKit/Source/web/WebViewImpl.cpp
diff --git a/third_party/WebKit/Source/web/WebViewImpl.cpp b/third_party/WebKit/Source/web/WebViewImpl.cpp
index 9d68a6777ef6392c804aad67173f0ca12cef93b4..204c9a2d9f51158dc3942b4c98612ccecb257ec7 100644
--- a/third_party/WebKit/Source/web/WebViewImpl.cpp
+++ b/third_party/WebKit/Source/web/WebViewImpl.cpp
@@ -356,7 +356,6 @@ WebViewImpl::WebViewImpl(WebViewClient* client,
       fake_page_scale_animation_use_anchor_(false),
       compositor_device_scale_factor_override_(0),
       suppress_next_keypress_event_(false),
-      ime_accept_events_(true),
       dev_tools_emulator_(nullptr),
       tabs_to_links_(false),
       layer_tree_view_(nullptr),
@@ -2289,9 +2288,9 @@ void WebViewImpl::MouseCaptureLost() {
 
 void WebViewImpl::SetFocus(bool enable) {
   page_->GetFocusController().SetFocused(enable);
+  LocalFrame* focused_frame = page_->GetFocusController().FocusedFrame();
   if (enable) {
     page_->GetFocusController().SetActive(true);
-    LocalFrame* focused_frame = page_->GetFocusController().FocusedFrame();
     if (focused_frame) {
       Element* element = focused_frame->GetDocument()->FocusedElement();
       if (element && focused_frame->Selection()
@@ -2313,43 +2312,47 @@ void WebViewImpl::SetFocus(bool enable) {
               SelectionInDOMTree::Builder().Collapse(position).Build());
         }
       }
+      // TODO(ekaramad): We need to figure out the right way to propagate page
+      // focus to OOPIFs. Right now WebFrameWidgetImpl::SetFocus never gets
+      // called (https://crbug.com/689777).
+      WebLocalFrameBase::FromFrame(focused_frame->LocalFrameRoot())
+          ->FrameWidget()
+          ->ime_accept_events_ = true;

[alexmos, 2017/06/15 02:53:06]

Ugh, really hope we can clean this up soon.  I agree with you that it'd be nice
to not dispatch IME events without page focus in the first place, but that's
probably not for this CL.  Is there a use case for page focus in WebFrameWidgets
other than IME?  If so, we should just consider calling
WebFrameWidgetImpl::SetFocus.

[EhsanK, 2017/06/16 19:25:14]

Agreed. I have been meaning to get started on this soon. I will try to put up a
fix soon after this CL.
From looking at WebFrameWidgetImpl::SetFocus(enabled)

1- if (enabled): there is some reference to FocusController().SetActive() which
is perhaps incorrect since this should be done by WebViewImpl instead. Other
than that there seems to be some frame selection upates on the focused frame.

2- if(!enabled): this is the case where we commit an ongoing IME composition.
Again dealing with the focused frame.

From 1 & 2 I actually do not see a compelling reason to keep
WebFrameWidget::SetFocus and perhaps it could live inside WebView.h instead.
However, it will be quite nice to expose WebView in RenderWidget so that when
necessary (i.e., handling IME IPCs), RenderWidget can drop IPC if WebView is not
focused.

I will try to investigate this soon.

[alexmos, 2017/06/17 01:06:43]

Sounds good.  The other concern I remember with this in the past was  that this
is really about page-level focus, so it made more sense to do it once per-view
rather than multiple times for each widget.  I'll defer to Daniel about exposing
WebView.

     }
-    ime_accept_events_ = true;
   } else {
     HidePopups();
 
     // Clear focus on the currently focused frame if any.
-    if (!page_)
+    if (!page_ || !focused_frame)
       return;
 
-    LocalFrame* frame = page_->MainFrame() && page_->MainFrame()->IsLocalFrame()
-                            ? page_->DeprecatedLocalMainFrame()
-                            : nullptr;
-    if (!frame)
-      return;
+    // Finish an ongoing composition to delete the composition node.
+    if (focused_frame->GetInputMethodController().HasComposition()) {
+      // TODO(editing-dev): The use of
+      // updateStyleAndLayoutIgnorePendingStylesheets needs to be audited.
+      // See http://crbug.com/590369 for more details.
+      focused_frame->GetDocument()
+          ->UpdateStyleAndLayoutIgnorePendingStylesheets();
 
-    LocalFrame* focused_frame = FocusedLocalFrameInWidget();
-    if (focused_frame) {
-      // Finish an ongoing composition to delete the composition node.
-      if (focused_frame->GetInputMethodController().HasComposition()) {
-        // TODO(editing-dev): The use of
-        // updateStyleAndLayoutIgnorePendingStylesheets needs to be audited.
-        // See http://crbug.com/590369 for more details.
-        focused_frame->GetDocument()
-            ->UpdateStyleAndLayoutIgnorePendingStylesheets();
-
-        focused_frame->GetInputMethodController().FinishComposingText(
-            InputMethodController::kKeepSelection);
+      focused_frame->GetInputMethodController().FinishComposingText(
+          InputMethodController::kKeepSelection);
       }
-      ime_accept_events_ = false;
-    }
+      // TODO(ekaramad): We need to figure out the right way to propagate page
+      // focus to OOPIFs. Right now WebFrameWidgetImpl::SetFocus never gets
+      // called (https://crbug.com/689777).
+      WebLocalFrameBase::FromFrame(focused_frame->LocalFrameRoot())
+          ->FrameWidget()
+          ->ime_accept_events_ = false;
   }
 }
 
 // TODO(ekaramad):This method is almost duplicated in WebFrameWidgetImpl as
 // well. This code needs to be refactored  (http://crbug.com/629721).
 WebRange WebViewImpl::CompositionRange() {
-  LocalFrame* focused = FocusedLocalFrameAvailableForIme();
+  if (!MainFrameImpl() || !MainFrameImpl()->FrameWidget()->ime_accept_events_)
+    return WebRange();
+
+  LocalFrame* focused = FocusedLocalFrameInWidget();
   if (!focused)
     return WebRange();
 
@@ -4141,17 +4144,9 @@ float WebViewImpl::DeviceScaleFactor() const {
 }
 
 LocalFrame* WebViewImpl::FocusedLocalFrameInWidget() const {
-  if (!MainFrameImpl())
-    return nullptr;
-
-  LocalFrame* focused_frame = ToLocalFrame(FocusedCoreFrame());
-  if (focused_frame->LocalFrameRoot() != MainFrameImpl()->GetFrame())
-    return nullptr;
-  return focused_frame;
-}
-
-LocalFrame* WebViewImpl::FocusedLocalFrameAvailableForIme() const {
-  return ime_accept_events_ ? FocusedLocalFrameInWidget() : nullptr;
+  if (auto* frame = MainFrameImpl())
+    return frame->FrameWidget()->FocusedLocalFrame();
+  return nullptr;
 }
 
 }  // namespace blink