853351 - [Browser][User Story] Video save from webpage

Status: VERIFIED FIXED

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

Description

[Peter Dolanjski [:pdol]]

UCID: Browser-118

User Story:
As a user, I want the ability to save a video file from the webpage (currently supported MIME types) so that I can later access that video from my Video Player application.

Comment 1

[Ben Francis [:benfrancis]]

Renominating for leo- because this new leo+ feature request was filed today and feature freeze for v1.1.0 was nearly a week ago https://wiki.mozilla.org/Release_Management/B2G_Landing#Versions_and_Scheduling

Please schedule for a future iteration.

Comment 2

[Jason Smith [:jsmith]]

Naoki - Do we have moztrap coverage for this?

Comment 3

[Arthur Chen [:arthurcc][inactive after 6/30]]

Videos take longer time to load than images. Users need UI to check all downloading files and are able to stop or pause downloads. Detail UX specs are needed.

Comment 4

[Peter Dolanjski [:pdol]]

Re-blocking based on a new FC date as March 29th and based on partner negotiations for the HTTP download capability.

In Rob's spec, only the ability to save from the actual file link (not from the Video player) is a blocker for leo.

Comment 5

[Arthur Chen [:arthurcc][inactive after 6/30]]

Attachment: Link to https://github.com/mozilla-b2g/gaia/pull/8840

Ben, please help review the patch. Thanks!

Comment 7

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

odd. should have had this test case.  not sure what happened to it.

6890.  Need test cases around sd card.

Comment 8

[Stephany Wilkes]

Comment on attachment 730022 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/8840

Ben, is there anything standing in the way of this moving to approval?

We have passed our 03/29 date for being feature complete on the P1/leo+ user stories (excluding MMS) and this is one of those nine bugs that are still open.

Comment 9

[Joe Cheng [:jcheng] (please needinfo)]

anyone can assist in review?

Comment 10

[Chris Lee [:clee]]

Maybe Dale can help review?

Comment 11

[Dale Harvey (:daleharvey)]

I dont have time to get to this tonight, but I will steal the review in the morning if its still open

Comment 12

[Ben Francis [:benfrancis]]

Sorry I wasn't able to review this before the 29th, I was on PTO.

I've had a look over the code and it looks good and works well, but what specification was this written against exactly? It doesn't match either the UX spec at https://www.dropbox.com/s/aolxy3mzrdfvg17/browser-downloading.pdf or the description in comment #4 which says "only the ability to save from the actual file link (not from the Video player) is a blocker for leo."

