1331203 - Port Bug 1243643 - ["Save Image As..." Not working] to SeaMonkey

Status: RESOLVED FIXED

(SeaMonkey :: Download & File Handling, defect)

Description

[Frank-Rainer Grahl (:frg)]

While SeaMonkey has no problem with saving images (code in contentAreaUtils.js sets defaults and e10s is not enabled) the resulting log error is distracting:

> Error: DEPRECATION WARNING: saveImageURL should be passed the private 
> state of the containing window.
> You may find more details about this deprecation at: 
> https://bugzilla.mozilla.org/show_bug.cgi?id=1243643
> chrome://global/content/contentAreaUtils.js 154 saveImageURL
> chrome://communicator/content/nsContextMenu.js 1122 
> nsContextMenu.prototype.saveMedia
>
> chrome://navigator/content/navigator.xul 1 oncommand

Comment 1

[Frank-Rainer Grahl (:frg)]

Attachment: 1331203-saveimageurl.patch

Comment 2

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8826859 [details] [diff] [review]
1331203-saveimageurl.patch

Drat needs an additonal fix to work with mailnews.

Comment 3

[Frank-Rainer Grahl (:frg)]

Attachment: 1331203-saveimageurl-V2.patch

As a bonus seems to set image names correctly now when saving images from draft messages.

Comment 4

[Philip Chee]

Comment on attachment 8826872 [details] [diff] [review]
1331203-saveimageurl-V2.patch

> +Components.utils.import("resource://gre/modules/PrivateBrowsingUtils.jsm");
We can use gPrivate here. The rest of nsContextMenu.js uses gPrivate. This is not available in mailnews which doesn't have the concept of private browsing. However we can still use gPrivate because it is set to null in MailNews.

> +    let isPrivate = PrivateBrowsingUtils.isBrowserPrivate(this.browser);
We don't have individual private tabs.

>      saveImageURL(canvas.toDataURL("image/jpeg", ""), name, "SaveImageTitle",
>                                    true, true,
>                                    this.target.ownerDocument.documentURIObject,
> -                                  this.target.ownerDocument);
> +                                  null, null, null, isPrivate);
Since gPrivate is null in MailNews we can just use gPrivate everwhere instead of isPrivate. (I think, please test this)

> +    let referrerURI = doc.documentURIObject;
> +    let isPrivate = PrivateBrowsingUtils.isBrowserPrivate(this.browser);
Looks like we don't need to import PrivateBrowsingUtils.jsm. Phew!

>      if (this.onCanvas)
>        // Bypass cache, since it's a data: URL.
>        saveImageURL(this.target.toDataURL(), "canvas.png", "SaveImageTitle",
> -                   true, true, null, doc);
> -    else if (this.onImage)
> -      saveImageURL(this.mediaURL, null, "SaveImageTitle", false, true,
> -                   doc.documentURIObject, doc);
> +                   true, false, referrerURI, null, null, null,
> +                   isPrivate);
......................gPrivate

> +    else if (this.onImage) {
> +        // gContextMenuContentData is null when called via mailWindowOverlay.xul.
> +        if (gContextMenuContentData)
> +          saveImageURL(this.mediaURL, null, "SaveImageTitle", false,
> +                       false, referrerURI, null, gContextMenuContentData.contentType,
> +                       gContextMenuContentData.contentDisposition, isPrivate);
> +        else
> +          saveImageURL(this.mediaURL, null, "SaveImageTitle", false,
> +                       false, referrerURI, null, null, null, isPrivate);
> +    }

Maybe:

saveImageURL(this.mediaURL, null, "SaveImageTitle", false, false,
             referrerURI, null,
             gContextMenuContentData ? gContextMenuContentData.contentType : null,
             gContextMenuContentData ? gContextMenuContentData.contentDisposition : null,
             gPrivate);

Although we should keep this comment.
> +        // gContextMenuContentData is null when called via mailWindowOverlay.xul.

Ratty cogitates.....
Why is gContextMenuContentData null when called from mailWindowOverlay.xul?

>   <menupopup id="mailContext"
>          onpopupshowing="return event.target != this ||
>                                 FillMailContextMenu(this);"
.....
> function FillMailContextMenu(aTarget)
> {
>   var inThreadPane = InThreadPane(aTarget);
>   gContextMenu = new nsContextMenu(aTarget, getBrowser());

Oops. This should be something like:
gContextMenu = new nsContextMenu(this, event.shiftKey, event);
.....
    if (aEvent)
      this.initContentData(aEvent);
.....

So gContextMenuContentData is null because we are not passing in the event.
We should definitely pass the event to FillMailContextMenu(aTarget, aEvent)
Which should then pass it to nsContextMenu()
Gurgle! Gurgle!

We iz wantz newz patchzz!

Comment 5

[Philip Chee]

Maybe you can fix Bug 1326377 next?
Bug 1326377 - Media view 'Save As ...' for embedded(?) picture fails

Comment 6

[Frank-Rainer Grahl (:frg)]

> Since gPrivate is null in MailNews we can just use gPrivate everwhere instead of isPrivate. 
> (I think, please test this)

Using gPrivate results in:
> Error: Error: saveImageURL couldn't compute private state of content window
> Source File: chrome://global/content/contentAreaUtils.js
> Line: 158

Checking what needs to be done to make it work.

Comment 7

[Frank-Rainer Grahl (:frg)]

Attachment: 1331203-saveimageurl-V3.patch

This seems to work. Tested with a private browser window and mail etc...

> gContextMenu = new nsContextMenu(this, event.shiftKey, event);
 
Using the suite nsContextMenu prototype in nsContextMenu.js results in a context menu with the browser items mixed into the mail items so just removed the imho useless getBrowser() from the call. Only im/content/nsContextMenu.js has this prototype so maybe copied from IM code a long time ago or it changed some time ago without changing it here too.

Comment 8

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8833747 [details] [diff] [review]
1331203-saveimageurl-V3.patch

Redirecting review to IanN because Ratty is absent.

Needed also im comm-esr52.

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: not much impact but seaving images sets the name correctly in some cases.
Testing completed (on m-c, etc.): c-r to c-c
Risk to taking this patch (and alternatives if risky): not uch risk but I would probably wait a little time until putting it in release.

String changes made by this patch: --

Comment 9

[Ian Neal]

Comment on attachment 8833747 [details] [diff] [review]
1331203-saveimageurl-V3.patch

r/a=me

Comment 10

[Frank-Rainer Grahl (:frg)]

https://hg.mozilla.org/comm-central/rev/a9362ac035d86bc03da71cfaa530d0c588201462
https://hg.mozilla.org/releases/comm-aurora/rev/02b43fc9af4255a0125309e527c3a5db3caf2215
https://hg.mozilla.org/releases/comm-beta/rev/ca4788cafa29df4915670fe013e388de705d7daf
https://hg.mozilla.org/releases/comm-release/rev/95cca2602d76e5be7c06a55c37521f3d8f07606e
https://hg.mozilla.org/releases/comm-esr52/rev/3a1dfd9b7e4acb6eac5f0f6f9ade9d9cdaee1e62

I missed an unintentional whitespace change:
https://hg.mozilla.org/comm-central/rev/11634182063c1d71bfc222faa3a3a0011f900b41
https://hg.mozilla.org/releases/comm-aurora/rev/5aa9e909f6b5cbeb6a888b9002ab40176112379b
https://hg.mozilla.org/releases/comm-beta/rev/71f51b24ab02a9da68199e41cd6ed52893839862