449141 - Saving an HTML page does not save the audio and video objects

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[cajbir (:cajbir)]

When saving an HTML document which contains audio and video elements the audio and video objects are not saved.

Comment 1

[cajbir (:cajbir)]

Attachment: Save media objects when saving document

Comment 2

[maikmerten]

I'm unfamiliar with the inner workings of the downloads system, so I wonder what happens with your patch if someone tries to save media "files" which actually happen to be "infinite" streams?

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The same thing that happens if you try to save any other infinite stream, I guess. We should probably just set a maximum size for all saved objects (if we don't already).

Chris, this seems like low-hanging fruit. Probably should use currentSrc though.

Comment 4

[cajbir (:cajbir)]

Should saving the page save the media resources specified in all the 'source' elements? For example, would the following save 3 video files:

<video>
  <source src='one.ogg'>
  <source src='two.mp4'>
  <source src='three.webm'>
</video>

or should it save only one file, that of the 'currentSrc' attribute on the video, and remove the 'source' elements when saving? 

I'm thinking the first option is probably the one that is required?

Comment 5

[cajbir (:cajbir)]

Attachment: Persist media and source elements

This patch takes the first approach mentioned in comment 4 to save all source elements. This matches the intent to save the contents of the entire page rather than just the 'currentSrc' of the playing video element. I'm not sure which is actually the better approach and am open to suggestions if the preference is 'currentSrc' only (and remove source elements when persisting).

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 459286 [details] [diff] [review]
Persist media and source elements

-                nsCAutoString url;
+                nsAutoString url;
                 uriSource->GetSpec(url);

Don't change this.

Otherwise, lovely!

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Do we have any tests for "Save As"?

Comment 8

[cajbir (:cajbir)]

Attachment: Test for media persisting

This contains a test for the persisting of media files. It persists a page containing a video to the tmp directory, checks if the video exists after persisting, then removes the files.

Comment 9

[cajbir (:cajbir)]

Attachment: Persist media and source elements

Fixes review comment, carries review forward r+=roc

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 460432 [details] [diff] [review]
Test for media persisting

Nice. Just one suggestion: add some randomness to the file name/directory name, so that we don't ever accidentally see old files and think the test passed.

Comment 11

[cajbir (:cajbir)]

Attachment: Test for media persisting

I had to change the test a fair bit since it turns out it was intermittent as to whether it passed or failed. saveDocument saves the video file asychronously. Changes are:

* add randomness to filename
* add a listener to track the download progress and perform tests when completed.

Comment 12

[cajbir (:cajbir)]

Attachment: Fix + test

Rolled up patch with fix and test. Requesting approval2.0. The is fairly minor, includes a test, and allows people to "Save As" page's containing videos and have the video saved with the page.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/4e274fd92b25