148671 - function retrieveURLFromData() is triplicated (?) in the code with slight differences.

Status: RESOLVED FIXED

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

Description

[Pierre Chanial]

I have selected the best retrieveURLFromData I found and added on optimization.
and I put it in transferUtils inside nsTransferable.js
Patch will follow.
rginda, jag, ben, do you agree with the way this cleanup is done?

Comment 1

[Pierre Chanial]

Attachment: Patch v1.0

r and sr needed

Comment 2

[timeless]

i think you should document in tabbrowser.xml that you require  transferUtils 
from nsTransferable.js

otherwise someone who decides to use a <tabbrowser/> might not know that it's 
required...

Comment 3

[Pierre Chanial]

tabbrowser uses also nsDragAndDrop.js. It could be a good idea to specify the
dependencies in a header of tabbrowser.xml

Comment 4

[timeless]

Comment on attachment 85982 [details] [diff] [review]
Patch v1.0

just a reminder about an else after return here:
+	 if (aData.search(client.linkRE) != -1)
+	   return aData;
+	 else
+	   return null;

you might consider
+	 if (aData.search(client.linkRE) == -1)
+	   return null;
+	 return aData;

Comment 5

[jag (Peter Annema)]

Pierre: client.linkRE is a chatzilla thing which doesn't apply to tabbrowser etc.

You could factor out the text/unicode case like:

switch (flavour) {
  case "text/unicode":
    var url = ... // do whatever needs doing at each site
    return url;
  default:
    return transferUtils.retrieveURLFromData(data, flavour);
}

and make retrieveURLFromData deal with just the x-moz-url and x-moz-file cases.

Or add the |case "text/unicode": return data;| to retrieveURLFromData and
override it for chatzilla.

Comment 6

[Pierre Chanial]

Attachment: Patch v1.1

Patch adressing jag's and rginda's concerns by leaving the link filtering on
the chatzilla side.

Comment 7

[Robert Ginda]

Comment on attachment 90560 [details] [diff] [review]
Patch v1.1

r=rginda on the chatzilla changes

Comment 8

[jag (Peter Annema)]

Comment on attachment 90560 [details] [diff] [review]
Patch v1.1

>+        var ioService = Components.classes["@mozilla.org/network/io-service;1"]
>+          .getService(Components.interfaces.nsIIOService);

Could you format it like this:

	var ioService = Components.classes["@mozilla.org/network/io-service;1"]
				 
.getService(Components.interfaces.nsIIOService);

No need for a new patch for this.

>Index: extensions/irc/xul/content/static.js
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/irc/xul/content/static.js,v
>retrieving revision 1.53
>diff -u -r1.53 static.js
>--- extensions/irc/xul/content/static.js	25 May 2002 00:39:59 -0000	1.53
>+++ extensions/irc/xul/content/static.js	8 Jul 2002 23:55:11 -0000
>@@ -1932,8 +1907,8 @@
> contentDropObserver.onDrop =
> function tabdnd_drop (aEvent, aXferData, aDragSession)
> {
>-    var url = retrieveURLFromData(aXferData.data, aXferData.flavour.contentType);
>-    if (!url)
>+    var url = transferUtils.retrieveURLFromData(aXferData.data, aXferData.flavour.contentType);
>+    if (!url || url.search(client.linkRE) == -1)
>         return;
>     
>     if (url.search(/\.css$/i) != -1  && confirm (getMsg("tabdnd_drop", url)))

Is it okay to search on client.linkRE for all flavours?

Comment 9

[Robert Ginda]

> Is it okay to search on client.linkRE for all flavours?

yes.

Comment 10

[jag (Peter Annema)]

For the indentation, this is what I meant (stupid bugzilla turning spaces into tabs)

var ioService = Components.classes["@mozilla.org/network/io-service;1"]
                          .getService(Components.interfaces.nsIIOService);

sr=jag

Comment 11

[Pierre Chanial]

Attachment: Patch v1.2

indentation corrected

Comment 12

[Pierre Chanial]

checked in by timeless weeks ago, marking fix