Implement GetMessagesListing function

RESOLVED FIXED in Firefox 43

Status

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: shawnjohnjr, Assigned: shawnjohnjr)

Tracking

unspecified
FxOS-S7 (18Sep)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

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

Details

Attachments

(2 attachments, 9 obsolete attachments)

9.26 KB, patch
Details | Diff | Splinter Review
10.05 KB, patch
Details | Diff | Splinter Review
Implement GetMessagesListing function
Assignee: nobody → shuang
This patch depends on bug1180554 patch (iplh) file.
Attachment #8648664 - Attachment is obsolete: true
Attachment #8648664 - Flags: review?(btian)
Attachment #8648701 - Attachment is obsolete: true
Attachment #8648701 - Flags: review?(btian)
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+
(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?
Flags: needinfo?(btian)
(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)

Updated

4 years ago
Blocks: 1195685
(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.
Sorry, Ben. I found I made a mistake :(.
MessageListing function does not contain 'name', I will remove it.

Updated

4 years ago
No longer blocks: 1195685
blocking-b2g: --- → 2.2r?
Flags: needinfo?(whuang)
blocking-b2g: 2.2r? → ---
feature-b2g: --- → 2.2r+
Flags: needinfo?(whuang)
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?
Flags: needinfo?(btian)
Attachment #8659371 - Attachment description: Bug 1166675 - Implement GetMessagesListing function, r=btian → Bug 1166675 - Implement GetMessagesListing function, r=btian (m-c)
https://hg.mozilla.org/mozilla-central/rev/9f6bd98f767c
Status: NEW → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S7 (18Sep)
(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)
(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.