Last Comment Bug 481395 - get rid of nsAccessibilityAtoms in favor of nsGkAtoms
: get rid of nsAccessibilityAtoms in favor of nsGkAtoms
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla9
Assigned To: Trevor Saunders (:tbsaunde)
:
Mentors:
Depends on:
Blocks: cleana11y
  Show dependency treegraph
 
Reported: 2009-03-04 05:19 PST by alexander :surkov
Modified: 2011-09-16 07:29 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part1 (4.22 KB, patch)
2011-06-06 20:51 PDT, Trevor Saunders (:tbsaunde)
jonas: review+
Details | Diff | Review
part2 (33.12 KB, patch)
2011-06-06 20:52 PDT, Trevor Saunders (:tbsaunde)
surkov.alexander: review+
Details | Diff | Review
part 3 (186.01 KB, patch)
2011-06-06 20:54 PDT, Trevor Saunders (:tbsaunde)
surkov.alexander: review+
Details | Diff | Review
part 4 (1.39 KB, patch)
2011-06-06 20:56 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Review
make content/base/src/nsGkAtomList.h contain all atoms in nsAccessibilityAtomList.h (3.52 KB, patch)
2011-08-17 02:13 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Review
remove nsAccessibilityAtom files and remove includes for nsAccessibilityAtoms.h (33.54 KB, patch)
2011-08-17 02:13 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Review
s/nsAccessibilityAtoms/nsGkAtoms/g and fixups (188.37 KB, patch)
2011-08-17 02:14 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Review

Description alexander :surkov 2009-03-04 05:19:08 PST
this name is shorter and therefore better ;)

rename structure and file names
Comment 1 Trevor Saunders (:tbsaunde) 2011-06-06 13:40:49 PDT
Lets use this bug to just get rid of them all Lets use this bug to consolidate  all of our atoms in nsGkAtoms.
Comment 2 Trevor Saunders (:tbsaunde) 2011-06-06 20:51:46 PDT
Created attachment 537726 [details] [diff] [review]
part1

add all the atoms accessibility defines to nsGkAtomList in a #ifdef ACCESSIBILITY block so there only used in builds with accessibility on.
Comment 3 Trevor Saunders (:tbsaunde) 2011-06-06 20:52:29 PDT
Created attachment 537727 [details] [diff] [review]
part2

get rid of the files for nsAccessibilityAtoms
Comment 4 Trevor Saunders (:tbsaunde) 2011-06-06 20:54:56 PDT
Created attachment 537728 [details] [diff] [review]
part 3

literally just ran
sed -i 's/nsAccessibilityAtoms/nsGkAtoms/g accesible/src/**/*.cpp accessible/src/**/*.h
Comment 5 Trevor Saunders (:tbsaunde) 2011-06-06 20:56:16 PDT
Created attachment 537729 [details] [diff] [review]
part 4

2 atoms had different names in nsGkAtomList than in nsAccessibility Atoms use the nsGkAtoms version
Comment 6 alexander :surkov 2011-06-06 21:20:43 PDT
Just wondering if we really want to not have own atoms at all. This makes us to change the code outside a11y whenever we need to change atoms (one more review is required). A11y specific atoms are instantiated not depending on whether a11y is running or not. Why we don't consider to kepp gk and a11y atoms both?
Comment 7 Jonas Sicking (:sicking) PTO Until July 5th 2011-06-06 21:53:52 PDT
Comment on attachment 537726 [details] [diff] [review]
part1

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

r=me with that.

::: content/base/src/nsGkAtomList.h
@@ +1917,5 @@
> +
> +  // Alphabetical list of frame types
> +  // unused needed for anything?
> +GK_ATOM(areaFrame, "AreaFrame")
> +GK_ATOM(inlineBlockFrame, "InlineBlockFrame")

