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

RESOLVED FIXED in seamonkey2.52

Status

defect
RESOLVED FIXED
2 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)

Assignee

Description

2 years ago
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
Assignee

Comment 1

2 years ago
Posted patch 1331203-saveimageurl.patch (obsolete) — Splinter Review
Attachment #8826859 - Flags: review?(philip.chee)
Assignee

Comment 2

2 years ago
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)
Assignee

Comment 3

2 years ago
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 4

2 years ago
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!

Updated

2 years ago
Attachment #8826872 - Flags: review?(philip.chee)

Comment 5

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

Comment 6

2 years ago
> 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.
Assignee

Comment 7

2 years ago
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)
Assignee

Comment 8

2 years ago
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 9

2 years ago
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+
Assignee

Updated

2 years ago
Duplicate of this bug: 1370898
Assignee

Updated

2 years ago
Blocks: 1388729
You need to log in before you can comment on or make changes to this bug.