133355 - implement adding a fake account to folder pane

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[shliang]

see http://bugscape.mcom.com/show_bug.cgi?id=11513 for full summary

Comment 1

[Lori Kaplan]

Approving this bug based on Bugscape approval from Mail team in bug 11513 (see
below). Changes required to Moz files for the implementation. 

Comment 2

[shliang]

Attachment: updated patch

previous patches see bugscape 11513

w/ some of bhuvan's changes, fixed some other stuff that i found

Comment 3

[(not reading, please use [e-mail] instead)]

1)

reuse the helper function GetSelectedFolderResource()

+function IsFakeAccountSelected() 
+{
+  try {
+    var folderResource = GetSelectedFolderResource();
+    return (folderResource.Value ==
"http://home.netscape.com/NC-rdf#PageTitleFakeAccount");
+  }
+  catch(ex) {
+  }
+  return false;
+}

2)

make sure to test and make sure this code plays nice with ssu's change for right
click.

also, does right clicking on this fake account give you any context menus?

3)

why is the change to the folderListener in msgMail3PaneWindow.js needed?

4)

Index: mailnews/base/src/nsMsgAccountManagerDS.cpp
===================================================================
RCS file: /cvsroot/mozilla/mailnews/base/src/nsMsgAccountManagerDS.cpp,v
retrieving revision 1.81
diff -u -w -u -r1.81 nsMsgAccountManagerDS.cpp
--- mailnews/base/src/nsMsgAccountManagerDS.cpp	19 Feb 2002 08:42:06 -0000	1.81
+++ mailnews/base/src/nsMsgAccountManagerDS.cpp	27 Mar 2002 20:19:55 -0000
@@ -56,9 +56,12 @@
 #include "nsIMsgFolder.h"
 #include "nsMsgBaseCID.h"
 #include "nsMsgIncomingServer.h"
+#include "nsIImapIncomingServer.h"

not needed, worse, makes base depend on imap.

5)

+#include "nsCRT.h"

not going to be needed, see below.  don't use nsCRT::strcmp() for str compares
of char *.  use strcmp(), it's faster.


6)

Use "mailnews.fakeaccount.show" instead of waiting for the CreateBundle() call
to fail.
 
+      else if (source == kNC_PageTitleFakeAccount) {
+        nsCOMPtr<nsIStringBundleService> strBundleService =
do_GetService(NS_STRINGBUNDLE_CONTRACTID, &rv);
+        if (NS_FAILED(rv)) return rv;
+
+        rv =
strBundleService->CreateBundle("chrome://messenger/locale/fakeAccount.properties",
+                                             getter_AddRefs(mStringBundle));
+        if (NS_SUCCEEDED(rv))
+           
mStringBundle->GetStringFromName(NS_LITERAL_STRING("prefPanel-fake-account").get(), 
+                                             getter_Copies(pageTitle));
+      }
+

7)
  
add to mailnews.js

pref("mailnews.fakeaccount.show",false);

you can remove the NS_FAILED(rv) check and the comment.

+  // Pref is in mailnews-ns.js but not mailnews.js, so don't show if this is
+  // not Netscape or if it is turned off.
+  if (!showFakeAccount || NS_FAILED(rv))
+    return PR_FALSE;


8)

+  if (!am) return NS_ERROR_FAILURE;
+  if (NS_FAILED(rv)) return rv;

the convention (these days) is to put return on a newline so that we can set a
break point.

9)

this should be a call to FindServer()

+  nsCOMPtr<nsISupportsArray> servers;
+  rv = am->GetAllServers(getter_AddRefs(servers));
+  if (NS_FAILED(rv)) return rv;
+
+  PRUint32 length;
+  servers->Count(&length);
+
+  // Go thru servers and check if any of them are Netscape WebMail accounts.
+  // If one exists, then "Get Free Web Mail" is not displayed.
+  for (PRUint32 i = 0; i < length; ++i) {
+    nsCOMPtr<nsISupports> serverSupports;
+    servers->GetElementAt(i, getter_AddRefs(serverSupports));
+    nsCOMPtr<nsIMsgIncomingServer> server(do_QueryInterface(serverSupports, &rv));
+    if (NS_FAILED(rv)) 
+      continue;
+
+    char* type;
+    server->GetHostName(&type);
+
+    if (nsCRT::strcmp(type, FAKE_ACCOUNT_SERVER) == 0)
+      return PR_FALSE;
+  }  

nsXPIDLCString fakeHostName;
rv = GetFakeHostNameFromPrefs(getter_Copies(fakeHostName));
NS_ENSURE_SUCCESS(rv,rv);

if (!prefValue.IsEmpty()) {
  rv = accountManager->FindServer("",fakeHostName.get(),"", getter_AddRefs(server));
  if (NS_SUCCEEDED(rv) && server)
    return PR_FALSE;
}
  
10)

+#define FAKE_ACCOUNT_SERVER "imap.mail.netcenter.com"

this has to be in mailnews.js:

pref("mailnews.fakeaccount.show.hostname","");

