Last Comment Bug 686433 - move out nsXULSelectableAccessible class from nsXULMenuAccessible.h file
: move out nsXULSelectableAccessible class from nsXULMenuAccessible.h file
Status: RESOLVED FIXED
[good first bug][mentor=trev.saunders...
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla12
Assigned To: Jignesh Kakadiya [:jhk]
:
Mentors:
Depends on:
Blocks: cleana11y
  Show dependency treegraph
 
Reported: 2011-09-13 02:18 PDT by alexander :surkov
Modified: 2012-01-20 02:46 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (31.71 KB, patch)
2012-01-13 03:38 PST, Jignesh Kakadiya [:jhk]
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review
Patch (31.40 KB, patch)
2012-01-13 07:46 PST, Jignesh Kakadiya [:jhk]
no flags Details | Diff | Splinter Review

Description alexander :surkov 2011-09-13 02:18:03 PDT
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) 2011-09-15 17:45:27 PDT
(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) 2012-01-07 10:13:35 PST
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] 2012-01-13 03:38:21 PST
Created attachment 588369 [details] [diff] [review]
Patch
Comment 4 Trevor Saunders (:tbsaunde) 2012-01-13 06:11:14 PST
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] 2012-01-13 07:46:22 PST
Created attachment 588413 [details] [diff] [review]
Patch
Comment 6 Trevor Saunders (:tbsaunde) 2012-01-16 22:13:12 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/252d3d08fbc4
Comment 7 Marco Bonardo [::mak] 2012-01-17 07:43:00 PST
https://hg.mozilla.org/mozilla-central/rev/252d3d08fbc4
Comment 8 alexander :surkov 2012-01-18 10:45:10 PST
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 (aaronl@netscape.com)
> + *   Jignesh Kakadiya (jigneshhk1992@gmail.com)

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) 2012-01-18 11:25:30 PST
(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 (aaronl@netscape.com)
> > + *   Jignesh Kakadiya (jigneshhk1992@gmail.com)
> 
> 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 2012-01-19 01:28:19 PST
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) 2012-01-19 06:25:14 PST
(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 2012-01-20 02:46:51 PST
Ok, I see, let's do not waste a time.

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