Closed Bug 748719 Opened 9 years ago Closed 9 years ago

put ApplicationAccessible class into mozilla::a11y namespace

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: surkov, Assigned: capella)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file, 1 obsolete file)

was missed in bug 745740
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Depends on: 750295
Depends on: 750216
No longer depends on: 750295
Depends on: 750295
FYI, I'll remove accessible-docs.html during this patch instead of a new bug as mentioned in bug 748724 comment 13 :

"btw, would you mind to file a bug to remove accessible-docs.html from sources, it's out of date and all docs should be on devmo."
Attached patch Patch (v1) (obsolete) — Splinter Review
Asking mentor for initial feedback ...

Did a push to try ... I build on WIN but this affects builds cross platform ... not sure why just the OS X 10.7 opt went red ...
https://tbpl.mozilla.org/?tree=Try&rev=7df18c639c92
Attachment #622682 - Flags: feedback?(eitan)
Comment on attachment 622682 [details] [diff] [review]
Patch (v1)

It is hard to say why this fails to build on mac. Hub, would you mind trying?
Attachment #622682 - Flags: feedback?(eitan) → feedback?(hub)
Comment on attachment 622682 [details] [diff] [review]
Patch (v1)

it got restarted and worked. I don't know what happened. tbpl says "0 jobs failing"
Attachment #622682 - Flags: feedback?(hub) → feedback+
Comment on attachment 622682 [details] [diff] [review]
Patch (v1)

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

::: accessible/src/atk/ApplicationAccessibleWrap.h
@@ +38,5 @@
>   *
>   * ***** END LICENSE BLOCK ***** */
>  
> +#ifndef mozilla_a11y_Application_Accessible_Wrap_h__
> +#define mozilla_a11y_Application_Accessible_Wrap_h__

nit: mozilla_a11y_ApplicationAccessibleWrap_h__

@@ +66,2 @@
>  
> +  // return the atk object for app root accessible

please change it to 
/**
 * Return the atk object for app root accessible.
 */

::: accessible/src/base/nsAccessNode.cpp
@@ +60,5 @@
>  /* For documentation of the accessibility architecture, 
>   * see http://lxr.mozilla.org/seamonkey/source/accessible/accessible-docs.html
>   */
>  
> +mozilla::a11y::ApplicationAccessible* nsAccessNode::gApplicationAccessible = nsnull;

do you really need mozilla::a11y:: because there's using namespace mozilla::a11y?

::: accessible/src/generic/ApplicationAccessible.h
@@ +40,5 @@
>   *
>   * ***** END LICENSE BLOCK ***** */
>  
> +#ifndef mozilla_a11y_Application_Accessible_h__
> +#define mozilla_a11y_Application_Accessible_h__

same here: no '_' between Application and Accessible and in other files

::: accessible/src/msaa/nsAccessNodeWrap.cpp
@@ +173,5 @@
>    }
>  
>    // Can get to IAccessibleApplication from any node via QS
>    if (iid == IID_IAccessibleApplication) {
> +    mozilla::a11y::ApplicationAccessible* applicationAcc = 

you don't need mozilla::a11y, right?

@@ +175,5 @@
>    // Can get to IAccessibleApplication from any node via QS
>    if (iid == IID_IAccessibleApplication) {
> +    mozilla::a11y::ApplicationAccessible* applicationAcc = 
> +      reinterpret_cast<mozilla::a11y::ApplicationAccessible*>(GetApplicationAccessible());
> +      GetApplicationAccessible();

you forgot to remove GetApplicationAccessible.
I'm curious why do you need reinterpret_cast and casting at all
FYI re: you forgot to remove GetApplicationAccessible.
I'm curious why do you need reinterpret_cast and casting at all

I put this in while resolving (what turned out to be) unrelated build errors and forgot to remove it ... thanks for the tip :)
Attached patch Patch (v2)Splinter Review
Test of nits addressed ... builds and tests local to my WIN machine.
Attachment #622682 - Attachment is obsolete: true
Attachment #623417 - Flags: review?(surkov.alexander)
Attachment #623417 - Flags: review?(surkov.alexander) → review+
TRY test, pushing to inbound next

https://tbpl.mozilla.org/?tree=Try&rev=9ae02896fe7b
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/c02ce3b34b49
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.