Open Bug 777603 Opened 12 years ago Updated 2 years ago

move some nsAccessibilityService methods into layout/

Categories

(Core :: Disability Access APIs, defect)

Product:
Core
Component:
Disability Access APIs
Type:
defect

Tracking

()

People

(Reporter: tbsaunde, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 2 obsolete files)

patch v3
50.85 KB, patch
roc
: review+
Details | Diff | Splinter Review
patch
55.28 KB, patch
surkov
: feedback+
Details | Diff | Splinter Review
patch
38.00 KB, patch
Details | Diff | Splinter Review
Attached patch patch (obsolete) — — Splinter Review 
This should hopefully make it clearer what's going on and remove silly indirection.
Attachment #645992 - Flags: review?(surkov.alexander)
Attachment #645992 - Flags: review?(bzbarsky)
Comment on attachment 645992 [details] [diff] [review]
patch

Review of attachment 645992 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/base/Makefile.in
@@ +49,4 @@
>  EXPORTS = \
>    a11yGeneric.h \
>  	AccEvent.h \
> +	NotificationController.h \

fix these two, you are based on your previous patch that doesn't have indentation fix

::: accessible/src/base/nsAccessibilityService.h
@@ -154,5 @@
> -   * Update list bullet accessible.
> -   */
> -  virtual void UpdateListBullet(nsIPresShell* aPresShell,
> -                                nsIContent* aHTMLListItemContent,
> -                                bool aHasBullet);

please move comments as well

::: layout/base/nsIPresShell.h
@@ +279,5 @@
>  #ifdef ACCESSIBILITY
> +  DocAccessible* GetAccDocument()
> +  {
> +    return mAccDocument;
> +  }

I'd like to see these methods commented, it can be something new for some layout folks who doesn't know much about a11y.
> ::: accessible/src/base/nsAccessibilityService.h
> @@ -154,5 @@
> > -   * Update list bullet accessible.
> > -   */
> > -  virtual void UpdateListBullet(nsIPresShell* aPresShell,
> > -                                nsIContent* aHTMLListItemContent,
> > -                                bool aHasBullet);
> 
> please move comments as well

most of the methods layout calls are already commented, and I'll add comments for the rest, where else do you want comments?

> ::: layout/base/nsIPresShell.h
> @@ +279,5 @@
> >  #ifdef ACCESSIBILITY
> > +  DocAccessible* GetAccDocument()
> > +  {
> > +    return mAccDocument;
> > +  }
> 
> I'd like to see these methods commented, it can be something new for some
> layout folks who doesn't know much about a11y.

true, but there isn't much more than what the name say to say about what they do, but I can add a short comment.
Comment on attachment 645992 [details] [diff] [review]
patch

