Magnus Melin [:mkmelin] Reporter
	Description • 10 years ago

+++ This bug was initially created as a clone of Bug #457296 +++

When bug 457296 lands AllowRemoteContent property in the address book will be gone. 
For the remote content in mail buttons to work, SeaMonkey should adapt the code to use the permission manager instead.

The Thunderbird UI is in bug 953426 (which is still awaiting review), with some small changes in bug 457296. 

At least you want to call something like allowRemoteContentForURI, 
and remove the "allow remote" checkboxes from ab. See especially attachment 8405845 [details] [diff] [review].

	Philip Chee Assignee
	Comment 2 • 9 years ago

>  # LOCALIZATION NOTE (remoteContentBarMessage): %S is the brandname
>  remoteContentBarMessage=To protect your privacy, %S has blocked remote content in this message.
> -remoteContentBarButton=Show Remote Content
> -remoteContentBarButtonKey=S
> +remoteContentPrefLabel=Options
> +remoteContentPrefAccesskey=O
Thunderbird uses Options/Preferences. This doesn't sound quite right. How about "Actions" instead?

Differences with the Thunderbird implementation:
1. The list of blockable sites/hosts in Thunderbird is held in an attribute on the XUL popup element as a space separated strings.
I am using a property on gMessageNotificationBar. remoteHosts is a Set().
2. TB spends a lot of time converting the hosts back and forth between strings and nsIURIs.
In my Set, the keys are always strings and I only convert to a URI at the point of adding the host to the permissions manager.

	Ian Neal
	Comment 3 • 9 years ago

Comment on attachment 8602247 [details] [diff] [review]
Patch v1.0 Proposed implementation.

Should we add an "Exceptions" button next to the preferences for "Block images and other content from remote sources"? It will be interesting find an access key though!
Need to raise a bug about updating help pages too.

	neil@parkwaycc.co.uk
	Comment 4 • 9 years ago

Comment on attachment 8602247 [details] [diff] [review]
Patch v1.0 Proposed implementation.

>+  remoteHosts: null,
>+
>+  setRemoteContentMsg: function(aMsgHdr, aContentURI)
>+  {
>+    // remoteHosts is a Set of all blockable hosts.
>+    if (!this.remoteHosts)
>+      this.remoteHosts = new Set();
When you "reset" this set, you set it to a new Set. So why not initialise it to a new Set too, that way you know it's always a Set. (Or, reset it to null instead, so it's only a Set if there's something in the set.)

>+    hosts.splice(0, 0, authorEmailAddress);
unshift

>+  if(!Services.prefs.getBoolPref("browser.preferences.instantApply"))
>+    ReloadMessage();
??? (Also, space after if.)

	Philip Chee Assignee
	Comment 5 • 9 years ago

(In reply to Ian Neal from comment #3)
> Comment on attachment 8602247 [details] [diff] [review]
> Patch v1.0 Proposed implementation.
> 
> Should we add an "Exceptions" button next to the preferences for "Block
> images and other content from remote sources"? It will be interesting find
> an access key though!
Our UI is a wee bit more crowded than TB. Do we have space for that?

	Karsten Düsterloh
	Comment 6 • 9 years ago

Comment on attachment 8602247 [details] [diff] [review]
Patch v1.0 Proposed implementation.

moa+ by inspection.

Just some nits:

>+function onRemoteContentOptionsShowing(aEvent) {

Move { onto new line.

>+  for (let child of childNodes) {
>+    child.remove();
>+  }

No braces needed here. 

>+  for (let host of hosts) {
>+function allowRemoteContentForURI(aItem) {
>+function editRemoteContentSettings() {

Move { onto new line.

	Philip Chee Assignee
	Comment 7 • 9 years ago

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/3c00ed4749ee

	neil@parkwaycc.co.uk
	Comment 8 • 9 years ago

Comment on attachment 8602247 [details] [diff] [review]
Patch v1.0 Proposed implementation.

>+    var headerParser = MailServices.headerParser;
>+    // update the allow remote content for sender string
>+    var mailbox = headerParser.extractHeaderAddressMailboxes(aMsgHdr.author);
>+    var emailAddress = mailbox || aMsgHdr.author;
>+    var displayName = headerParser.extractHeaderAddressName(aMsgHdr.author);
Unused?

>+  MailServices.headerParser.parseHeadersWithArray(
>+    gMessageDisplay.displayedMessage.author, addresses, {}, {});
>+  var authorEmailAddress = addresses.value[0];
Nit: parseHeadersWithArray is deprecated, use parseEncodedHeader()[0].email instead.

>+  if(!Services.prefs.getBoolPref("browser.preferences.instantApply"))
>+    ReloadMessage();
Still ???

	Magnus Melin [:mkmelin] Reporter
	Comment 9 • 9 years ago

Re browser.preferences.instantApply, it's to reload the message to reflect changes you might have done. See bug 926473 comment 13 or so.

	Joey Connick
	Comment 10 • 8 years ago

So... this says it is fixed in 2.38 but I'm on 2.40 and I can't make remote content show by selecting Options --> "Allow remote content for <email_address>"

Am I missing something?

	David C. Mores
	Comment 11 • 8 years ago

I had been using 2.38 because it did seem to be fixed there, but 2.39 was broken again to the point where I went back to 2.38.  I migrated to 2.40 recently, and see that it is still broken.  When given the option list on a message, clicking on one of the <email_address> entries results in varied results like the clicked on result may or may not disappear from the list and that may or may not result in a change in what is displayed in the email message.  The only thing that does seem to work is the first entry in the list (show remote content in this message), but it does not get remembered in subsequent Seamonkey sessions.  You have to go through the options list again.

I will go back to the 2.38 version again, when I am sufficiently annoyed.