Closed Bug 821253 Opened 11 years ago Closed 11 years ago

Remove nsISupportsArray usage from nsIMsgFilter::getSortedActionList

Categories

(MailNews Core :: Filters, defect)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 20.0

People

(Reporter: aceman, Assigned: aceman)

References

()

Details

Attachments

(1 file, 3 obsolete files)

There are only a few callers:
http://mxr.mozilla.org/comm-central/ident?i=getSortedActionList

I understood removing nsISupportsArray is wanted. Can this argument or getSortedActionList be replaced by nsIArray or something else?
Blocks: 436089
At a quick glance, I would replace it with:

readonly attribute nsIArray sortedActionList;

and do the array creation inside that function.
Attached patch patch (obsolete) — Splinter Review
Attachment #692032 - Flags: review?(mbanner)
Status: NEW → ASSIGNED
Comment on attachment 692032 [details] [diff] [review]
patch

> 
>+
???

> 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.
nsMsgFilter::nsMsgFilter():
    m_temporary(false),
    m_unparseable(false),
    m_filterList(nullptr),
    m_actionList(do_CreateInstance(NS_ARRAY_CONTRACTID)),
    m_expressionTree(nullptr)

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

> NS_IMETHODIMP
> 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.)
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?
Flags: needinfo?(neil)
(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.)
Flags: needinfo?(neil)
Isn't "nsTArray<nsCOMPtr<nsIMsgRuleAction> > actionList" already the internal member as type-safe array ? So what are those 2 steps?
Flags: needinfo?(neil)
(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.
Flags: needinfo?(neil)
Blocks: 821743
Attached patch patch v2 (obsolete) — Splinter Review
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 .
Attachment #692032 - Attachment is obsolete: true
Attachment #692032 - Flags: review?(mbanner)
Attachment #692452 - Flags: review?(neil)
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)
Ditto.

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

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

>+#include "nsIArray.h"
> #include "nsIMutableArray.h"
Ditto.
Attachment #692452 - Flags: review?(neil) → review-
Attached patch patch v3 (obsolete) — Splinter Review
Attachment #692452 - Attachment is obsolete: true
Attachment #692600 - Flags: review?(neil)
Hmm, I can't seem to get this patch to apply, yet nsParseMailbox.cpp hasn't changed for several weeks...
Attached patch patch v4Splinter Review
That's strange because it applies fine for me. But I'll reupload it.
Attachment #692600 - Attachment is obsolete: true
Attachment #692600 - Flags: review?(neil)
Attachment #693507 - Flags: review?(neil)
Well, the patches have different size so something must have changed...
Attachment #693507 - Flags: review?(neil) → review+
Attachment #693507 - Flags: superreview?(kent)
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.
Attachment #693507 - Flags: superreview?(kent) → feedback+
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.
Keywords: checkin-needed
Blocks: 747284
https://hg.mozilla.org/comm-central/rev/6bf7b6fdb2c3
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 20.0
You need to log in before you can comment on or make changes to this bug.