1158081 - Define macro to declare SocketObserver functions

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Ben Tian (inactive)]

TODO:
- Define macro to declare SocketObserver functions
- Simplify |BluetoothOppManager::SendObexData| function

Comment 1

[Ben Tian (inactive)]

Attachment: Patch 1 (v1): Define macro to declare SocketObserver functions

Comment 2

[Ben Tian (inactive)]

Attachment: Patch 2 (v1): Simplify |BluetoothOppManager::SendObexData| function

Comment 3

[Ben Tian (inactive)]

Attachment: Patch 1 (v2): Define macro to declare SocketObserver functions

Add |public:| into macro.

Comment 4

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8597160 [details] [diff] [review]
Patch 2 (v1): Simplify |BluetoothOppManager::SendObexData| function

Review of attachment 8597160 [details] [diff] [review]:
-----------------------------------------------------------------

I've been traveling on Friday. Sorry that the review took a bit longer.

Comment 5

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8597840 [details] [diff] [review]
Patch 1 (v2): Define macro to declare SocketObserver functions

Review of attachment 8597840 [details] [diff] [review]:
-----------------------------------------------------------------

R+ with a nit.

::: dom/bluetooth/BluetoothSocketObserver.h
@@ +44,3 @@
>  
> +#define BT_DECL_SOCKET_OBSERVER                                             \
> +public:                                                                     \

Please don't include the 'public' statement in the macro. When using the macro this affects anything that comes afterwards and you might accidentally alter the protection of a class' methods/fields.

Comment 6

[Ben Tian (inactive)]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #5)
> Please don't include the 'public' statement in the macro. When using the
> macro this affects anything that comes afterwards and you might accidentally
> alter the protection of a class' methods/fields.

I add 'public' to follow [1] to ensure these functions are public. Do you suggest to remove 'public' from [2] as well?

[1] https://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsISupportsImpl.h?from=NS_DECL_ISUPPORTS_INHERITED&case=true#969
[2] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/BluetoothProfileManagerBase.h#93

Comment 7

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hmm, that sounds like a good idea to me.

There are also lots of helper macros in xpcom/. If there are design guidelines for these macros, it probably makes sense to follow them in BT as well.

Comment 8

[Ben Tian (inactive)]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #7)
> Hmm, that sounds like a good idea to me.
> 
> There are also lots of helper macros in xpcom/. If there are design
> guidelines for these macros, it probably makes sense to follow them in BT as
> well.

Didn't find guidelines regarding it on [1]. I prefer to keep 'public' as helper macros in xpcom/. Let me know for any further concern.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

Comment 9

[Ben Tian (inactive)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2dee38f2d10b

Comment 10

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi Olli,

in the Bluetooth code, there a number of helper macros for base classes and implementations, maybe similar to NS_CYLE_*, NS_IMPL_* in xpcom/glues/nsCycleCollectionParticipant.h.

Do you of any design rules or best practices for designing such macros? Thanks!

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

There are really no good design rules, except perhaps keep the macro names easy to understand.
(NS_CYCLE* macros are horribly long, but I prefer that over shorter and less-easy to understand.)
Other than that, make the macros do whatever magic it is needed to do.

Comment 12

[Ben Tian (inactive)]

https://hg.mozilla.org/integration/b2g-inbound/rev/8721fd2d3cae
https://hg.mozilla.org/integration/b2g-inbound/rev/4c438af16067

Comment 13

[Ryan VanderMeulen (PTO, back July 27)]

https://hg.mozilla.org/mozilla-central/rev/8721fd2d3cae
https://hg.mozilla.org/mozilla-central/rev/4c438af16067

Comment 14

[Ben Tian (inactive)]

Wesley, please help mark this bug feature-b2g:2.2r since BT feature PBAP depends on this bug.

Comment 15

[Ben Tian (inactive)]

Attachment: [2.2r] Patch 1: Define macro to declare SocketObserver functions, r=tzimmermann

Upload patch 1 for 2.2r. Patch 2 can be applied to 2.2r without conflict.

Comment 16

[Ryan VanderMeulen (PTO, back July 27)]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/389b28b5135f
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/088fe97ffa60