Some questions:
* This patch allows the user to download HTML5 audio/video by long pressing on the video or built-in audio controls (assuming they're displayed on the screen). Long pressing on a hyperlink to an audio/video file will just show the "Open in new tab" option which if selected will load Gecko's HTML5 audio/video controls which the user can then long press to save. This is fine, but is this what the requirement was?
* The "Saving a video to Videos - from player" section on page 9 looks nothing like what Gecko currently does when you click on an audio/video file, nor what this patch does.
* Is the user expected to be able to see a download icon, view the download progress, cancel and download and open a downloaded file from the system utility tray as suggested in the UX spec? I would hope this is out of scope for this user story, but this isn't clear. I think that requirement would require a very different implementation and isn't covered by this patch.
* The spec says "Users can only download files that are compatible with the existing Gallery, Video and Music applications". As far as I can tell this patch makes no attempt to determine this, and it would be difficult to do so. We know that the binary blog reports to be an image/audio/video file, but not what format it's in.
* Video and audio tags in HTML5 can have multiple source files specified in different formats. It seems that gecko picks the one it prefers and that's the one that gets downloaded. Is that the expected behaviour?

In future I think it would be very valuable to have a UX spec per user story, so it's clearer what is and isn't in scope for each story.

Note that this patch also addresses bug 853352 (saving audio), though I noticed that when saving an ogg audio file it thought it was a video file.

Comment 13

[Joe Cheng [:jcheng] (please needinfo)]

comment 12 seems to be more for UX to answer
before we get clarification from UX, do we want this to land given that we need to complete the user story asap? so the feature works in a way but needs further modifications

Comment 14

[Ben Francis [:benfrancis]]

Comment on attachment 730022 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/8840

I'm going to r+ this on the basis that the code is fine and I don't want to hold up one of the last leo P1s from landing, but I don't think this should land unless UX approve it, because it doesn't match the UX spec.

Will any test cases need to be updated accordingly?

Comment 15

[Arthur Chen [:arthurcc][inactive after 6/30]]

Ben, thank you for reviewing. I'll add test cases for this patch.

Sorry I did not aware we have the UX spec posted in the meta bug. My concern is that we may not able to get the correct type of the file simply using the URL because it may not contain correct extension name or even has no extension name at all. For such cases, suggest we can simply display "download the file" when users long press on the link, we can then save the file to correct device storage once it is downloaded.

Comment 16

[Ben Francis [:benfrancis]]

(In reply to Arthur Chen [:arthurcc] from comment #15)
> Ben, thank you for reviewing. I'll add test cases for this patch.

I really meant the test cases in Moztrap that Naoki was talking about. New unit tests/integration tests for the browser are always welcome though.

> Sorry I did not aware we have the UX spec posted in the meta bug. My concern
> is that we may not able to get the correct type of the file simply using the
> URL because it may not contain correct extension name or even has no
> extension name at all. For such cases, suggest we can simply display
> "download the file" when users long press on the link, we can then save the
> file to correct device storage once it is downloaded.

This makes sense.

Comment 17

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

I was going to retest and rewrite the draft test case once this lands so I can play around with it and get a better feel.  (Trying to learn from the v1.0 experience and have cleaner test cases)

I do have some questions in regards to the type of videos allowed to download:
is it just webm?  or does it apply to flash, mpeg, avi, 3gp, etc?

When should Video app launch?

I should probably check to see if the common no-right click js works in regards to tapping.

Comment 18

[jachen]

Apologies on the delay. Rob is the owner of this UX spec, but he is unavailable at the moment. I've ask Casey to take a look since he has been peer reviewing the work with Rob.

In regards to Ben's comment to split the UX spec per user story, this is difficult for UX because we need to think across user stories since many of them naturally bucket together from an interaction standpoint. Also, splitting them makes it tougher to manage when we make a change to an interaction. We're happy to discuss this more when we meet up in London next week.

Comment 19

[Casey Yee]

The requirement is actually really quite vague and doesn't cover all the possible scenarios that are present when saving media from web pages:

1. Media files inserted inline onto a web page using <video>, <audio> and other media tags.    

2. Direct hyperlink to media files.   For example: 
<a href="http://fileshare.com/newsong.mp3">newsong</a>

3. Direct links to media files with a Content-disposition header:
Content-Disposition: attachment; filename="<file name.ext>"
 
On the desktop scenario 3 would trigger a Save-as (to local file system) dialogue.  


Robs proposal covers scenario 3, direct download links with Content-disposition: attachment header.  In the proposal, downloads are handled externally from the browser in the utility tray and persisted to the appropriate application based on MIME type.

The patch that has been posted seems to cover scenario 1, inline media tags and perhaps scenario 2, direct links to media files.     Ben, can you confirm?

From what I can tell, what we have is two solutions for different scenarios of the same requirement.   (Doh!)


I hate to answer a question with more questions, but Product will need to give us a little more clarity on how we proceed:
What scenarios do we need to cover as part of this requirement?   It seems that we may have a patch for the first two scenarios, do we need to also cover 3, Direct links to media files with Content-dosposition: attachment header?  

By the sounds of it, scenario 3 is going to be a pretty big job and still needs to be discussed wether this is even possible.   

In any case, UX will need to review the patch and work on covering any additional IxD in scenarios 1 (and 2?) that need to be considered.

Comment 20

[Peter Dolanjski [:pdol]]

Adding Sri to confirm whether the patch covering Casey's scenario 1 and 2 meets the partner requirement.

Comment 21

[Peter Dolanjski [:pdol]]

Scratch the needinfo on Sri.

Daniel, can you confirm whether or not your requirement is satisfied with the above scenarios (for the purpose of operator content portals)?

Comment 22

[Chris Lee [:clee]]

Can we mark as resolved once Daniel confirms he's good with the UX here?  

Sounds like the land is r+'ed and ready to land?

Comment 23

[Ben Francis [:benfrancis]]

We could land this as-is, but bear in mind that if we take the direction described in the meta bug (bug 848371) and this thread https://groups.google.com/d/msg/mozilla.dev.gaia/_gxHb_0RjTw/o_z_kEw4X-YJ we may have to re-factor it in future.

Comment 24

[Arthur Chen [:arthurcc][inactive after 6/30]]

master: https://github.com/mozilla-b2g/gaia/commit/65dd37c938b1cd38fce96fcfa7f4be09e259c255
Need to re-factor the code after the UX clarified in the future.

Comment 25

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted commit 65dd37c938b1cd38fce96fcfa7f4be09e259c255 as:
v1-train: c998c9ce045a8a858b7e155944b7b4f6573d2c22

Comment 28

[Tony Chung [:tchung]]

Verified fix on Buri 1.2 nightly: 20130912040201.   but do note that bug 915137 is 100% reproducible and should be considered a koi blocker.

Comment 29

[Natalya Kot [:nkot]]

Verified fix on Leo 1.1 as well - was able to save a video from the webpage (tried YouTube and Break.com) to a device and then play it with Video app.

Build ID: 20130912041201
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/091fcf40d427
Gaia: 5f7ea0c3f9290dae2db1274472af343cb61890fa
Platform Version: 18.1