309068 - URL presence check should be disabled by default.

Status: RESOLVED FIXED

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

Description

[Mark Banner (:standard8)]

In Thunderbird bug 295726, Scott commented:

> It looks like we still have some residual code from the Netscape e-mail client
> days in the address book which trys to resolve aim presence for any address 
> book card that has a screen name in it when you display that card.

> We're currently pinging a server on oscar.aol.com to see if that person is
> online.

> We should disable this for now so we aren't hitting this server without
> informing the user. 

> I'd still like to investigate presence support in Thunderbird for a variety of
> IM clients (not just AOL) down the line, but we'll want UI for allowing a user
> to turn it on and off.

Rather than disabling this completely, I think we should provide a preference
(disabled by default) for the user to enable it as they wish. This could be
modified later (via other existing bugs) to extend for other IM systems, and
allow changing of the server to check.

Comment 1

[Mark Banner (:standard8)]

Attachment: Draft Version

This is a draft patch for what I think we should do for this bug. Basically, it allows a pref to be set for whether or not to do the check.

At the moment I've put the pref on the Mail/News -> Addressing page, but I don't think thats the right place, and it'll currently break Thunderbird (we share the xul). How about just the top level Mail/News?

The idea is that later we might expand this in future into providing different possibilites for im presence checks (if there are any) and perhaps allowing interfaces to different IM clients.

Any suggestions/thoughts on locations/wording would be useful :-)

Comment 2

[[e-mail]]

Comment on attachment 201356 [details] [diff] [review]
Draft Version

How about "Check AOL screen name online presence"? We could make it a menuitem in the XUL, as that's not shared, right?

Comment 3

[Mark Banner (:standard8)]

Attachment: Better version

ok, I like this version better following Neil's suggestion. This one gives a menu item to enable/disable the screen name check.

Comment 4

[Karsten Düsterloh]

Comment on attachment 201374 [details] [diff] [review]
Better version

Just a few style nits:

>Index: mailnews/addrbook/resources/content/addressbook.js
>===================================================================
>     document.getElementById("cmd_SortBy_PhoneticName")
>             .setAttribute("hidden", "false");
> 
>+  document.getElementById("allowOnlineCheck").setAttribute('checked',
>+    gPrefs.getBoolPref("mail.addr_book.im.onlineCheckAllowed"));

Maybe you could make "mail.addr_book.im.onlineCheckAllowed" a global constant (since you are using it twice in this file)? This would help avoiding the ugly line wrapping above:

document.getElementById("allowOnlineCheck")
        .setAttribute("checked", gPrefs.getBoolPref(ksOnlineCheckAllowed));

In any case, 'checked' should be in "" like the rest of the strings, and "allowOnlineCheck" should be named "menu_allow_online_check" to match "menu_search_adresses" in the very same menu in addressbook.xul.

>+function onAllowOnlineCheck(target)
>+{
>+  // Update the pref
>+  gPrefs.setBoolPref("mail.addr_book.im.onlineCheckAllowed",
>+                     document.getElementById("allowOnlineCheck").
>+                              getAttribute("checked") ? true : false);

First, the dot behind getElementById() belongs onto the next line.
Second, | getAttribute("checked") ? true : false | is just | getAttribute("checked") == "true" | or | Boolean(getAttribute("checked")) | or even shorter | !!getAttribute("checked") |. 
No need for ?:.

gPrefs.setBoolPref(ksOnlineCheckAllowed, 
                   !!document.getElementById("allowOnlineCheck")
                             .getAttribute("checked"));


>Index: mailnews/addrbook/resources/content/addressbook.xul
>===================================================================
>+                      id="allowOnlineCheck"

See above.

r=me with that.

(Heh, this was the first time I used my AIM account for something useful. *g*)

Comment 5

[[e-mail]]

>>+  gPrefs.setBoolPref("mail.addr_book.im.onlineCheckAllowed",
>>+                     document.getElementById("allowOnlineCheck").
>>+                              getAttribute("checked") ? true : false);
>gPrefs.setBoolPref(ksOnlineCheckAllowed, 
>                   !!document.getElementById("allowOnlineCheck")
>                             .getAttribute("checked"));
Both wrong... getAttribute("checked") == "true" is the correct test.
[alternatively fix .checked to work for menuitems as well as everywhere else]

Comment 6

[Mark Banner (:standard8)]

Attachment: v3

This version addresses the previous comments. Carrying forward r, requesting sr.

Comment 7

[Karsten Düsterloh]

Comment on attachment 204139 [details] [diff] [review]
v3

Erm, I guess you attached the wrong patch? :)

>+      var promptService = Components.
>+                          classes["@mozilla.org/embedcomp/prompt-service;1"].
>+                          getService(Components.interfaces.nsIPromptService);

