win: Add Scoped...Handle

win: Add Scoped...Handle

Intended for future use to implement util/file/file_writer.

There's a similar class in base:
https://code.google.com/p/chromium/codesearch#chromium/src/base/win/scoped_handle.h&l=28

However (perhaps for historical reasons) it does not distinguish between
the possible types of HANDLEs which have different invalid values,
resulting in a need to copy a bunch of code rather than simply using
ScopedGeneric.

Instead, distinguish between the types so the caller can use the
correct one.

Refs:
http://blogs.msdn.com/b/oldnewthing/archive/2004/03/02/82639.aspx
http://msdn.microsoft.com/en-us/magazine/cc302328.aspx (Figure 2)

R=mark@chromium.org
BUG=crashpad:1
Committed: https://chromium.googlesource.com/crashpad/crashpad/+/4a9b858fbd1fcddcddfff00c8537a48f07f7ae69

[scottmg, 2014-12-17 22:04:36]

scottmg@chromium.org changed reviewers:
+ mark@chromium.org

[scottmg, 2014-12-17 22:04:46]

Oops, and now with an R=.

[Mark Mentovai, 2014-12-17 22:45:00]

https://codereview.chromium.org/813873004/diff/20001/util/win/scoped_handle.h
File util/win/scoped_handle.h (right):

https://codereview.chromium.org/813873004/diff/20001/util/win/scoped_handle.h...
util/win/scoped_handle.h:15: #ifndef CRASHPAD_UTIL_WIN_SCOPED_HANDLE_H
Needs a trailing _.

https://codereview.chromium.org/813873004/diff/20001/util/win/scoped_handle.h...
util/win/scoped_handle.h:44: using ScopedFileHandle =
These names may not fully convey their Windows API-ness. Since they’re not
namespaced, if I saw them in some random code, I don’t know if I’d immediately
make the connection that a “scoped file handle” was a Windows HANDLE.

ScopedFileHANDLE? ScopedWinFileHandle?

[scottmg, 2014-12-17 22:57:35]

https://codereview.chromium.org/813873004/diff/20001/util/win/scoped_handle.h
File util/win/scoped_handle.h (right):

https://codereview.chromium.org/813873004/diff/20001/util/win/scoped_handle.h...
util/win/scoped_handle.h:15: #ifndef CRASHPAD_UTIL_WIN_SCOPED_HANDLE_H
On 2014/12/17 22:45:00, Mark Mentovai wrote:
> Needs a trailing _.

Done.

https://codereview.chromium.org/813873004/diff/20001/util/win/scoped_handle.h...
util/win/scoped_handle.h:44: using ScopedFileHandle =
On 2014/12/17 22:45:00, Mark Mentovai wrote:
> These names may not fully convey their Windows API-ness. Since they’re not
> namespaced, if I saw them in some random code, I don’t know if I’d immediately
> make the connection that a “scoped file handle” was a Windows HANDLE.
> 
> ScopedFileHANDLE? ScopedWinFileHandle?

Maybe win::ScopedFileHandle is better? (is closer to base/) Also happy to
s/Handle/HANDLE/, it just seemed a bit screamy. On the other hand, there is a
ScopedFILE in base, so I guess either has precedent.

[Mark Mentovai, 2014-12-18 14:29:21]

We’ve avoided subnamespaces like this in Crashpad.

Since style prohibits “using namespace”, namespaces are cumbersome when there’s
more than one sort of thing that might appear at a particular point in a
namespace hierarchy. The sort of thing we currently use the second level for now
is “test”. Since all test stuff goes into crashpad::test, a test, even for win
code, even for this specific code, would need to access it using win::
qualifiers. That doesn’t seem right, it doesn’t make the best example code, and
it doesn’t help as code migrates in and out of crashpad::test.

Worse, since Crashpad is necessarily made up of lots of platform-specific code,
this pattern could get a little out of control. I personally find that when I’m
working with base, I have a hard time remembering whether it’s
base::mac::ScopedMachSendRight or base::ScopedMachSendRight, and in Crashpad, I
work with that one every day. It’s the former, but we also have
base::scoped_nsobject and base::ScopedCFTypeRef, no mac subnamespace.

In my opinion, the weight of this makes it pretty unfavorable relative to just
picking a less-ambiguous name.

Normally, “handle” alone is sufficiently Windowsy to identify it without any
extra adornment, but when I see “FileHandle,” the first thing I think is a
little more generic. I think ScopedKernelHandle is an OK name, although
depending on what happens to ScopedFileHandle, it might be desirable for it to
change too for consistency.

[scottmg, 2014-12-18 17:19:10]

Patchset #5 (id:80001) has been deleted

[scottmg, 2014-12-18 17:20:01]

On 2014/12/18 14:29:21, Mark Mentovai wrote:
> We’ve avoided subnamespaces like this in Crashpad.
> 
> Since style prohibits “using namespace”, namespaces are cumbersome when
there’s
> more than one sort of thing that might appear at a particular point in a
> namespace hierarchy. The sort of thing we currently use the second level for
now
> is “test”. Since all test stuff goes into crashpad::test, a test, even for win
> code, even for this specific code, would need to access it using win::
> qualifiers. That doesn’t seem right, it doesn’t make the best example code,
and
> it doesn’t help as code migrates in and out of crashpad::test.
> 
> Worse, since Crashpad is necessarily made up of lots of platform-specific
code,
> this pattern could get a little out of control. I personally find that when
I’m
> working with base, I have a hard time remembering whether it’s
> base::mac::ScopedMachSendRight or base::ScopedMachSendRight, and in Crashpad,
I
> work with that one every day. It’s the former, but we also have
> base::scoped_nsobject and base::ScopedCFTypeRef, no mac subnamespace.
> 
> In my opinion, the weight of this makes it pretty unfavorable relative to just
> picking a less-ambiguous name.
> 
> Normally, “handle” alone is sufficiently Windowsy to identify it without any
> extra adornment, but when I see “FileHandle,” the first thing I think is a
> little more generic. I think ScopedKernelHandle is an OK name, although
> depending on what happens to ScopedFileHandle, it might be desirable for it to
> change too for consistency.

Thanks for the background and rationale, makes sense to me.

I went with ScopedFileHANDLE and also used HANDLE for the Kernel one for
consistency.

[scottmg, 2014-12-18 17:21:23]

Patchset #6 (id:120001) has been deleted

[Mark Mentovai, 2014-12-18 17:41:17]

LGTM

[scottmg, 2014-12-18 17:51:25]

Committed patchset #6 (id:140001) manually as
4a9b858fbd1fcddcddfff00c8537a48f07f7ae69 (presubmit successful).