799131 - Update calls to saveURL() etc. due to function signature change in Bug 795065 (Add privacy status to nsDownload).

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Philip Chee]

Bug 795065 added privacy status to nsDownload and changed the signature of saveURL(), internalSave(), etc. in contentAreaUtils.js. We need to update all the callers in Suite code.

Comment 1

[Philip Chee]

Attachment: Patch v1.0 Proposed fix.

Neil is swamped at the moment so setting review flags to IanN.
You may have to apply the SeaMonkey bustage patch in Bug 795065 Coment 32 if it hasn't made it's way to mozilla-central yet.

-    if (url)
-      saveURL(url, null, "SaveImageTitle", false, true, makeURI(item.baseURI));
+    if (url) {
+      let titleKey = "SaveImageTitle";
+
+      if (item instanceof HTMLVideoElement)
+        titleKey = "SaveVideoTitle";
+      else if (item instanceof HTMLAudioElement)
+        titleKey = "SaveAudioTitle";
+
+      saveURL(url, null, titleKey, false, true, makeURI(item.baseURI),
+              gDocument);
+    }

This is from Bug 733297 - When saving a video from Page Info, the file picker title is "Save Image".

-function openUILink(url, e, ignoreButton, ignoreSave, allowKeywordFixup, postData, referrerUrl)
+function openUILink(url, aEvent, aIgnoreButton, aIgnoreSave, allowKeywordFixup, aPostData, aReferrerURI)
 {
-  var where = whereToOpenLink(e, ignoreButton, ignoreSave);
-  return openUILinkIn(url, where, allowKeywordFixup, postData, referrerUrl);
+  var params;
+  if (aIgnoreButton && typeof aIgnoreButton == "object") {
+    params = aIgnoreButton;

Some changes from Firefox. I didn't bother to track down the changeset but this looks useful.

I also deleted some trailing whitespace in and around the code I touched.

Comment 2

[Ian Neal]

(In reply to Philip Chee from comment #1)
> Created attachment 669172 [details] [diff] [review]
> Patch v1.0 Proposed fix.
> 
> -function openUILink(url, e, ignoreButton, ignoreSave, allowKeywordFixup,
> postData, referrerUrl)
> +function openUILink(url, aEvent, aIgnoreButton, aIgnoreSave,
> allowKeywordFixup, aPostData, aReferrerURI)
>  {
> -  var where = whereToOpenLink(e, ignoreButton, ignoreSave);
> -  return openUILinkIn(url, where, allowKeywordFixup, postData, referrerUrl);
> +  var params;
> +  if (aIgnoreButton && typeof aIgnoreButton == "object") {
> +    params = aIgnoreButton;
> 
> Some changes from Firefox. I didn't bother to track down the changeset but
> this looks useful.
> 
This came in Bug 631500 and had a follow up of Bug 753308.

Comment 3

[Ian Neal]

Comment on attachment 669172 [details] [diff] [review]
Patch v1.0 Proposed fix.

>+++ b/suite/common/utilityOverlay.js
>-// openUILink handles clicks on UI elements that cause URLs to load.
>-function openUILink(url, e, ignoreButton, ignoreSave, allowKeywordFixup, postData, referrerUrl)
>+/* openUILink handles clicks on UI elements that cause URLs to load.
>+ *
>+ * As the third argument, you may pass an object with the same properties as
>+ * accepted by openUILinkIn, plus "ignoreButton" and "ignoreAlt".
>+ */
>+function openUILink(url, aEvent, aIgnoreButton, aIgnoreSave, allowKeywordFixup, aPostData, aReferrerURI)
> {
You've changed some of the argument names but not ignoreSave -> aIgnoreAlt or allowKeywordFixup -> aAllowThirdPartyFixup
Any reason why?

>+  var params;
>+  if (aIgnoreButton && typeof aIgnoreButton == "object") {
>+    params = aIgnoreButton;
>+
>+    // don't forward "ignoreButton" and "ignoreSave" to openUILinkIn.
This comment does not match what is done below, it should say "ignoreAlt" rather than "ignoreSave".

>+    aIgnoreButton = params.ignoreButton;
>+    aIgnoreSave = params.ignoreAlt;
>+    delete params.ignoreButton;
>+    delete params.ignoreAlt;
>+  }
>+  else {
>+    params = {allowThirdPartyFixup: allowKeywordFixup,
>+              postData: aPostData,
>+              referrerURI: aReferrerURI,
>+              initiatingDoc: aEvent ? aEvent.target.ownerDocument : document}
>+  }
>+
>+  var where = whereToOpenLink(aEvent, aIgnoreButton, aIgnoreSave);
>+  return openUILinkIn(url, where, params);

r=me with arguments changed/explained and comment fixed.

Comment 4

[Ian Neal]

(In reply to Ian Neal from comment #2)
> (In reply to Philip Chee from comment #1)
> > Created attachment 669172 [details] [diff] [review]
> > Patch v1.0 Proposed fix.
> > 
> > -function openUILink(url, e, ignoreButton, ignoreSave, allowKeywordFixup,
> > postData, referrerUrl)
> > +function openUILink(url, aEvent, aIgnoreButton, aIgnoreSave,
> > allowKeywordFixup, aPostData, aReferrerURI)
> >  {
> > -  var where = whereToOpenLink(e, ignoreButton, ignoreSave);
> > -  return openUILinkIn(url, where, allowKeywordFixup, postData, referrerUrl);
> > +  var params;
> > +  if (aIgnoreButton && typeof aIgnoreButton == "object") {
> > +    params = aIgnoreButton;
> > 
> > Some changes from Firefox. I didn't bother to track down the changeset but
> > this looks useful.
> > 
> This came in Bug 631500 and had a follow up of Bug 753308.
Not sure if you want to add a follow up to deal with anything similar to what was done in the original follow up.

Comment 5

[Philip Chee]

> You've changed some of the argument names but not ignoreSave -> aIgnoreAlt or
> allowKeywordFixup -> aAllowThirdPartyFixup
> Any reason why?
I know it's confusing but I've tried to keep API compatibility with Firefox in case an extension uses the { object } syntax.

Comment 6

[Philip Chee]

Attachment: Patch v1.1 Arguments changed/explained and comment fixed. [check-in comment 7]

> You've changed some of the argument names but not ignoreSave -> aIgnoreAlt or allowKeywordFixup -> aAllowThirdPartyFixup
> Any reason why?

I've changed "allowKeywordFixup" to "aAllowThirdPartyFixup"

Firefox uses aIgnoreAlt while SeaMonkey uses aIgnoreSave because in SeaMonkey, Save can be Alt or Shift depending on ui.key.saveLink.shift. I've put a note in the comments to this effect.

>>+  var params;
>>+  if (aIgnoreButton && typeof aIgnoreButton == "object") {
>>+    params = aIgnoreButton;
>>+
>>+    // don't forward "ignoreButton" and "ignoreSave" to openUILinkIn.
> This comment does not match what is done below, it should say "ignoreAlt" rather than "ignoreSave".

I've added a comment note to explain the discrepancy.

Comment 7

[Philip Chee]

Comment on attachment 670404 [details] [diff] [review]
Patch v1.1 Arguments changed/explained and comment fixed. [check-in comment 7]

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

Base m-c bug landed in Aurora/Firefox 18
[Approval Request Comment]
Regression caused by (bug #): Bug 795065
User impact if declined: Save Link and Save Inline Image right-click actions don't work.
Testing completed (on m-c, etc.): m-c, c-c
Risk to taking this patch (and alternatives if risky): Low.
String changes made by this patch: None.

Comment 8

[Philip Chee]

Attachment: Patch t1.0 Fix tests. [check-in comment 11]

Fixes for tests that call .addDownload()

> +++ b/suite/modules/test/browser_sanitizer.js
This test has other problems with nsICacheSession::openCacheEntry (to be fixed in Bug 792735) but this patch fixes only the download test part.

Comment 9

[Ian Neal]

Comment on attachment 673274 [details] [diff] [review]
Patch t1.0 Fix tests. [check-in comment 11]

Looks good r=me

Comment 10

[Ian Neal]

Comment on attachment 673274 [details] [diff] [review]
Patch t1.0 Fix tests. [check-in comment 11]

[Triage Comment]

Comment 11

[Philip Chee]

Comment on attachment 673274 [details] [diff] [review]
Patch t1.0 Fix tests. [check-in comment 11]

Pushed to comm-central

Comment 12

[Philip Chee]

Pushed to comm-aurora.
http://hg.mozilla.org/releases/comm-aurora/rev/bbcd326bc0b0
http://hg.mozilla.org/releases/comm-aurora/rev/640b3961018d