change AppendTarget(nsIContent* aContent) to take nsDocAccessible* argument

RESOLVED FIXED in mozilla15

Status

()

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

People

(Reporter: surkov, Assigned: capella)

Tracking

unspecified
mozilla15
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=eitan@monotonous.org][lang=c++])

Attachments

(1 attachment, 1 obsolete attachment)

6.27 KB, patch
surkov
: review+
eeejay
: feedback+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
change AppendTarget(nsIContent* aContent) (see http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/Relation.h#123) to AppendTarget(nsDocAccessible* aDocument, nsIContent* aContent) and use nsDocAccessible to get an accessible for the content instead GetAccService().

Comment 1

5 years ago
Created attachment 611287 [details] [diff] [review]
patch

Need some comments, thanks!
Attachment #611287 - Flags: feedback?
Comment on attachment 611287 [details] [diff] [review]
patch

>     if (aContent)
>-      AppendTarget(GetAccService()->GetAccessible(aContent, nsnull));
>+      AppendTarget(aDocument->GetAccessible(aContent, nsnull));

nsDocAccessible::GetAccessible() only takes one argument.

you might also want to update the callers.
Attachment #611287 - Flags: feedback?

Comment 3

5 years ago
Hi everyone!

Can I continue what Kaigi began ? 

If it's permitted, so I will recapitulate, the bug is about fixing  AppendTarget as Surkov detailed in the first comment,
and after Kaigi's patch and Trevor's comment the problem was fixed a little bit but we should update nsDocAccessible::GetAccessible(), right?

thanks!
(Assignee)

Comment 4

5 years ago
Created attachment 621364 [details] [diff] [review]
Patch (v1)

Requesting feedback from mentor ... 

This patch builds and mochitests successfully locally.
Assignee: nobody → markcapella
Attachment #611287 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #621364 - Flags: feedback?(eitan)
Comment on attachment 621364 [details] [diff] [review]
Patch (v1)

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

Looks straightforward. Thanks!
Attachment #621364 - Flags: feedback?(eitan) → feedback+
(Assignee)

Comment 6

5 years ago
Comment on attachment 621364 [details] [diff] [review]
Patch (v1)

Thank you !
Attachment #621364 - Flags: review?(surkov.alexander)
(Reporter)

Comment 7

5 years ago
Comment on attachment 621364 [details] [diff] [review]
Patch (v1)

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

::: accessible/src/base/Relation.h
@@ +119,5 @@
>    /**
>     * Append the one accessible for this content node to the set of related
>     * accessibles.
>     */
> +  inline void AppendTarget(nsDocAccessible* aDocument, nsIContent* aContent)

nit: you don't need 'inline' (i'll fix it before landing)
Attachment #621364 - Flags: review?(surkov.alexander) → review+
(Reporter)

Comment 8

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f593f893ebd7

Comment 9

5 years ago
https://hg.mozilla.org/mozilla-central/rev/f593f893ebd7
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
You need to log in before you can comment on or make changes to this bug.