in mailnews-ns.js:

pref("mailnews.fakeaccount.show.hostname","imap.mail.netcenter.com");

nsresult
nsMsgAccountManagerDataSource::IsIncomingServerForFakeAccount(nsIMsgIncomingServer*
aServer, PRBool *aResult)
{
  NS_ENSURE_ARG_POINTER(aServer);
  NS_ENSURE_ARG_POINTER(aResult);
  
  nsXPIDLCString fakeHostName;
  rv = GetFakeHostNameFromPrefs(getter_Copies(fakeHostName));
  NS_ENSURE_SUCCESS(rv,rv);

  if (fakeHostName.IsEmpty()) {
    *aResult = PR_FALSE;
    return NS_OK;
  }

  nsXPIDLCString hostname;
  rv = aServer->GetHostName(getter_Copies(hostName));
  NS_ENSURE_SUCCESS(rv,rv);

  *aResult = (strcmp(hostname.get(), pref val) == 0);
  return NS_OK;
}

nsresult 
nsMsgAccountManagerDataSource::GetFakeHostName(char **aHostName)
{
  NS_ENSURE_ARG_POINTER(aHostName);
  // get pref branch
  // get value of "mailnews.fakeaccount.show.hostname" pref
  // note, you must define this pref in mailnews.js as "" and override
mailnews-ns.js 
  return NS_OK;
}

Comment 4

[(not reading, please use [e-mail] instead)]

instead of "mailnews.fakeaccount.show.hostname"

let's use the pref you defined in your bugscape patch:

+pref("mailnews.fakeaccount.server", "imap.mail.netcenter.com");

just make sure to define defaults for this pref and the .show pref in mailnews.js
and make the code handle the empty case.

Comment 5

[Olga]

Interesting, after Seth comments my change QA contact to nbaca does not work.
One more time.

Comment 6

[shliang]

2) 
also, does right clicking on this fake account give you any context menus?

nope, no context menu

3)

why is the change to the folderListener in msgMail3PaneWindow.js needed?

it's a change to GetSelectedMsgFolders so that it doesn't do it if the fake 
account is selected

Comment 7

[scottputterman]

adding adt2.

Comment 8

[shliang]

Attachment: patch

with above changes

Comment 9

[(not reading, please use [e-mail] instead)]

Comment on attachment 77579 [details] [diff] [review]
patch

sr=sspitzer

looks good, please have bhuvan do the review.

when testing, make sure that for mozilla only builds, nothing happens.

also make sure that in your ns build, this "fake account" doesn't show up in
the account tree in the account manager.

Comment 10

[(not reading, please use [e-mail] instead)]

also, this "fake account" shouldn't show up in any of the pickers or UI in the 
product that use the account manager datasource:

1) folder pickers for new folder, rename, etc
2) copies and folder pickers in the account manager
3) file / move button flyouts
4) file / move / copy context menus
5) offline select dialog

basically, look for anything that uses the account manager datasource, and make 
sure that in your ns build, this fake account only shows up there.

I think it won't, but please verify. 

Comment 11

[shliang]

nope, doesn't show up in any of those.

bhuvan, could you review please?

thanks :)

Comment 12

[racham]

Comment on attachment 77579 [details] [diff] [review]
patch

r=bhuvan

Comment 13

[Lori Kaplan]

nominating for beta. Needed for mail account acquisition for NS. No impact to
Mozilla build. 

Comment 14

[Jaime Rodriguez, Jr.]

