932192 - [Bluetooth][Gecko] Refactor OPP manager for multiple file transfer

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Ben Tian (inactive)]

Refactor gecko bluetooth first for multiple file transfer. Keep the API and behavior consistent as before.

[TODO in bug 897782]
Revise opp manager and profile controller after gaia is ready:
- remove onconnect/ondisconnect and connect/disconnect functions from opp manager
- remove opp from profile controller

Comment 1

[Ben Tian (inactive)]

Attachment: Patch 1 (v1): OPP mgr for multiple file transfer

Changes:
- Add function ConnectInternal() for real socket connecting; Connect() function becomes a dummy function.
- Add class SendFileBatch to remember device address and blobs for queuing.
- Add logs when
  1) socket connects/disconnects/encounters error
  2) discard remaing blobs to send
  3) process next SendFileBatch
- Fix warnings

Comment 2

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 823872 [details] [diff] [review]
Patch 1 (v1): OPP mgr for multiple file transfer

Review of attachment 823872 [details] [diff] [review]:
-----------------------------------------------------------------

ok, overall looks good. I'd like to review once again after the new patch is ready. In addition, once Connect() is no more used in Bluetooth File Transfer, we could remove BluetoothProfileController related implementation then. Please file a followup to track on this. Thanks.

::: dom/bluetooth/BluetoothOppManager.cpp
@@ +51,5 @@
>  static bool sInShutdown = false;
>  }
>  
> +class mozilla::dom::bluetooth::SendFileBatch {
> +  public:

super-nit: do not indent for the whole class, please.

@@ +399,5 @@
>    MOZ_ASSERT(NS_IsMainThread());
>  
> +  AppendBlobToSend(aDeviceAddress, aActor);
> +  if (!mSocket) {
> +    ProcessNextBatch();

Question: this may fail. Do we need error handling here?

@@ +408,5 @@
>  
> +void
> +BluetoothOppManager::AppendBlobToSend(const nsAString& aDeviceAddress,
> +                                      BlobParent* aActor)
> +{

nit: please add threading assertion check.

@@ +439,5 @@
> +    BT_LOGR("%s: idx %d", __FUNCTION__, mCurrentBlobIndex);
> +    ExtractBlobHeaders();
> +    StartFileTransfer();
> +
> +    mSendTransferCompleteFlag = false;

Well, setting mSendTransferCompleteFlag to false here is really awkward. I know it's because mSendTransferCompleteFlag will be checked at the beginning of FileTransferComplete(), but it's still not easy to understand.

Since mSendTransferCompleteFlag would be set to true in FileTransferComplete(), how about setting it to false in StartFileTransfer()? It seems to be more reasonable.

Comment 3

[Ben Tian (inactive)]

> ok, overall looks good. I'd like to review once again after the new patch is
> ready. In addition, once Connect() is no more used in Bluetooth File
> Transfer, we could remove BluetoothProfileController related implementation
> then. Please file a followup to track on this. Thanks.

The followup bug is bug 897782. I'll remove unused functions once gaia part (bug 929306) is done.
 
> > +  AppendBlobToSend(aDeviceAddress, aActor);
> > +  if (!mSocket) {
> > +    ProcessNextBatch();
> 
> Question: this may fail. Do we need error handling here?

No need. ProcessNextBatch() here always returns true (means next batch is in queue) as a new batch is just enqueued.

Comment 4

[Ben Tian (inactive)]

> > > +  AppendBlobToSend(aDeviceAddress, aActor);
> > > +  if (!mSocket) {
> > > +    ProcessNextBatch();
> > 
> > Question: this may fail. Do we need error handling here?
> 
> No need. ProcessNextBatch() here always returns true (means next batch is in
> queue) as a new batch is just enqueued.

Correction: you are right. I missed the socket connection failure case. I'll handle the possible failure in next patch.

Comment 5

[Ben Tian (inactive)]

Attachment: Patch 1 (v2): OPP mgr for multiple file transfer

Changes:
- Revise ConnectInternal()
  > void return. ProcessNextBatch() returns true only if next batch is in queue.
  > handle errors with OnSocketConnectError()
  > close server sockets first in case OnSocketConnectError() nullifies them before disconnection
- Add thread safe assertion in functions using mBatches
- Move setting of mSendTransferCompleteFlag=false into StartFileTransfer()
- Fix nits

Comment 6

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 825698 [details] [diff] [review]
Patch 1 (v2): OPP mgr for multiple file transfer

Review of attachment 825698 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Thanks.

Comment 7

[Ben Tian (inactive)]

try server link: https://tbpl.mozilla.org/?tree=Try&rev=3795df0dac89

Comment 8

[Eric Chou [:ericchou] [:echou]]

https://hg.mozilla.org/integration/b2g-inbound/rev/c01c64954f59

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c01c64954f59