Trev, I'm very unlikely to get to this before the end of August.  I suggest finding a different reviewer if you want the review before then...
(In reply to Boris Zbarsky (:bz) from comment #3)
> Comment on attachment 645992 [details] [diff] [review]
> patch
> 
> Trev, I'm very unlikely to get to this before the end of August.  I suggest
> finding a different reviewer if you want the review before then...

that's completely understandable, suggestions?  I don't think its terribly urgent, but would be nice to not keep it in my queue too long.
Maybe try roc for general sr stuff and dholbert for the actual review?
Attachment #645992 - Flags: superreview?(roc)
Attachment #645992 - Flags: review?(dholbert)
Attachment #645992 - Flags: review?(bzbarsky)
Comment on attachment 645992 [details] [diff] [review]
patch

Review of attachment 645992 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsIPresShell.h
@@ +279,5 @@
>  #ifdef ACCESSIBILITY
> +  DocAccessible* GetAccDocument()
> +  {
> +    return mAccDocument;
> +  }

GetDocAccessible seems like a better name here.

@@ +285,1 @@
>    void SetAccDocument(DocAccessible* aAccDocument)

SetDocAccessible

::: layout/generic/nsObjectFrame.cpp
@@ +464,5 @@
>  
>  #ifdef ACCESSIBILITY
> +  DocAccessible* docAcc = PresContext()->PresShell()->GetAccDocument();
> +  if (docAcc) {
> +    docAcc->RecreateAccessible(mContent);

This pattern of getting the DocAccessible actually adds code and overhead. How about adding nsIFrame::GetDocAccessible() that can quickly (inline) check whether accessibility is enabled and return null if it isn't, otherwise go through the prescontext and shell to call GetAccDocument?

::: layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp
@@ +59,5 @@
>  #include "nsIScriptableRegion.h"
>  
>  #ifdef ACCESSIBILITY
> +#include "mozilla/a11y/DocAccessible.h"
> +#include "mozilla/a11y/XULTreeAccessible.h"

Why the subdir?
Putting them in "mozilla" should be perfectly adequate.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> Comment on attachment 645992 [details] [diff] [review]
> patch
> 
> Review of attachment 645992 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/nsIPresShell.h
> @@ +279,5 @@
> >  #ifdef ACCESSIBILITY
> > +  DocAccessible* GetAccDocument()
> > +  {
> > +    return mAccDocument;
> > +  }
> 
> GetDocAccessible seems like a better name here.
> 
> @@ +285,1 @@
> >    void SetAccDocument(DocAccessible* aAccDocument)
> 
> SetDocAccessible

both are used in a11y some, but I have no issue with that.

> ::: layout/generic/nsObjectFrame.cpp
> @@ +464,5 @@
> >  
> >  #ifdef ACCESSIBILITY
> > +  DocAccessible* docAcc = PresContext()->PresShell()->GetAccDocument();
> > +  if (docAcc) {
> > +    docAcc->RecreateAccessible(mContent);
> 
> This pattern of getting the DocAccessible actually adds code and overhead.
> How about adding nsIFrame::GetDocAccessible() that can quickly (inline)
> check whether accessibility is enabled and return null if it isn't,
> otherwise go through the prescontext and shell to call GetAccDocument?

yeah, didn't think about that :/

> ::: layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp
> @@ +59,5 @@
> >  #include "nsIScriptableRegion.h"
> >  
> >  #ifdef ACCESSIBILITY
> > +#include "mozilla/a11y/DocAccessible.h"
> > +#include "mozilla/a11y/XULTreeAccessible.h"
> 
> Why the subdir?
> Putting them in "mozilla" should be perfectly adequate.

consistancy with other stuff that's already there, otherwise 302 surkov.
Comment on attachment 645992 [details] [diff] [review]
patch

Review of attachment 645992 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsPresShell.cpp
@@ +2932,5 @@
>    }
>  
>  #ifdef ACCESSIBILITY
> +  if (mAccDocument && content) {
> +    mAccDocument->SetAnchorJump(content);

you should use anchorTarget which can be different from content. If you have objections why we sholudn't do that then you should remove anchorTarget at all
(In reply to Trevor Saunders (:tbsaunde) from comment #2)
> > please move comments as well
> 
> most of the methods layout calls are already commented, and I'll add
> comments for the rest, where else do you want comments?

I just noticed this one. If others are commented then it's fine. I should have been say: don't get comments lost.

> > I'd like to see these methods commented, it can be something new for some
> > layout folks who doesn't know much about a11y.
> 
> true, but there isn't much more than what the name say to say about what
> they do, but I can add a short comment.

Yeah, you can say something like: return associated document accessible if any or if the presshell is accessible.

(In reply to Trevor Saunders (:tbsaunde) from comment #7)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> > >  #ifdef ACCESSIBILITY
> > > +  DocAccessible* GetAccDocument()
> > 
> > GetDocAccessible seems like a better name here.

> > @@ +285,1 @@
> > >    void SetAccDocument(DocAccessible* aAccDocument)
> > 
> > SetDocAccessible
> 
> both are used in a11y some, but I have no issue with that.

that's my thinking, I think I like Get/SetDocAccessible. In a11y we tend to use 'Document' term.

> > >  #ifdef ACCESSIBILITY
> > > +#include "mozilla/a11y/DocAccessible.h"
> > > +#include "mozilla/a11y/XULTreeAccessible.h"
> > 
> > Why the subdir?
> > Putting them in "mozilla" should be perfectly adequate.
> 
> consistancy with other stuff that's already there, otherwise 302 surkov.

These names aren't in conflict but probably others can be, keeping them in a11y namespace under a11y folder looks safe. And right it's consistent to what we did before.
(In reply to alexander :surkov from comment #9)
> (In reply to Trevor Saunders (:tbsaunde) from comment #2)

> > > > +#include "mozilla/a11y/XULTreeAccessible.h"
> > > 
> > > Why the subdir?
> > > Putting them in "mozilla" should be perfectly adequate.
> > 
> > consistancy with other stuff that's already there, otherwise 302 surkov.
> 
> These names aren't in conflict but probably others can be, keeping them in
> a11y namespace under a11y folder looks safe. And right it's consistent to
> what we did before.

Note: https://groups.google.com/forum/?fromgroups#!topic/mozilla.dev.platform/U_YGHuChdFk

(but let's not hold this bug up)
(In reply to Trevor Saunders (:tbsaunde) from comment #7)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> > Comment on attachment 645992 [details] [diff] [review]
> > patch
> > 
> > Review of attachment 645992 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: layout/base/nsIPresShell.h
> > @@ +279,5 @@
> > >  #ifdef ACCESSIBILITY
> > > +  DocAccessible* GetAccDocument()
> > > +  {
> > > +    return mAccDocument;
> > > +  }
> > 
> > GetDocAccessible seems like a better name here.
> > 
> > @@ +285,1 @@
> > >    void SetAccDocument(DocAccessible* aAccDocument)
> > 
> > SetDocAccessible
> 
> both are used in a11y some, but I have no issue with that.
> 
> > ::: layout/generic/nsObjectFrame.cpp
> > @@ +464,5 @@
> > >  
> > >  #ifdef ACCESSIBILITY
> > > +  DocAccessible* docAcc = PresContext()->PresShell()->GetAccDocument();
> > > +  if (docAcc) {
> > > +    docAcc->RecreateAccessible(mContent);
> > 
> > This pattern of getting the DocAccessible actually adds code and overhead.
> > How about adding nsIFrame::GetDocAccessible() that can quickly (inline)
> > check whether accessibility is enabled and return null if it isn't,
> > otherwise go through the prescontext and shell to call GetAccDocument?
> 
> yeah, didn't think about that :/

on the other hand, I'd guess that the pointers we want to chase this way are more likely to be in the cache than nsAccessibilityService::mIsShutdown especially in the case accessibility is off, so I'm not sure how much this will actually help.
> > > @@ +285,1 @@
> > > >    void SetAccDocument(DocAccessible* aAccDocument)
> > > 
> > > SetDocAccessible
> > 
> > both are used in a11y some, but I have no issue with that.
> 
> that's my thinking, I think I like Get/SetDocAccessible. In a11y we tend to
> use 'Document' term.

true, on other hand nsAccDocManager ;)

> > > >  #ifdef ACCESSIBILITY
> > > > +#include "mozilla/a11y/DocAccessible.h"
> > > > +#include "mozilla/a11y/XULTreeAccessible.h"
> > > 
> > > Why the subdir?
> > > Putting them in "mozilla" should be perfectly adequate.
> > 
> > consistancy with other stuff that's already there, otherwise 302 surkov.
> 
> These names aren't in conflict but probably others can be, keeping them in
> a11y namespace under a11y folder looks safe. And right it's consistent to
> what we did before.

I'd actually be fairly confident things like FocusManager, roles, and states conflict with something someone else wants to do.
> > > > +  DocAccessible* docAcc = PresContext()->PresShell()->GetAccDocument();
> > > > +  if (docAcc) {
> > > > +    docAcc->RecreateAccessible(mContent);
> > > 
> > > This pattern of getting the DocAccessible actually adds code and overhead.
> > > How about adding nsIFrame::GetDocAccessible() that can quickly (inline)
> > > check whether accessibility is enabled and return null if it isn't,
> > > otherwise go through the prescontext and shell to call GetAccDocument?
> > 
> > yeah, didn't think about that :/
> 
> on the other hand, I'd guess that the pointers we want to chase this way are
> more likely to be in the cache than nsAccessibilityService::mIsShutdown
> especially in the case accessibility is off, so I'm not sure how much this
> will actually help.

oh, an actual problem I thought of, doing this check in nsIFrame.h would seem to require nsIFrame.h include nsAccessibilityService.h which seems rather undesirable thoughts?
Hi Trevor -- sorry for not getting to this yet -- I'm actually leaving tonight for a weeklong vacation, and I've been scrambling over the past few days to get some of my own patches ready to be reviewed while I'm gone.  I was hoping to squeeze this in, too, but I don't think I'll be able to, unfortuantely.  (It looks like roc's already looked at it, though, so I think you're more than covered with his feedback, for layout-people review-coverage.)
Attached patch patch v2 (obsolete) — — Splinter Review 
renamed GetAccDocument and mAccDocument to GetDocumentAccessible / mDocumentAccessible
commented Get / Set DocumentAccessible()
commented methods that had been called from nsAccessibilityService.cpp that only had comments in nsAccessibilityService.h before.
added mozilla::a11y::enabled in its own header so nsIFrame::GetDocumentAccessible() can quickly check if a11y is on without including nsAccessibilityService.
added nsIFrame::GetDocumentAccessible() and used where appropriate
Attachment #645992 - Attachment is obsolete: true
Attachment #645992 - Flags: superreview?(roc)
Attachment #645992 - Flags: review?(surkov.alexander)
Attachment #645992 - Flags: review?(dholbert)
Attachment #646939 - Flags: review?(surkov.alexander)
Attachment #646939 - Flags: review?(roc)
Comment on attachment 646939 [details] [diff] [review]
patch v2

Review of attachment 646939 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/base/Enabled.h
@@ +11,5 @@
> +namespace a11y {
> +/**
> + * True if accessibility is enabled.
> + */
> +  extern bool enabled;

gEnabled would match our naming conventions better. I think mozilla::gAccessibilityEnabled (in AccessibilityEnabled.h) would be even better but I can live with mozilla:a11y::gEnabled I guess.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16)
> Comment on attachment 646939 [details] [diff] [review]
> patch v2
> 
> Review of attachment 646939 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/base/Enabled.h
> @@ +11,5 @@
> > +namespace a11y {
> > +/**
> > + * True if accessibility is enabled.
> > + */
> > +  extern bool enabled;
> 
> gEnabled would match our naming conventions better. I think

err, yeah, oops

> mozilla::gAccessibilityEnabled (in AccessibilityEnabled.h) would be even
> better but I can live with mozilla:a11y::gEnabled I guess.

I'd probably go slightly the other way, but leave it up to you / Alex.
I'm not sure why you change gIsShutdown approach at all, enabled accessibility is equivalent to running the accessibility service. It seems to me it adds extra level of complexity.
Comment on attachment 646939 [details] [diff] [review]
patch v2

Review of attachment 646939 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/base/Makefile.in
@@ +48,5 @@
>  
>  EXPORTS = \
>    a11yGeneric.h \
> +AccEvent.h \
> +NotificationController.h \

nit: wrong indentation

@@ +66,5 @@
>  
>  ifdef MOZ_DEBUG
>  EXPORTS_mozilla/a11y += \
> +Logging.h \
> +$(NULL)

here too
(In reply to alexander :surkov from comment #18)
> I'm not sure why you change gIsShutdown approach at all, enabled
> accessibility is equivalent to running the accessibility service. It seems
> to me it adds extra level of complexity.

I'm not exactly thrilled with it, but I'm less thrilled with the idea of nsIFrame.h including nsAccessibilityService.h
Why wouldn't you return simple
return PresContext()->PresShell()->GetDocumentAccessible();
in nsIFrame.h?
If accessibility is not enabled then it returns null.
(In reply to alexander :surkov from comment #21)
> Why wouldn't you return simple
> return PresContext()->PresShell()->GetDocumentAccessible();
> in nsIFrame.h?
> If accessibility is not enabled then it returns null.

see comment 6?
I see. I'm not sure I like to have a separate file for constant. nsIFrame-inl.h is not an option? It seems like more generic approach.

Btw, what is the future of nsIPresShell::AccService() and IsAccessibilityActive? you don't get rid them.
> Btw, what is the future of nsIPresShell::AccService() and
> IsAccessibilityActive? you don't get rid them.

they should probably go away eventually, but iirc they're still used for now.
ok, we are again handing in the middle. What's the future of the accessibility service?
(In reply to alexander :surkov from comment #25)
> ok, we are again handing in the middle. What's the future of the
> accessibility service?

I'm not sure what you mean by "handing in the middle"

I have no grand plans.

It seems conceptually it does many things.
* implement nsIAccessibleRetrieval we need to keep around some global xpcom object for this.
* cause accessibility to be started when platform requests it.  It would be nice to seperate this from xpcom accessibility support, but I don't have detailed plan yet.
* act as glue place for others to call methods to update accessibility.  I don't see any reason to keep this glue.
* store some global state for accessibility focus manager, and AccDocManager come to mind.
* other things I'm not thinking of right now.
(In reply to alexander :surkov from comment #23)
> I see. I'm not sure I like to have a separate file for constant.
> nsIFrame-inl.h is not an option? It seems like more generic approach.

noot if the next problem is another common header that wants to check if a11y is on.

any way I don't really care much one way or the other, but I'm not really interested in spending time figuring it out, so you and roc should figure it out :)
\
If GetDocAccessible is not inline, that's OK. Just don't make it virtual :-).
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #28)
> If GetDocAccessible is not inline, that's OK. Just don't make it virtual :-).

ok, that's not hard :)

I imagine your hoping pgo will take care of the inlining for us on windows?
A direct call with no parameters (other than 'this') is pretty fast anyway.
Attached patch patch v3 — — Splinter Review 
don't make nsIFrame::GetDocumentAccessible() inline, and go back to nsAccessibilityService::IsShutdown()
Attachment #649545 - Flags: review?(surkov.alexander)
Attachment #649545 - Flags: review?(roc)
Attachment #646939 - Attachment is obsolete: true
Attachment #646939 - Flags: review?(surkov.alexander)
Attachment #646939 - Flags: review?(roc)
Comment on attachment 649545 [details] [diff] [review]
patch v3

Review of attachment 649545 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/atk/Makefile.in
@@ +46,5 @@
> +AccessibleWrap.h \
> +DocAccessibleWrap.h \
> +HyperTextAccessibleWrap.h \
> +ImageAccessibleWrap.h \
> +$(null)

wrong indentation

@@ +54,5 @@
>  
>  include $(topsrcdir)/config/rules.mk
>  
> +CFLAGS	+= $(MOZ_GTK2_CFLAGS)
> +CXXFLAGS+= $(MOZ_GTK2_CFLAGS)

no space before + (not sure why you changed that at all)

::: accessible/src/base/Makefile.in
@@ +48,5 @@
>  
>  EXPORTS = \
>    a11yGeneric.h \
> +AccEvent.h \
> +NotificationController.h \

fix indentation

(NotificationController: exporting a pure internal a11y thing).
(AccEvent maybe won't be used outside the a11y)

@@ +65,5 @@
>  
>  ifdef MOZ_DEBUG
>  EXPORTS_mozilla/a11y += \
> +Logging.h \
> +$(NULL)

fix indentation

::: accessible/src/base/nsAccessNode.cpp
@@ -86,5 @@
>  nsAccessNode::GetApplicationAccessible()
>  {
> -  NS_ASSERTION(!nsAccessibilityService::IsShutdown(),
> -               "Accessibility wasn't initialized!");
> -

why you remove it?

::: accessible/src/generic/DocAccessible.h
@@ +193,5 @@
>    void MaybeNotifyOfValueChange(Accessible* aAccessible);
>  
>    /**
>     * Get/set the anchor jump.
>     */

that 'set' should be updated

::: accessible/src/generic/Makefile.in
@@ +37,5 @@
> +DocAccessible-inl.h \
> +HyperTextAccessible.h \
> +ImageAccessible.h \
> +RootAccessible.h \
> +TableAccessible.h \

wrong indentation

(so many things to export and some things don't belong to a11y namespace which is bad)

TableAccessibe.h isn't used outside?

::: accessible/src/html/HTMLListAccessible.h
@@ +59,5 @@
>  
>    // nsHTMLLIAccessible
> +
> +  /**
> +   * Update the accessible for the list item's bullet.

it'd be nice to document an argument as well

::: accessible/src/html/Makefile.in
@@ +32,5 @@
> +EXPORTS_mozilla/a11y = \
> +HTMLImageMapAccessible.h \
> +HTMLLinkAccessible.h \
> +HTMLListAccessible.h \
> +$(NULL)

fix indent pls

::: accessible/src/mac/Makefile.in
@@ +39,5 @@
> +AccessibleWrap.h \
> +DocAccessibleWrap.h \
> +HyperTextAccessibleWrap.h \
> +ImageAccessibleWrap.h \
> +$(null)

indent, lowercased null?

::: accessible/src/msaa/Makefile.in
@@ +57,5 @@
> +				ia2AccessibleComponent.h \
> +				ia2AccessibleEditableText.h \
> +				ia2AccessibleHyperlink.h \
> +				ia2AccessibleHypertext.h \
> +				ia2AccessibleText.h \

indent

@@ +68,5 @@
> +AccessibleWrap.h \
> +DocAccessibleWrap.h \
> +HyperTextAccessibleWrap.h \
> +ImageAccessibleWrap.h \
> +$(null)

indent
(dunno why but this null is in lowercase)

::: accessible/src/other/Makefile.in
@@ +21,5 @@
>    $(NULL)
>  
>  EXPORTS = \
>    nsAccessNodeWrap.h \
> +$(null)

lowercase null and indent

@@ +30,5 @@
> +AccessibleWrap.h \
> +DocAccessibleWrap.h \
> +HyperTextAccessibleWrap.h \
> +ImageAccessibleWrap.h \
> +$(null)

lowercase null and indent

::: accessible/src/xpcom/Makefile.in
@@ +21,5 @@
>    $(NULL)
>  
> +EXPORTS = \
> +				xpcAccessibleTable.h \
> +				$(NULL)

indent (this patch makes me think that one improvement makes us to do not nice thing by exporting headers we shouldn't export (basically sort of reverting of our previous work)).

::: accessible/src/xul/Makefile.in
@@ +37,5 @@
> +XULListboxAccessible.h \
> +XULMenuAccessible.h \
> +XULSelectControlAccessible.h \
> +XULTreeAccessible.h \
> +$(NULL)

indent

::: layout/base/nsIPresShell.h
@@ +284,3 @@
>    {
> +    return mDocumentAccessible;
> +  }

I wonder whether GetDocAccessible is nicer since it returns DocAccessible type and the name is shorter?

@@ +285,5 @@
> +    return mDocumentAccessible;
> +  }
> +
> +  /**
> +   * Set the accessible document for this pres shell.

Perhaps you should say that this method is used by a11y and it shouldn't be used until you know what you do.

@@ +1342,5 @@
>    // GetRootFrame() can be inlined:
>    nsFrameManagerBase*       mFrameManager;
>    nsWeakPtr                 mForwardingContainer;
>  #ifdef ACCESSIBILITY
> +  DocAccessible* mDocumentAccessible;

same: mDocAccessible?

::: layout/base/nsPresShell.cpp
@@ +2932,5 @@
>    }
>  
>  #ifdef ACCESSIBILITY
> +  if (mDocumentAccessible && anchorTarget) {
> +    mDocumentAccessible->SetAnchorJump(anchorTarget);

it doesn't sounds as a good method name anymore, maybe AnchorJumped() is better (what keeps it similar to ContentInserted/Removed methods)

::: layout/generic/nsIFrame.h
@@ +2448,5 @@
>    virtual already_AddRefed<Accessible> CreateAccessible() = 0;
> +
> +  /**
> +   * If accessibility is active return the accessible document object for the
> +   * pres shell this frame is in.

I'd say:
Return the accessible document for the pres shell this frame is in (or resides in). Note: always returns null if accessibility is not running.

::: layout/generic/nsImageMap.cpp
@@ +800,5 @@
> +          // If image map was initialized after we created an accessible (that'll
> +          // be an image accessible) then recreate it.
> +          docAcc->RecreateAccessible(mImageFrame->GetContent());
> +        }
> +      }

(additionally to nsTreeBodyFrame concerns) It's a good step on the way to move out the a11y logic from a11y module and spred it out through whole Gecko, I'm not sure I like it. AccService layer is obviously excess but it was used nicely for abstraction layer. Maybe we can turn DocAccessible into new AccService and won't export everything from a11y module?

::: layout/generic/nsTextFrameThebes.cpp
@@ +7213,1 @@
>      }

it'd be nice to have some comment here like
The rendered text might be changed. Notify the document accessible.

::: layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp
@@ +483,5 @@
> +    if (docAcc) {
> +      Accessible* acc = docAcc->GetAccessible(treeContent);
> +      if (acc && acc->IsXULTree()) {
> +        acc->AsXULTree()->TreeViewChanged(mView);
> +      }

there's downside of these close relations between a11y and rest core. You never know which method is used outside and which method is for internal a11y usage. And another side the building gets potentially longer (for example, when we put Accessible/DocAccessible under a11y namespace).
(In reply to Trevor Saunders (:tbsaunde) from comment #26)
> (In reply to alexander :surkov from comment #25)
> > ok, we are again handing in the middle. What's the future of the
> > accessibility service?
> 
> I'm not sure what you mean by "handing in the middle"
> I have no grand plans.

since you get rid the glue then we don't need nsIPresShell::AccService() and IsAccessibilityActive. So hanging in the middle we did some work on this way but it wasn't complete. Usually it makes the code looking strange and inconsistent.
(In reply to alexander :surkov from comment #33)
> (In reply to Trevor Saunders (:tbsaunde) from comment #26)
> > (In reply to alexander :surkov from comment #25)
> > > ok, we are again handing in the middle. What's the future of the
> > > accessibility service?
> > 
> > I'm not sure what you mean by "handing in the middle"
> > I have no grand plans.
> 
> since you get rid the glue then we don't need nsIPresShell::AccService() and
> IsAccessibilityActive. So hanging in the middle we did some work on this way
> but it wasn't complete. Usually it makes the code looking strange and
> inconsistent.

ah, so handing was a typo, I thought it was some funny saying.
(In reply to alexander :surkov from comment #32)> @@ +54,5 @@
> >  
> >  include $(topsrcdir)/config/rules.mk
> >  
> > +CFLAGS	+= $(MOZ_GTK2_CFLAGS)
> > +CXXFLAGS+= $(MOZ_GTK2_CFLAGS)
> 
> no space before + (not sure why you changed that at all)

I think it happened the first time I tried to fix this and screwed up.

> >  EXPORTS = \
> >    a11yGeneric.h \
> > +AccEvent.h \
> > +NotificationController.h \
> 
> fix indentation
> 
> (NotificationController: exporting a pure internal a11y thing).
> (AccEvent maybe won't be used outside the a11y)

probably not, but DocAccessible.h needs it so that it can take an enum declared there as an argument to a function.  I don't remember the story for NotificationController.h any more.  As I said a while back  I'm not exacly thrilled about this either.

> ::: accessible/src/base/nsAccessNode.cpp
> @@ -86,5 @@
> >  nsAccessNode::GetApplicationAccessible()
> >  {
> > -  NS_ASSERTION(!nsAccessibilityService::IsShutdown(),
> > -               "Accessibility wasn't initialized!");
> > -
> 
> why you remove it?

forgot to readd it after removing Enabled.h

> 
> ::: accessible/src/generic/DocAccessible.h
> @@ +193,5 @@
> >    void MaybeNotifyOfValueChange(Accessible* aAccessible);
> >  
> >    /**
> >     * Get/set the anchor jump.
> >     */
> 
> that 'set' should be updated

true

> TableAccessibe.h isn't used outside?

XULListBoxAccessible.h needs to include it.  However I think its a pretty reasonable api so exporting it in particular doesn't make me too sad.

> ::: accessible/src/xpcom/Makefile.in
> @@ +21,5 @@
> >    $(NULL)
> >  
> > +EXPORTS = \
> > +				xpcAccessibleTable.h \
> > +				$(NULL)
> 
> indent (this patch makes me think that one improvement makes us to do not
> nice thing by exporting headers we shouldn't export (basically sort of
> reverting of our previous work)).

I think we went to far in the direction of exporting to little before, but as I said I'm not thrilled with exporting everything we do here.

However most of the bad exports should be fixable eventually one way or another.
the AccEvent.h thing can be fixed with an enum class when we can use those
the xpcAccessibleTable.h can be fixed by seperate xpcom objects, and similar for the wrap andia2 headers with platform objects.

> ::: layout/base/nsIPresShell.h
> @@ +284,3 @@
> >    {
> > +    return mDocumentAccessible;
> > +  }
> 
> I wonder whether GetDocAccessible is nicer since it returns DocAccessible
> type and the name is shorter?

it doesn't matter much to me, but I'm not terribly interested in renaming it all the places its used again.

> @@ +285,5 @@
> > +    return mDocumentAccessible;
> > +  }
> > +
> > +  /**
> > +   * Set the accessible document for this pres shell.
> 
> Perhaps you should say that this method is used by a11y and it shouldn't be
> used until you know what you do.

well, used by people managing life cycle of accessible documents, but sure.

> 
> ::: layout/base/nsPresShell.cpp
> @@ +2932,5 @@
> >    }
> >  
> >  #ifdef ACCESSIBILITY
> > +  if (mDocumentAccessible && anchorTarget) {
> > +    mDocumentAccessible->SetAnchorJump(anchorTarget);
> 
> it doesn't sounds as a good method name anymore, maybe AnchorJumped() is
> better (what keeps it similar to ContentInserted/Removed methods)

that seems reasonable, but follow up gfb?

> 
> ::: layout/generic/nsIFrame.h
> @@ +2448,5 @@
> >    virtual already_AddRefed<Accessible> CreateAccessible() = 0;
> > +
> > +  /**
> > +   * If accessibility is active return the accessible document object for the
> > +   * pres shell this frame is in.
> 
> I'd say:
> Return the accessible document for the pres shell this frame is in (or
> resides in). Note: always returns null if accessibility is not running.

sounds good

> 
> ::: layout/generic/nsImageMap.cpp
> @@ +800,5 @@
> > +          // If image map was initialized after we created an accessible (that'll
> > +          // be an image accessible) then recreate it.
> > +          docAcc->RecreateAccessible(mImageFrame->GetContent());
> > +        }
> > +      }
> 
> (additionally to nsTreeBodyFrame concerns) It's a good step on the way to
> move out the a11y logic from a11y module and spred it out through whole
> Gecko, I'm not sure I like it. AccService layer is obviously excess but it
> was used nicely for abstraction layer. Maybe we can turn DocAccessible into
> new AccService and won't export everything from a11y module?

I'm not terribly big on abstractions for there own sake, putting it on the DocAccessible instead of AccService certainly makes things better, and exporting less is nice, but I'm not sure I like it.

> 
> ::: layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp
> @@ +483,5 @@
> > +    if (docAcc) {
> > +      Accessible* acc = docAcc->GetAccessible(treeContent);
> > +      if (acc && acc->IsXULTree()) {
> > +        acc->AsXULTree()->TreeViewChanged(mView);
> > +      }
> 
> there's downside of these close relations between a11y and rest core. You
> never know which method is used outside and which method is for internal
> a11y usage. And another side the building gets potentially longer (for
> example, when we put Accessible/DocAccessible under a11y namespace).

well, you can check easily enough if something is uesd outside of accessible/ its also not really clear to why it matters much if other parts of gecko use something so long as they use it the way it is supposed to be.
Alex?
(In reply to Trevor Saunders (:tbsaunde) from comment #36)
> Alex?

my problem is I don't think I really like what we get in the end. I'm ok with idea to not use the accessibility service as mediator between a11y and rest of gecko (since the call into accservice from presshell to get a document from presshell looks silly) but the same time it moves a lot a11y logic outside of a11y exporting extra headers what breaks encapsulation idea currently maintained by accservice.
Comment on attachment 649545 [details] [diff] [review]
patch v3

canceling review until we're on the same page
Attachment #649545 - Flags: review?(surkov.alexander)
Attached patch patch — — Splinter Review 
Surkov this isn't really cleaned up yet, but Ii think its the approach you wanted or closer.  I'm still not really a fan of it, but I tried it for thuroughness's sake.
Attachment #658001 - Flags: feedback?(surkov.alexander)
Comment on attachment 658001 [details] [diff] [review]
patch

Review of attachment 658001 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsImageMap.cpp
@@ +790,5 @@
>  #ifdef ACCESSIBILITY
>    if (NS_SUCCEEDED(rv)) {
> +    DocAccessible* docAcc = mImageFrame->GetDocumentAccessible();
> +    if (docAcc) {
> +      docAcc->UpdateImageMap(mImageFrame->GetContent());

if this frame class is unique consumer of this method then you could pass image frame instead its content node, layout part gets simpler

::: layout/xul/base/src/tree/src/Makefile.in
@@ +49,5 @@
> +  -I$(topsrcdir)/accessible/src/html \
> +  -I$(topsrcdir)/accessible/src/xpcom \
> +  -I$(topsrcdir)/accessible/src/xul \
> +  $(null)
> +endif

it might be nice to have mk for all of this

::: layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp
@@ +482,5 @@
> +    DocAccessible* docAcc = GetDocumentAccessible();
> +    if (docAcc) {
> +      Accessible* acc = docAcc->GetAccessible(treeContent);
> +      if (acc && acc->IsXULTree()) {
> +        docAcc->UpdateTreeWithNewView(acc->AsXULTree(), mView);

you can keep these things inside like
docAcc->UpdateTreeView(treeContent, mView);
Attachment #658001 - Flags: feedback?(surkov.alexander) → feedback+
> ::: layout/generic/nsImageMap.cpp
> @@ +790,5 @@
> >  #ifdef ACCESSIBILITY
> >    if (NS_SUCCEEDED(rv)) {
> > +    DocAccessible* docAcc = mImageFrame->GetDocumentAccessible();
> > +    if (docAcc) {
> > +      docAcc->UpdateImageMap(mImageFrame->GetContent());
> 
> if this frame class is unique consumer of this method then you could pass
> image frame instead its content node, layout part gets simpler

at the price of making the other method more complicated, what's point?

> ::: layout/xul/base/src/tree/src/Makefile.in
> @@ +49,5 @@
> > +  -I$(topsrcdir)/accessible/src/html \
> > +  -I$(topsrcdir)/accessible/src/xpcom \
> > +  -I$(topsrcdir)/accessible/src/xul \
> > +  $(null)
> > +endif
> 
> it might be nice to have mk for all of this

yeah, maybe, I'd been hoping I could limit what got local included more based on dirs, but the DocAccessible-inl.h being the only file containing all the methods prevents that.

> ::: layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp
> @@ +482,5 @@
> > +    DocAccessible* docAcc = GetDocumentAccessible();
> > +    if (docAcc) {
> > +      Accessible* acc = docAcc->GetAccessible(treeContent);
> > +      if (acc && acc->IsXULTree()) {
> > +        docAcc->UpdateTreeWithNewView(acc->AsXULTree(), mView);
> 
> you can keep these things inside like
> docAcc->UpdateTreeView(treeContent, mView);

I could, but then I'd lose the ability to force people to only pass in trees statically.

anyway I'm starting to think I'll just fix the ones that are already on DocAccessible so we don't disagree about, and leave the others for some other day.
(In reply to Trevor Saunders (:tbsaunde) from comment #41)

> > > +      docAcc->UpdateImageMap(mImageFrame->GetContent());
> > 
> > if this frame class is unique consumer of this method then you could pass
> > image frame instead its content node, layout part gets simpler
> 
> at the price of making the other method more complicated, what's point?

the idea of inlines on document accessible is to keep layout simple as possible. UpdateImageMap() is created only for nsImageMap and it can take any arguments that nsImageMap wants.

> > > +        docAcc->UpdateTreeWithNewView(acc->AsXULTree(), mView);
> > 
> > you can keep these things inside like
> > docAcc->UpdateTreeView(treeContent, mView);
> 
> I could, but then I'd lose the ability to force people to only pass in trees
> statically.

Please rephrase, I don't follow this.
(In reply to alexander :surkov from comment #42)
> (In reply to Trevor Saunders (:tbsaunde) from comment #41)
> 
> > > > +      docAcc->UpdateImageMap(mImageFrame->GetContent());
> > > 
> > > if this frame class is unique consumer of this method then you could pass
> > > image frame instead its content node, layout part gets simpler
> > 
> > at the price of making the other method more complicated, what's point?
> 
> the idea of inlines on document accessible is to keep layout simple as
> possible. UpdateImageMap() is created only for nsImageMap and it can take
> any arguments that nsImageMap wants.

I'ts not really clear to me that its better for the layout part to be simple at the expense of something else.

If you really wanted that then why not right a function like
void
UpdateImageMap(nsImageFrame* aFrame)
{
  DocAccessible* doc = aFrame->GetDocumentAccessible();
  if (!doc)
    return;

  docAccessible->UpdateImageMap(aFrame->GetContent());
}

and put that somewhere in accessible/ ?

> > > > +        docAcc->UpdateTreeWithNewView(acc->AsXULTree(), mView);
> > > 
> > > you can keep these things inside like
> > > docAcc->UpdateTreeView(treeContent, mView);
> > 
> > I could, but then I'd lose the ability to force people to only pass in trees
> > statically.
> 
> Please rephrase, I don't follow this.

if the function takes a nsIContent* someone can pass in an sorot of content thing, say a <div> instead of the nsIContent* for a xul tree.
on the other hand if the argument is a XULTreeAccessible* they can't pass in say an Accessible* for a <div>
(In reply to Trevor Saunders (:tbsaunde) from comment #43)
> (In reply to alexander :surkov from comment #42)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #41)
> > 
> > > > > +      docAcc->UpdateImageMap(mImageFrame->GetContent());
> > > > 
> > > > if this frame class is unique consumer of this method then you could pass
> > > > image frame instead its content node, layout part gets simpler
> > > 
> > > at the price of making the other method more complicated, what's point?
> > 
> > the idea of inlines on document accessible is to keep layout simple as
> > possible. UpdateImageMap() is created only for nsImageMap and it can take
> > any arguments that nsImageMap wants.
> 
> I'ts not really clear to me that its better for the layout part to be simple
> at the expense of something else.

originally that was my concern. For this specific part I think I can live with either case. But trivial layout code for notifications to a11y points out that all a11y logic on a11y side. If you're an unique consumer of some method then the method can look as you want.

> If you really wanted that then why not right a function like
> void
> UpdateImageMap(nsImageFrame* aFrame)
> {
>   DocAccessible* doc = aFrame->GetDocumentAccessible();
>   if (!doc)
>     return;
> 
>   docAccessible->UpdateImageMap(aFrame->GetContent());
> }
> 
> and put that somewhere in accessible/ ?

I think I'd be fine with that. It might be looking odd on nsPresShell though where mDocAccessible is defined.

> if the function takes a nsIContent* someone can pass in an sorot of content
> thing, say a <div> instead of the nsIContent* for a xul tree.
> on the other hand if the argument is a XULTreeAccessible* they can't pass in
> say an Accessible* for a <div>

if somebody does this then nothing really bad happens. You can add an assertion if somebody starts doing weird things.

if we follow your patch approach then we get funny inlines like
UpdateTreeView(aTree, aView) { aTree->UpdateTreeView(aView); }

but I wanted to hide a11y logic under a11y but not to introduce bunch of trivial inlines :)
(In reply to alexander :surkov from comment #44)
> (In reply to Trevor Saunders (:tbsaunde) from comment #43)
> > (In reply to alexander :surkov from comment #42)
> > > (In reply to Trevor Saunders (:tbsaunde) from comment #41)
> > > 
> > > > > > +      docAcc->UpdateImageMap(mImageFrame->GetContent());
> > > > > 
> > > > > if this frame class is unique consumer of this method then you could pass
> > > > > image frame instead its content node, layout part gets simpler
> > > > 
> > > > at the price of making the other method more complicated, what's point?
> > > 
> > > the idea of inlines on document accessible is to keep layout simple as
> > > possible. UpdateImageMap() is created only for nsImageMap and it can take
> > > any arguments that nsImageMap wants.
> > 
> > I'ts not really clear to me that its better for the layout part to be simple
> > at the expense of something else.
> 
> originally that was my concern. For this specific part I think I can live
> with either case. But trivial layout code for notifications to a11y points
> out that all a11y logic on a11y side. If you're an unique consumer of some
> method then the method can look as you want.

Sure, but if your the unique caller of a method I'd generally think you should look at why that function exists in the first place.

> > If you really wanted that then why not right a function like
> > void
> > UpdateImageMap(nsImageFrame* aFrame)
> > {
> >   DocAccessible* doc = aFrame->GetDocumentAccessible();
> >   if (!doc)
> >     return;
> > 
> >   docAccessible->UpdateImageMap(aFrame->GetContent());
> > }
> > 
> > and put that somewhere in accessible/ ?
> 
> I think I'd be fine with that. It might be looking odd on nsPresShell though
> where mDocAccessible is defined.
> 
> > if the function takes a nsIContent* someone can pass in an sorot of content
> > thing, say a <div> instead of the nsIContent* for a xul tree.
> > on the other hand if the argument is a XULTreeAccessible* they can't pass in
> > say an Accessible* for a <div>
> 
> if somebody does this then nothing really bad happens. You can add an
> assertion if somebody starts doing weird things.
> 
> if we follow your patch approach then we get funny inlines like
> UpdateTreeView(aTree, aView) { aTree->UpdateTreeView(aView); }
> 
> but I wanted to hide a11y logic under a11y but not to introduce bunch of
> trivial inlines :)

Well, you could argue that the trivial inline is the only part that is actually "a11y logic" and the rest is just pulling out the information the method requires or something.
Attached patch patch — — Splinter Review 
basically all of these methods where already defined to do something useful on DocAccessible so I image we agree about them so I want to take this then think about the others.
Attachment #658299 - Flags: review?(surkov.alexander)
Comment on attachment 658299 [details] [diff] [review]
patch

Review of attachment 658299 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/generic/DocAccessible.cpp
@@ +86,5 @@
>    mPresShell(aPresShell)
>  {
>    mFlags |= eDocAccessible;
>    if (mPresShell)
> +  mPresShell->SetDocumentAccessible(this);

nit: wrong indentation

@@ +1444,5 @@
> +      logging::Node("content", child);
> +    }
> +    logging::MsgEnd();
> +  }
> +#endif

I recall the reason I pushed the logging into AccService method rather than into DocAccessible method is it allowed to handle missed content insertions/removals because of inaccessible document.

we might need a thin layer in a11y for calls from layout similar to what we have in AccService() but it would take nsDocAccessible* instead of nsIPresShell*

but if the document accessible is visible from layout then nothing prevents people to use DocAccessible::ContentInserted instead thin layer version. What do you think?

::: accessible/src/generic/DocAccessible.h
@@ +200,4 @@
>    void MaybeNotifyOfValueChange(Accessible* aAccessible);
>  
>    /**
>     * Get/set the anchor jump.

nit: remove this 'set', maybe: Return the anchor jump if any.

::: accessible/src/html/HTMLListAccessible.h
@@ +59,5 @@
>  
>    // nsHTMLLIAccessible
> +
> +  /**
> +   * Update the accessible for the list item's bullet.

nit: I'd be nice to have more descriptive comment. I'm not sure what update the accessible could mean.

::: layout/base/nsPresShell.cpp
@@ +139,5 @@
>  #include "nsITimer.h"
>  #ifdef ACCESSIBILITY
>  #include "nsAccessibilityService.h"
>  #include "mozilla/a11y/DocAccessible.h"
> +#include "mozilla/a11y/RootAccessible.h"

RootAccessible should be enough for includling

@@ +8967,1 @@
>      }

this is controversial again, what should be hidden in a11y and what shouldn't.
> @@ +1444,5 @@
> > +      logging::Node("content", child);
> > +    }
> > +    logging::MsgEnd();
> > +  }
> > +#endif
> 
> I recall the reason I pushed the logging into AccService method rather than
> into DocAccessible method is it allowed to handle missed content
> insertions/removals because of inaccessible document.
> 
> we might need a thin layer in a11y for calls from layout similar to what we
> have in AccService() but it would take nsDocAccessible* instead of
> nsIPresShell*
> 
> but if the document accessible is visible from layout then nothing prevents
> people to use DocAccessible::ContentInserted instead thin layer version.
> What do you think?

if we didn't call it a bunch of places in layout/ I'd be tempted to just put it there.  I really not a fan of the wrapper idea, but haven't thought of something better yet.

btw Can you say when its useful to know about content changes in inaccessible documents?

> @@ +8967,1 @@
> >      }
> 
> this is controversial again, what should be hidden in a11y and what
> shouldn't.

I assume you mean the Document activated stuff? (splinter is useless when it comes to giving context)
(In reply to Trevor Saunders (:tbsaunde) from comment #48)

> > we might need a thin layer in a11y for calls from layout similar to what we
> > have in AccService() but it would take nsDocAccessible* instead of
> > nsIPresShell*
> > 
> > but if the document accessible is visible from layout then nothing prevents
> > people to use DocAccessible::ContentInserted instead thin layer version.
> > What do you think?
> 
> if we didn't call it a bunch of places in layout/ I'd be tempted to just put
> it there.  I really not a fan of the wrapper idea, but haven't thought of
> something better yet.

I don't have good ideas either. On the one hand I want to keep a11y stuffs under a11y (I like if layout just notifies a11y and it doesn't care how this notification is processed) but on another hand we've got a weird thing that presshell calls into a11y and a11y calls into presshell.

> btw Can you say when its useful to know about content changes in
> inaccessible documents?

I think when you debug timing issues to make sure layout does right things and the error is on a11y side.

> > @@ +8967,1 @@
> > >      }
> > 
> > this is controversial again, what should be hidden in a11y and what
> > shouldn't.
> 
> I assume you mean the Document activated stuff? (splinter is useless when it
> comes to giving context)

yes
Attachment #658299 - Flags: review?(surkov.alexander)
Assignee: tbsaunde+mozbugs → nobody
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: