Closed Bug 817766 Opened 10 years ago Closed 10 years ago

move atkutil class implementation out of ApplicationAccessibleWrap.cpp

Categories

(Core :: Disability Access APIs, defect)

All
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: tbsaunde, Assigned: tbsaunde)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

No description provided.
Attached patch part 1 move all of it (obsolete) — Splinter Review
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.
Attachment #687916 - Flags: review?(surkov.alexander)
just remove some stuff that wasn't used
Attachment #687917 - Flags: review?(surkov.alexander)
(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.
Attachment #687916 - Flags: review?(surkov.alexander)
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?
Attachment #687917 - Flags: review?(surkov.alexander) → review+
Attached patch patch 1 v2Splinter Review
I bet this patch is harder to review but you asked for it ;)
Attachment #687916 - Attachment is obsolete: true
Attachment #688121 - Flags: review?(surkov.alexander)
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.
Attachment #688121 - Flags: review?(surkov.alexander) → review+
> 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.
(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".
(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.
(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.
ok, up to you
(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
https://hg.mozilla.org/mozilla-central/rev/a76ec70d30a7
https://hg.mozilla.org/mozilla-central/rev/4553da6ca2e2
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.