Last Comment Bug 791654 - Add save file as to the html5 player context menu
: Add save file as to the html5 player context menu
Status: VERIFIED FIXED
[mentor=wesj][lang=js] [html5]
: regression
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: Trunk
: ARM Android
: -- enhancement with 5 votes (vote)
: Firefox 21
Assigned To: heyjinsu
:
Mentors:
: 821860 (view as bug list)
Depends on:
Blocks: 867604 818987
  Show dependency treegraph
 
Reported: 2012-09-17 05:01 PDT by Cristian Nicolae (:xti)
Modified: 2013-10-06 05:40 PDT (History)
25 users (show)
ioana.chiorean: in‑moztrap+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified
21+
-


Attachments
patch to add "Save Video" or "Save Audio" context menu option in HTML5 player(for browser.js) (12.68 KB, patch)
2013-01-18 16:12 PST, heyjinsu
no flags Details | Diff | Review
"Save Video" and "Save Audio" string patch (for browser.properties) (498 bytes, patch)
2013-01-18 16:13 PST, heyjinsu
no flags Details | Diff | Review
adding "Save Video" and "Save Audio" to HTML5 player in browser.js (9.95 KB, patch)
2013-01-18 21:15 PST, heyjinsu
no flags Details | Diff | Review
add "Save Video" and "Save Audio" strings in browser.properties (1.54 KB, patch)
2013-01-18 21:17 PST, heyjinsu
no flags Details | Diff | Review
mq patch file for bug 791654 (16.93 KB, patch)
2013-01-22 16:17 PST, heyjinsu
wjohnston2000: feedback+
Details | Diff | Review
mq patch file for bug 791654 clean (16.62 KB, patch)
2013-01-23 09:31 PST, heyjinsu
wjohnston2000: feedback+
Details | Diff | Review
code style and white space corrections (3.71 KB, patch)
2013-01-24 18:04 PST, heyjinsu
no flags Details | Diff | Review
Added a line to default to "Save Video" when the element is neither HTMLVideo nor HTMLAudio. (3.88 KB, patch)
2013-01-24 18:42 PST, heyjinsu
wjohnston2000: review+
Details | Diff | Review

