Make scoped dc objects smarter.

base/win/scoped_hdc.h

Index: base/win/scoped_hdc.h
===================================================================
--- base/win/scoped_hdc.h	(revision 118400)
+++ base/win/scoped_hdc.h	(working copy)
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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.
 
@@ -14,65 +14,162 @@
 namespace base {
 namespace win {
 
-// Like ScopedHandle but for HDC.  Only use this on HDCs returned from
-// GetDC.
-class ScopedGetDC {
- public:
-  explicit ScopedGetDC(HWND hwnd)
-      : hwnd_(hwnd),
-        hdc_(GetDC(hwnd)) {
-    DCHECK(!hwnd_ || IsWindow(hwnd_));
-    DCHECK(hdc_);
+// The ScopedGetDC and ScopedCreateDC classes manage the default GDI objects
+// that are initially selected into a DC. It helps you avoid the following

[Peter Kasting, 2012/01/20 21:48:12]

Nit: It helps -> They help

+// common mistake:
+//
+// HDC hdc = GetDC(NULL);
+// SelectObject(hdc, new_obj);
+// .. drawing code here ..
+// ReleaseDC(hdc);   <--- error: the DC has a custom object still selected!
+//
+// This code should be:
+//
+// HDC hdc = GetDC(NULL);
+// HGDIOBJ old_obj = SelectObject(hdc, new_obj);
+// .. drawing code here ..
+// SelectObject(hdc, old_obj);
+// ReleaseDC(hdc);    <--- ok to release now.
+//
+// But why work so hard? use our handy classes:
+//
+// ScopedGetDC dc(NULL);
+// dc.SelectObject(hdc, new_obj);
+// .. drawing here
+// .. when dc goes out of scope it will select the original object.

[Peter Kasting, 2012/01/20 21:48:12]

Nit: "...before being released."

+//
+template <typename Derived>
+class ScopedDC {
+public:
+  ~ScopedDC() {
+    Close();
   }
 
-  ~ScopedGetDC() {
-    if (hdc_)
-      ReleaseDC(hwnd_, hdc_);
+  HDC Get() {
+    return hdc_;
   }
 
-  operator HDC() { return hdc_; }
+  operator HDC() {
+    return hdc_;
+  }
 
+  void SelectBitmap(HBITMAP bitmap) {
+    Select(bitmap, &bitmap_);
+  }
+
+  void SelectFont(HFONT font) {
+    Select(font, &font_);
+  }
+
+  void SelectBrush(HBRUSH brush) {
+    Select(brush, &brush_);
+  }
+
+  void SelectPen(HPEN pen) {
+    Select(pen, &pen_);
+  }
+
+  void SelectRegion(HRGN region) {
+    Select(region, &region);
+  }
+
+ protected:
+  ScopedDC(HDC hdc)
+    : hdc_(hdc), bitmap_(0), font_(0), brush_(0), pen_(0), region_(0) {
+  }
+
+  void Close() {
+    if (!hdc_)
+      return;
+    ResetObjects();
+    static_cast<Derived*>(this)->DisposeDC(hdc_);

[Peter Kasting, 2012/01/20 21:48:12]

Why use templates and static casts to do this instead of simply making
DisposeDC() a pure virtual function and subclassing the class?

[cpu_(ooo_6.6-7.5), 2012/01/20 23:50:59]

Because the client code does not treat them polimorfically, as in a reference
(or pointer) to the base class, so in the grand tradition of only pay for what
you use the code imposes no penalty.

We can also do this without templates or vtables but then testing sucks, other
than that I can only say is a standard technique
http://en.wikipedia.org/wiki/Template_metaprogramming#Static_polymorphism


On 2012/01/20 21:48:12, Peter Kasting wrote:

+  }
+
+  void Reset(HDC hdc) {
+    Close();
+    hdc_ = hdc;
+  }
+
  private:
-  HWND hwnd_;
+  void ResetObjects() {
+    if (bitmap_) {
+      SelectObject(hdc_, bitmap_);
+      bitmap_ = 0;
+    }
+    if (font_) {
+      SelectObject(hdc_, font_);
+      font_ = 0;
+    }
+    if (brush_) {
+      SelectObject(hdc_, brush_);
+      brush_ = 0;
+    }
+    if (pen_) {
+      SelectObject(hdc_, pen_);
+      pen_ = 0;
+    }
+    if (region_) {
+      SelectObject(hdc_, region_);
+      region_ = 0;
+    }
+  }
+
+  void Select(HGDIOBJ object, HGDIOBJ* holder) {
+    HGDIOBJ old = SelectObject(hdc_, object);
+    DCHECK(old);
+    if (!*holder)
+      *holder = old;
+  }
+
   HDC hdc_;
+  HGDIOBJ bitmap_;
+  HGDIOBJ font_;
+  HGDIOBJ brush_;
+  HGDIOBJ pen_;
+  HGDIOBJ region_;
+};
 
+// Creates and manages a HDC obtained by GetDC.
+class ScopedGetDC : public ScopedDC<ScopedGetDC>  {
+ public:
+  explicit ScopedGetDC(HWND hwnd)
+    : ScopedDC(GetDC(hwnd)), hwnd_(hwnd) {
+  }
+
+ private:
+  friend class ScopedDC<ScopedGetDC>;
+
+  void DisposeDC(HDC hdc) {
+    ReleaseDC(hwnd_, hdc);
+  }
+
+  HWND hwnd_;
   DISALLOW_COPY_AND_ASSIGN(ScopedGetDC);
 };
 
 // Like ScopedHandle but for HDC.  Only use this on HDCs returned from
 // CreateCompatibleDC, CreateDC and CreateIC.

[Peter Kasting, 2012/01/20 21:48:12]

The comments here and on ScopedGetDC are structured differently, which is just a
nit, but the constructors are also structured differently.  Can we at least
eliminate the NULL constructor (and Set()) on this class?  (More changes, like
removing the GetDC() call from ScopedGetDC or trying to make this class'
constructors call CreateDC() and similar, seem like bad ideas.)

[cpu_(ooo_6.6-7.5), 2012/01/20 23:50:59]

The null ctor and the set are used in our code, specifically in our printing
code which is, hmmm, how to put it, hard to read. What I have now is a drop in
replacement.

For a few minutes I did consider touching the printing code because what you ask
is how my code was, but the prospects of owning future crashes stacks on that
code is distressing.
I am not parsing this second part, GetDC is in line 136. 

-class ScopedCreateDC {
+class ScopedCreateDC : public ScopedDC<ScopedCreateDC>  {
  public:
-  ScopedCreateDC() : hdc_(NULL) { }
-  explicit ScopedCreateDC(HDC h) : hdc_(h) { }
-
-  ~ScopedCreateDC() {
-    Close();
+  ScopedCreateDC()
+    : ScopedDC(0) {
   }
 
-  HDC Get() {
-    return hdc_;
+  explicit ScopedCreateDC(HDC hdc)
+    : ScopedDC(hdc) {
   }
 
-  void Set(HDC h) {
-    Close();
-    hdc_ = h;
+  void Set(HDC hdc) {
+    Reset(hdc);
   }
 
-  operator HDC() { return hdc_; }
+ private:
+  friend class ScopedDC<ScopedCreateDC>;
 
- private:
-  void Close() {
-#ifdef NOGDI
-    assert(false);
-#else
-    if (hdc_)
-      DeleteDC(hdc_);
-#endif  // NOGDI
+  void DisposeDC(HDC hdc) {
+    DeleteDC(hdc);
   }
 
-  HDC hdc_;
-
   DISALLOW_COPY_AND_ASSIGN(ScopedCreateDC);
 };