======================================================================== http://mondrian.corp.google.com/file/10892071///depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_cr.cc?a=1 File //depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_cr.cc (snapshot 1) ------------------------------------ Line 43: #define ELEMENT_EVENT_DRAGENTER 1 On 5:05 pm, 27 Apr, nigeltao wrote: > On 24 Apr, noel wrote: > > I get the same questions from the chromium side; someone has to go first, so > can > > we? > > Sure, somebody still has to go first, but I would still like to know more about > the chromium side, whether that's code that's actually been checked into > chromium or code that's merely been uploaded to codereview.chromium.org. Perhaps > some document-level comments, either in this .cc file or in its .h file, > mentioning how this will relate to chromium would be worthwhile.
ok sent http://mondrian/10947778 to michael, to document this interface. If you're interested in the chromium renderer <-> plugin communications, there's a engineering document describing that at http://dev.chromium.org/developers/design-documents, and the CPAPI sits atop it. The CPAPI is based on NPAPI (used uft8 and such), an it's documented in the chrome_plugin_api.h file. I modifed to data() and type() members of DragSession to make that uft8 explicit. > > I'd like to use the defines from desktop/drag_and_drop_utils_common.h - if we > > can start that gears-defined event enum from 1, then I could remove the > defines > > here and that'd be ok in chrome and for the other browsers we support in gears > I > > think. > > Making the INVALID enum be zero sounds good to me. ok, so I can add the drag_and_drop_utils_common.h to this CL and make that change? ------------------------------------ Line 272: if (std::string(drag->type()) != "Files") On 5:06 pm, 27 Apr, nigeltao wrote: > On 24 Apr, noel wrote: > > We discussed this before, the "Files" is internal between chrome and gears > (it's > > never exposed to user code) and I looked for some short name that makes sense, > > like "Files". > > OK, I remember now that we discussed this, but I would still have liked it in > writing, either in code review or, preferably, as a comment that "Files" here is > part of the Gears <--> Chrome IPC wire protocol, and not, for example, > necessarily the argument to the JavaScript dataTransfer.getData method. ok documented on http://mondrian/10947778 I use "Files", "Text" and "URL" here only because over in the chrome renderer V8/Webkit engine, I call the event.dataTransfer.getData method but from C++. The names felt right to me, saved me inventing them, but the CPAPI is designed to be extensible so that was also a consideration. ------------------------------------ Line 292: for (size_t i = 0; i < g_drag_files; ++i) { On 4:53 pm, 27 Apr, nigeltao wrote: > On 24 Apr, noel wrote: > > If you compared the AddFileMetaData here to the one in V1, note that I rewrote > > it here closer to FileDragAndDropMetaData and added the error checking. So > I'm > > heading in the direction of FileDragAndDropMetaData, but the inconsistencies > in > > that code you mentioned means it doesn't quite work for me. I'm chasing a > > crasher in chrome, for example, and the directory handling is helping me debug > > that issue, so I'd rather not remove it. We have TODOs for the > > FileDragAndDropMetaData cross-platform inconsistencies on our site page. > > OK, then please add a TODO to re-use the FileDragAndDropMetaData class from > drag_and_drop_utils_common.cc. Done. ------------------------------------ Line 382: if (!g_identity || g_identity < drag.identity()) { On 5:02 pm, 27 Apr, nigeltao wrote: > On 24 Apr, noel wrote: > > On 4:54 pm, 23 Apr, nigeltao wrote: > > > Is identity strictly increasing? If so, would sequence_number be a better > > name? > > > > Tony was ok with identity, so maybe that'll do. > > OK, identity is already in the chromium codebase. I didn't know this before you > sent some pointers to codereview.chromium.org. I mentioned this to you at the time of the review -- I was pretty excited about getting my first chromium review away with Tony and thought, you'd be interested. > > He wanted it initialized to -1, > > and turned into an int (that's what they do with ids). I wanted an uint32 > > because yes it's strictly increasing from the gears point of view. But our > > coding standard says we can't use uint32 for that, so we compromised on an > > strictly increasing int32! Maybe not such a great compromise on my part, I > now > > have to remember to cast it uint32 in chrome when returning it to gears on the > > CPAPI to cover it up. > > I'm not following why you have to cast between int32 and uint32. > In my experience, unsignedness is usually not worth it, even for things that are > conceptually never negative. But I don't know the CPAPI and where it > discriminates between signed and unsigned. Yeah the CPAPI is for gears, it's not fussy re: signed vs unsigned, which is nice for once. The identity always increases so unsigned looks right. We want an initializer for it; chrome and the CPAPI use 0. And it is unique, given that it increases each time the user drag enters to renderer tab content area. So when we see the identity increase, "<", we snapshot the drag files information at that time. > > > Or should the "<" instead be a "!="? > > > > We need the "<" > > OK, if it's strictly increasing, then can you assert that !(g_identity > > drag.identity())? hmm, the identity check does that, or were you taking about the case where the identity wraps back to 0? ------------------------------------ Line 436: #endif // OFFICIAL_BUILD updated the CL description, removed some TODO's, minor renames. ======================================================================== -- To respond, reply to this email or visit http://mondrian.corp.google.com/10892071
