Implement the Mozilla side Palm sync logic

VERIFIED FIXED in mozilla1.2beta

Status

MailNews Core Graveyard
Palm Sync
VERIFIED FIXED
16 years ago
9 years ago

People

(Reporter: Rajiv Dayal, Assigned: Rajiv Dayal)

Tracking

Trunk
mozilla1.2beta
x86
Windows NT
Dependency tree / graph

Details

Attachments

(1 attachment, 8 obsolete attachments)

(Assignee)

Description

16 years ago
This bug tracks the implementation done in Mozilla for Palm sync. The Mozilla
Palm Conduit communicates with this part of code. This implements the sync logic
for synchronization with Palm. It exposes its functions via the existing nsIMapi
interface currently which is used for MAPI support thus reusing it as the IPC
infrastructure to communicate with the PalmConduit.

This implementation gets any updated (modified/new/deleted) Palm records,
retrieves any updated (modified/new/deleted) for a given AB and sync the Palm
record list and sends the updated Moz card list back to the Palm conduit for
modifying the Palm AB.

See the design doc at: http://www.mozilla.org/mailnews/arch/MozAB_Palm_HotSync.html
for more details.
(Assignee)

Comment 1

16 years ago
Created attachment 97055 [details] [diff] [review]
First patch for implementation of Palm sync in Mozilla

This implementation takes care of synchronizing the Palm records sent by Palm
Conduit into the Mozilla AB as well as supplying the Palm Conduit with
modified, new or deleted cards for existing AB, adding new AB that exist on
Palm but not in Mozilla, sending all cards from an AB that existing in Mozilla
but not on Palm. To facilitate the transfer of data across process space a
class derived from nsIAbCard is implemented that converts to the data structure
required for IPC and vice-versa.
(Assignee)

Updated

16 years ago
Attachment #97055 - Attachment description: Patch for implementation of Palm sync in Mozilla → First patch for implementation of Palm sync in Mozilla
(Assignee)

Updated

16 years ago
Summary: implement the Mozilla side Palm sync logic → Implement the Mozilla side Palm sync logic
(Assignee)

Comment 2

16 years ago
Created attachment 97056 [details] [diff] [review]
First patch exposing Palm sync interface for IPC using COM

This patch implements extends the existing nsIMapi interface for Palm AB sync,
implements the new methods in the interface and defines the data structure for
Palm sync to be called from another process space.
(Assignee)

Comment 3

16 years ago
Created attachment 97744 [details] [diff] [review]
updated patch for implementation of Palm sync in Mozilla

This patch takes care of Cavin's comments :
- use macros for nsIAbIPCCard::GetABCOMCardStruct implementation
- check the PalmRecordId in nsIAbIPCCard::Same function besides the name fields

- modify Equals to return a list of differing attributes instead of single one

Besides that this patch also has the following changes:
- make sure PalmRecordId is getting saved in AB DB in all cases
- use the PR conversion operator macros and functions instead of atol and _ltoa

- changed the logic for the case when same records / cards are modified on both
the Palm HH AB and Mozilla AB and made it work like the Palm desktop. It now
adds another record for the modified records and let the user manually delete
the unwanted record. This is how Palm Desktop works. Hence removed the unwanted
functions for merging records using previous backed up Moz AB file.
Attachment #97055 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Blocks: 155417
(Assignee)

Comment 4

16 years ago
Created attachment 98611 [details] [diff] [review]
updated patch for Palm sync support in Mozilla

This patch has the following changes:
- changed the code so that a list of pointers for new cards to be added in Palm
is kept and defined a new function Done() which is called when the Palm Conduit
calls back with the generated Palm Record Ids after the new records are added.
The Palm Record Ids is updated for the corresponding cards kept in the list.
- made it so that before a card is added in Moz AB during Palm sync validation
is done that it does not already exist in Moz AB.
- made it so that if a Palm modified card is not already existing on Moz AB it
adds a new card during the Palm sync.
- changed the code for modified card, instead of deleting and adding the card
(done since the fields modified are not known), all fields of the card are
modified. This is done as per comment #10 on bug #164970.

