Cristian Nicolae (:xti) Reporter
	Description • 12 years ago

Firefox 18.0a1 (2012-09-17)
Device: Galaxy Note
OS: Android 4.0.4

Steps to reproduce:
1. Open Firefox for Android
2. Go to http://searchmp3.mobi/ and tap any Top Rated Tracks
3. Tap on Download button
4. Type the given captcha code and tap on Download button

Expected result:
After step 4, a File Picker is displayed with the following options:
* at least one 3rd party app, in this case, the default Media Player is listed
* Firefox for Android
If the first option is tapped, the selected app will start playing the song.
If the second option is selected, then Firefox will start downloading and saving the mp3 to sdcard/Downloads location.

Actual result:
Firefox will start playing the mp3 file and HTML 5 controls are displayed.

Note:
I assume that when a user wants to Download something, it means that he/she really wants to have that file on  the sdcard.

	Zazie Lavender
	Comment 1 • 12 years ago

This should be a no brainer, there should be a configurable option to treat different media types differently when clicked on, and offer the other option on the menu.

	Aaron Train [:aaronmt]
	Comment 2 • 12 years ago

Wont be 17 as that's out the door.

	Karen Rudnitski [:kar]
	Comment 3 • 12 years ago

I agree that by hitting the 'download' button, the file should download (and I would expect the corresponding Android 'download' notifications happening at the top of the screen). 

In parallel, if you have picked another media player to 'play' it and it plays as well, great. I would also expect Firefox for Android to do the same thing - be able to play it. 

It is unclear from this bug whether an installed third-party media player can be 'chosen' to play the downloaded song (in parallel to the download itself), at which point I would want Firefox to have similar behaviour.

However at the crux, if a user selects 'download', I would want the file downloaded (since that is the intent) with the option to play it within Fx with HTML5 controls.

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 4 • 12 years ago

In comment 0, Cristian click a "Download" button which is part of the webpage, not Firefox. I don't know why the download did not start, but that is completely different than adding a context menu for "Save Link As".

I can assume that the download did not happen as expected because Firefox has a default action to "open" files that are downloaded, if we find an application that can handle the file.

	Karen Rudnitski [:kar]
	Comment 5 • 12 years ago

I don't think we need a 'save link as' if the download actually downloaded (which is what I would expect to happen by clicking on that button).

	Aaron Train [:aaronmt]
	Comment 6 • 12 years ago

I would expect the ability to hard tap on a link and select 'save link as' or 'save target as' on an indexed file directory like a music FTP rather than tapping each link and having the browser playing the music

	Kevin Brosnan [Ex-Mozilla]
	Comment 7 • 12 years ago

Discussed this at length in the Mobile Triage meeting. The solution we are looking at is adding a context menu option to the html5 player to save the file.

	Henri Sivonen (:hsivonen)
	Comment 8 • 12 years ago

