Last Comment Bug 739191 - get rid nsAccUtils::GetDocAccessibleFor(const nsIPresShell* aPresShell)
: get rid nsAccUtils::GetDocAccessibleFor(const nsIPresShell* aPresShell)
Status: RESOLVED FIXED
[good first bug][mentor=hub@mozilla.c...
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla14
Assigned To: Andrei Touzik
:
Mentors:
Depends on:
Blocks: cleana11y
  Show dependency treegraph
 
Reported: 2012-03-26 05:37 PDT by alexander :surkov
Modified: 2012-04-17 14:26 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v.1.0 (13.18 KB, patch)
2012-04-13 14:24 PDT, Andrei Touzik
no flags Details | Diff | Splinter Review
Patch v.2.0 (19.56 KB, patch)
2012-04-14 16:58 PDT, Andrei Touzik
surkov.alexander: review+
tbsaunde+mozbugs: feedback+
Details | Diff | Splinter Review
Patch v.3.0 (19.01 KB, patch)
2012-04-16 14:02 PDT, Andrei Touzik
surkov.alexander: review+
Details | Diff | Splinter Review

Description alexander :surkov 2012-03-26 05:37:58 PDT
1) get rid nsAccUtils::GetDocAccessibleFor(const nsIPresShell* aPresShell)
2) add nsAccDocManager::GetDocAccessible(const nsIPresShell* aPresShell)
Comment 1 Veeraya Pupatwibul 2012-04-02 10:30:18 PDT
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) 2012-04-02 18:04:41 PDT
(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 2012-04-11 08:55:29 PDT
Could you assign it to me, please?
Comment 4 Andrei Touzik 2012-04-12 03:40:20 PDT
(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) 2012-04-12 03:51:12 PDT
(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 2012-04-13 14:24:07 PDT
Created attachment 614913 [details] [diff] [review]
Patch v.1.0
Comment 7 Trevor Saunders (:tbsaunde) 2012-04-13 14:44:28 PDT
Comment on attachment 614913 [details] [diff] [review]
Patch v.1.0

you didn't address part 2 of the description.
Comment 8 alexander :surkov 2012-04-13 20:14:01 PDT
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 2012-04-14 16:58:09 PDT
Created attachment 615088 [details] [diff] [review]
Patch v.2.0
Comment 10 alexander :surkov 2012-04-15 18:22:09 PDT
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) 2012-04-15 22:20:00 PDT
> ::: 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) 2012-04-15 22:20:45 PDT
Comment on attachment 615088 [details] [diff] [review]
Patch v.2.0

seems fine with comment 10 fixed
Comment 13 Andrei Touzik 2012-04-16 01:33:23 PDT
(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 2012-04-16 01:34:29 PDT
(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 2012-04-16 14:02:15 PDT
Created attachment 615458 [details] [diff] [review]
Patch v.3.0
Comment 16 alexander :surkov 2012-04-16 17:32:59 PDT
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 2012-04-16 22:30:37 PDT
landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/2745411f4bb9
Comment 18 Marco Bonardo [::mak] 2012-04-17 07:51:22 PDT
https://hg.mozilla.org/mozilla-central/rev/2745411f4bb9
Comment 19 Andrei Touzik 2012-04-17 14:26:36 PDT
(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

Note You need to log in before you can comment on or make changes to this bug.