Last Comment Bug 838805 - remove some small occurrences of nsISupportsArray in mailnews
: remove some small occurrences of nsISupportsArray in mailnews
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: All All
: -- minor (vote)
: Thunderbird 23.0
Assigned To: :aceman
:
Mentors:
Depends on: 831993 833988
Blocks: 394167
  Show dependency treegraph
 
Reported: 2013-02-06 13:20 PST by :aceman
Modified: 2013-04-03 12:41 PDT (History)
0 users
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
WIP patch (14.24 KB, patch)
2013-02-06 13:23 PST, :aceman
neil: feedback+
Details | Diff | Review
patch v2 (14.16 KB, patch)
2013-02-07 15:42 PST, :aceman
no flags Details | Diff | Review
patch v3 (16.78 KB, patch)
2013-03-25 16:38 PDT, :aceman
neil: review-
Details | Diff | Review
patch v4 (18.72 KB, patch)
2013-03-26 15:19 PDT, :aceman
no flags Details | Diff | Review
patch v5 (18.71 KB, patch)
2013-03-27 12:38 PDT, :aceman
no flags Details | Diff | Review
patch v6 (19.57 KB, patch)
2013-03-29 16:21 PDT, :aceman
neil: review+
Details | Diff | Review
patch v7 (19.35 KB, patch)
2013-04-03 10:05 PDT, :aceman
acelists: review+
Details | Diff | Review

Description :aceman 2013-02-06 13:20:18 PST
Convert these to nsCOMArray (or something else):
mailnews/local/src/nsLocalMailFolder.cpp::mDownloadMessages
mailnews/base/src/nsMsgSearchDBView.cpp::m_hdrsForEachFolder

Remove now unneeded 'interface nsISupportsArray;' from nsIMsgFolder.idl.
Some files inherited the definitions without including nsISupportsArray.h so I add them where needed now.
Comment 1 :aceman 2013-02-06 13:23:29 PST
Created attachment 710940 [details] [diff] [review]
WIP patch

I am not sure what the nsISupportsArray.clone() usages were supposed to do and if the conversion to AppendElements is enough for them.
Comment 2 neil@parkwaycc.co.uk 2013-02-06 13:34:12 PST
Comment on attachment 710940 [details] [diff] [review]
WIP patch

>   newMsgDBView->m_folders.InsertObjectsAt(m_folders, 0);
m_folders is an nsCOMArray so you see how it works (although feel free to use AppendObjects/AppendElements instead).

>+  // ACE: is this still needed? There is no other use of m_copyListenerList...
I agree, it looks unused.
Comment 3 :aceman 2013-02-07 15:42:05 PST
Created attachment 711570 [details] [diff] [review]
patch v2
Comment 4 neil@parkwaycc.co.uk 2013-02-07 16:18:26 PST
Comment on attachment 711570 [details] [diff] [review]
patch v2

>   // now copy all of our private member data
>   newMsgDBView->mDestFolder = mDestFolder;
>   newMsgDBView->mCommand = mCommand;
>   newMsgDBView->mTotalIndices = mTotalIndices;
>   newMsgDBView->mCurIndex = mCurIndex; 
>   newMsgDBView->m_folders.InsertObjectsAt(m_folders, 0);
>   newMsgDBView->m_curCustomColumn = m_curCustomColumn;
> 
Nit: don't need this blank line any more.

>+  m_hdrsForEachFolder.InsertObjectsAt(newMsgDBView->m_hdrsForEachFolder, 0);
Still the wrong way around, compare m_folders above and m_uniqueFoldersSelected below!

> 
>   newMsgDBView->m_uniqueFoldersSelected.InsertObjectsAt(m_uniqueFoldersSelected, 0);
[The blank line here can probably go too.]

> nsMsgSearchDBView::GetFoldersAndHdrsForSelection(nsMsgViewIndex *indices, int32_t numIndices)
[Wow, this method is lame. It loops through all of the selected messages and establishes an array of unique folders. It then loops through the array again once for each folder, extracting those messages that are from that folder... After this lands, I might submit a rewrite.]
Comment 5 :aceman 2013-03-25 16:38:13 PDT
Created attachment 729285 [details] [diff] [review]
patch v3
Comment 6 neil@parkwaycc.co.uk 2013-03-26 04:09:54 PDT
Comment on attachment 729285 [details] [diff] [review]
patch v3

>-            nsCOMPtr<nsISupports> child;
>+            nsCOMPtr<nsIMsgFolder> child;
>             rv = enumerator->GetNext(getter_AddRefs(child));
I consider it a bug that this actually compiles :-( You need to GetNext into an nsISupports, then do_QueryInterface into the nsIMsgFolder.
Comment 7 :aceman 2013-03-26 15:19:47 PDT
Created attachment 729826 [details] [diff] [review]
patch v4
Comment 8 neil@parkwaycc.co.uk 2013-03-26 15:41:43 PDT
Comment on attachment 729826 [details] [diff] [review]
patch v4

>-            nsCOMPtr<nsISupports> child;
>-            rv = enumerator->GetNext(getter_AddRefs(child));
>+            nsCOMPtr<nsIMsgFolder> folder(do_QueryInterface(item, &rv));
>+            rv = enumerator->GetNext(getter_AddRefs(folder));
Didn't I say GetNext comes first, then do_QueryInterface? (And you can keep the existing child variable.)
Comment 9 :aceman 2013-03-27 00:24:03 PDT
Sorry, what a thinko :)

Why should I keep 'child'? I tried to reuse the existing variable 'item'.
Comment 10 :aceman 2013-03-27 12:38:34 PDT
Created attachment 730287 [details] [diff] [review]
patch v5
Comment 11 neil@parkwaycc.co.uk 2013-03-27 16:16:53 PDT
(In reply to aceman from comment #9)
> Why should I keep 'child'? I tried to reuse the existing variable 'item'.

Depends on your point of view; 'child' looks like it's the existing variable compared to the code before the patch, but 'item' is "existing" compared to the code after the original patch.
Comment 12 :aceman 2013-03-27 16:22:56 PDT
OK, I merged child and item and moved the new declaration above them so it can be shared. So what to do now?
Comment 13 :aceman 2013-03-29 16:21:59 PDT
Created attachment 731369 [details] [diff] [review]
patch v6
Comment 14 :aceman 2013-04-03 05:56:15 PDT
Neil?
Comment 15 neil@parkwaycc.co.uk 2013-04-03 07:00:55 PDT
Comment on attachment 731369 [details] [diff] [review]
patch v6

[Sorry for the delay; bugmail backlog.]

I'm not entirely convinced about merging those two variables, as one is child folders while the other is messages and it might confuse someone later on.

The hasNext merger isn't so bad, but IMHO you should have kept the short name.
Comment 16 :aceman 2013-04-03 07:08:13 PDT
(In reply to neil@parkwaycc.co.uk from comment #15)
> I'm not entirely convinced about merging those two variables, as one is
> child folders while the other is messages and it might confuse someone later
> on.

I merged it based on the variable type. Semantically, in this case of nsISupports, it is not a folder or a message (but a generic enumerator item) until you queryInterface it to one of those. And that object then has a separate variable with proper name.
Comment 17 :aceman 2013-04-03 10:05:35 PDT
Created attachment 732898 [details] [diff] [review]
patch v7

Fixed the hasMore variable name.
Comment 18 Ryan VanderMeulen [:RyanVM] 2013-04-03 12:41:52 PDT
https://hg.mozilla.org/comm-central/rev/fbc1cf677a5c

Note You need to log in before you can comment on or make changes to this bug.