Define macro to declare SocketObserver functions

RESOLVED FIXED in Firefox 40, Firefox OS v2.2r

Status

Firefox OS
Bluetooth
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: btian, Assigned: btian)

Tracking

unspecified
2.2 S11 (1may)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(feature-b2g:2.2r+, firefox40 fixed, b2g-v2.2r fixed, b2g-master fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
TODO:
- Define macro to declare SocketObserver functions
- Simplify |BluetoothOppManager::SendObexData| function
(Assignee)

Comment 1

3 years ago
Created attachment 8597159 [details] [diff] [review]
Patch 1 (v1): Define macro to declare SocketObserver functions
Assignee: nobody → btian
Attachment #8597159 - Flags: review?(tzimmermann)
(Assignee)

Comment 2

3 years ago
Created attachment 8597160 [details] [diff] [review]
Patch 2 (v1): Simplify |BluetoothOppManager::SendObexData| function
Attachment #8597160 - Flags: review?(tzimmermann)
(Assignee)

Comment 3

3 years ago
Created attachment 8597840 [details] [diff] [review]
Patch 1 (v2): Define macro to declare SocketObserver functions

Add |public:| into macro.
Attachment #8597159 - Attachment is obsolete: true
Attachment #8597159 - Flags: review?(tzimmermann)
Attachment #8597840 - Flags: review?(tzimmermann)
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.
Attachment #8597160 - Flags: review?(tzimmermann) → review+
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.
Attachment #8597840 - Flags: review?(tzimmermann) → review+
(Assignee)

Comment 6

3 years ago
(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
Flags: needinfo?(tzimmermann)
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.
Flags: needinfo?(tzimmermann)
(Assignee)

Comment 8

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

Comment 9

3 years ago
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2dee38f2d10b
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!
Flags: needinfo?(bugs)
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.
Flags: needinfo?(bugs)
(Assignee)

Comment 12

3 years ago
https://hg.mozilla.org/integration/b2g-inbound/rev/8721fd2d3cae
https://hg.mozilla.org/integration/b2g-inbound/rev/4c438af16067
https://hg.mozilla.org/mozilla-central/rev/8721fd2d3cae
https://hg.mozilla.org/mozilla-central/rev/4c438af16067
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S11 (1may)
(Assignee)

Comment 14

2 years ago
Wesley, please help mark this bug feature-b2g:2.2r since BT feature PBAP depends on this bug.
Flags: needinfo?(whuang)
(Assignee)

Updated

2 years ago
Blocks: 1152694
(Assignee)

Comment 15

2 years ago
Created attachment 8650324 [details] [diff] [review]
[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.
feature-b2g: --- → 2.2r+
status-b2g-v2.2r: --- → affected
Flags: needinfo?(whuang)
status-b2g-master: --- → fixed
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/389b28b5135f
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/088fe97ffa60
status-b2g-v2.2r: affected → fixed
You need to log in before you can comment on or make changes to this bug.