Also as suggested by Cavin changed the nsIAbIPCCard::Equals to intialize
_retval once instead of for each condition check. Also changed the
nsIAbIPCCard::Copy code to copy the DBRowId, DBTableId and Key attribs.

Cavin, can u please review this patch, thanks.
Attachment #97744 - Attachment is obsolete: true
(Assignee)

Comment 5

16 years ago
Created attachment 98613 [details] [diff] [review]
updated patch for nsIMapi Palm sync interfaces

this patch has the following changes:
- defined a new method nsSyncUpdateIds called by the Conduit when new records
are added into Palm and sync is done to set the generated Palm record Ids in
Moz AB.
Attachment #97056 - Attachment is obsolete: true

Comment 6

16 years ago
Comment on attachment 98611 [details] [diff] [review]
updated patch for Palm sync support in Mozilla

r=cavin.
Attachment #98611 - Flags: review+

Comment 7

16 years ago
Comment on attachment 98613 [details] [diff] [review]
updated patch for nsIMapi Palm sync interfaces

r=cavin.
Attachment #98613 - Flags: review+

Comment 8

16 years ago
Copyright (C) 2001 should be 2002 wherever it's 2001

+    mStatus = srcCard->dwStatus;
+
+	SetFirstName(srcCard->firstName);
+	SetLastName(srcCard->lastName);

could we make the indentation consistent here?

Have you looked at the string guide?

http://www.mozilla.org/projects/xpcom/string-guide.html#Guidelines

it says you shouldn't use AssignWithConversion

+    str.AssignWithConversion((char*)srcCard->firstName);

this should just be "return mRecordId == palmID;
+                if(mRecordId == palmID)
+                    return PR_TRUE;
+                else
+                    return PR_FALSE;

more indentation problems:
+
+	nsXPIDLString str;
+	card->GetFirstName(getter_Copies(str));
+    if (Compare(str, m_FirstName, nsCaseInsensitiveStringComparator()))

this looks windows-specific. We should avoid windows-specific code if we ever
hope to do this on other platforms.
+                                            d = (LPTSTR) new
PRUnichar[s.Length()+1];   \
+                                            wcscpy(d, s.get());               
         \
+ 

+void nsAbIPCCard::CleanUpABCOMCardStruct(nsABCOMCardStruct * card)
delete checks for null, so you don't need to check every field for null before
calling delete.

I think we still use PR_Calloc, not raw calloc, and PR_Free, not free()

we still use PRInt32, not int, like here (and whereever else):

