Created attachment 10794 [details]
mapping mozilla mailing lists as db tables

add to meta bug

this does sound great. Does it require your latest patches to Mozilla,
or does it already work when applied to a current OOo code base?

It requests nothing.It already work when applied to a current OOo code
base.

great! So perhaps I should get you a 680-based CWS where you can place
this ...

OpenOffice patch does not need r+ and sr+?

not in the same way. Usually you get checkin permissions if somebody
(usually the project lead) thinks you, hmm, "deserve" it :). This
doesn't free the project lead from looking at what you're doing or
checking in, but there is no formal r/sr process for patches.

Created attachment 11189 [details]
patch to mapping mozilla mailing list as OOo table

last patch fix a bug that mailing lists in Personal Address Book and
Collected Book are not been mapped.
Tested on win32 and linux.
Frank I think we need a CWS now.

Wind, you wished a review - here it goes :)

> -    // Test connection by getting to get the Table Names
> -    ::std::vector< ::rtl::OUString > tables;
> -    if ( !_aDbHelper.getTableStrings( this, tables, sal_True ) ) {
> -        ::dbtools::throwGenericSQLException(
_aDbHelper.getErrorString(), NULL);
> -    }
> +//    // Test connection by getting to get the Table Names
> +//    ::std::vector< ::rtl::OUString > tables;
> +//    if ( !_aDbHelper.getTableStrings( this, tables, sal_True ) ) {
> +//        ::dbtools::throwGenericSQLException(
_aDbHelper.getErrorString(), NULL);
> +//    }

What's the reason why you removed this? I do not know why the original
author wrote this piece, but the comment suggests it's necessary. Do
you think it's not necessary anymore?


