nsIClipboard uses the deprecated nsISupportsArray interface

RESOLVED FIXED in mozilla1.9beta2

Status

()

--
minor
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: WeirdAl, Assigned: standard8)

Tracking

({dev-doc-complete})

unspecified
mozilla1.9beta2
dev-doc-complete
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

12 years ago
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
(Assignee)

Comment 1

11 years ago
Created attachment 284624 [details] [diff] [review]
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.
Assignee: jag → bugzilla
Status: NEW → ASSIGNED
Attachment #284624 - Flags: superreview?(roc)
Attachment #284624 - Flags: review?(roc)
Attachment #284624 - Flags: review?(neil)
(Assignee)

Comment 2

11 years ago
Created attachment 284625 [details] [diff] [review]
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.
Attachment #284625 - Flags: review?(daniel.boelzle)

Comment 3

11 years ago
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)?
I'd be OK with this patch, but Neil's option sounds better.
(Assignee)

Comment 5

11 years ago
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.
Attachment #284624 - Attachment is obsolete: true
Attachment #284624 - Flags: superreview?(roc)
Attachment #284624 - Flags: review?(roc)
Attachment #284624 - Flags: review?(neil)
(Assignee)

Updated

11 years ago
Attachment #284625 - Attachment is obsolete: true
Attachment #284625 - Flags: review?(daniel.boelzle)
(Assignee)

Comment 6

11 years ago
Created attachment 285115 [details] [diff] [review]
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?
Attachment #285115 - Flags: superreview?(roc)
Attachment #285115 - Flags: review?(roc)
Attachment #285115 - Flags: review?(neil)
(Assignee)

Updated

11 years ago
Attachment #285115 - Attachment description: Replaces nsISupportsArray with nsIArray in nsIClipboard v2 → Replaces nsISupportsArray with nsIArray in nsIClipboard v2 (-w patch)
(Assignee)

Comment 7

11 years ago
Created attachment 285117 [details] [diff] [review]
Replaces nsISupportsArray with nsIArray in nsIClipboard v2 (normal patch)

The normal version of the patch.
(Assignee)

Comment 8

11 years ago
Created attachment 285118 [details] [diff] [review]
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.
Attachment #285118 - Flags: review?(daniel.boelzle)
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!
Attachment #285115 - Flags: superreview?(roc)
Attachment #285115 - Flags: superreview+
Attachment #285115 - Flags: review?(roc)
Attachment #285115 - Flags: review+

Comment 10

11 years ago
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?
Attachment #285115 - Attachment description: Replaces nsISupportsArray with nsIArray in nsIClipboard v2 (-w patch) → Replaces nsISupportsArray with an XPIDL array in nsIClipboard v2 (-w patch)
Attachment #285115 - Flags: review?(neil) → review+
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.
Attachment #285118 - Flags: review?(daniel.boelzle) → review-
Created attachment 285372 [details] [diff] [review]
[checked in] Calendar patch v2 - to be cross-committed to branch/trunk
Attachment #285118 - Attachment is obsolete: true
Attachment #285372 - Flags: review?(michael.buettner)
Comment on attachment 285372 [details] [diff] [review]
[checked in] Calendar patch v2 - to be cross-committed to branch/trunk

r=mickey.
Attachment #285372 - Flags: review?(michael.buettner) → review+
(Assignee)

Comment 14

11 years ago
Created attachment 288376 [details] [diff] [review]
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?
Attachment #288376 - Flags: review?(sspitzer)
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 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.
Attachment #288376 - Flags: review?(sspitzer)

Comment 17

11 years ago
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],

/**
neil, good point, we should just cache the array now.
(Assignee)

Comment 19

11 years ago
Created attachment 288747 [details] [diff] [review]
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.
Attachment #285115 - Attachment is obsolete: true
Attachment #285117 - Attachment is obsolete: true
Attachment #288376 - Attachment is obsolete: true
Attachment #288747 - Flags: review?(sspitzer)
Attachment #288747 - Flags: review?(roc)
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!
Attachment #288747 - Flags: review?(sspitzer) → review+
(Assignee)

Comment 21

11 years ago
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.
Attachment #288747 - Flags: approval1.9?

Updated

11 years ago
Attachment #288747 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 22

11 years ago
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.
Attachment #288747 - Attachment description: Replaces nsISupportsArray with XPIDL array in nsIClipboard v4 → Replaces nsISupportsArray with XPIDL array in nsIClipboard v4 (checked in)
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.
Attachment #285372 - Attachment description: Calendar patch v2 - to be cross-committed to branch/trunk → [checked in] Calendar patch v2 - to be cross-committed to branch/trunk
(Assignee)

Comment 24

11 years ago
All patches checked in -> fixed.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M10

Updated

11 years ago
Keywords: dev-doc-needed

Updated

11 years ago
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.