1034828 - [Browser] Save image in browser would not put image to Gallery

Status: RESOLVED FIXED

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

Description

[Greg Weng [:snowmantw][:gweng][:λ]]

STR

1. Open browser -> Google Image search -> search anything, for example, 'cat'
2. Open the picture and long press to save the image

Expect

Image would save to Gallery

Actual

Nothing would be saved into Gallery. Although the notification shows that the 'image' has been downloaded.

----

Device Flame:

Gecko: 33.0a1 20140704040205
Gaia: b597b862

Comment 1

[Greg Weng [:snowmantw][:gweng][:λ]]

I've found if I open it with full size image link, it's OK to save the image However, to click on the result that Google searched out, or the preview image it gives, the "save image" menu would still open, but the image would not be saved.

Comment 2

[Jason Smith [:jsmith]]

QA Wanted for branch checks.

Comment 3

[Chris Kreinbring [:CKreinbring]]

The bug repros on Flame 2.1, Flame 2.0 and Buri 2.0

Flame 2.1
Build ID: 20140707040201
Gaia: 93daa354671a698634a3dc661c8c9dcb7d824c31
Gecko: 1dc6b294800d
Platform Version: 33.0a1
Firmware Version: v122
User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0

Flame 2.0
Build ID: 20140707000200
Gaia: ef67af27dff3130d41a9139d6ae7ed640c34d922
Gecko: f53099796238
Version: 32.0a2 (2.0)
Firmware Version: v122
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Buri 2.0
Build ID: 20140707064631
Gaia: 9b0f757efaf9fbd60dd932da0ff9b9182c8b8ed9
Gecko: c87c07dc23b3
Version: 32.0a2 (2.0)
Firmware Version: v1.2device.cfg
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Actual result: Attempting to save the image from selecting an image search result will return a download complete notification, but the image will not appear in the Gallery.

--------------------------------------------------------------------------------------------------------

The bug does not repro on Flame 1.4

Build ID: 20140707000200
Gaia: 5c9e1e4131d3ac8915ed88b72bb66dc7d97be6a0
Gecko: 2d0c15450488
Version: 30.0 (1.4)
Firmware Version: v122
User Agent: Mozilla/5.0 (Mobile; rv:30.0) Gecko/30.0 Firefox/30.0

Actual result: Attempting to save the image from selecting an image search result will return a download complete notification and the image will appear in the Gallery.

Comment 4

[Joshua Mitchell (Inactive)]

Nomming as a 2.0 blocker - saving images is a major function / feature.

Comment 5

[Gregor Wagner [:gwagner]]

Aus, should this work?

Comment 6

[Aus Lacroix [:aus]]

Hmmm, I would expect this to work. Not sure what's going on, will investigate.

Comment 7

[Aus Lacroix [:aus]]

Hmmm. This downloads the image as you would expect for me but it doesn't add an extension to it so the Gallery app doesn't pick it up. You can open it just fine from the Downloads in the Settings App (even opens in Gallery using a blob with content type information).

There are two options that I'm considering.

1 - Make the Gallery App more lenient when it's trying to detect new images to add. 
2 - Check in the underlying Browser API method if there is no extension and attempt (to the best of our knowledge) to add one later as best we can. This would only work for known Gecko content-types that map directly to extensions.

Comment 8

[Dale Harvey (:daleharvey)]

The manual code that used to do this did that, tried mapping an content type to an extension, there is code in shared to do that

Comment 9

[Aus Lacroix [:aus]]

Another possible solution would be to simply add the extension to the file when the download completes. This seems to make a lot of sense from a Gaia Download Management perspective as we're likely to run other post-completion steps anyway in the future. This will end up using the shared code as well that picks an extension based on content-type. Double win!

Comment 10

[Duane Dixon [:ddixon]]

B2G Inbound Regression Window

Last Working

Environmental Variables:
Device: Flame Master
Build ID: 20140609154428
Gaia: d64172b2df8b406183e4d6de0cab2494c6dcf211
Gecko: 7fb68b04c53c
Version: 33.0a1 (Master)
Firmware Version: v122
User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0

First Broken

Environmental Variables:
Device: Flame Master
Build ID: 20140609155654
Gaia: 901646a3279c66daa7621bebb62641779d8ae22c
Gecko: 58cf55af0ec1
Version: 33.0a1 (Master)
Firmware Version: v122
User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0

Last Working Gaia and First Broken Gecko
Issue DOES NOT occur here. 
Gaia: d64172b2df8b406183e4d6de0cab2494c6dcf211
Gecko: 58cf55af0ec1

Last Working Gecko and First Broken Gaia
Issue DOES occur here. 
Gaia: 901646a3279c66daa7621bebb62641779d8ae22c
Gecko: 7fb68b04c53c

Gaia Pushlog: https://github.com/mozilla-b2g/gaia/compare/d64172b2df8b406183e4d6de0cab2494c6dcf211...901646a3279c66daa7621bebb62641779d8ae22c

Possible Cause:

bug 982295 - Use new Browser API download method

Comment 11

[Joshua Mitchell (Inactive)]

possibly broken by bug 982295 - Aus, can you take a look?

Comment 12

[Jason Smith [:jsmith]]

