Last Comment Bug 739190 - change AppendTarget(nsIContent* aContent) to take nsDocAccessible* argument
: change AppendTarget(nsIContent* aContent) to take nsDocAccessible* argument
Status: RESOLVED FIXED
[good first bug][mentor=eitan@monoton...
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Mark Capella [:capella]
:
Mentors:
Depends on:
Blocks: 725572
  Show dependency treegraph
 
Reported: 2012-03-26 05:30 PDT by alexander :surkov
Modified: 2012-05-08 03:23 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (996 bytes, patch)
2012-04-01 09:32 PDT, Ye Kaiqi
no flags Details | Diff | Splinter Review
Patch (v1) (6.27 KB, patch)
2012-05-05 18:26 PDT, Mark Capella [:capella]
surkov.alexander: review+
eitan: feedback+
Details | Diff | Splinter Review

Description alexander :surkov 2012-03-26 05:30:19 PDT
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 Ye Kaiqi 2012-04-01 09:32:10 PDT
Created attachment 611287 [details] [diff] [review]
patch

Need some comments, thanks!
Comment 2 Trevor Saunders (:tbsaunde) 2012-04-01 11:36:12 PDT
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.
Comment 3 Oussama BADR 2012-04-19 02:25:28 PDT
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!
Comment 4 Mark Capella [:capella] 2012-05-05 18:26:22 PDT
Created attachment 621364 [details] [diff] [review]
Patch (v1)

Requesting feedback from mentor ... 

This patch builds and mochitests successfully locally.
Comment 5 Eitan Isaacson [:eeejay] 2012-05-06 18:43:14 PDT
Comment on attachment 621364 [details] [diff] [review]
Patch (v1)

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

Looks straightforward. Thanks!
Comment 6 Mark Capella [:capella] 2012-05-06 18:49:34 PDT
Comment on attachment 621364 [details] [diff] [review]
Patch (v1)

Thank you !
Comment 7 alexander :surkov 2012-05-07 04:34:46 PDT
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)
Comment 9 Ed Morley [:emorley] 2012-05-08 03:23:13 PDT
https://hg.mozilla.org/mozilla-central/rev/f593f893ebd7

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