make atk interface vfuncs static

RESOLVED FIXED

Status

()

Core
Disability Access APIs
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: tbsaunde, Assigned: tbsaunde)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

6 years ago
Created attachment 606017 [details] [diff] [review]
make atk interface vfuncs static
(Assignee)

Updated

6 years ago
Attachment #606017 - Flags: review?(askalski)

Comment 2

6 years ago
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)
(Assignee)

Comment 3

6 years ago
(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

6 years ago
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

6 years ago
(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;
(Assignee)

Comment 6

6 years ago
(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.
(Assignee)

Comment 7

6 years ago
(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

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

Updated

6 years ago
Attachment #606017 - Flags: review?(askalski) → review+
(Assignee)

Comment 9

6 years ago
Created attachment 606348 [details] [diff] [review]
patch 2

fixed comments
Attachment #606348 - Flags: review?(ginn.chen)
(Assignee)

Updated

6 years ago
Attachment #606017 - Attachment is obsolete: true

Updated

6 years ago
Attachment #606348 - Flags: review?(ginn.chen) → review+
(Assignee)

Comment 10

6 years ago
landed https://hg.mozilla.org/integration/mozilla-inbound/rev/4e78e0214934
https://hg.mozilla.org/mozilla-central/rev/4e78e0214934
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Version: unspecified → Trunk

Updated

6 years ago
Assignee: nobody → trev.saunders
You need to log in before you can comment on or make changes to this bug.