(In reply to Joshua Mitchell (Joshua_M) from comment #11)
> possibly broken by bug 982295 - Aus, can you take a look?

Josh - Aus already assigned himself to the bug, so you don't need to needinfo him.

Comment 13

[Aus Lacroix [:aus]]

Yep, already on it. See comment #9 for more details about how this is going to get fixed.

Comment 14

[Aus Lacroix [:aus]]

Attachment: Patch - v1 - Browser API download method should add extension to file if not present.

Comment 15

[Aus Lacroix [:aus]]

It ends up being so much cleaner to do it in the Browser API download method that I went ahead and did that. It makes it a lot easier to use the method to boot so I think it's a win-win.

Comment 16

[Aus Lacroix [:aus]]

Attachment: Patch - v1 - Browser API download method should add extension to file if not present.

Comment 17

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8454733 [details] [diff] [review]
Patch - v1 - Browser API download method should add extension to file if not present.

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

Looks good. We need to test this.

::: dom/browser-element/BrowserElementParent.jsm
@@ +634,5 @@
> +
> +        // Check if we need to add an extension to the filename.
> +        if (ext && _options.filename.indexOf(ext) === -1) {
> +          _options.filename += ('.' + ext);
> +        }

What if the filename starts with a '.'? I think we should ignore the period at the beginning.

Comment 18

[Aus Lacroix [:aus]]

(In reply to Kan-Ru Chen [:kanru] from comment #17)
> Comment on attachment 8454733 [details] [diff] [review]
> Patch - v1 - Browser API download method should add extension to file if not
> present.
> 
> Review of attachment 8454733 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. We need to test this.

Yes, there's are tests for this already, unfortunately they are disabled because of issues with the test harness (specifically with mochitests and how we bootstrap the tests, see bug 1022281).
 
> ::: dom/browser-element/BrowserElementParent.jsm
> @@ +634,5 @@
> > +
> > +        // Check if we need to add an extension to the filename.
> > +        if (ext && _options.filename.indexOf(ext) === -1) {
> > +          _options.filename += ('.' + ext);
> > +        }
> 
> What if the filename starts with a '.'? I think we should ignore the period
> at the beginning.

The underlying call to doContent actually takes care of all of this (in nsExternalHelperAppService.cpp, see http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#614 and http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#1154).

Comment 19

[Aus Lacroix [:aus]]

Comment on attachment 8454733 [details] [diff] [review]
Patch - v1 - Browser API download method should add extension to file if not present.

Based on the comments I left, I would like to check this in as is.

Comment 20

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Ghislain Aus Lacroix [:aus] from comment #18)
> (In reply to Kan-Ru Chen [:kanru] from comment #17)
> > Comment on attachment 8454733 [details] [diff] [review]
> > Patch - v1 - Browser API download method should add extension to file if not
> > present.
> > 
> > Review of attachment 8454733 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Looks good. We need to test this.
> 
> Yes, there's are tests for this already, unfortunately they are disabled
> because of issues with the test harness (specifically with mochitests and
> how we bootstrap the tests, see bug 1022281).

I mean to test that a known file type should always has a extension.
  
> > ::: dom/browser-element/BrowserElementParent.jsm
> > @@ +634,5 @@
> > > +
> > > +        // Check if we need to add an extension to the filename.
> > > +        if (ext && _options.filename.indexOf(ext) === -1) {
> > > +          _options.filename += ('.' + ext);
> > > +        }
> > 
> > What if the filename starts with a '.'? I think we should ignore the period
> > at the beginning.
> 
> The underlying call to doContent actually takes care of all of this (in
> nsExternalHelperAppService.cpp, see
> http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/
> nsExternalHelperAppService.cpp#614 and
> http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/
> nsExternalHelperAppService.cpp#1154).

For a file named ".actually_is_a_jpeg", with this patch we will save it as is without append a .jpg extension to it, right? I think nsExternalHelperAppService only handles the part that creates temp file and find appropriate mime handler.

Comment 21

[Aus Lacroix [:aus]]

Attachment: Patch - v2 - Browser API download method should add extension to file if not present.

Updated patch to address potential issue with 'this_is_a_jpeg' style filenames.

Comment 22

[Aus Lacroix [:aus]]

Attachment: Patch - v3 - Browser API download method should add extension to file if not present.

Hmmm, how about this? :)

Comment 23

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8456025 [details] [diff] [review]
Patch - v3 - Browser API download method should add extension to file if not present.

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

Looks good!

::: dom/browser-element/BrowserElementParent.jsm
@@ +631,5 @@
> +        // that's only an extension with no extension (e.g. Sending in
> +        // '.jpeg' without stripping the '.' would result in a filename of
> +        // 'jpeg' where we want 'jpeg.jpeg'.
> +        _options.filename = _options.filename.replace(/^\.+/, "");
> +         

nit: trailing whitespaces

Comment 24

[Aus Lacroix [:aus]]

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

Leaving open for sheriffs to take care of the rest. :)

Comment 25

[Candice Serran (:cserran)]

did not get merged by sheriff, updating ETA

Comment 26

[Carsten Book [:Tomcat]]

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

Comment 27

[Carsten Book [:Tomcat]]

(In reply to Candice Serran (:cserran) from comment #25)
> did not get merged by sheriff, updating ETA

i got merges, just was not marked as such, sorry for that

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8502e15d3a77

Comment 29

[SandKing]

This issue has been verified successfully on Flame2.0&2.1
Verify video:"verify_1034828.mp4".

Flame2.0 build
Gaia-Rev        824a61cccec4c69be9a86ad5cb629a1f61fa142f
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/acde07cb4e4d
Build-ID        20141125040209
Version         36.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141125.113029
FW-Date         Tue Nov 25 11:30:43 EST 2014
Bootloader      L1TC00011880

Flame2.1 build:
Gaia-Rev        db2e84860f5a7cc334464618c6ea9e92ff82e9dd
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/211eae88f119
Build-ID        20141126001202
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141126.033519
FW-Date         Wed Nov 26 03:35:30 EST 2014
Bootloader      L1TC00011880

Comment 30

[SandKing]

Attachment: Verified video: verify_1034828.mp4