Closed Bug 481395 Opened 11 years ago Closed 8 years ago

get rid of nsAccessibilityAtoms in favor of nsGkAtoms

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: surkov, Assigned: tbsaunde)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 4 obsolete files)

this name is shorter and therefore better ;)

rename structure and file names
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
Attached patch part1 (obsolete) — Splinter Review
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)
Attached patch part2 (obsolete) — Splinter Review
get rid of the files for nsAccessibilityAtoms
Attachment #537727 - Flags: review?(surkov.alexander)
Attached patch part 3 (obsolete) — Splinter Review
literally just ran
sed -i 's/nsAccessibilityAtoms/nsGkAtoms/g accesible/src/**/*.cpp accessible/src/**/*.h
Attachment #537728 - Flags: review?(surkov.alexander)
Attached patch part 4 (obsolete) — Splinter Review
2 atoms had different names in nsGkAtomList than in nsAccessibility Atoms use the nsGkAtoms version
Attachment #537729 - Flags: review?(surkov.alexander)
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).
(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 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)
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).
(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 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+
Trevor, ping?
(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.
> @@ +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.
Attachment #537728 - Attachment is obsolete: true
Attachment #537729 - Attachment is obsolete: true
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.
(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
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
You need to log in before you can comment on or make changes to this bug.