If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

convert nsFileView to not use nsISupportsArray

RESOLVED FIXED in mozilla20

Status

()

Toolkit
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: froydnj, Unassigned)

Tracking

unspecified
mozilla20
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
Created attachment 662675 [details] [diff] [review]
patch

The patch compiles, at least.  Haven't pushed to try.
Attachment #662675 - Flags: review?(enndeakin)

Comment 1

5 years ago
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.
(Reporter)

Comment 2

5 years ago
(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.
(Reporter)

Comment 3

5 years ago
Not a telemetry bug, moving.  No Bugzilla component for the filepicker. :(
Component: Telemetry → General

Comment 4

5 years ago
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.
Attachment #662675 - Flags: review?(enndeakin) → review+
I don't see a problem with the swap usage here.  But a comment would definitely be nice!
(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
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/edea17a87998

Comment 8

5 years ago
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!)
(In reply to neil@parkwaycc.co.uk 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 :(
https://hg.mozilla.org/mozilla-central/rev/edea17a87998
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.