On Wed, Oct 22, 2008 at 8:19 PM, Aaron Boodman <[EMAIL PROTECTED]> wrote:
> LGTM
>
> ========================================================================
> http://mondrian.corp.google.com/file/8653382///depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_registry.cc?a=1
> File
> //depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_registry.cc
> (snapshot 1)
> ------------------------------------
> Line 108: *error_out = STRING16(L"Could not initialize a DropTarget.");
> If these errors are unexpected to occur, please use
> GET_INTERNAL_ERROR_MESSAGE()
> instead.
> ========================================================================
> http://mondrian.corp.google.com/file/8653382///depot/googleclient/gears/opensource/gears/desktop/drop_target_ie.h?a=1
> File //depot/googleclient/gears/opensource/gears/desktop/drop_target_ie.h
> (snapshot 1)
> ------------------------------------
> Line 35: #include <mshtmdid.h>
> Question about the approach here, feel free to save for Noel. I wonder why not
> use the same design as is used for Firefox, where the C++ code is a listener
> to
> the DOM drop target, not adding code into that element via binary behavior.
> ------------------------------------
> Line 45: // The 1 in the IDispEventSimpleImpl line is just an arbitrary
> positive
> Please add more comments here explaining why using this interface (like the
> comments from the CL description).
> ------------------------------------
> Line 99: CComPtr<IDispatch> event_source_for_disp_event_unadvise_;
> What if we just call this event_source_, or dom_drop_target_ or something? the
> for unadvise_ is just a detail of what this member will be used for, right?
> ========================================================================
> http://mondrian.corp.google.com/file/8653382///depot/googleclient/gears/opensource/gears/test/manual/drag_and_drop.html?a=1
> File
> //depot/googleclient/gears/opensource/gears/test/manual/drag_and_drop.html
> (snapshot 1)
> ------------------------------------
> Line 62: var dropZoneIdIsGood = false;
> Maybe move this to a unit test?
>
> It would also be good to have unit tests that exercised as much of the API as
> is
> feasible. Just calling the APIs will uncover lots of bugs.
> ========================================================================
>
> --
> To respond, reply to this email or visit
> http://mondrian.corp.google.com/8653382
>
