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

RESOLVED FIXED in seamonkey2.52

Status

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: frg, Assigned: frg)

Tracking

(Blocks 1 bug)

SeaMonkey 2.44 Branch
seamonkey2.52
All
Unspecified
Dependency tree / graph

SeaMonkey Tracking Flags

(seamonkey2.46 wontfix, seamonkey2.47 wontfix, seamonkey2.48 wontfix, seamonkey2.49esr fixed, seamonkey2.50 fixed, seamonkey2.51 fixed, seamonkey2.52 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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
Posted patch 1331203-saveimageurl.patch (obsolete) — Splinter Review
Attachment #8826859 - Flags: review?(philip.chee)
Comment on attachment 8826859 [details] [diff] [review]
1331203-saveimageurl.patch

Drat needs an additonal fix to work with mailnews.
Attachment #8826859 - Flags: review?(philip.chee)
Posted patch 1331203-saveimageurl-V2.patch (obsolete) — Splinter Review
As a bonus seems to set image names correctly now when saving images from draft messages.
Attachment #8826859 - Attachment is obsolete: true
Attachment #8826872 - Flags: review?(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!
Attachment #8826872 - Flags: review?(philip.chee)
Maybe you can fix Bug 1326377 next?
Bug 1326377 - Media view 'Save As ...' for embedded(?) picture fails
> 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.
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.
Attachment #8826872 - Attachment is obsolete: true
Attachment #8833747 - Flags: review?(philip.chee)
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: --
Attachment #8833747 - Flags: review?(philip.chee)
Attachment #8833747 - Flags: review?(iann_bugzilla)
Attachment #8833747 - Flags: approval-comm-release?
Attachment #8833747 - Flags: approval-comm-beta?
Attachment #8833747 - Flags: approval-comm-aurora?
Comment on attachment 8833747 [details] [diff] [review]
1331203-saveimageurl-V3.patch

r/a=me
Attachment #8833747 - Flags: review?(iann_bugzilla)
Attachment #8833747 - Flags: review+
Attachment #8833747 - Flags: approval-comm-release?
Attachment #8833747 - Flags: approval-comm-release+
Attachment #8833747 - Flags: approval-comm-beta?
Attachment #8833747 - Flags: approval-comm-beta+
Attachment #8833747 - Flags: approval-comm-aurora?
Attachment #8833747 - Flags: approval-comm-aurora+
Duplicate of this bug: 1370898
Blocks: 1388729
You need to log in before you can comment on or make changes to this bug.