not bottleneck, just doing weird things (about .5% perf lost)

not super nice but might be a good step on states decomposition InteractiteState is introduced which is unavailable, focusable and selectable states for now. selectable state dependency on unavailable is not consistent through the code but in general it seems we don't expose selectable if the accessible is disabled. I added more consistence on this way we had. However taking into account editable state story then we might want later to exclude this selectable state dependency.

Comment on attachment 628393 [details] [diff] [review] patch >+Accessible::IsNativeUnavailable() const name seems kind of funny, my first thought is "is there's no native people around???" ;) maybe nativelyUnAvailable() or just NativelyAvailable() return true or false in the way that makes sense for each. >+ * Return native interactice state (unavailable, focusable or selectable). >+ */ >+ virtual PRUint64 NativeInteractiveState() const; >+ >+ /** > * Return native link states present on the accessible. > */ > virtual PRUint64 NativeLinkState() const; do you want to keep the relative order of these consistant? it doesn't look like you do that now. >+ state |= states::FOCUSABLE; // keep in sync with NativeIteractiveState() impl We should probably be able to replace this with NativeInteractiveState() call once we can trust compilers to devirtualize well and use final, or lto. >+nsXFormsAccessible::IsNativeUnavailable() const >+{ >+ nsCOMPtr<nsIDOMNode> DOMNode(do_QueryInterface(mContent)); >+ >+ bool isRelevant = false; >+ nsresult rv = sXFormsService->IsRelevant(DOMNode, &isRelevant); >+ NS_ENSURE_SUCCESS(rv, 0); returning 0 as a bool is kind of funny, and what happens when it doesn't fail? >+ if (IsNativeUnavailable()) { >+ // Note: keep in sinc with nsXULPopupManager::IsValidMenuItem() logic. >+ bool skipNavigatingDisabledMenuItem = true; >+ nsMenuFrame* menuFrame = do_QueryFrame(GetFrame()); >+ if (!menuFrame->IsOnMenuBar()) { is the reason you can't use the nsXULPopupManager method directly that you don't know if its on a menubar or popup? >+ skipNavigatingDisabledMenuItem = LookAndFeel:: >+ GetInt(LookAndFeel::eIntID_SkipNavigatingDisabledMenuItem, 0) != 0; > } >+ >+ if (skipNavigatingDisabledMenuItem) >+ return states::UNAVAILABLE; >+ >+ return states::UNAVAILABLE | states::FOCUSABLE | states::SELECTABLE; you return unavailable always? >- nsCOMPtr<nsIContent> mSliderNode; >+ mutable nsCOMPtr<nsIContent> mSliderNode; cute, lets hope compilers choose reasonable behavior for this case. > nsXULTreeItemAccessibleBase::NativeState() > { > if (!mTreeView) > return states::DEFUNCT; btw, shouldn't this go away? while having a tree with no view makes sense it doesn't really make sense that tree has rows or cells does it? >--- a/accessible/tests/mochitest/states/test_controls.xul >+++ b/accessible/tests/mochitest/states/test_controls.xul >@@ -10,31 +10,79 @@ add links to tests?

(In reply to Trevor Saunders (:tbsaunde) from comment #2) > Comment on attachment 628393 [details] [diff] [review] > patch > > >+Accessible::IsNativeUnavailable() const > > name seems kind of funny, my first thought is "is there's no native people > around???" ;) > > maybe nativelyUnAvailable() or just NativelyAvailable() return true or false > in the way that makes sense for each. I think I'm fine with NativelyUnavailable() > do you want to keep the relative order of these consistant? it doesn't look > like you do that now. I think I kept them in alphabetical order. Do you like to keep nativeUnavailabe and nativeInteractive states together? > >+ state |= states::FOCUSABLE; // keep in sync with NativeIteractiveState() impl > > We should probably be able to replace this with NativeInteractiveState() > call once we can trust compilers to devirtualize well and use final, or lto. can you look at this please? > >+nsXFormsAccessible::IsNativeUnavailable() const > >+{ > > returning 0 as a bool is kind of funny, and what happens when it doesn't > fail? yep, funny :) > >+ if (IsNativeUnavailable()) { > >+ // Note: keep in sinc with nsXULPopupManager::IsValidMenuItem() logic. > >+ bool skipNavigatingDisabledMenuItem = true; > >+ nsMenuFrame* menuFrame = do_QueryFrame(GetFrame()); > >+ if (!menuFrame->IsOnMenuBar()) { > > is the reason you can't use the nsXULPopupManager method directly that you > don't know if its on a menubar or popup? I didn't see nothing ready for us but I agree we should have code shared (I'll file a bug on this?) > >+ skipNavigatingDisabledMenuItem = LookAndFeel:: > >+ GetInt(LookAndFeel::eIntID_SkipNavigatingDisabledMenuItem, 0) != 0; > > } > >+ > >+ if (skipNavigatingDisabledMenuItem) > >+ return states::UNAVAILABLE; > >+ > >+ return states::UNAVAILABLE | states::FOCUSABLE | states::SELECTABLE; > > you return unavailable always? yes when it's unavailable :) this code is under IsNativeUnavailable() condition > >- nsCOMPtr<nsIContent> mSliderNode; > >+ mutable nsCOMPtr<nsIContent> mSliderNode; > > cute, lets hope compilers choose reasonable behavior for this case. do you know something about backgrounds? > > nsXULTreeItemAccessibleBase::NativeState() > > { > > if (!mTreeView) > > return states::DEFUNCT; > > btw, shouldn't this go away? while having a tree with no view makes sense it > doesn't really make sense that tree has rows or cells does it? yes technically IsDefunct should work here. I'll file a bug to be on safe side? > add links to tests? HTML:a?

(In reply to alexander :surkov from comment #3) > (In reply to Trevor Saunders (:tbsaunde) from comment #2) > > Comment on attachment 628393 [details] [diff] [review] > > patch > > > > >+Accessible::IsNativeUnavailable() const > > > > name seems kind of funny, my first thought is "is there's no native people > > around???" ;) > > > > maybe nativelyUnAvailable() or just NativelyAvailable() return true or false > > in the way that makes sense for each. > > I think I'm fine with NativelyUnavailable() ok > > do you want to keep the relative order of these consistant? it doesn't look > > like you do that now. > > I think I kept them in alphabetical order. Do you like to keep > nativeUnavailabe and nativeInteractive states together? doesn't really matter to me. > > >+ state |= states::FOCUSABLE; // keep in sync with NativeIteractiveState() impl > > > > We should probably be able to replace this with NativeInteractiveState() > > call once we can trust compilers to devirtualize well and use final, or lto. > > can you look at this please? not sure what you want me to do, I'm pretty sure we can't do that now, since we only do lto on windows, and I'm sure the mac gcc doesn't support devirtualization ever, or support final. > > >+nsXFormsAccessible::IsNativeUnavailable() const > > >+{ > > > > returning 0 as a bool is kind of funny, and what happens when it doesn't > > fail? > > yep, funny :) I assume you'll fix it? > > >+ if (IsNativeUnavailable()) { > > >+ // Note: keep in sinc with nsXULPopupManager::IsValidMenuItem() logic. > > >+ bool skipNavigatingDisabledMenuItem = true; > > >+ nsMenuFrame* menuFrame = do_QueryFrame(GetFrame()); > > >+ if (!menuFrame->IsOnMenuBar()) { > > > > is the reason you can't use the nsXULPopupManager method directly that you > > don't know if its on a menubar or popup? > > I didn't see nothing ready for us but I agree we should have code shared > (I'll file a bug on this?) sure > > >- nsCOMPtr<nsIContent> mSliderNode; > > >+ mutable nsCOMPtr<nsIContent> mSliderNode; > > > > cute, lets hope compilers choose reasonable behavior for this case. > > do you know something about backgrounds? not sure I follow > > > nsXULTreeItemAccessibleBase::NativeState() > > > { > > > if (!mTreeView) > > > return states::DEFUNCT; > > > > btw, shouldn't this go away? while having a tree with no view makes sense it > > doesn't really make sense that tree has rows or cells does it? > > yes technically IsDefunct should work here. I'll file a bug to be on safe > side? sure > > add links to tests? > > HTML:a? I meant links to the bug

(In reply to Trevor Saunders (:tbsaunde) from comment #4) > > can you look at this please? > > not sure what you want me to do, I'm pretty sure we can't do that now, > since we only do lto on windows, and I'm sure the mac gcc doesn't support > devirtualization ever, or support final. got it > > > returning 0 as a bool is kind of funny, and what happens when it doesn't > > > fail? > > > > yep, funny :) > > I assume you'll fix it? sure :) > > > >- nsCOMPtr<nsIContent> mSliderNode; > > > >+ mutable nsCOMPtr<nsIContent> mSliderNode; > > > > > > cute, lets hope compilers choose reasonable behavior for this case. > > > > do you know something about backgrounds? > > not sure I follow I meant reasonable vs not reasonable behavior if you know what bad compiler can do. > > > add links to tests? > > > > HTML:a? > > I meant links to the bug ok :)

> > > > >- nsCOMPtr<nsIContent> mSliderNode; > > > > >+ mutable nsCOMPtr<nsIContent> mSliderNode; > > > > > > > > cute, lets hope compilers choose reasonable behavior for this case. > > > > > > do you know something about backgrounds? > > > > not sure I follow > > I meant reasonable vs not reasonable behavior if you know what bad compiler > can do. well, my quick read of the spec is that ever making use of mutable is undefined behavior. However I think we're fine here, especially since all this code is single threaded. I suppose we could get really unlucky and compute the slider node a second time, but that's it.

(In reply to Trevor Saunders (:tbsaunde) from comment #4) > > > >+ if (IsNativeUnavailable()) { > > > >+ // Note: keep in sinc with nsXULPopupManager::IsValidMenuItem() logic. > > > >+ bool skipNavigatingDisabledMenuItem = true; > > > >+ nsMenuFrame* menuFrame = do_QueryFrame(GetFrame()); > > > >+ if (!menuFrame->IsOnMenuBar()) { > > > > > > is the reason you can't use the nsXULPopupManager method directly that you > > > don't know if its on a menubar or popup? > > > > I didn't see nothing ready for us but I agree we should have code shared > > (I'll file a bug on this?) > > sure bug 761062 > > > > nsXULTreeItemAccessibleBase::NativeState() > > > > { > > > > if (!mTreeView) > > > > return states::DEFUNCT; > > > > > > btw, shouldn't this go away? while having a tree with no view makes sense it > > > doesn't really make sense that tree has rows or cells does it? > > > > yes technically IsDefunct should work here. I'll file a bug to be on safe > > side? > > sure bug 761064