[b2g-bluetooth] Save received file

RESOLVED FIXED in Firefox 18

Status

()

defect
P1
normal
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: echou, Assigned: echou)

Tracking

unspecified
mozilla19
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

Details

(Whiteboard: [LOE:S])

Attachments

(1 attachment, 5 obsolete attachments)

Now we can get full byte stream of the file sent from the remote device, so the next step is to save it to local storage.
Assignee

Updated

7 years ago
Assignee: nobody → echou
Blocks: 792683
blocking-basecamp: --- → ?
Whiteboard: [LOE:S]
Assignee

Comment 1

7 years ago
Attachment #673310 - Flags: review?(kyle)
Comment on attachment 673310 [details] [diff] [review]
patch 1: v1: Save the file from byte stream

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

So the OPP operations look good, but I'd like to get another opinion on how the file writing happens, just because I have zero experience with that. Trying to track someone down for that. Will r+ and add addl. review once I find someone.

::: dom/bluetooth/ObexBase.cpp
@@ +115,4 @@
>    }
>  }
>  
> +// -1 means cannot find header "Body"

Nit: Is there an error code enum or something we should be doing?
Assignee

Comment 3

7 years ago
> ::: dom/bluetooth/ObexBase.cpp
> @@ +115,4 @@
> >    }
> >  }
> >  
> > +// -1 means cannot find header "Body"
> 
> Nit: Is there an error code enum or something we should be doing?
Probably not. -1 here means that "we couldn't find header BODY in this packet", and it's o.k for remote devices sending a packet with only other headers(e.g. NAME, TYPE, LENGTH ...). So, as you can see, in BluetoothOppManager, we still continue processing this packet even GetHeaderBodyOffset returned -1, just not write anything to the output file.
Assignee

Comment 4

7 years ago
* It will send the system message "UpdateProgress" during receiving
* Slightly clean up
Attachment #673310 - Attachment is obsolete: true
Attachment #673310 - Flags: review?(kyle)
Attachment #673681 - Flags: review?(kyle)
File transfers require saving of files so blocking.
blocking-basecamp: ? → +
Priority: -- → P1
Comment on attachment 673681 [details] [diff] [review]
patch 1: v2: Save the file sent from remote device

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

::: dom/bluetooth/BluetoothOppManager.cpp
@@ +35,5 @@
>  static uint32_t sFileLength = 0;
>  static nsString sContentType;
>  static int sUpdateProgressCounter = 0;
> +static nsCOMPtr<nsIFile> sFile = nullptr;
> +static nsCOMPtr<nsIOutputStream> outputStream = nullptr;

static nsCOMPtr<>'s are bad.  See bug 60697.  The existing ones should have been r-'ed in review.

@@ +424,5 @@
> +
> +        // Create a file in kRootDirectory
> +        nsString path;
> +
> +        path.AssignASCII(kRootDirectory);

AssignLiteral()

@@ +433,5 @@
> +          NS_WARNING("Couldn't new a local file");
> +        }
> +
> +        // FIXME: Need a mechanism to save a file which has the
> +        // same name as an existing file

call Delete() first?  Open the file and write into it?

