The default bug view has changed. See this FAQ.

rm unused a11y CIDs

RESOLVED FIXED in mozilla14

Status

()

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

People

(Reporter: tbsaunde, Assigned: capella)

Tracking

unspecified
mozilla14
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=hub@mozilla.com][lang=c++])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

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

Updated

5 years ago
Depends on: 741709
(Assignee)

Comment 1

5 years ago
Created attachment 613410 [details] [diff] [review]
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
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attachment #613410 - Flags: feedback?(hub)
(Assignee)

Comment 2

5 years ago
Created attachment 613411 [details]
Odd Build Error
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.
Attachment #613410 - Flags: feedback?(hub) → feedback-
(Assignee)

Comment 4

5 years ago
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.
Please forget that last comment.
(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.
(Reporter)

Comment 7

5 years ago
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.
(Assignee)

Comment 8

5 years ago
Created attachment 613492 [details] [diff] [review]
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.
Attachment #613410 - Attachment is obsolete: true
Attachment #613411 - Attachment is obsolete: true
Attachment #613492 - Flags: feedback?(hub)
This one works much better. Let me see.
Comment on attachment 613492 [details] [diff] [review]
Patch (v2) still rough

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

So far so good
Attachment #613492 - Flags: feedback?(hub) → feedback+

Comment 11

5 years ago
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
(Assignee)

Comment 12

5 years ago
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?
(Assignee)

Comment 13

5 years ago
Created attachment 613532 [details] [diff] [review]
Patch (v3) rough

Forgot to post my next best-guess ...
Attachment #613492 - Attachment is obsolete: true

Comment 14

5 years ago
(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.
(Assignee)

Comment 15

5 years ago
Created attachment 613541 [details] [diff] [review]
Patch (v4)

Latest try - builds and tests ok locally ...
Attachment #613532 - Attachment is obsolete: true

Comment 16

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

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

looks ok
Attachment #613541 - Flags: review+
I'll check it in
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea2aa8357d28
Target Milestone: --- → mozilla14
backed out. Will re-land.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1df10e8b5ce2
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9a538375c8a
https://hg.mozilla.org/mozilla-central/rev/f9a538375c8a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.