587027 - Use as little memory as possible

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Philipp von Weitershausen [:philikon]]

Sync should do as little as possible on startup, especially when it's not configured:

* Move the Weave object to a separate file (main.js)
* Make properties lazy getters that import only when accessed (incl. service.js)
* Move startup related status stuff (_checkSetup etc.) to status.js so service.js doesn't have to be imported if the client isn't configured.

Comment 1

[Philipp von Weitershausen [:philikon]]

Attachment: part1 v1

Part 1: Move the Weave object out of service.js

Comment 2

[Philipp von Weitershausen [:philikon]]

Attachment: part2 v1

Part 2: Fix tests after Weave relocation

Comment 3

[Philipp von Weitershausen [:philikon]]

Attachment: part3 v1

Part 3: Fix up add-on UI to import main.js to get the Weave object.

Comment 4

[Philipp von Weitershausen [:philikon]]

Attachment: part4 v1

Refactor Service._checkSetup() to Status.checkSetup().

Also move the test fixture for WeaveCryptoID to the individual tests that need it.

Comment 5

[Philipp von Weitershausen [:philikon]]

Attachment: part5 v1

Avoid accessing Weave.Service (=importing service.js) at all cost.

Comment 6

[Philipp von Weitershausen [:philikon]]

Comment on attachment 465916 [details] [diff] [review]
part5 v1

Clearing r? flag from WIP patch... muscle memory ;)

Comment 7

[Mike Connor [:mconnor]]

Comment on attachment 465860 [details] [diff] [review]
part1 v1


>+let Weave = {};
>+Components.utils.import("resource://services-sync/constants.js", Weave);
>+lazyImport("resource://services-sync/auth.js", Weave,
>+           ['Auth', 'BrokenBasicAuthenticator', 'BasicAuthenticator',
>+            'NoOpAuthenticator']);
>+lazyImport("resource://services-sync/base_records/keys.js", Weave,
>+           ['PubKey', 'PrivKey', 'PubKeys', 'PrivKeys']);
>+lazyImport("resource://services-sync/engines.js", Weave,
>+           ['Engines', 'Engine', 'SyncEngine']);
...etc 

for the sake of readability, can we do:

let imports = {
  "auth.js":              ['Auth', 'BrokenBasicAuthenticator', 
                           'BasicAuthenticator', 'NoOpAuthenticator'],
  "base_records/keys.js": ['PubKey', 'PrivKey', 'PubKeys', 'PrivKeys'],
  etc
};

for (let i in imports)
  lazyImport("resource://services-sync/" + i, Weave, imports[i])

Just makes it more explicitly mapping file -> symbols and removes a lot of text duplication 

r=me with that change

Comment 8

[Philipp von Weitershausen [:philikon]]

Attachment: part1 v2

Addressed review comments.

Comment 9

[Philipp von Weitershausen [:philikon]]

Attachment: part2 v2

Fix up new test that was committed in the mean time.

Comment 10

[Philipp von Weitershausen [:philikon]]

Attachment: part4 v2

Fix bug in Weave.Status.checkSetup(): We need to create the Identity objects if they don't exist yet before being able to check the password.

Comment 11

[Philipp von Weitershausen [:philikon]]

Comment on attachment 465916 [details] [diff] [review]
part5 v1

Turns out that part 5 (avoiding accessing Weave.Service in the UI code) was working, there was just a small bug in part 3 (Weave.Service.checkSetup()). Requesting r?

Comment 12

[Philipp von Weitershausen [:philikon]]

(In reply to comment #11)
> bug in part 3 (Weave.Service.checkSetup()).

Of course I meant part 4.

Comment 13

[Philipp von Weitershausen [:philikon]]

Attachment: part5 v2

Don't explicitly access Weave.Service, wait for Weave.js to import service.js.

Comment 14

[Philipp von Weitershausen [:philikon]]

Attachment: part5 v3

Also avoid accessing Weave.Service in Fennec UI and Weave.js when not necessary.

Comment 15

[Philipp von Weitershausen [:philikon]]

Attachment: part1 v3

Continue to export the "Weave" object from service.js for backwards compatibility with the Firefox 4.0b4 UI.

Comment 16

[Mike Connor [:mconnor]]

Comment on attachment 469091 [details] [diff] [review]
part5 v3


>diff --git a/ui/fennec/content/overlay.js b/ui/fennec/content/overlay.js
>     // Generating keypairs is expensive on mobile, so disable it
>-    Weave.Service.keyGenEnabled = false;
>+    if (Weave.Status.checkSetup() != Weave.CLIENT_NOT_CONFIGURED)
>+      Weave.Service.keyGenEnabled = false;

this feels like something we should have as a pref-based property, and not set it on every startup.  Then we don't need this call here, and it's just a pref for fennec trunk.

Comment 17

[Philipp von Weitershausen [:philikon]]

(In reply to comment #16)
> this feels like something we should have as a pref-based property, and not set
> it on every startup.  Then we don't need this call here, and it's just a pref
> for fennec trunk.

Filed follow-up bug 590743

Comment 18

[Philipp von Weitershausen [:philikon]]

http://hg.mozilla.org/services/fx-sync/pushloghtml?changeset=4ca292df057c

part1: http://hg.mozilla.org/services/fx-sync/rev/eca588ee0c21
part2: http://hg.mozilla.org/services/fx-sync/rev/75f4d80c94d5
part3: http://hg.mozilla.org/services/fx-sync/rev/b9657bce354f
part4: http://hg.mozilla.org/services/fx-sync/rev/422ce92d2192
part5: http://hg.mozilla.org/services/fx-sync/rev/4ca292df057c