Closed Bug 1166645 Opened 10 years ago Closed 10 years ago

Implement MAP profile manager connection related function

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

Product:
Firefox OS Graveyard
Component:
Bluetooth
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

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

Status:
RESOLVED FIXED
Milestone:
FxOS-S7 (18Sep)
Project Flags:
blocking-b2g 2.2r+
Tracking Flags:
Tracking Status
firefox43 --- fixed
b2g-v2.2r --- fixed

People

(Reporter: shawnjohnjr, Assigned: shawnjohnjr)

References

Details

(Whiteboard: needs-uplift)

Attachments

(2 files, 21 obsolete files)

Bug 1166645 - Implement MAP profile manager connection related function, r=btian (m-c)
40.45 KB, patch
Details | Diff | Splinter Review
Bug 1166645 - Implement MAP profile manager connection related function, r=btian (v2.2r)
41.78 KB, patch
Details | Diff | Splinter Review
MAP Message Server Equipment (MSE) role. 1. Listen MAP MSE socket 2. Handle MCE connection/disconnection requests
Assignee: nobody → shuang
Attached patch bug1166645-mc.patch(WIP) (obsolete) — — Splinter Review
1. Handle foler listing query 2. Add virtual folder
Comment on attachment 8644904 [details] [diff] [review] Bug 1166645 - Implement MAP profile manager connection related function Review of attachment 8644904 [details] [diff] [review]: ----------------------------------------------------------------- Please see my comment below. ::: dom/bluetooth/bluedroid/BluetoothMapFolder.cpp @@ +14,5 @@ > +BluetoothMapFolder::~BluetoothMapFolder() > +{ } > + > +BluetoothMapFolder::BluetoothMapFolder(const nsAString& aFolderName, > + BluetoothMapFolder* aParent) : mName(aFolderName), mParent(aParent) nit: Align as following BluetoothMapFolder::BluetoothMapFolder(const nsAString& aFolderName, BluetoothMapFolder* aParent) : mName(aFolderName) , mParent(aParent) @@ +51,5 @@ > + // Based on Element Specification, 9.1.1, IrObex 1.2 > + nsAutoCString folderListingObejct("<?xml version=\"1.0\"?>\n"); > + folderListingObejct. > + Append("<!DOCTYPE folder-listing SYSTEM \" obex-folder-listing.dtd\">\n"); > + folderListingObejct.Append("<folder-listing version=\"1.0\">\n"); Replace with defined strings. #define FOLDER_LIST_PREFIX "<?xml ..." #define FOLDER_LIST_SUFFIX "</folder-listing>" @@ +53,5 @@ > + folderListingObejct. > + Append("<!DOCTYPE folder-listing SYSTEM \" obex-folder-listing.dtd\">\n"); > + folderListingObejct.Append("<folder-listing version=\"1.0\">\n"); > + > + for (auto iter = mSubFolders.Iter(); !iter.Done(); iter.Next()) { Declare |count| right before for loop to be close where it's first used. @@ +58,5 @@ > + if (count > aMaxListCount) { > + break; > + } > + > + if (count < aStartOffset) { nit: switch the order of these if-conditions since the second one decides where to start and first one where to end. @@ +64,5 @@ > + } > + > + const nsAString& key = iter.GetKey(); > + folderListingObejct.Append("<folder name=\""); > + folderListingObejct.Append( NS_ConvertUTF16toUTF8(key).get()); nit: remove extra space after (. ::: dom/bluetooth/bluedroid/BluetoothMapFolder.h @@ +4,5 @@ > + * License, v. 2.0. If a copy of the MPL was not distributed with this file, > + * You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#ifndef mozilla_dom_bluetooth_bluetoothmapfolder_h__ > +#define mozilla_dom_bluetooth_bluetoothmapfolder_h__ Revise to |mozilla_dom_bluetooth_bluedroid_BluetoothMapFolder_h|. See bug 1106007 for more info. @@ +9,5 @@ > + > +#include "BluetoothCommon.h" > +#include "nsAutoPtr.h" > +#include "nsTArray.h" > +#include "nsRefPtrHashtable.h" nit: alphabetical order. @@ +13,5 @@ > +#include "nsRefPtrHashtable.h" > + > +BEGIN_BLUETOOTH_NAMESPACE > + > +/* This class maps MAP virtual folder strctures */ nit: str'u'ctures @@ +37,5 @@ > + nsRefPtrHashtable<nsStringHashKey, BluetoothMapFolder> mSubFolders; > +}; > + > +END_BLUETOOTH_NAMESPACE > +#endif Add // mozilla_dom_bluetooth_bluedroid_BluetoothMapFolder_h ::: dom/bluetooth/bluedroid/BluetoothMapSmsManager.cpp @@ +65,5 @@ > + > +NS_IMETHODIMP > +BluetoothMapSmsManager::Observe(nsISupports* aSubject, > + const char* aTopic, > + const char16_t* aData) nit: align these lines. @@ +180,5 @@ > + mServerSocket = nullptr; > + } > + > + mServerSocket = > + new BluetoothSocket(this); nit: put in one line. @@ +203,5 @@ > + sdpString.AppendLiteral("SMS/MMS Message Access"); > + nsresult rv = mServerSocket->Listen(sdpString, kMapMas, > + BluetoothSocketType::RFCOMM, -1, false, > + true); > + nit: remove this newline. @@ +215,5 @@ > + > +// Virtual function of class SocketConsumer > +void > +BluetoothMapSmsManager::ReceiveSocketData(BluetoothSocket* aSocket, > + nsAutoPtr<UnixSocketBuffer>& aMessage) nit: alignment @@ +219,5 @@ > + nsAutoPtr<UnixSocketBuffer>& aMessage) > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + > + if (aSocket == mMnsSocket) { Wrap this section into function |MnsDataHandler|. @@ +231,5 @@ > + const uint8_t* data = aMessage->GetData(); > + uint8_t opCode = data[0]; > + > + // Check response code and send out system message as finished if the > + // response code is somehow incorrect. Remove this outdated comment since we don't send out system message here. @@ +233,5 @@ > + > + // Check response code and send out system message as finished if the > + // response code is somehow incorrect. > + uint8_t expectedOpCode = ObexResponseCode::Success; > + if (opCode != expectedOpCode) { Simplify to if (opCode != ObexResponseCode::Success) { @@ +234,5 @@ > + // Check response code and send out system message as finished if the > + // response code is somehow incorrect. > + uint8_t expectedOpCode = ObexResponseCode::Success; > + if (opCode != expectedOpCode) { > + BT_LOGR("Not expectedOpCode: %d", opCode); "Unexpected opCode: %x" @@ +240,5 @@ > + mLastCommand == ObexRequestCode::Put || > + mLastCommand == ObexRequestCode::Abort || > + mLastCommand == ObexRequestCode::PutFinal) { > + SendMnsClientDisconnectRequest(); > + } What to do if |mLastCommand| is not one of these four? @@ +243,5 @@ > + SendMnsClientDisconnectRequest(); > + } > + } > + } else { > + /** Wrap this section into function |MnsDataHandler|. @@ +360,5 @@ > + BT_LOGR("MAS folder-listing, current path: %s", > + NS_ConvertUTF16toUTF8(mCurrentPath).get()); > + HandleSmsMmsFolderListing(pktHeaders); > + } else if (type.EqualsLiteral("x-bt/MAP-msg-listing")) { > + } else if (type.EqualsLiteral("x-bt/message")) { Please leave comment if you'll implement in later patches. @@ +404,5 @@ > +} > + > +uint8_t > +BluetoothMapSmsManager::SetPath(uint8_t flags, > + const ObexHeaderSet& aHeader) nit: alignment @@ +406,5 @@ > +uint8_t > +BluetoothMapSmsManager::SetPath(uint8_t flags, > + const ObexHeaderSet& aHeader) > +{ > + // Section 5.2 "SetPat Function", MapSms 1.2 nit: SetPat'h' @@ +531,5 @@ > + > + req[3] = 0x10; // version=1.0 > + req[4] = 0x00; // flag=0x00 > + req[5] = BluetoothMapSmsManager::MAX_PACKET_LENGTH >> 8; > + req[6] = (uint8_t) BluetoothMapSmsManager::MAX_PACKET_LENGTH; nit: remove space after ). @@ +590,5 @@ > + > +void > +BluetoothMapSmsManager::CreateMnsObexConnection() > +{ > + if (!mMnsSocket) { Revise with guardian clause: if (mMnsSocket) { return; } mMnsSocket = new BluetoothSocket(this); mMnsSocket->Connect(mDeviceAddress, kMapMns, BluetoothSocketType::RFCOMM, -1, false, false); @@ +592,5 @@ > +BluetoothMapSmsManager::CreateMnsObexConnection() > +{ > + if (!mMnsSocket) { > + mMnsSocket = new BluetoothSocket(this); > + // Already encrypted in previous session? What's the ? comment for? @@ +601,5 @@ > + > +void > +BluetoothMapSmsManager::DestroyMnsObexConnection() > +{ > + if (mMnsSocket) { Revise with guardian clause: if (!mMnsSocket) { return; } mMnsSocket->Close(); @@ +604,5 @@ > +{ > + if (mMnsSocket) { > + mMnsSocket->Close(); > + } else { > + BT_LOGR("MSN client socket is not opened"); nit: MNS @@ +652,5 @@ > + > +void > +BluetoothMapSmsManager::HandleSmsMmsFolderListing(const ObexHeaderSet& aHeader) > +{ > + MOZ_ASSERT(NS_IsMainThread()); nit: add newline after assertion. @@ +662,5 @@ > + int index = 3; > + > + foldersize = mCurrentFolder->GetSubFolderSize(); > + BT_LOGR("CurrentPath: %s, sub folder size: %d", > + NS_ConvertUTF16toUTF8(mCurrentPath).get(), foldersize); Move this section to below. @@ +664,5 @@ > + foldersize = mCurrentFolder->GetSubFolderSize(); > + BT_LOGR("CurrentPath: %s, sub folder size: %d", > + NS_ConvertUTF16toUTF8(mCurrentPath).get(), foldersize); > + > + if (aHeader.GetAppParameter(Map::AppParametersTagId::MaxListCount, Declare |maxListCount| right before if to close where it's first used. @@ +671,5 @@ > + // convert big endian to little endian > + maxListCount = (maxListCount >> 8) | (maxListCount << 8); > + } > + > + if (aHeader.GetAppParameter(Map::AppParametersTagId::StartOffset, Ditto. @@ +678,5 @@ > + // convert big endian to little endian > + startOffset = (startOffset >> 8) | (startOffset << 8); > + } > + > + // folder listing size Revise as following: // Folder listing size int foldersize = mCurrentFolder->GetSubFolderSize(); BT_LOGR("CurrentPath: %s, sub folder size: %d", NS_ConvertUTF16toUTF8(mCurrentPath).get(), foldersize); // Convert little endian to big endian uint8_t folderListingSizeValue[2]; folderListingSizeValue[0] = (foldersize & 0xFF00) >> 8; folderListingSizeValue[1] = (foldersize & 0x00FF); // Section 3.3.4 "GetResponse", IrOBEX 1.2 // [opcode:1][length:2][Headers:var] where // [FolderListingSize:4] = [tagId:1][length:1][value: 2] AppendAppParameter(appParameter, sizeof(appParameter), (uint8_t) Map::AppParametersTagId::FolderListingSize, folderListingSizeValue, sizeof(folderListingSizeValue)); @@ +680,5 @@ > + } > + > + // folder listing size > + // Section 3.3.4 "GetResponse", IrOBEX 1.2 > + // [opcode:1][length:2][Headers:var] where Where is [FolderListingSize:4]? @@ +689,5 @@ > + // Convert little endian to big endian > + folderListingSizeValue[0] = (foldersize & 0xFF00) >> 8; > + folderListingSizeValue[1] = (foldersize & 0x00FF); > + AppendAppParameter(appParameter, sizeof(appParameter), > + (uint8_t) Map::AppParametersTagId::FolderListingSize, nit: remove space after ) @@ +695,5 @@ > + > + index += AppendHeaderAppParameters(&resp[index], 255, appParameter, > + sizeof(appParameter)); > + > + if (maxListCount == 0) { if (!maxListCount) { @@ +744,5 @@ > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + > + uint8_t buf[64]; > + if (aHeader.GetAppParameter(Map::AppParametersTagId::NotificationStatus, Revise with guardian clause as following. Also remove |mNtfRegistered| since it's unused elsewhere. if (!aHeader.Get( ... )) { return; } if (buf[0]) { CreateMnsObexConnection(); } else { DestroyMnsObexConnection(); } @@ +749,5 @@ > + buf, 64)) { > + mNtfRegistered = buf[0]; > + /* > + * Initialization sequence for a MAP session that uses both the Messsage > + * Access service and the Message Notification service. The MSN connection MNS @@ +751,5 @@ > + /* > + * Initialization sequence for a MAP session that uses both the Messsage > + * Access service and the Message Notification service. The MSN connection > + * shall be established by the first SetNotificationRegistration set to ON > + * during MAP session. Only one MSN connection per device pair. MNS @@ +790,5 @@ > + > +void > +BluetoothMapSmsManager::SendObexData(uint8_t* aData, uint8_t aOpcode, int aSize) > +{ > + mLastCommand = aOpcode; Store |mLastCommand| only for client as [1] since server replies also call |SendObexData|. [1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothOppManager.cpp#1402 Also will you add more client commands? This patch only implements |ObexRequestCode::Connect| and |ObexRequestCode::Disonnect|. @@ +835,5 @@ > + > + if (aSocket == mMnsSocket) { > + mMnsConnected = false; > + mMnsSocket = nullptr; > + BT_LOGR("MSN socket disconnected"); nit: MNS @@ +865,5 @@ > +NS_IMPL_ISUPPORTS(BluetoothMapSmsManager, nsIObserver) > + > +void > +BluetoothMapSmsManager::Connect(const nsAString& aDeviceAddress, > + BluetoothProfileController* aController) nit: alignment @@ +873,5 @@ > + > +void > +BluetoothMapSmsManager::OnGetServiceChannel(const nsAString& aDeviceAddress, > + const nsAString& aServiceUuid, > + int aChannel) nit: alignment ::: dom/bluetooth/bluedroid/BluetoothMapSmsManager.h @@ +4,5 @@ > + * License, v. 2.0. If a copy of the MPL was not distributed with this file, > + * You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#ifndef mozilla_dom_bluetooth_bluetoothmapsmsmanager_h__ > +#define mozilla_dom_bluetooth_bluetoothmapsmsmanager_h__ Ditto. Revise to |mozilla_dom_bluetooth_bluedroid_BluetoothMapSmsManager_h|. @@ +46,5 @@ > +}; > + > +class BluetoothSocket; > +class ObexHeaderSet; > +class BluetoothNamedValue; nit: alphabetical order. @@ +50,5 @@ > +class BluetoothNamedValue; > + > +/* > + * BluetoothMapSmsManager acts as Message Server Equipment (MSE) and runs both > + * MAS server and MNS client to exchange Sms/MMS message. nit: SMS/MMS for consistency @@ +51,5 @@ > + > +/* > + * BluetoothMapSmsManager acts as Message Server Equipment (MSE) and runs both > + * MAS server and MNS client to exchange Sms/MMS message. > + * Remove this extra line. @@ +55,5 @@ > + * > + */ > + > +class BluetoothMapSmsManager : public BluetoothSocketObserver > + , public BluetoothProfileManagerBase nit: alignment @@ +71,5 @@ > + static const int SDP_MESSAGE_TYPE_EMAIL = 0x01; > + static const int SDP_MESSAGE_TYPE_SMS_GSM = 0x02; > + static const int SDP_MESSAGE_TYPE_SMS_CDMA = 0x04; > + static const int SDP_MESSAGE_TYPE_MMS = 0x08; > + // By defualt SMS/MMS is default support type Do you mean "By default SMS/MMS type is supported"? @@ +103,5 @@ > + void AfterMapSmsDisconnected(); > + void CreateMnsObexConnection(); > + void DestroyMnsObexConnection(); > + void SendMnsClientConnectRequest(); > + void SendMnsClientDisconnectRequest(); Rename to |SendMnsConnectRequest| since connect request implies client role. @@ +132,5 @@ > + > + // If a connection has been established, mSocket will be the socket > + // communicating with the remote socket. We maintain the invariant that if > + // mSocket is non-null, mServerSocket must be null (and vice versa). > + nsRefPtr<BluetoothSocket> mSocket; Rename to |mMasSocket|. @@ +137,5 @@ > + > + // Server socket. Once an inbound connection is established, it will hand > + // over the ownership to mSocket, and get a new server socket while Listen() > + // is called. > + nsRefPtr<BluetoothSocket> mServerSocket; Rename to |mMasServerSocket|. @@ +139,5 @@ > + // over the ownership to mSocket, and get a new server socket while Listen() > + // is called. > + nsRefPtr<BluetoothSocket> mServerSocket; > + > + // Message Notification service client socket nit: to lower case - 'n'otification @@ +145,5 @@ > +}; > + > +END_BLUETOOTH_NAMESPACE > + > +#endif Add // mozilla_dom_bluetooth_bluedroid_BluetoothMapSmsManager_h ::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp @@ +1918,5 @@ > BT_LOGR("Fail to start BluetoothPbapManager listening"); > } > + > + BluetoothMapSmsManager* map = BluetoothMapSmsManager::Get(); > + BT_LOGR("Start BluetoothMapSmsManager listening"); Move this log into |BluetoothMapSmsManager::Listen|. @@ +2007,5 @@ > BT_LOGR("Fail to start BluetoothPbapManager listening"); > } > + > + BluetoothMapSmsManager* map = BluetoothMapSmsManager::Get(); > + BT_LOGR("Start BluetoothMapSmsManager listening"); Ditto.
Attachment #8644904 - Flags: review?(btian)
Comment on attachment 8644904 [details] [diff] [review] Bug 1166645 - Implement MAP profile manager connection related function Review of attachment 8644904 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/bluedroid/BluetoothMapFolder.cpp @@ +51,5 @@ > + // Based on Element Specification, 9.1.1, IrObex 1.2 > + nsAutoCString folderListingObejct("<?xml version=\"1.0\"?>\n"); > + folderListingObejct. > + Append("<!DOCTYPE folder-listing SYSTEM \" obex-folder-listing.dtd\">\n"); > + folderListingObejct.Append("<folder-listing version=\"1.0\">\n"); Do you want to put "<?xml version...<folder-listing version=\"1.0\">\n" into FOLDER_LIST_PREFIX? I guess that will be too long and not able to read easily. ::: dom/bluetooth/bluedroid/BluetoothMapSmsManager.cpp @@ +231,5 @@ > + const uint8_t* data = aMessage->GetData(); > + uint8_t opCode = data[0]; > + > + // Check response code and send out system message as finished if the > + // response code is somehow incorrect. Thanks! I forgot to remove this comment. @@ +240,5 @@ > + mLastCommand == ObexRequestCode::Put || > + mLastCommand == ObexRequestCode::Abort || > + mLastCommand == ObexRequestCode::PutFinal) { > + SendMnsClientDisconnectRequest(); > + } I actually reference this code from |BluetoothOppManager::ClientDataHandler|. And I think I'm wrong about |ObexRequestCode::Get|, MNS client doesn't support |Get| anyway. @@ +680,5 @@ > + } > + > + // folder listing size > + // Section 3.3.4 "GetResponse", IrOBEX 1.2 > + // [opcode:1][length:2][Headers:var] where I don't understand this question. @@ +744,5 @@ > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + > + uint8_t buf[64]; > + if (aHeader.GetAppParameter(Map::AppParametersTagId::NotificationStatus, Will it be good to keep |mNtfRegistered| so we can keep track that MCE already sent MAP-NotificationRegistration request? Maybe we shall handle one case that registered but fail to get connected. @@ +790,5 @@ > + > +void > +BluetoothMapSmsManager::SendObexData(uint8_t* aData, uint8_t aOpcode, int aSize) > +{ > + mLastCommand = aOpcode; MNS client shall support connect/disconnect/put/abort operations (see 6.2.2 Table 6.2). And I handle |MnsDataHandler| to disconnect obex if not success. Does it make sense? I think I will use |mMnsConnected| to check. @@ +835,5 @@ > + > + if (aSocket == mMnsSocket) { > + mMnsConnected = false; > + mMnsSocket = nullptr; > + BT_LOGR("MSN socket disconnected"); I guess Schizophrenia is hunting on me. ::: dom/bluetooth/bluedroid/BluetoothMapSmsManager.h @@ +20,5 @@ > + MaxListCount = 0x1, > + StartOffset = 0x2, > + FilterMessageType = 0x3, > + FilterPeriodBegin = 0x4, > + EndFilterPeriodEnd = 0x5, I found the specification 6.3.1 Table 6.5 is wrong. 'EndFilterPeriodEnd' shall be 'FilterPeriodEnd'. ::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp @@ +1918,5 @@ > BT_LOGR("Fail to start BluetoothPbapManager listening"); > } > + > + BluetoothMapSmsManager* map = BluetoothMapSmsManager::Get(); > + BT_LOGR("Start BluetoothMapSmsManager listening"); I think I should remove this log.
Attachment #8644904 - Flags: feedback?(jaliu)
Attachment #8646940 - Flags: review?(btian)
Attachment #8646940 - Flags: feedback?(jaliu)
(In reply to Shawn Huang [:shawnjohnjr] from comment #11) > ::: dom/bluetooth/bluedroid/BluetoothMapFolder.cpp > @@ +51,5 @@ > > + // Based on Element Specification, 9.1.1, IrObex 1.2 > > + nsAutoCString folderListingObejct("<?xml version=\"1.0\"?>\n"); > > + folderListingObejct. > > + Append("<!DOCTYPE folder-listing SYSTEM \" obex-folder-listing.dtd\">\n"); > > + folderListingObejct.Append("<folder-listing version=\"1.0\">\n"); > > Do you want to put "<?xml version...<folder-listing version=\"1.0\">\n" into > FOLDER_LIST_PREFIX? I guess that will be too long and not able to read > easily. You can format in string definition for better readability. > @@ +680,5 @@ > > + } > > + > > + // folder listing size > > + // Section 3.3.4 "GetResponse", IrOBEX 1.2 > > + // [opcode:1][length:2][Headers:var] where > > I don't understand this question. Please state in comment where [FolderListingSize:4] is. > @@ +744,5 @@ > > +{ > > + MOZ_ASSERT(NS_IsMainThread()); > > + > > + uint8_t buf[64]; > > + if (aHeader.GetAppParameter(Map::AppParametersTagId::NotificationStatus, > > Will it be good to keep |mNtfRegistered| so we can keep track that MCE > already sent MAP-NotificationRegistration request? Maybe we shall handle one > case that registered but fail to get connected. Agree. But please add the corresponding code together in one patch. > @@ +790,5 @@ > > + > > +void > > +BluetoothMapSmsManager::SendObexData(uint8_t* aData, uint8_t aOpcode, int aSize) > > +{ > > + mLastCommand = aOpcode; > > MNS client shall support connect/disconnect/put/abort operations (see 6.2.2 > Table 6.2). And I handle |MnsDataHandler| to disconnect obex if not success. > Does it make sense? I think I will use |mMnsConnected| to check. So more command implementation will come in later patches. Please separate outgoing requests and replies to remember only requests.
Attachment #8646940 - Attachment is obsolete: true
Attachment #8646940 - Flags: review?(btian)
Attachment #8646940 - Flags: feedback?(jaliu)
Comment on attachment 8647341 [details] [diff] [review] bug1166645-mc.patch (v3) Review of attachment 8647341 [details] [diff] [review]: ----------------------------------------------------------------- Please see my comment below. ::: dom/bluetooth/bluedroid/BluetoothMapFolder.cpp @@ +10,5 @@ > + > +#define FOLDER_LISTING_PREFIX \ > + "<?xml version=\"1.0\"?>\n \ > + <!DOCTYPE folder-listing SYSTEM \" obex-folder-listing.dtd\">\n \ > + <folder-listing version=\"1.0\">\n" nit: two-space indention @@ +20,5 @@ > +BluetoothMapFolder::~BluetoothMapFolder() > +{ } > + > +BluetoothMapFolder::BluetoothMapFolder(const nsAString& aFolderName, > + BluetoothMapFolder* aParent) nit: align with ( ::: dom/bluetooth/bluedroid/BluetoothMapFolder.h @@ +18,5 @@ > +class BluetoothMapFolder > +{ > +public: > + NS_INLINE_DECL_THREADSAFE_REFCOUNTING(BluetoothMapFolder) > + BluetoothMapFolder(); Can we remove the constructor with default parameter values? @@ +23,5 @@ > + ~BluetoothMapFolder(); > + > + BluetoothMapFolder(const nsAString& aFolderName, BluetoothMapFolder* aParent); > + // Add virtual folder to subfolders > + void AddFolder(const nsAString& aFolderName); Rename to |AddSubFolder| to be clearer. @@ +25,5 @@ > + BluetoothMapFolder(const nsAString& aFolderName, BluetoothMapFolder* aParent); > + // Add virtual folder to subfolders > + void AddFolder(const nsAString& aFolderName); > + BluetoothMapFolder* GetSubFolder(const nsAString& aFolderName); > + int GetSubFolderSize(); Rename to |GetSubFolderCount| since folder size may be mis-regarded as sum of file sizes under the subfolder. ::: dom/bluetooth/bluedroid/BluetoothMapSmsManager.cpp @@ +32,5 @@ > + 0x80, 0x00, 0x00, 0x80, 0x5F, 0x9B, 0x34, 0xFB > + } > + }; > + > + // UUID of Map Mas nit: put Map Mas UUID before MNS one. @@ +203,5 @@ > + */ > + sdpString.AppendLiteral("SMS/MMS Message Access"); > + nsresult rv = mMasServerSocket->Listen(sdpString, kMapMas, > + BluetoothSocketType::RFCOMM, -1, false, > + true); nit: alignment @@ +225,5 @@ > + > + const uint8_t* data = aMessage->GetData(); > + uint8_t opCode = data[0]; > + if (opCode != ObexResponseCode::Success) { > + BT_LOGR("Not expectedOpCode: %x", opCode); Revise to "Unexpected opCode" since "Not expectedOpCode: %x" is ambiguous that whether %x is expected or not. @@ +411,5 @@ > + return true; > +} > + > +uint8_t > +BluetoothMapSmsManager::SetPath(uint8_t flags, Integrate |mCurrentPath| into |mCurrentFolder| in this function. @@ +463,5 @@ > + > + mCurrentPath = newPath; > + BT_LOGR("MAS Current path [%s]", NS_ConvertUTF16toUTF8(mCurrentPath).get()); > + > + if (mCurrentPath.IsEmpty()) { Where do you handle "go up 1 level"? @@ +615,5 @@ > + if (!mMnsSocket) { > + return; > + } > + > + mMnsSocket->Close(); Reset |mMnsSocket| to nullptr. @@ +682,5 @@ > + startOffset = (startOffset >> 8) | (startOffset << 8); > + } > + > + // folder listing size > + int foldersize = mCurrentFolder->GetSubFolderSize(); nit: revise as following to be clearer // Folder listing size int foldersize = mCurrentFolder->GetSubFolderSize(); BT_LOGR("CurrentPath: %s, sub folder size: %d", NS_ConvertUTF16toUTF8(mCurrentPath).get(), foldersize); // Convert little endian to big endian uint8_t folderListingSizeValue[2]; folderListingSizeValue[0] = (foldersize & 0xFF00) >> 8; folderListingSizeValue[1] = (foldersize & 0x00FF); @@ +698,5 @@ > + AppendAppParameter(appParameter, sizeof(appParameter), > + (uint8_t)Map::AppParametersTagId::FolderListingSize, > + folderListingSizeValue, sizeof(folderListingSizeValue)); > + > + index += AppendHeaderAppParameters(&resp[index], 255, appParameter, Declare |resp| and |index| here, right before where they are first used. @@ +712,5 @@ > + NS_ConvertUTF16toUTF8(output).get()), > + NS_ConvertUTF16toUTF8(output).Length()); > + > + index += AppendHeaderEndOfBody(&resp[index]); > + SendMasObexData(resp, ObexResponseCode::Success, index); Revise as following: /* MCE wants to query sub-folder size * FolderListingSize AppParameter shall be used in the response if the value * of MaxListCount in the request is 0. If MaxListCount = 0, the MSE shall * ignore all other applications parameters that may be presented in the * request. The response shall contain any Body header. */ if (!maxListCount) { ... } SendObexData(resp, ObexResponseCode::Success, index); @@ +745,5 @@ > +} > + > +void > +BluetoothMapSmsManager::HandleNotificationRegistration(const ObexHeaderSet& > + aHeader) nit: align as following BluetoothMapSmsManager::HandleNotificationRegistration( const ObexHeaderSet& aHeader) @@ +756,5 @@ > + return; > + } > + > + bool ntfRegistered = static_cast<bool>(buf[0]); > + nit: remove this newline. @@ +783,5 @@ > + > +void > +BluetoothMapSmsManager::HandleEventReport(const ObexHeaderSet& aHeader) > +{ > + // TODO: Handle Handle event report in Bug 1166666 nit: remove extra 'Handle' @@ +826,5 @@ > +void > +BluetoothMapSmsManager::OnSocketConnectSuccess(BluetoothSocket* aSocket) > +{ > + MOZ_ASSERT(aSocket); > + MOZ_ASSERT(!mMasSocket); Remove this assertion since |mMasSocket| is non-nullptr when |mMnsSocket| connect success. @@ +828,5 @@ > +{ > + MOZ_ASSERT(aSocket); > + MOZ_ASSERT(!mMasSocket); > + > + if (aSocket == mMnsSocket) { Add following comment: // MNS socket is connected if (aSocket == mMnsSocket) { @@ +834,5 @@ > + SendMnsConnectRequest(); > + return; > + } > + > + // Close server socket as only one session is allowed at a time Add following comment: // MAS socket is connected // Close server socket as only one session is allowed at a time @@ +845,5 @@ > + > +void > +BluetoothMapSmsManager::OnSocketConnectError(BluetoothSocket* aSocket) > +{ > + if (aSocket == mMnsSocket) { Add following comment: // MNS socket connection error if (aSocket == mMnsSocket) { @@ +846,5 @@ > +void > +BluetoothMapSmsManager::OnSocketConnectError(BluetoothSocket* aSocket) > +{ > + if (aSocket == mMnsSocket) { > + mMnsConnected = false; Should we reset |mMnsSocket| as nullptr here? @@ +859,5 @@ > +BluetoothMapSmsManager::OnSocketDisconnect(BluetoothSocket* aSocket) > +{ > + MOZ_ASSERT(aSocket); > + > + if (aSocket == mMnsSocket) { Add following comment: // MNS socket is disconnected if (aSocket == mMnsSocket) { @@ +866,5 @@ > + BT_LOGR("MNS socket disconnected"); > + return; > + } > + > + if (aSocket != mMasSocket) { Add following comment: // MAS server socket is closed if (aSocket != mMasSocket) { @@ +871,5 @@ > + // Do nothing when a listening server socket is closed. > + return; > + } > + > + AfterMapSmsDisconnected(); Add following comment: // MAS socket is disconnected AfterMapSmsDisconnected(); @@ +881,5 @@ > + > +void > +BluetoothMapSmsManager::Disconnect(BluetoothProfileController* aController) > +{ > + if (mMasSocket) { Revise with guardian clause: if (!mMasSocket) { BT_WARNING("%s: No ongoing connection to disconnect", __FUNCTION__); return; } mMasSocket->Close(); ::: dom/bluetooth/bluedroid/BluetoothMapSmsManager.h @@ +116,5 @@ > + void HandleSmsMmsFolderListing(const ObexHeaderSet& aHeader); > + /** > + * Current virtual folder path > + */ > + nsString mCurrentPath; Remove |mCurrentPath| and remember current folder in |mCurrentFolder|. @@ +129,5 @@ > + * Record the last command > + */ > + int mLastCommand; > + bool mMnsConnected; > + bool mNtfRegistered; Rename to |mNtfRequired|.
Attachment #8647341 - Flags: review?(btian)
Attached patch bug1166645-mc.patch (v4) (obsolete) — — Splinter Review
I rebased to the latest m-c, and test with MCE equipment.
Attachment #8647934 - Attachment is obsolete: true
Attachment #8647934 - Flags: review?(btian)
Comment on attachment 8647953 [details] [diff] [review] bug1166645-mc.patch (v4) Review of attachment 8647953 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/bluedroid/BluetoothMapFolder.cpp @@ +41,5 @@ > + return mParent; > +} > + > +void > +BluetoothMapFolder::GetFolderName(nsString& aFolderName) Replace this function with |PrintFolderName()| to print out folder name directly. ::: dom/bluetooth/bluedroid/BluetoothMapSmsManager.cpp @@ +432,5 @@ > + // Go up 1 level > + BluetoothMapFolder* parent = mCurrentFolder->GetParentFolder(); > + if (!parent) { > + BT_LOGR("MAS SetPath Go up 1 level"); > + mCurrentFolder = parent; nit: assign first and then print log to conform with below. @@ +447,5 @@ > + BT_LOGR("MAS SetPath Go back to root"); > + } else { > + // Go down 1 level > + BluetoothMapFolder* child = mCurrentFolder->GetSubFolder(childFolderName); > + nit: remove this newline @@ +449,5 @@ > + // Go down 1 level > + BluetoothMapFolder* child = mCurrentFolder->GetSubFolder(childFolderName); > + > + if (!child) { > + return ObexResponseCode::NotFound; Print log BT_LOGR("Illegal subfolder name [%s]", NS_ConvertUTF16toUTF8(childFolderName)); @@ +667,5 @@ > + folderListingSizeValue, sizeof(folderListingSizeValue)); > + > + uint8_t resp[255]; > + int index = 3; > + nit: remove this newline. @@ +677,5 @@ > + * If MaxListCount = 0, the MSE shall ignore all other applications > + * parameters that may be presented in the request. The response shall > + * contain any Body header. > + */ > + if (maxListCount) { The condition was |!maxListCount| in previous patch. Which one is correct? @@ +740,5 @@ > + * during MAP session. Only one MNS connection per device pair. > + * Section 6.4.2, MAP > + * If the Message Access connection is disconnected after Message Notification > + * connection establishment, this will automatically indicate a MAS > + * Notification-Deregistration for this MAS instance. Where do we do auto de-registration? @@ +821,5 @@ > + mMnsSocket = nullptr; > + return; > + } > + > + mMasServerSocket = nullptr; Add comment // MAS socket connection error @@ +856,5 @@ > +void > +BluetoothMapSmsManager::Disconnect(BluetoothProfileController* aController) > +{ > + if (!mMasSocket) { > + BT_WARNING("%s: No ongoing connection to disconnect", __FUNCTION__); nit: remove extra 1-space indention. ::: dom/bluetooth/bluedroid/BluetoothMapSmsManager.h @@ +120,5 @@ > + nsRefPtr<BluetoothMapFolder> mRootFolder; > + /** > + * OBEX session status. Set when OBEX session is established. > + */ > + bool mConnected; Rename to |mMasConnected| and move declaration close to |mMnsConnected|.
(In reply to Ben Tian [:btian] from comment #20) > Comment on attachment 8647953 [details] [diff] [review] > bug1166645-mc.patch (v4) > > Review of attachment 8647953 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/bluetooth/bluedroid/BluetoothMapFolder.cpp > @@ +41,5 @@ > > + return mParent; > > +} > > + > > +void > > +BluetoothMapFolder::GetFolderName(nsString& aFolderName) > > Replace this function with |PrintFolderName()| to print out folder name > directly. > > ::: dom/bluetooth/bluedroid/BluetoothMapSmsManager.cpp > @@ +432,5 @@ > > + // Go up 1 level > > + BluetoothMapFolder* parent = mCurrentFolder->GetParentFolder(); > > + if (!parent) { > > + BT_LOGR("MAS SetPath Go up 1 level"); > > + mCurrentFolder = parent; > > nit: assign first and then print log to conform with below. > > @@ +447,5 @@ > > + BT_LOGR("MAS SetPath Go back to root"); > > + } else { > > + // Go down 1 level > > + BluetoothMapFolder* child = mCurrentFolder->GetSubFolder(childFolderName); > > + > > nit: remove this newline > > @@ +449,5 @@ > > + // Go down 1 level > > + BluetoothMapFolder* child = mCurrentFolder->GetSubFolder(childFolderName); > > + > > + if (!child) { > > + return ObexResponseCode::NotFound; > > Print log > > BT_LOGR("Illegal subfolder name [%s]", > NS_ConvertUTF16toUTF8(childFolderName)); > > @@ +667,5 @@ > > + folderListingSizeValue, sizeof(folderListingSizeValue)); > > + > > + uint8_t resp[255]; > > + int index = 3; > > + > > nit: remove this newline. > > @@ +677,5 @@ > > + * If MaxListCount = 0, the MSE shall ignore all other applications > > + * parameters that may be presented in the request. The response shall > > + * contain any Body header. > > + */ > > + if (maxListCount) { > > The condition was |!maxListCount| in previous patch. Which one is correct? The previous v3 patch is wrong. I reversed the logic and use |if (!maxListCount)|. I did the full test and found v3 is with this bug. > > @@ +740,5 @@ > > + * during MAP session. Only one MNS connection per device pair. > > + * Section 6.4.2, MAP > > + * If the Message Access connection is disconnected after Message Notification > > + * connection establishment, this will automatically indicate a MAS > > + * Notification-Deregistration for this MAS instance. > > Where do we do auto de-registration? It looks like Android indeed close MNS connection after receiving MAS message notification-registration=off. http://androidxref.com/5.1.1_r6/xref/packages/apps/Bluetooth/src/com/android/bluetooth/map/BluetoothMnsObexClient.java#194 > > @@ +821,5 @@ > > + mMnsSocket = nullptr; > > + return; > > + } > > + > > + mMasServerSocket = nullptr; > > Add comment // MAS socket connection error OK. > > @@ +856,5 @@ > > +void > > +BluetoothMapSmsManager::Disconnect(BluetoothProfileController* aController) > > +{ > > + if (!mMasSocket) { > > + BT_WARNING("%s: No ongoing connection to disconnect", __FUNCTION__); > > nit: remove extra 1-space indention. > > ::: dom/bluetooth/bluedroid/BluetoothMapSmsManager.h > @@ +120,5 @@ > > + nsRefPtr<BluetoothMapFolder> mRootFolder; > > + /** > > + * OBEX session status. Set when OBEX session is established. > > + */ > > + bool mConnected; > > Rename to |mMasConnected| and move declaration close to |mMnsConnected|. Ok.
(In reply to Ben Tian [:btian] from comment #20) > Comment on attachment 8647953 [details] [diff] [review] > bug1166645-mc.patch (v4) > > Review of attachment 8647953 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/bluetooth/bluedroid/BluetoothMapFolder.cpp > @@ +41,5 @@ > > + return mParent; > > +} > > + > > +void > > +BluetoothMapFolder::GetFolderName(nsString& aFolderName) > > Replace this function with |PrintFolderName()| to print out folder name > directly. I prefer to keep this getter, but provide |Dump()| for debugging purpose.
(In reply to Ben Tian [:btian] from comment #20) > Comment on attachment 8647953 [details] [diff] [review] > bug1166645-mc.patch (v4) > > Review of attachment 8647953 [details] [diff] [review]: > ----------------------------------------------------------------- > @@ +740,5 @@ > > + * during MAP session. Only one MNS connection per device pair. > > + * Section 6.4.2, MAP > > + * If the Message Access connection is disconnected after Message Notification > > + * connection establishment, this will automatically indicate a MAS > > + * Notification-Deregistration for this MAS instance. > > Where do we do auto de-registration? I consider to drop MNS connection after MAS connection dropped to ensure MAS ntf-registration=off was not sent via MCE. But the current logic remains the same. When we received "x-bt/MAP-NotificationRegistration" type, we connect/disconnect MNS connection. If MAS connection already dropped but MNS is still connected, we disconnect anyway in |AfterMapSmsDisconnected|.
Attachment #8647953 - Attachment is obsolete: true
Attachment #8647953 - Flags: review?(btian)
Comment on attachment 8648056 [details] [diff] [review] bug1166645-mc.patch (v5) Review of attachment 8648056 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comment addressed. ::: dom/bluetooth/bluedroid/BluetoothMapFolder.cpp @@ +18,5 @@ > +{ > +} > + > +void > +BluetoothMapFolder::AddSubFolder(const nsAString& aFolderName) Return the added |folder| so you don't have to call |GetSubFolder| again. @@ +41,5 @@ > + return mParent; > +} > + > +void > +BluetoothMapFolder::GetFolderName(nsString& aFolderName) Remove this function since it's unused. ::: dom/bluetooth/bluedroid/BluetoothMapSmsManager.cpp @@ +690,5 @@ > + > +void > +BluetoothMapSmsManager::BuildDefaultFolderStructure() > +{ > + // MAP specification defines virtual folders structure List mandatory folders in comment as following. /** * MAP spec defined virtual folders structure: * / * /telecom * /telecom/msg * /telecom/msg/inbox * /telecom/msg/draft * /telecom/msg/outbox * /telecom/msg/sent * /telecom/msg/deleted */ @@ +694,5 @@ > + // MAP specification defines virtual folders structure > + mRootFolder = new BluetoothMapFolder(NS_LITERAL_STRING("root"), nullptr); > + mRootFolder->AddSubFolder(NS_LITERAL_STRING("telecom")); > + nsRefPtr<BluetoothMapFolder> folder; > + folder = mRootFolder->GetSubFolder(NS_LITERAL_STRING("telecom")); Integrate |GetSubFolder| into |GetSubFolder|. ::: dom/bluetooth/bluedroid/BluetoothMapSmsManager.h @@ +93,5 @@ > + > + void HandleNotificationRegistration(const ObexHeaderSet& aHeader); > + void HandleEventReport(const ObexHeaderSet& aHeader); > + void HandleMessageStatus(const ObexHeaderSet& aHeader); > + void ReplyError(uint8_t aError); nit: move this function right after |ReplyToPut| to group together. @@ +111,5 @@ > + /* > + * Build mandatory folders > + */ > + void BuildDefaultFolderStructure(); > + void HandleSmsMmsFolderListing(const ObexHeaderSet& aHeader); nit: move this function right after |HandleMessageStatus| to group together.
Attachment #8648056 - Flags: review?(btian) → review+
Hi Jamin, This patch depends on bug1180556 Obex append Appparameters. int AppendHeaderAppParameters(uint8_t* aRetBuf, int aBufferSize, const uint8_t* aAppParameters, int aLength); int AppendAppParameter(uint8_t* aRetBuf, int aBufferSize, const uint8_t aTagId, const uint8_t* aValue, int aLength); Can you separate OBEX change from bug1180556?
Flags: needinfo?(jaliu)
Clear the ni flag from #comment 28 since Bug 1180554 has been resolved.
Flags: needinfo?(6jamin)
(In reply to Shawn Huang [:shawnjohnjr] from comment #37) > https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&revision=3545d8b3257c All testfailed items are not related to this patch.
Due to the change for nsBaseHashtable, I need to rebase v2.2r and give up Iterator. :(
(In reply to Shawn Huang [:shawnjohnjr] from comment #41) > Due to the change for nsBaseHashtable, I need to rebase v2.2r and give up > Iterator. :( See Bug 1181445.
https://hg.mozilla.org/mozilla-central/rev/3545d8b3257c
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S7 (18Sep)
After rebase to v2.2r, now I can see MAP socket listening failure. It said rfc_channel: 1 already in use. I have no idea why it fails. ni myself to dig into the error before I uplift to v2.2r branch. This is sad. 09-10 03:39:40.040 D/bt-btif ( 1601): btsock_rfc_listen, service_name:SMS/MMS Message Access 09-10 03:39:40.040 D/bt-btif ( 1601): BTA_JvCreateRecordByUser:SMS/MMS Message Access 09-10 03:39:40.040 I/bt-btif ( 1601): BTA_JvCreateRecordByUser 09-10 03:39:40.040 D/bt-btif ( 1601): adding fd:32, flags:0x4 09-10 03:39:40.040 D/bt-btif ( 1601): cmd.id:3 09-10 03:39:40.040 I/bt-btif ( 1601): BTA got event 0x1a0e 09-10 03:39:40.040 D/bt-btif ( 1601): jv_dm_cback: event:11, slot id:3 09-10 03:39:40.040 E/bt-btif ( 1601): rfc channel:1 already in use 09-10 03:39:40.040 E/bt-btif ( 1601): jv_dm_cback: cannot start server, slot found:0xb6e1a4f4 09-10 03:39:40.040 D/bt-btif ( 1601): cleanup slot:3, fd:32, scn:1, sdp_handle:0x0 09-10 03:39:40.040 D/bt-btm ( 1601): BTM_FreeSCN 09-10 03:39:40.040 D/bt-btif ( 1601): print poll event:20 09-10 03:39:40.040 D/bt-btif ( 1601): POLLNVAL 09-10 03:39:40.040 W/bt-btif ( 1601): invalid rfc slot id: 3
Flags: needinfo?(shuang)
Clear ni, i found rebase and mess up parameter.
Flags: needinfo?(shuang)
blocking-b2g: --- → 2.2r?
Flags: needinfo?(whuang)
Flags: needinfo?(shuang)
blocking-b2g: 2.2r? → 2.2r+
Flags: needinfo?(whuang)
(In reply to Shawn Huang [:shawnjohnjr] from comment #47) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa751b2f066a I'm not sure why try server won't execute my request for v2.2r.
(In reply to Shawn Huang [:shawnjohnjr] from comment #48) > (In reply to Shawn Huang [:shawnjohnjr] from comment #47) > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa751b2f066a > > I'm not sure why try server won't execute my request for v2.2r. Any idea or I can ask anyone? I still don't see any result.
Flags: needinfo?(cbook)
Flags: needinfo?(cbook)
Sadly, I don't know why my rebase v2.2r patch became to m-c version. I lost all my change locally for v2.2r. I need to rebase again :(
Hey Shawn, yeah this didn't applied to b2g-i , but will checkin after you rebased. Thanks!
Flags: needinfo?(shuang)
Keywords: checkin-needed
Keywords: checkin-needed
Whiteboard: needs-uplift
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: