More Windows-is-a-crack-baby mitigation.

I think I've finally fixed this. Knock on wood. Turns out that these two
additional places do non-client rendering:

- DefWindowProc for WM_NCLBUTTONDOWN
- the function EnableMenuItem (called from CustomFrameWindow::OnInitMenu)

For the first case, we also need to unlock updating when we begin to receive
mouse move events during modal size/move loops, otherwise the feedback won't be
visually continuous.

http://crbug.com/3264
Review URL: http://codereview.chromium.org/7662

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@3620 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 86 insertions, 31 deletions

diff --git a/chrome/browser/views/frame/opaque_frame.cc b/chrome/browser/views/frame/opaque_frame.cc
index e8740b9..ab0f746 100644
--- a/chrome/browser/views/frame/opaque_frame.cc
+++ b/chrome/browser/views/frame/opaque_frame.cc
@@ -109,7 +109,7 @@ void OpaqueFrame::OnSysCommand(UINT notification_code, CPoint click) {
   if (!browser_view_->SystemCommandReceived(notification_code,
                                             gfx::Point(click))) {
     // Use the default implementation for any other command.
-    SetMsgHandled(FALSE);
+    CustomFrameWindow::OnSysCommand(notification_code, click);
   }
 }
 
diff --git a/chrome/views/custom_frame_window.cc b/chrome/views/custom_frame_window.cc
index 0febe17..af94798 100644
--- a/chrome/views/custom_frame_window.cc
+++ b/chrome/views/custom_frame_window.cc
@@ -25,45 +25,52 @@
 
 namespace views {
 
-// A scoping class that removes the WS_VISIBLE style of a window.
+// When the user presses the mouse down in the non-client area of the window,
+// Windows sends WM_SYSCOMMAND with one of these notification codes. They
+// represent frame size/move operations initiated by the user's direct mouse
+// gesture-driven manipulation of the frame, as opposed to SC_SIZE/SC_MOVE
+// which are the result of system menu operations.
+static const int SC_FRAMESIZE = 0xF002;
+static const int SC_FRAMEMOVE = 0xF012;
+
+// A scoping class that prevents a window from being able to redraw in response
+// to invalidations that may occur within it for the lifetime of the object.
 //
 // Why would we want such a thing? Well, it turns out Windows has some
 // "unorthodox" behavior when it comes to painting its non-client areas.
-// Sadly, the default implementation of some messages, e.g. WM_SETTEXT and
-// WM_SETICON actually paint all or parts of the native title bar of the
-// application. That's right, they just paint it. They don't go through
-// WM_NCPAINT or anything like that that we already override. What this means
-// is that we end up with occasional flicker of bits of the normal Windows
-// title bar whenever we do things like change the title text, or right click
-// on the caption. The solution turns out to be to handle these messages,
-// use this scoped object to remove the WS_VISIBLE style which prevents this
-// rendering from happening, call the default window procedure, then add the
-// WS_VISIBLE style back when this object goes out of scope.
+// Occasionally, Windows will paint portions of the default non-client area
+// right over the top of the custom frame. This is not simply fixed by handling
+// WM_NCPAINT/WM_PAINT, with some investigation it turns out that this
+// rendering is being done *inside* the default implementation of some message
+// handlers and functions:
+//  . WM_SETTEXT
+//  . WM_SETICON
+//  . WM_NCLBUTTONDOWN
+//  . EnableMenuItem, called from our WM_INITMENU handler
+// The solution is to handle these messages and call DefWindowProc ourselves,
+// but prevent the window from being able to update itself for the duration of
+// the call. We do this with this class, which automatically calls its
+// associated CustomFrameWindow's lock and unlock functions as it is created
+// and destroyed. See documentation in those methods for the technique used.
+//
+// IMPORTANT: Do not use this scoping object for large scopes or periods of
+//            time! IT WILL PREVENT THE WINDOW FROM BEING REDRAWN! (duh).
+//
 // I would love to hear Raymond Chen's explanation for all this. And maybe a
 // list of other messages that this applies to ;-)
-//
-// *** Sigh. ***
-class ScopedVisibilityRemover {
+class CustomFrameWindow::ScopedRedrawLock {
  public:
-  explicit ScopedVisibilityRemover(HWND hwnd)
-      : hwnd_(hwnd),
-        window_style_(0) {
-    window_style_ = GetWindowLong(hwnd_, GWL_STYLE);
-    if (window_style_ & WS_VISIBLE)
-      SetWindowLong(hwnd_, GWL_STYLE, window_style_ & ~WS_VISIBLE);
+  explicit ScopedRedrawLock(CustomFrameWindow* window) : window_(window) {
+    window_->LockUpdates();
   }
 
-  ~ScopedVisibilityRemover() {
-    if (window_style_ & WS_VISIBLE)
-      SetWindowLong(hwnd_, GWL_STYLE, window_style_);
+  ~ScopedRedrawLock() {
+    window_->UnlockUpdates();
   }
 
  private:
   // The window having its style changed.
-  HWND hwnd_;
-
-  // The original style of the window, including WS_VISIBLE if present.
-  DWORD window_style_;
+  CustomFrameWindow* window_;
 };
 
 HCURSOR CustomFrameWindow::resize_cursors_[6];
@@ -878,7 +885,8 @@ class NonClientViewLayout : public LayoutManager {
 
 CustomFrameWindow::CustomFrameWindow(WindowDelegate* window_delegate)
     : Window(window_delegate),
-      is_active_(false) {
+      is_active_(false),
+      lock_updates_(false) {
   InitClass();
   non_client_view_ = new DefaultNonClientView(this);
 }
@@ -1016,6 +1024,7 @@ void CustomFrameWindow::OnInitMenu(HMENU menu) {
   bool maximized = IsMaximized();
   bool minimized_or_maximized = minimized || maximized;
 
+  ScopedRedrawLock lock(this);
   EnableMenuItem(menu, SC_RESTORE,
                  window_delegate()->CanMaximize() && minimized_or_maximized);
   EnableMenuItem(menu, SC_MOVE, !minimized_or_maximized);
@@ -1194,6 +1203,17 @@ void CustomFrameWindow::OnNCLButtonDown(UINT ht_component,
     }
     default:
       Window::OnNCLButtonDown(ht_component, point);
+      if (!IsMsgHandled()) {
+        // Window::OnNCLButtonDown set the message as unhandled. This normally
+        // means ContainerWin::ProcessWindowMessage will pass it to
+        // DefWindowProc. Sadly, DefWindowProc for WM_NCLBUTTONDOWN does weird
+        // non-client painting, so we need to call it directly here inside a
+        // scoped update lock.
+        ScopedRedrawLock lock(this);
+        DefWindowProc(GetHWND(), WM_NCLBUTTONDOWN, ht_component,
+                      MAKELPARAM(point.x, point.y));
+        SetMsgHandled(TRUE);
+      }
       break;
   }
 }
@@ -1260,13 +1280,13 @@ LRESULT CustomFrameWindow::OnSetCursor(HWND window, UINT hittest_code,
 }
 
 LRESULT CustomFrameWindow::OnSetIcon(UINT size_type, HICON new_icon) {
-  ScopedVisibilityRemover remover(GetHWND());
+  ScopedRedrawLock lock(this);
   return DefWindowProc(GetHWND(), WM_SETICON, size_type,
                        reinterpret_cast<LPARAM>(new_icon));
 }
 
 LRESULT CustomFrameWindow::OnSetText(const wchar_t* text) {
-  ScopedVisibilityRemover remover(GetHWND());
+  ScopedRedrawLock lock(this);
   return DefWindowProc(GetHWND(), WM_SETTEXT, NULL,
                        reinterpret_cast<LPARAM>(text));
 }
@@ -1279,6 +1299,18 @@ void CustomFrameWindow::OnSize(UINT param, const CSize& size) {
   ResetWindowRegion();
 }
 
+void CustomFrameWindow::OnSysCommand(UINT notification_code, CPoint click) {
+  if (notification_code == SC_FRAMEMOVE || notification_code == SC_FRAMESIZE) {
+    if (lock_updates_) {
+      // We were locked, before entering a resize or move modal loop. Now that
+      // we've begun to move the window, we need to unlock updates so that the
+      // sizing/moving feedback can be continuous.
+      UnlockUpdates();
+    }
+  }
+  Window::OnSysCommand(notification_code, click);
+}
+
 ///////////////////////////////////////////////////////////////////////////////
 // CustomFrameWindow, private:
 
@@ -1295,6 +1327,18 @@ void CustomFrameWindow::InitClass() {
   }
 }
 
+void CustomFrameWindow::LockUpdates() {
+  lock_updates_ = true;
+  // This message causes invalidations to be discarded until it is called again
+  // with WPARAM TRUE (see UnlockUpdates).
+  SendMessage(GetHWND(), WM_SETREDRAW, FALSE, 0);
+}
+
+void CustomFrameWindow::UnlockUpdates() {
+  SendMessage(GetHWND(), WM_SETREDRAW, TRUE, 0);
+  lock_updates_ = false;
+}
+
 void CustomFrameWindow::ResetWindowRegion() {
   // Changing the window region is going to force a paint. Only change the
   // window region if the region really differs.
diff --git a/chrome/views/custom_frame_window.h b/chrome/views/custom_frame_window.h
index ed03230..0c2b7f9 100644
--- a/chrome/views/custom_frame_window.h
+++ b/chrome/views/custom_frame_window.h
@@ -63,8 +63,16 @@ class CustomFrameWindow : public Window {
   virtual LRESULT OnSetIcon(UINT size_type, HICON new_icon);
   virtual LRESULT OnSetText(const wchar_t* text);
   virtual void OnSize(UINT param, const CSize& size);
+  virtual void OnSysCommand(UINT notification_code, CPoint click);
 
  private:
+  class ScopedRedrawLock;
+
+  // Lock or unlock the window from being able to redraw itself in response to
+  // updates to its invalid region.
+  void LockUpdates();
+  void UnlockUpdates();
+
   // Resets the window region.
   void ResetWindowRegion();
 
@@ -78,6 +86,9 @@ class CustomFrameWindow : public Window {
   // True if this window is the active top level window.
   bool is_active_;
 
+  // True if updates to this window are currently locked.
+  bool lock_updates_;
+
   // Static resource initialization.
   static void InitClass();
   enum ResizeCursor {