Closed Bug 690701 Opened 13 years ago Closed 13 years ago

"Save Video As..." should respect the filename set in the Content-Disposition header.

Categories

(SeaMonkey :: UI Design, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: philip.chee, Assigned: philip.chee)

Details

Attachments

(1 file, 1 obsolete file)

Port the Following Firefox bugs:
Bug 564387 - Let "Save Video As..." respect the filename set in the Content-Disposition header.

Plus a related one line patch:
Bug 682478. Wait longer to get correct filename for "save link as".
Attached patch Patch v1.0 Proposed Fix. (obsolete) — Splinter Review
I found a Ogg video to test on:
http://commonspace.wordpress.com/2011/09/27/friends-and-mentors/

> -pref("browser.download.saveLinkAsFilenameTimeout", 1000);
> +pref("browser.download.saveLinkAsFilenameTimeout", 4000);
From Bug 682478. Wait longer to get correct filename for "save link as".

> +  // Save URL of clicked-on link.
> +  saveLink: function() {
> +    var doc = this.target.ownerDocument;
> +    urlSecurityCheck(this.linkURL, this.target.nodePrincipal);
Firefox uses |this.target.ownerDocument.nodePrincipal| here. Wonder what's the difference?

> +    this.saveHelper(this.linkURL, this.linkText(), null, true, doc);
Attachment #563701 - Flags: review?(neil)
Comment on attachment 563701 [details] [diff] [review]
Patch v1.0 Proposed Fix.

>Bug 690701 "Save Video As..." should respect the filename set in the Content-Disposition header.
Where is the actual change for saving videos?

>-  // Save URL of clicked-on link.
>-  saveLink: function() {
Nit: leave saveLink here and move saveHelper below it.

>-          saveURL(linkURL, linkText, null, true, true, doc.documentURIObject);
>+          saveURL(linkURL, linkText, dialogTitle, bypassCache, false, doc.documentURIObject);
The true/false change is incorrect. The aSkipPrompt name is misleading; false ignores the preference, while true respects the preference, which is what we want here.
Attachment #563701 - Flags: review?(neil) → review-
Comment on attachment 563701 [details] [diff] [review]
Patch v1.0 Proposed Fix.

>+      flags |= Ci.nsICachingChannel.LOAD_BYPASS_LOCAL_CACHE_IF_BUSY;
Also,             ^
Attached patch Patch v1.1Splinter Review
>>Bug 690701 "Save Video As..." should respect the filename set in the Content-Disposition header.
> Where is the actual change for saving videos?
Oops.

>>-  // Save URL of clicked-on link.
>>-  saveLink: function() {
> Nit: leave saveLink here and move saveHelper below it.
Fixed.

>>-          saveURL(linkURL, linkText, null, true, true, doc.documentURIObject);
>>+          saveURL(linkURL, linkText, dialogTitle, bypassCache, false, doc.documentURIObject);
> The true/false change is incorrect. The aSkipPrompt name is misleading;
> false ignores the preference, while true respects the preference, which is
> what we want here.
Fixed.

>>+      flags |= Ci.nsICachingChannel.LOAD_BYPASS_LOCAL_CACHE_IF_BUSY;
> Also,             ^
Double Oops.
Attachment #563701 - Attachment is obsolete: true
Attachment #565606 - Flags: review?(neil)
Comment on attachment 565606 [details] [diff] [review]
Patch v1.1

>+    var doc = this.target.ownerDocument;
>+    urlSecurityCheck(this.linkURL, this.target.nodePrincipal);
>+    this.saveHelper(this.linkURL, this.linkText(), null, true, doc);
[I guess inlining doc would break the 80-character limit...]
Attachment #565606 - Flags: review?(neil) → review+
Pushed to comm-central.
http://hg.mozilla.org/comm-central/rev/ef2d000c5580
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.