Closed Bug 1326494 Opened 3 years ago Closed 3 years ago

Remove some remaining nsISupportsArray references in mailnews

Categories

(MailNews Core :: Backend, defect, minor)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 54.0

People

(Reporter: aceman, Assigned: aceman)

References

Details

Attachments

(3 files)

Some misc references to nsISupportsArray.

1.
https://dxr.mozilla.org/comm-central/rev/72bc94bebbabfed990212cb8959c11fe5c974e05/mailnews/import/test/unit/resources/TestMailImporter.js#77 :

  FindMailboxes: function(location) {
    let result;
    result = Cc["@mozilla.org/supports-array;1"]
             .createInstance(Ci.nsIArray);
    this._collectMailboxesInDirectory(location, 0, result);

    return result;
  },

The nsIArray was converted in bug 945045 (https://hg.mozilla.org/comm-central/diff/117ad84fb83e/mailnews/import/test/unit/resources/TestMailImporter.js) but the "@mozilla.org/supports-array;1" has remained. Probably needs changing to "@mozilla.org/array;1". Actually the array contents is being changed in the functions around so it should probably be nsIMutableArray.

Then, at
https://dxr.mozilla.org/comm-central/rev/72bc94bebbabfed990212cb8959c11fe5c974e05/mailnews/import/test/unit/resources/TestMailImporter.js#66

result.AppendElement(descriptor, false);
needs to change to
result.appendElement(descriptor, false);

2. https://dxr.mozilla.org/comm-central/rev/72bc94bebbabfed990212cb8959c11fe5c974e05/suite/browser/pageinfo/pageInfo.js#220
const ARRAY_CONTRACTID          = "@mozilla.org/supports-array;1";
seems unused.

3. https://dxr.mozilla.org/comm-central/rev/72bc94bebbabfed990212cb8959c11fe5c974e05/suite/common/directory/directory.js#14
const ARRAY_CONTRACTID          = "@mozilla.org/mutable-array;1";
is the only occurence of "@mozilla.org/mutable-array;1" in whole of c-c/m-c. Is it correct or should it be "@mozilla.org/array;1" ?

4. https://dxr.mozilla.org/comm-central/rev/72bc94bebbabfed990212cb8959c11fe5c974e05/mail/base/content/mailWindowOverlay.js#2809
 // array of isupportsarrays of servers for a particular folder
Comment mentions iSupportsArray but the  pop3DownloadServersArray is an array of arrays (JS arrays).

5. https://dxr.mozilla.org/mozilla-central/rev/6f63f95e28ffc05c0d2f5ef6cd6e05905fe8ea5a/editor/libeditor/HTMLEditRules.cpp#5019
Comment 
" * and insert the inner content into the supplied issupportsarray at offset"
but the argument is a nsTArray.
> 2. https://dxr.mozilla.org/comm-central/rev/72bc94bebbabfed990212cb8959c11fe5c974e05/suite/browser/pageinfo/pageInfo.js#220
> const ARRAY_CONTRACTID          = "@mozilla.org/supports-array;1";
> seems unused.
Yes, this can be removed.

> 3. https://dxr.mozilla.org/comm-central/rev/72bc94bebbabfed990212cb8959c11fe5c974e05/suite/common/directory/directory.js#14
> const ARRAY_CONTRACTID          = "@mozilla.org/mutable-array;1";
> is the only occurence of "@mozilla.org/mutable-array;1" in whole of c-c/m-c. 
> Is it correct or should it be "@mozilla.org/array;1" ?
Right. Needs to be changed. Looks like a typo introduced in
https://hg.mozilla.org/comm-central/rev/2cb52566a6b7
That no one noticed.
Just a heads up, I'm landing the |nsISupportsArray| removal patch in m-c next week (March 6, 2017).
Thanks, yes I'm fixing this right now.

I now noticed point 5. is for m-c. Do you want to take it to  new bug?
Assignee: nobody → acelists
Status: NEW → ASSIGNED
(In reply to :aceman from comment #3)
> Thanks, yes I'm fixing this right now.
> 
> I now noticed point 5. is for m-c. Do you want to take it to  new bug?

Yeah that's fine, I can review and land it (or I can just take care of the patch).
Attached patch patch for TBSplinter Review
Attachment #8842652 - Flags: review?(jorgk)
Attached patch patch for SMSplinter Review
Attachment #8842653 - Flags: review?(philip.chee)
Attachment #8842653 - Flags: review?(iann_bugzilla)
Attached patch patch for m-cSplinter Review
Attachment #8842657 - Flags: review?(erahm)
Comment on attachment 8842657 [details] [diff] [review]
patch for m-c

Review of attachment 8842657 [details] [diff] [review]:
-----------------------------------------------------------------

r=me Thanks!
Attachment #8842657 - Flags: review?(erahm) → review+
Comment on attachment 8842653 [details] [diff] [review]
patch for SM

LGTM r=me thanks
Attachment #8842653 - Flags: review?(philip.chee)
Attachment #8842653 - Flags: review?(iann_bugzilla)
Attachment #8842653 - Flags: review+
Comment on attachment 8842652 [details] [diff] [review]
patch for TB

I haven't tested it since I assume you have.

I note that bug 1318806 is also in progress.
Attachment #8842652 - Flags: review?(jorgk) → review+
When running all xpcshell, it appears to me that the TestMailImporter.js functions are unused. That may be why the got away with the typos.
Keywords: checkin-needed
(In reply to :aceman from comment #11)
> That may be why the got away with the typos.
Which typos (plural)? *A*ppendElement() and what else?
The other part was creating a nsIArray from nsISupportsArray and then even adding elements to it as if it were a nsIMutableArray. The .appendElement() should not exist on nsIArray.
I love working on dead code because you know that it's going to work 100% ;-)
Yeah. Maybe it was used by some of the importers in the past that got removed. But I would leave it in for now in case we need it again the future.
https://hg.mozilla.org/comm-central/rev/04a1176052ddeab18e98a680dc90b55502c1eaf0
https://hg.mozilla.org/comm-central/rev/e32e7215e0e726eeef616940d4e57b3db70755e6

Dear M-C sheriff, could you please check-in "patch for m-c". It's a one-word comment change only, so really no testing required here. Thank you.
Target Milestone: --- → Thunderbird 54.0
Dear Sheriff, please fix the commit message, r=erahm (lose the ?). You might consider checking this in with DONTBUILD or combine with other patches.
Flags: needinfo?(cbook)
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/77d5a39a4677
Remove an outdated reference to nsISupportsArray in libeditor. r=erahm DONTBUILD a=tomcat
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Flags: needinfo?(cbook)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.