Change these to AreaFrame and InlineBlockFrame
Comment 8 Jonas Sicking (:sicking) PTO Until July 5th 2011-06-06 21:58:42 PDT
(In reply to comment #6)
> Just wondering if we really want to not have own atoms at all. This makes us
> to change the code outside a11y whenever we need to change atoms (one more
> review is required).

You don't really need to ask anyone special for changes to just add or remove atoms in atom lists. 

> A11y specific atoms are instantiated not depending on
> whether a11y is running or not.

Given the very small number of additional atoms needed for accessibility, I think that's fine. It won't meaningfully slow down startup. Atom list instantiation is pretty optimized.

> Why we don't consider to kepp gk and a11y atoms both?

Saves binary and code size by not duplicating lists. Makes the accessibility code more similar to the rest of the code. (IMHO we should only have one atomlist. We're slowly moving in that direction).
Comment 9 alexander :surkov 2011-06-06 23:08:07 PDT
(In reply to comment #8)

> Saves binary and code size by not duplicating lists. Makes the accessibility
> code more similar to the rest of the code. (IMHO we should only have one
> atomlist. We're slowly moving in that direction).

Ok, fine with me. But is nsGkAtoms final name, will it be renamed in future (for example, cut 'ns' prefix)? If so then does it make sense to typedef it so that we can avoid future mass renames?
Comment 10 alexander :surkov 2011-06-08 19:37:08 PDT
Comment on attachment 537729 [details] [diff] [review]
part 4


> class nsFontSizeTextAttr : public nsTextAttr<nscoord>
> {
> public:
>   nsFontSizeTextAttr(nsIFrame *aRootFrame, nsIFrame *aFrame);
> 
>   // nsITextAttr
>-  virtual nsIAtom *GetName() { return nsGkAtoms::fontSize; }
>+  virtual nsIAtom *GetName() { return nsGkAtoms::font_size; }

this is a name of text attribute, you keep them separately in nsGkAtoms (like fontFamily), so you should add this one and do not share it with CSS name.
Comment 11 alexander :surkov 2011-06-08 19:45:21 PDT
Comment on attachment 537727 [details] [diff] [review]
part2

I'd prefer if this patch had removals of atoms initialization what listed in other patch by mistake I think. Anyway just thoughts, doens't affect on review since you're going fold patches into one.
Comment 12 Jonas Sicking (:sicking) PTO Until July 5th 2011-06-08 20:28:57 PDT
(In reply to comment #10)
> this is a name of text attribute, you keep them separately in nsGkAtoms
> (like fontFamily), so you should add this one and do not share it with CSS
> name.

Atoms don't have any semantics. They are effectively just strings expressed as a different data type. So no need to have a separate atom names for the same string.

In fact, I'd argue that you should merge all the "sections" that you are adding and just make it into one big list sorted by string contents (as to avoid people feeling the need to have multiple names for the same string).
Comment 13 alexander :surkov 2011-06-08 20:33:23 PDT
(In reply to comment #12)
> (In reply to comment #10)
> > this is a name of text attribute, you keep them separately in nsGkAtoms
> > (like fontFamily), so you should add this one and do not share it with CSS
> > name.
> 
> Atoms don't have any semantics. They are effectively just strings expressed
> as a different data type. So no need to have a separate atom names for the
> same string.

Trevor, then you should get rid dupes font-family, etc. Also I think it makes sense to name other properties like backgroundColor as background_color to be consistent.

> In fact, I'd argue that you should merge all the "sections" that you are
> adding and just make it into one big list sorted by string contents (as to
> avoid people feeling the need to have multiple names for the same string).

I like this
Comment 14 alexander :surkov 2011-06-09 01:31:53 PDT
Comment on attachment 537728 [details] [diff] [review]
part 3

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

r=me with comments addressed

::: accessible/src/base/nsAccessNode.cpp
@@ +212,5 @@
>      stringBundleService->CreateBundle(PLATFORM_KEYS_BUNDLE_URL, 
>                                        &gKeyStringBundle);
>    }
>  
> +  nsGkAtoms::AddRefAtoms();

You dont' need to double addref them, since I bet it must be done by content

::: accessible/src/base/nsAccessibilityService.cpp
@@ +275,1 @@
>                      mapElmName);

nit: put everything into one line please

::: accessible/src/base/nsAccessible.cpp
@@ +1433,1 @@
>                             value);

nit: keep on the same line

@@ +1435,5 @@
>    // Expose 'text-align' attribute.
>    rv = GetComputedStyleValue(EmptyString(), NS_LITERAL_STRING("text-align"),
>                               value);
>    if (NS_SUCCEEDED(rv))
> +    nsAccUtils::SetAccAttr(aAttributes, nsGkAtoms::textAlign,

the same

@@ +1447,1 @@
>                             value);

the same

@@ +1777,1 @@
>                             PR_TRUE);

reduce amount of lines

::: accessible/src/base/nsCoreUtils.cpp
@@ +562,5 @@
>      return PR_FALSE;
>  
> +  return aContent->AttrValueIs(kNameSpaceID_XLink, nsGkAtoms::type,
> +                               nsGkAtoms::simple, eCaseMatters) &&
> +         aContent->HasAttr(kNameSpaceID_XLink, nsGkAtoms::href);

two space indent relative indent of 'return'

@@ +574,5 @@
>  
>    nsIContent *walkUp = aContent;
>    while (walkUp && walkUp != aRootContent &&
>           !walkUp->GetAttr(kNameSpaceID_None,
> +                          nsGkAtoms::lang, aLanguage))

likely you can keep them on the same line

::: accessible/src/base/nsDocAccessible.cpp
@@ +1192,1 @@
>              eCaseMatters)))) {

please try to improve indentation here, it's has bad readability now

::: accessible/src/base/nsRootAccessible.cpp
@@ +524,1 @@
>                                        kNameSpaceID_XUL) : PR_FALSE;

