Closed Bug 799131 Opened 7 years ago Closed 7 years ago

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

Categories

(SeaMonkey :: General, defect)

defect
Not set

Tracking

(seamonkey2.15 fixed, seamonkey2.16 fixed)

RESOLVED FIXED
seamonkey2.16
Tracking Status
seamonkey2.15 --- fixed
seamonkey2.16 --- fixed

People

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

References

Details

Attachments

(2 files, 1 obsolete file)

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.
Attached patch Patch v1.0 Proposed fix. (obsolete) — Splinter Review
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.
Attachment #669172 - Flags: review?(iann_bugzilla)
(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 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.
Attachment #669172 - Flags: review?(iann_bugzilla) → review+
(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.
> 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.
> 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.
Attachment #669172 - Attachment is obsolete: true
Attachment #670404 - Flags: review?(iann_bugzilla)
Blocks: 799529
Attachment #670404 - Flags: review?(iann_bugzilla) → review+
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.
Attachment #670404 - Flags: approval-comm-aurora?
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.
Attachment #673274 - Flags: review?(iann_bugzilla)
Attachment #670404 - Flags: approval-comm-aurora? → approval-comm-aurora+
Comment on attachment 673274 [details] [diff] [review]
Patch t1.0 Fix tests. [check-in comment 11]

Looks good r=me
Attachment #673274 - Flags: review?(iann_bugzilla) → review+
Comment on attachment 673274 [details] [diff] [review]
Patch t1.0 Fix tests. [check-in comment 11]

[Triage Comment]
Attachment #673274 - Flags: approval-comm-aurora+
Comment on attachment 673274 [details] [diff] [review]
Patch t1.0 Fix tests. [check-in comment 11]

Pushed to comm-central
Attachment #673274 - Attachment description: Patch t1.0 Fix tests. → Patch t1.0 Fix tests. [check-in comment 11]
Attachment #670404 - Attachment description: Patch v1.1 Arguments changed/explained and comment fixed. → Patch v1.1 Arguments changed/explained and comment fixed. [check-in comment 7]
Pushed to comm-aurora.
http://hg.mozilla.org/releases/comm-aurora/rev/bbcd326bc0b0
http://hg.mozilla.org/releases/comm-aurora/rev/640b3961018d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.16
You need to log in before you can comment on or make changes to this bug.