I had just started this code review, and have mostly pointed out superficial nits, but as we have just discussed, let's focus on the "v2" API instead of the "v1" API that this CL implements. If we want to resurrect the "v1" API, then we can come back to this CL (and also presumably the matching patch for Chromium).
Still, for the record, here are some comments. ======================================================================== http://mondrian.corp.google.com/file/10012347///depot/googleclient/gears/opensource/gears/Makefile?a=1 File //depot/googleclient/gears/opensource/gears/Makefile (snapshot 1) ------------------------------------ Line 1267: Whitespace. ------------------------------------ Line 1271: Whitespace. ======================================================================== http://mondrian.corp.google.com/file/10012347///depot/googleclient/gears/opensource/gears/Makefile?a=0 File //depot/googleclient/gears/opensource/gears/Makefile ------------------------------------ Line 2065: IEMOBILE_CPPSRCS += \ Unrelated change? ------------------------------------ Line 2096: OPERA_CPPSRCS += \ Unrelated change? ======================================================================== http://mondrian.corp.google.com/file/10012347///depot/googleclient/gears/opensource/gears/desktop/desktop.cc?a=1 File //depot/googleclient/gears/opensource/gears/desktop/desktop.cc (snapshot 1) ------------------------------------ Line 63: #else I would prefer #elif BROWSER_CHROME && WIN32 as per the other #if's in this changelist. ------------------------------------ Line 287: // On platforms other than Windows desktop, the HTML dialog always returns Any idea why Mondrian is marking this section as changed? ======================================================================== http://mondrian.corp.google.com/file/10012347///depot/googleclient/gears/opensource/gears/desktop/drop_target_base.h?a=1 File //depot/googleclient/gears/opensource/gears/desktop/drop_target_base.h (snapshot 1) ------------------------------------ Line 36: #elif BROWSER_NPAPI && WIN32 How about s/BROWSER_NPAPI/BROWSER_CHROME/ (and then remove the superfluous comment on the next line)? ======================================================================== http://mondrian.corp.google.com/file/10012347///depot/googleclient/gears/opensource/gears/desktop/drop_target_cr.cc?a=1 File //depot/googleclient/gears/opensource/gears/desktop/drop_target_cr.cc (snapshot 1) ------------------------------------ Line 1: // Copyright 2008, Google Inc. s/2008/2009/. ------------------------------------ Line 29: #if defined(BROWSER_NPAPI) && defined(WIN32) This shouldn't be necessary, since the Makefile only pulls in drop_target_cr.h for Chrome. ------------------------------------ Line 44: #include "gears/desktop/tracewin.h" Your tracewin utility is not part of this CL. Either propose to add tracewin to Gears, or remove all references to tracewin from this CL. ------------------------------------ Line 50: // dicey; it needs to be per-NPP-instance state. I don't think that this example (accepting .txt versus .pdf) is convincing. Even if two different tabs accept different sorts of files, the global state (i.e. what files are being dragged (and specifically file names, mimes, total size)) is the same, regardless of whether or not the tab accepts those files. I think the aspect worth mentioning is that Chrome (and eventually other browsers) are *multi-process*, and that one tab's DnD event stream can interleave (as far as Gears sees it) with another tab's stream. The multi-process thing is what is of concern, not what types of files a tab (i.e. a web page) accepts. ------------------------------------ Line 55: static std::set<std::string16> m_mime_set; Gears coding style (which may or may not agree 100% with Google coding style) seems to be that global variables have a g_ prefix, not a m_ prefix. ------------------------------------ Line 67: class EventListener : public PluginBase { The other NPAPI port (drop_target_sf.cc) already implements a GearsHtmlEventListener class. We should be able to re-use this. Perhaps GearsHtmlEventListener needs to be moved to drag_and_drop_utils_common (possibly behind an #if BROWSER_NPAPI). Also, I believe the GearsHtmlEventListener approach (with 4 instances to listen to the 4 events ("dragenter", "dragover", "dragleave", "drop")) is simpler (and therefore better) than this approach (with 1 instance for 4 events, and therefore has to maintain a std::map of strings, and needs to both look up the NPN_GetProperty(... "type"), as well as do a std::map look-up on every event handled). ------------------------------------ Line 85: data_ = data, callback_ = callback; While this is valid C++, and leads to a smaller LOC-count, I much prefer one statement per line. In other words, I wouldn't use the comma operator here. ------------------------------------ Line 129: #define ELEMENT_EVENT_DRAGENTER 1 drag_and_drop_utils_common.h already defines a DRAG_AND_DROP_EVENT_DRAGENTER, enum with values like DRAG_AND_DROP_EVENT_DRAGENTER. Please re-use that, since the enum is a tighter type than the classic C style "plain int and #define'd macros". ------------------------------------ Line 141: target->Ref(); // Ref() the drop target and return it. I don't think this comment adds anything that the code itself doesn't tell you. Furthermore, my understanding is that Gears practice is to usually have //-style comments on their own line preceding the code that they comment on, rather than as a suffix on the same line of the code that they comment on. ------------------------------------ Line 150: JsDomElement &element, JsObject *options, std::string16 *error) Is element used? If not, cut it. ------------------------------------ Line 158: listener_ = NULL; I would put member variable initializers up a few lines, so that it is listener_(NULL) between debug_(false) and the open-curly. Also, can you also initialize accept_, dragging_, and debug_. ------------------------------------ Line 315: NPN_GetValue(npp_, static_cast<NPNVariable>(7000), &event_); These magic numbers (7000 and 7001) are not commented, as far as I can tell. ------------------------------------ Line 374: private: We don't need this second private: ------------------------------------ Line 380: private: We don't need this third private: ------------------------------------ Line 382: int32 identity_; If identity_ is not yet implemented (or, at least as far as I can tell, identity_ is not set anywhere), then I would leave it out for now entirely, and introduce it in a later CL. Also, if it identity_ is meant to be a pointer to something (an IUnknown* ??), then I would prefer a size_t or a void* than an int32, since eventually we might support 64-bit Gears. ------------------------------------ Line 383: int32 event_id_; Similarly, AFAICT event_id_ is never set. I'd cut it. ======================================================================== http://mondrian.corp.google.com/file/10012347///depot/googleclient/gears/opensource/gears/desktop/drop_target_cr.h?a=1 File //depot/googleclient/gears/opensource/gears/desktop/drop_target_cr.h (snapshot 1) ------------------------------------ Line 1: // Copyright 2008, Google Inc. s/2008/2009/. ------------------------------------ Line 30: // The Drag-and-Drop API has not been finalized for official builds. Please replace tabs with spaces, all throughout this file. ------------------------------------ Line 32: #if defined(BROWSER_NPAPI) && defined(WIN32) This shouldn't be necessary, since the Makefile only pulls in drop_target_cr.h for Chrome. ======================================================================== http://mondrian.corp.google.com/file/10012347///depot/googleclient/gears/opensource/gears/desktop/drop_target_registration.h?a=1 File //depot/googleclient/gears/opensource/gears/desktop/drop_target_registration.h (snapshot 1) ------------------------------------ Line 43: #elif BROWSER_NPAPI && WIN32 // Chrome Again, s/BROWSER_NPAPI/BROWSER_CHROME/. ======================================================================== -- To respond, reply to this email or visit http://mondrian.corp.google.com/10012347
