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

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Philip Chee]

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".

Comment 1

[Philip Chee]

Attachment: 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);

Comment 2

[[e-mail]]

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.

Comment 3

[[e-mail]]

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

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

Comment 4

[Philip Chee]

Attachment: 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.

Comment 5

[[e-mail]]

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...]

Comment 6

[Philip Chee]

Pushed to comm-central.
http://hg.mozilla.org/comm-central/rev/ef2d000c5580