Closed Bug 1158081 Opened 5 years ago Closed 5 years ago

Define macro to declare SocketObserver functions

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

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

RESOLVED FIXED
2.2 S11 (1may)
feature-b2g 2.2r+
Tracking Status
firefox40 --- fixed
b2g-v2.2r --- fixed
b2g-master --- fixed

People

(Reporter: ben.tian, Assigned: ben.tian)

References

Details

Attachments

(3 files, 1 obsolete file)

TODO:
- Define macro to declare SocketObserver functions
- Simplify |BluetoothOppManager::SendObexData| function
Assignee: nobody → btian
Attachment #8597159 - Flags: review?(tzimmermann)
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+
(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)
(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
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)
https://hg.mozilla.org/mozilla-central/rev/8721fd2d3cae
https://hg.mozilla.org/mozilla-central/rev/4c438af16067
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S11 (1may)
Wesley, please help mark this bug feature-b2g:2.2r since BT feature PBAP depends on this bug.
Flags: needinfo?(whuang)
Blocks: 1152694
Upload patch 1 for 2.2r. Patch 2 can be applied to 2.2r without conflict.
feature-b2g: --- → 2.2r+
Flags: needinfo?(whuang)
You need to log in before you can comment on or make changes to this bug.