move out nsXULSelectableAccessible class from nsXULMenuAccessible.h file

RESOLVED FIXED in mozilla12

Status

()

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

People

(Reporter: surkov, Assigned: jhk)

Tracking

(Blocks: 1 bug, {access})

unspecified
mozilla12
access
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=trev.saunders@gmail.com][lang=c++])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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?
(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
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
Whiteboard: [good first bug][mentor=trev.saunders@gmail.com][lang=c++]
(Assignee)

Updated

6 years ago
Assignee: nobody → jigneshhk1992
(Assignee)

Comment 3

6 years ago
Created attachment 588369 [details] [diff] [review]
Patch
Attachment #588369 - Flags: review?(trev.saunders)
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
Attachment #588369 - Flags: review?(trev.saunders) → review+
(Assignee)

Comment 5

6 years ago
Created attachment 588413 [details] [diff] [review]
Patch
Attachment #588369 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/252d3d08fbc4
https://hg.mozilla.org/mozilla-central/rev/252d3d08fbc4
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
(Reporter)

Comment 8

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

Comment 10

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

Comment 12

6 years ago
Ok, I see, let's do not waste a time.
You need to log in before you can comment on or make changes to this bug.