you can keep on the same line I think

::: accessible/src/base/nsTextAttrs.h
@@ +246,5 @@
>    nsLangTextAttr(nsHyperTextAccessible *aRootAcc, nsIContent *aRootContent,
>                   nsIContent *aContent);
>  
>    // nsITextAttr
> +  virtual nsIAtom *GetName() { return nsGkAtoms::language; }

nit: while you're here, change to 'nsIAtom* GetName() const'

@@ +294,5 @@
>  public:
>    nsBGColorTextAttr(nsIFrame *aRootFrame, nsIFrame *aFrame);
>  
>    // nsITextAttr
> +  virtual nsIAtom *GetName() { return nsGkAtoms::backgroundColor; }

same

@@ +317,5 @@
>  public:
>    nsFontSizeTextAttr(nsIFrame *aRootFrame, nsIFrame *aFrame);
>  
>    // nsITextAttr
> +  virtual nsIAtom *GetName() { return nsGkAtoms::fontSize; }

same

@@ +349,5 @@
>  public:
>    nsFontWeightTextAttr(nsIFrame *aRootFrame, nsIFrame *aFrame);
>  
>    // nsITextAttr
> +  virtual nsIAtom *GetName() { return nsGkAtoms::fontWeight; }

same

::: accessible/src/html/nsHTMLFormControlAccessible.cpp
@@ +304,1 @@
>                           name)) {

you can keep 'name' on the same line

::: accessible/src/html/nsHTMLImageAccessible.cpp
@@ +235,5 @@
>  {
>    if (IsDefunct())
>      return PR_FALSE;
>  
> +  return mContent->HasAttr(kNameSpaceID_None, nsGkAtoms::longDesc);

this is fixed by follow up, right?

::: accessible/src/html/nsHTMLImageMapAccessible.cpp
@@ +164,1 @@
>                           aName)) {

aName on the same line?

::: accessible/src/html/nsHTMLTableAccessible.cpp
@@ +138,1 @@
>                           stringIdx);

can fit the same line?

