Last Comment Bug 741707 - rm unused a11y CIDs
: rm unused a11y CIDs
Status: RESOLVED FIXED
[good first bug][mentor=hub@mozilla.c...
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla14
Assigned To: Mark Capella [:capella]
:
: alexander :surkov
Mentors:
Depends on: 741709
Blocks: dexpcoma11y
  Show dependency treegraph
 
Reported: 2012-04-02 23:56 PDT by Trevor Saunders (:tbsaunde)
Modified: 2012-04-11 09:20 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) very rough (9.00 KB, patch)
2012-04-09 15:07 PDT, Mark Capella [:capella]
hub: feedback-
Details | Diff | Splinter Review
Odd Build Error (4.23 KB, text/plain)
2012-04-09 15:08 PDT, Mark Capella [:capella]
no flags Details
Patch (v2) still rough (12.46 KB, patch)
2012-04-09 21:51 PDT, Mark Capella [:capella]
hub: feedback+
Details | Diff | Splinter Review
Patch (v3) rough (11.19 KB, patch)
2012-04-10 02:56 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v4) (12.18 KB, patch)
2012-04-10 03:52 PDT, Mark Capella [:capella]
surkov.alexander: review+
Details | Diff | Splinter Review

Description Trevor Saunders (:tbsaunde) 2012-04-02 23:56:31 PDT
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] 2012-04-09 15:07:33 PDT
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
Comment 2 Mark Capella [:capella] 2012-04-09 15:08:15 PDT
Created attachment 613411 [details]
Odd Build Error
Comment 3 Hubert Figuiere [:hub] 2012-04-09 17:40:14 PDT
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] 2012-04-09 17:43:58 PDT
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] 2012-04-09 17:46:53 PDT
Please forget that last comment.
Comment 6 Hubert Figuiere [:hub] 2012-04-09 17:47:26 PDT
(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) 2012-04-09 20:10:32 PDT
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] 2012-04-09 21:51:15 PDT
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.
Comment 9 Hubert Figuiere [:hub] 2012-04-09 22:02:27 PDT
This one works much better. Let me see.
Comment 10 Hubert Figuiere [:hub] 2012-04-09 22:08:12 PDT
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 2012-04-10 00:32:42 PDT
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] 2012-04-10 01:53:31 PDT
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] 2012-04-10 02:56:04 PDT
Created attachment 613532 [details] [diff] [review]
Patch (v3) rough

Forgot to post my next best-guess ...
Comment 14 alexander :surkov 2012-04-10 03:09:41 PDT
(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] 2012-04-10 03:52:23 PDT
Created attachment 613541 [details] [diff] [review]
Patch (v4)

Latest try - builds and tests ok locally ...
Comment 16 alexander :surkov 2012-04-10 04:22:53 PDT
Comment on attachment 613541 [details] [diff] [review]
Patch (v4)

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

looks ok
Comment 17 Hubert Figuiere [:hub] 2012-04-10 10:53:06 PDT
I'll check it in
Comment 19 Hubert Figuiere [:hub] 2012-04-10 12:43:44 PDT
backed out. Will re-land.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1df10e8b5ce2
Comment 21 Matt Brubeck (:mbrubeck) 2012-04-11 09:20:01 PDT
https://hg.mozilla.org/mozilla-central/rev/f9a538375c8a

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