(In reply to Karen Rudnitski from comment #5)
> I don't think we need a 'save link as' if the download actually downloaded
> (which is what I would expect to happen by clicking on that button).

“Download buttons” on many sites are mere links without the download attribute and without the HTTP response having a Content-Disposition header. So from the point of view of Firefox, those are just links and don’t have any “download” semantics. Yet, as a user, I expect to be able to tap&hold for the context menu and do “Save As…” even when the site has done nothing to make the link have download semantics.

	Karen Rudnitski [:kar]
	Comment 9 • 12 years ago

(In reply to Henri Sivonen (:hsivonen) from comment #8)
> “Download buttons” on many sites are mere links without the download
> attribute and without the HTTP response having a Content-Disposition header.
> So from the point of view of Firefox, those are just links and don’t have
> any “download” semantics. Yet, as a user, I expect to be able to tap&hold
> for the context menu and do “Save As…” even when the site has done nothing
> to make the link have download semantics.

Thanks Henri - we did talk about this at length in Thursday's triage meeting and the above was explained. We are discussing how best to implement better functionality in this kind of scenario.

	(not currently active) Ted Mielczarek
	Comment 10 • 12 years ago

A compromise here would be to replace "Save as PDF" in the dropdown menu with just "Save" for media files, where "Save as PDF" is not useful.

	Wesley Johnston (:wesj)
	Comment 11 • 12 years ago

If anyone is interested in picking this up, wanted to write up some quick instructions on how you would go about fixing this. This bug is similar to bug 818987, so these are similar instructions.

First steps are to get a Fennec build up and running. There are pretty in depth instructions here: https://wiki.mozilla.org/Mobile/Fennec/Android but don't hesitate to ask if you run into problems.

Context menu options for Fennec are added using our context menu api:

https://developer.mozilla.org/en-US/docs/Extensions/Mobile/API/NativeWindow/contextmenus

This is very similar to the "Save Image" context menu item we already have here:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#457

In fact, I think we could likely hijack that context menu item for this (maybe we should do that for the shareImage element above this as well? That's a separate bug....).

Instead of a string, the contextmenu api also allows passing in a function for the title. The function is passed the element that we're looking at. So you could take that element, see if its a video/audio/image element (element instanceof HTMLVideoElement) and show an appropriate title "Save Image", "Save Audio", "Save Video".

We'll also have to modify the second argument passed here (imageSaveableContex) to also return true for video/audio elements. Its defined here:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#1435

It does some checks to make sure the image is not a 404 link. For now I'd be fine skipping that for video/audio (which we try to avoid loading unless the user has hit play to save mobile bandwidth).

The third argument is the function called when this context menu item is selected. Again, we can skip the attempt to pull the mimetype out of the cache for video/audio for now. We'll just have to get the src of the media and pass it straight to ContentAreaUtils.internalSave.

For reference, the desktop firefox code for doing this same stuff is at:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js#1089

but its significantly different than Fennec's. For now, I don't think we need all that complexity here. Someday it would be nice to move that desktop file to toolkit and try and share more of this code.

	Brad Lassey [:blassey] (use needinfo?)
	Comment 12 • 12 years ago

Xti, out of curiosity, is this site setting Content-Disposition correctly? If it is, then we're handling it incorrectly.

If it isn't, then I'd still like to ensure we're handling it correctly for sites that set it correctly. But that would be a new bug.

	Cristian Nicolae (:xti) Reporter
	Comment 13 • 12 years ago

I searched on the page source for this setting, but I didn't found anything.

I rechecked this issue on a couple of browsers and as you can see on this video http://youtu.be/LXoolhWLs9Y there is a different behaviour for each tested mobile browser (Nightly, Chrome, Stock Browser, Dolphin and Opera).

I opened http://searchmp3.mobi/download-music-i-dont-like-feat-kanye-west-chief-keef-pusha-t-jadakiss-big-sean-725741.xhtml?source=index (mobile version - it can be switched from the bottom of the page) on Firefox for Desktop and if I tap on the Download button, the download popup is triggered (which asks me what I want: to save the file and where, or open it with an app).

I inspected the Download button and this is how it looks:
<a href="http://searchmp3.mobi/download-music-i-dont-like-feat-kanye-west-chief-keef-pusha-t-jadakiss-big-sean-725741-mp3-free.xhtml?source=index"><img src="http://searchmp3.mobi/assets/mobile/images/download.png" alt="Download: Music - I Dont Like Feat. Kanye West, Chief Keef, Pusha T, Jadakiss &amp; Big Sean mp3 free" title="Download: Music - I Dont Like Feat. Kanye West, Chief Keef, Pusha T, Jadakiss &amp; Big Sean Mp3 Free"></a>

Also, I didn't see any meta name="content-disposition" in page's <head>. Let me know if I can help more regarding this issue.

	Dietrich Ayala (:dietrich)
	Comment 15 • 12 years ago

Report of bug 821860 said file-saving of audio previously worked -> Regression.

	heyjinsu Assignee
	Comment 16 • 11 years ago

Hi everyone,
Is this bug still good to work on?
I have been checking out this bug and I would like to work on it.

	Wesley Johnston (:wesj)
	Comment 17 • 11 years ago

Yep! I wrote pretty detailed instructions on this in comment #11, but feel free to ping me here or on irc in #mobile if you need help. Step #1 is build Fennec: https://wiki.mozilla.org/Mobile/Fennec/Android

	Kyle
	Comment 18 • 11 years ago

Hi,

I would like to solve this bug for a school project.  Is it available to be worked on?

	heyjinsu Assignee
	Comment 19 • 11 years ago

Wesley, so far, I have a "Save Video" show up in html5 player's context menu. For a proper html5 video tag in http://html5demos.com/video, the long tap on the video shows "Save Video." As for other video/audio elements in anchor tags such as the download buttons in searchmp3.mobi, comment #7 says that's not what we are after in this bug.

Now I have one question, how can I identify audio files in html5 player?
It seems that when mp3 files are opened in html5 player, it shows up as HTMLVideoElement. Is checking for videoHeight and videoWidth == 0 sufficient?

I've only tested this change with my phone. Please let me know how I can proceed on from this point on. Thank you!

	heyjinsu Assignee
	Comment 22 • 11 years ago

Let me know if there's anything I can improve.

	Aaron Train [:aaronmt]
	Comment 23 • 11 years ago

The attachments you have added need to be in the form of a patch on the modified files, and not whole attachments of your local files.

https://developer.mozilla.org/en/docs/Creating_a_patch

	heyjinsu Assignee
	Comment 24 • 11 years ago

I wasn't sure who to put in for review, so I left it blank.

	heyjinsu Assignee
	Comment 25 • 11 years ago

again..wasn't sure who to put for review.

	heyjinsu Assignee
	Comment 26 • 11 years ago

Messed up many times, but hopefully i can get it right this time. Thanks Wesley.

	Wesley Johnston (:wesj)
	Comment 27 • 11 years ago

Comment on attachment 705155 [details] [diff] [review]
mq patch file for bug 791654

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

This looks good. I need to test and talk to dolske about the desktop code, so just a feedback+ for now. If you want to address these nits and put it up again, I think it will be good.

::: mobile/android/chrome/content/browser.js
@@ +491,5 @@
>            }
>          });
>        });
> +
> +    //JINSU BUG 791654

