686433 - move out nsXULSelectableAccessible class from nsXULMenuAccessible.h file

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Give nsXULSelectableAccessible a proper name and move it into separate file.

Options
1) XULSelectAccessible can be used since internal interface it implements is referred as SelectAccessible but that doesn't correspond to class names well in XUL directory
2) XULSelectControlAccessible may be better because it points that derived classes are for XUL elements that implements corresponding XUL interface.

Trevor, what do you think?

Comment 1

[Trevor Saunders (:tbsaunde)]

(In reply to alexander surkov from comment #0)
> Give nsXULSelectableAccessible a proper name and move it into separate file.
> 
> Options
> 1) XULSelectAccessible can be used since internal interface it implements is
> referred as SelectAccessible but that doesn't correspond to class names well
> in XUL directory
> 2) XULSelectControlAccessible may be better because it points that derived
> classes are for XUL elements that implements corresponding XUL interface.

Well, 2 seems to make more sense, my only real complaint is that its a bit longer

Comment 2

[Trevor Saunders (:tbsaunde)]

shouldn't be too hard.

1. create accessible/src/xul/XULSelectControlAccessible.h and move the nsXULSelectableAccessible class into it.
2. move method implementations from nsXULMenuAccessible.cpp to a new XULSelectControlAccessible.cpp
3. rename from nsXULSelectableAccessible to XULSelectControlAccessible
4. fix up classes that inherit from it including macros for nsISupports impl and cycle collection

Comment 3

[Jignesh Kakadiya [:jhk]]

Attachment: Patch

Comment 4

[Trevor Saunders (:tbsaunde)]

Comment on attachment 588369 [details] [diff] [review]
Patch

>+++ b/accessible/src/xul/Makefile.in
>@@ -56,16 +56,17 @@ CPPSRCS = \
>   nsXULFormControlAccessible.cpp \
>   nsXULListboxAccessible.cpp \
>   nsXULMenuAccessible.cpp \
>   nsXULSliderAccessible.cpp \
>   nsXULTabAccessible.cpp \
>   nsXULTextAccessible.cpp \
>   nsXULTreeAccessible.cpp \
>   nsXULTreeGridAccessible.cpp \
>+  XULSelectControlAccessible.cpp \

this should be sorted in A ... Z a ... z order so it should come before all the nsXULFoo not after

>+
>+#include "nsAccessibilityService.h"
>+#include "nsAccUtils.h"
>+#include "nsDocAccessible.h"
>+#include "nsXULFormControlAccessible.h"
>+#include "States.h"

are any of these last 4 actually used?

>+
>+#include "nsIDOMElement.h"
>+#include "nsIDOMXULElement.h"
>+#include "nsIMutableArray.h"

should come after all of the nsIDOMFoo.h

>+#include "nsIDOMXULContainerElement.h"
>+#include "nsIDOMXULSelectCntrlItemEl.h"
>+#include "nsIDOMXULMultSelectCntrlEl.h"
>+#include "nsIDOMKeyEvent.h"
>+#include "nsIServiceManager.h"
>+#include "nsIPresShell.h"
>+#include "nsIContent.h"
>+#include "nsGUIEvent.h"

are these last three used?

>+#include "nsMenuBarFrame.h"
>+#include "nsMenuPopupFrame.h"

same

>+
>+#include "mozilla/Preferences.h"
>+#include "mozilla/LookAndFeel.h"
same

>+  }
>+  else {  // Single select?
>+    nsCOMPtr<nsIDOMXULSelectControlItemElement> itemElm;

} else {

>+#ifndef _XULSelectControlAccessible_H_
>+#define _XULSelectControlAccessible_H_
>+
>+#include "nsAccessibleWrap.h"
>+#include "nsIDOMXULSelectCntrlEl.h"'

is that needed, or would a forward declaration work?

>+
>+/**
>+ * The basic implementation of SelectAccessible for XUL select controls.

how about

The basic implementation of accessible selection interface for XUL controls.

r=me with that addressed

Comment 5

[Jignesh Kakadiya [:jhk]]

Attachment: Patch

Comment 6

[Trevor Saunders (:tbsaunde)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/252d3d08fbc4

Comment 7

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/252d3d08fbc4

Comment 8

[alexander :surkov (:asurkov)]

Comment on attachment 588413 [details] [diff] [review]
Patch

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

::: accessible/src/xul/XULSelectControlAccessible.cpp
@@ +20,5 @@
> + * the Initial Developer. All Rights Reserved.
> + *
> + * Contributor(s):
> + *   Aaron Leventhal ([e-mail])
> + *   Jignesh Kakadiya ([e-mail])

Aaron disappeared from landed version, I wonder why? Copy/paste into another file doesn't sound like a good reason to do this.

Comment 9

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #8)
> Comment on attachment 588413 [details] [diff] [review]
> Patch
> 
> Review of attachment 588413 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/xul/XULSelectControlAccessible.cpp
> @@ +20,5 @@
> > + * the Initial Developer. All Rights Reserved.
> > + *
> > + * Contributor(s):
> > + *   Aaron Leventhal ([e-mail])
> > + *   Jignesh Kakadiya ([e-mail])
> 
> Aaron disappeared from landed version, I wonder why? Copy/paste into another
> file doesn't sound like a good reason to do this.

This is where this file level contributors thing becomes a pain :/ (note when we upgrade to mpl 2.0 it goes away).  I think I may have thought that it made sense to say contributors should be related to file name, but saying they should be arried over when code is cut and paste is sort of reasonable, though I'm not sure where the limit here is.  In any case feel free to add it if your like or I can if you want for the couple weeks before we do the mpl 2.0 upgrade.

Comment 10

[alexander :surkov (:asurkov)]

copy/paste doesn't necessary make you a contributor really because obviously you might not know the copied code, also it shouldn't change (original author) mark (which was missed in original files). Don't you know why mpl 2.0 upgrade is going to get rid contributors and authors?

Comment 11

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #10)
> copy/paste doesn't necessary make you a contributor really because obviously
> you might not know the copied code, also it shouldn't change (original
> author) mark (which was missed in original files). Don't you know why mpl
> 2.0 upgrade is going to get rid contributors and authors?

my gues would be that hg / git blaim does the same sort of thing but without manuel effort so better, but I don't actually know.

Comment 12

[alexander :surkov (:asurkov)]

Ok, I see, let's do not waste a time.