Last Comment Bug 821253 - Remove nsISupportsArray usage from nsIMsgFilter::getSortedActionList
: Remove nsISupportsArray usage from nsIMsgFilter::getSortedActionList
Product: MailNews Core
Classification: Components
Component: Filters (show other bugs)
: Trunk
: All All
-- minor (vote)
: Thunderbird 20.0
Assigned To: :aceman
Depends on:
Blocks: 394167 436089 747284 821743
  Show dependency treegraph
Reported: 2012-12-13 03:49 PST by :aceman
Modified: 2012-12-23 06:17 PST (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

patch (23.02 KB, patch)
2012-12-13 14:45 PST, :aceman
no flags Details | Diff | Splinter Review
patch v2 (20.33 KB, patch)
2012-12-14 13:54 PST, :aceman
neil: review-
Details | Diff | Splinter Review
patch v3 (17.61 KB, patch)
2012-12-15 07:51 PST, :aceman
no flags Details | Diff | Splinter Review
patch v4 (16.96 KB, patch)
2012-12-18 10:57 PST, :aceman
neil: review+
rkent: feedback+
Details | Diff | Splinter Review

Description User image :aceman 2012-12-13 03:49:58 PST
There are only a few callers:

I understood removing nsISupportsArray is wanted. Can this argument or getSortedActionList be replaced by nsIArray or something else?
Comment 1 User image Mark Banner (:standard8) 2012-12-13 03:59:12 PST
At a quick glance, I would replace it with:

readonly attribute nsIArray sortedActionList;

and do the array creation inside that function.
Comment 2 User image :aceman 2012-12-13 14:45:00 PST
Created attachment 692032 [details] [diff] [review]
Comment 3 User image 2012-12-13 16:29:57 PST
Comment on attachment 692032 [details] [diff] [review]


> nsMsgFilter::nsMsgFilter():
>     m_temporary(false),
>     m_unparseable(false),
>     m_filterList(nullptr),
>     m_expressionTree(nullptr)
> {
>   NS_NewISupportsArray(getter_AddRefs(m_termList));
>-  NS_NewISupportsArray(getter_AddRefs(m_actionList));
>+  m_actionList = do_CreateInstance(NS_ARRAY_CONTRACTID);
It's possible to add this to the initialiser list i.e.

>+  nsCOMPtr<nsIMutableArray> orderedActions(do_CreateInstance(NS_ARRAY_CONTRACTID, &rv));
Nit: setting rv without checking it (you do this more than once).

> nsMsgFilter::GetActionAt(int32_t aIndex, nsIMsgRuleAction **aAction)
[Given that this exists, I wonder why we use filter.actions.queryElementAt instead of filter.getActionAt; if we go all the way and expose the length we can then switch to a type-safe internal array.]

>-    nsCOMPtr<nsISupportsArray> actionList;
>-    rv = NS_NewISupportsArray(getter_AddRefs(actionList));
>+    nsCOMPtr<nsIArray> actionList(do_CreateInstance(NS_ARRAY_CONTRACTID, &rv));
>     NS_ENSURE_SUCCESS(rv, rv);
>-    rv = m_curFilter->GetSortedActionList(actionList);
>+    rv = m_curFilter->GetSortedActionList(getter_AddRefs(actionList));
You don't need to create the sorted action list because the new method does it for you.
(Above issue happens multiple times.)
Comment 4 User image :aceman 2012-12-14 00:05:05 PST
Thanks Neil. Looks like I should really only do the sortedActionList in this bug.

I'll create a new bug for the change of actionList to nsTArray<nsCOMPtr<nsIMsgRuleAction> > as Neil suggests. Do I understand it correctly, that we would then add a actionsCount attribute, but remove the actionList attribute?
Comment 5 User image 2012-12-14 06:36:20 PST
(In reply to aceman from comment #4)
> I'll create a new bug for the change of actionList to
> nsTArray<nsCOMPtr<nsIMsgRuleAction> > as Neil suggests. Do I understand it
> correctly, that we would then add a actionsCount attribute, but remove the
> actionList attribute?
Yes, that would be the first step; the second step would be to update the internal member to a type-safe array. (I say two steps because you may find it helpful to split the work into two patches.)
Comment 6 User image :aceman 2012-12-14 07:58:02 PST
Isn't "nsTArray<nsCOMPtr<nsIMsgRuleAction> > actionList" already the internal member as type-safe array ? So what are those 2 steps?
Comment 7 User image 2012-12-14 08:10:20 PST
(In reply to :aceman from comment #6)
> Isn't "nsTArray<nsCOMPtr<nsIMsgRuleAction> > actionList" already the
> internal member as type-safe array ? So what are those 2 steps?

The first step would be to add the actionsCount attribute and remove the actionsList attribute. The internal member does not have to be changed to achieve this change. However once this is done there are no external restrictions affecting the type of the internal member, so it can now be turned into a type-safe array rather than a scriptable array.
Comment 8 User image :aceman 2012-12-14 13:54:53 PST
Created attachment 692452 [details] [diff] [review]
patch v2

The patch is reduced to only convert the sortedActionList. The other changes I do are in relation to that and try to merge all the different implementations of checks on the fetched action. So there are all the same checks on actionType, filterActionList->QueryElementAt, filterAction->GetTargetFolderUri .
Comment 9 User image 2012-12-14 16:57:46 PST
Comment on attachment 692452 [details] [diff] [review]
patch v2

>+      actionList->QueryElementAt(actionIndex, NS_GET_IID(nsIMsgRuleAction),
>+                                              getter_AddRefs(filterAction));
>+      if (NS_FAILED(rv) || !filterAction)
Test for rv without setting it first...

> #include "nsIMutableArray.h"
>+#include "nsIArray.h"
> #include "nsArrayUtils.h"
Nit: nsIMutableArray and nsArrayUtils both already include nsIArray.

>+    filterActionList->QueryElementAt(actionIndex, NS_GET_IID(nsIMsgRuleAction),
>+                                                  getter_AddRefs(filterAction));
>+    if (NS_FAILED(rv) || !filterAction)

> #include "nsIMutableArray.h"
> #include "nsArrayUtils.h"
>+#include "nsIArray.h"

>-  uint32_t newFlags;
This change causes more trouble than it's worth. Please put it back.

>+#include "nsIArray.h"
> #include "nsIMutableArray.h"
Comment 10 User image :aceman 2012-12-15 07:51:41 PST
Created attachment 692600 [details] [diff] [review]
patch v3
Comment 11 User image 2012-12-18 05:21:02 PST
Hmm, I can't seem to get this patch to apply, yet nsParseMailbox.cpp hasn't changed for several weeks...
Comment 12 User image :aceman 2012-12-18 10:57:01 PST
Created attachment 693507 [details] [diff] [review]
patch v4

That's strange because it applies fine for me. But I'll reupload it.
Comment 13 User image :aceman 2012-12-18 10:57:41 PST
Well, the patches have different size so something must have changed...
Comment 14 User image Kent James (:rkent) 2012-12-20 08:13:55 PST
Comment on attachment 693507 [details] [diff] [review]
patch v4

I changed your sr request to a simple feedback. I am not a qualified super reviewer, and I don't think that you need a review beyond Neil's for this.

I did look over the code to scan for issues, and I had no concerns. The only comment that I would make is that, personally, I would not be doing changes of this sort:

-        filterAction->GetTargetFolderUri(actionTargetFolderUri);
-        if (actionTargetFolderUri.IsEmpty())
+        rv = filterAction->GetTargetFolderUri(actionTargetFolderUri);
+        if (NS_FAILED(rv) || actionTargetFolderUri.IsEmpty())

though others may disagree with me. My own philosophy is that in the vast majority of cases, the purpose of these checks are to prevent failures from resulting in crashes or other kinds of undesirable behavior. The extra clutter of checking both the rv and the result is not worth the effort.

But that does not mean I object to this change.
Comment 15 User image :aceman 2012-12-20 23:43:45 PST
I marked you for superreview as according to guidelines a sr is needed for changes of interfaces and you are most familiar with the filters component (and you can also see addons whether they use these interfaces). But if it is not needed I am OK with that. Thanks.
Comment 16 User image Ryan VanderMeulen [:RyanVM] 2012-12-23 06:17:42 PST

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