814376 - [b2g-bluetooth] follow-up to bug 812412

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

Jonas provided some useful suggestion in bug 812412, which is about how to retrieve and compose file name for bluetooth file-sharing.

I think this bug is bb- because bluetooth file sharing should work well on the latest build.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Actually, won't it fail for anyone that uses a "share" WebActivity and provides a Blob (not a File)? Since that falls back to using the name "unknown" which appears to be rejected by many clients?

Comment 2

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

This is an interoperability issue with some devices. According to Bluetooth Object Push spec, senders don't need to send a file with the extension, and the remote Bluetooth device (called Push Server in Object Push Profile) has power to decide if they are going to accept the request or not.

I think we should try our best to make the transfer done, such as sending a file with the extension or with setting the blob's content type in the "Type" header, but we don't need to feel bad if we really can't get these information.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ok. Marking per that.

Comment 4

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

Attachment: patch 1: v1: get file extension

* Based on discussion of Bug 812412, try to get file extension by using nsIMimeService.getPrimaryExtension()

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 685030 [details] [diff] [review]
patch 1: v1: get file extension

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

This doesn't quite match what I had in mind. This won't use the mime service if we can't find a filename or if we have a filename that doesn't have an extension.

I think we should do something like:

name = ""
if (blob is File)
  name = ((file)blob).name; // This part is correct in the patch

if (name.IsEmpty())
  name = "unknown"

if (name.Contains("/"))
  name = getLeafName(name)

if (!name.Contains("."))
  name = name + "." + getExtension(blob.type)

Comment 6

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

Attachment: patch 1: v2: get file extension

* Revise the logic of previous patch.

Thanks for reminding, Jonas.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 685068 [details] [diff] [review]
patch 1: v2: get file extension

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

r=me with that fixed. The checkin comment needs some improvement too :)

::: dom/bluetooth/BluetoothOppManager.cpp
@@ +321,5 @@
> +                                     EmptyCString(),
> +                                     extension);
> +      if (NS_SUCCEEDED(rv)) {
> +        sFileName.AppendLiteral(".");
> +        sFileName.Append(NS_ConvertUTF8toUTF16(extension));

AppendUTF8toUTF16(extension, sFileName);

That will append and copy as a single step.

Comment 8

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

Attachment: patch 1: final: Improve the mechanism for setting file extension of a blob, r=sicking

* Nit picked and commit message modified.

Comment 9

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

try:

https://tbpl.mozilla.org/?tree=Try&rev=f818244324e9
https://tbpl.mozilla.org/?tree=Try&rev=0041b72d7c4f

Comment 10

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/0da6d7fb2dd3

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0da6d7fb2dd3

Comment 12

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

Comment on attachment 685080 [details] [diff] [review]
patch 1: final: Improve the mechanism for setting file extension of a blob, r=sicking

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Follow-up of bug 812412
User impact if declined: It would be more likely for remote devices to reject the file sharing request sent by b2g devices because of missing file extension.
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): Fairly low. It's only used on sharing files, and the logic just slightly changed. For platforms other than FOS, the risk should be very low as well since the Bluetooth module will not be built.
String or UUID changes made by this patch: No

Comment 13

[Alex Keybl [:akeybl]]

(In reply to Eric Chou [:ericchou] [:echou] from comment #12)
> User impact if declined: It would be more likely for remote devices to
> reject the file sharing request sent by b2g devices because of missing file
> extension.

Since this isn't b-b+, I need to ask - have we run through the bluetooth smoketests with this change? It would be a shame to regress the build for a bug that wasn't a strict requirement for v1.

Comment 14

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

> Since this isn't b-b+, I need to ask - have we run through the bluetooth
> smoketests with this change? It would be a shame to regress the build for a
> bug that wasn't a strict requirement for v1.

Hi Alex,

Speaking of Bluetooth smoke test, currently we rely on QA's smoke test report
to see if regression happened. As far as I can tell, I've tested this patch on 
my b2g device and it worked well. 

If you need any further information that I can provide to verify the correctness
of this patch, please let me know. Thank you.

Eric

Comment 15

[Alex Keybl [:akeybl]]

Comment on attachment 685080 [details] [diff] [review]
patch 1: final: Improve the mechanism for setting file extension of a blob, r=sicking

Engineering needs to test all patches for the possibility of regression at this point in the cycle - relying on QA is not enough to prevent new regressions. Sounds like Eric has done due diligence here.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5e1bebbff5fb
https://hg.mozilla.org/releases/mozilla-beta/rev/9ef7a0de9aba