179658 - clean up external drag and drop selection target type handling [gtk]

Status: REOPENED

(Core :: XUL, task)

Description

[Bolian Yin]

Comment 1

[Bolian Yin]

This is caused by the different target name in Mozilla and gtk2. Some converts 
are needed like what has been done in Copy/Paste. Simple patch about auto 
mapping of target name can work. But I'd like to rewrite part of the code in 
widget/src/gtk2/nsDragService.cpp, to make it easy to read, to expand and I 
think use less lines of code.

Comment 2

[Bolian Yin]

-> bolian

Comment 3

[Bolian Yin]

Attachment: patch

setup a table to help the converting between mozilla internal target and
outside target.

Comment 4

[Bolian Yin]

a DnD problem with StartOffice and its fix:
---------------
Now, both Mozilla and StarOffice are using XDnD protocol (Mozilla use it through
gtk). But it seems there is problem in ther cooperation.
Think this situation,  user Drags something from StarOffice into Mozilla and
"Keeps Moving the mouse pointer while drops".  In this case, StarOffice will
send one or more "XdndPosition" after the "XdndLeave" and "XdndDrop".  This will
confuse Mozilla, who think "XdndLeave" indicates the end of the current drag
session, and the "XdndPostion" after that will make Mozilla start another drag
session. It of course cause problems.
---------------
Answer from [e-mail]
fix is already in the code, will be merged into the 644 tree around mid of
March. You can look at

http://www.openoffice.org/issues/show_bug.cgi?id=11783

to track the corresponding issue. 
---------------

Comment 5

[Christian Persch (GNOME) (away; not receiving bug mail)]

The patch in attachment
http://bugzilla.mozilla.org/attachment.cgi?id=106626&action=view still applies
cleanly [with only line offset fuzz], and I checked that it does make it
possible to drag text from mozilla to GtkEntry/GtkTextView:s. This is an
important issue for embedding [Epiphany]. What work (if any) is needed to make
the patch ready to check it in?

Comment 6

[Bolian Yin]

Hi Blizzard,

Can you give r= for this long time waiting bug? It works well for a long time in
our internal built gtk2 mozilla.

Comment 7

[Crispin Flowerday (not receiving bug mail)]

*** Bug 210308 has been marked as a duplicate of this bug. ***

Comment 8

[Crispin Flowerday (not receiving bug mail)]

Aren't there encoding issues with the UTF8_STRING drag type ? When passing the
data to gtk, the string is encoded using |ConvertUnicodeToPlatformPlainText|
which from my reading of the code isn't guaranteed to return UTF-8.

Comment 9

[Alexander Gerasimov]

[e-mail] from the Mozilla team reported that this bug is 
pertinent to this Java issue (which is not a Java bug actually):
http://developer.java.sun.com/developer/bugParade/bugs/5005676.html.

When dragging some text from Mozilla (verified with 'Mozilla 1.5, Copyright (c)
2003 mozilla.org, build 2003100718') to an XDND-aware drop target (e.g., a
Java drop target as of JDK 1.5), Mozilla exports data in the following
targets: text/_moz_htmlcontext, text/_moz_htmlinfo, text/html, text/unicode,
text/plain.

Only text/html and text/plain are standard formats that other applications
should be able to understand. But they contain no information about
encoding, so drop targets can't figure out how to translate text/html and
text/plain data. If Mozilla exported text targets specifying their charsets
(e.g., text/html;charset=ISO-2022-JP), then there would not be any troubles.

Comment 10

[Louie Zhao]

Attachment: patch v2

This patch is based on mozilla 1.7 branch.

Comment 11

[Louie Zhao]

Attachment: patch v2

updated patch.

Comment 12

[Christian Persch (GNOME) (away; not receiving bug mail)]

Louie Zhao: is this patch (attachment 145992 [details] [diff] [review]) ready for review? If so, please
request r/sr, so this will be looked at by the right people -- this is an
important issue for embed (Epiphany). Thanks!

Comment 13

[Louie Zhao]

Comment on attachment 145992 [details] [diff] [review]
patch v2

This patch has been using in Sun Mozilla codebase for a long time and works
fine. 

Blizzard, can you review this patch? Thanks a lot.

Comment 14

[Lorenzo Colitti]

*** Bug 246741 has been marked as a duplicate of this bug. ***

Comment 15

[Brian Ryner (not reading)]

Comment on attachment 145992 [details] [diff] [review]
patch v2

>+//converters
>+
>+void
>+utf8_to_ucs2 (const char *aDataIn, unsigned int aDataInLen,
>+              char **aDataOut, unsigned int *aDataOutLen)

I believe you need |static| here since it was declared with |static|.  Same for
the other converter functions.

Comment 16

[Christian Persch (GNOME) (away; not receiving bug mail)]

Comment on attachment 145992 [details] [diff] [review]
patch v2