+            for(int i=mPalmRecords.Count()-1; i >=0;  i--) {

+            if ( (aIPCCard->GetStatus() == ATTR_DELETED) && 
+                 ((palmRec->dwStatus == ATTR_NEW) 
+                   ||(palmRec->dwStatus == ATTR_MODIFIED)) ) {
+                keep = PR_FALSE;
+                return keep;
last two lines should just be return PR_FALSE;

no need to check NS_FAILED here - we'll already return rv;
+    mDirectory = do_QueryInterface(resource, &rv);
+    if (NS_FAILED(rv)) 
+        return rv;
+
+    return rv;

I don't see any makefile changes (but it's a huge diff) - is this intended to be
an extension that normal users won't have to install or load?

That's enough for now - that's a lot of code to look at!

Comment 9

16 years ago
adding self to cc list.
(Assignee)

Comment 10

16 years ago
I am sorry, the patches are not untabified, i will remove tabs and thus correct
the indentation.

This is Windows only feature/implementation and I use LPTSTR in IPC calls since
if the calling app is not UNICODE compliant it will get the correct pointer
(char*), and if it is, then it will get (wchar *). I take isUnicode as an input
parameter to the IPC COM calls made from the calling Conduit and accordingly
convert, typecast if required, and return the data.

The reason i used calloc instead of PR_Calloc was because calloc is ANSI C, but
I can change if required.

Comment 11

16 years ago
the the nsIMapi patch,
+        if(server->PalmSyncTimeStamp > 0)
+            *m_FirstTimeSyncList = PR_FALSE;
+        else
+            *m_FirstTimeSyncList = PR_TRUE;
this can just be *m_firstTimeSyncList = (server->PalmSyncTimeStamp <= 0) (or ==
0, I don't know the type of the time stamp

again, no need to check for null here:
+    if(tempPalmHotSync)
+        delete tempPalmHotSync;

but overall, I really don't like adding these thing to mapi. It doesn't make any
sense, does it? It's really just for your convenience. Is there a technical
reason to put it here?
(Assignee)

Comment 12

16 years ago
Ideally this should be a separate interface but the idea to put it as part of
the MAPI interface was to reuse the IPC framework we already have for MAPI, as I
had discussed during the design review meeting. I was thinking to check it in
currently reusing the MAPI IPC framework and later do all the work required to
have its own MS COM interface.
(Assignee)

Updated

16 years ago
Target Milestone: --- → mozilla1.2beta

Comment 13

16 years ago
nsAbPalmHotSync::nsAbPalmHotSync(PRBool aIsUnicode, PRUnichar * aAbDescUnicode,
char * aAbDesc, PRInt32 aPalmCatID)
+{
+    mTotalCardCount=0;
+
+    mCardForPalmCount = 0;
+    mCardListForPalm = nsnull;
+    mPalmRecords.Clear();
+

why are you clearing mPalmRecords? I would think it would already be empty

+    if(NS_SUCCEEDED(rv))
+        rv = mABDB->Close(PR_TRUE);
+    else
+        rv = mABDB->Close(PR_FALSE);
+
why not rv = mABDB->Close(NS_SUCCEEDED(rv)); ?

is an empty db really an error? Or is that not what not being able to ge an
enumerator means? I would just return rv here, and make sure EnumerateCards
returns an enumerator on success.
+    nsCOMPtr<nsIEnumerator> cardsEnumerator;
+    rv = mABDB->EnumerateCards(mDirectory, getter_AddRefs(cardsEnumerator));
+    if (NS_FAILED(rv) || !cardsEnumerator) {
+        mABDB->Close(PR_FALSE);
+        return NS_ERROR_NOT_AVAILABLE; // no cards available
+    }


+        if(NS_SUCCEEDED(rv) && mPalmRecords.Count())
+            rv = mABDB->Close(PR_TRUE);
+        else
+            rv = mABDB->Close(PR_FALSE);
+
can just be one line of code.

use QueryElementAt here:
+        nsCOMPtr<nsISupports> support =
getter_AddRefs(deletedCardArray->ElementAt(i));
+        nsCOMPtr<nsIAbCard> card;
+        card = do_QueryInterface(support, &rv);


+            if(lastModifiedDate)
+                ipcCard.SetStatus(ATTR_MODIFIED);
+            else
+                ipcCard.SetStatus(ATTR_NEW);
ipCard.SetStatus((lastModifiedDate) ? ATTR_MODIFIED : ATTR_NEW);

+                keep = PR_FALSE;
+                mPalmRecords.RemoveElementAt(i);
+                return keep;
 
why not just:
+                mPalmRecords.RemoveElementAt(i);
+                return PR_FALSE;

similarly,
+                   ||(palmRec->dwStatus == ATTR_MODIFIED)) ) {
+                keep = PR_FALSE;
+                return keep;
return PR_FALSE;

combine these into one if statement:
    if(!mInitialized) 
+        return NS_ERROR_NOT_INITIALIZED;
+    
+    if(!mABDB || !mDBOpen)
+        return NS_ERROR_NOT_INITIALIZED;
+

this code seems to be more or less duplicated:
+            if(!aIPCCard->GetRecordId()) {
+                PRInt64 l;
+                LL_UI2L(l, palmRec->dwRecordId);
+                PRFloat64 f;
+                LL_L2F(f, l);
+                char buf[128]; 
+                PR_cnvtf(buf, 128, 0, f);
+                aIPCCard->SetAbDatabase(mABDB);
+                aIPCCard->SetStringAttribute(CARD_ATTRIB_PALMID,
NS_ConvertASCIItoUCS2(buf).get());
+                mABDB->EditCard(aIPCCard, PR_FALSE);
+                aIPCCard->SetRecordId(palmRec->dwRecordId);


and

+                PRInt64 l;
+                LL_UI2L(l, aPalmRecordIDList[i]);
+                PRFloat64 f;
+                LL_L2F(f, l);
+                char buf[128]; 
+                PR_cnvtf(buf, 128, 0, f);
+                dbCard->SetAbDatabase(mABDB);
+                dbCard->SetStringAttribute(CARD_ATTRIB_PALMID,
NS_ConvertASCIItoUCS2(buf).get());
+                nsCOMPtr<nsIAbCard> newCard;
+                newCard = do_QueryInterface(dbCard, &rv);
+                if(NS_SUCCEEDED(rv))
+                    mABDB->EditCard(newCard, PR_FALSE);

would it be possible to put this in a helper routine?


why limit yourself to 16 bits here? Is there a good reason?
+GetAllCards(PRInt16 * aCount, 
(Assignee)

Comment 14

16 years ago
Created attachment 100699 [details] [diff] [review]
updated Patch implementing new IPC interface and taking care of comments above

This patch has the following changes:
- all code for a new interface as the MSCOM bridge used for IPC rather than
extending existing MAPI IPC interface. Code for initialization of this, etc.
- makefiles for making this part of mailnews/extensions
- no need for any changes to the nsIMapi interface
- take care of above super review comments by David.
Attachment #98611 - Attachment is obsolete: true
Attachment #98613 - Attachment is obsolete: true
(Assignee)

Comment 15

16 years ago
Hi David,
Can u please super review the above updated patch.
thanks,
- Rajiv.

Comment 16

16 years ago
this should be 2002
+# Copyright (C) 1998 Netscape Communications Corporation. All
+# Rights Reserved.

tab?
+    unsigned long   dwCategoryId; // remote category Id for this card
+	unsigned long   dwStatus; // change status for this card
and this:
+# Communications Corporation.  Portions created by Netscape are
+# Copyright (C) 1998 Netscape Communications Corporation. All

this seems a bit odd - you can't call nsGetAllABCards more than once? Or does
this protect against calling this while a sync is already going on?
STDMETHODIMP CPalmSyncImp::nsGetAllABCards(BOOL aIsUnicode, unsigned long
aCategoryId, LPTSTR aABName,
+
+    if(m_PalmHotSync)
+        return E_FAIL;
+
+    m_PalmHotSync = (nsAbPalmHotSync *) new nsAbPalmHotSync(aIsUnicode,
aABName, (char*)aABName, aCategoryId);
+    if(!m_PalmHotSync)
+        return E_FAIL;
+

I'm pretty sure you don't need to check for null before calling free - that will
save some code.
    if(m_ServerDescList)
+        free(m_ServerDescList);
+    m_ServerDescList = nsnull;
+
+    if(m_FirstTimeSyncList)
+        free(m_FirstTimeSyncList);
+    m_FirstTimeSyncList = nsnull;
+
+    if(m_CatIDList)
+        free(m_CatIDList);

this code could be more compact if you made it all one if clause with ||

if (!setKey...)
  || !setKeyValue...
  || !setKeyValue...
     return false;

+    // Add the CLSID to the registry.
+    if (!setKeyAndValue(registryKey, NULL, szFriendlyName))
+        return S_FALSE;
+
+    if (!setKeyAndValue(registryKey, "LocalServer32", moduleName.get()))
+        return S_FALSE;
+
+    // Add the ProgID subkey under the CLSID key.
+    if (!setKeyAndValue(registryKey, "ProgID", szProgID))
+        return S_FALSE;
+
+    // Add the version-independent ProgID subkey under CLSID key.
+    if (!setKeyAndValue(registryKey, "VersionIndependentProgID", szVerIndProgID))
+        return S_FALSE;
+
+    // Add the version-independent ProgID subkey under HKEY_CLASSES_ROOT.
+    if (!setKeyAndValue(independentProgId, NULL, szFriendlyName))
+        return S_FALSE; 
+    if (!setKeyAndValue(independentProgId, "CLSID", szCLSID))
+        return S_FALSE;
+    if (!setKeyAndValue(independentProgId, "CurVer", szProgID))
+        return S_FALSE;
+
+    // Add the versioned ProgID subkey under HKEY_CLASSES_ROOT.
+    if (!setKeyAndValue(progId, NULL, szFriendlyName))
+        return S_FALSE; 
+    if (!setKeyAndValue(progId, "CLSID", szCLSID))
+        return S_FALSE;
+
+
I'll get to the rest later today. Overall, looks good.
(Assignee)

Comment 17

16 years ago
That is correct, the code below:
+    if(m_PalmHotSync)
+        return E_FAIL;
does not allow the function to be called if a sync already is going on.
(Assignee)

Comment 18

16 years ago
Created attachment 101085 [details] [diff] [review]
updated patch

Updated patch to take care of all suggestions from David.

Hi David, Can u please sr this patch. thanks.
(Assignee)

Updated

16 years ago
Attachment #100699 - Attachment is obsolete: true

Comment 19

16 years ago
+nsPalmSyncSupport::Observe(nsISupports *aSubject, const char *aTopic, const
PRUnichar *aData)
+{
+    nsresult rv = NS_OK ;
+
+    if (!nsCRT::strcmp(aTopic, "profile-after-change"))
+        return InitializePalmSyncSupport();
+
+    if (!nsCRT::strcmp(aTopic, NS_XPCOM_SHUTDOWN_OBSERVER_ID))
+        return ShutdownPalmSyncSupport();
+
+    nsCOMPtr<nsIObserverService>
observerService(do_GetService("@mozilla.org/observer-service;1", &rv));
+    if (NS_FAILED(rv)) return rv;
+ 
+    rv = observerService->AddObserver(this,"profile-after-change", PR_FALSE);
+    if (NS_FAILED(rv)) return rv;
+
+    rv = observerService->AddObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID,
PR_FALSE);
+    if (NS_FAILED(rv))  return rv;
+
+    return rv;
+

just put return observerService->AddObserver(....)

+    // donot close the DB yet, it will be closed when Done is called
+    // mABDB->Close(PR_FALSE);
could you remove the commented out line of code, and fix the typo? "Don't close..."

eh? can't you make it a static member function of nsAbPalmHotSync? how does it
not fit? Isn't it used by several other functions in nsAbPalmHotSync?
+// this is a utility function which doesnot fit as a member of the nsAbPalmSync
class
+void ConvertAssignPalmIDAttrib(PRUint32 id, nsIAbMDBCard * card,
nsIAddrDatabase * abDB)  

why are these defined in two separate header files in palmsync/src? Could they
just be in one header file instead?
+// these are states of Palm record
+// as defined in the Palm CDK
+#define ATTR_DELETED        0x0001
+#define ATTR_ARCHIVED       0x0002
+#define ATTR_MODIFIED       0x0004
+#define ATTR_NEW            0x0008
+#define ATTR_NONE           0x0020
+#define ATTR_NO_REC         0x0040

shouldn't this be new char[strlen(m_mailListURI) + 1]; ? you need room for the
terminating null byte.

+    card->isMailList = m_IsMailList;
+    if(m_MailListURI) {
+        card->mailListURI = new char[strlen(m_MailListURI)];
+        strcpy(card->mailListURI, m_MailListURI);
+    }
(Assignee)

Comment 20

16 years ago
Created attachment 101143 [details] [diff] [review]
updated patch to take care of comments above
Attachment #101085 - Attachment is obsolete: true

Comment 21

16 years ago
+            m_ServerDescList->lpszABDesc = (LPTSTR) malloc(sizeof(PRUnichar) *
abName.Length());
+            memcpy(m_ServerDescList->lpszABDesc, abName.get(), abName.Length());
+        }
this doesn't null terminate lpszABDesc in the unicode case - it should, right?
If this is wrong, this needs to be fixed.

this is a nit - fix it if you want:

It could just be mStatus = (lastModifiedDate) ? ATTR_MODIFIED : ATTR_NEW;

+    if(lastModifiedDate)
+        mStatus = ATTR_MODIFIED;
+    else
+        mStatus = ATTR_NEW;
+

another nit:
+
+    if(differingAttrs.Count())
+        return PR_FALSE;
+
+    return PR_TRUE;

this could just be 

return (differingAttrs.Count() == 0)

+        if(mRecordId == card->dwRecordId)
+            return PR_TRUE;
+        else
+            return PR_FALSE;

this should be return (mRecordId == card->dwRecordId);

this still looks broken in your new patch - it will corrupt memory, so it really
needs to be fixed:
+    if(m_MailListURI) {
+        card->mailListURI = new char[strlen(m_MailListURI)];
+        strcpy(card->mailListURI, m_MailListURI);
+    }
+


I believe rv is not used in this function - if I'm correct, could you please
remove it?
+// this take care of the all cases when the state is either mod or new
+PRBool nsAbPalmHotSync::CheckWithPalmRecord(nsAbIPCCard  * aIPCCard)
+{
+    NS_ENSURE_ARG_POINTER(aIPCCard);
+
+    if(!mInitialized) 
+        return NS_ERROR_NOT_INITIALIZED;
+
+    nsresult rv=NS_OK;


Looking at this code, I'm unclear on the need for cardForPalm. Couldn't you save
an extra allocation, memcpy, and free by doing this:

+    nsresult rv = ipcCard.GetABCOMCardStruct(mIsPalmDataUnicode,
mCardListForPalm[mCardForPalmCount]); ??

+// utility function
+nsresult nsAbPalmHotSync::AddToListForPalm(nsAbIPCCard & ipcCard)
+{
+    nsABCOMCardStruct * cardForPalm = new nsABCOMCardStruct; //allocate this on
heap
+    
+    nsresult rv = ipcCard.GetABCOMCardStruct(mIsPalmDataUnicode, cardForPalm);
+    cardForPalm->dwCategoryId = mPalmCategoryId;
+    if(NS_SUCCEEDED(rv)) {
+        // this should never happen but check for crossing array index
+        if(mCardForPalmCount >= mTotalCardCount)
+            return NS_ERROR_UNEXPECTED;
+
+        // append to the list of cards to be sent to Palm
+        memcpy(&mCardListForPalm[mCardForPalmCount], cardForPalm,
sizeof(nsABCOMCardStruct));
+        mCardForPalmCount++;
+    }
+
+    delete cardForPalm;
+    return rv;
+}
+


+    if (!mDirServerInfo->uri) {
+        nsCAutoString URI;        
+        URI = NS_LITERAL_CSTRING(kMDBDirectoryRoot) +
nsDependentCString(mDirServerInfo->fileName);
+        mDirServerInfo->uri = nsCRT::strdup(URI.get());
+    }

should use mDirServerInfo->uri = ToNewCString(URI);

just use strcmp instead of nsCRT::strcmp - I believe they're trying to get rid
of the latter.
+    if (!nsCRT::strcmp(aTopic, "profile-after-change"))
+        return InitializePalmSyncSupport();
+
+    if (!nsCRT::strcmp(aTopic, NS_XPCOM_SHUTDOWN_OBSERVER_ID))
+        return ShutdownPalmSyncSupport();

Comment 22

16 years ago
Rajiv, sorry to find all those things in that last review that I missed before.
I think we're almost done. I'm mostly concerned about the potential memory
trouncing problems.
(Assignee)

Comment 23

16 years ago
Created attachment 101185 [details] [diff] [review]
updated patch
Attachment #101143 - Attachment is obsolete: true

Comment 24

16 years ago
Comment on attachment 101185 [details] [diff] [review]
updated patch

you missed this one:

+	 if(mRecordId == card->dwRecordId)
+	     return PR_TRUE;
+	 else
+	     return PR_FALSE;

should just be 

return (mRecordId == card->dwRecordId);

fix that and sr=bienvenu, thx.
Attachment #101185 - Flags: superreview+
(Assignee)

Comment 25

16 years ago
thanks David, i will make the change before checking in.
(Assignee)

Comment 26

16 years ago
The code for this has landed. Marking as fixed.

Also the Mozilla Palm HotSync feature has gone through QA verified by Gregg
Meehan before landing. Changing QA contact.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
QA Contact: nbaca → meehansqa
Resolution: --- → FIXED
(Assignee)

Updated

16 years ago
Component: Address Book → Palm Sync

Comment 27

16 years ago
Trunk build 2003-02-20: WinXP
Verified Fixed. This has been fixed for awhile.
Status: RESOLVED → VERIFIED
Product: MailNews → Core
Product: Core → MailNews Core

Updated

9 years ago
Component: Palm Sync → Palm Sync
Product: MailNews Core → MailNews Core Graveyard
You need to log in before you can comment on or make changes to this bug.