[Roaming] Changes 1

VERIFIED FIXED in mozilla1.8alpha2

Status

Core Graveyard
Profile: Roaming
VERIFIED FIXED
14 years ago
2 years ago

People

(Reporter: BenB, Assigned: BenB)

Tracking

Trunk
mozilla1.8alpha2
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Assignee)

Description

14 years ago
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

14 years ago
Created attachment 150751 [details] [diff] [review]
Patch, v1, diff -uw

extensions/sroaming/
(Assignee)

Comment 2

14 years ago
Created attachment 150752 [details] [diff] [review]
Changes to filesList.js, v1 (for convience)
(Assignee)

Updated

14 years ago
Attachment #150751 - Attachment description: Patch, v1 → Patch, v1, duff -uw
(Assignee)

Updated

14 years ago
Attachment #150751 - Attachment description: Patch, v1, duff -uw → Patch, v1, diff -uw
(Assignee)

Comment 3

14 years ago
Created attachment 150753 [details] [diff] [review]
Patch, v1, diff -uwN

Forgot the -N to include filesList.js
(Assignee)

Updated

14 years ago
Attachment #150751 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
No longer blocks: 244589
(Assignee)

Updated

14 years ago
Blocks: 244589
(Assignee)

Updated

14 years ago
Target Milestone: --- → mozilla1.8alpha2
(Assignee)

Updated

14 years ago
Blocks: 246201
(Assignee)

Comment 4

14 years ago
Created attachment 150896 [details] [diff] [review]
Patch, v2, diff -uwN

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

14 years ago
Created attachment 150897 [details] [diff] [review]
Patch, v2, diff -uN (includes whitespace changes, for applying)

See the other bugs (e.g. bug 246201) for further fix description.
(Assignee)

Comment 6

14 years ago
Created attachment 150899 [details] [diff] [review]
Changes to filesList.js, v2 (for review)

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

14 years ago
Attachment #150896 - Flags: review?(pete.zha)

Comment 7

14 years ago
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

14 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

14 years ago
Attachment #150896 - Flags: superreview?(darin)

Comment 9

14 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

14 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

14 years ago
Checked in. Thanks for the fast reviews!
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Updated

14 years ago
Component: Profile: BackEnd → Profile: Roaming
QA Contact: core.profile-manager-backend → core.profile-roaming
(Assignee)

Updated

14 years ago
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.