486288 - Mailnews idl's comments does not adhere JavaDoc-style comments

Status: NEW

(MailNews Core :: Backend, defect)

Description

[palo misik]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.8) Gecko/2009032609 Firefox/3.0.8 (.NET CLR 3.5.30729) XPCOMViewer/1.0a1
Build Identifier: 

https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide#section_1

Because comments are wrong "xpidl -m header" doesn't copies comment to generated header.

Reproducible: Always

Comment 1

[Mark Banner (:standard8)]

I expect many of the functions in mailnews idl are incorrectly formatted, I'd like to suggest we have just one bug to fix them, rather than many.

Comment 2

[palo misik]

Attachment: [checked in] Fixed comments style

Please add missing parameters description.
I can guess what they mean. It is better when author specifies its meaning.
What is XXXbz ?
I didn't change interface definition ( I hope) therefore it isn't necessary to change IID.

Comment 3

[palo misik]

>I expect many of the functions in mailnews idl are incorrectly formatted, I'd
>like to suggest we have just one bug to fix them, rather than many.
I think it is simpler to review every interface separately.
There is a lot of missing comments about parameters. 

At the beginning I can prepare patch for mailnews\base\public\,
but I think it take a long time to clarify all parameters.

Comment 4

[Mark Banner (:standard8)]

