Closed
Bug 817766
Opened 12 years ago
Closed 12 years ago
move atkutil class implementation out of ApplicationAccessibleWrap.cpp
Categories
(Core :: Disability Access APIs, defect)
Tracking
()
RESOLVED
FIXED
mozilla20
People
(Reporter: tbsaunde, Assigned: tbsaunde)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 1 obsolete file)
1.61 KB,
patch
|
surkov
:
review+
|
Details | Diff | Splinter Review |
52.44 KB,
patch
|
surkov
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•12 years ago
|
||
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)
Assignee | ||
Comment 2•12 years ago
|
||
just remove some stuff that wasn't used
Attachment #687917 -
Flags: review?(surkov.alexander)
Comment 3•12 years ago
|
||
(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.
Updated•12 years ago
|
Attachment #687916 -
Flags: review?(surkov.alexander)
Comment 4•12 years ago
|
||
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+
Assignee | ||
Comment 5•12 years ago
|
||
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 6•12 years ago
|
||
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+
Assignee | ||
Comment 7•12 years ago
|
||
> 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.
Assignee | ||
Comment 8•12 years ago
|
||
(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".
Assignee | ||
Comment 9•12 years ago
|
||
Comment 10•12 years ago
|
||
(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.
Assignee | ||
Comment 11•12 years ago
|
||
(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•12 years ago
|
||
ok, up to you
Comment 13•12 years ago
|
||
(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•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a76ec70d30a7
https://hg.mozilla.org/mozilla-central/rev/4553da6ca2e2
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in
before you can comment on or make changes to this bug.
Description
•