@@ +468,5 @@
> +
> +        int headerBodyOffset = GetHeaderBodyOffset(&aMessage->mData[headerStartIndex],
> +                                                   receivedLength - headerStartIndex);
> +        if (headerBodyOffset != -1) {
> +          /* 

trailing whitespace
Comment on attachment 673681 [details] [diff] [review]
patch 1: v2: Save the file sent from remote device

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

r?'ing for nsCOMPtr fixes. Can probably r+ after that though.
Attachment #673681 - Flags: review?(kyle)
Assignee

Comment 8

7 years ago
* Moved those static nsCOMPtrs to class BluetoothOppManager
* Fix nit: AssignASCII => AssignLiteral
* Fix nit: trailing whitespace
* In this patch(v3), I used [timestamp + sent file name] as the name of the saved file. I think it can effectively solve the problem of file name conflict.
Attachment #673681 - Attachment is obsolete: true
Attachment #674654 - Flags: review?(kyle)
Attachment #674654 - Flags: review?(doug.turner)
Attachment #674654 - Flags: review?(kyle) → review+
Assignee

Updated

7 years ago
Blocks: 802080
Assignee

Updated

7 years ago
Blocks: 805724
Comment on attachment 674654 [details] [diff] [review]
patch 1: v3: Save the file sent from remote device

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

I commented on some things that clearly aren't your doing.  Bonus points for fixing them up.

::: dom/bluetooth/BluetoothOppManager.cpp
@@ +32,1 @@
>  static const uint32_t kUpdateProgressBase = 50 * 1024;

what is this about?  Why 50k?

@@ +35,4 @@
>  static uint32_t sSentFileLength = 0;
>  static nsString sFileName;
>  static uint32_t sFileLength = 0;
>  static nsString sContentType;

Do these statics mean that we only can do one file at a time?  That *can't* be good.

@@ +98,5 @@
>                                             , mPutFinal(false)
>                                             , mWaitingForConfirmationFlag(false)
> +                                           , mFile(nullptr)
> +                                           , mOutputStream(nullptr)
> +                                           , mInputStream(nullptr)

You do not need to assign nsCOMPtr<> to null here.

@@ +243,5 @@
>  }
>  
> +void
> +BluetoothOppManager::AfterOppConnected()
> +{

please add MOZ_ASSERT(NS_IsMainThread()); here

@@ +252,5 @@
> +}
> +
> +void
> +BluetoothOppManager::AfterOppDisconnected()
> +{

same.  add MOZ_ASSERT(NS_IsMainThread());


You should also change the silent failure in ReadFileTask::Run to be an assertion.

@@ +257,5 @@
> +  mConnected = false;
> +  mReceiving = false;
> +  mLastCommand = 0;
> +  mBlob = nullptr;
> +  mReadFileThread = nullptr;

Don't you need to Cancel()?

@@ +357,2 @@
>  
> +    if (mAbortFlag || mReadFileThread == nullptr) {

s/ mReadFileThread == nullptr/ mReadFileThread

@@ +357,3 @@
>  
> +    if (mAbortFlag || mReadFileThread == nullptr) {
> +      SendAbortRequest();

It's hard to read from here, but don't you just want to return after posting the alert request?

@@ +402,2 @@
>      if (opCode == ObexRequestCode::Connect) {
>        ParseHeaders(&aMessage->mData[7], receivedLength - 7, &pktHeaders);

Why 7?

@@ +406,2 @@
>      } else if (opCode == ObexRequestCode::Disconnect) {
>        ParseHeaders(&aMessage->mData[3], receivedLength - 3, &pktHeaders);

Why 3?

@@ +410,3 @@
>      } else if (opCode == ObexRequestCode::Put ||
>                 opCode == ObexRequestCode::PutFinal) {
> +      int headerStartIndex = 3;

why 3?  Magic numbers need comments.

@@ +420,5 @@
>          pktHeaders.GetContentType(sContentType);
>          pktHeaders.GetLength(&sFileLength);
>  
> +        nsString path, localFileName;
> +        path.AssignLiteral("/sdcard/download/bluetooth/");

You should put this literal at the top of this file (or a preference).

@@ +425,5 @@
> +
> +        // Use timestamp as a part of the filename. If the name still
> +        // conflicts with an existing file, just return an error.
> +        PRExplodedTime now;
> +        PR_ExplodeTime(PR_Now(), PR_LocalTimeParameters, &now);

Would http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsIFile.idl#271 be better?

::: dom/bluetooth/ObexBase.cpp
@@ +115,4 @@
>    }
>  }
>  
> +// -1 means cannot find header "Body"

Should move a comment like this into the header.

@@ +116,5 @@
>  }
>  
> +// -1 means cannot find header "Body"
> +int
> +GetHeaderBodyOffset(uint8_t* headerStart, int totalLength)

Sorry, I don't really understand the Obex format.  Does a header always preceed the body?  If so, maybe you can make reuse ParseHeaders().  Basically that code that this are mostly identical.

How about you do something like:

Can you change/reused ParseHeaders() such that 
  a) it returns the header a contentLength of the header.
  b) can take a null ObexHeaderSet if you just want to get the length

I think if you do this (assuming that headers precede bodies), you don't need this extra code.
Attachment #674654 - Flags: review?(doug.turner) → review-
* Added assertion checks
* Added comments for those magic numbers
* Modified function ParseHeaders and removed GetHeaderBodyOffset()
* Used function nsIFile.CreateUnique() to create a file with an unique file name. (Thanks for your advice of this, Doug.)
* Some other nits picked.

Also, let me explain why I didn't change some places you mentioned:

> ::: dom/bluetooth/BluetoothOppManager.cpp
> @@ +32,1 @@
> >  static const uint32_t kUpdateProgressBase = 50 * 1024;
> 
> what is this about?  Why 50k?
> 
We need to send notifications to Gaia to inform the user of file transferring progress, and currently we will send one notification at each 50kb. Basically, it's another magic number, just can't think of any other better ideas now. :)

> >  static uint32_t sSentFileLength = 0;
> >  static nsString sFileName;
> >  static uint32_t sFileLength = 0;
> >  static nsString sContentType;
> 
> Do these statics mean that we only can do one file at a time?  That *can't*
> be good.

Unfortunately, yes. For current implementation, we only allow one OPP connection (transfer session) at a time. We definitely will offer the ability to transfer multiple files at the same time, but I'm not sure if it would be done before v1.
Attachment #674654 - Attachment is obsolete: true
Attachment #675597 - Flags: review?(doug.turner)
* Fixed a problem of v4 (incorrect file body header)
Attachment #675597 - Attachment is obsolete: true
Attachment #675597 - Flags: review?(doug.turner)
Attachment #676081 - Flags: review?(doug.turner)
Comment on attachment 676081 [details] [diff] [review]
patch 1: v5: Save the file sent from remote device

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

There there tests for this?

::: dom/bluetooth/BluetoothOppManager.cpp
@@ +77,1 @@
>  static uint32_t sSentFileLength = 0;

Please file a v2 bug on how we can't do multiple ops at the same time.  Add a comment w/ the bug number here.

::: dom/bluetooth/BluetoothOppManager.h
@@ +110,4 @@
>  
>    nsCOMPtr<nsIDOMBlob> mBlob;
>    nsCOMPtr<nsIThread> mReadFileThread;
> +  nsCOMPtr<nsIFile> mFile;

I don't think you need this as a member var.  Just declare it where you use it.
* Removed mFile, declared a local variable.
* Created a follow-up bug (bug 806749) for having multiple socket connections in Bluetooth*Manager.
Attachment #676081 - Attachment is obsolete: true
Attachment #676081 - Flags: review?(doug.turner)
Attachment #676468 - Flags: review?(doug.turner)
Comment on attachment 676468 [details] [diff] [review]
patch 1: v6: Save the file sent from remote device

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

dom/bluetooth needs lots of work.  thanks for making it a bit better :)
Attachment #676468 - Flags: review?(doug.turner) → review+
(In reply to Doug Turner (:dougt) from comment #14)
> Comment on attachment 676468 [details] [diff] [review]
> patch 1: v6: Save the file sent from remote device
> 
> Review of attachment 676468 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> dom/bluetooth needs lots of work.  thanks for making it a bit better :)

Thanks for review, Doug. :D
try: 

https://tbpl.mozilla.org/?tree=Try&rev=833924f74b32
https://tbpl.mozilla.org/?tree=Try&rev=adfe1c7ab334

(The patch sent to try server is patch v5, and the final patch is v6. Since there was no big change from v5 to v6, I think it is fine to just use the result and save some resource.)
https://hg.mozilla.org/mozilla-central/rev/76ee46919011
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
https://hg.mozilla.org/releases/mozilla-aurora/rev/26ee9bdc2d5e

Note that this didn't apply completely cleanly due to some bitrot from bug 802288. Probably wouldn't hurt to look it over to make sure that I unbitrotted it correctly.
You need to log in before you can comment on or make changes to this bug.