Last Comment Bug 735915 - make atk interface vfuncs static
: make atk interface vfuncs static
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Trevor Saunders (:tbsaunde)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-14 16:24 PDT by Trevor Saunders (:tbsaunde)
Modified: 2012-03-27 22:55 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
make atk interface vfuncs static (100.43 KB, patch)
2012-03-14 16:24 PDT, Trevor Saunders (:tbsaunde)
andrzej.j.skalski: review+
Details | Diff | Review
patch 2 (100.46 KB, patch)
2012-03-15 14:06 PDT, Trevor Saunders (:tbsaunde)
ginn.chen: review+
Details | Diff | Review

Description Trevor Saunders (:tbsaunde) 2012-03-14 16:24:40 PDT

    
Comment 1 Trevor Saunders (:tbsaunde) 2012-03-14 16:24:43 PDT
Created attachment 606017 [details] [diff] [review]
make atk interface vfuncs static
Comment 2 Ginn Chen 2012-03-14 16:51:50 PDT
Why would we need to do that?
We build our code with visibility hidden, it doesn't make any different with "static".

BTW:
extra spacing here:
+
+  void
+imageInterfaceInitCB(AtkImageIface* aIface)
Comment 3 Trevor Saunders (:tbsaunde) 2012-03-14 17:00:05 PDT
(In reply to Ginn Chen from comment #2)
> Why would we need to do that?
> We build our code with visibility hidden, it doesn't make any different with
> "static".

I'd say "need" is too strong, but it should help reduce what gets included hopefully making  the build a little faster and reducing what depends on what which at least in theory should help incremental builds.

> BTW:
> extra spacing here:
> +
> +  void
> +imageInterfaceInitCB(AtkImageIface* aIface)

yeah, fixed locally
Comment 4 Ginn Chen 2012-03-14 17:44:00 PDT
In AtkSocketAccessible.cpp

it should be
extern "C" {
void (*AtkSocketAccessible::g_atk_socket_embed) (AtkSocket*, gchar*) = NULL;
}

or just
AtkSocketEmbedType AtkSocketAccessible::g_atk_socket_embed;
Comment 5 Ginn Chen 2012-03-14 17:45:00 PDT
(In reply to Ginn Chen from comment #4)
> or just
> AtkSocketEmbedType AtkSocketAccessible::g_atk_socket_embed;

I mean

AtkSocketEmbedType AtkSocketAccessible::g_atk_socket_embed = NULL;
Comment 6 Trevor Saunders (:tbsaunde) 2012-03-15 06:55:15 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #3)
> (In reply to Ginn Chen from comment #2)
> > Why would we need to do that?
> > We build our code with visibility hidden, it doesn't make any different with
> > "static".
> 
> I'd say "need" is too strong, but it should help reduce what gets included
> hopefully making  the build a little faster and reducing what depends on
> what which at least in theory should help incremental builds.

Another nice advantage of this way imo is that it makes it clearer to the reader what boundaries there are between "modular" components.
Comment 7 Trevor Saunders (:tbsaunde) 2012-03-15 06:58:39 PDT
(In reply to Ginn Chen from comment #5)
> (In reply to Ginn Chen from comment #4)
> > or just
> > AtkSocketEmbedType AtkSocketAccessible::g_atk_socket_embed;
> 
> I mean
> 
> AtkSocketEmbedType AtkSocketAccessible::g_atk_socket_embed = NULL;

yeah, sounds good I'm not really sure why Mike didn't just use the typedef originally.
Comment 8 Andrzej Skalski 2012-03-15 13:45:40 PDT
It looks good to me, though I just checked whether code transfer matches and mochitests passes.
Comment 9 Trevor Saunders (:tbsaunde) 2012-03-15 14:06:06 PDT
Created attachment 606348 [details] [diff] [review]
patch 2

fixed comments
Comment 10 Trevor Saunders (:tbsaunde) 2012-03-20 01:27:24 PDT
landed https://hg.mozilla.org/integration/mozilla-inbound/rev/4e78e0214934
Comment 11 Mounir Lamouri (:mounir) 2012-03-20 03:45:28 PDT
https://hg.mozilla.org/mozilla-central/rev/4e78e0214934

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