Closed Bug 739191 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: surkov, Assigned: xph9753)

References

(Blocks 1 open bug)

Details

(Whiteboard: [good first bug][mentor=hub@mozilla.com][lang=c++])

Attachments

(1 file, 2 obsolete files)

1) get rid nsAccUtils::GetDocAccessibleFor(const nsIPresShell* aPresShell)
2) add nsAccDocManager::GetDocAccessible(const nsIPresShell* aPresShell)
does this mean replacing function call like these:

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

with 

nsAccDocManager::GetDocAccessible(aPresShell);
nsAccDocManager::GetDocAccessible(root);
(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.
Could you assign it to me, please?
Assignee: nobody → xph9753
Status: NEW → ASSIGNED
(In reply to Trevor Saunders (:tbsaunde) from comment #2)

should these 2 methods also be repalced?

nsAccUtils::GetDocAccessibleFor(nsINode *aNode)
nsAccUtils::GetDocAccessibleFor(nsIDocShellTreeItem *aContainer)
(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.
Attached patch Patch v.1.0 (obsolete) — Splinter Review
Attachment #614913 - Flags: review?(surkov.alexander)
Comment on attachment 614913 [details] [diff] [review]
Patch v.1.0

you didn't address part 2 of the description.
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));
Attachment #614913 - Flags: review?(surkov.alexander)
Attached patch Patch v.2.0 (obsolete) — Splinter Review
Attachment #614913 - Attachment is obsolete: true
Attachment #615088 - Flags: review?(surkov.alexander)
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
Attachment #615088 - Flags: review?(surkov.alexander)
Attachment #615088 - Flags: review+
Attachment #615088 - Flags: feedback?(trev.saunders)
> ::: 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 on attachment 615088 [details] [diff] [review]
Patch v.2.0

seems fine with comment 10 fixed
Attachment #615088 - Flags: feedback?(trev.saunders) → feedback+
(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?
(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
Attached patch Patch v.3.0Splinter Review
Attachment #615088 - Attachment is obsolete: true
Attachment #615458 - Flags: review?(surkov.alexander)
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
Attachment #615458 - Flags: review?(surkov.alexander) → review+
https://hg.mozilla.org/mozilla-central/rev/2745411f4bb9
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
(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
You need to log in before you can comment on or make changes to this bug.