Description Cristian Nicolae (:xti) 2012-09-17 05:01:15 PDT
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.
Comment 1 Zazie Lavender 2012-11-19 09:38:51 PST
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.
Comment 2 Aaron Train [:aaronmt] 2012-11-19 12:29:47 PST
Wont be 17 as that's out the door.
Comment 3 Karen Rudnitski [:kar] 2012-12-05 14:22:08 PST
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.
Comment 4 Mark Finkle (:mfinkle) (use needinfo?) 2012-12-05 14:36:00 PST
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.
Comment 5 Karen Rudnitski [:kar] 2012-12-05 15:10:42 PST
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).
Comment 6 Aaron Train [:aaronmt] 2012-12-06 06:41:54 PST
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
Comment 7 Kevin Brosnan [:kbrosnan] 2012-12-06 10:37:14 PST
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.
Comment 8 Henri Sivonen (:hsivonen) 2012-12-07 04:42:27 PST
(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.
Comment 9 Karen Rudnitski [:kar] 2012-12-07 06:53:33 PST
(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.
Comment 10 Ted Mielczarek [:ted.mielczarek] 2012-12-07 14:09:27 PST
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.
Comment 11 Wesley Johnston (:wesj) 2012-12-09 22:48:32 PST
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.
Comment 12 Brad Lassey [:blassey] (use needinfo?) 2012-12-11 07:41:46 PST
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.
Comment 13 Cristian Nicolae (:xti) 2012-12-12 01:53:37 PST
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.
Comment 14 Aaron Train [:aaronmt] 2012-12-14 13:08:41 PST
*** Bug 821860 has been marked as a duplicate of this bug. ***
Comment 15 Dietrich Ayala (:dietrich) 2012-12-14 13:11:53 PST
Report of bug 821860 said file-saving of audio previously worked -> Regression.
Comment 16 heyjinsu 2013-01-14 15:38:08 PST
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.
Comment 17 Wesley Johnston (:wesj) 2013-01-14 16:10:18 PST
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
Comment 18 Kyle 2013-01-14 23:49:50 PST
Hi,

I would like to solve this bug for a school project.  Is it available to be worked on?
Comment 19 heyjinsu 2013-01-15 17:57:52 PST
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!
Comment 20 heyjinsu 2013-01-18 16:12:06 PST
Created attachment 704133 [details] [diff] [review]
patch to add "Save Video" or "Save Audio" context menu option in HTML5 player(for browser.js)
Comment 21 heyjinsu 2013-01-18 16:13:18 PST
Created attachment 704134 [details] [diff] [review]
"Save Video" and "Save Audio" string patch (for browser.properties)
Comment 22 heyjinsu 2013-01-18 16:14:18 PST
Let me know if there's anything I can improve.
Comment 23 Aaron Train [:aaronmt] 2013-01-18 20:48:50 PST
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
Comment 24 heyjinsu 2013-01-18 21:15:46 PST
Created attachment 704177 [details] [diff] [review]
adding "Save Video" and "Save Audio" to HTML5 player in browser.js

I wasn't sure who to put in for review, so I left it blank.
Comment 25 heyjinsu 2013-01-18 21:17:20 PST
Created attachment 704178 [details] [diff] [review]
add "Save Video" and "Save Audio" strings in browser.properties

again..wasn't sure who to put for review.
Comment 26 heyjinsu 2013-01-22 16:17:09 PST
Created attachment 705155 [details] [diff] [review]
mq patch file for bug 791654

Messed up many times, but hopefully i can get it right this time. Thanks Wesley.
Comment 27 Wesley Johnston (:wesj) 2013-01-22 17:44:18 PST
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
Comment 28 Wesley Johnston (:wesj) 2013-01-22 17:45:53 PST
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?
Comment 29 Justin Dolske [:Dolske] 2013-01-22 18:35:05 PST
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.
Comment 30 heyjinsu 2013-01-22 20:17:23 PST
@@ +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.
Comment 31 heyjinsu 2013-01-23 09:31:01 PST
Created attachment 705399 [details] [diff] [review]
mq patch file for bug 791654 clean

comments cleaned up
Comment 32 Wesley Johnston (:wesj) 2013-01-24 15:19:18 PST
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!
Comment 33 heyjinsu 2013-01-24 18:04:25 PST
Created attachment 706195 [details] [diff] [review]
code style and white space corrections

Thanks for the heads up. Im learning a lot about good coding styles.
Comment 34 heyjinsu 2013-01-24 18:42:26 PST
Created attachment 706214 [details] [diff] [review]
Added a line to default to "Save Video" when the element is neither HTMLVideo nor HTMLAudio.

Sorry, Wes. There  was a snafu with some indentations.
Comment 35 Wesley Johnston (:wesj) 2013-01-25 08:11:29 PST
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
Comment 36 Wesley Johnston (:wesj) 2013-01-25 08:13:34 PST
Actually, I'll land right now. Should be in nightly tomorrow:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce79b39c6e0e
Comment 37 heyjinsu 2013-01-25 18:32:19 PST
Yay, thanks Wesley!
Comment 38 Ryan VanderMeulen [:RyanVM] 2013-01-26 16:52:46 PST
https://hg.mozilla.org/mozilla-central/rev/ce79b39c6e0e
Comment 39 Aaron Train [:aaronmt] 2013-01-27 11:39:46 PST
I don't see this functionality at all in Nightly 01/27 in the context-menus of various HTML5 videos, how is this used?
Comment 40 heyjinsu 2013-01-28 17:37:22 PST
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.
Comment 41 Aaron Train [:aaronmt] 2013-01-28 18:02:00 PST
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!
Comment 42 Ioana Chiorean 2013-02-06 09:15:07 PST
TC added in the Context menu suite: https://moztrap.mozilla.org/manage/case/6198/
Comment 43 Kevin Brosnan [:kbrosnan] 2013-02-21 18:13:37 PST
This is a common user request in the support forums. Adding h264/mp3 playback 'broke' the downloading of these files.
Comment 44 Justin Dolske [:Dolske] 2013-02-22 17:14:56 PST
(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?
Comment 45 Wesley Johnston (:wesj) 2013-02-22 19:00:37 PST
(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.
Comment 46 Justin Dolske [:Dolske] 2013-02-22 21:30:51 PST
Ah! Ok. Sounded like this bug added a menu, but the H.264 support then broke it.

Note You need to log in before you can comment on or make changes to this bug.