817766 - move atkutil class implementation out of ApplicationAccessibleWrap.cpp

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Comment 1

[Trevor Saunders (:tbsaunde)]

Attachment: part 1 move all of it

I'd really rather not clean up the style of the code in UtilInterface.cpp right now mostly because I suspect the really crazy bits are the stuff around the GHashTables, and I want to remove them all together, so it'd just be a waste of time.

Comment 2

[Trevor Saunders (:tbsaunde)]

Attachment: part 2 a little cleanup

just remove some stuff that wasn't used

Comment 3

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #1)
> Created attachment 687916 [details] [diff] [review]
> part 1 move all of it
> 
> I'd really rather not clean up the style of the code in UtilInterface.cpp
> right now mostly because I suspect the really crazy bits are the stuff
> around the GHashTables, and I want to remove them all together, so it'd just
> be a waste of time.

I prefer hg copy and then remove not needed code, we keep history and you can leave code style as is.

Comment 4

[alexander :surkov (:asurkov)]

Comment on attachment 687917 [details] [diff] [review]
part 2 a little cleanup

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

this code is from beginning of times, bug 202085, and I don't see why it was necessary. Btw, why wouldn't do a bigger clean up and remove those typedefs?

Comment 5

[Trevor Saunders (:tbsaunde)]

Attachment: patch 1 v2

I bet this patch is harder to review but you asked for it ;)

Comment 6

[alexander :surkov (:asurkov)]

Comment on attachment 688121 [details] [diff] [review]
patch 1 v2

I support the idea and its implementation, it doesn't seem we need a normal review here so rs=me.

about naming: you used UtilInterface, should we rename all nsMaiInterfaceDocument into DocumentInterface eventually? Shouldn't we add 'atk' prefix as we do for ia2, uia and sdn? So that we end up atkUtilInterface.

Comment 7

[Trevor Saunders (:tbsaunde)]

> about naming: you used UtilInterface, should we rename all
> nsMaiInterfaceDocument into DocumentInterface eventually? Shouldn't we add
> 'atk' prefix as we do for ia2, uia and sdn? So that we end up
> atkUtilInterface.

isn't atk/atkFooInterface.cpp redundant?  In an ideal world I'd rather the other things didn't have prefixs too, but we started with them because of not seperate folders, and then kept them to reduce churn and keep consistant.

Comment 8

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #4)
> Comment on attachment 687917 [details] [diff] [review]
> part 2 a little cleanup
> 
> Review of attachment 687917 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> this code is from beginning of times, bug 202085, and I don't see why it was
> necessary. Btw, why wouldn't do a bigger clean up and remove those typedefs?


I think its standard gobject boiler plate, that was added because someone thought it might be useful someday.

I'm not totally opposed to removing the typedef's but I wonder if it might be more confusing since your creating a new "class" that has the same name as the "class" it "inherits from".

Comment 9

[Trevor Saunders (:tbsaunde)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a76ec70d30a7
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4553da6ca2e2

Comment 10

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #7)
> > about naming: you used UtilInterface, should we rename all
> > nsMaiInterfaceDocument into DocumentInterface eventually? Shouldn't we add
> > 'atk' prefix as we do for ia2, uia and sdn? So that we end up
> > atkUtilInterface.
> 
> isn't atk/atkFooInterface.cpp redundant?  In an ideal world I'd rather the
> other things didn't have prefixs too, but we started with them because of
> not seperate folders, and then kept them to reduce churn and keep consistant.

it is, primarily I was asked because of consistence.

(In reply to Trevor Saunders (:tbsaunde) from comment #8)

> > this code is from beginning of times, bug 202085, and I don't see why it was
> > necessary. Btw, why wouldn't do a bigger clean up and remove those typedefs?

> I think its standard gobject boiler plate, that was added because someone
> thought it might be useful someday.
> 
> I'm not totally opposed to removing the typedef's but I wonder if it might
> be more confusing since your creating a new "class" that has the same name
> as the "class" it "inherits from".

if you don't have ideas how these classes can be used in future then I would remove typedefs.

Comment 11

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #10)
> (In reply to Trevor Saunders (:tbsaunde) from comment #7)
> > > about naming: you used UtilInterface, should we rename all
> > > nsMaiInterfaceDocument into DocumentInterface eventually? Shouldn't we add
> > > 'atk' prefix as we do for ia2, uia and sdn? So that we end up
> > > atkUtilInterface.
> > 
> > isn't atk/atkFooInterface.cpp redundant?  In an ideal world I'd rather the
> > other things didn't have prefixs too, but we started with them because of
> > not seperate folders, and then kept them to reduce churn and keep consistant.
> 
> it is, primarily I was asked because of consistence.
> 
> (In reply to Trevor Saunders (:tbsaunde) from comment #8)
> 
> > > this code is from beginning of times, bug 202085, and I don't see why it was
> > > necessary. Btw, why wouldn't do a bigger clean up and remove those typedefs?
> 
> > I think its standard gobject boiler plate, that was added because someone
> > thought it might be useful someday.
> > 
> > I'm not totally opposed to removing the typedef's but I wonder if it might
> > be more confusing since your creating a new "class" that has the same name
> > as the "class" it "inherits from".
> 
> if you don't have ideas how these classes can be used in future then I would
> remove typedefs.

I'm pretty sure that class won't have external users ever it'll just be to over ride the virtual functions on the parent.  I see your point in wanting to remove the typedefs, but Im still not convinced it won't be rather confusing for people who know gobject.

Comment 12

[alexander :surkov (:asurkov)]

ok, up to you

Comment 13

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #9)
> remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a76ec70d30a7
> remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4553da6ca2e2

it seems you landed the version without hg copy

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a76ec70d30a7
https://hg.mozilla.org/mozilla-central/rev/4553da6ca2e2