Don't put the dots at the end of the line...

Neil's "both" in comment #5 meant ?: and !!.

Comment 8

[Mark Banner (:standard8)]

Attachment: real v3

Ok, this is the real v3. I got the bug numbers wrong (300968 *is* very similar to 309068 when you're tired). Carrying forward the original r+

Comment 9

[[e-mail]]

Comment on attachment 204166 [details] [diff] [review]
real v3

>+pref("mail.addr_book.im.onlineCheckAllowed", false);
Nit: A cursory glance suggests that underline_join_style is used here, not camelCase.

>+  var onlineCheckAllowed = false;
>+  try {
>+    onlineCheckAllowed = gPrefs.getBoolPref("mail.addr_book.im.onlineCheckAllowed");
>+  }
>+  catch (ex) {}
Unnecessary as a) you define the pref anyway b) your startup would fail if the pref was missing.

>+                      checked="true"/>
Unnecessary as you set it on startup.

Comment 10

[Mark Banner (:standard8)]

Checked in with Neil's nits addressed.

Checking in mailnews/mailnews.js;
/cvsroot/mozilla/mailnews/mailnews.js,v  <--  mailnews.js
new revision: 3.256; previous revision: 3.255
done
Checking in mailnews/addrbook/resources/content/abCardViewOverlay.js;
/cvsroot/mozilla/mailnews/addrbook/resources/content/abCardViewOverlay.js,v  <--  abCardViewOverlay.js
new revision: 1.38; previous revision: 1.37
done
Checking in mailnews/addrbook/resources/content/addressbook.js;
/cvsroot/mozilla/mailnews/addrbook/resources/content/addressbook.js,v  <--  addressbook.js
new revision: 1.120; previous revision: 1.119
done
Checking in mailnews/addrbook/resources/content/addressbook.xul;
/cvsroot/mozilla/mailnews/addrbook/resources/content/addressbook.xul,v  <--  addressbook.xul
new revision: 1.175; previous revision: 1.174
done
Checking in mailnews/addrbook/resources/locale/en-US/abMainWindow.dtd;
/cvsroot/mozilla/mailnews/addrbook/resources/locale/en-US/abMainWindow.dtd,v  <--  abMainWindow.dtd
new revision: 1.56; previous revision: 1.55

Comment 11

[Mark Banner (:standard8)]

Comment on attachment 204166 [details] [diff] [review]
real v3

Requesting approval for checkin to the 1.8 branch for SeaMonkey 1.0b.

This is a SeaMonkey only patch that has already been checked in on trunk. It will add one pref to mailnews.js - a file which Thunderbird uses, however Thunderbird won't be affected by the change as it has no code to use the new pref.

Comment 12

[Mark Banner (:standard8)]

Scott, the "real v3" patch on this bug we'd like to be able to put into the 1.8 branch for SeaMonkey - however it adds just one new pref to mailnews.js (the rest of the changes are SeaMonkey specific).

I am therefore asking if you would grant us approval to check the patch into the branch.

Thanks.

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 204166 [details] [diff] [review]
real v3

SeaMonkey 1.0 freeze too close now to put it into mailnews.js, and I didn't realise Scott was away. So cancelling 1.8.0.1 approval request.

Comment 14

[Mark Banner (:standard8)]

Attachment: SeaMonkey-only Patch for 1.8 branch [checked in 1.8/1.8.0]

This patch is the same as real v3 but changed slightly for the branch - it adds the pref to browser-prefs.js rather than the mailnews.js which is shared between SeaMonkey & Thunderbird. I've added a comment warning to this as well. We can either change it when the tree opens for 1.8.1 or leave it and it will then be mailnews.js when we pick up the trunk again.

Comment 15

[[e-mail]]

Comment on attachment 205739 [details] [diff] [review]
SeaMonkey-only Patch for 1.8 branch [checked in 1.8/1.8.0]

r+sr=me for the temporary browser.js hack.

Comment 16

[Mark Banner (:standard8)]

Comment on attachment 205739 [details] [diff] [review]
SeaMonkey-only Patch for 1.8 branch [checked in 1.8/1.8.0]

Requesting SeaMonkey 1.0 approval for SeaMonkey-only patch. Should be low risk, allows the user to control whether or not we contact a server for the URL presence check.

Comment 17

[Robert Kaiser]

Comment on attachment 205739 [details] [diff] [review]
SeaMonkey-only Patch for 1.8 branch [checked in 1.8/1.8.0]

a=me for SeaMonkey-only branch checkin, still one needed to go

Comment 18

[Ian Neal]

