Closed Bug 737724 Opened 13 years ago Closed 13 years ago

make IsDefunct() inline

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: tbsaunde, Assigned: capella)

References

Details

(Whiteboard: [good first bug][mentor=trev.saunders@gmail.com][lang=c++])

Attachments

(1 file, 12 obsolete files)

1. add a flag to mFlags in nsAccessible to tell if an accessible is defunct or not. 2. initialize the bit to alive in the constructor and make it defunct in Shutdown() 3. reimplement IsDefunct() as an inline function on nsAccessible that just checks the flag. Alex sounds good?
(In reply to Trevor Saunders (:tbsaunde) from comment #0) > 1. add a flag to mFlags in nsAccessible to tell if an accessible is defunct > or not. > 2. initialize the bit to alive in the constructor and make it defunct in > Shutdown() sounds good but it miss cycle collection thing which can make an accessible defunct but never calls Shutdown
(In reply to alexander :surkov from comment #1) > (In reply to Trevor Saunders (:tbsaunde) from comment #0) > > 1. add a flag to mFlags in nsAccessible to tell if an accessible is defunct > > or not. > > 2. initialize the bit to alive in the constructor and make it defunct in > > Shutdown() > > sounds good but it miss cycle collection thing which can make an accessible > defunct but never calls Shutdown well, but then it calls LastRelease() which calls Shutdown() right, so if Shutdown() stays virtual this should work.
(In reply to Trevor Saunders (:tbsaunde) from comment #2) > well, but then it calls LastRelease() which calls Shutdown() right, so if > Shutdown() stays virtual this should work. ok, sounds right
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attached patch Patch (v1) (obsolete) — Splinter Review
I thought this would be easy ... compiles, but every single mochitest-a11y test fails miserably ... reviewing from this end ...
Attachment #608311 - Flags: feedback?(trev.saunders)
there's a ton of IsDefunct implementations through a11y code
Attached patch Patch (v2) (obsolete) — Splinter Review
Well DUH... this one works gooder
Attachment #608311 - Attachment is obsolete: true
Attachment #608311 - Flags: feedback?(trev.saunders)
Attachment #608402 - Flags: review?(trev.saunders)
Comment on attachment 608402 [details] [diff] [review] Patch (v2) > printf("\n"); > } > #endif >+ mFlags &= ~eDefunctAccessible; |= > nsAccessible::~nsAccessible() > { >+ mFlags |= eDefunctAccessible; don't bother there's no point. >+ eTextLeafAccessible = 1 << 16, >+ eDefunctAccessible = 1 << 17 I'm not sure I like putting this in the same enum as the accessible type stuff Alex, thoughts? also, you still haven't removed all of the virtual implementations of IsDefunct()
Attachment #608402 - Flags: review?(trev.saunders) → review-
(In reply to Trevor Saunders (:tbsaunde) from comment #7) > > nsAccessible::~nsAccessible() > > { > >+ mFlags |= eDefunctAccessible; > > don't bother there's no point. right. Do that on Shutdown > >+ eTextLeafAccessible = 1 << 16, > >+ eDefunctAccessible = 1 << 17 > > I'm not sure I like putting this in the same enum as the accessible type > stuff Alex, thoughts? right, that's not a type. I think we need something like StateFlags with constants placed between ChildrenFlags and AccessibleTypes. It'd be really nice if we can find a way to not adjust constants manually when some flag set is changed.
Made all changes, then I found / removed "virtual bool IsDefunct() const;" definitions from (5) files: nsXULTreeGridAccessible.h, nsXULTreeAccessible.h, nsApplicationAccessible.h, nsAccessNode.h, and nsDocAccessible.h This is causing compiler errors ... and is where my knowledge of classes starts to get thin ...
Attached patch Patch (v3) (obsolete) — Splinter Review
See the patch (v3) and the below: c:/Users/Master/mozilla-central/accessible/src/msaa/nsAccessNodeWrap.cpp(218) : error C3861: 'IsDefunct': identifier not found c:/Users/Master/mozilla-central/accessible/src/msaa/nsAccessNodeWrap.cpp(265) : error C3861: 'IsDefunct': identifier not found c:/Users/Master/mozilla-central/accessible/src/msaa/nsAccessNodeWrap.cpp(296) : error C3861: 'IsDefunct': identifier not found c:/Users/Master/mozilla-central/accessible/src/msaa/nsAccessNodeWrap.cpp(338) : error C3861: 'IsDefunct': identifier not found c:/Users/Master/mozilla-central/accessible/src/msaa/nsAccessNodeWrap.cpp(373) : error C3861: 'IsDefunct': identifier not found c:/Users/Master/mozilla-central/accessible/src/msaa/nsAccessNodeWrap.cpp(445) : error C3861: 'IsDefunct': identifier not found c:/Users/Master/mozilla-central/accessible/src/msaa/nsAccessNodeWrap.cpp(458) : error C3861: 'IsDefunct': identifier not found c:/Users/Master/mozilla-central/accessible/src/msaa/nsAccessNodeWrap.cpp(471) : error C3861: 'IsDefunct': identifier not found c:/Users/Master/mozilla-central/accessible/src/msaa/nsAccessNodeWrap.cpp(484) : error C3861: 'IsDefunct': identifier not found c:/Users/Master/mozilla-central/accessible/src/msaa/nsAccessNodeWrap.cpp(497) : error C3861: 'IsDefunct': identifier not found c:/Users/Master/mozilla-central/accessible/src/msaa/nsAccessNodeWrap.cpp(514) : error C3861: 'IsDefunct': identifier not found
Attachment #608589 - Flags: feedback?(trev.saunders)
because you define IsDefunct on nsAccessible while you should do that on nsAccessNode. The fix getting tricky because mFlags is defined on nsAccessible
alternatively you could do mContent check instead of IsDefunct on nsAccessNodeWrap. But let's get Trevor check if it's ok.
Comment on attachment 608589 [details] [diff] [review] Patch (v3) >- virtual bool IsDefunct() const; you might want to try removing the definitions as well as the declarations... > /** >+ * Flags used to describe whether the accessible is in defunct state. >+ */ >+ enum StateFlags { >+ eIsDefunct = 1 << 17 // accessible is Defunct >+ }; please file a folow up to cleanup the way these enums work.
Attachment #608589 - Flags: feedback?(trev.saunders)
(In reply to Trevor Saunders (:tbsaunde) from comment #13) > Comment on attachment 608589 [details] [diff] [review] > Patch (v3) > > >- virtual bool IsDefunct() const; > > you might want to try removing the definitions as well as the declarations... > > > /** > >+ * Flags used to describe whether the accessible is in defunct state. > >+ */ > >+ enum StateFlags { > >+ eIsDefunct = 1 << 17 // accessible is Defunct do 1 << 2 (place it "after" ChildrenFlags), otherwise for any new Accessible Type you need to shift this one, that happens often enough
Attached patch Patch (v4) (obsolete) — Splinter Review
See Patch (v4) I tried removing the definitions as well as the declarations (comment 13) ... and changed the eIsDefunct = 1 << 17 to be: 1 << 2 (placed it "after" ChildrenFlags) as in (comment 14) Build still fails as in (comment 10) ... re-reading Alexs (Comments 11 and 12) ...
Attachment #608608 - Flags: feedback?(trev.saunders)
(In reply to alexander :surkov from comment #12) > alternatively you could do mContent check instead of IsDefunct on > nsAccessNodeWrap. But let's get Trevor check if it's ok. yeah, I think for ISimpleDOMNode stuff like that checking mContent probably makes more sense anyway.
(In reply to Mark Capella [:capella] from comment #15) > Created attachment 608608 [details] [diff] [review] > Patch (v4) > > See Patch (v4) > > I tried removing the definitions as well as the declarations (comment 13) > ... and changed the eIsDefunct = 1 << 17 to be: 1 << 2 (placed it "after" > ChildrenFlags) as in (comment 14) > > Build still fails as in (comment 10) ... re-reading Alexs (Comments 11 and > 12) ... yeah, you need to fix the issue of calling IsDefunct() in nsAccessNode stuff, it looks like I forgot about a couple cases in nsAccessNode too, for those cases it looks as if since they're accessor type things we should only call them on non-defunct accessibles and they're callers will need to make sure that's true, either by checking mContent or IsDefunt()
Attached patch Patch (v5) (obsolete) — Splinter Review
Ok, I'm posting the patch as I have it so far ... this one may be exceeding my c++ classes capabilities without a larger degree of hand holding ... I'm a little hampered by the fact that this is a technical / performance change making it harder for me to follow, as I can't tell when I've "fixed" something or am even getting close :)
Attachment #608608 - Attachment is obsolete: true
Attachment #608608 - Flags: feedback?(trev.saunders)
Attachment #608938 - Flags: feedback?(trev.saunders)
Attached patch Patch (v6) (obsolete) — Splinter Review
Finally got a clean build - all mochitest --a11y pass ok ...
Attachment #608402 - Attachment is obsolete: true
Attachment #608589 - Attachment is obsolete: true
Attachment #608938 - Attachment is obsolete: true
Attachment #608938 - Flags: feedback?(trev.saunders)
Attachment #608961 - Flags: feedback?(trev.saunders)
Comment on attachment 608961 [details] [diff] [review] Patch (v6) generally looks fine, I'll bet bet Alex has a clear mind for looking at details :)
Attachment #608961 - Flags: feedback?(trev.saunders) → feedback+
Comment on attachment 608961 [details] [diff] [review] Patch (v6) r=surkov
Attachment #608961 - Flags: review?(surkov.alexander)
Comment on attachment 608961 [details] [diff] [review] Patch (v6) Review of attachment 608961 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/src/base/nsAccessNode.cpp @@ +216,1 @@ > return nsnull; see comment below @@ +260,1 @@ > return; currently we don't support document accessible without mContent but we want do this one day. Maybe you could use mDoc as a check instead. ::: accessible/src/base/nsAccessible.cpp @@ +2522,2 @@ > // Invalidate the child count and pointers to other accessibles, also make > // sure none of its children point to this parent group comments into single comment @@ +2522,3 @@ > // Invalidate the child count and pointers to other accessibles, also make > // sure none of its children point to this parent > + mFlags |= eIsDefunct; new line pls ::: accessible/src/base/nsAccessible.h @@ +636,5 @@ > */ > static void TranslateString(const nsAString& aKey, nsAString& aStringOut); > > + /** > + * Return true/false if the accessible is defunct. Return true if @@ +638,5 @@ > > + /** > + * Return true/false if the accessible is defunct. > + */ > + inline bool IsDefunct() const { return mFlags & eIsDefunct; } you don't need inline keyword @@ +684,5 @@ > inline void SetChildrenFlag(ChildrenFlags aFlag) > { mFlags = (mFlags & ~kChildrenFlagsMask) | aFlag; } > > /** > + * Flags used to describe whether the accessible is in defunct state. maybe: State flags of the accessible? @note keep these flags in sync with ChildrenFlags @@ +687,5 @@ > /** > + * Flags used to describe whether the accessible is in defunct state. > + */ > + enum StateFlags { > + eIsDefunct = 1 << 2 // accessible is Defunct if you want to keep this comment then please lowercase 'Defunct' @@ +695,2 @@ > * Flags describing the accessible itself. > * @note keep these flags in sync with ChildrenFlags and StateFlags ::: accessible/src/base/nsApplicationAccessible.cpp @@ -312,5 @@ > bool > -nsApplicationAccessible::IsDefunct() const > -{ > - return nsAccessibilityService::IsShutdown(); > -} thinking aloud: that's equivalent to IsShutdown check since when accessibility gets shutdown via nsAccessibilityService then it destroys application accessible (and thus appacc shutdown is called) ::: accessible/src/msaa/nsAccessNodeWrap.cpp @@ +219,3 @@ > return E_FAIL; > > nsCOMPtr<nsIDOMNode> DOMNode(do_QueryInterface(GetNode())); it sounds like you need to use result of GetNode() as defunct check @@ +446,3 @@ > return E_FAIL; > > *aNode = MakeAccessNode(GetNode()->GetNodeParent()); same here @@ +459,1 @@ > return E_FAIL; same here @@ +472,1 @@ > return E_FAIL; same @@ +485,1 @@ > return E_FAIL; same @@ +498,1 @@ > return E_FAIL; same @@ +515,1 @@ > return E_FAIL; same ::: accessible/src/xul/nsXULTreeAccessible.cpp @@ -171,5 @@ > > -bool > -nsXULTreeAccessible::IsDefunct() const > -{ > - return nsAccessibleWrap::IsDefunct() || !mTree || !mTreeView; that's not equivalent, mTreeView can go away without accessible Shutdown(), thus this change should result in crashes the same for other IsDefunct impls.
Attachment #608961 - Flags: review?(surkov.alexander) → review-
> @@ +638,5 @@ > > > > + /** > > + * Return true/false if the accessible is defunct. > > + */ > > + inline bool IsDefunct() const { return mFlags & eIsDefunct; } > > you don't need inline keyword I thought you usually prefered it? (I don't care either way, but we should be consistant) > > -nsApplicationAccessible::IsDefunct() const > > -{ > > - return nsAccessibilityService::IsShutdown(); > > -} > > thinking aloud: that's equivalent to IsShutdown check since when > accessibility gets shutdown via nsAccessibilityService then it destroys > application accessible (and thus appacc shutdown is called) yeah, that makes sense, tehre is actually a little bit of a problem here in that nsApplicationAccessible doesn't call its parent class shutdown. I'm sort of tempted to change that, but haven't thought it through yet. > > -nsXULTreeAccessible::IsDefunct() const > > -{ > > - return nsAccessibleWrap::IsDefunct() || !mTree || !mTreeView; > > that's not equivalent, mTreeView can go away without accessible Shutdown(), > thus this change should result in crashes > the same for other IsDefunct impls. which others in particular, and where does it go away other thn in a Shutdown()? I can't find a spot off hand.
> > that's not equivalent, mTreeView can go away without accessible Shutdown(), > > thus this change should result in crashes > > the same for other IsDefunct impls. > > which others in particular, and where does it go away other thn in a > Shutdown()? I can't find a spot off hand. I meant all XUL tree related
Attached patch Patch (v7) (obsolete) — Splinter Review
This is the most current version of the patch with issues addressed as I understand them. I'm waiting for more information regarding what is outstanding / under discussion in previous comment#24 ...
Attachment #608961 - Attachment is obsolete: true
Attachment #609607 - Flags: feedback?(trev.saunders)
Comment on attachment 609607 [details] [diff] [review] Patch (v7) > nsPresContext* nsAccessNode::GetPresContext() > { >- if (IsDefunct()) >+ if (!mContent) > return nsnull; I'd probably remove these checks from here to be consistant with other getters of properties and make sure that this object isn't defunct or has an mContent in the caller as makes sense. alternatively you could use mDoc here since that's what we actually use as I believe Alex suggested. > nsAccessNode::ScrollTo(PRUint32 aScrollType) > { >- if (IsDefunct()) >+ if (!mContent) > return; same > nsAccessNode::Language(nsAString& aLanguage) > { > aLanguage.Truncate(); > >- if (IsDefunct()) >+ if (!mContent) same >- // Invalidate the child count and pointers to other accessibles, also make >- // sure none of its children point to this parent >+ // Mark the accessible as defunct, invalidate the child, remove all children not as descriptive as the old comment. > /** >- * Flags describing the accessible itself. >+ * Flags used to describe state flags of the accessible how about flags used to describe the state of the accessible? >+ * Flags describing the accessible itself. I believe Alex wanted something else here? >- if (IsDefunct()) >+ if (!GetNode()) > return E_FAIL; use a local variable and save moultiple calls to GetNode? > STDMETHODIMP nsAccessNodeWrap::get_attributesForNames( > /* [in] */ unsigned short aNumAttribs, > /* [length_is][size_is][in] */ BSTR __RPC_FAR *aAttribNames, > /* [length_is][size_is][in] */ short __RPC_FAR *aNameSpaceID, > /* [length_is][size_is][retval] */ BSTR __RPC_FAR *aAttribValues) > { > __try { >- if (IsDefunct() || !IsElement()) >+ if (!mContent || !IsElement()) kind of crazy to check mContent and then that GetNode() returns an element. >- if (IsDefunct() || IsDocumentNode()) >+ if (!mContent || IsDocumentNode()) > return E_FAIL; same >- if (IsDefunct() || IsDocumentNode()) >+ if (!mContent || IsDocumentNode()) > return E_FAIL; same > > nsCOMPtr<nsIDOMCSSStyleDeclaration> cssDecl = > nsWinUtils::GetComputedStyleDeclaration(mContent); > NS_ENSURE_TRUE(cssDecl, E_FAIL); > > PRUint32 index; > for (index = 0; index < aNumStyleProperties; index ++) { >@@ -437,86 +437,86 @@ nsAccessNodeWrap::MakeAccessNode(nsINode > > return iNode; > } > > > STDMETHODIMP nsAccessNodeWrap::get_parentNode(ISimpleDOMNode __RPC_FAR *__RPC_FAR *aNode) > { > __try { >- if (IsDefunct()) >+ if (!GetNode()) > return E_FAIL; > > *aNode = MakeAccessNode(GetNode()->GetNodeParent()); use a loca var? > __try { >- if (IsDefunct()) >+ if (!GetNode()) > return E_FAIL; same >-nsXULTreeAccessible::IsDefunct() const >-{ >- return nsAccessibleWrap::IsDefunct() || !mTree || !mTreeView; >-} plese add null checks per irc.
Attachment #609607 - Flags: feedback?(trev.saunders)
FYI per irc: "you should add null checks into every nsXULTreeAccessbile method that deals with mTreeView "
Attached patch Patch (v8) (obsolete) — Splinter Review
I think this patch catches all issues up to but not yet including null checks ...
Attachment #609607 - Attachment is obsolete: true
Attachment #609694 - Flags: feedback?(trev.saunders)
Comment on attachment 609694 [details] [diff] [review] Patch (v8) > nsAccessNode::ScrollTo(PRUint32 aScrollType) > { >- if (IsDefunct()) >+ if (!mDoc) > return; that's fine, but eventually we should stop checking this here to be consistant with other methods on accessibles which expect the platform layer ensures the accessible is in a good state. we can either file a follow up for these or you can add IsDefunct checks in nsAccessible::ScrollTo and nsAccessibleWrap::ScrollTo and check mContent in nsAccessNodeWrap::scrollTo > nsAccessNode::Language(nsAString& aLanguage) > { > aLanguage.Truncate(); > >- if (IsDefunct()) >+ if (!mDoc) > return; same accept where you'd need to check changes a little check mContnet in nsAccessNodeWrap::get_Language and check IsDefunct in nsAccessibleWrap::get_Locale and nsAccessible::GetLanguage >+ /** >+ * It is used in nsAccessibleNodeWrap for performance. >+ * implementation. >+ */ >+ nsINode* gNode; nah, I meant local variable to each function. the little bit of ISimpleDOMNode perf this gets isn't worth the increase in size of every accessible. otherwise looks good, thanks!
Attachment #609694 - Flags: feedback?(trev.saunders)
Attached patch Patch (v9) (obsolete) — Splinter Review
This patch includes mTreeView null checks, and GetNode() local vars where appropriate to functions to avoid multiple calls. I plan to file a followup bug "to be consistent with other methods on accessibles which expect the platform layer ensures the accessible is in a good state".
Attachment #609694 - Attachment is obsolete: true
Attachment #609881 - Flags: feedback?(trev.saunders)
Comment on attachment 609881 [details] [diff] [review] Patch (v9) >+ * Flags used to describe state flags of the accessible. just "Flags used to describe the state of this accessible." >+ * Flags describing the accessible itself. "flags describing the type of this accessible." >@@ -257,17 +258,17 @@ STDMETHODIMP nsAccessNodeWrap::get_attri > /* [length_is][size_is][out] */ BSTR __RPC_FAR *aAttribNames, > /* [length_is][size_is][out] */ short __RPC_FAR *aNameSpaceIDs, > /* [length_is][size_is][out] */ BSTR __RPC_FAR *aAttribValues, > /* [out] */ unsigned short __RPC_FAR *aNumAttribs) > { > __try{ > *aNumAttribs = 0; > >- if (IsDefunct() || IsDocumentNode()) >+ if (!GetNode() || IsDocumentNode()) in this case it should be equivelent, but just use mContent. > return E_FAIL; > > PRUint32 numAttribs = mContent->GetAttrCount(); > if (numAttribs > aMaxAttribs) > numAttribs = aMaxAttribs; > *aNumAttribs = static_cast<unsigned short>(numAttribs); > > for (PRUint32 index = 0; index < numAttribs; index++) { >@@ -288,17 +289,17 @@ __try{ > > STDMETHODIMP nsAccessNodeWrap::get_attributesForNames( > /* [in] */ unsigned short aNumAttribs, > /* [length_is][size_is][in] */ BSTR __RPC_FAR *aAttribNames, > /* [length_is][size_is][in] */ short __RPC_FAR *aNameSpaceID, > /* [length_is][size_is][retval] */ BSTR __RPC_FAR *aAttribValues) > { > __try { >- if (IsDefunct() || !IsElement()) >+ if (!GetNode() || !IsElement()) same > return E_FAIL; > > nsCOMPtr<nsIDOMElement> domElement(do_QueryInterface(mContent)); > nsCOMPtr<nsINameSpaceManager> nameSpaceManager = > do_GetService(NS_NAMESPACEMANAGER_CONTRACTID); > > PRInt32 index; > >@@ -330,17 +331,17 @@ STDMETHODIMP nsAccessNodeWrap::get_compu > /* [in] */ boolean aUseAlternateView, > /* [length_is][size_is][out] */ BSTR __RPC_FAR *aStyleProperties, > /* [length_is][size_is][out] */ BSTR __RPC_FAR *aStyleValues, > /* [out] */ unsigned short __RPC_FAR *aNumStyleProperties) > { > __try{ > *aNumStyleProperties = 0; > >- if (IsDefunct() || IsDocumentNode()) >+ if (!GetNode() || IsDocumentNode()) same > > STDMETHODIMP nsAccessNodeWrap::get_computedStyleForProperties( > /* [in] */ unsigned short aNumStyleProperties, > /* [in] */ boolean aUseAlternateView, > /* [length_is][size_is][in] */ BSTR __RPC_FAR *aStyleProperties, > /* [length_is][size_is][out] */ BSTR __RPC_FAR *aStyleValues) > { > __try { >- if (IsDefunct() || IsDocumentNode()) >+ if (!GetNode() || IsDocumentNode()) same >-//////////////////////////////////////////////////////////////////////////////// >-// nsXULTreeItemAccessibleBase: nsAccessNode implementation leave it, Shutdown() is part of nsAccessNode other than those nits the idea is still good :) I hope surkov has a clear mind todya to look at details because I don't
Attachment #609881 - Flags: feedback?(trev.saunders) → feedback+
Attached patch Patch (v10) (obsolete) — Splinter Review
Attachment #609990 - Flags: review?(surkov.alexander)
(In reply to Trevor Saunders (:tbsaunde) from comment #23) > > > -nsApplicationAccessible::IsDefunct() const > > > -{ > > > - return nsAccessibilityService::IsShutdown(); > > > -} > > > > thinking aloud: that's equivalent to IsShutdown check since when > > accessibility gets shutdown via nsAccessibilityService then it destroys > > application accessible (and thus appacc shutdown is called) > > yeah, that makes sense, tehre is actually a little bit of a problem here in > that nsApplicationAccessible doesn't call its parent class shutdown. I'm > sort of tempted to change that, but haven't thought it through yet. if there's anything to shutdown in the base class of application accessible
Comment on attachment 609990 [details] [diff] [review] Patch (v10) Review of attachment 609990 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/src/base/nsAccessNode.cpp @@ +211,5 @@ > > // nsAccessNode protected > nsPresContext* nsAccessNode::GetPresContext() > { > + if (!mDoc) !mDoc is equivalent to !mContent (what you did in previous iterations) but I don't mind, all this code makes me think that one day we should remove nsAccessNode ::: accessible/src/base/nsAccessible.cpp @@ +2517,5 @@ > > void > nsAccessible::Shutdown() > { > + // Mark the accessible as defunct, Invalidate the child count and pointers to I -> i in 'Invalidate' ::: accessible/src/msaa/nsAccessNodeWrap.cpp @@ +262,5 @@ > { > __try{ > *aNumAttribs = 0; > > + if (!mContent) you missed IsDocumentNode() check. Document accessible has not null mContent. @@ +293,5 @@ > /* [length_is][size_is][in] */ short __RPC_FAR *aNameSpaceID, > /* [length_is][size_is][retval] */ BSTR __RPC_FAR *aAttribValues) > { > __try { > + if (!mContent) similar issue @@ +335,5 @@ > { > __try{ > *aNumStyleProperties = 0; > > + if (!mContent) same @@ +370,5 @@ > /* [length_is][size_is][in] */ BSTR __RPC_FAR *aStyleProperties, > /* [length_is][size_is][out] */ BSTR __RPC_FAR *aStyleValues) > { > __try { > + if (!mContent) same ::: accessible/src/xul/nsXULTreeAccessible.cpp @@ +477,5 @@ > { > // tree's children count is row count + treecols count. > PRInt32 childCount = nsAccessible::GetChildCount(); > if (childCount == -1) > return -1; i prefer if (childCount == -1 || !mTreeView) return childCount; @@ +620,5 @@ > // We dealt with removed tree items already however we may keep tree items > // having row indexes greater than row count. We should remove these dead tree > // items silently from caches. > + if (!mTreeView) > + return; not equivalent to existing code, when no mTreeView you should assume that newRowCount = 0 and continue I don't think this case is possible at all though. So maybe I'd go with no check here @@ +654,5 @@ > > nsresult rv; > if (endRow == -1) { > + if (!mTreeView) > + return; that's not correct as well: we receive notification that rows were changed but null-mTreeView claims no rows at all. That means something goes wrong and therefore we need to make sure we don't have created accessibles for absent tree rows. Probably I'd go with no null check at all since I don't think this case is possible. @@ +824,5 @@ > > NS_IMETHODIMP > nsXULTreeItemAccessibleBase::SetSelected(bool aSelect) > { > + if (IsDefunct() || !mTreeView) thinking aloud: all nsXULTreeItemAccessible checks gets redundant when bug 739524 is fixed @@ +861,5 @@ > if (aType != nsIAccessibleRelation::RELATION_NODE_CHILD_OF) > return Relation(); > > + if (!mTreeView) > + return Relation(); I'd say to return early before any relation type checks @@ +863,5 @@ > > + if (!mTreeView) > + return Relation(); > + > + PRInt32 parentIndex; while you are here pls initialize it by -1 @@ +895,5 @@ > } > > if (aIndex == eAction_Expand && IsExpandable()) { > + if (!mTreeView) > + return NS_ERROR_FAILURE; you added mTreeView check into IsExpandable already @@ +1013,5 @@ > // focusable and selectable states > PRUint64 state = states::FOCUSABLE | states::SELECTABLE; > > // expanded/collapsed state > + if (IsExpandable() && mTreeView) { you should return defunct state early in the beginning of method like if (!mTreeView) return states::DEFUNCT; ::: accessible/src/xul/nsXULTreeGridAccessible.cpp @@ -1120,1 @@ > no any single mTreeView check in this file
Attachment #609990 - Flags: review?(surkov.alexander) → review-
Attached patch Patch (v11) (obsolete) — Splinter Review
Attachment #609881 - Attachment is obsolete: true
Attachment #609990 - Attachment is obsolete: true
Attachment #610048 - Flags: review?(surkov.alexander)
Comment on attachment 610048 [details] [diff] [review] Patch (v11) Review of attachment 610048 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/src/xul/nsXULTreeGridAccessible.cpp @@ +325,1 @@ > return NS_ERROR_FAILURE; technically no tree view means there's no rows and thus there's no selected rows and it's ok actually (no failure) but you just preserved the logic so it's up to you to fix these GetSelected methods or not. @@ +547,5 @@ > NS_IMETHODIMP > nsXULTreeGridAccessible::SelectRow(PRInt32 aRowIndex) > { > + if (!mTreeView) > + return NS_ERROR_FAILURE; but here it means invalid_argument since no rows, again up to you @@ +566,5 @@ > NS_IMETHODIMP > nsXULTreeGridAccessible::UnselectRow(PRInt32 aRowIndex) > { > + if (!mTreeView) > + return NS_ERROR_FAILURE; same here
Attachment #610048 - Flags: review?(surkov.alexander) → review+
Attached patch Patch (v12) (obsolete) — Splinter Review
Comment on attachment 610060 [details] [diff] [review] Patch (v12) Review of attachment 610060 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/src/xul/nsXULTreeGridAccessible.cpp @@ +111,1 @@ > return NS_ERROR_FAILURE; ok, since you decided to fix return values then let's run through. !mTreeView means no rows, NS_OK @@ +325,1 @@ > return NS_ERROR_FAILURE; !mTreeView - NS_OK @@ +531,1 @@ > return NS_ERROR_FAILURE; !mTreeView - invalid arg since no row at the given index
Attached patch Patch (v13)Splinter Review
Attachment #610060 - Attachment is obsolete: true
Attachment #610048 - Attachment is obsolete: true
Comment on attachment 610079 [details] [diff] [review] Patch (v13) Review of attachment 610079 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/src/xul/nsXULTreeGridAccessible.cpp @@ +107,5 @@ > NS_ENSURE_ARG_POINTER(arowCount); > *arowCount = nsnull; > > + if (IsDefunct() || !mTreeView) > + return NS_OK; sorry to say this but IsDefunct is still failure :) I'll fix it before landing @@ +321,5 @@ > NS_ENSURE_ARG_POINTER(aRows); > *aRows = nsnull; > > + if (IsDefunct() || !mTreeView) > + return NS_OK; same here @@ +527,5 @@ > NS_ENSURE_ARG_POINTER(aIsSelected); > *aIsSelected = false; > > + if (IsDefunct() || !mTreeView) > + return NS_ERROR_INVALID_ARG; and here
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
Depends on: 740958
Depends on: 741053
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: