679773 - Replace custom helpers with Services.jsm (Port Bug 648364)

Status: RESOLVED FIXED

(SeaMonkey :: Sync UI, defect)

Description

[H. Hofer]

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:8.0a1) Gecko/20110816 Firefox/8.0a1 SeaMonkey/2.5a1
Build ID: 20110816003027

Steps to reproduce:

Trying to connect to an existing sync account or editing sync preferences, manage account, reset account.
On the Sync Options page, select 'Replace all data on this computer...', then press 'Done' or 'Next'


Actual results:

Nothing (on the Sync Options page), but in the error log there's an entry that says:

> Error: Weave.Svc.History is undefined
> Source File: chrome://communicator/content/sync/syncSetup.js
> Line: 778


Expected results:

Confirm replacement of local data, then continue setup.

Trunk (2.5a1) is also affected.
SeaMonkey 2.2 worked OK, Firefox 6.0 still does.

Comment 1

[Philip Chee]

We probably need to port Bug 648364 - Replace custom helpers with Services.jsm

Comment 3

[Jens Hatlak (:InvisibleSmiley)]

(In reply to Philip Chee from comment #1)
> We probably need to port Bug 648364 - Replace custom helpers with
> Services.jsm

Right,
http://hg.mozilla.org/mozilla-central/rev/a839caa0357b

and Bug 648364 - Replace custom helpers with PlacesUtils.jsm
http://hg.mozilla.org/mozilla-central/rev/aa2c607672e1

I marked them as todo locally but didn't copy/move them to my Sync Todo folder, which is why I missed to address them. Sorry, will take care of the above RSN.

Comment 4

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch [Checkin: Comments 5 and 11]

Comment 5

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 554184 [details] [diff] [review]
patch [Checkin: Comments 5 and 11]

http://hg.mozilla.org/comm-central/rev/ad67b3c1a764

Comment 6

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 554184 [details] [diff] [review]
patch [Checkin: Comments 5 and 11]

AFAICS this has propagated to both Aurora and Beta already; requesting approval.

Comment 7

[Justin Wood (:Callek)]

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #6)
> Comment on attachment 554184 [details] [diff] [review]
> patch [Checkin: Comment 5]
> 
> AFAICS this has propagated to both Aurora and Beta already; requesting
> approval.

Have you tested this patch on either branch? hard for me to gage a risk assessment from a quick glance.

Comment 8

[H. Hofer]

The errors described here and in bug 679823 don't happen anymore.
But I still get this error after sync finishes:

> Error: Weave is not defined
> Source File: chrome://communicator/content/sync/syncSetup.js
> Line: 575
I don't know what the consequences are...

And, yes it would be _really_ good if this get's fixed on beta!

Comment 9

[Jens Hatlak (:InvisibleSmiley)]

(In reply to Justin Wood (:Callek) from comment #7)
> > AFAICS this has propagated to both Aurora and Beta already; requesting
> > approval.
> 
> Have you tested this patch on either branch? hard for me to gage a risk
> assessment from a quick glance.

There's no need to test this. The corresponding changes (which are 1:1, only different folders: browser vs suite) have already landed on both FF branches, which is what I was trying to say. The changes are related to Core (Toolkit) changes, where some methods have been moved from Weave.Svc.* to Services.* and PlacesUtils.* (refactoring). IOW, instead of calling A.* we have to now call X.* and Y.* because some methods were moved from A to X and Y. Obviously, still accessing A.* for those will result in "undefined" or "null" errors. It's the equivalent of references to non-existent/moved files, with the unfortunate difference that we get no error at compile time.

To please you, I've just created a build from current comm-beta with the patch. Both Change Password and My Sync Key (see bug 679823) worked again.

Note: 2.3 is also affected by this. Sorry I didn't catch it earlier; now it's too late. Will add to the release notes.

Comment 10

[Jens Hatlak (:InvisibleSmiley)]

(In reply to H. Hofer from comment #8)
> But I still get this error after sync finishes:
> 
> > Error: Weave is not defined
> > Source File: chrome://communicator/content/sync/syncSetup.js
> > Line: 575
> I don't know what the consequences are...

Would be interesting to know whether this happens in FF, too (since AFAIK the code is equal there) but in any case that's a different cause so deserves its own bug.

Comment 11

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 554184 [details] [diff] [review]
patch [Checkin: Comments 5 and 11]

http://hg.mozilla.org/releases/comm-aurora/rev/c521b24510c9
http://hg.mozilla.org/releases/comm-beta/rev/012db6331856