Fix some issues found looking at the code....

Fix some issues found looking at the code.

Patch from Gaetano Mendola <mendola@gmail.com>;
Original review: http://codereview.chromium.org/4273

I added some additions on my part and two unit test fix due to the added DCHECK. Reduced atl header inclusion.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=2730

[M-A Ruel, 2008-09-26 18:14:54]

Original review at http://codereview.chromium.org/4273

[Dean McNamee, 2008-09-27 10:18:49]

The hmac code was correct before right?  scoped_ptr is just cleaner, but there
wasn't a bug?

Anyway, LGTM.

http://codereview.chromium.org/5009/diff/1/5
File base/base_drag_source.cc (left):

http://codereview.chromium.org/5009/diff/1/5#oldcode58
Line 58: ULONG copied_refcnt = ref_count_;
That was strange?

http://codereview.chromium.org/5009/diff/1/6
File base/hmac_nss.cc (right):

http://codereview.chromium.org/5009/diff/1/6#newcode68
Line 68: HMAC::~HMAC() {
Maybe just make it one line then.

[Dean McNamee, 2008-09-28 13:27:26]

http://codereview.chromium.org/5009/diff/1/5
File base/base_drag_source.cc (left):

http://codereview.chromium.org/5009/diff/1/5#oldcode62
Line 62: return ref_count_;
Evan pointed out that this code isn't thread safe at all.  Interlocked
instructions are used, but then ref_count_ is accessed incorrectly and
not-atomically.  The correct way to do it would be to cache the result, but that
would still be incorrect with regards to threading, since you could return 1
while another thread has destroyed the object.

I don't know how the COM APIs are supposed to work.  My guess is this object
isn't meant to be thread safe to begin with?  Or is there some thread safety
concerns with COM that we were trying to address here?

[cpu_(ooo_6.6-7.5), 2008-09-29 00:00:44]

I think what MA wants to say is that STA com objects are thread safe as long
they are accessed thru the COM interfaces, the STA machinery serializes the
access to the object.

On 2008/09/28 13:27:26, deanm wrote:
> http://codereview.chromium.org/5009/diff/1/5
> File base/base_drag_source.cc (left):
> 
> http://codereview.chromium.org/5009/diff/1/5#oldcode62
> Line 62: return ref_count_;
> Evan pointed out that this code isn't thread safe at all.  Interlocked
> instructions are used, but then ref_count_ is accessed incorrectly and
> not-atomically.  The correct way to do it would be to cache the result, but
that
> would still be incorrect with regards to threading, since you could return 1
> while another thread has destroyed the object.
> 
> I don't know how the COM APIs are supposed to work.  My guess is this object
> isn't meant to be thread safe to begin with?  Or is there some thread safety
> concerns with COM that we were trying to address here?

[cpu_(ooo_6.6-7.5), 2008-09-29 00:09:59]

http://codereview.chromium.org/5009/diff/1/3
File base/event_recorder.h (right):

http://codereview.chromium.org/5009/diff/1/3#newcode72
Line 72: playback_msg_(),
we normally don't make explicit the calls to default ctors of subobjects. The
compiler does that for us just fine.

[cpu_(ooo_6.6-7.5), 2008-09-29 00:11:30]

http://codereview.chromium.org/5009/diff/1/9
File base/base_drop_target.cc (right):

http://codereview.chromium.org/5009/diff/1/9#newcode17
Line 17: : current_data_object_(),
Same, here why call default ctor?

[M-A Ruel, 2008-09-29 19:24:31]

I added a few files to fix tests, compile failure and reduce atl dependency
while at it.

[Dean McNamee, 2008-09-30 20:47:26]

Looks good enough for me.  Sorry about the confusion, too many review URLs.

http://codereview.chromium.org/5009/diff/616/422
File base/base_drop_target.cc (right):

http://codereview.chromium.org/5009/diff/616/422#newcode24
Line 24: BaseDropTarget::~BaseDropTarget() {
Maybe just put this on one line

http://codereview.chromium.org/5009/diff/616/426
File base/base_drop_target.h (right):

http://codereview.chromium.org/5009/diff/616/426#newcode22
Line 22: // This class is meant to be used in a STA and is not multithread-safe.
an STA, I think

http://codereview.chromium.org/5009/diff/616/419
File base/hmac_nss.cc (right):

http://codereview.chromium.org/5009/diff/616/419#newcode68
Line 68: HMAC::~HMAC() {
One line?

[cpu_(ooo_6.6-7.5), 2008-09-30 23:35:05]

lgtm

http://codereview.chromium.org/5009/diff/616/422
File base/base_drop_target.cc (right):

http://codereview.chromium.org/5009/diff/616/422#newcode24
Line 24: BaseDropTarget::~BaseDropTarget() {
On 2008/09/30 20:47:26, Dean McNamee wrote:
> Maybe just put this on one line

I rather have a way to put a breakpoint in the dtor