> +	while (NS_ENUMERATOR_FALSE == subDirectories->IsDone())	{
> +		nsCOMPtr<nsISupports> newDirSupports ;
> +
> +		retCode =
subDirectories->CurrentItem(getter_AddRefs(newDirSupports)) ;
> +		if (NS_FAILED(retCode)) { continue ; }

This is probably an endless loop in case of an error, isn't it? Since
there is no |Next| call, the |CurrentItem| call will fail again an
again, right?

> +        subDirectories->Next();

Can |Next| fail? Should we check a |retcode| here?

> +		nsCOMPtr<nsIAbDirectory> childDir =
do_QueryInterface(newDirSupports, &retCode) ;
> +		if (NS_FAILED(retCode)) { continue ; }
> +		array->AppendElement(childDir);

Is it guaranteed that |childDir| is not |nsnull| when the |retcode|
indicates a success?

In the whole enumSubs method, it seems that |retcode| can *never* be
different from |NS_OK| except in the very first GetChildNodes call. Is
this intended?
Overall, it seems to me that the error handling in this method is
still not really matured.


> +        nsCOMPtr<nsIAbDirectory>
listDir(do_QueryInterface(pSupport, &rv));
> +        if (NS_FAILED(rv))
> +            break;

Out of pure interest: Are these 3 lines necessary? Can there be
elements which are an |nsIAddrDBListener|, but no |nsIAbDirectory|?

> +static nsresult openMDBFromURL(nsDependentCString
&uriStr,nsIAbDirectory * abDir)

Can the first parameter be a "const ...&" instead of just "...&"?

> +    retCode = abSession->GetUserProfileDirectory(&dbPath);
> +

Could you please check the |retcode| here? Else, if the call fails,
but |dbPath< is not set to |nsnull|, this could potentially crash
(since |dbPath| is not being initialized with |nsnull| above).

> +      if (Substring(uriStr, 0,
kMDBDirectoryRootLen).Equals(kMDBDirectoryRoot))
> +          fileName = Substring(uriStr, kMDBDirectoryRootLen,
uriStr.Length() - kMDBDirectoryRootLen);
> +      (*dbPath) += fileName.get();

What if the Substring(...) returns |PR_FALSE|? In this case, |dbPath|
points to the profile directory, right? Is this intended?

> +      nsCOMPtr<nsIAddrDatabase>  addrDBFactory =
do_GetService(NS_ADDRDATABASE_CONTRACTID, &retCode);
> +      NS_ENSURE_SUCCESS(retCode, retCode);

You have a resource leak here: In case |retCode| does not indicate a
success, |dbPath| is *not* freed.

> +    retCode = RemoveMailListDBListeners (listDatabase, abDir);
Isn't there another possibility to prevent the assertions which you
mention you in the comment? I am somehwat unhappy with what you
yourself call a hack here. I do not understand all of the involved
code, but would be the following possible:
- openMDBFromURL gets an additional parameter of type
|nsIAddrDatabase*|, where it *also* returns the database.
- The caller of openMDBFromURL is responsible for _first_ releasing
the lists (means |dirPAB| and |dirCA|), and _then_ the database.
Wouldn't this also avoid the assertions?

> +		nsDependentCString pabURLStr("moz-abmdbdirectory://abook.mab");
> ...

I know this is originally not introduced, but while you're touching this:
After the patch, there are now 2 blocks of code (starting with
"retCode = rdfService->GetResource(..:" and ending with
"enumSubs(...") which are *nearly* identical: one for retrieving data
from the "abook.mab", and one for retrieving data from the
"history.mab". Could you please create a dedicated method for this,
which is called twice, instead of the code duplication? Thanks.
This would also prevent the duplicate occurance of both the name for
"abook.mab" and "history.mab".

> -        subDirectory -> GetIsMailList(&bIsMailList);
> +        bIsMailList = PR_FALSE;
> +        //subDirectory -> GetIsMailList(&bIsMailList);

Could you please remove the obsolete code here, instead of just
uncommenting it? It seems to me that the |bIsMailList| flag is
completely superfluous now.

> -        card->GetIsMailList( &bIsMailList );
> +        //card->GetIsMailList( &bIsMailList );

Same as above, please remove obsolete code instead of just
uncommenting it. In case you want to keep the code uncommented (which
would be valid, e.g. for tracking the original code later), then
please add a comment *why* you did the change (means why it isn't
necessary to ask for the |bIsMailList| flag).



Other than this, I'm fine with this :). I admit I do not know enough
about the Mozilla AB API, so I cannot fully judge this from a
semantical perspective, but I am confident in you here :)

Thanks

Created attachment 11566 [details]
mapping mailing lists to db views

The major changes in the 3th patch are:
  1.add a function insertPABDescription to add
"ldap_2.servers.history.description" and
"ldap_2.servers.pab.description" to mozilla profile,so we can use
GetChildNodes to get all sub directories from mozilla.Not need add the
personal and collected addresses address books by hand any
more."ldap_2.servers.*.description" equal to table name  in OOo.
   2.add more code to MNameMapper::add to make sure we never add a
table more then one time and create new names(like oldname + num) for
those tables with same names.
   3.Put back the test code.There is a bug in mozilla,it result in
that if we call GetMailingList more then one times,we get more then
one mailing list entry.Mozilla does not check it any more.I make this
check in MNameMapper::add now.
   4.Map mailing lists to views.In deed mozilla mailing lists are some
kinds of views.Address included in mailing lists are also included in
it's parent. 

Created attachment 11577 [details]
patch officecfg,add mozilla pab and history addressbook name nodes

Created attachment 11578 [details]
patch officecfg,add mozilla pab and history addressbook name nodes,using utf-8

Created attachment 11579 [details]
patch connectivity,remove some riskful codes from function "enumSubs"

looks good to me :)

mapping mozilla mailing list as db table
->mapping mozilla mailing list as db view

Created attachment 11694 [details]
add lang=de,add long descriptions

set target to OOo 2.0

change subcomponent to 'none'

Migrate to a new account for security reasons.

fixed in cws mozab03

verified in mozab03

verify in CWS mozab03

close -> works in src680m33