741707 - rm unused a11y CIDs

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Trevor Saunders (:tbsaunde)]

remove the NS_CLASS_CID / IMPL_CID macros for each of nsDocAccessible, nsRootAccessile, nsHTMLTableAccessible and nsXULTreeAccessible

all of the files are in the accessible/src/ directory.

Comment 1

[Mark Capella [:capella]]

Attachment: Patch (v1) very rough

Ok, I thought this was going to be a good and quick cleanup patch ... I pulled the _IMPL_CID vars from the nsRootAccessible.h and nsXULTreeAccessible.h files and build with no problems.

Then I tried to perform the same changes to nsDocAccessible.h and received an odd build error. (See attached).

Apparently there is more to removing these definitions than I know ... can you elucidate me on what I'm missing?

Thanks -- mark

Comment 2

[Mark Capella [:capella]]

Attachment: Odd Build Error

Comment 3

[Hubert Figuiere [:hub]]

Comment on attachment 613410 [details] [diff] [review]
Patch (v1) very rough

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

It definitely does not build on Linux either.

::: accessible/src/base/nsRootAccessible.cpp
@@ -97,5 @@
>  NS_IMPL_QUERY_HEAD(nsRootAccessible)
>  NS_IMPL_QUERY_BODY(nsIDOMEventListener)
> -if (aIID.Equals(NS_GET_IID(nsRootAccessible)))
> -  foundInterface = reinterpret_cast<nsISupports*>(this);
> -else

I do believe that you should NOT remove this. It is need to access the CID that is being inherited.

Comment 4

[Mark Capella [:capella]]

Ok, I may have to ask Alex to look here then ... this piece was required to be removed to allow the nsRootAccessible.cpp to build after pulling it's headers CID's.

Comment 5

[Hubert Figuiere [:hub]]

Please forget that last comment.

Comment 6

[Hubert Figuiere [:hub]]

(In reply to Mark Capella [:capella] from comment #4)
> Ok, I may have to ask Alex to look here then ... this piece was required to
> be removed to allow the nsRootAccessible.cpp to build after pulling it's
> headers CID's.

You can still ask him. My comment was wrong.

Comment 7

[Trevor Saunders (:tbsaunde)]

I'm tired, but that looks fine other than you need the dependancy bug 741709 fixed.  I'm suprised this didn't build on linux since we don't build that file there.

Comment 8

[Mark Capella [:capella]]

Attachment: Patch (v2) still rough

Ok, after making the requested changes code changes to the four header files, I had to make a few other (creative) changes to get a clean build, but this runs and passes all mochitests-a11y locally.

Please review it carefully. I'm really not sure if I cut too much out trying to achieve the desired results.

Comment 9

[Hubert Figuiere [:hub]]

This one works much better. Let me see.

Comment 10

[Hubert Figuiere [:hub]]

Comment on attachment 613492 [details] [diff] [review]
Patch (v2) still rough

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

So far so good

Comment 11

[alexander :surkov (:asurkov)]

Comment on attachment 613492 [details] [diff] [review]
Patch (v2) still rough

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

you forgot to update nsXULTreeAccessible.cpp part

you remove nsXULTreeItemAccessibleBase but don't get rid its usecases (for example nsXULTreeGridCellAccessible::GetSiblingAtOffset)

do I understand right that nsXULTreeGridAccessible is supposed for another bug?

(if you are going to remove that for nsXULTreeGridRowAccessible then make sure to remove NS_INTERFACE_MAP_STATIC_AMBIGUOUS from a11yGeneric.h)

::: accessible/src/base/nsRootAccessible.cpp
@@ +99,4 @@
>  NS_IMPL_QUERY_TAIL_INHERITING(nsDocAccessible)
>  
>  NS_IMPL_ADDREF_INHERITED(nsRootAccessible, nsDocAccessible) 
>  NS_IMPL_RELEASE_INHERITED(nsRootAccessible, nsDocAccessible)

use NS_IMPL_ISUPPORTS_INHERITED1

Comment 12

[Mark Capella [:capella]]

Re: you forgot to update nsXULTreeAccessible.cpp part

I thought mostly all I needed was to removed _CID macros ... not sure what code can then be pulled in addition to that. Can you explain a little more here please?

Re: you remove nsXULTreeItemAccessibleBase but don't get rid its usecases (for example nsXULTreeGridCellAccessible::GetSiblingAtOffset) do I understand right that nsXULTreeGridAccessible is supposed for another bug? (if you are going to remove that for nsXULTreeGridRowAccessible then make sure to remove NS_INTERFACE_MAP_STATIC_AMBIGUOUS from a11yGeneric.h)

No, this was overkill on my part, not for this bug and I replaced those _CID items.

Re: accessible/src/base/nsRootAccessible.cpp, use NS_IMPL_ISUPPORTS_INHERITED1

The changes I already made involving _INHERITED macros up to this point was a hit or miss thing ... I'm not really sure how these work, so I don't know how to change this piece as you request. Can you provide more detail, or is there documentation I can read?

Comment 13

[Mark Capella [:capella]]

Attachment: Patch (v3) rough

Forgot to post my next best-guess ...

Comment 14

[alexander :surkov (:asurkov)]

(In reply to Mark Capella [:capella] from comment #12)
> Re: you forgot to update nsXULTreeAccessible.cpp part
> 
> I thought mostly all I needed was to removed _CID macros ... not sure what
> code can then be pulled in addition to that. Can you explain a little more
> here please?

same thing you did for nsDocAccessible for exampe (fix QueryInterface).

> Re: you remove nsXULTreeItemAccessibleBase but don't get rid its usecases
> (for example nsXULTreeGridCellAccessible::GetSiblingAtOffset) do I
> understand right that nsXULTreeGridAccessible is supposed for another bug?
> (if you are going to remove that for nsXULTreeGridRowAccessible then make
> sure to remove NS_INTERFACE_MAP_STATIC_AMBIGUOUS from a11yGeneric.h)
> 
> No, this was overkill on my part, not for this bug and I replaced those _CID
> items.

replaced? what do you mean? anyway, you remove nsXULTreeItemAccessibleBase cid then you should make sure nobody queries it.

> Re: accessible/src/base/nsRootAccessible.cpp, use
> NS_IMPL_ISUPPORTS_INHERITED1
> 
> The changes I already made involving _INHERITED macros up to this point was
> a hit or miss thing ... I'm not really sure how these work, so I don't know
> how to change this piece as you request. Can you provide more detail, or is
> there documentation I can read?

just use http://mxr.mozilla.org/mozilla-central/ to find these macros. I think that's the best way of learning to look at sources.

Comment 15

[Mark Capella [:capella]]

Attachment: Patch (v4)

Latest try - builds and tests ok locally ...

Comment 16

[alexander :surkov (:asurkov)]

Comment on attachment 613541 [details] [diff] [review]
Patch (v4)

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

looks ok

Comment 17

[Hubert Figuiere [:hub]]

I'll check it in

Comment 18

[Hubert Figuiere [:hub]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea2aa8357d28

Comment 19

[Hubert Figuiere [:hub]]

backed out. Will re-land.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1df10e8b5ce2

Comment 20

[Hubert Figuiere [:hub]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f9a538375c8a

Comment 21

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/f9a538375c8a