949945 - [B2G][Helix][Browser][Gallery] use new File([blob], filename) to return a blob with filename when picking

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::Gallery, defect, P2)

Description

[John Hu [:johnhu][:johu][:醬糊小弟]]

+++ This bug was initially created as a clone of Bug #920905 +++

When handling pick activity, we need to use new File([blob], filename) to return a blob with readable file name. Although we won't have image editor in pick activity from browser, it still has other cases that we should return a blob with its original filename.

Comment 1

[John Hu [:johnhu][:johu][:醬糊小弟]]

nominate it as 1.3? since its original bug is 1.3?.

Comment 2

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: use new file to create blob with correct file name

The patch is finished but there may be a bug in FilePicker.js. So, this patch is not tested correctly.

Comment 3

[Marcia Knous [:marcia]]

Lets make sure we fix all the apps affected and get this in the next train (1.4). Not blocking it for 1.3 (if folks think otherwise, we can revisit 1.3 blocker status)

blocking-b2g: 1.3? → 1.4?

Comment 4

[John Hu [:johnhu][:johu][:醬糊小弟]]

The gecko patch is in bug 949944.

Comment 5

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 8347934 [details] [review]
use new file to create blob with correct file name

Use new file to return a named blob and add the name field to the activity result.

Comment 6

[David Flanagan [:djf]]

Comment on attachment 8347934 [details] [review]
use new file to create blob with correct file name

r- because I think most of the time the call to new File() will not be necessary. I'd put it in getCroppedRegionBlob() instead.

Comment 7

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 8347934 [details] [review]
use new file to create blob with correct file name

Thanks for your review. I had update the patch to address your concerns. I prefer put it in the cropAndEndPick which is in gallery.js because we don't need to guarantee that getCroppedRegionBlob always returns a file.

Comment 8

[David Flanagan [:djf]]

Comment on attachment 8347934 [details] [review]
use new file to create blob with correct file name

r+, assuming you've tested this.  But see my query on github about whether we should change the filename if we change the type of the file when cropping it.

Comment 9

[Hema Koka [:hema]]

Johu, 

If needed Punam can take this, test and land it. Please let us know.

Thanks
Hema

Comment 10

[John Hu [:johnhu][:johu][:醬糊小弟]]

Thanks Hema,

That will be great if Punam can take this one. I am currently busy on TV things.

I feel David's suggestion in Github is correct. Although David gives me r+, I think my patch is not good enough to land. The insufficient part is that my patch returns wrong mime type. When user wants pick a "png" image, we only can provide jpg format while cropping. I know there is no cropping feature in file picker case. But the patch should take care all of the cases.

Comment 11

[Punam Dahiya [:pdahiya]]

Attachment: PR with the fix for Bug 949945

Hi David

I have updated John's patch with your suggestion in Comment #8 to change the filename if we change the type of the file when cropping it.

I have also added my findings in the discussion https://github.com/mozilla-b2g/gaia/pull/14696#discussion-diff-9659497R863

Please review the attached patch. Thanks

Comment 12

[Jason Smith [:jsmith]]

This is a good idea to fix, but I don't think we would hold a release on this. We've shipped with this bug in multiple releases historically.

Comment 13

[Hema Koka [:hema]]

This is not a blocker for 1.4

Punam worked on it and a patch is in review (but lowest in the review priority stack compared to all the other 1.4 work)

thanks
hema

Comment 14

[David Flanagan [:djf]]

We may not want to land this patch at all given this: https://bugzilla.mozilla.org/show_bug.cgi?id=985167#c12

It looks like using the File() constructor around memory-backed blobs turns out to be a really bad idea....

Comment 15

[Punam Dahiya [:pdahiya]]

Comment on attachment 8383805 [details] [review]
PR with the fix for Bug 949945

As per comment#14, removing the review request on the patch

Comment 16

[BMO Automation]

Firefox OS is not being worked on