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

RESOLVED FIXED

Status

RESOLVED FIXED
7 years ago
7 years ago

People

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

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

7 years ago
Created attachment 563701 [details] [diff] [review]
Patch v1.0 Proposed Fix.

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 2

7 years ago
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 3

7 years ago
Comment on attachment 563701 [details] [diff] [review]
Patch v1.0 Proposed Fix.

>+      flags |= Ci.nsICachingChannel.LOAD_BYPASS_LOCAL_CACHE_IF_BUSY;
Also,             ^
(Assignee)

Comment 4

7 years ago
Created attachment 565606 [details] [diff] [review]
Patch v1.1

>>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 5

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

Comment 6

7 years ago
Pushed to comm-central.
http://hg.mozilla.org/comm-central/rev/ef2d000c5580
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.