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

RESOLVED FIXED

Status

()

Core
Audio/Video
RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: cajbir, Assigned: cajbir)

Tracking

Trunk
Points:
---
Bug Flags:
wanted1.9.2 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

9 years ago
When saving an HTML document which contains audio and video elements the audio and video objects are not saved.
(Assignee)

Comment 1

9 years ago
Created attachment 332289 [details] [diff] [review]
Save media objects when saving document
Assignee: nobody → chris.double
Status: NEW → ASSIGNED

Comment 2

9 years ago
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?

Updated

9 years ago
Flags: wanted1.9.2?
OS: Linux → All
Hardware: x86 → All
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.
Flags: wanted1.9.2? → wanted1.9.2+
(Assignee)

Comment 4

7 years ago
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?
(Assignee)

Comment 5

7 years ago
Created attachment 459286 [details] [diff] [review]
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).
Attachment #332289 - Attachment is obsolete: true
Attachment #459286 - Flags: review?(roc)
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!
Attachment #459286 - Flags: review?(roc) → review+
Do we have any tests for "Save As"?
(Assignee)

Comment 8

7 years ago
Created attachment 460432 [details] [diff] [review]
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.
Attachment #460432 - Flags: review?(roc)
(Assignee)

Comment 9

7 years ago
Created attachment 460435 [details] [diff] [review]
Persist media and source elements

Fixes review comment, carries review forward r+=roc
Attachment #459286 - Attachment is obsolete: true
Attachment #460435 - Flags: review+
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.
Attachment #460432 - Flags: review?(roc) → review+
(Assignee)

Comment 11

7 years ago
Created attachment 461107 [details] [diff] [review]
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.
Attachment #460432 - Attachment is obsolete: true
Attachment #461107 - Flags: review?(roc)
Attachment #461107 - Flags: review?(roc) → review+
(Assignee)

Comment 12

7 years ago
Created attachment 461964 [details] [diff] [review]
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.
Attachment #460435 - Attachment is obsolete: true
Attachment #461107 - Attachment is obsolete: true
Attachment #461964 - Flags: review+
Attachment #461964 - Flags: approval2.0?
Attachment #461964 - Flags: approval2.0? → approval2.0+
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/4e274fd92b25
Flags: in-testsuite+
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.