Closed Bug 246710 Opened 20 years ago Closed 20 years ago

[Roaming] Changes 1

Categories

(Core Graveyard :: Profile: Roaming, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.8alpha2

People

(Reporter: BenB, Assigned: BenB)

References

Details

Attachments

(3 files, 3 obsolete files)

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.
Attached patch Patch, v1, diff -uw (obsolete) — Splinter Review
extensions/sroaming/
Attachment #150751 - Attachment description: Patch, v1 → Patch, v1, duff -uw
Attachment #150751 - Attachment description: Patch, v1, duff -uw → Patch, v1, diff -uw
Attached patch Patch, v1, diff -uwN (obsolete) — Splinter Review
Forgot the -N to include filesList.js
Attachment #150751 - Attachment is obsolete: true
No longer blocks: 244589
Blocks: 244589
Target Milestone: --- → mozilla1.8alpha2
Blocks: 246201
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
See the other bugs (e.g. bug 246201) for further fix description.
The patches here fix a number of serious roaming bugs and should go in before
alpha2 closes.
Attachment #150752 - Attachment is obsolete: true
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+
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
Attachment #150896 - Flags: superreview?(darin)
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+
Nothing to worry (see above "- Please ignore the |XXX|es for now"), not relevant
for Mozilla, will remove them before checkin.
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
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: