Fix some issues found looking at the code. I have made two posts about it:...

Fix some issues found looking at the code. I have made two posts about it:
    http://groups.google.com/group/chromium-discuss/browse_frm/thread/71ad6766910127f9#
    http://groups.google.com/group/chromium-dev/browse_frm/thread/a9fb86678f3b759c

[M-A Ruel, 2008-09-25 13:57:09]

http://codereview.chromium.org/4273/diff/1/4
File base/base_drag_source.cc (right):

http://codereview.chromium.org/4273/diff/1/4#newcode59
Line 59: return ULONG(0);
return 0U;

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

http://codereview.chromium.org/4273/diff/1/8#newcode27
Line 27: //TODO(gmendola) at this point the class user should already have call
the
Make sure to always have a space between the // and the actual comment, e.g:
// TODO...
// RevokeDragDrop, ...
// registered ...
and so on.

Also, if you read RevokeDragDrop() doc, you'll see that it calls Release() on
the currently registered drag handler object object, so in theory, while the
destructor is called, is it dangerous and illegal to call RevokeDragDrop()
recursively.

So I don't understand what you exactly want to achieve here.

http://codereview.chromium.org/4273/diff/1/8#newcode144
Line 144: return ULONG(0);
return 0U;

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

http://codereview.chromium.org/4273/diff/1/5#newcode69
Line 69: //EMPTY
Remove the comment.

[mendola, 2008-09-25 15:19:06]

http://codereview.chromium.org/4273/diff/1/4
File base/base_drag_source.cc (right):

http://codereview.chromium.org/4273/diff/1/4#newcode59
Line 59: return ULONG(0);
On 2008/09/25 13:57:09, M-A wrote:
> return 0U;

Done.

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

http://codereview.chromium.org/4273/diff/1/8#newcode27
Line 27: //TODO(gmendola) at this point the class user should already have call
the
On 2008/09/25 13:57:09, M-A wrote:
> Make sure to always have a space between the // and the actual comment, e.g:
> // TODO...
> // RevokeDragDrop, ...
> // registered ...
> and so on.
> 
> Also, if you read RevokeDragDrop() doc, you'll see that it calls Release() on
> the currently registered drag handler object object, so in theory, while the
> destructor is called, is it dangerous and illegal to call RevokeDragDrop()
> recursively.
> 
> So I don't understand what you exactly want to achieve here.

I would like to have an error in case of a BaseDropTarget instance being
destroyed without call the RevokeDragDrop:

HWND my_hwnd = <whatever>;
BaseDropTarget * my_target = new BaseDropTarget(my_hwnd);
delete my_target;

instead of:

HWND my_hwnd = <whatever>;
BaseDropTarget * my_target = new BaseDropTarget(my_hwnd);
RevokeDragDrop(my_hwnd);
delete my_target;

or this is not an issue ?

http://codereview.chromium.org/4273/diff/1/8#newcode144
Line 144: return ULONG(0);
On 2008/09/25 13:57:09, M-A wrote:
> return 0U;

Done.

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

http://codereview.chromium.org/4273/diff/1/5#newcode69
Line 69: //EMPTY
On 2008/09/25 13:57:09, M-A wrote:
> Remove the comment.

Done.

[M-A Ruel, 2008-09-25 15:56:58]

On 2008/09/25 15:19:06, mendola wrote:
> I would like to have an error in case of a BaseDropTarget instance being
> destroyed without call the RevokeDragDrop:
> 
> HWND my_hwnd = <whatever>;
> BaseDropTarget * my_target = new BaseDropTarget(my_hwnd);
> delete my_target;
> 
> instead of:
> 
> HWND my_hwnd = <whatever>;
> BaseDropTarget * my_target = new BaseDropTarget(my_hwnd);
> RevokeDragDrop(my_hwnd);
> delete my_target;
> 
> or this is not an issue ?

You know COM? Because it's COM code there. Here's the way the object is used:
HWND my_hwnd = <whatever>;
BaseDropTarget * my_target = new BaseDropTarget(my_hwnd);
// which calls RegisterDragDrop(hwnd, this), which calls this->AddRef().
(...)
RevokeDragDrop(my_hwnd);
// which calls my_target->Release(), which calls delete this.

That's why I say the comment and the DCHECK are wrong.

M-A

[mendola, 2008-09-25 18:31:49]

On 2008/09/25 15:56:58, M-A wrote:
> On 2008/09/25 15:19:06, mendola wrote:
> > I would like to have an error in case of a BaseDropTarget instance being
> > destroyed without call the RevokeDragDrop:
> > 
> > HWND my_hwnd = <whatever>;
> > BaseDropTarget * my_target = new BaseDropTarget(my_hwnd);
> > delete my_target;
> > 
> > instead of:
> > 
> > HWND my_hwnd = <whatever>;
> > BaseDropTarget * my_target = new BaseDropTarget(my_hwnd);
> > RevokeDragDrop(my_hwnd);
> > delete my_target;
> > 
> > or this is not an issue ?
> 
> You know COM? Because it's COM code there. Here's the way the object is used:
> HWND my_hwnd = <whatever>;
> BaseDropTarget * my_target = new BaseDropTarget(my_hwnd);
> // which calls RegisterDragDrop(hwnd, this), which calls this->AddRef().
> (...)
> RevokeDragDrop(my_hwnd);
> // which calls my_target->Release(), which calls delete this.
> 
> That's why I say the comment and the DCHECK are wrong.
> 
> M-A

I see, my fault I got it wrong from the reply on this thread:
http://groups.google.com/group/chromium-dev/browse_frm/thread/a9fb86678f3b759c

I'll remove the comment block from DTOR then.

Gaetano

[M-A Ruel, 2008-09-26 15:56:50]

Ok this patch looks fine, I'll look at committing it.

[M-A Ruel, 2008-09-30 21:01:44]

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=2730
(albeit with some changes)

Please close this issue. Thanks.