374347 - nsIClipboard uses the deprecated nsISupportsArray interface

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Alex Vincent [:WeirdAl]]

http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsISupportsArray.idl#42 states nsISupportsArray is deprecated.  We should probably change it over to nsIArray.


http://lxr.mozilla.org/seamonkey/search?string=hasDataMatchingFlavors

Comment 1

[Mark Banner (:standard8)]

Attachment: Replaces nsISupportsArray with nsIArray in nsIClipboard

This is the fix for replacing nsISupportsArray with nsIArray in nsIClipboard.

Robert can you review the widget/ and browser/ parts please?

Neil can you review the suite/ and editor/ parts please?

There's a calendar change to go with this, but I'll attach that in a separate patch.

Comment 2

[Mark Banner (:standard8)]

Attachment: Calendar patch

This is the calendar patch to go with the main patch on this bug.

Note: This patch cannot be checked in on the 1.8 branch to keep it in sync as that has a different interface that cannot be changed.

Please let me know if I've requested review from the wrong person for this.

Comment 3

[[e-mail]]

All the callers actually have a native array {JS: var flavours = ["flavour"]; C++: const char *flavours = {"flavour", nsnull};} yet due to the poor choice of interface they both have to convert to an XPCOM object. Wouldn't it be saner to change the signature to booean hasDataMatchingFlavours([array, size_is(aCount)] in string aFlavourList, in unsigned long aCount, in long aWhichClipboard)?

Comment 4

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

I'd be OK with this patch, but Neil's option sounds better.

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 284624 [details] [diff] [review]
Replaces nsISupportsArray with nsIArray in nsIClipboard

