[Search] Implementation of the invisible focus on Windows

chrome/browser/ui/views/omnibox/omnibox_view_win.cc

Index: chrome/browser/ui/views/omnibox/omnibox_view_win.cc
diff --git a/chrome/browser/ui/views/omnibox/omnibox_view_win.cc b/chrome/browser/ui/views/omnibox/omnibox_view_win.cc
index cdfce5fc6b9f1a7eb3e29d7aae290b38491a33b1..a0050fa91d794d110e0305870b7f1167e1dac99b 100644
--- a/chrome/browser/ui/views/omnibox/omnibox_view_win.cc
+++ b/chrome/browser/ui/views/omnibox/omnibox_view_win.cc
@@ -760,11 +760,41 @@ void OmniboxViewWin::UpdatePopup() {
 
 void OmniboxViewWin::SetFocus() {
   ::SetFocus(m_hWnd);
-}
-
-void OmniboxViewWin::ApplyFocusVisibility() {
-  // TODO(mathp): implement for Windows.
-  NOTIMPLEMENTED();
+  // Restore caret visibility if focused explicitly. We need to do this here
+  // because the ::SetFocus() call will not reach
+  // OmniboxEditModel::OnSetFocus() in cases where we already had focus, such as
+  // when a new tab is created and the previous tab had invisible focus.

[Peter Kasting, 2012/12/05 04:10:57]

Given that this apparently does short-circuit, I think the comment in the Views
impl is actually somewhat clearer here, despite not having an example (we don't
need one).

[Mathieu, 2012/12/05 04:28:35]

Done.

+  model()->SetCaretVisibility(true);

[Peter Kasting, 2012/12/05 04:10:57]

The Views impl calls this before setting focus.  I don't care which order you
guys use, but be consistent.  It's much easier to maintain things later if each
platform is as close to the other platforms' implementations as possible in the
small details like this.

[Mathieu, 2012/12/05 04:28:35]

This is definitely the right order for Windows, since we can't act on a caret if
we don't have focus. ChromeOS doesn't differentiate. I will leave this part as
is.

[Peter Kasting, 2012/12/05 04:31:17]

K.  See if you can get the other CL to change to match, then.

I wonder if we should have a comment here noting what you just noted...

+}
+
+void OmniboxViewWin::ApplyCaretVisibility() {
+  // We hide the caret just before destroying it, since destroying a caret that
+  // is in the "solid" phase of its blinking will leave a solid vertical bar.
+  // We even hide and destroy the caret if we're going to create it again below.
+  // If the caret was already visible on entry to this function, the
+  // CreateCaret() call (which first destroys the old caret) might leave a solid
+  // vertical bar for the same reason as above.  Unconditionally hiding prevents
+  // this.  The caret could be visible on entry to this function if the
+  // underlying edit control had re-created it automatically (see comments in
+  // OnPaint()).
+  HideCaret();
+  // We use DestroyCaret()/CreateCaret() instead of simply HideCaret()/
+  // ShowCaret() because HideCaret() is not sticky across paint events, e.g. a
+  // window resize will effectively restore caret visibility, regardless of
+  // whether HideCaret() was called before. While we do catch and handle these
+  // paint events (see OnPaint()), it doesn't seem to be enough to simply call
+  // HideCaret() while handling them because of the unpredictability of this
+  // Windows API. According to the documentation, it should be a cumulative call
+  // e.g. 5 hide calls should be balanced by 5 show calls. We have not found
+  // this to be true, which may be explained by the fact that this API is called
+  // internally in Windows, as well.
+  ::DestroyCaret();
+  if (model()->is_focus_visible()) {
+    ::CreateCaret(m_hWnd, (HBITMAP) NULL, 1, font_.GetHeight());
+    // According to the Windows API documentation, a newly created caret needs
+    // ShowCaret to be visible.
+    ShowCaret();
+  }
 }
 
 void OmniboxViewWin::SetDropHighlightPosition(int position) {
@@ -1741,25 +1771,32 @@ LRESULT OmniboxViewWin::OnMouseActivate(HWND window,
   // reached before OnXButtonDown(), preventing us from detecting this properly
   // there.  Also in those cases, we need to already know in OnSetFocus() that
   // we should not restore the saved selection.
-  if (!model()->has_focus() &&
-      ((mouse_message == WM_LBUTTONDOWN || mouse_message == WM_RBUTTONDOWN)) &&
+  if (((mouse_message == WM_LBUTTONDOWN || mouse_message == WM_RBUTTONDOWN)) &&
       (result == MA_ACTIVATE)) {
-    if (gaining_focus_) {
-      // On Windows 8 in metro mode, we get two WM_MOUSEACTIVATE messages when
-      // we click on the omnibox with the mouse.
-      DCHECK(base::win::IsMetroProcess());
-      return result;
+    if (model()->has_focus()) {

[Peter Kasting, 2012/12/05 04:10:57]

I don't understand why you did things this way instead of the way I suggested. 
I was very intentional about wanting you to trigger the gaining_focus_
manipulation because that in turn is what triggers the click-to-select-all etc.
behavior.  I don't see advantage in going this route, it's just monotonically
less correct.  Even if you don't have the ability to test the with-text case
it's much better to write code that's likely correct instead of code that's
certainly not correct.  But frankly, no, I don't want you to worry about those
cases later, I'd like things to be correct from the start.

I also don't understand your "the proposed if clause would not have done the
trick" comment.  The proposed if clause explicitly checked for invisible focus. 
You need to explain in more detail if something about it wasn't right.

[Mathieu, 2012/12/05 04:28:35]

Discussed offline. I had misunderstood the original proposal.

+      if (!model()->is_caret_visible()) {
+        // Explicitely setting visibility of the focus as a result of directly
+        // clicking on the omnibox while it has invisible focus.
+        SetCaretVisibility(true);
+      }
+    } else {
+      if (gaining_focus_) {
+        // On Windows 8 in metro mode, we get two WM_MOUSEACTIVATE messages when
+        // we click on the omnibox with the mouse.
+        DCHECK(base::win::IsMetroProcess());
+        return result;
+      }
+      gaining_focus_.reset(new ScopedFreeze(this, GetTextObjectModel()));
+      // NOTE: Despite |mouse_message| being WM_XBUTTONDOWN here, we're not
+      // guaranteed to call OnXButtonDown() later!  Specifically, if this is the
+      // second click of a double click, we'll reach here but later call
+      // OnXButtonDblClk().  Make sure |gaining_focus_| gets reset both places,
+      // or we'll have visual glitchiness and then DCHECK failures.
+
+      // Don't restore saved selection, it will just screw up our interaction
+      // with this edit.
+      saved_selection_for_focus_change_.cpMin = -1;
     }
-    gaining_focus_.reset(new ScopedFreeze(this, GetTextObjectModel()));
-    // NOTE: Despite |mouse_message| being WM_XBUTTONDOWN here, we're not
-    // guaranteed to call OnXButtonDown() later!  Specifically, if this is the
-    // second click of a double click, we'll reach here but later call
-    // OnXButtonDblClk().  Make sure |gaining_focus_| gets reset both places,
-    // or we'll have visual glitchiness and then DCHECK failures.
-
-    // Don't restore saved selection, it will just screw up our interaction
-    // with this edit.
-    saved_selection_for_focus_change_.cpMin = -1;
   }
   return result;
 }
@@ -1899,6 +1936,12 @@ void OmniboxViewWin::OnPaint(HDC bogus_hdc) {
          rect.left, rect.top, SRCCOPY);
   memory_dc.SelectBitmap(old_bitmap);
   edit_hwnd = old_edit_hwnd;
+
+  // This needs to be called regardless of the current state of the caret, even
+  // if reaffirming a current state (hidden or shown). This is because the
+  // underlying edit control will automatically re-create the caret when it
+  // receives certain events that trigger repaints, e.g. window resize events.
+  ApplyCaretVisibility();
 }
 
 void OmniboxViewWin::OnPaste() {