Do not fallback to FocusedOrMainFrame() in FocusedLocalFrameInWidget()

third_party/WebKit/Source/web/WebViewImpl.cpp

 /*
  * Copyright (C) 2011, 2012 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 338 matching lines @@
       maximum_zoom_level_(ZoomFactorToZoomLevel(kMaxTextSizeMultiplier)),
       zoom_factor_for_device_scale_factor_(0.f),
       maximum_legible_scale_(1),
       double_tap_zoom_page_scale_factor_(0),
       double_tap_zoom_pending_(false),
       enable_fake_page_scale_animation_for_testing_(false),
       fake_page_scale_animation_page_scale_factor_(0),
       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),
       root_layer_(nullptr),
       root_graphics_layer_(nullptr),
       visual_viewport_container_layer_(nullptr),
       matches_heuristics_for_gpu_rasterization_(false),
       fling_modifier_(0),
       fling_source_device_(kWebGestureDeviceUninitialized),
       fullscreen_controller_(FullscreenController::Create(this)),
@@ skipping 1912 matching lines @@
     page_->SetIsCursorVisible(is_visible);
 }
 
 void WebViewImpl::MouseCaptureLost() {
   TRACE_EVENT_ASYNC_END0("input", "capturing mouse", this);
   mouse_capture_node_ = nullptr;
 }
 
 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()
                          .ComputeVisibleSelectionInDOMTreeDeprecated()
                          .IsNone()) {
         // If the selection was cleared while the WebView was not
         // focused, then the focus element shows with a focus ring but
         // no caret and does respond to keyboard inputs.
         focused_frame->GetDocument()->UpdateStyleAndLayoutTree();
         if (element->IsTextControl()) {
           element->UpdateFocusAppearance(SelectionBehaviorOnFocus::kRestore);
         } else if (HasEditableStyle(*element)) {
           // updateFocusAppearance() selects all the text of
           // contentseditable DIVs. So we set the selection explicitly
           // instead. Note that this has the side effect of moving the
           // caret back to the beginning of the text.
           Position position(element, 0);
           focused_frame->Selection().SetSelection(
               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();
 
   const EphemeralRange range =
       focused->GetInputMethodController().CompositionEphemeralRange();
   if (range.IsNull())
     return WebRange();
 
   Element* editable =
       focused->Selection().RootEditableElementOrDocumentElement();
@@ skipping 1771 matching lines @@
   // TODO(oshima): Investigate if this should return the ScreenInfo's scale
   // factor rather than page's scale factor, which can be 1 in use-zoom-for-dsf
   // mode.
   if (!GetPage())
     return 1;
 
   return GetPage()->DeviceScaleFactorDeprecated();
 }
 
 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