Make scoped dc objects smarter.

Make scoped dc objects smarter

So we don't destroy the dc with gdi objects selected.
Also remove implicit conversion to HDC.

BUG=110113, 113683
TEST=chrome runs, base unittests green.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=121840

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

http://codereview.chromium.org/9212020/diff/13001/base/win/scoped_hdc.h
File base/win/scoped_hdc.h (right):

http://codereview.chromium.org/9212020/diff/13001/base/win/scoped_hdc.h#newco...
base/win/scoped_hdc.h:18: // that are initially selected into a DC. It helps you
avoid the following
Nit: It helps -> They help

http://codereview.chromium.org/9212020/diff/13001/base/win/scoped_hdc.h#newco...
base/win/scoped_hdc.h:39: // .. when dc goes out of scope it will select the
original object.
Nit: "...before being released."

http://codereview.chromium.org/9212020/diff/13001/base/win/scoped_hdc.h#newco...
base/win/scoped_hdc.h:85: static_cast<Derived*>(this)->DisposeDC(hdc_);
Why use templates and static casts to do this instead of simply making
DisposeDC() a pure virtual function and subclassing the class?

http://codereview.chromium.org/9212020/diff/13001/base/win/scoped_hdc.h#newco...
base/win/scoped_hdc.h:151: // CreateCompatibleDC, CreateDC and CreateIC.
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.)

http://codereview.chromium.org/9212020/diff/13001/base/win/scoped_hdc_unittes...
File base/win/scoped_hdc_unittest.cc (right):

http://codereview.chromium.org/9212020/diff/13001/base/win/scoped_hdc_unittes...
base/win/scoped_hdc_unittest.cc:34: return NULL != GetCurrentObject(hdc,
OBJ_BITMAP);
Nit: Use "!= NULL", not "NULL !=".

I thought there was some way to just ask if an HDC was valid...

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

http://codereview.chromium.org/9212020/diff/13001/base/win/scoped_hdc.h
File base/win/scoped_hdc.h (right):

http://codereview.chromium.org/9212020/diff/13001/base/win/scoped_hdc.h#newco...
base/win/scoped_hdc.h:85: static_cast<Derived*>(this)->DisposeDC(hdc_);
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:
> Why use templates and static casts to do this instead of simply making
> DisposeDC() a pure virtual function and subclassing the class?

http://codereview.chromium.org/9212020/diff/13001/base/win/scoped_hdc.h#newco...
base/win/scoped_hdc.h:151: // CreateCompatibleDC, CreateDC and CreateIC.
On 2012/01/20 21:48:12, Peter Kasting wrote:
> 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


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.


> 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.)

I am not parsing this second part, GetDC is in line 136.

[cpu_(ooo_6.6-7.5), 2012-01-20 23:53:55]

changes made on the other nits. PTAL.

http://codereview.chromium.org/9212020/diff/13001/base/win/scoped_hdc_unittes...
File base/win/scoped_hdc_unittest.cc (right):

http://codereview.chromium.org/9212020/diff/13001/base/win/scoped_hdc_unittes...
base/win/scoped_hdc_unittest.cc:34: return NULL != GetCurrentObject(hdc,
OBJ_BITMAP);
On 2012/01/20 21:48:12, Peter Kasting wrote:
> Nit: Use "!= NULL", not "NULL !=".
> 
> I thought there was some way to just ask if an HDC was valid...

Let me review the apis in case I missed something.

[Peter Kasting, 2012-01-21 01:00:25]

We argued some about this at TGIF and I still really would prefer the virtual
function route to the template + static_cast route.

[Peter Kasting, 2012-01-21 01:01:51]

On 2012/01/20 23:50:59, cpu wrote:
> 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 definitely don't think you should be forced to own anything in the future
about printing.  I support you telling other people you can't fix crashes in
this and that I OKed that :)

I still think it would be nice to eliminate these, though doing it in a separate
CL is fine.  If even that makes you uncomfortable, can we at least file a bug?

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

