Closed
Bug 821253
Opened 11 years ago
Closed 11 years ago
Remove nsISupportsArray usage from nsIMsgFilter::getSortedActionList
Categories
(MailNews Core :: Filters, defect)
MailNews Core
Filters
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 20.0
People
(Reporter: aceman, Assigned: aceman)
References
()
Details
Attachments
(1 file, 3 obsolete files)
16.96 KB,
patch
|
neil
:
review+
rkent
:
feedback+
|
Details | Diff | Splinter Review |
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?
Comment 1•11 years ago
|
||
At a quick glance, I would replace it with: readonly attribute nsIArray sortedActionList; and do the array creation inside that function.
Attachment #692032 -
Flags: review?(mbanner)
Comment 3•11 years ago
|
||
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)
Comment 5•11 years ago
|
||
(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)
Comment 7•11 years ago
|
||
(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)
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 9•11 years ago
|
||
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-
Assignee | ||
Comment 10•11 years ago
|
||
Attachment #692452 -
Attachment is obsolete: true
Attachment #692600 -
Flags: review?(neil)
Comment 11•11 years ago
|
||
Hmm, I can't seem to get this patch to apply, yet nsParseMailbox.cpp hasn't changed for several weeks...
Assignee | ||
Comment 12•11 years ago
|
||
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)
Assignee | ||
Comment 13•11 years ago
|
||
Well, the patches have different size so something must have changed...
Updated•11 years ago
|
Attachment #693507 -
Flags: review?(neil) → review+
Attachment #693507 -
Flags: superreview?(kent)
Comment 14•11 years ago
|
||
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+
Assignee | ||
Comment 15•11 years ago
|
||
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
Comment 16•11 years ago
|
||
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.
Description
•