========================================================================
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 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.
> 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.
------------------------------------
Line 272: if (std::string(drag->type()) != "Files")
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.
------------------------------------
Line 292: for (size_t i = 0; i < g_drag_files; ++i) {
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.
------------------------------------
Line 382: if (!g_identity || g_identity < drag.identity()) {
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.
> 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.
> > Or should the "<" instead be a "!="?
>
> We need the "<"
OK, if it's strictly increasing, then can you assert that !(g_identity >
drag.identity())?
========================================================================
--
To respond, reply to this email or visit
http://mondrian.corp.google.com/10892071
