481395 - get rid of nsAccessibilityAtoms in favor of nsGkAtoms

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

this name is shorter and therefore better ;)

rename structure and file names

Comment 1

[Trevor Saunders (:tbsaunde)]

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)]

Attachment: 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)]

Attachment: part2

get rid of the files for nsAccessibilityAtoms

Comment 4

[Trevor Saunders (:tbsaunde)]

Attachment: part 3

literally just ran
sed -i 's/nsAccessibilityAtoms/nsGkAtoms/g accesible/src/**/*.cpp accessible/src/**/*.h

Comment 5

[Trevor Saunders (:tbsaunde)]

Attachment: part 4

2 atoms had different names in nsGkAtomList than in nsAccessibility Atoms use the nsGkAtoms version

Comment 6

[alexander :surkov (:asurkov)]

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) No longer reading bugmail consistently]

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) No longer reading bugmail consistently]

(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 (:asurkov)]

(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 (:asurkov)]

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 (:asurkov)]

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) No longer reading bugmail consistently]

(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 (:asurkov)]

(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 (:asurkov)]

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 (:asurkov)]

Trevor, ping?

Comment 16

[Trevor Saunders (:tbsaunde)]

(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)]

> @@ +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)]

Attachment: make content/base/src/nsGkAtomList.h contain all atoms in nsAccessibilityAtomList.h

Comment 19

[Trevor Saunders (:tbsaunde)]

Attachment: remove nsAccessibilityAtom files and remove includes for nsAccessibilityAtoms.h

Comment 20

[Trevor Saunders (:tbsaunde)]

Attachment: s/nsAccessibilityAtoms/nsGkAtoms/g and fixups

Comment 21

[Trevor Saunders (:tbsaunde)]

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)]

(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] (NeedInfo me for attention)]

Please get a content person to review the content changes.

Comment 24

[David Bolter [:davidb] (NeedInfo me for attention)]

Ah I see we can forward Jonas's r+. OK.

Comment 25

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/d8e48951c3ef