792566 - convert nsFileView to not use nsISupportsArray

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Nathan Froyd [:froydnj]]

Attachment: patch

The patch compiles, at least.  Haven't pushed to try.

Comment 1

[Neil Deakin]

Comment on attachment 662675 [details] [diff] [review]
patch

> protected:
>+  typedef nsTArray<nsCOMPtr<nsIFile> > FileList;

You can also just use nsCOMArray, without the typedef.

>-    nsCOMPtr<nsISupports> element = dont_AddRef(aArray->ElementAt(i));
>-    nsCOMPtr<nsISupports> element2 = dont_AddRef(aArray->ElementAt(count-i-1));
>-    aArray->ReplaceElementAt(element2, i);
>-    aArray->ReplaceElementAt(element, count-i-1);
>+    nsCOMPtr<nsIFile>& element = aArray[i];
>+    nsCOMPtr<nsIFile>& element2 = aArray[count - i - 1];
>+    element.swap(element2);

I'm not sure the swap here is doing what is expected. Maybe it is, I don't know, but it's confusing looking code. Better would to be just assign the other element to the array normally.

>-  for (i = 0; i < count; ++i) {
>-    aArray->ReplaceElementAt(array[i], i);
>-    NS_RELEASE(array[i]);
>+  for (uint32_t i = 0; i < count; ++i) {
>+    aArray[i].swap(array[i]);

Same here.

Comment 2

[Nathan Froyd [:froydnj]]

(In reply to Neil Deakin from comment #1)
> You can also just use nsCOMArray, without the typedef.

Mmm, good point.

> >-    nsCOMPtr<nsISupports> element = dont_AddRef(aArray->ElementAt(i));
> >-    nsCOMPtr<nsISupports> element2 = dont_AddRef(aArray->ElementAt(count-i-1));
> >-    aArray->ReplaceElementAt(element2, i);
> >-    aArray->ReplaceElementAt(element, count-i-1);
> >+    nsCOMPtr<nsIFile>& element = aArray[i];
> >+    nsCOMPtr<nsIFile>& element2 = aArray[count - i - 1];
> >+    element.swap(element2);
> 
> I'm not sure the swap here is doing what is expected. Maybe it is, I don't
> know, but it's confusing looking code. Better would to be just assign the
> other element to the array normally.

I thought it was nice to avoid the extraneous AddRef/Release bits that would be required with a more straightforward:

  nsCOMPtr<nsIFile> e1 = ...
  nsCOMPtr<nsIFile> e2 = ...
  aArray.Replace(..., e1);
  aArray.Replace(..., e2);

or similar.  Using .swap this way seems to be fairly well understood in other parts of the code.

> >-  for (i = 0; i < count; ++i) {
> >-    aArray->ReplaceElementAt(array[i], i);
> >-    NS_RELEASE(array[i]);
> >+  for (uint32_t i = 0; i < count; ++i) {
> >+    aArray[i].swap(array[i]);
> 
> Same here.

Ditto.

Comment 3

[Nathan Froyd [:froydnj]]

Not a telemetry bug, moving.  No Bugzilla component for the filepicker. :(

Comment 4

[Neil Deakin]

Comment on attachment 662675 [details] [diff] [review]
patch

OK, so the rest is fine, but you should ask someone more familiar with the comptr details, such as bsmedberg, to check for sure.

Comment 5

[(no longer active)]

I don't see a problem with the swap usage here.  But a comment would definitely be nice!

Comment 6

[Trevor Saunders (:tbsaunde)]

(In reply to Ehsan Akhgari [:ehsan] from comment #5)
> I don't see a problem with the swap usage here.  But a comment would
> definitely be nice!

me too, I was going to claim authority to r+ that bit, I won't bother now.

I'd prefer the Sort() method just called nsTarray::Sort() but I'm not sure I feel like bothering  to shave that yak

Comment 7

[Trevor Saunders (:tbsaunde)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/edea17a87998

Comment 8

[[e-mail]]

Comment on attachment 662675 [details] [diff] [review]
patch

>+  for (uint32_t i = 0; i < count; ++i) {
>+    array[i] = aArray[i];
>+  }
> 
>   NS_QuickSort(array, count, sizeof(nsIFile*), compareFunc, nullptr);
> 
>+  for (uint32_t i = 0; i < count; ++i) {
>+    aArray[i].swap(array[i]);
nsTArray does actually have a Sort method. (So does nsCOMArray, and its callback function is easier to use too!)

Comment 9

[Trevor Saunders (:tbsaunde)]

(In reply to [e-mail] from comment #8)
> Comment on attachment 662675 [details] [diff] [review]
> patch
> 
> >+  for (uint32_t i = 0; i < count; ++i) {
> >+    array[i] = aArray[i];
> >+  }
> > 
> >   NS_QuickSort(array, count, sizeof(nsIFile*), compareFunc, nullptr);
> > 
> >+  for (uint32_t i = 0; i < count; ++i) {
> >+    aArray[i].swap(array[i]);
> nsTArray does actually have a Sort method. (So does nsCOMArray, and its
> callback function is easier to use too!)

I filed bug 820672 to add a similar Sort() to nsTArray.

I new about the existing Sort() methods, but it didn't seem worth the effort.  On the other hand I have no idea why the array isn't just sorted in place like
nsQuickSort(array.Elements(), array.Length(), ...) and I have no excuse for not fixing that other than having been tired :(

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/edea17a87998