Louie Zhao: This patch has r + sr; if it's still relevant can the sr's comment
in comment 15 be addressed and the patch checked in?

Comment 17

[Yevgen Muntyan]

See http://bugzilla.gnome.org/show_bug.cgi?id=429738

Comment 18

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: forward-port to Mozilla 1.8 (Firefox 2.0)

Fixed up conflicts and diffed against 2.0.x and 1.5.x.  Tested and works.  There were only minor changes to this patch, so I'll carry over the r=blizzard sr=bryner and seek branch approvals.

This is a rather important patch that Sun has carried for a long time and tested according to earlier comments.  I've ran some testing and this also works without regressing anything AFAICT.  This enables dragging and dropping between e.g. Firefox and GTK2 applications, which has been rather broken all this time.

Comment 19

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: forward-port to HEAD

Same as the 2.0 patch, however, there is a change in one hunk in nsDragService.cpp @@ -1104,28 +1066,18 @@ that should probably get looked at.  The changes to bug 376777 conflicted, but I believe that removing the code in question is correct because it will be converted with the in2out function ucs2_to_text) which should do the same job.  Seeking r+sr on this change.

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 271699 [details] [diff] [review]
forward-port to Mozilla 1.8 (Firefox 2.0)

This is the sort of thing we need to get in early in a cycle, not after the nomination deadline. Now we're in firedrill mode and can't take it. ->1.8.1.6

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 271701 [details] [diff] [review]
forward-port to HEAD

+            addBOM(NS_REINTERPRET_CAST(guchar **, &primitive_data),
+                   NS_REINTERPRET_CAST(gint *, &len));

We're using reinterpret_cast<> now.

Comment 22

[Alfred Peng]

With the patch applied, Firefox trunk build will hang with dead lock when dragging some text.

Comment 23

[Daniel Veditz [:dveditz]]

Comment on attachment 271699 [details] [diff] [review]
forward-port to Mozilla 1.8 (Firefox 2.0)

approved for 1.8.1.7 and 1.8.0.14, a=dveditz for release-drivers

Comment 24

[Daniel Veditz [:dveditz]]

reassigning to caillon based on patch activity/interest.

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #22)
> With the patch applied, Firefox trunk build will hang with dead lock when
> dragging some text.

Is this still a problem?

Comment 26

[Alfred Peng]

Attachment: For trunk

Update the patch to apply to the CVS HEAD. It seems that NS_ConvertUTF8toUCS2 isn't in use any more. Use NS_ConvertASCIItoUTF16 instead in function utf8_to_ucs2. Any other better replacement?

(In reply to comment #25)
> Is this still a problem?

Yes, I can still see the hang on my Solaris x86 box here with this patch applied. Will post the stack trace next.

Comment 27

[Alfred Peng]

Attachment: stack trace during the hang

This is one of the traces I've got here. The stack traces are different for the 1-19 frames every time I attach to the Firefox process.

Comment 28

[Mike Connor [:mconnor]]

Comment on attachment 271701 [details] [diff] [review]
forward-port to HEAD

a- based on alfred's comments

Comment 29

[Reed Loden [:reed]]

(In reply to comment #26)
> Yes, I can still see the hang on my Solaris x86 box here with this patch
> applied. Will post the stack trace next.

Can you test MOZILLA_1_8_BRANCH to see if it has the same problem (with regards to the hang)?

Comment 30

[Boris Zbarsky [:bzbarsky]]

This breaks branch compat.  At the very least, it caused bug 398422 on branch.

Comment 31

[Reed Loden [:reed]]

This caused bug 398422 at least, and may have other issues. Maybe we should back this out and respin without it? :/

Comment 32

[Daniel Veditz [:dveditz]]

Backed out of
 MOZILLA_1_8_BRANCH
 MOZILLA_1_8_0_BRANCH
 GECKO181_20071004_RELBRANCH

to fix regression bug 398422

Comment 33

[Daniel Veditz [:dveditz]]

Comment on attachment 271699 [details] [diff] [review]
forward-port to Mozilla 1.8 (Firefox 2.0)

removing approvals, this patch caused regressions.

Comment 34

[Karl Tomlinson (:karlt)]

It looks like bug 513885 has fixed this as originally reported by switching from text/plain to text/plain;charset=utf-8.  Bug 513885 comment 6 mentions some other target types that perhaps could/should be added, but text/plain;charset=utf-8 should be enough for GTK apps.

I suspect there is further value in the patch here.

Ultimately, sharing the selection handling code between nsDragService and nsClipboard would mean that these kind of issues don't need to be fixed twice.

Comment 35

[Emilio Cobos Álvarez (:emilio)]

Yeah Stransky also reworked GTK DnD for Wayland over time.

Comment 36

[Emilio Cobos Álvarez (:emilio)]

Well I guess I'll let him confirm whether there's something to do here.

Comment 37

[Martin Stránský [:stransky] (ni? me)]

Will look at it, adding to D&D tracker.