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

RESOLVED FIXED in mozilla14

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: surkov, Assigned: Andrei Touzik)

Tracking

(Blocks: 1 bug)

unspecified
mozilla14
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

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

Comment 1

5 years ago
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.
(Assignee)

Comment 3

5 years ago
Could you assign it to me, please?

Updated

5 years ago
Assignee: nobody → xph9753
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 4

5 years ago
(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.
(Assignee)

Comment 6

5 years ago
Created attachment 614913 [details] [diff] [review]
Patch v.1.0
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.
(Reporter)

Comment 8

5 years ago
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)
(Assignee)

Comment 9

5 years ago
Created attachment 615088 [details] [diff] [review]
Patch v.2.0
Attachment #614913 - Attachment is obsolete: true
Attachment #615088 - Flags: review?(surkov.alexander)
(Reporter)

Comment 10

5 years ago
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+
(Assignee)

Comment 13

5 years ago
(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?
(Reporter)

Comment 14

5 years ago
(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
(Assignee)

Comment 15

5 years ago
Created attachment 615458 [details] [diff] [review]
Patch v.3.0
Attachment #615088 - Attachment is obsolete: true
Attachment #615458 - Flags: review?(surkov.alexander)
(Reporter)

Comment 16

5 years ago
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+
(Reporter)

Comment 17

5 years ago
landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/2745411f4bb9
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/2745411f4bb9
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 19

5 years ago
(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.