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,163 @@
 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. They help you avoid the following
+// 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:

[Derek Bruening, 2012/01/21 02:30:49]

nit: "Use"

[cpu_(ooo_6.6-7.5), 2012/01/24 04:48:04]

Done.

+//
+// ScopedGetDC dc(NULL);
+// dc.SelectObject(hdc, new_obj);

[Derek Bruening, 2012/01/21 02:30:49]

a little misleading: there is no "SelectObject".  you mean "Select<Object>" or
"Select{Bitmap,Font,...}" or just an example "SelectBitmap"

[cpu_(ooo_6.6-7.5), 2012/01/24 04:48:04]

Done.

+// .. drawing here

[Derek Bruening, 2012/01/21 02:30:49]

is there a nice way we can prevent someone from accidentally calling
SelectObject(dc)?  you have a cast operator to HDC so such a call will work, and
it will bypass the whole point of your diff.  static error would be best; if
only solution is runtime assert maybe still worth it.  even if you don't want to
overload SelectObject, perhaps you can have the destructor call GetCurrentObject
and assert if it's non-null and yet the stored orig obj is null

[cpu_(ooo_6.6-7.5), 2012/01/24 04:48:04]

Let me look into that, I believe the cast to HDC is used in our code.

On 2012/01/21 02:30:49, Derek Bruening wrote:

[Derek Bruening, 2012/01/24 18:49:43]

OK, waiting on that

+// .. when dc goes out of scope it will select the original object before
+// .. being released.
+//
+template <typename Derived>
+class ScopedDC {
+public:

[Derek Bruening, 2012/01/21 02:30:49]

indentation

[cpu_(ooo_6.6-7.5), 2012/01/24 04:48:04]

Done.

+  ~ScopedDC() {
+    Close();
   }
 
-  ~ScopedGetDC() {
-    if (hdc_)
-      ReleaseDC(hwnd_, hdc_);
+  HDC Get() {
+    return hdc_;
   }
 
-  operator HDC() { return hdc_; }
+  operator HDC() {
+    return hdc_;
+  }
 
+  void SelectBitmap(HBITMAP bitmap) {

[Derek Bruening, 2012/01/21 02:30:49]

do callers never need to know return value of SelectObject? you know GDI
programming more than me: there are no situations where failure might not be a
fatal error?

[cpu_(ooo_6.6-7.5), 2012/01/24 04:48:04]

Ideally the objects that are not the initial, will be managed by smart holders
so I am trying to discourage the use of naked gdi objects which is what this
will return.

Not the kind of errors this code is trying to solve. Maybe we can do bool or
dcheck or do nothing.

+    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_);
+  }
+
+  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);

[Derek Bruening, 2012/01/21 02:30:49]

DCHECK(holder)

[cpu_(ooo_6.6-7.5), 2012/01/24 04:48:04]

I rather not, a null pointer is guaranteed crash in the next line.

+    if (!*holder)

[Derek Bruening, 2012/01/21 02:30:49]

ok you're not returning the HGDIOBJ and are instead using an OUT param b/c you
want to only write to it on the very first Select. perhaps worth a comment that
you only want the original default obj and don't care about multiple Selects
after that.

[cpu_(ooo_6.6-7.5), 2012/01/24 04:48:04]

Done.

+      *holder = old;
+  }
+
   HDC hdc_;
+  HGDIOBJ bitmap_;
+  HGDIOBJ font_;
+  HGDIOBJ brush_;
+  HGDIOBJ pen_;
+  HGDIOBJ region_;

[Derek Bruening, 2012/01/21 02:30:49]

prior_bitmap_ or old_bitmap_ or orig_bitmap_ are more descriptive names for
these IMHO.

[cpu_(ooo_6.6-7.5), 2012/01/24 04:48:04]

ugly. do you really really want me to change them?


On 2012/01/21 02:30:49, Derek Bruening wrote:

+};
 
+// Creates and manages a HDC obtained by GetDC.
+class ScopedGetDC : public ScopedDC<ScopedGetDC>  {

[Derek Bruening, 2012/01/21 02:30:49]

yeah IMHO simpler code via virtual is the way to go unless you have evidence
that the virtual dispatch perf cost shows up and outweighs the more complex code

[cpu_(ooo_6.6-7.5), 2012/01/24 04:48:04]

Ok, by popular demand changed the code to use vtable. Liked better as it was
because the variant behavior was done at destruction and I can't do virtual
dispatch at the base dtor so now there has to be a Close() in the dtor of each
class.


On 2012/01/21 02:30:49, Derek Bruening wrote:

+ 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.
-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);
 };