995737 - Adapt seamonkey for the addressbook remote content policy change; use the permission manager instead of address book property

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Address Book & Contacts, defect)

Description

[Magnus Melin [:mkmelin]]

+++ 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].

Comment 2

[Philip Chee]

Attachment: Patch v1.0 Proposed implementation.

>  # 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.

Comment 3

[Ian Neal]

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.

Comment 4

[[e-mail]]

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.)

Comment 5

[Philip Chee]

(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?

Comment 6

[Karsten Düsterloh]

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.

Comment 7

[Philip Chee]

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

Comment 8

[[e-mail]]

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 ???

Comment 9

[Magnus Melin [:mkmelin]]

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

Comment 10

[Joey Connick]

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?

Comment 11

[David C. Mores]

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.