Closed
Bug 1166675
Opened 9 years ago
Closed 9 years ago
Implement GetMessagesListing function
Categories
(Firefox OS Graveyard :: Bluetooth, defect)
Tracking
(feature-b2g:2.2r+, firefox43 fixed, b2g-v2.2r fixed)
People
(Reporter: shawnjohnjr, Assigned: shawnjohnjr)
References
Details
Attachments
(2 files, 9 obsolete files)
Implement GetMessagesListing function
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → shuang
Assignee | ||
Comment 1•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8648082 -
Attachment is obsolete: true
Assignee | ||
Comment 2•9 years ago
|
||
Assignee | ||
Comment 3•9 years ago
|
||
This patch depends on bug1180554 patch (iplh) file.
Assignee | ||
Updated•9 years ago
|
Attachment #8648592 -
Attachment is obsolete: true
Assignee | ||
Comment 4•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8648659 -
Flags: review?(btian)
Assignee | ||
Updated•9 years ago
|
Attachment #8648659 -
Flags: review?(btian)
Assignee | ||
Updated•9 years ago
|
Attachment #8648659 -
Attachment is obsolete: true
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8648664 -
Flags: review?(btian)
Assignee | ||
Updated•9 years ago
|
Attachment #8648664 -
Attachment is obsolete: true
Attachment #8648664 -
Flags: review?(btian)
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8648701 -
Flags: review?(btian)
Assignee | ||
Updated•9 years ago
|
Attachment #8648701 -
Attachment is obsolete: true
Attachment #8648701 -
Flags: review?(btian)
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8648722 -
Flags: review?(btian)
Comment 8•9 years ago
|
||
Comment on attachment 8648722 [details] [diff] [review]
Bug 1166675 - Implement GetMessagesListing function
Review of attachment 8648722 [details] [diff] [review]:
-----------------------------------------------------------------
r=me with comment addressed.
::: dom/bluetooth/BluetoothCommon.h
@@ +230,5 @@
> */
> #define REQUEST_MEDIA_PLAYSTATUS_ID "requestmediaplaystatus"
>
> /**
> + * When receiving a MAP request from a remote device, we'll dispatch an
a MAP request 'of messages listing' from a remote device ...
::: dom/bluetooth/bluedroid/BluetoothMapSmsManager.cpp
@@ +699,5 @@
> + /*
> + * Follow MAP 6.3.1 Application Parameter Header
> + */
> + switch (aTagId) {
> + case Map::AppParametersTagId::MaxListCount: {
We should make |BT_APPEND_NAME_VALUE| a function to take parameter and remove redundancy here. Please open a follow-up bug for it.
@@ +700,5 @@
> + * Follow MAP 6.3.1 Application Parameter Header
> + */
> + switch (aTagId) {
> + case Map::AppParametersTagId::MaxListCount: {
> + if (!aHeader.GetAppParameter(Map::AppParametersTagId::MaxListCount,
Check at the beginning of the function.
uint8_t buf[64];
if (!aHeader.GetAppParameter(aTagId, buf, 64)) {
return;
}
@@ +843,5 @@
> + nsString name;
> + aHeader.GetName(name);
> + BT_APPEND_NAMED_VALUE(data, "name", name);
> +
> + AppendBtNamedValueByTagId(aHeader, data,
I prefer to have a array for these app parameters:
static Map::AppParametersTagId sMsgListingParameters = {
CONVERT(0, Map::AppParametersTagId::MaxListCount);
...
CONVERT(n, Map::AppParametersTagId::FilterPriority);
}
for (uint8_t i = 0; i < MOZ_ARRAY_LENGTH(sMsgListingParameters); i++) {
AppendBtNamedValueByTagId(aHeader, data, sMsgListingParameters[i]);
}
@@ +867,5 @@
> + AppendBtNamedValueByTagId(aHeader, data,
> + Map::AppParametersTagId::FilterPriority);
> +
> + #ifdef MOZ_B2G_BT_API_V1
> + bs->DistributeSignal(
Remove v1 implementation since it's already removed on m-c. Also restore the indention.
Attachment #8648722 -
Flags: review?(btian) → review+
Assignee | ||
Comment 9•9 years ago
|
||
(In reply to Ben Tian [:btian] from comment #8)
> Comment on attachment 8648722 [details] [diff] [review]
> Bug 1166675 - Implement GetMessagesListing function
>
> Review of attachment 8648722 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> r=me with comment addressed.
>
> ::: dom/bluetooth/BluetoothCommon.h
> @@ +230,5 @@
> > */
> > #define REQUEST_MEDIA_PLAYSTATUS_ID "requestmediaplaystatus"
> >
> > /**
> > + * When receiving a MAP request from a remote device, we'll dispatch an
>
> a MAP request 'of messages listing' from a remote device ...
>
> ::: dom/bluetooth/bluedroid/BluetoothMapSmsManager.cpp
> @@ +699,5 @@
> > + /*
> > + * Follow MAP 6.3.1 Application Parameter Header
> > + */
> > + switch (aTagId) {
> > + case Map::AppParametersTagId::MaxListCount: {
>
> We should make |BT_APPEND_NAME_VALUE| a function to take parameter and
> remove redundancy here. Please open a follow-up bug for it.
>
I'm not sure I understand your point. Are you saying BT_APPEND_NAME_VALUE shall take AppParametersTagId as parameter?
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(btian)
Comment 10•9 years ago
|
||
(In reply to Shawn Huang [:shawnjohnjr] from comment #9)
> > We should make |BT_APPEND_NAME_VALUE| a function to take parameter and
> > remove redundancy here. Please open a follow-up bug for it.
> >
> I'm not sure I understand your point. Are you saying BT_APPEND_NAME_VALUE
> shall take AppParametersTagId as parameter?
Yes. But it can't because of being macro instead of function, per bug 1180554 comment 6.
"I had tried to revise the codes by using a literal strings array, but I can't find a proper way to do this.
One of the reasons is that the array will be used as the second parameter of 'BT_APPEND_NAMED_VALUE'. It means the array will be manipulated by the macro 'NS_LITERAL_STRING' first before the compilation process."
Flags: needinfo?(btian)
Assignee | ||
Updated•9 years ago
|
Attachment #8648722 -
Attachment is obsolete: true
Assignee | ||
Comment 11•9 years ago
|
||
Comment 12•9 years ago
|
||
(In reply to Ben Tian [:btian] from comment #10)
> Yes. But it can't because of being macro instead of function, per bug
> 1180554 comment 6.
>
> "I had tried to revise the codes by using a literal strings array, but I
> can't find a proper way to do this.
>
> One of the reasons is that the array will be used as the second parameter of
> 'BT_APPEND_NAMED_VALUE'. It means the array will be manipulated by the macro
> 'NS_LITERAL_STRING' first before the compilation process."
I opened bug 1195685 to track it.
Assignee | ||
Updated•9 years ago
|
Attachment #8649170 -
Attachment is obsolete: true
Assignee | ||
Comment 13•9 years ago
|
||
Sorry, Ben. I found I made a mistake :(.
MessageListing function does not contain 'name', I will remove it.
Assignee | ||
Comment 14•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
blocking-b2g: --- → 2.2r?
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(whuang)
Assignee | ||
Updated•9 years ago
|
Attachment #8649235 -
Attachment is obsolete: true
Assignee | ||
Comment 15•9 years ago
|
||
Rebase to the latest m-c.
Assignee | ||
Comment 16•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8659367 -
Attachment is obsolete: true
Assignee | ||
Comment 17•9 years ago
|
||
Re-upload the file, the previous upload is old version.
Assignee | ||
Comment 18•9 years ago
|
||
Updated•9 years ago
|
blocking-b2g: 2.2r? → ---
feature-b2g: --- → 2.2r+
Flags: needinfo?(whuang)
Assignee | ||
Comment 20•9 years ago
|
||
Comment on attachment 8659371 [details] [diff] [review]
Bug 1166675 - Implement GetMessagesListing function, r=btian (m-c)
Review of attachment 8659371 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/bluetooth/bluedroid/BluetoothMapSmsManager.cpp
@@ +708,5 @@
> + uint16_t maxListCount = *((uint16_t *)buf);
> + // convert big endian to little endian
> + maxListCount = (maxListCount >> 8) | (maxListCount << 8);
> + BT_LOGR("max list count: %d", maxListCount);
> + AppendNamedValue(aValues, "maxListCount", maxListCount);
I just found when rebasing to v2.2r, the compiler won't cast to uint32_t, it cause build failure. But I have no idea why m-c can build pass and I checked BluetoothTypes.ipdlh, it doesn't contain uint16_t type, but still can build pass, maybe compiler cast it implicitly?
Anyway, I rebase it and cast it just like PBAP manager to type uint32_t. Or we shall add uint8_t/uint16_t type for BluetoothValue?
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(btian)
Assignee | ||
Updated•9 years ago
|
Attachment #8659371 -
Attachment description: Bug 1166675 - Implement GetMessagesListing function, r=btian → Bug 1166675 - Implement GetMessagesListing function, r=btian (m-c)
Assignee | ||
Comment 21•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 22•9 years ago
|
||
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S7 (18Sep)
Comment 23•9 years ago
|
||
status-b2g-v2.2r:
--- → fixed
Comment 24•9 years ago
|
||
(In reply to Shawn Huang [:shawnjohnjr] from comment #20)
> Anyway, I rebase it and cast it just like PBAP manager to type uint32_t. Or
> we shall add uint8_t/uint16_t type for BluetoothValue?
I personally prefer uint32_t casting, but I'm fine for both options.
Flags: needinfo?(btian)
Assignee | ||
Comment 25•9 years ago
|
||
(In reply to Ben Tian [:btian] from comment #24)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #20)
> > Anyway, I rebase it and cast it just like PBAP manager to type uint32_t. Or
> > we shall add uint8_t/uint16_t type for BluetoothValue?
>
> I personally prefer uint32_t casting, but I'm fine for both options.
Got it. I already use uint32_t casting for both m-c and v22r patch sets.
You need to log in
before you can comment on or make changes to this bug.
Description
•