Closed
Bug 246710
Opened 20 years ago
Closed 20 years ago
[Roaming] Changes 1
Categories
(Core Graveyard :: Profile: Roaming, defect)
Core Graveyard
Profile: Roaming
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla1.8alpha2
People
(Reporter: BenB, Assigned: BenB)
References
Details
Attachments
(3 files, 3 obsolete files)
52.66 KB,
patch
|
zhayupeng
:
review+
darin.moz
:
superreview+
|
Details | Diff | Splinter Review |
62.92 KB,
patch
|
Details | Diff | Splinter Review | |
35.61 KB,
patch
|
Details | Diff | Splinter Review |
I'll attach a bunch of changes to the roaming code. Some of them shuffle code around, others fix bugs in the same code, so I can't attach them as simple patches to the relevant bugs. This contains a few changes I did while using much of the same code for another application (Mozilla updater). List of changes: - Some code refactoring: Creating file filesList.js, which contains generic functions for using file lists (arrays whose entries keep file names and statistics) and corresponding listing files. They can be (and are) used in another application, and previously were in file conflictCheck.js. conflictCheck.js is now even more focussed on the 2-way-sync logic of roaming. - Added some safe-exception-catches, and showing the error in the UI. Currently, the code just malfunctions (broken dialog buttons?) in that case. This should help with bug 244589 and bug 244720, if the fix below fails, and other future unexpected problems. - Making the status message actually show something. - Renaming dumbObject() to ddumpObject() to match ddump() for easier replace In filesList.js: - Changing listing file to actually use Unixtime as claimed (breaks existing files, but should only cause conflicts once) - Using indexed file lists, should be faster, probably not significant for roaming - Allow last modified time to differ 1 sec, because of FAT inaccuracy, not needed for roaming - Option to allow newer files, not needed for roaming - Fixing bug 244589 and probably bug 244720 by moving dom creation after the empty filename check. - Please ignore the |XXX|es for now. To allow you to more easily see the changes I made to the functions which now moved to filesList, I'll add a diff between the old conflictCheck.js and the new filesList.js. Because I splitted the old conflictCheck.js into conflictCheck and filesList.js, you'll see lots of removal in the diffs. Pete Zha, please review.
Assignee | ||
Comment 1•20 years ago
|
||
extensions/sroaming/
Assignee | ||
Comment 2•20 years ago
|
||
Assignee | ||
Updated•20 years ago
|
Attachment #150751 -
Attachment description: Patch, v1 → Patch, v1, duff -uw
Assignee | ||
Updated•20 years ago
|
Attachment #150751 -
Attachment description: Patch, v1, duff -uw → Patch, v1, diff -uw
Assignee | ||
Comment 3•20 years ago
|
||
Forgot the -N to include filesList.js
Assignee | ||
Updated•20 years ago
|
Attachment #150751 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Target Milestone: --- → mozilla1.8alpha2
Assignee | ||
Comment 4•20 years ago
|
||
More changes: - fixed bug 246201 - infinite conflicts - less conflicts, if files non-existant - transfer less (if we know we have the file already) - minimal API change for extractFiles() - code doc improved - pref API usage
Attachment #150753 -
Attachment is obsolete: true
Assignee | ||
Comment 5•20 years ago
|
||
See the other bugs (e.g. bug 246201) for further fix description.
Assignee | ||
Comment 6•20 years ago
|
||
The patches here fix a number of serious roaming bugs and should go in before alpha2 closes.
Attachment #150752 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #150896 -
Flags: review?(pete.zha)
Comment on attachment 150896 [details] [diff] [review] Patch, v2, diff -uwN >+ setTimeout(loaded, 0); >+ /* using timeout, so that we first return (to deliver |result| to the >+ caller) before we invoke loadedCallback, so that loadedCallback >+ can use |result|. */ Probably you can just pass the result to the "loaded" function? >+function printtree(domnode, indent) >+{ >+ return; >+ if (!indent) >+ indent = 1; >+ for (var i = 0; i < indent; i++) >+ ddumpCont(" "); >+ ddumpCont(domnode.nodeName); >+ if (domnode.nodeType == Node.ELEMENT_NODE) >+ ddump(" (Tag)"); >+ else >+ ddump(""); >+ >+ var root = domnode.childNodes; >+ for (var i = 0, l = root.length; i < l; i++) >+ printtree(root.item(i), indent + 1); >+} Do we need to comment out the ddump? > //ddump(" finished callbacks: " + this.finishedCallbacks.length); > //ddump(" progress callbacks: " + this.progressCallbacks.length); >- for (var i = 0, l = this.finishedCallbacks.length; i < l; i++) >- ddump(this.finishedCallbacks[i]); >+ //for (var i = 0, l = this.finishedCallbacks.length; i < l; i++) >+ //dump(this.finishedCallbacks[i]); We really need to think about the debug code style. not just comment out. There is a feature that we can use the compiling switch in js. Probably we can use this feature to write debug code in the furture. Besides these issues, it looks ok for me.
Attachment #150896 -
Flags: review?(pete.zha) → review+
Assignee | ||
Comment 8•20 years ago
|
||
Thanks for the review, Pete! > Probably you can just pass the result to the "loaded" function? No, this would cause a different execution flow, it would execute the loaded function before the readAndLoad... function returns, so anything that potentially comes after the call would not be executed at the right time. This code is just moved anyways, IIRC, I didn't change this code itself in the patch. > Do we need to comment out the ddump? No, there's a |return;| at the very beginning. > We really need to think about the debug code style. Yes! It's a terrible hack. I have a constant in ddump() to suppress the output, but the ddump call and its parameters would still be evaluated, which IIRC causes a somewhat notable slowdown. Let's talk about that in another bug, if necessary.
Status: NEW → ASSIGNED
Assignee | ||
Updated•20 years ago
|
Attachment #150896 -
Flags: superreview?(darin)
Comment 9•20 years ago
|
||
Comment on attachment 150896 [details] [diff] [review] Patch, v2, diff -uwN +//XXX! what's that for? should we worry? there are several of these. how about adding some comments? rs=darin
Attachment #150896 -
Flags: superreview?(darin) → superreview+
Assignee | ||
Comment 10•20 years ago
|
||
Nothing to worry (see above "- Please ignore the |XXX|es for now"), not relevant for Mozilla, will remove them before checkin.
Assignee | ||
Comment 11•20 years ago
|
||
Checked in. Thanks for the fast reviews!
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Component: Profile: BackEnd → Profile: Roaming
QA Contact: core.profile-manager-backend → core.profile-roaming
Assignee | ||
Updated•20 years ago
|
Status: RESOLVED → VERIFIED
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•