This comment likely won't be helpful in the future. Might as well remove it.

@@ +498,5 @@
> +            //my test shows that HTML5 player sees mp3 as video.
> +            //Be smart and display audio context menu
> +            if (aTarget.videoWidth == 0 || aTarget.videoHeight == 0 ) {
> +                return Strings.browser.GetStringFromName("contextmenu.saveAudio");
> +            }

We shouldn't need this. I'm guessing that someone is pushing an audio file in a <video> tag. Do you have an example?

@@ +507,5 @@
> +    }, NativeWindow.contextmenus.mediaSaveableContext,
> +    function(aTarget) {
> +        let url = aTarget.currentSrc || aTarget.src;
> +        let filePickerTitleKey = (aTarget instanceof HTMLVideoElement &&
> +            (aTarget.videoWidth != 0 && aTarget.videoHeight != 0)) ? "SaveVideoTitle" : "SaveAudioTitle";

TBH, these titles aren't even shown, but they're nice to have.

@@ +509,5 @@
> +        let url = aTarget.currentSrc || aTarget.src;
> +        let filePickerTitleKey = (aTarget instanceof HTMLVideoElement &&
> +            (aTarget.videoWidth != 0 && aTarget.videoHeight != 0)) ? "SaveVideoTitle" : "SaveAudioTitle";
> +        //skipped trying to pull MIME type out of cache until further instructions say so
> +        ContentAreaUtils.internalSave(url, null, null, null, null, false, filePickerTitleKey, null, aTarget.ownerDocument.documentURIObject, aTarget.ownerDocument, true, null);

Desktop does something wildly more complex. I have no idea why. I'll ping some people to see if they know. If there is, we should try and share there code. i.e. this saveHelper code should be moved into ContentAreaUtils:

http://hg.mozilla.org/mozilla-central/annotate/2876e73c9b6f/browser/base/content/nsContextMenu.js#l943

@@ +1563,5 @@
> +    mediaSaveableContext: {
> +      matches: function mediaSaveableContextMatches(aElement) {
> +        //a simple check to see if the DOM element is of video/audio tag.
> +        //skipped on checking for 404 links for now because we don't load
> +        //video/audio data without the user's consent.

No need for this one either. The code is implied from the method name.

::: mobile/android/locales/en-US/chrome/browser.properties
@@ +234,5 @@
>  contextmenu.playMedia=Play
>  contextmenu.pauseMedia=Pause
>  contextmenu.shareMedia=Share Video
>  contextmenu.showControls2=Show Controls
> +#JINSU BUG 791654

No need for the comment

	Wesley Johnston (:wesj)
	Comment 28 • 11 years ago

In fact, might as well just ping dolske here. Any idea why the desktop code for saving a media is using the nsIExternalHelperAppService and not just a simple nsIWebBrowserPersist?

	Justin Dolske [:Dolske]
	Comment 29 • 11 years ago

I don't think we added anything special for saving media, it just happened to end up using the existing context menu code for saving items. I'm not really familiar with the merits of nsIExternalHelperAppService vs nsIWebBrowserPersist.

	heyjinsu Assignee
	Comment 30 • 11 years ago

@@ +498,5 @@
> +            //my test shows that HTML5 player sees mp3 as video.
> +            //Be smart and display audio context menu
> +            if (aTarget.videoWidth == 0 || aTarget.videoHeight == 0 ) {
> +                return Strings.browser.GetStringFromName("contextmenu.saveAudio");
> +            }

Added this check because I saw the desktop version doing the similar check.
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js#498

Also, when I was testing it with my Galaxy S3 device on http://searchmp3.mobi/, the context menu showed up with "Save Video" without this check.

	heyjinsu Assignee
	Comment 31 • 11 years ago

comments cleaned up

	Wesley Johnston (:wesj)
	Comment 32 • 11 years ago

Comment on attachment 705399 [details] [diff] [review]
mq patch file for bug 791654 clean

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

Looks great! Just some style nits and still a bunch of whitespace. Can you fix them and add a commit message and then I'll r+ this! i.e. make sure your Username is set in your .hgrc file and then run something like:

hg qref -U
hg qref -m "Bug 791654 - Add Save Video/Audio to context menus. r=wesj"

The top of the patch file should then have that info in it like this one here:

https://hg.mozilla.org/integration/mozilla-inbound/raw-rev/ed2ed42808ca

and upload the patch straight from your mozilla-central/.hg/patches/myPatch.

::: mobile/android/chrome/content/browser.js
@@ +492,5 @@
>          });
>        });
> +
> +    NativeWindow.contextmenus.add(function(aTarget) {
> +        if (aTarget instanceof HTMLVideoElement) {

I should have mentioned earlier. We use a 2 space indent in our JS files.

@@ +493,5 @@
>        });
> +
> +    NativeWindow.contextmenus.add(function(aTarget) {
> +        if (aTarget instanceof HTMLVideoElement) {
> +            //If the media is actually audio,

We don't enforce strict line lengths here. But do put a space after the //. i.e. I'd rather have:

// If a video element is zero width or height, its essentially an HTMLAudioElement.

@@ +495,5 @@
> +    NativeWindow.contextmenus.add(function(aTarget) {
> +        if (aTarget instanceof HTMLVideoElement) {
> +            //If the media is actually audio,
> +            //be smart and display audio context menu
> +            if (aTarget.videoWidth == 0 || aTarget.videoHeight == 0 ) {

And we don't put braces around single line ifs, so this can just be:

if (aTarget.videoWidth == 0 || aTarget.videoHeight == 0 )
  return Strings.browser.GetStringFromName("contextmenu.saveAudio");

@@ +1559,5 @@
>      },
>  
> +    mediaSaveableContext: {
> +      matches: function mediaSaveableContextMatches(aElement) {
> +       if (aElement instanceof HTMLVideoElement) {

Same. No need for these braces, you can even just move this to a single statement:

return (aElement instanceof HTMLVideoElement ||
        aElement instanceof HTMLAudioElement);

@@ +3566,5 @@
>        let restoring = aStateFlags & Ci.nsIWebProgressListener.STATE_RESTORING;
>        let showProgress = restoring ? false : this.showProgress;
>  
>        // true if the page loaded successfully (i.e., no 404s or other errors)
> +      let success = false;

Pull all these whitespace changes out. An easy way to do that is to just open the patch file and delete them physically. It won't work if there are changes further down the file, but these are all after your real changes. Yay!

	heyjinsu Assignee
	Comment 33 • 11 years ago

Thanks for the heads up. Im learning a lot about good coding styles.

	heyjinsu Assignee
	Comment 34 • 11 years ago

Sorry, Wes. There  was a snafu with some indentations.

	Wesley Johnston (:wesj)
	Comment 35 • 11 years ago

Comment on attachment 706214 [details] [diff] [review]
Added a line to default to "Save Video" when the element is neither HTMLVideo nor HTMLAudio.

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

Looks great! I'll get this landed today! Thanks

	Wesley Johnston (:wesj)
	Comment 36 • 11 years ago

Actually, I'll land right now. Should be in nightly tomorrow:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce79b39c6e0e

	heyjinsu Assignee
	Comment 37 • 11 years ago

Yay, thanks Wesley!

	Ryan VanderMeulen [:RyanVM]
	Comment 38 • 11 years ago

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

	Aaron Train [:aaronmt]
	Comment 39 • 11 years ago

I don't see this functionality at all in Nightly 01/27 in the context-menus of various HTML5 videos, how is this used?

	heyjinsu Assignee
	Comment 40 • 11 years ago

Hmm...I mainly tested on 'searchmp3.mobi' and 'html5demos.com/videos'. I'm migrating my hard disk at the moment...I will test it out hopefully by tomorrow.

	Aaron Train [:aaronmt]
	Comment 41 • 11 years ago

Ok, I'm not sure why, but this 01/28 has this change-set, perhaps there were a series of back-outs? I can confirm, I see this now in Nightly (01/28). Looks good!

	Ioana Chiorean
	Comment 42 • 11 years ago

TC added in the Context menu suite: https://moztrap.mozilla.org/manage/case/6198/

	Kevin Brosnan [Ex-Mozilla]
	Comment 43 • 11 years ago

This is a common user request in the support forums. Adding h264/mp3 playback 'broke' the downloading of these files.

	Justin Dolske [:Dolske]
	Comment 44 • 11 years ago

(In reply to Kevin Brosnan [:kbrosnan] from comment #43)
> This is a common user request in the support forums. Adding h264/mp3
> playback 'broke' the downloading of these files.

Is there a bug on file for that?

	Wesley Johnston (:wesj)
	Comment 45 • 11 years ago

(In reply to Justin Dolske [:Dolske] from comment #44)
> (In reply to Kevin Brosnan [:kbrosnan] from comment #43)
> > This is a common user request in the support forums. Adding h264/mp3
> > playback 'broke' the downloading of these files.
> 
> Is there a bug on file for that?

Yeah. Its this one. We fixed it.

	Justin Dolske [:Dolske]
	Comment 46 • 11 years ago

Ah! Ok. Sounded like this bug added a menu, but the H.264 support then broke it.