get rid of nsAccessibilityAtoms in favor of nsGkAtoms

RESOLVED FIXED in mozilla9

Status

()

Core
Disability Access APIs
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: surkov, Assigned: tbsaunde)

Tracking

(Blocks: 1 bug)

unspecified
mozilla9
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

8 years ago
this name is shorter and therefore better ;)

rename structure and file names
(Assignee)

Comment 1

6 years ago
Lets use this bug to just get rid of them all Lets use this bug to consolidate  all of our atoms in nsGkAtoms.
Assignee: nobody → trev.saunders
Summary: rename nsAccessibilityAtoms to nsAccAtoms → get rid of nsAccessibilityAtoms in favor of nsGkAtoms
(Assignee)

Comment 2

6 years ago
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.
Attachment #537726 - Flags: review?(jonas)
(Assignee)

Comment 3

6 years ago
Created attachment 537727 [details] [diff] [review]
part2

get rid of the files for nsAccessibilityAtoms
Attachment #537727 - Flags: review?(surkov.alexander)
(Assignee)

Comment 4

6 years ago
Created attachment 537728 [details] [diff] [review]
part 3

literally just ran
sed -i 's/nsAccessibilityAtoms/nsGkAtoms/g accesible/src/**/*.cpp accessible/src/**/*.h
Attachment #537728 - Flags: review?(surkov.alexander)
(Assignee)

Comment 5

6 years ago
Created attachment 537729 [details] [diff] [review]
part 4

2 atoms had different names in nsGkAtomList than in nsAccessibility Atoms use the nsGkAtoms version
Attachment #537729 - Flags: review?(surkov.alexander)
(Reporter)

Comment 6

6 years ago
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 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
Attachment #537726 - Flags: review?(jonas) → review+
(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).
(Reporter)

Comment 9

6 years ago
(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?
(Reporter)

Comment 10

6 years ago
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.
Attachment #537729 - Flags: review?(surkov.alexander)
(Reporter)

Comment 11

6 years ago
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.
Attachment #537727 - Flags: review?(surkov.alexander) → review+
(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).
(Reporter)

Comment 13

6 years ago
(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
(Reporter)

Comment 14

6 years ago
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?
Attachment #537728 - Flags: review?(surkov.alexander) → review+
(Reporter)

Comment 15

6 years ago
Trevor, ping?
(Assignee)

Comment 16

6 years ago
(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.
(Assignee)

Comment 17

6 years ago
> @@ +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.
(Assignee)

Comment 18

6 years ago
Created attachment 553705 [details] [diff] [review]
make content/base/src/nsGkAtomList.h contain all atoms in nsAccessibilityAtomList.h
Attachment #537726 - Attachment is obsolete: true
(Assignee)

Comment 19

6 years ago
Created attachment 553707 [details] [diff] [review]
remove nsAccessibilityAtom files and remove includes for nsAccessibilityAtoms.h
Attachment #537727 - Attachment is obsolete: true
(Assignee)

Comment 20

6 years ago
Created attachment 553708 [details] [diff] [review]
s/nsAccessibilityAtoms/nsGkAtoms/g and fixups
Attachment #537728 - Attachment is obsolete: true
Attachment #537729 - Attachment is obsolete: true
(Assignee)

Comment 21

6 years ago
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.
(Assignee)

Comment 22

6 years ago
(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
Please get a content person to review the content changes.
Ah I see we can forward Jonas's r+. OK.
https://hg.mozilla.org/mozilla-central/rev/d8e48951c3ef
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
You need to log in before you can comment on or make changes to this bug.