739191 - get rid nsAccUtils::GetDocAccessibleFor(const nsIPresShell* aPresShell)

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

1) get rid nsAccUtils::GetDocAccessibleFor(const nsIPresShell* aPresShell)
2) add nsAccDocManager::GetDocAccessible(const nsIPresShell* aPresShell)

Comment 1

[Veeraya Pupatwibul]

does this mean replacing function call like these:

nsAccUtils::GetDocAccessibleFor(aPresShell);
nsAccUtils::GetDocAccessibleFor(root);

with 

nsAccDocManager::GetDocAccessible(aPresShell);
nsAccDocManager::GetDocAccessible(root);

Comment 2

[Trevor Saunders (:tbsaunde)]

(In reply to Veeraya Pupatwibul from comment #1)
> does this mean replacing function call like these:
> 
> nsAccUtils::GetDocAccessibleFor(aPresShell);
> nsAccUtils::GetDocAccessibleFor(root);
> 
> with 
> 
> nsAccDocManager::GetDocAccessible(aPresShell);
> nsAccDocManager::GetDocAccessible(root);

if root is a nsIPressShell yes otherwise no.
you should call it as either GetAccService()->GetDocAccessibleFor() or just GetDocAccessibleFor() if you are in the accessibilityService.

Comment 3

[Andrei Touzik]

Could you assign it to me, please?

Comment 4

[Andrei Touzik]

(In reply to Trevor Saunders (:tbsaunde) from comment #2)

should these 2 methods also be repalced?

nsAccUtils::GetDocAccessibleFor(nsINode *aNode)
nsAccUtils::GetDocAccessibleFor(nsIDocShellTreeItem *aContainer)

Comment 5

[Trevor Saunders (:tbsaunde)]

(In reply to Andrei from comment #4)
> (In reply to Trevor Saunders (:tbsaunde) from comment #2)
> 
> should these 2 methods also be repalced?
> 
> nsAccUtils::GetDocAccessibleFor(nsINode *aNode)
> nsAccUtils::GetDocAccessibleFor(nsIDocShellTreeItem *aContainer)

might be nice, but I'd limitt the scope of this particular bug to only the first of the three.

Comment 6

[Andrei Touzik]

Attachment: Patch v.1.0

Comment 7

[Trevor Saunders (:tbsaunde)]

Comment on attachment 614913 [details] [diff] [review]
Patch v.1.0

you didn't address part 2 of the description.

Comment 8

[alexander :surkov (:asurkov)]

Comment on attachment 614913 [details] [diff] [review]
Patch v.1.0

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

::: accessible/src/base/nsAccessibilityService.cpp
@@ +201,5 @@
>                                                   nsIPresShell* aPresShell)
>  {
>    nsAccessible* accessible =
>      new OuterDocAccessible(aContent,
> +                           GetDocAccessible(aPresShell->GetDocument()));

as Trevor said you need to add GetDocAccessible(nsIPresShell* aPresShell) version to nsAccDocManager and use it here and below.

Btw, please make sure to not keep it on separate line if you don't break 80 chars per 
line restriction like

nsAccessible* accessible =
  new OuterDocAccessible(aContent, GetDocAccessible(aPresShell));

Comment 9

[Andrei Touzik]

Attachment: Patch v.2.0

Comment 10

[alexander :surkov (:asurkov)]

Comment on attachment 615088 [details] [diff] [review]
Patch v.2.0

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

::: accessible/src/base/nsAccDocManager.cpp
@@ +83,5 @@
>  
> +nsDocAccessible*
> +nsAccDocManager::GetDocAccessible(const nsIPresShell* aPresShell)
> +{
> +	return aPresShell ? GetDocAccessible(aPresShell->GetDocument()) : nsnull;

you should use two spaces as indent (not tab), please fix the config of your editor. Also please make sure you use unix style line endings (just in case).

::: accessible/src/base/nsAccDocManager.h
@@ +71,5 @@
>  
>    /**
> +   * Return document accessible for the given presshell.
> +   */
> +  nsDocAccessible* GetDocAccessible(const nsIPresShell* aPresShell);

it'd be nice to keep in inline. I think you can include "nsIPresShell.h" for that since it shouldn't harm us. Anyway later we can introduce "AccDocManager-inl.h" to handle this case.

Trevor, are you ok with that.

::: accessible/src/base/nsAccessibilityService.cpp
@@ +322,5 @@
>                                                    nsIPresShell* aPresShell)
>  {
>    nsAccessible* accessible = 
> +    new nsEnumRoleAccessible(aContent, GetDocAccessible(aPresShell),
> +    		                 roles::GROUPING);

wrong indentation

@@ +366,5 @@
>  
>      nsAccessible* accessible = 
>        new nsHTMLWin32ObjectOwnerAccessible(aContent,
> +    		                               GetDocAccessible(aPresShell),
> +    		                               pluginPort);

wrong indentation

@@ +380,5 @@
>        NPPVpluginNativeAccessibleAtkPlugId, &plugId);
>      if (NS_SUCCEEDED(rv) && !plugId.IsEmpty()) {
>        AtkSocketAccessible* socketAccessible =
> +        new AtkSocketAccessible(aContent, GetDocAccessible(aPresShell),
> +        		                plugId);

same

Comment 11

[Trevor Saunders (:tbsaunde)]

> ::: accessible/src/base/nsAccDocManager.h
> @@ +71,5 @@
> >  
> >    /**
> > +   * Return document accessible for the given presshell.
> > +   */
> > +  nsDocAccessible* GetDocAccessible(const nsIPresShell* aPresShell);
> 
> it'd be nice to keep in inline. I think you can include "nsIPresShell.h" for
> that since it shouldn't harm us. Anyway later we can introduce
> "AccDocManager-inl.h" to handle this case.
> 
> Trevor, are you ok with that.

yes

Comment 12

[Trevor Saunders (:tbsaunde)]

Comment on attachment 615088 [details] [diff] [review]
Patch v.2.0

seems fine with comment 10 fixed

Comment 13

[Andrei Touzik]

(In reply to alexander :surkov from comment #10)
> ::: accessible/src/base/nsAccDocManager.h
> @@ +71,5 @@
> >  
> >    /**
> > +   * Return document accessible for the given presshell.
> > +   */
> > +  nsDocAccessible* GetDocAccessible(const nsIPresShell* aPresShell);
> 
> it'd be nice to keep in inline. I think you can include "nsIPresShell.h" for
> that since it shouldn't harm us. Anyway later we can introduce
> "AccDocManager-inl.h" to handle this case.

Do you mean that GetDocAccessible(const nsIPresShell* aPresShell) should be defined in nsAccDocManager.h and not in nsAccDocManager.cpp?

Comment 14

[alexander :surkov (:asurkov)]

(In reply to Andrei from comment #13)
> Do you mean that GetDocAccessible(const nsIPresShell* aPresShell) should be
> defined in nsAccDocManager.h and not in nsAccDocManager.cpp?

yes

Comment 15

[Andrei Touzik]

Attachment: Patch v.3.0

Comment 16

[alexander :surkov (:asurkov)]

Comment on attachment 615458 [details] [diff] [review]
Patch v.3.0

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

r=me, cool, thanks!

if you like to pick up another bug then please take a look here https://bugzilla.mozilla.org/buglist.cgi?emailtype1=exact;resolution=---;email1=nobody%40mozilla.org;emailassigned_to1=1;component=Disability%20Access%20APIs;product=Core;status_whiteboard=mentor;status_whiteboard_type=allwordssubstr;query_format=advanced;list_id=2870897

::: accessible/src/base/nsAccDocManager.h
@@ +72,5 @@
>  
>    /**
> +   * Return document accessible for the given presshell.
> +   */
> +  inline nsDocAccessible* GetDocAccessible(const nsIPresShell* aPresShell)

nit: you don't need inline keyword

Comment 17

[alexander :surkov (:asurkov)]

landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/2745411f4bb9

Comment 18

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/2745411f4bb9

Comment 19

[Andrei Touzik]

(In reply to alexander :surkov from comment #16)

I have asked for level 1 commit access. Could you please vouch for me?

https://bugzilla.mozilla.org/show_bug.cgi?id=746207