::: accessible/src/xul/nsXULFormControlAccessible.cpp
@@ +636,1 @@
>                          name)) {

name on the same line please

::: accessible/src/xul/nsXULMenuAccessible.cpp
@@ +286,5 @@
>    PRUint64 state = nsAccessible::NativeState();
>  
>    // Focused?
>    if (mContent->HasAttr(kNameSpaceID_None,
> +                        nsGkAtoms::_moz_menuactive))

use existing menuactive atom

@@ +593,5 @@
>  
>  #ifdef DEBUG_A11Y
>    // We are onscreen if our parent is active
>    PRBool isActive = mContent->HasAttr(kNameSpaceID_None,
> +                                      nsGkAtoms::menuactive);

maybe I miss something but I don't understand how it worked because we don't have menuactive atom in nsAccessibilityAtoms list. Any idea?
Comment 15 alexander :surkov 2011-06-27 21:38:42 PDT
Trevor, ping?
Comment 16 Trevor Saunders (:tbsaunde) 2011-06-27 21:53:39 PDT
(In reply to comment #15)
> Trevor, ping?

It occured to me that I'll be much happier merging this on top of bug 641838 the relations bug  than I will be the other way around, so I'm going to wait for that to land before continueing here unless someone objects.
Comment 17 Trevor Saunders (:tbsaunde) 2011-08-13 21:46:34 PDT
> @@ +593,5 @@
> >  
> >  #ifdef DEBUG_A11Y
> >    // We are onscreen if our parent is active
> >    PRBool isActive = mContent->HasAttr(kNameSpaceID_None,
> > +                                      nsGkAtoms::menuactive);
> 
> maybe I miss something but I don't understand how it worked because we don't
> have menuactive atom in nsAccessibilityAtoms list. Any idea?

uh, your assuming it does, try arranging for that ifdef you see to be defined ;)  Why that stuff is in an ifdef for DEBUG_A11Y I have no idea, it happened atleast partially in bug 395400, but I haven't started reading that yet.
Comment 18 Trevor Saunders (:tbsaunde) 2011-08-17 02:13:22 PDT
Created attachment 553705 [details] [diff] [review]
make content/base/src/nsGkAtomList.h contain all atoms in nsAccessibilityAtomList.h
Comment 19 Trevor Saunders (:tbsaunde) 2011-08-17 02:13:49 PDT
Created attachment 553707 [details] [diff] [review]
remove nsAccessibilityAtom files and remove includes for nsAccessibilityAtoms.h
Comment 20 Trevor Saunders (:tbsaunde) 2011-08-17 02:14:17 PDT
Created attachment 553708 [details] [diff] [review]
s/nsAccessibilityAtoms/nsGkAtoms/g and fixups
Comment 21 Trevor Saunders (:tbsaunde) 2011-08-17 02:17:02 PDT
hopefully I got all the nits, I regenerated the script generated patch, rather than try to merge in the changes, but it should be the same patch just rebased and with nits fixed.  This passes mochitest-a11y locally, and assuming there's no objections to the nit pick fixes / merged changes I'd like to land this in the morning / when ever the tree opens.
Comment 22 Trevor Saunders (:tbsaunde) 2011-08-17 02:23:57 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #21)
> hopefully I got all the nits, I regenerated the script generated patch,
> rather than try to merge in the changes, but it should be the same patch
> just rebased and with nits fixed.  This passes mochitest-a11y locally, and
> assuming there's no objections to the nit pick fixes / merged changes I'd
> like to land this in the morning / when ever the tree opens.

(trying to minimize rebasing pain) I'd be willing to do a follow up patch if I missed anything non escential though
Comment 23 David Bolter [:davidb] 2011-08-24 10:12:59 PDT
Please get a content person to review the content changes.
Comment 24 David Bolter [:davidb] 2011-08-24 10:14:43 PDT
Ah I see we can forward Jonas's r+. OK.
Comment 25 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-16 07:29:40 PDT
https://hg.mozilla.org/mozilla-central/rev/d8e48951c3ef

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