(In reply to comment #3)
> >I expect many of the functions in mailnews idl are incorrectly formatted, I'd
> >like to suggest we have just one bug to fix them, rather than many.
> I think it is simpler to review every interface separately.
> There is a lot of missing comments about parameters. 
> 
> At the beginning I can prepare patch for mailnews\base\public\,
> but I think it take a long time to clarify all parameters.

It may be worth one bug per an area, e.g. mailnews\base\public, mailnews\compose\public etc. We generally keep bugs to one bug per issue, but it doesn't mean that you can't have multiple patches for one bug.

Note I wouldn't expect you to document everything, I was only suggesting that if you want to go through fixing the existing documentation, you do it in one bug rather than twenty bugs.

We have lots of missing documentation, and we've been adding to it as we change interfaces - rather than taking the hit of a mass documentation drive (which would take a long time and not actually provide much overall gain).

Comment 5

[palo misik]

>It may be worth one bug per an area, e.g. mailnews\base\public,
>mailnews\compose\public etc. We generally keep bugs to one bug per issue, but
>it doesn't mean that you can't have multiple patches for one bug.
Ok.

>Note I wouldn't expect you to document everything, I was only suggesting that
>if you want to go through fixing the existing documentation, you do it in one
>bug rather than twenty bugs.
OK

>We have lots of missing documentation, and we've been adding to it as we change
>interfaces - rather than taking the hit of a mass documentation drive (which
>would take a long time and not actually provide much overall gain).
With this approach can happened that interface stay undocumented forever if it stay untouched.  

Well I have a tool which helps to generate comments in java style. 
I can attach all patches to this bug.
There is a problem that English isn't my first language therefore they will need correct and reviews.

>not actually provide much overall gain
I disagree.
An extension developers and new mozilla developers it is essential source of documentation.

Comment 6

[David :Bienvenu]

XXXbz is a comment from bzBarsky, aka bz.

Right, you don't need to change the IID.

Comment 7

[David :Bienvenu]

Comment on attachment 370383 [details] [diff] [review]
[checked in] Fixed comments style 

thx for the patch. It's certainly better than what we had before. I'll try to land this soon.

Comment 8

[Mark Banner (:standard8)]

Palo, thanks for doing this.

Checked in: http://hg.mozilla.org/comm-central/rev/15bbf0178c8e

Comment 9

[palo misik]

Attachment: Added documentation for nsIMsgCopyService

added documentation based on discussion
from mozilla.dev.apps.thunderbird
Subject: nsIMsgCopyService::CopyFileMessage arguments meaning 

+ added documentation for remaining methods
+ fixed document style

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 378542 [details] [diff] [review]
Added documentation for nsIMsgCopyService

> [scriptable, uuid(f21e428b-73c5-4607-993b-d37325b33722)]
> interface nsIMsgCopyService : nsISupports {
> 
>     /**
>-     *
>+     * It copies or moves existing messages from source folder to destination folder.

To start the description with 'It' sounds a bit strange in english. I'd suggest just "Copies or moves existing messages...".

>+     * @param srcFolder Source folder of an operation.
>+     * @param messages The array (of nsIMsgHdrs) of messages from source folder which 

Not sure which character you are using there but I think you can just drop it and the 's', i.e. just make the bit in brackets (of nsIMsgHdr).

>+     *                 will operation be done with.

>+     * @param dstFolder Destination folder of operation.
>+     * @param isMove false for copy operation, true for move operation.
>+     * @param listener Listener which receive operation notifications
>+     * @param msgWindow Window for notification callbacks, can be null.
>+     * @param allowUndo Specifies if this operation will be done as an transaction 
>+     *                  that can be undone.

Can you align the descriptions please? Having them started in different places makes it really hard to see where the param ends and the description starts. Here's an example which is easier to read:

  * @param srcFolder  Source folder of an operation.
  * @param messages   The array (of nsIMsgHdrs) of messages from source folder which 
  *                   will operation be done with.
  * @param dstFolder  Destination folder of operation.
  * @param isMove     false for copy operation, true for move operation.
  * @param listener   Listener which receive operation notifications
  * @param msgWindow  Window for notification callbacks, can be null.
  * @param allowUndo  Specifies if this operation will be done as an transaction 
  *                   that can be undone.

In this case I've gone for a minimum of two spaces between parameter and description as there are a few all the same length, it just gives a slightly better visual break.

This applies to the other functions as well.

>+    /**
>+     * It copies message in rfc format from file to folder. 
>+     * 
>+     * @param aFile A file which contains message in rfc format which will 
>+     *              copied to destFolder
>+     * @param dstFolder Destination folder where a message will be copied.
>+     * @param msgToReplace Header which identifies message which will be replaced by newly 
>+     *                     coppied message or null. It is used generally for drafts, when 

nit: only a single p in copied.

In general looking good, but I'd like to see it again with the formatting updated.

Comment 11

[palo misik]

Attachment: Added documentation for nsIMsgCopyService

formatting updated

Comment 12

[David :Bienvenu]

+   * @param messages   The array (of nsIMsgHdr�s) of messages from source folder which 
+   *                   will operation be done with.
+

there's an odd character in nsIMsgHdrs

"which will operation be done with" (repeated a few times) isn't correct grammar (I realize English isn't your first language).  Perhaps "The array of nsIMsgHdrs in source folder which will be moved/copied."

Comment 13

[palo misik]

Attachment: Documentation for nsIMsgCopyService

fixed description 
odd character removed

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 379522 [details] [diff] [review]
Documentation for nsIMsgCopyService   

Looking good.

>+  /**
>+   * Copies or moves existing messages from source folder to destination folder.
>+   * 

There's a few lines (such as the last one above) where you have one or more spaces after the last printable character. Please remove those.

r=Standard8 with those fixed.

Comment 15

[palo misik]

Attachment: [checked in] Documentation for nsIMsgCopyService

removed spaces after the last printable character of line

Comment 16

[palo misik]

Comment on attachment 380042 [details] [diff] [review]
[checked in] Documentation for nsIMsgCopyService

I'm not sure if I have to request review. For sureness I request it again.

Comment 17

[Mark Banner (:standard8)]

(In reply to comment #16)
> (From update of attachment 380042 [details] [diff] [review])
> I'm not sure if I have to request review. For sureness I request it again.

Generally if you're given r+ (or sr+) as long as you just do any changes requested and don't do substantial changes, then you don't need to re-request review.

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 380042 [details] [diff] [review]
[checked in] Documentation for nsIMsgCopyService

For some reason, this didn't apply to my build. So I applied it by hand. At the same time I noticed a few of the line lengths were a bit long (I'd missed that in the review), so I reformatted those, and I re-indented the functions to 2-space as we're touching virtually the whole file and the comments are 2-space as well.

Thanks for doing the patch.

http://hg.mozilla.org/comm-central/rev/2457fbd6cc1f

Comment 19

[palo misik]

Attachment: Proposed patch for nsIDBChangeListener.idl

Comment 20

[palo misik]

Attachment: Proposed patch for nsIMsgAccount.idl

Comment 21

[palo misik]

Comment on attachment 386683 [details] [diff] [review]
Proposed patch for nsIMsgAccount.idl

wrong file

Comment 22

[palo misik]

Attachment: Proposed patch for nsIMsgAccount.idl

Comment 23

[palo misik]

Attachment: Proposed patch for nsIUrlListener.idl

Comment 24

[Mark Banner (:standard8)]

Comment on attachment 386685 [details] [diff] [review]
Proposed patch for nsIUrlListener.idl

>+/** General interface that signify URL processing. */      

For single line comments we generally use /// (which is doxygen compatible and I would expect javadoc, though I haven't checked).

Note you have unnecessary whitespace on the end of this line as well, please remove that.

> [scriptable, uuid(47618220-D008-11d2-8069-006008128C4E)]
> interface nsIUrlListener : nsISupports {
>+    /**
>+     * Called to signify the beginning of an URL processing.
>+     * It doesn't have to be called.
>+     *
>+     * @param url URL being processed.
>+     */
>+    void OnStartRunningUrl(in nsIURI url);
> 
>-    void OnStartRunningUrl(in nsIURI url);
>+    /**
>+     * Called to signify the end of an URL processing.
>+     * It doesn't have to be called.
>+     *
>+     * @param url        URL being processed.
>+     * @param aExitCode  A result code of URL processing.
>+     */
>     void OnStopRunningUrl(in nsIURI url, in nsresult aExitCode);

I'm a little surprised at the assertions that it doesn't have to be called. In many instances we must call OnStopRunningUrl so that we stop the throbbers, I'd therefore expect the same with the start function. Have you seen cases where this isn't the case?

Comment 25

[palo misik]

Attachment: [checked in] Proposed documentation patch for nsIUrlListener.idl

>For single line comments we generally use /// (which is doxygen compatible and
>I would expect javadoc, though I haven't checked).
But xpidl.exe doesn't include single line comments to header file, but I changed it. I should report it as bug in xpidl.

>Note you have unnecessary whitespace on the end of this line as well, please
>remove that.
Fixed

>I'm a little surprised at the assertions that it doesn't have to be called. In
>many instances we must call OnStopRunningUrl so that we stop the throbbers, I'd
>therefore expect the same with the start function. Have you seen cases where
>this isn't the case?
I encountered with it in Tb 2. It was during xpcom-shutdown. But i'm not sure which class caused it. So I removed that assertion. And I added "This call is always preceded by a call to OnStartRunningUrl."

Comment 26

[David :Bienvenu]

Comment on attachment 385324 [details] [diff] [review]
Proposed patch for nsIDBChangeListener.idl

r=me, with one nit:

good place to release weak pointers to announcer.

should be good place to release strong pointers

Comment 27

[palo misik]

Attachment: [checked in] Proposed documentation patch for nsIDBChangeListener.idl

Obsoletes  385324: Proposed patch for nsIDBChangeListener.idl which has review grant with nit.

Fixed nit: "good place to release weak pointers to announcer"
to 
"good place to release strong pointers"

Comment 28

[palo misik]

checkin-needed for patch: https://bug486288.bugzilla.mozilla.org/attachment.cgi?id=386943
( Proposed documentation patch for nsIDBChangeListener.idl )

Comment 29

[Mark Banner (:standard8)]

Comment on attachment 386716 [details] [diff] [review]
[checked in] Proposed documentation patch for nsIUrlListener.idl

Thanks for this patch r/sr=me.

Comment 30

[Mark Banner (:standard8)]

Comment on attachment 386943 [details] [diff] [review]
[checked in] Proposed documentation patch for nsIDBChangeListener.idl

Not sure what we're doing about reviews on these comment only-changes, but sr=Standard8 anyway.

Comment 31

[Mark Banner (:standard8)]

Comment on attachment 386716 [details] [diff] [review]
[checked in] Proposed documentation patch for nsIUrlListener.idl

Checked in: http://hg.mozilla.org/comm-central/rev/8b4a8078e1c1

Comment 32

[Mark Banner (:standard8)]

Comment on attachment 386943 [details] [diff] [review]
[checked in] Proposed documentation patch for nsIDBChangeListener.idl

Checked in (same changeset as previous patch): http://hg.mozilla.org/comm-central/rev/8b4a8078e1c1

Comment 33

[palo misik]

I have one bad information about issue with single line comments in idl.
Benjamin says it isn't worthwhile to add this to xpidl (https://bugzilla.mozilla.org/show_bug.cgi?id=504743).

Would it be better to use /** ... */ instead of /// ?

Comment 34

[Mark Banner (:standard8)]

Comment on attachment 386684 [details] [diff] [review]
Proposed patch for nsIMsgAccount.idl

>+/**
>+ * An account consists of an incoming server and one or more
>+ * outgoing identities. An account is identitfied by a key,

nit: identified

>-  /* internal key identifying itself */
>+  /** Internal key identifying itself */

nit: I know you created this patch at the same time as the last one, please update to use /// instead for the single line comments.

>-  /* add a new identity to this account (probably does not work) */
>+  /** Add a new identity to this account (probably does not work) */
>   void addIdentity(in nsIMsgIdentity identity);
> 
>-  /* remove an identity from this account (probably does not work) */
>+  /** Remove an identity from this account (probably does not work) */
>   void removeIdentity(in nsIMsgIdentity identity);

Whilst you're just fixing up comments here I'd like David to comment on this (hence requesting sr from him) as I'm not sure the "(probably does not work)" comment really applies.

r=Standard8 with the nits fixed and whatever David requires with respect to that comment.

Comment 35

[Mark Banner (:standard8)]

(In reply to comment #33)
> I have one bad information about issue with single line comments in idl.
> Benjamin says it isn't worthwhile to add this to xpidl
> (https://bugzilla.mozilla.org/show_bug.cgi?id=504743).
> 
> Would it be better to use /** ... */ instead of /// ?

The issue I have with /** ... */ on a single line is that I fine it much harder to read than ///. It is also generally a pain if you want to block comment out lines in a file.

What is your use case for requiring comments in the generated header?

Comment 36

[palo misik]

>The issue I have with /** ... */ on a single line is that I fine it much harder
>to read than ///. It is also generally a pain if you want to block comment out
>lines in a file.

I take a look at comments in idl of Mozilla and there usually is used following pattern for single line comments in idl:

/**
 * Indicates a request by a plugin.
 */
const unsigned long TYPE_OBJECT_SUBREQUEST = 12;

Mailnews uses /// for single line comments. Is there serious reason why should mailnews idls differ from other? As you are owner of mailnews this decision is up to you. 


>What is your use case for requiring comments in the generated header?
It saves time to find out documentation of interface when I use feature of editor go to declaration/implementation. I don’t have to find out idl file and there corresponding declaration. Other advantage is that intellisence can show you information about parameters or Visual Assist can parse this information and help you.

Comment 37

[David :Bienvenu]

Comment on attachment 386684 [details] [diff] [review]
Proposed patch for nsIMsgAccount.idl

sr=me, if you remove the "probably doesn't work" parts. We call that code when removing an account, and afaik, it does work. The code looks to me like it cleans up the identities.

Comment 38

[David :Bienvenu]

Comment on attachment 386684 [details] [diff] [review]
Proposed patch for nsIMsgAccount.idl

marking sr.

Comment 39

[Joshua Cranmer [:jcranmer]]

(In reply to comment #36)
> Mailnews uses /// for single line comments. Is there serious reason why should
> mailnews idls differ from other? As you are owner of mailnews this decision is
> up to you.

This feature of doxygen seems to have only recently been rediscovered.

> >What is your use case for requiring comments in the generated header?
> It saves time to find out documentation of interface when I use feature of
> editor go to declaration/implementation. I don’t have to find out idl file and
> there corresponding declaration. Other advantage is that intellisence can show
> you information about parameters or Visual Assist can parse this information
> and help you.

It sounds as if you are using Visual Studio or a derived tool thereof, which (to my recollection) can parse IDL files to a degree instead of relying on the header files.

Comment 40

[palo misik]

My question about new single line comments '///' for (:standard8, :jcranmer and :Bienvenu ):
Do you want to use  /// instead of /**   */ however Mozilla coding style recommends JavaDoc-style comments(https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide)?

I didn't find /// in JavaDoc-style.

Comment 41

[Joshua Cranmer [:jcranmer]]

(In reply to comment #40)
> I didn't find /// in JavaDoc-style.

We've been following the Doxygen style more, as that's the tool that actually generates the documentation:
http://www.stack.nl/~dimitri/doxygen/manual.html

Comment 42

[palo misik]

Attachment: [checked in] Proposed patch for nsIMsgAccount.idl

Changes required in Comment #34 and  Comment #37.
Check in needed.

Comment 43

[Mark Banner (:standard8)]

Comment on attachment 390413 [details] [diff] [review]
[checked in] Proposed patch for nsIMsgAccount.idl

Checked in: http://hg.mozilla.org/comm-central/rev/74c5fac90cbc

Comment 44

[palo misik]

Attachment: [checked in] Proposed documentation patch for nsIIncomingServerListener.idl

Comment 45

[palo misik]

Attachment: Documentation for nsIMsgAccountManager

Comment 46

[Mark Banner (:standard8)]

Comment on attachment 391302 [details] [diff] [review]
[checked in] Proposed documentation patch for nsIIncomingServerListener.idl

Thanks for this, I've checked it in as well:

http://hg.mozilla.org/comm-central/rev/4d8c06f9a242

Comment 47

[Mark Banner (:standard8)]

Comment on attachment 391309 [details] [diff] [review]
Documentation for nsIMsgAccountManager

Looking good, I think some of the functions don't always do exactly what you may expect e.g. I've a feeling createAccount doesn't add the account to the list until you do something with it. However David should know better on that front, so requesting review from him as well.

>+  /**
>+   * Removes an account from account manager. It also remove associated
>+   * identities if no other account uses it.

nit: "uses them" rather than "uses it".

>+
>+  /// DO NOTHING
>   void removeIdentity(in nsIMsgIdentity identity);
>+
>+  /**
>+   * DO NOTHING
>+   * @throw NS_ERROR_NOT_IMPLEMENTED
>+   */
>   void duplicateAccount(in nsIMsgAccount account);

Can you file a bug to remove these please? I think given that the implementations are null, there isn't much point in having these as it just confuses matters. Although I'm not sure why we don't implement removeIdentity, perhaps we do that elsewhere.

>+   * Creates a new server and assigns it a new, unique "key"
>+   * the given type will be used to construct a ContractID.
>    *
>-   *  @param type "imap", "pop3", "nntp", "movemail", "none", "rss", "generic"
>-   * (suffix of contract ID @mozilla.org/messenger/server;1?type= )
>+   * @param username User name to log on the server.
>+   * @param hostname Host name of server.
>+   * @param type     Type of server. This parameter is suffix of contract ID
>+   *                 @mozilla.org/messenger/server;1?type=
>+   *                 There are available following types: "imap", "pop3", "nntp",
>+   *                 "movemail", "none", "rss", "generic".

Can you change this introduction of the types to be "The following types are available in core: "

>+  /// Load a virtual folders ???
>   void loadVirtualFolders();

This probably need similar documentation to LoadAccounts, but hopefully David can expand on it.

Comment 48

[Joshua Cranmer [:jcranmer]]

Comment on attachment 391309 [details] [diff] [review]
Documentation for nsIMsgAccountManager

Some comments from someone who uses documentation:

>+  /**
>+   * Creates a new account with a unique account key.
>+   *
>+   * @return Created account.
>+   */
>   nsIMsgAccount createAccount();

I'm quite sure this doesn't actually add the account to the account manager (but it's been a year since I last played around with the code), so could you verify that and add it to the comment?

>+  /**
>+   * Creates a new identity and assigns it a new, unique "key".
>+   *
>+   * @return Created identity.
>    */
>   nsIMsgIdentity createIdentity();

Ditto for this method.

>+   * @param type     Type of server. This parameter is suffix of contract ID
>+   *                 @mozilla.org/messenger/server;1?type=
>+   *                 There are available following types: "imap", "pop3", "nntp",
>+   *                 "movemail", "none", "rss", "generic".

I think, given the plans (AIU them, anyways) for future TB core work, that this list will cease to be exhaustive or terribly useful. At the very least, mention that these are the servers implemented by core Mozilla implementations, if not drop it altogether.

>   /**
>    * Ordered list of all accounts, by the order they are in the prefs.
>    * Accounts with hidden servers are not returned.
>    * array of nsIMsgAccount
>    */
>   readonly attribute nsISupportsArray accounts;

I find "the prefs" to be a very ambiguous term. I believe the correct order derives from the order of the master account pref (the one that lists all the accounts).

>-  /* summary of summary files folder cache */
>+  ///  Summary of summary files folder cache.
>   readonly attribute nsIMsgFolderCache folderCache;

I would prefer a more descriptive summary here. The folder cache represents cached data from the folder databases to avoid having to open them up and possibly reparse entire mboxes on startup.

>   /**
>-   * for preventing unauthenticated users from seeing header information 
>+   * For preventing unauthenticated users from seeing header information.
>+   * This option is stored in preferences mail.password_protect_local_cache.
>    */
>   attribute boolean userNeedsToAuthenticate;

I would prefer something a little more normal for the first line, more along the lines of "Represents whether or not we need to authenticate before displaying the headers."

>+   * @param type     Type of server. The type is the same as is specified
>+   *                 in the preferences.  ("imap", "pop3", "nntp", "movemail",
>+   *                 "none", "rss", "generic")

See above comment.

>+   * @return Index of registered server or an index higher than all "registered"
>+   *         servers if server isn't "registered".

"registered" ?

>+  /**
>+   * Search account for given server.

Please insert `the' here (x several other places).

>+  /**
>+   * Return all identities in accounts that have given server.

To my knowledge, only one account can have any given nsIMsgIncomingServer.

>+  /**
>+   * There is a special server "Local Folders" that is guaranteed to exist.
>+   * This will allow you to get.
>+   */
>   attribute nsIMsgIncomingServer localFoldersServer;

Please reformat this comment, something like "The server that represents local storage of mail".

>-  // Create the account for that special server.
>+  /// Create the account for that special server.
>   void createLocalMailAccount();

And explicitly provide the antecedent of `that'. You won't necessarily see this line immediately following the previous in generated documentation.

>+  /**
>+   * Write out all known folder data to 'panacea.dat' for every server.
>+   *
>+   * @param folderCache
>+   */
>   void WriteToFolderCache(in nsIMsgFolderCache folderCache);
>+
>+  /**
>+   * Saves all virtual folders to file "virtualFolders.dat" in
>+   * NS_APP_USER_PROFILE_50_DIR  directory.
>+   */
>   void saveVirtualFolders();

Please don't refer to the filenames specifically here, especially since panacea.dat will probably be changing its name before long.

>+  ///  examples:  mdn
>+  readonly attribute ACString name;

Please actually document these attributes instead of keeping them empty.

Also, could I kindly request that you provide some documentation on the interfaces themselves as well?

Comment 49

[palo misik]

Attachment: Updated documentation for nsIMsgAccountManager

Updated documentation by Comment #47 and Comment #48.
I also added documentation for nsIMsgAccountManagerExtension but I'm not sure if it is right.

Comment 50

[palo misik]

>>+  /**
>>+   * Creates a new account with a unique account key.
>>+   *
>>+   * @return Created account.
>>+   */
>>   nsIMsgAccount createAccount();
>I'm quite sure this doesn't actually add the account to the account manager
>(but it's been a year since I last played around with the code), so could you
>verify that and add it to the comment?
I took a look into implementation. An account is appended to m_accounts array.
So I would expect the account is added to account manager

>>   nsIMsgIdentity createIdentity();
>Ditto for this method.
Created identity is put into m_identities.

>>   /**
>>    * Ordered list of all accounts, by the order they are in the prefs.
>>    * Accounts with hidden servers are not returned.
>>    * array of nsIMsgAccount
>>    */
>>   readonly attribute nsISupportsArray accounts;
>I find "the prefs" to be a very ambiguous term. I believe the correct order
>derives from the order of the master account pref (the one that lists all the
>accounts).

I think accounts are loaded by the order specified in pref
mail.accountmanager.accounts. Is it right?

Comment 51

[Mark Banner (:standard8)]

Comment on attachment 392426 [details] [diff] [review]
Updated documentation for nsIMsgAccountManager

Sorry for the delay, I'm going to pass this to Joshua as he is more up on the account manager.

Comment 52

[Joshua Cranmer [:jcranmer]]

Comment on attachment 392426 [details] [diff] [review]
Updated documentation for nsIMsgAccountManager

>+  /**
>+   * Creates a new account with a unique account key.
>+   *
>+   * @return Created account.
>+   */
>   nsIMsgAccount createAccount();

Mention that it also adds the account to the list of accounts.

>+  /**
>+   * Removes an account from account manager. It also remove associated
>+   * identities if no other account uses them.
>+   *
>+   * @param account Account to remove.
>+   */
>   void removeAccount(in nsIMsgAccount account);

Nit: "It also removes"

>+
>+  /// DO NOTHING
>   void removeIdentity(in nsIMsgIdentity identity);
>+
>+  /**
>+   * DO NOTHING
>+   * @throw NS_ERROR_NOT_IMPLEMENTED
>+   */
>   void duplicateAccount(in nsIMsgAccount account);

If they do nothing, it would be better to remove them and their implementations.

>+  /**
>+   * Creates a new identity and assigns it a new, unique "key".
>+   *
>+   * @return Created identity.
>    */
>   nsIMsgIdentity createIdentity();

Again, please mention that this adds it to the list of identities.

>+   * @param type     Type of server. This parameter is suffix of contract ID
>+   *                 @mozilla.org/messenger/server;1?type=
>+   *                 The following types are available in core:
>+   *                 "imap", "pop3", "nntp", "movemail", "none", "rss", "generic".

I'm hesitant to include an exhaustive list here, as we might change these without updating the list.

>+  /**
>+   * Get the identity with the given key.
>+   * If the identity does not exist, it will be created.
>+   *
>+   * @param key Key of required identity.
>+   *
>+   * @return Created or existing identity.
>    */
>   nsIMsgIdentity getIdentity(in ACString key);

"Gets" or "Retrieves" would be better.

>+  /**
>+   * Default account. It should always be set if there are any accounts
>    * in the account manager. You can only set the defaultAccount to an
>-   * account already in the account manager */
>+   * account already in the account manager.
>+   */
>   attribute nsIMsgAccount defaultAccount;

"The default account," please. The second part is also somewhat misleading as to what the attribute returns: accountManager.defaultAccount will always return *some* account as long as one exists.

>   /**
>    * Ordered list of all accounts, by the order they are in the prefs.
>    * Accounts with hidden servers are not returned.
>    * array of nsIMsgAccount
>    */
>   readonly attribute nsISupportsArray accounts;

Nit: "An ordered list" (similar for many other attributes).

>+  /**
>+   * List of all identities in all accounts
>    * array of nsIMsgIdentity
>    */
>   readonly attribute nsISupportsArray allIdentities;

Nit: Add a period to the end of the sentence.

>+  /** 
>+   * The folder cache represents cached data from the folder databases 
>+   * to avoid having to open them up and possibly reparse entire mboxes 
>+   * on startup.
>+   */
>   readonly attribute nsIMsgFolderCache folderCache;

Please make the first sentence "The object representing cached folder information" and separate the rest into a separate paragraph.

>+  /// Flag that indicate if we are shutting down.
>   readonly attribute boolean shutdownInProgress;

Nit: `indicates'

>   /**
>-   * for preventing unauthenticated users from seeing header information 
>+   * Represents whether or not we need to authenticate before
>+   * displaying the headers.
>+   * This option is stored in preferences mail.password_protect_local_cache.
>    */
>   attribute boolean userNeedsToAuthenticate;

Nit: "preference", not "preferences"

>+
>+  /**
>+   * Search for the server with the given username, hostname, and type.
>+   *
>+   * @param userName User name.
>+   * @param hostname Server host name.
>+   * @param type     Type of server. The type is the same as is specified
>+   *                 in the preferences.  ("imap", "pop3", "nntp", "movemail",
>+   *                 "none", "rss", "generic")
>+   *
>+   * @return Required server if exists.
>+   * @throws NS_ERROR_UNEXPECTED if server doesn't exist.
>    */
>   nsIMsgIncomingServer
>       FindServer(in ACString userName, in ACString hostname, in ACString type);

"Searches" here, and you can omit the list of server types.

>+  /**
>+   * Search for the server with the given uri an analog to FindServer.
>+   *
>+   * @param aURI       uri to compare.
>+   * @param aRealFlag  Selects whether we compare input against the 'realhostname'
>+   *                   and 'realuserName' pref settings or 'hostname' and 'username'.
>+   *                   If is true we comapre against realHostName and realHostName.
>+   *
>+   * @return Required server if exists.
>+   * @throws NS_ERROR_UNEXPECTED if server doesn't exist.
>    */
>   nsIMsgIncomingServer
>       findServerByURI(in nsIURI aURI, in boolean aRealFlag);

"Searches for the server with the given URI" here, and add a @see FindServer to the bottom.

Similarly for findRealServer.

>   /**
>-   * find the index of this server in the (ordered) list of accounts
>+   * Find the index of this server in the (ordered) list of accounts.
>+   *
>+   * @param server Server we want to find.
>+   *
>+   * @return Index of registered server or an index higher than all "registered"
>+   *         servers if server isn't "registered".
>    */
>   long FindServerIndex(in nsIMsgIncomingServer server);

Nit: "Finds" (similarly for the next several methods)

>+
>+  /**
>+   * Load accounts kicks off the creation of all accounts. You do not need
>    * to call this and all accounts should be loaded lazily if you use any
>    * of the above.
>    */
>   void LoadAccounts();

Please change the first sentence to something along the lines of "Loads all accounts into memory."

>+  /**
>+   * This routine goes through all the identities and makes sure
>+   * that the special folders for each identity have the
>+   * correct special folder flags set, e.g, the Sent folder has
>+   * the sent flag set, Draft folder has the drafts flag set.
>+   */
>   void setSpecialFolders();

Change the first line to "Ensures that the special folders..."

>+  /// Load a virtual folders from file. 
>   void loadVirtualFolders();

"Loads the virtual folders into the accounts"

>-  /* unload accounts frees all the account manager data structures */
>+  /// Unload accounts frees all the account manager data structures.
>   void UnloadAccounts();

"Frees all internal data structures before shutdown."

>+  /**
>+   * Write out all known folder data to file for every server.
>+   *
>+   * @param folderCache
>+   */
>   void WriteToFolderCache(in nsIMsgFolderCache folderCache);

"Write all folders to the folder cache", perhaps with a link to the folderCache attribute above.

>+  /**
>+   * Saves all virtual folders to file "virtualFolders.dat" in
>+   * NS_APP_USER_PROFILE_50_DIR  directory.
>+   */
>   void saveVirtualFolders();

Nit: "the profile" instead of "NS_APP_USER_PROFILE_50_DIR".

>+  /// Call Shutdown for loaded servers.
>   void shutdownServers();

Nit: "Calls"

>+  /**
>+   * Executes cleanup actions on exit for every loaded account.
>+   * It empties a trash, inbox depending on an account preferences.
>+   */
>   void CleanupOnExit();

Nit: "empties the trash or compacts the inbox" (It doesn't empty the inbox, which would be a rather alarming action to take on shutdown).

>+  /**
>+   * Sets folder that is emptying Trash.
>+   *
>+   * @param folder Given folder.
>+   */
>   void SetFolderDoingEmptyTrash(in nsIMsgFolder folder);

"Sets the trash folder being emptied" (similar set up for next three).

>+  /**
>+   * Registers folder listener for root folders.
>+   *
>+   * @param listener Listener to register.
>+   */
>   void addRootFolderListener(in nsIFolderListener listener);

Nit: "the folder listener" (ditto for next method)

>+  /**
>+   * Registers server listeners.
>+   *
>+   * @param serverListener Listener to register.
>+   */
>   void addIncomingServerListener(in nsIIncomingServerListener serverListener);

"Registers the server listener to all incoming servers", similar for next.

>+  /**
>+   * Registers server listeners.
>+   *
>+   * @param serverListener Listener to register.
>+   */
>   void removeIncomingServerListener(in nsIIncomingServerListener serverListener);

Excessive copy-paste?

>+  /**
>+   * @{
>+   * Notifies all registered server listeners that server
>+   * is loaded, unloaded of changed.
>+   *
>+   * @param server Impacted server.
>+   */
>   void notifyServerLoaded(in nsIMsgIncomingServer server);
>   void notifyServerUnloaded(in nsIMsgIncomingServer server);
>   void notifyServerChanged(in nsIMsgIncomingServer server);
>+  /** @} */

Nit: "or", and please add the comma before the conjunction.

>-  // force account info out to prefs file
>+  /// Force account info out to prefs file.
>   void saveAccountInfo();

"Flushes preference changes to disk."

>+  /**
>+   * Search for package name of given extension name.
>+   *
>+   * @param aExtensionName Given extension name.
>+   *
>+   * @return Chrome package name.
>+   * @throws NS_ERROR_UNEXPECTED if not found
>+   */
>   ACString getChromePackageName(in ACString aExtensionName);

Please mention somewhere that the extension name is the part between the `-' and the `.' in strings like `am-mdn.xul', and also cross-reference nsIMsgAccountManagerExtension.

>+/** 
>+ * This interface is used to find out wheter extension for
>+ * the account manager should be displayed or not.
>+ */

Nit: "whether extensions"

You also probably want to mention how to make the extensions discoverable to the account manager.

> [scriptable, uuid(70032DE0-CD59-41ba-839D-FC1B65367EE7)]
> interface nsIMsgAccountManagerExtension : nsISupports
> {

>+  /** 
>+   * Name of the extension.
>+   * examples:  mdn
>+   */
>+  readonly attribute ACString name;

The name, if I am not mistaken, is the middle part of the string names like `am-mdn.xul'.

>+  /**
>+   * Checks whether display pannel for extension or not for given the server.
>+   *
>+   * @param server Given the server.
>+   *
>+   * @return true if panel shoul be displayed for iven the server.
>+   */
>   boolean showPanel(in nsIMsgIncomingServer server);

"Checks whether the display panel for the extension should be shown for the given server."
Similar spelling mistakes on the last line should be fixed as well.

>+  /**
>+   * Chrome package name.
>+   * @example:  messenger, chrome://messenger/content/am-mdn.xul and chrome://messenger/locale/am-mdn.properties
>+   */
>+  readonly attribute ACString chromePackageName;

"The package where the extension can be found.

For example, for the URI <tt>chrome://messenger/content/am-mdn.xul</tt>, this should return <tt>messenger</tt>."

Comment 53

[Joshua Cranmer [:jcranmer]]

Oh, and please provide documentation for the interface itself, preferably including the contract id as well.

Comment 54

[palo misik]

Attachment: Updated documentation for nsIMsgAccountManager

>>   void duplicateAccount(in nsIMsgAccount account);
>If they do nothing, it would be better to remove them and their
>implementations.
>>   nsIMsgIdentity createIdentity();
>Again, please mention that this adds it to the list of identities.
Done in  Bug 508200.

>>+   * @param type     Type of server. This parameter is suffix of contract ID
>>+   *                 @mozilla.org/messenger/server;1?type=
>>+   *                 The following types are available in core:
>>+   *                 "imap", "pop3", "nntp", "movemail", "none", "rss", "generic".
>
>I'm hesitant to include an exhaustive list here, as we might change these
>without updating the list.
I think it is better to have it here however it will be incomplete. A developer will know what to search.

>>+  /**
>>+   * Executes cleanup actions on exit for every loaded account.
>>+   * It empties a trash, inbox depending on an account preferences.
>>+   */
>>   void CleanupOnExit();
> It doesn't empty the inbox, which would be a rather alarming action to take on shutdown.
I think it does expunge on exit on imap server so messages marked for delete will be deleted in inbox.

>>+/** 
>>+ * This interface is used to find out wheter extension for
>>+ * the account manager should be displayed or not.
>>+ */
>You also probably want to mention how to make the extensions discoverable to
the account manager.
I have no idea. Is it documented somewhere?

There is comment "// these are going away in favor of add/removeRootFolderListener" in original idl for method addIncomingServerListener.

There is also "// these are going away in favor of nsIMsgFolder::NotifyEvent(in nsIAtom event); " for notifyServer*.

What to do with them?

Comment 55

[palo misik]

Comment on attachment 396661 [details] [diff] [review]
Updated documentation for nsIMsgAccountManager

I have no idea why diff included header into patch file. I created patch by:
hg diff -p -U 8 nsIMsgAccountManager.idl > nsIMsgAccountManager.patch

Comment 56

[Joshua Cranmer [:jcranmer]]

Comment on attachment 396661 [details] [diff] [review]
Updated documentation for nsIMsgAccountManager

>diff --git a/mailnews/base/public/nsIMsgAccountManager.idl b/mailnews/base/public/nsIMsgAccountManager.idl
>--- a/mailnews/base/public/nsIMsgAccountManager.idl
>+++ b/mailnews/base/public/nsIMsgAccountManager.idl

This seems to touch every line? Can you change this?

>+  /**
>+   * Creates a new server and assigns it a new, unique "key"
>+   * the given type will be used to construct a ContractID.
>+   *
>+   * @param username User name to log on the server.
>+   * @param hostname Host name of server.
>+   * @param type     Type of server. This parameter is suffix of contract ID
>+   *                 @mozilla.org/messenger/server;1?type=
>+   *                 The following types are available in core:
>+   *                 "imap", "pop3", "nntp", "movemail", "none", "rss", "generic".

As mentioned earlier, I'd rather not have an exhaustive list, since we almost surely won't keep this in sync. Either remove it or indicate that this is may only be a partial list.

>+  /**
>+   * The default account. This attribute will always return *some* account
>+   * as long as one exists. You can only set the defaultAccount to an account
>+   * that is already in the account manager.
>+   */
>+  attribute nsIMsgAccount defaultAccount;
Nit: remove the emphasis on some, or perhaps wrap it in <i>; * is only useful in plain text contexts.

>+  /**
>+   * An ordered list of all accounts, by the order they are in the prefs.
>+   * Accounts with hidden servers are not returned.
>+   * array of nsIMsgAccount

Nit: make the last line into a sentence (ugh at the specification of arrays in IDL)
[Applies to several subsequent comments too]

>+  /**
>+   * Represents whether or not we need to authenticate before
>+   * displaying the headers.
>+   * This option is stored in preference mail.password_protect_local_cache.
>+   */
Nit: "the preference"

>+   * @param type     Type of server. The type is the same as is specified
>+   *                 in the preferences.  ("imap", "pop3", "nntp", "movemail",
>+   *                 "none", "rss", "generic")

You definitely don't need a list here, since it's at best redundant with lists elsewhere.

Also, this method should explicitly mention that it searches the username/hostname properties, and provide a link to the findRealServer, and what happens if those values are null/empty.

>+  /**
>+   * Searches for the server with the given URI.

You probably want to have some comment that indicates how it picks apart hostname, username, etc. from the URI, and what happens if those components are missing.

>+   *
>+   * @param aURI       uri to compare.
>+   * @param aRealFlag  Selects whether we compare input against the 'realhostname'
>+   *                   and 'realuserName' pref settings or 'hostname' and 'username'.
>+   *                   If is true we comapre against realHostName and realHostName.
>+   *
>+   * @return Required server if exists.
>+   * @throws NS_ERROR_UNEXPECTED if server doesn't exist.
>+   * @see FindServer
>+   */
>+  nsIMsgIncomingServer
>+      findServerByURI(in nsIURI aURI, in boolean aRealFlag);

>+  /**
>+   * Searches for the server with the given username, hostname, type and port
>+   * except it compares the input values against 'realhostname' and
>+   * 'realuserName' pref settings.
>+   *
>+   * @param userName User name.
>+   * @param hostname Server host name.
>+   * @param type     Type of server. The type is the same as is specified
>+   *                 in the preferences.  ("imap", "pop3", "nntp", "movemail",
>+   *                 "none", "rss", "generic")
>+   * @param port     Port of server.
>+   *
>+   * @return Required server if exists.
>+   * @throws NS_ERROR_UNEXPECTED if server doesn't exist.
>+   * @see FindServer
>+   */
>+  nsIMsgIncomingServer
>+      findRealServer(in ACString userName, in ACString hostname, in ACString type, in long port );

Again, drop the list in the type parameter. Also, what happens with missing, etc. parameters (IIRC, port == -1 means something special).

>+  /**
>+   * Finds the index of this server in the (ordered) list of accounts.
>+   *
>+   * @param server Server we want to find.
>+   *
>+   * @return Index of registered server or an index higher than all "registered"
>+   *         servers if server isn't "registered".
>+   */
>+  long FindServerIndex(in nsIMsgIncomingServer server);

I see you don't define "registered" anywhere, and the quotes are distracting from a documentation standpoint.

>+  /**
>+   * Returns all identities in accounts that have the given server.
>+   *
>+   * @param server Given a server.

Change this to the string you had for GetAccountForServer (as well as for the next few methods).

>+  /**
>+   * Loads all accounts into memory. You do not need to call this
>+   * and all accounts should be loaded lazily if you use any
>+   * of the above.
>+   */
>+  void LoadAccounts();

"Any of the above" is a bit vague, especially since I think documentation tools can reorder the documentation to align with alphabetical order.

>+  /// Loads the virtual folders into the accounts.
>+  void loadVirtualFolders();

Can you be more specific about what this actually does?

>+  /**
>+   * Executes cleanup actions on exit for every loaded account.
>+   * It empties the trash, compacts the inbox depending on
>+   * an account preferences.
>+   */
>+  void CleanupOnExit();

Nit: empties the trash or compacts the inbox

>+  /**
>+   * Sets the inbox folder being cleanup.
>+   *
>+   * @param folder Given folder.
>+   */
>+  void SetFolderDoingCleanupInbox(in nsIMsgFolder folder);

Nit "being cleaned up"

>+/**
>+ * This interface is used to find out whether extensions for
>+ * the account manager should be displayed or not.
>+ */

This probably wants some expanded documentation on the steps one would need take to get this part working, i.e., the category manager registration (IIRC), etc.