970275 - [Download Manager] A strange white header is shown when you open an image file

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[rafael.marquez]

Attachment: cabecera.png

*Procedure
1. Download a .jpg file
2. Open download list
3. Tap on the .jpg file downloaded

*Expected Result
The image file is opened susccessfully

*Actual Result
A strange white header is shown when you open an image file. See the attached file

Comment 1

[Gregor Wagner [:gwagner]]

This seems like a gallery problem. Hema can you take a look?

Comment 2

[Punam Dahiya [:pdahiya]]

(In reply to rafael.marquez from comment #0)
> Created attachment 8373300 [details]
> cabecera.png
> 
> *Procedure
> 1. Download a .jpg file
Please provide details steps, was it a download from bluetooth, email, sms

> 2. Open download list
Is it downloaded files in notifications, please elaborate with steps on how to see download list?

> 3. Tap on the .jpg file downloaded

> *Expected Result
> The image file is opened susccessfully
> 
> *Actual Result
> A strange white header is shown when you open an image file. See the
> attached file

Also, if possible, please attach the .jpg file used while testing this issue. Thanks

Comment 3

[Punam Dahiya [:pdahiya]]

Attachment: Attaching image opened from SMS app

Attaching screen shot of an image downloaded and viewed via SMS app. Is this expected behavior? It will help if you can provide requested info in comment #2, that will help to replicate issue. Thanks

Comment 4

[rafael.marquez]

Attachment: Crazy_Horse.jpg

Comment 5

[rafael.marquez]

I have attached the file used(Crazy_Horse.jpg), but the bug is reproduced with any image

Comment 6

[rafael.marquez]

This test is a download manager failure. To reproduce: 
1. Open browser app
2. Open the web pag https://owd.tid.es/dm/
3. Tap on the file Crazy_Horse.jpg to download it.
4. Open settings app
5. Open download list
6. Tap on the downloaded file
7. Tap on Open

The file is opened with the white header.

Comment 7

[David Flanagan [:djf]]

The gallery app wants to display the filename of the image in the white header bar, and it expects that name to be sent in the activity with the name 'filename'.  The download manager is sending a url instead, which I don't believe has ever been part of the open activity protocol before.  It is easy enough to change Gallery to accept both filename or url as the image title, though.

I'm pretty sure that bluetooth, sms, and email all send a filename. Punam is checking this.

Rafael: you might want to check what the music and video apps do when asked to open a downloaded movie or video. They might also want a filename property instead of a url property.  If we can standardize on just one, that would be good.  Or consider sending both in your activity requests to be maximally robust.

Comment 8

[David Flanagan [:djf]]

Punam's investigation shows that download manager is actually sending a filename in the url property of the activity request.  Since you're not sending the actual URL that was downloaded, you shouldn't call the property 'url'.

I'd say that the bug is in the download manager, and you should just fix it there.

Rafael: is there any reason that you can't just change 'url' to 'filename' in download manager?

Changing the component back to Gaia:System. I think the burden is on the download manager to be compatible with the existing activities that the media apps support.  We shouldn't be modifying the media apps to add support for non-standard properties in the activity request.

Comment 9

[Punam Dahiya [:pdahiya]]

Attachment: Patch with fix for Bug970275

Hi Rafael

I have attached a proposed fix to bug 970275. Please review. Thanks!

Comment 10

[Punam Dahiya [:pdahiya]]

(In reply to David Flanagan [:djf] from comment #7)
> The gallery app wants to display the filename of the image in the white
> header bar, and it expects that name to be sent in the activity with the
> name 'filename'.  The download manager is sending a url instead, which I
> don't believe has ever been part of the open activity protocol before.  It
> is easy enough to change Gallery to accept both filename or url as the image
> title, though.
> 
> I'm pretty sure that bluetooth, sms, and email all send a filename. Punam is
> checking this.

I checked the above three flows, here are my findings:

SMS:
https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/attachment.js#L236
We are setting the filename property in SMS

Bluetooth:
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/bluetooth_transfer.js#L538

We are setting the filename property in Bluetooth

Email:
https://github.com/mozilla-b2g/gaia/blob/master/apps/email/js/cards/message_reader.js#L612

We are not setting the filename in Email. In Email on opening the attached image, similar blank white header is observerd. James, can you pl. confirm if blank header is desired when opening an image attachment in email or should this be fixed by passing filename property to open activity. Please confirm. Thanks

> 
> Rafael: you might want to check what the music and video apps do when asked
> to open a downloaded movie or video. They might also want a filename
> property instead of a url property.  If we can standardize on just one, that
> would be good.  Or consider sending both in your activity requests to be
> maximally robust.

Comment 11

[Jason Smith [:jsmith]]

Comment on attachment 8382445 [details] [review]
Patch with fix for Bug970275

Rafael is a QA, but not a code reviewer. Cristian is the person who could probably help review this.

Comment 12

[rafael.marquez]

Thanks Jason. I work in QA and can not review the code. I have reviewed the performance for video and music applications. If you open an audio or video downloaded, the name is displayed in the header.

Comment 13

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8382445 [details] [review]
Patch with fix for Bug970275

Hi, thanks a lot for the patch, I've just added some comments on github. Please answer/address them and ask me for a new review again, thx

Comment 14

[Punam Dahiya [:pdahiya]]

Comment on attachment 8382445 [details] [review]
Patch with fix for Bug970275

Updated the PR with review feedback. Please review. Thanks

Comment 15

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8382445 [details] [review]
Patch with fix for Bug970275

Good job! Thx

Comment 16

[Punam Dahiya [:pdahiya]]

Thanks Cristian for the review. Fix landed on master

https://github.com/mozilla-b2g/gaia/commit/9422aca1931ba6c68784f9e80bb1b6a7fcfd92e3

Comment 17

[James Burke [:jrburke]]

(In reply to Punam Dahiya from comment #10)
> Email:
> https://github.com/mozilla-b2g/gaia/blob/master/apps/email/js/cards/
> message_reader.js#L612
> 
> We are not setting the filename in Email. In Email on opening the attached
> image, similar blank white header is observerd. James, can you pl. confirm
> if blank header is desired when opening an image attachment in email or
> should this be fixed by passing filename property to open activity. Please
> confirm. Thanks

Looks like email should send the filename in the activity call, feel free to open an email-specific bug about that.

Comment 18

[Punam Dahiya [:pdahiya]]

Created bug 978329 for fixing white header inside email

Comment 19

[rafael.marquez]

Verified in master branch(1.4)