Closed Bug 133355 Opened 22 years ago Closed 22 years ago

implement adding a fake account to folder pane

Categories

(SeaMonkey :: MailNews: Message Display, defect)

Product:
SeaMonkey
Component:
MailNews: Message Display
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla1.0

People

(Reporter: shliang, Assigned: shliang)

References

(
URL
)

Details

(Whiteboard: [adt1])

Attachments

(1 file, 1 obsolete file)

patch
16.60 KB, patch
racham
: review+
sspitzer
: superreview+
asa
: approval+
Details | Diff | Splinter Review 
see http://bugscape.mcom.com/show_bug.cgi?id=11513 for full summary
Status: NEW → ASSIGNED
Keywords: nsbeta1
Target Milestone: --- → mozilla1.0
Approving this bug based on Bugscape approval from Mail team in bug 11513 (see
below). Changes required to Moz files for the implementation.
Keywords: nsbeta1 → nsbeta1+
Attached patch updated patch (obsolete) — — Splinter Review 
previous patches see bugscape 11513

w/ some of bhuvan's changes, fixed some other stuff that i found
QA Contact: olgam → nbaca
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;
}
QA Contact: nbaca → olgam
QA Contact: olgam → nbaca
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.
QA Contact: nbaca → olgam
Interesting, after Seth comments my change QA contact to nbaca does not work.
One more time.
QA Contact: olgam → nbaca
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


adding adt2.
Whiteboard: [adt2]
Attached patch patch — — Splinter Review 
with above changes
Attachment #76453 - Attachment is obsolete: true
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.
Attachment #77579 - Flags: superreview+
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. 
nope, doesn't show up in any of those.

bhuvan, could you review please?

thanks :)
Comment on attachment 77579 [details] [diff] [review]
patch

r=bhuvan
Attachment #77579 - Flags: review+
nominating for beta. Needed for mail account acquisition for NS. No impact to
Mozilla build.
Keywords: adt1.0.0
adt1.0.0+ (on ADT's behalf) approval for checkin to 1.0. Chaning to ADT1.
Keywords: adt1.0.0 → adt1.0.0+
Whiteboard: [adt2] → [adt1]
Comment on attachment 77579 [details] [diff] [review]
patch

a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #77579 - Flags: approval+
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?
> 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.


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

I saw an additional icon displays on the folder pane on Linux platform ONLY, is
that icon comes from this bug fix?
Unless this was checked in without notice here (which would be bad...), it isn't
this - unless you're running this patch locally.
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.

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?
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.
> 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.
> 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.
fixed
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This happens on Windows as well with a new profile!
> We definitely verified that backing out the JS changes in this bug fixed it.

which js changes did you back out?
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.
Should we close this back out and keep discussions in the new bug (136874)
yes, let's close this and take this to the other bug.

if we end up backing it out, we'll reopen.
Status: REOPENED → RESOLVED
Closed: 22 years ago → 22 years ago
Resolution: --- → FIXED
for completeness, bug 136874 was fixed.
Keywords: adt1.0.0+ → fixed1.0.0
When Esther was testing bug# 135594 she also checked this bug and it appears to
be fixed.
Status: RESOLVED → VERIFIED
removing fixed 1.0.0, this was tested with branch builds on winme, winxp, mac
os9.1, mac x and linux.
Keywords: fixed1.0.0
adding branch verification keyword.  This bug has been verified in a comment,
just moving that to the keyword field.
Keywords: verified1.0.0
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: