Closed
Bug 1158081
Opened 9 years ago
Closed 9 years ago
Define macro to declare SocketObserver functions
Categories
(Firefox OS Graveyard :: Bluetooth, defect)
Tracking
(feature-b2g:2.2r+, firefox40 fixed, b2g-v2.2r fixed, b2g-master fixed)
People
(Reporter: ben.tian, Assigned: ben.tian)
References
Details
Attachments
(3 files, 1 obsolete file)
1.95 KB,
patch
|
tzimmermann
:
review+
|
Details | Diff | Splinter Review |
5.75 KB,
patch
|
tzimmermann
:
review+
|
Details | Diff | Splinter Review |
5.94 KB,
patch
|
Details | Diff | Splinter Review |
TODO: - Define macro to declare SocketObserver functions - Simplify |BluetoothOppManager::SendObexData| function
Assignee | ||
Comment 1•9 years ago
|
||
Assignee: nobody → btian
Attachment #8597159 -
Flags: review?(tzimmermann)
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8597160 -
Flags: review?(tzimmermann)
Assignee | ||
Comment 3•9 years ago
|
||
Add |public:| into macro.
Attachment #8597159 -
Attachment is obsolete: true
Attachment #8597159 -
Flags: review?(tzimmermann)
Attachment #8597840 -
Flags: review?(tzimmermann)
Comment 4•9 years ago
|
||
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 5•9 years ago
|
||
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•9 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)
Comment 7•9 years ago
|
||
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•9 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•9 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2dee38f2d10b
Comment 10•9 years ago
|
||
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)
Comment 11•9 years ago
|
||
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•9 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/8721fd2d3cae https://hg.mozilla.org/integration/b2g-inbound/rev/4c438af16067
Comment 13•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8721fd2d3cae https://hg.mozilla.org/mozilla-central/rev/4c438af16067
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S11 (1may)
Assignee | ||
Comment 14•9 years ago
|
||
Wesley, please help mark this bug feature-b2g:2.2r since BT feature PBAP depends on this bug.
Flags: needinfo?(whuang)
Assignee | ||
Comment 15•9 years ago
|
||
Upload patch 1 for 2.2r. Patch 2 can be applied to 2.2r without conflict.
Updated•9 years ago
|
Updated•9 years ago
|
status-b2g-master:
--- → fixed
Comment 16•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/389b28b5135f https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/088fe97ffa60
You need to log in
before you can comment on or make changes to this bug.
Description
•