221651 - OS/2 D&D Enhancements - Phase III

Status: RESOLVED FIXED

(Core :: DOM: Copy & Paste and Drag & Drop, enhancement)

Description

[Rich Walsh]

Attached is Phase III of my proposed enhancements to OS/2 drag & drop.

It introduces a new interface, nsIDragSessionOS2, which encapsulates
support for OS/2 dragover/drop events (i.e. operations where Mozilla
is the target of a drag).  Its public methods correspond to the d&d
messages an OS/2 window receives.  Its protected methods recast native
data as nsITransferable flavors during drag-enter and drop events.

The interface, implemented by nsDragService, relieves its client,
nsWindow, of any need to "understand" drag and drop.  nsWindow simply
dispatches the messages, then performs whatever action(s) the handler
specifies.

This design also simplifies the task of avoiding video corruption
during d&d.  Virtually all of the overhead currently required to
determine if a drag is in progress has been eliminated.

Changed Files:  nsWindow (.cpp & .h) and nsDragService (.cpp & .h)
New File:  nsIDragSessionOS2.h

Note:  nsIDragSessionOS2's header belongs in mozilla/widget/public
along with those for similar platform-specific interfaces.  However,
I don't know which support files need to be modified to permit this.

Comment 1

[Rich Walsh]

Attachment: diff

Comment 2

[Rich Walsh]

Attachment: nsIDragSessionOS2.h (new file)

Comment 3

[timeless]

.

Comment 4

[Mike Kaply [:mkaply]]

Rich, this isn't compiling for me on the GCC build but I have no idea why.

g++ -o nsWidgetFactory.o -c -DOSTYPE=\"OS21\" -DOSARCH=\"OS2\"
-DHAVE_DEPENDENT_LIBS -D_IMPL_NS_WIDGET -I.
-ID:/BUILDS/current/mozilla/widget/src/os2/../xpwidgets
-ID:/BUILDS/current/mozilla/widget/src/os2 -I../../../dist/include/gfx
-I../../../dist/include/xpcom -I../../../dist/include/string
-I../../../dist/include/dom -I../../../dist/include/necko
-I../../../dist/include/uconv -I../../../dist/include/intl
-I../../../dist/include/pref -I../../../dist/include/plugin
-I../../../dist/include/webshell -I../../../dist/include/webbrowserpersist
-I../../../dist/include/widget -I../../../dist/include
-I/BUILDS/current/mozilla/obj-dbg/dist/include/nspr     -I.       -fno-rtti
-fno-exceptions -Wall -Wconversion -Wpointer-arith -Wcast-align
-Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-long-long -pedantic
-Zomf -pipe  -DDEBUG -D_DEBUG -DDEBUG_ -DTRACING -g -fno-inline  
-DMOZILLA_CLIENT -include ../../../mozilla-config.h -Uunix -U__unix -U__unix__
-Wp,-MD,.deps/nsWidgetFactory.pp
D:/BUILDS/current/mozilla/widget/src/os2/nsWidgetFactory.cpp
In file included from D:/BUILDS/current/mozilla/widget/src/os2/nsWindow.h:42,
                 from
D:/BUILDS/current/mozilla/widget/src/os2/nsWidgetFactory.cpp:51:
D:/BUILDS/current/mozilla/widget/src/os2/nsToolkit.h:55: warning: `class 
   nsToolkit' has virtual functions but non-virtual destructor
In file included from D:/BUILDS/current/mozilla/widget/src/os2/nsDragService.h:42,
                 from
D:/BUILDS/current/mozilla/widget/src/os2/nsWidgetFactory.cpp:52:
../../../dist/include/widget/nsIDragSessionOS2.h:123: parse error before `{' 
   token
../../../dist/include/widget/nsIDragSessionOS2.h: In static member function 
   `static const nsIID& nsIDragSessionOS2::GetIID()':
../../../dist/include/widget/nsIDragSessionOS2.h:133: parse error before `;' 
   token
../../../dist/include/string/nsBufferHandle.h: At top level:
D:/BUILDS/current/mozilla/widget/src/os2/nsToolkit.h:87: warning: `void* 
   nsToolkitWindowProc(long unsigned int, long unsigned int, void*, void*)' 
   declared `static' but never defined
make.exe[1]: *** [nsWidgetFactory.o] Error 1
make.exe[1]: Leaving directory `D:/BUILDS/current/mozilla/obj-dbg/widget/src/os2'
make: *** [all] Error 2

There is no line 123 in nsIDragSessionOS2.h....

Comment 5

[jhp (no longer active)]

I do not get the error that Kaply is seeing.  Instead, I get "'IDC_DNDCOUNT' was
not declared in this scope".  Rich, was your patch missing a resource file change?

Comment 6

[jhp (no longer active)]

Attachment: patch against trunk

Bug 218312 was finally checked in, so I diffed against the very latest trunk.

Comment 7

[jhp (no longer active)]

Rich, have a look at the patch that I attached here and make sure that I didn't
screw things up too bad.

Comment 8

[jhp (no longer active)]

+// return true if all items support this flavor
+//  for (PRUint32 itemIndex = 0, *_retval = PR_TRUE;
+//       itemIndex < numDragItems && *_retval; ++itemIndex) {
+//    *_retval = PR_FALSE;

Is there any reason to keep this commented-out code here?

Also, some of the lines you added in this patch are pretty long.  If possible,
could you make them wrap at 80 chars?

Otherwise, looks good to me.

Comment 9

[Rich Walsh]

mozilla/widget/public/nsIDragSessionOS2.h is misformatted:  each line break
has an extra 0x0D (i.e. CrCrLf).  This accounts for the errors Mike saw -
line 123 is a blank line in the middle of a multiline macro.  BTW... this
defective version has already been checked in - can you back it out?

> +// return true if all items support this flavor
> +//  for (PRUint32 itemIndex = 0, *_retval = PR_TRUE;
> +//       itemIndex < numDragItems && *_retval; ++itemIndex) {
> +//    *_retval = PR_FALSE;

>Is there any reason to keep this commented-out code here?

IMHO, IsDataFlavorSupported() is misimplemented.  If support for multiple-item
drags is ever added, this code will provide a more correct result.  Right now,
it doesn't matter.

Would you like a revised diff with shorter line lengths?  

Comment 10

[Rich Walsh]

Attachment: patch against trunk - shorter lines

Shorter line lengths per comment #8

Comment 11

[jhp (no longer active)]

Attachment: updated patch

I added a change to mozilla/widget/public/Makefile.in, so that
nsIDragSessionOS2.h gets properly installed in the dist.  Otherwise, this patch
is exactly like the previous one.

Comment 12

[jhp (no longer active)]

Comment on attachment 135509 [details] [diff] [review]
updated patch

This looks good to me.	Kaply, why don't you take a look, and if you agree,
check this in as soon as you can (before 1.6b).

Comment 13

[Mike Kaply [:mkaply]]

We finally checked this in!

Thanks Rich :)