adt1.0.0+ (on ADT's behalf) approval for checkin to 1.0. Chaning to ADT1.

Comment 15

[Asa Dotzler [:asa]]

Comment on attachment 77579 [details] [diff] [review]
patch

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 16

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

Can you explain this fake account thing? What is it good for? Why check it into
Mozilla if it's only needed for NSCP anyway? What's the NS tree for?

Comment 17

[(not reading, please use [e-mail] instead)]

> Can you explain this fake account thing? 
it allows ISPs (any ISPs, Earthlink, AOL, etc) to create mozilla builds with UI 
to drive users to set up mail accounts on their service.

> Why check it into Mozilla if it's only needed for NSCP anyway? What's the NS 
tree for?

mozilla needed changes to support this feature.  But only in the NS tree do we 
take advantage of this new feature.  Other ISPs can create mozilla builds with 
this feature for their ISP.

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

Some comments as a driver, even though this has a= already:

We should not be approving checkins to the main mozilla tree with a
description/justification of "please see internal netscape bug".  I'm a driver;
I can't read netscape-internal documents, and many other developers certainly
can't. Seth's comment does start to explain it after the fact, though I'd really
want to see more information, and some idea of what this really does and why. 
Also an side-effects of having this code in all builds and any risks, since it's
only going to be used by netscape and perhaps for custom builds by other ISP's -
a build option might have been appropriate (or might not have, but the
discussion and design was hidden, so I can't tell).

Comment 19

[Karen Huang]

I saw an additional icon displays on the folder pane on Linux platform ONLY, is
that icon comes from this bug fix?

Comment 20

[Randell Jesup [:jesup] (needinfo me)]

Unless this was checked in without notice here (which would be bad...), it isn't
this - unless you're running this patch locally.

Comment 21

[Brian Ryner (not reading)]

This patch was indeed checked in at about 5AM today.  If you're seeing this
extra icon on today's Linux nightly, this could be the culprit.

Comment 22

[(not reading, please use [e-mail] instead)]

others (nbaca) are seeing the bogus folder on linux mozilla (and linux ns).

the folder should not be there for linux mozilla, I'll help investigate.

note to shuehan:  I'm not seeing the fake account in the ns builds, linux or 
windows.  should it be there?

Comment 23

[(not reading, please use [e-mail] instead)]

I've taken the issue of the bogus folder on mozilla linux to another bug.

see bug #136730.

note to netscape QA, until we enable this feature, you should not see anything 
in mozilla or ns builds.

once the feature is enabled, you'll see the "FREE Webmail" thing in ns builds.

the feature is not enabled yet.

Comment 24

[(not reading, please use [e-mail] instead)]

> I saw an additional icon displays on the folder pane on Linux platform ONLY, is
> that icon comes from this bug fix?

that's a netscape only bug.

Comment 25

[(not reading, please use [e-mail] instead)]

> We should not be approving checkins to the main mozilla tree with a
> description/justification of "please see internal netscape bug".  I'm a driver;
> I can't read netscape-internal documents, and many other developers certainly
> can't. 

In the future, I'll do a better job of explain what the primarily ns driven
changes to mozilla (for mailnews) are for.  If something isn't clear, always
feel free to send me mail directly letting me know that you need more
information in a particular bug.  (I get too much bugmail to respond to bugzilla
comments in a timely fashion.)

> Seth's comment does start to explain it after the fact, though I'd really
> want to see more information, and some idea of what this really does and why.

In this case, by setting a few prefs, and packaging up some addition xul and dtd
files, those who package and distribute products based on mozilla can provide
additional UI hooks to aid users to create accounts.

Mailnews has been good about making any features we add for netscape friendly to
 other ISPs.  For example, you can extend the account wizard for other ISPs.
(see http://lxr.mozilla.org/mozilla/source/mailnews/base/ispdata/example.rdf),
and imap redirectors.

Right now, we're a little behind in documenting some of the new features.

There is some information in various bugs and in previous posts to
n.p.m.mail-news, but nothing formal, complete and 100% up to date.

We've been getting better about adding documentation (see
http://www.mozilla.org/mailnews/arch/index.html), but more is needed.

> Also an side-effects of having this code in all builds and any risks, 

I agree, any changes imply some level of risk.  In this case, the changes to
mozilla seemed low risk, and should not have impacted mozilla users (no UI
changes for default mozilla builds, and no performance implications.)

There was a leak jump (see 136643, I'm investigating).

The "fake folder" issue is ns only, but that doesn't affect mozilla users.

> since it's only going to be used by netscape and perhaps for custom builds by 
> other ISP's - a build option might have been appropriate 
> (or might not have, but the discussion and design was hidden, so I can't tell).

As with the other ISP friendly features, we've been trying to put them into the
mozilla build by default.  This way, other ISPs could just provide .xpis with
all the components, default prefs and UI, instead of having to provide their own
mozilla distribution.

let me know if you have more questions.

Comment 26

[shliang]

fixed

Comment 27

[Mike Kaply [:mkaply]]

This checkin broke mailnews on OS/2.

See:

http://bugzilla.mozilla.org/show_bug.cgi?id=137044

We have no idea why.

Suggestions are welcome.

We definitely verified that backing out the JS changes in this bug fixed it.

Comment 28

[Mike Kaply [:mkaply]]

This happens on Windows as well with a new profile!

Comment 29

[(not reading, please use [e-mail] instead)]

> We definitely verified that backing out the JS changes in this bug fixed it.

which js changes did you back out?

Comment 30

[Mike Kaply [:mkaply]]

There are changes to one C file and a few JS files in this patch.

First we tried the C file and it did not fix it.

Then we did the JS files and it fixed it.

We have not narrowed down which JS file caused it yet.

What we have determined is that a mail window is being opened the second time, 
but creation of the window is never being completed for some reason.

Comment 31

[Mike Kaply [:mkaply]]

Should we close this back out and keep discussions in the new bug (136874)

Comment 32

[(not reading, please use [e-mail] instead)]

yes, let's close this and take this to the other bug.

if we end up backing it out, we'll reopen.

Comment 33

[(not reading, please use [e-mail] instead)]

for completeness, bug 136874 was fixed.

Comment 34

[Ninoschka Baca]

When Esther was testing bug# 135594 she also checked this bug and it appears to
be fixed.

Comment 35

[esther]

removing fixed 1.0.0, this was tested with branch builds on winme, winxp, mac
os9.1, mac x and linux.

Comment 36

[Asa Dotzler [:asa]]

adding branch verification keyword.  This bug has been verified in a comment,
just moving that to the keyword field.