Comment on attachment 205739 [details] [diff] [review]
SeaMonkey-only Patch for 1.8 branch [checked in 1.8/1.8.0]

a=me, here's the 2nd one ;-)

Comment 19

[Mark Banner (:standard8)]

Comment on attachment 205739 [details] [diff] [review]
SeaMonkey-only Patch for 1.8 branch [checked in 1.8/1.8.0]

Checked into branch.

Comment 20

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 205739 [details] [diff] [review]
SeaMonkey-only Patch for 1.8 branch [checked in 1.8/1.8.0]

it kinda would be nice if the browser-prefs.js comment would say why the pref is in that file rather than mailnews.js

Comment 21

[Mark Banner (:standard8)]

Comment on attachment 205739 [details] [diff] [review]
SeaMonkey-only Patch for 1.8 branch [checked in 1.8/1.8.0]

Checked in to 1.8.0 branch.

Checking in mailnews/addrbook/resources/content/abCardViewOverlay.js;
/cvsroot/mozilla/mailnews/addrbook/resources/content/abCardViewOverlay.js,v  <--  abCardViewOverlay.js
new revision: 1.36.26.1.2.1; previous revision: 1.36.26.1
done
Checking in mailnews/addrbook/resources/content/addressbook.js;
/cvsroot/mozilla/mailnews/addrbook/resources/content/addressbook.js,v  <--  addressbook.js
new revision: 1.117.2.1.2.1; previous revision: 1.117.2.1
done
Checking in mailnews/addrbook/resources/content/addressbook.xul;
/cvsroot/mozilla/mailnews/addrbook/resources/content/addressbook.xul,v  <--  addressbook.xul
new revision: 1.173.16.1; previous revision: 1.173
done
Checking in mailnews/addrbook/resources/locale/en-US/abMainWindow.dtd;
/cvsroot/mozilla/mailnews/addrbook/resources/locale/en-US/abMainWindow.dtd,v  <--  abMainWindow.dtd
new revision: 1.55.26.1; previous revision: 1.55

Comment 22

[Mark Banner (:standard8)]

Attachment: Move the pref back to mailnews.js in the branch.

1.8 branch patch to move the pref that we temporarily put in browser-prefs.js back to where it should be in mailnews.js (as per trunk). This means it will be in the correct location for 1.1.

Comment 23

[Mark Banner (:standard8)]

Comment on attachment 206623 [details] [diff] [review]
Move the pref back to mailnews.js in the branch.

Request approval to checkin patch. This moves the pref that we hacked for SeaMonkey 1.0 from browser-prefs.js to mailnews.js its correct location (we didn't want to affect Thunderbird 1.5) 

Low risk to both SeaMonkey & Thunderbird.

Comment 24

[Robert Kaiser]

Comment on attachment 206623 [details] [diff] [review]
Move the pref back to mailnews.js in the branch.

a=me for SeaMonkey given this gets "normal" a+ as well :)

Comment 25

[Benjamin Smedberg]

Comment on attachment 206623 [details] [diff] [review]
Move the pref back to mailnews.js in the branch.

This needs scott's approval since mailnews.js is shipped by tbird.

Comment 26

[Scott MacGregor]

Comment on attachment 206623 [details] [diff] [review]
Move the pref back to mailnews.js in the branch.

can we wrap this new pref in a seamonkey ifdef since it's only used by the suite?

Comment 27

[Mark Banner (:standard8)]

Attachment: ifdef out pref on trunk.

This patch adds an #ifdef for the trunk pref (for SeaMonkey). Revised branch patch coming up.

Comment 28

[Mark Banner (:standard8)]

Comment on attachment 210389 [details] [diff] [review]
ifdef out pref on trunk.

I just realised the pref is different on the trunk to the branch - unfortunately we're gonna have to change the trunk version to match the branch.

Comment 29

[Mark Banner (:standard8)]

Attachment: ifdef the pref for suite only and match trunk to branch (checked-in)

Ok, this patch matches the trunk pref to the branch one and adds the #ifdef so its only defined for SeaMonkey.

Comment 30

[Mark Banner (:standard8)]

Attachment: Move the pref back to mailnews.js in the branch and ifdef it for SeaMonkey only (checked in)

Comment 31

[Mark Banner (:standard8)]

Comment on attachment 210401 [details] [diff] [review]
Move the pref back to mailnews.js in the branch and ifdef it for SeaMonkey only (checked in)

Requesting branch approval for this SeaMonkey-only patch that moves a pref back into mailnews.js with a suite only ifdef.

Comment 32

[Robert Kaiser]

Comment on attachment 210401 [details] [diff] [review]
Move the pref back to mailnews.js in the branch and ifdef it for SeaMonkey only (checked in)

a=me for SeaMonkey 1.1