Last Comment Bug 748719 - put ApplicationAccessible class into mozilla::a11y namespace
: put ApplicationAccessible class into mozilla::a11y namespace
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: 750216 750295
Blocks: cleana11y
  Show dependency treegraph
 
Reported: 2012-04-25 04:41 PDT by alexander :surkov
Modified: 2012-05-13 17:41 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (61.07 KB, patch)
2012-05-10 04:25 PDT, Mark Capella [:capella]
hub: feedback+
Details | Diff | Splinter Review
Patch (v2) (59.46 KB, patch)
2012-05-12 05:41 PDT, Mark Capella [:capella]
surkov.alexander: review+
Details | Diff | Splinter Review

Description alexander :surkov 2012-04-25 04:41:21 PDT
was missed in bug 745740
Comment 1 Mark Capella [:capella] 2012-05-10 03:46:58 PDT
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."
Comment 2 Mark Capella [:capella] 2012-05-10 04:25:30 PDT
Created attachment 622682 [details] [diff] [review]
Patch (v1)

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
Comment 3 Eitan Isaacson [:eeejay] 2012-05-10 11:03:05 PDT
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?
Comment 4 Hubert Figuiere [:hub] 2012-05-10 11:08:29 PDT
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"
Comment 5 alexander :surkov 2012-05-10 20:40:08 PDT
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
Comment 6 Mark Capella [:capella] 2012-05-12 04:53:03 PDT
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 :)
Comment 7 Mark Capella [:capella] 2012-05-12 05:41:03 PDT
Created attachment 623417 [details] [diff] [review]
Patch (v2)

Test of nits addressed ... builds and tests local to my WIN machine.
Comment 8 Mark Capella [:capella] 2012-05-12 13:01:57 PDT
TRY test, pushing to inbound next

https://tbpl.mozilla.org/?tree=Try&rev=9ae02896fe7b
Comment 9 Mark Capella [:capella] 2012-05-12 13:16:12 PDT
fingers crossed ...

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=c02ce3b34b49
Comment 10 Matt Brubeck (:mbrubeck) 2012-05-13 17:41:09 PDT
https://hg.mozilla.org/mozilla-central/rev/c02ce3b34b49

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