735915 - make atk interface vfuncs static

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Comment 1

[Trevor Saunders (:tbsaunde)]

Attachment: make atk interface vfuncs static

Comment 2

[Ginn Chen]

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)]

(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]

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]

(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)]

(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)]

(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]

It looks good to me, though I just checked whether code transfer matches and mochitests passes.

Comment 9

[Trevor Saunders (:tbsaunde)]

Attachment: patch 2

fixed comments

Comment 10

[Trevor Saunders (:tbsaunde)]

landed https://hg.mozilla.org/integration/mozilla-inbound/rev/4e78e0214934

Comment 11

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/mozilla-central/rev/4e78e0214934