http://codereview.chromium.org/9212020/diff/17003/base/win/scoped_hdc.h
File base/win/scoped_hdc.h (right):

http://codereview.chromium.org/9212020/diff/17003/base/win/scoped_hdc.h#newco...
base/win/scoped_hdc.h:34: // But why work so hard? use our handy classes:
nit: "Use"

http://codereview.chromium.org/9212020/diff/17003/base/win/scoped_hdc.h#newco...
base/win/scoped_hdc.h:37: // dc.SelectObject(hdc, new_obj);
a little misleading: there is no "SelectObject".  you mean "Select<Object>" or
"Select{Bitmap,Font,...}" or just an example "SelectBitmap"

http://codereview.chromium.org/9212020/diff/17003/base/win/scoped_hdc.h#newco...
base/win/scoped_hdc.h:38: // .. drawing here
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

http://codereview.chromium.org/9212020/diff/17003/base/win/scoped_hdc.h#newco...
base/win/scoped_hdc.h:44: public:
indentation

http://codereview.chromium.org/9212020/diff/17003/base/win/scoped_hdc.h#newco...
base/win/scoped_hdc.h:57: void SelectBitmap(HBITMAP bitmap) {
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?

http://codereview.chromium.org/9212020/diff/17003/base/win/scoped_hdc.h#newco...
base/win/scoped_hdc.h:120: DCHECK(old);
DCHECK(holder)

http://codereview.chromium.org/9212020/diff/17003/base/win/scoped_hdc.h#newco...
base/win/scoped_hdc.h:121: if (!*holder)
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.

http://codereview.chromium.org/9212020/diff/17003/base/win/scoped_hdc.h#newco...
base/win/scoped_hdc.h:130: HGDIOBJ region_;
prior_bitmap_ or old_bitmap_ or orig_bitmap_ are more descriptive names for
these IMHO.

http://codereview.chromium.org/9212020/diff/17003/base/win/scoped_hdc.h#newco...
base/win/scoped_hdc.h:134: class ScopedGetDC : public ScopedDC<ScopedGetDC>  {
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]

Code updated. I am looking at the code to see what bug I should file as per
Peter comments, I'll handle that with him offine.

http://codereview.chromium.org/9212020/diff/17003/base/win/scoped_hdc.h
File base/win/scoped_hdc.h (right):

http://codereview.chromium.org/9212020/diff/17003/base/win/scoped_hdc.h#newco...
base/win/scoped_hdc.h:34: // But why work so hard? use our handy classes:
On 2012/01/21 02:30:49, Derek Bruening wrote:
> nit: "Use"

Done.

http://codereview.chromium.org/9212020/diff/17003/base/win/scoped_hdc.h#newco...
base/win/scoped_hdc.h:37: // dc.SelectObject(hdc, new_obj);
On 2012/01/21 02:30:49, Derek Bruening wrote:
> a little misleading: there is no "SelectObject".  you mean "Select<Object>" or
> "Select{Bitmap,Font,...}" or just an example "SelectBitmap"

Done.

http://codereview.chromium.org/9212020/diff/17003/base/win/scoped_hdc.h#newco...
base/win/scoped_hdc.h:38: // .. drawing here
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:
> 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

http://codereview.chromium.org/9212020/diff/17003/base/win/scoped_hdc.h#newco...
base/win/scoped_hdc.h:44: public:
On 2012/01/21 02:30:49, Derek Bruening wrote:
> indentation

Done.

http://codereview.chromium.org/9212020/diff/17003/base/win/scoped_hdc.h#newco...
base/win/scoped_hdc.h:57: void SelectBitmap(HBITMAP bitmap) {
On 2012/01/21 02:30:49, Derek Bruening wrote:
> do callers never need to know return value of SelectObject? you know GDI

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.

> programming more than me: there are no situations where failure might not be a
> fatal error?

http://codereview.chromium.org/9212020/diff/17003/base/win/scoped_hdc.h#newco...
base/win/scoped_hdc.h:120: DCHECK(old);
On 2012/01/21 02:30:49, Derek Bruening wrote:
> DCHECK(holder)

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

http://codereview.chromium.org/9212020/diff/17003/base/win/scoped_hdc.h#newco...
base/win/scoped_hdc.h:121: if (!*holder)
On 2012/01/21 02:30:49, Derek Bruening wrote:
> 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.

Done.

http://codereview.chromium.org/9212020/diff/17003/base/win/scoped_hdc.h#newco...
base/win/scoped_hdc.h:130: HGDIOBJ region_;
ugly. do you really really want me to change them?


On 2012/01/21 02:30:49, Derek Bruening wrote:
> prior_bitmap_ or old_bitmap_ or orig_bitmap_ are more descriptive names for
> these IMHO.

http://codereview.chromium.org/9212020/diff/17003/base/win/scoped_hdc.h#newco...
base/win/scoped_hdc.h:134: class ScopedGetDC : public ScopedDC<ScopedGetDC>  {
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:
> 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

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

http://codereview.chromium.org/9212020/diff/17003/base/win/scoped_hdc.h
File base/win/scoped_hdc.h (right):

http://codereview.chromium.org/9212020/diff/17003/base/win/scoped_hdc.h#newco...
base/win/scoped_hdc.h:38: // .. drawing here
On 2012/01/24 04:48:04, cpu wrote:
> Let me look into that, I believe the cast to HDC is used in our code.

OK, waiting on that

[Peter Kasting, 2012-01-24 21:03:25]

I like the idea of killing operator HDC() if possible.  If we must we could
replace it with an explicit .get().

LGTM otherwise.  I agree with you that the explicit Close() call in each
destructor sucks.  I didn't think through that one.  I still think this is
arguably better than the template version, though.

http://codereview.chromium.org/9212020/diff/23002/base/win/scoped_hdc.h
File base/win/scoped_hdc.h (right):

http://codereview.chromium.org/9212020/diff/23002/base/win/scoped_hdc.h#newco...
base/win/scoped_hdc.h:89: void Reset(HDC hdc) {
Nit: We can eliminate this and inline it into the lone caller.

(That will make it less likely that we preserve this code once we kill said
caller.)

[cpu_(ooo_6.6-7.5), 2012-02-08 00:03:20]

Removed evil implicit conversion to HDC. Please look again.

The try bots are taking forever so I would like an lgtm even if I missed a call
place. I'll fix it before commit.

http://codereview.chromium.org/9212020/diff/27001/chrome/browser/ui/window_sn...
File chrome/browser/ui/window_snapshot/window_snapshot_win.cc (left):

http://codereview.chromium.org/9212020/diff/27001/chrome/browser/ui/window_sn...
chrome/browser/ui/window_snapshot/window_snapshot_win.cc:62: PrintWindowPointer
print_window =
Removing win2k hack.

[Peter Kasting, 2012-02-08 00:38:48]

Can you make sure there's a bug somewhere on killing the Set() accessor so that
both these are pure-RAII?

http://codereview.chromium.org/9212020/diff/27001/base/win/scoped_hdc.h
File base/win/scoped_hdc.h (right):

http://codereview.chromium.org/9212020/diff/27001/base/win/scoped_hdc.h#newco...
base/win/scoped_hdc.h:49: HDC Get() {
Nit: Seems like this should be "get()" since it's inline and cheap.

http://codereview.chromium.org/9212020/diff/27001/base/win/scoped_hdc.h#newco...
base/win/scoped_hdc.h:53: void SelectBitmap(HBITMAP bitmap) {
Nit: Can we move these and the other non-unix_hacker()-style function bodies to
a .cc file rather than having them inlined in the header?

http://codereview.chromium.org/9212020/diff/27001/base/win/scoped_hdc.h#newco...
base/win/scoped_hdc.h:75: : hdc_(hdc), bitmap_(0), font_(0), brush_(0), pen_(0),
region_(0) {
Nit: Indent 4 (4 places); 1 member per line since the entire definition isn't
one line

http://codereview.chromium.org/9212020/diff/27001/chrome/browser/ui/window_sn...
File chrome/browser/ui/window_snapshot/window_snapshot_win.cc (right):

http://codereview.chromium.org/9212020/diff/27001/chrome/browser/ui/window_sn...
chrome/browser/ui/window_snapshot/window_snapshot_win.cc:60: if
(snapshot_bounds.origin() == gfx::Point()) {
Nit: If you reverse this conditional you can combine these nested ifs:

  if (origin != Point()) {
    BitBlt(...);
  } else if (!PrintWindow(...)) {
    NOTREACHED();
  }

http://codereview.chromium.org/9212020/diff/27001/chrome/service/cloud_print/...
File chrome/service/cloud_print/print_system_win.cc (right):

http://codereview.chromium.org/9212020/diff/27001/chrome/service/cloud_print/...
chrome/service/cloud_print/print_system_win.cc:410: NOTREACHED();
Is it possible for the CreateDC() call to fail legitimately (i.e. in a way we
should handle)?

If so, we should eliminate the NOTREACHED() here.

If not, we should eliminate the entire conditional check and just inline the
CreateDC() call into the Set() call.

http://codereview.chromium.org/9212020/diff/27001/ui/gfx/native_theme_win.cc
File ui/gfx/native_theme_win.cc (right):

http://codereview.chromium.org/9212020/diff/27001/ui/gfx/native_theme_win.cc#...
ui/gfx/native_theme_win.cc:1703: bitmap_dc.Get(), 0, 0, SRCCOPY);
Nit: Arbitrary-seeming choice of wrapping points

[Derek Bruening, 2012-02-08 15:13:19]

LGTM

http://codereview.chromium.org/9212020/diff/27001/base/win/scoped_hdc.h
File base/win/scoped_hdc.h (right):

http://codereview.chromium.org/9212020/diff/27001/base/win/scoped_hdc.h#newco...
base/win/scoped_hdc.h:130: // Creates and manages a HDC obtained by GetDC.
nit: "an HDC" (unless you pronounce it "huhd-kuh" or something instead of saying
the letters?)

[cpu_(ooo_6.6-7.5), 2012-02-10 19:41:55]

I think I got all the comments done. PTAL See CL description for the new bug.

http://codereview.chromium.org/9212020/diff/27001/base/win/scoped_hdc.h
File base/win/scoped_hdc.h (right):

http://codereview.chromium.org/9212020/diff/27001/base/win/scoped_hdc.h#newco...
base/win/scoped_hdc.h:49: HDC Get() {
On 2012/02/08 00:38:49, Peter Kasting wrote:
> Nit: Seems like this should be "get()" since it's inline and cheap.

Done.

http://codereview.chromium.org/9212020/diff/27001/base/win/scoped_hdc.h#newco...
base/win/scoped_hdc.h:75: : hdc_(hdc), bitmap_(0), font_(0), brush_(0), pen_(0),
region_(0) {
On 2012/02/08 00:38:49, Peter Kasting wrote:
> Nit: Indent 4 (4 places); 1 member per line since the entire definition isn't
> one line

Done.

http://codereview.chromium.org/9212020/diff/27001/base/win/scoped_hdc.h#newco...
base/win/scoped_hdc.h:130: // Creates and manages a HDC obtained by GetDC.
On 2012/02/08 15:13:19, Derek Bruening wrote:
> nit: "an HDC" (unless you pronounce it "huhd-kuh" or something instead of
saying
> the letters?)

Done.

http://codereview.chromium.org/9212020/diff/27001/chrome/browser/ui/window_sn...
File chrome/browser/ui/window_snapshot/window_snapshot_win.cc (right):

http://codereview.chromium.org/9212020/diff/27001/chrome/browser/ui/window_sn...
chrome/browser/ui/window_snapshot/window_snapshot_win.cc:60: if
(snapshot_bounds.origin() == gfx::Point()) {
On 2012/02/08 00:38:49, Peter Kasting wrote:
> Nit: If you reverse this conditional you can combine these nested ifs:
> 
>   if (origin != Point()) {
>     BitBlt(...);
>   } else if (!PrintWindow(...)) {
>     NOTREACHED();
>   }

Done.

http://codereview.chromium.org/9212020/diff/27001/chrome/service/cloud_print/...
File chrome/service/cloud_print/print_system_win.cc (right):

http://codereview.chromium.org/9212020/diff/27001/chrome/service/cloud_print/...
chrome/service/cloud_print/print_system_win.cc:410: NOTREACHED();
On 2012/02/08 00:38:49, Peter Kasting wrote:
> Is it possible for the CreateDC() call to fail legitimately (i.e. in a way we
> should handle)?

Not an expert on printing but I can imagine that if the printer is on the
network this can fail.


> 
> If so, we should eliminate the NOTREACHED() here.
> 
> If not, we should eliminate the entire conditional check and just inline the
> CreateDC() call into the Set() call.

http://codereview.chromium.org/9212020/diff/27001/ui/gfx/native_theme_win.cc
File ui/gfx/native_theme_win.cc (right):

http://codereview.chromium.org/9212020/diff/27001/ui/gfx/native_theme_win.cc#...
ui/gfx/native_theme_win.cc:1703: bitmap_dc.Get(), 0, 0, SRCCOPY);
On 2012/02/08 00:38:49, Peter Kasting wrote:
> Nit: Arbitrary-seeming choice of wrapping points

Done.

[Peter Kasting, 2012-02-10 19:47:38]

LGTM

http://codereview.chromium.org/9212020/diff/37001/base/base.gypi
File base/base.gypi (right):

http://codereview.chromium.org/9212020/diff/37001/base/base.gypi#newcode407
base/base.gypi:407: 'win/scoped_hdc.cc',
Nit: tabs vs. spaces or something went wrong here

http://codereview.chromium.org/9212020/diff/37001/base/win/scoped_hdc.h
File base/win/scoped_hdc.h (right):

http://codereview.chromium.org/9212020/diff/37001/base/win/scoped_hdc.h#newco...
base/win/scoped_hdc.h:48: HDC get() {
Nit: Whole def'n can all be on one line

http://codereview.chromium.org/9212020/diff/37001/base/win/scoped_hdc.h#newco...
base/win/scoped_hdc.h:53: 
Nit: I'd remove the newlines between these

http://codereview.chromium.org/9212020/diff/37001/base/win/scoped_hdc.h#newco...
base/win/scoped_hdc.h:80: };
Nit: DISALLOW_COPY_AND_ASSIGN?

http://codereview.chromium.org/9212020/diff/37001/base/win/scoped_hdc.h#newco...
base/win/scoped_hdc.h:88: virtual ~ScopedGetDC() {
Nit: Never inline virtual functions or other functions that aren't unix_hacker()
accessors (several places)

[Derek Bruening, 2012-02-10 20:09:46]

LGTM

[brettw, 2012-02-13 04:24:28]

LGTM

http://codereview.chromium.org/9212020/diff/39016/base/win/scoped_hdc_unittes...
File base/win/scoped_hdc_unittest.cc (right):

http://codereview.chromium.org/9212020/diff/39016/base/win/scoped_hdc_unittes...
base/win/scoped_hdc_unittest.cc:12: namespace {
Move the extra blank line above this to below it.

http://codereview.chromium.org/9212020/diff/39016/base/win/scoped_hdc_unittes...
base/win/scoped_hdc_unittest.cc:39: }  // namespace.
No period

[commit-bot: I haz the power, 2012-02-14 01:03:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cpu@chromium.org/9212020/45001

[commit-bot: I haz the power, 2012-02-14 01:03:45]

Presubmit check for 9212020-45001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
ui/gfx/native_theme_win.cc,remoting/host/disconnect_window_win.cc,printing/emf_win_unittest.cc

Presubmit checks took 3.1s to calculate.