(In reply to comment #4)
> I'd be OK with this patch, but Neil's option sounds better.

Thanks for confirming. I'll do an updated patch in the next few days.

Comment 6

[Mark Banner (:standard8)]

Attachment: Replaces nsISupportsArray with an XPIDL array in nsIClipboard v2 (-w patch)

Neil was right, converting to array usage is much saner - it removes a lot of code.

Here's the -w patch as it removes quite a few if statements, so I've adjusted the indentation of some inner parts of functions. Normal patch coming up in a mo.

Same as last time:

Robert can you review the widget/ and browser/ parts please?

Neil can you review the suite/ and editor/ parts please?

Comment 7

[Mark Banner (:standard8)]

Attachment: Replaces nsISupportsArray with nsIArray in nsIClipboard v2 (normal patch)

The normal version of the patch.

Comment 8

[Mark Banner (:standard8)]

Attachment: Calendar patch v2

This is the revised calendar patch to go with the main patch on this bug.

Note: This patch cannot be checked in on the 1.8 branch to keep it in sync as
that has a different interface that cannot be changed.

Please let me know if I've requested review from the wrong person for this.

Comment 9

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

Comment on attachment 285115 [details] [diff] [review]
Replaces nsISupportsArray with an XPIDL array in nsIClipboard v2 (-w patch)

+interface nsIArray;
 
Lose this.

Otherwise, looks excellent!

Comment 10

[[e-mail]]

Comment on attachment 285115 [details] [diff] [review]
Replaces nsISupportsArray with an XPIDL array in nsIClipboard v2 (-w patch)

>+    * @param  aFlavorList     An array of ACString.
An array of ASCII strings?

>+          nsDependentCString passedFlavorStr(aFlavorList[k]);
>           if (passedFlavorStr.Equals(transferableFlavorStr)) {
transferableFlavorStr.Equals(aFlavorList[k])?

>+  for (PRUint32 i = 0;i < aLength; ++i) {
Nit: while you're here, space after ;

>     var flavors = PlacesUtils.placesFlavors;

>   get placesFlavors() {
>+    return [PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER,
>+            PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR,
>+            PlacesUtils.TYPE_X_MOZ_PLACE];
>   }
Dunno what the browser peers (hint) think but maybe this could be a property or simply inlined into the caller?

>-      var flavours = ["moz/bookmarkclipboarditem", "text/x-moz-url"];
>+      var flavorArray = ["moz/bookmarkclipboarditem", "text/x-moz-url"];
Why rename the variable?

Comment 11

[Daniel Boelzle [:dbo]]

Comment on attachment 285118 [details] [diff] [review]
Calendar patch v2

We focus development on the branch and thus want to keep branch and trunk in sync to make life easier when checking in. I will come up with a patch that can be cross-committed.

Comment 12

[Daniel Boelzle [:dbo]]

Attachment: [checked in] Calendar patch v2 - to be cross-committed to branch/trunk

Comment 13

[Michael Büttner (no reviews TFN)]

Comment on attachment 285372 [details] [diff] [review]
[checked in] Calendar patch v2 - to be cross-committed to branch/trunk

r=mickey.

Comment 14

[Mark Banner (:standard8)]

Attachment: Replaces nsISupportsArray with nsIArray in nsIClipboard v3

Addressed Neil's comments. Carrying forward his and roc's reviews.

Seth, can you review the browser/ changes (to places) please?

Comment 15

[(not reading, please use [e-mail] instead)]

Mark, you've removed the memoization that Mano added (see bug #399930)

even after your fix for nsIClipboard, we could still do the memoization.

a "fake" patch:

Index: browser/components/places/content/controller.js
===================================================================
RCS file: /cvsroot/mozilla/browser/components/places/content/controller.js,v
retrieving revision 1.183
diff -u -p -r1.183 controller.js
--- browser/components/places/content/controller.js	25 Oct 2007 02:02:28 -0000	1.183
+++ browser/components/places/content/controller.js	12 Nov 2007 21:56:48 -0000
@@ -363,10 +363,12 @@ PlacesController.prototype = {
     // if the clipboard contains TYPE_X_MOZ_PLACE_* data, it is definitely
     // pasteable, with no need to unwrap all the nodes.
 
     var flavors = PlacesUtils.placesFlavors;
     var clipboard = PlacesUtils.clipboard;
     var hasPlacesData =
-      clipboard.hasDataMatchingFlavors(flavors,
+      clipboard.hasDataMatchingFlavors(flavors, flavors.length,
                                        Ci.nsIClipboard.kGlobalClipboard);
     if (hasPlacesData)
       return this._view.insertionPoint != null;
Index: browser/components/places/content/utils.js
===================================================================
RCS file: /cvsroot/mozilla/browser/components/places/content/utils.js,v
retrieving revision 1.75
diff -u -p -r1.75 utils.js
--- browser/components/places/content/utils.js	25 Oct 2007 02:02:29 -0000	1.75
+++ browser/components/places/content/utils.js	12 Nov 2007 21:56:49 -0000
@@ -1627,22 +1627,6 @@ var PlacesUtils = {
     this._openTabset(urlsToOpen, aEvent);
   },
 
   get placesFlavors() {
     delete this.placesFlavors;
-    var placeTypes = [PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER,
-                      PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR,
-                      PlacesUtils.TYPE_X_MOZ_PLACE];
-    this.placesFlavors = Cc["@mozilla.org/supports-array;1"].
-                         createInstance(Ci.nsISupportsArray);
-    for (var i = 0; i < placeTypes.length; ++i) {
-      var cstring = Cc["@mozilla.org/supports-cstring;1"].
-                    createInstance(Ci.nsISupportsCString);
-      cstring.data = placeTypes[i];
-      this.placesFlavors.AppendElement(cstring);
-    }
+    this.placesFlavors = [PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER,
+                          PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR,
+                          PlacesUtils.TYPE_X_MOZ_PLACE];
     return this.placesFlavors;
   },

   /**
    * Helper for the toolbar and menu views
    */

Comment 16

[(not reading, please use [e-mail] instead)]

Comment on attachment 288376 [details] [diff] [review]
Replaces nsISupportsArray with nsIArray in nsIClipboard v3

clearing review request for the browser/components/places parts, but will re-review promptly if you have a new version.

Comment 17

[[e-mail]]

Seth, it's actually bug 394695 that Mark backed out, but if you really think you're not going to win enough perf by switching to a literal JS array, you'd actually cache the array like this:

},

placesFlavors: [PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER,
                PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR,
                PlacesUtils.TYPE_X_MOZ_PLACE],

/**

Comment 18

[(not reading, please use [e-mail] instead)]

neil, good point, we should just cache the array now.

Comment 19

[Mark Banner (:standard8)]

Attachment: Replaces nsISupportsArray with XPIDL array in nsIClipboard v4 (checked in)

Updated patch:

- Caches the placesFlavor array as requested by Seth
- Fixes mac build problems (in cocoa) where I'd added an extra bracket and missed changing half the function (I found this problem using the try servers).

Seth, can you do the browser parts please?

Roc, I've put you on again to check you're happy with the mac fix.

Comment 20

[(not reading, please use [e-mail] instead)]

Comment on attachment 288747 [details] [diff] [review]
Replaces nsISupportsArray with XPIDL array in nsIClipboard v4 (checked in)

r=sspitzer for the places parts, thanks Mark!

Comment 21

[Mark Banner (:standard8)]

Comment on attachment 288747 [details] [diff] [review]
Replaces nsISupportsArray with XPIDL array in nsIClipboard v4 (checked in)

Requesting approval 1.9. This patch reworks code that uses obsolete items.

It has been test built on the try servers. If there's any performance impact, I'd expect it to be to improve performance as there will be less code/interactions going on overall.

Comment 22

[Mark Banner (:standard8)]

Comment on attachment 288747 [details] [diff] [review]
Replaces nsISupportsArray with XPIDL array in nsIClipboard v4 (checked in)

I've now checked the main patch into the trunk - no perf regressions etc apparent.

Daniel, can you check your patch in when you've got time? I wasn't able to do it as Sunbird trees are currently broken and I had a limited time to get this patch in.

Comment 23

[Daniel Boelzle [:dbo]]

Comment on attachment 285372 [details] [diff] [review]
[checked in] Calendar patch v2 - to be cross-committed to branch/trunk

calendar part checked in on HEAD and MOZILLA_1_8_BRANCH.

Comment 24

[Mark Banner (:standard8)]

All patches checked in -> fixed.