Last Comment Bug 759310 - de-ns-ify nsHTMLTextAccessible file classes
: de-ns-ify nsHTMLTextAccessible file classes
Status: RESOLVED FIXED
:
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: densifya11y
  Show dependency treegraph
 
Reported: 2012-05-29 05:56 PDT by alexander :surkov
Modified: 2012-06-02 12:29 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (11.46 KB, patch)
2012-06-01 02:21 PDT, Mark Capella [:capella]
no flags Details | Diff | Review
Patch (v2) (11.55 KB, patch)
2012-06-01 03:25 PDT, Mark Capella [:capella]
no flags Details | Diff | Review
Patch (v3) (11.69 KB, patch)
2012-06-01 04:27 PDT, Mark Capella [:capella]
surkov.alexander: review+
Details | Diff | Review
Patch (final) (11.51 KB, patch)
2012-06-01 06:55 PDT, Mark Capella [:capella]
no flags Details | Diff | Review

Description alexander :surkov 2012-05-29 05:56:23 PDT
how about to rename the files to HTMLElementAccessibles.h/cpp ('s' on the end)? As follow up we can do that for base/nsBaseWidgetAccessible and xul/nsXULTextAccessible?
Comment 1 alexander :surkov 2012-05-30 18:19:49 PDT
Trevor, ping?
Comment 2 Mark Capella [:capella] 2012-05-31 22:43:55 PDT
Will we rename nsHTMLTextAccessible ...

contains two classes derived from nsLeafAccessible: 
class nsHTMLHRAccessible
class nsHTMLBRAccessible

and two classes derived from nsLeafAccessible: 
nsHyperTextAccessibleWrap
class nsHTMLLabelAccessible
class nsHTMLOutputAccessible

? Just remove the "ns" portion of the class names? Something further?
Comment 3 Trevor Saunders (:tbsaunde) 2012-05-31 22:49:02 PDT
(In reply to alexander :surkov from comment #1)
> Trevor, ping?

This doesn't seem like a high priority, comment 0 seems reasonable.  I'm not terribly a fan of html/HTMLBlah.{h,cpp} but the way do includes I don't have a better idea.
Comment 4 alexander :surkov 2012-05-31 22:52:10 PDT
(In reply to Mark Capella [:capella] from comment #2)
> Will we rename nsHTMLTextAccessible ...

yes (per comment #3)

> contains two classes derived from nsLeafAccessible: 
> class nsHTMLHRAccessible
> class nsHTMLBRAccessible
> 
> and two classes derived from nsLeafAccessible: 
> nsHyperTextAccessibleWrap
> class nsHTMLLabelAccessible
> class nsHTMLOutputAccessible
> 
> ? Just remove the "ns" portion of the class names? Something further?

yes/no
Comment 5 Mark Capella [:capella] 2012-05-31 22:53:19 PDT
thank you.
Comment 6 Mark Capella [:capella] 2012-06-01 02:21:07 PDT
Created attachment 629110 [details] [diff] [review]
Patch (v1)

Initial portion of the patch for review ...

For the followup:

  base/nsBaseWidgetAccessible will become base/BaseElementAccessibles?
    (dropping the "ns" from class nsLeafAccessible, class nsLinkableAccessible, and class nsEnumRoleAccessible)

  xul/nsXULTextAccessible will become xul/XULElementAccessibles?
     (dropping the "ns" from class nsXULTextAccessible, class nsXULTooltipAccessible, and class nsXULLinkAccessible)

And might you to go ahead and incorporate those changes here?
Comment 7 alexander :surkov 2012-06-01 02:37:24 PDT
Mark, please put them into mozilla::a11y namespace, it should be easy.
Comment 8 Mark Capella [:capella] 2012-06-01 03:25:26 PDT
Created attachment 629126 [details] [diff] [review]
Patch (v2)

Sure was! The classes with casting methods such as AsRoot(), AsDoc(), etc. are the ones that seem to get complicated. (Though I've done a couple of those without problems).

I'm still working to understand that.
Comment 9 alexander :surkov 2012-06-01 03:40:11 PDT
btw, don't forget to remove namespace stuffs like mozilla::a11y::role
Comment 10 Mark Capella [:capella] 2012-06-01 03:45:54 PDT
I've tried to remove mozilla::a11y::role from virtual mozilla::a11y::role NativeRole(); before but had trouble there also,  ... 

mxr.mozilla.org/mozilla-central/source/accessible/src/base/Role.h#10

Shows the enum in the roles namespace, which is like "under" a11y, so maybe it's just a matter of adding
namespace roles {
...
}
for that ?

Let me try it ...
Comment 11 alexander :surkov 2012-06-01 03:49:40 PDT
use a11y::role if role is ambiguous.
Comment 12 Mark Capella [:capella] 2012-06-01 03:52:36 PDT
Yep, that's what it took ... rebuilding / testing ....
Comment 13 Mark Capella [:capella] 2012-06-01 04:27:48 PDT
Created attachment 629141 [details] [diff] [review]
Patch (v3)

Latest and greatest ...
Comment 14 alexander :surkov 2012-06-01 04:33:00 PDT
Comment on attachment 629141 [details] [diff] [review]
Patch (v3)

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

::: accessible/src/html/nsHTMLTextAccessible.h
@@ +19,3 @@
>  {
>  public:
> +  HTMLHRAccessible(nsIContent* aContent, DocAccessible* aDoc);

if you don't mind then you can move implementation here hoping it's get inlined (same for other classes)
Comment 15 Mark Capella [:capella] 2012-06-01 06:55:13 PDT
Created attachment 629173 [details] [diff] [review]
Patch (final)

As pushed to try, headed to inbound eventually, see attached

Try :
https://tbpl.mozilla.org/?tree=Try&rev=da1838cb71aa
Comment 16 Mark Capella [:capella] 2012-06-02 05:14:17 PDT
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=d2624fac9886
Comment 17 :Ehsan Akhgari (out sick) 2012-06-02 12:29:41 PDT
https://hg.mozilla.org/mozilla-central/rev/b3856cc3e431

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