Last Comment Bug 737379 - change Comments to roles::ROLENAME
: change Comments to roles::ROLENAME
Status: RESOLVED FIXED
[good first bug][mentor=markcapella@t...
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- trivial (vote)
: mozilla14
Assigned To: Nagarjuna Varma [:Infinity]
:
Mentors:
Depends on: 743114
Blocks: cleana11y
  Show dependency treegraph
 
Reported: 2012-03-20 04:06 PDT by Mark Capella [:capella]
Modified: 2012-04-05 18:39 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
First Patch. (20.86 KB, patch)
2012-03-29 14:54 PDT, Nagarjuna Varma [:Infinity]
no flags Details | Diff | Review
Updated Patch1 (20.92 KB, patch)
2012-04-01 00:18 PDT, Nagarjuna Varma [:Infinity]
surkov.alexander: review+
markcapella: feedback+
Details | Diff | Review

Description Mark Capella [:capella] 2012-03-20 04:06:57 PDT
INCLUDE file has comments that could be made more clear. 

See ::: accessible/src/mac/nsRoleMap.h and the below for example ...

@@ +166,5 @@
>    NSAccessibilityGroupRole,                     // ROLE_GRID_CELL
>    NSAccessibilityGroupRole,                     // ROLE_EMBEDDED_OBJECT
>    NSAccessibilityGroupRole,                     // ROLE_NOTE
>    NSAccessibilityGroupRole,                     // ROLE_FIGURE
> +  NSAccessibilityCheckBoxRole,                  // ROLE_CHECK_RICH_OPTION
Comment 1 Mark Capella [:capella] 2012-03-29 09:44:02 PDT
At the bottom of the file, accessible/src/mac/nsRoleMap.h has comments like:
// ROLE_CHECK_RICH_OPTION

At the bottom of accessible/src/atk/nsRoleMap.h has comments like:
// roles::CHECK_RICH_OPTION    125

for the matching role definition.

Make mac/ version comments look like the atk/ version

Usually you'll just change // ROLE_foo to be // roles::foo
Comment 2 Nagarjuna Varma [:Infinity] 2012-03-29 14:54:36 PDT
Created attachment 610709 [details] [diff] [review]
First Patch.
Comment 3 Mark Capella [:capella] 2012-03-29 15:19:00 PDT
In the patch, you inserted two lines .... (title then a blank line) ... before the first role ... please remove them

+                                                // Cross Platform Roles        #      Description
+
+  NSAccessibilityUnknownRole,                   // roles::NOTHING              0
+  NSAccessibilityUnknownRole,                   // roles::TITLEBAR             1      (irrelevant on OS X; windows are always native.) 


My version of the nsrolemap.h file (before any changes you made) ends with these three entries ... your seems to have lost the CHECK_RICH_ OPTION entry ... did you start from the right file?

  NSAccessibilityGroupRole,                     // ROLE_FIGURE
  NSAccessibilityCheckBoxRole,                  // ROLE_CHECK_RICH_OPTION
  @"ROLE_LAST_ENTRY"                            // ROLE_LAST_ENTRY. bogus role th


Let me know ...
Comment 4 Nagarjuna Varma [:Infinity] 2012-04-01 00:18:41 PDT
Created attachment 611249 [details] [diff] [review]
Updated Patch1
Comment 5 Mark Capella [:capella] 2012-04-01 00:26:24 PDT
Looks good with the previous requested changes, and the proper start file ..

You did a local build with this new .h file and that worked ok?
You ran the whole mochitest-a11y suite locally successfully?
Comment 6 alexander :surkov 2012-04-01 20:43:08 PDT
Comment on attachment 611249 [details] [diff] [review]
Updated Patch1

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

r=me, thank you (I'll fix nit before landing)

::: accessible/src/mac/nsRoleMap.h
@@ +88,5 @@
> +  NSAccessibilityButtonRole,                    // roles::PUSHBUTTON           43
> +  NSAccessibilityCheckBoxRole,                  // roles::CHECKBUTTON          44
> +  NSAccessibilityRadioButtonRole,               // roles::RADIOBUTTON          45
> +  NSAccessibilityPopUpButtonRole,               // roles::COMBOBOX             46
> +  NSAccessibilityPopUpButtonRole,               // roles::DROPLIST.            47

nit: get rid dot in the end
Comment 8 Matt Brubeck (:mbrubeck) 2012-04-02 11:08:39 PDT
This patch has landed in mozilla-central for Firefox 14, and will be included in tomorrow's nightly build.  Thank you!
https://hg.mozilla.org/mozilla-central/rev/0296cdb4cb2b
Comment 9 Nagarjuna Varma [:Infinity] 2012-04-03 12:21:22 PDT
Its been a pleasure to work with you guys! Glad to see it out there.
Comment 10 alexander :surkov 2012-04-03 21:45:01 PDT
(In reply to Arjun from comment #9)
> Its been a pleasure to work with you guys! Glad to see it out there.

yep, nice work, thank you. If you like then please take another bug 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;list_id=2709998;status_whiteboard_type=allwordssubstr;query_format=advanced
Comment 11 Nagarjuna Varma [:Infinity] 2012-04-04 08:34:10 PDT
You are welcome!
I will check the list out and let you know if I am interested in anything.
Comment 12 alexander :surkov 2012-04-04 08:35:42 PDT
(In reply to Arjun from comment #11)
> You are welcome!
> I will check the list out and let you know if I am interested in anything.

ok, thanks again!
Comment 13 Hubert Figuiere [:hub] 2012-04-05 18:13:02 PDT
And this patch reverted a change made in changeset: 90162:fc9658fae44b

*sigh*

I was wondering why suddenly it was broken.

Bug 743114

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