Closed Bug 755375 Opened 12 years ago Closed 12 years ago

Implement AITC Storage layer

Categories

(Web Apps Graveyard :: AppsInTheCloud, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: anant, Assigned: anant)

References

Details

(Whiteboard: [qa!])

Attachments

(1 file, 4 obsolete files)

Attached patch AITC Storage RC1 (obsolete) — Splinter Review
Splitting off the storage layer from bug 754538.
Attachment #624100 - Flags: review?(gps)
Comment on attachment 624100 [details] [diff] [review]
AITC Storage RC1

Review of attachment 624100 [details] [diff] [review]:
-----------------------------------------------------------------

Nice tests!

My brain checked out 2/3 through storage.js and I didn't get to test_storage_registry.js as well.

From what I got to, this is very close to landing! I will hopefully have the review finished within 36 hours.

::: services/aitc/modules/storage.js
@@ +30,5 @@
> + *                    callback is invoked, if you do so, none of your operations
> + *                    will be persisted on disk.
> + *
> + */
> +function AitcQueue(filename, cb) {

if (!cb) { throw...

@@ +79,5 @@
> +          return;
> +        }
> +        // Write unsuccessful, don't add to queue.
> +        self._queue.pop();
> +        cb(new Error(err), false);

Should already be Error. See comment below.

@@ +80,5 @@
> +        }
> +        // Write unsuccessful, don't add to queue.
> +        self._queue.pop();
> +        cb(new Error(err), false);
> +      });

style nit: invert order so error handling happens first.

@@ +125,5 @@
> +   * Return the object at the front of the queue without removing it.
> +   */
> +  peek: function peek() {
> +    if (!this._queue.length) {
> +      throw new Error("Queue is empty");

Consider returning null instead. Exceptions don't feel right to me.

Or, you could just return this._queue[0], which would return undefined, which has the same effect.

@@ +142,5 @@
> +  /**
> +   * Get contents of cache file and parse it into an array. Will throw an
> +   * exception if there is an error while reading the file.
> +   */
> +  _getFile: function _getFile(cb) {

You may be interested in CommonUtils.json{Load,Save}, which were recently factored out of Sync.

@@ +178,5 @@
> +      throw new Error("_putFile already in progress");
> +    }
> +
> +    this._writeLock = true;
> +    try {

CommonUtils.jsonSave?

@@ +197,5 @@
> +          cb(null);
> +        } else {
> +          let msg = "asyncCopy failed with " + result;
> +          self._log.info(msg);
> +          cb(msg);

cast to Error

@@ +239,5 @@
> +    DOMApplicationRegistry.getAllWithoutManifests(
> +      function _processAppsGotLocalApps(localApps) {
> +        self._processApps(remoteApps, localApps, callback);
> +      }
> +    );

If you changed _processApps to _processApps(remoteApps, callback), you could write this as:

DOMApplicationRegistry.getAllWithoutManifests(this._processApps.bind(this, remoteApps, callback));

@@ +252,5 @@
> +    let localApps = {};
> +    
> +    // Convert lApps to a dictionary of origin -> app (instead of id -> app).
> +    for (let [id, app] in Iterator(lApps)) {
> +      app.id = id;

You are modifying the thing coming from DOMApplicationRegistry.getAllWithoutManifests. Since this is a reference, is it OK to modify? (I /think/ we may have gone over this already.) If it is, please write a comment somewhere in this file stating it is acceptable to mutate this result without ill side-effects.

@@ +268,5 @@
> +      }
> +
> +      // If there is a remote app that isn't local or 
> +      // if the remote app was installed later.
> +      let id = null;

Do you absolutely need id to be null? The default value is undefined if there is no assignment. Will that do?

@@ +275,5 @@
> +      }
> +      if (localApps[origin] &&
> +          (localApps[origin].installTime < app.installTime)) {
> +        id = localApps[origin].id;
> +      }

Somewhere around here my brain shut off. Will pick up this file later...

::: services/aitc/tests/unit/head_helpers.js
@@ +21,5 @@
> +
> +  return server;
> +}
> +
> +function httpd_handler(statusCode, status, body) {

I don't think you actually use this function.

::: services/aitc/tests/unit/test_storage_queue.js
@@ +3,5 @@
> +
> +Cu.import("resource://services-aitc/storage.js");
> +Cu.import("resource://services-common/async.js");
> +
> +var queue = null;

let

Strictly speaking, individuals tests should be free-standing. i.e. little to no usage of global variables.

We don't have a very good test running framework for xpcshell tests, so I really can't fault you too hard. If you find time to refactor this so a separate queue instance is used for each test, that would be splendid.

@@ +11,5 @@
> +}
> +
> +add_test(function test_queue_create() {
> +  do_check_eq(queue._queue.length, 0);
> +  do_check_eq(queue._writeLock, false);

And if you did refactor to use separate queues, these silly checks could go away, I think.

@@ +16,5 @@
> +
> +  try {
> +    queue.enqueue("foo");
> +  } catch (e) {
> +    do_check_eq(e.toString(), "Error: enqueue called without callback");

This doesn't actually test anything. There are some test helpers that ensure an exception is raised. I /think/ they are in the services/common test_helpers.js file now. I could be wrong.

@@ +22,5 @@
> +
> +  try {
> +    queue.dequeue();
> +  } catch (e) {
> +    do_check_eq(e.toString(), "Error: dequeue called without callback");

ditto.
Comment on attachment 624100 [details] [diff] [review]
AITC Storage RC1

Review of attachment 624100 [details] [diff] [review]:
-----------------------------------------------------------------

Looking pretty good. Just a few things to address.

::: services/aitc/modules/storage.js
@@ +246,5 @@
> +  /**
> +   * Take a list of remote and local apps and figured out what changes (if any)
> +   * are to be made to the local DOMApplicationRegistry.
> +   */
> +  _processApps: function _processApps(remoteApps, lApps, callback) {

This is one of the most important parts of the AITC code. And, reconciling data is hard. The last time I touched Sync's core reconciling logic, I had 3 reviewers (every person with review power). And, I believe they each found something.

I will insist that at least this function have multiple reviewers. I would also like to see more documentation. IMO, this function cannot be over documented. For reference, see https://hg.mozilla.org/services/services-central/file/e794cef56df6/services/sync/modules/engines.js#l1025.

@@ +253,5 @@
> +    
> +    // Convert lApps to a dictionary of origin -> app (instead of id -> app).
> +    for (let [id, app] in Iterator(lApps)) {
> +      app.id = id;
> +      toDelete[app.origin] = app;

This feels dangerous...

@@ +264,5 @@
> +      // Don't delete apps that are both local & remote.
> +      let origin = app.origin;
> +      if (!app.deleted) {
> +        delete toDelete[origin];
> +      }

Because what happens when you call this function with an empty set of remote apps? I haven't looked at the logic that calls this API, but the client may return a non-error result with an empty set. From client.js:

if (!this._isRequestAllowed()) {
  cb(null, null);
  return;
}

If that makes its way into this API, you just deleted everything.

@@ +301,5 @@
> +  _applyUpdates: function _applyUpdates(toInstall, toUninstall, callback) {
> +    let finalCommands = [];
> +
> +    let self = this;
> +    function onManifestsUpdated() {

let onManifestsUpdated = function onManifestsUpdated() { ... }.bind(this);

@@ +304,5 @@
> +    let self = this;
> +    function onManifestsUpdated() {
> +      if (toUninstall.length) {
> +        finalCommands = finalCommands.concat(toUninstall);
> +      }

This doesn't seem right to me. This callback may be executed multiple times. And, if toUninstall has elements, this will get executed every single time. Do you mean to only roll toUninstall into finalCommands once? If so, please move it outside of this callback. If not, please document what's going on so it is easier to grok.

@@ +314,5 @@
> +      } else {
> +        self._log.info(
> +          "processUpdates finished fetching, no finalCommands were found"
> +        );
> +        callback();

You're not passing results to the callback?

@@ +315,5 @@
> +        self._log.info(
> +          "processUpdates finished fetching, no finalCommands were found"
> +        );
> +        callback();
> +      }

onManifestsUpdated only gets called once, right? Shouldn't callback always be invoked at least once?

Style nit: invert logic and prefer early return.

@@ +348,5 @@
> +        // Not a big deal if we couldn't get a manifest, we will try to fetch
> +        // it again in the next cycle. Carry on.
> +        done += 1;
> +        if (done == total) {
> +          onManifestsUpdated();

So, you are gating on complete fetching before you call into DOMApplicationRegistry. I want you to think about this carefully. Is waiting really the best option? Could DOMApplicationRegistry be called sooner, as data becomes available, for example? What is the end-user experience if one manifest site is slowly responding?

@@ +357,5 @@
> +
> +  /**
> +   * Fetch a manifest from given URL. No retries are made on failure.
> +   */
> +  _getManifest: function _getManifest(url, callback)  {

This doesn't seem specific to AITC. Is this the right place for it?

@@ +361,5 @@
> +  _getManifest: function _getManifest(url, callback)  {
> +    let req = new RESTRequest(url);
> +
> +    let self = this;
> +    req.get(function(error) {

You probably want to define an explicit timeout. Otherwise, you could be waiting a long time.

@@ +371,5 @@
> +        callback(new Error("Non-200 while fetching manifest"), null);
> +        return;
> +      }
> +
> +      let err = null;

Nit: Having "err" and "error" in the same block is confusing.

@@ +396,5 @@
> +};
> +
> +XPCOMUtils.defineLazyGetter(this, "AitcStorage", function() {
> +  return new AitcStorageImpl();
> +});
\ No newline at end of file

Do you need a singleton here or will an instance be attached to a higher-level instance (like the AITC Manager)? The fewer singletons you create, the better.

::: services/aitc/tests/unit/test_storage_registry.js
@@ +5,5 @@
> +Cu.import("resource://services-aitc/storage.js");
> +
> +const SERVER = "http://localhost";
> +
> +var fakeApp1 = {

let

@@ +11,5 @@
> +  receipts: [],
> +  manifestURL: "/manifest.webapp",
> +  installOrigin: "http://localhost",
> +  installedAt: Date.now(),
> +  modifiedAt: Date.now()

Does it matter these may or may not be the same value?

@@ +40,5 @@
> +};
> +
> +// Valid manifest for app2
> +var manifest2_good = {
> +  name: "Supercalifragilisticexpialidocious",

What are you talking about, man? There's no such word!

@@ +66,5 @@
> +    let manifest = JSON.stringify(manifest2_good);
> +    res.setStatusLine(req.httpVersion, 200, "OK");
> +    res.setHeader("Content-Type", "application/x-web-app-manifest+json");
> +    res.bodyOutputStream.write(manifest, manifest.length);
> +  }}, 8083);

Nit: DRY violation. Consider writing a helper function to send manifest responses.
Attachment #624100 - Flags: review?(gps)
Blocks: 757261
No longer blocks: 757261
Nominating for k9o - this is part of the AITC client implementation
blocking-kilimanjaro: --- → ?
Removing nom - we'll just track the higher level tracking bug for AITC desktop for simplicity.
blocking-kilimanjaro: ? → ---
Attached patch AITC Storage RC2 (obsolete) — Splinter Review
We no longer need to update manifests (DOMApplicationRegistry does it), this simplifies the logic dramatically. I added more documentation, and fixed most comments.

I can't use CommonUtils.jsonLoad because it doesn't tell me if the write actually succeeded or not.
Attachment #624100 - Attachment is obsolete: true
Attachment #628042 - Flags: review?(mconnor)
Attachment #628042 - Flags: review?(gps)
(In reply to Gregory Szorc [:gps] from comment #2)
> @@ +314,5 @@
> > +      } else {
> > +        self._log.info(
> > +          "processUpdates finished fetching, no finalCommands were found"
> > +        );
> > +        callback();
> 
> You're not passing results to the callback?

No, the caller cannot meaningfully act on success or failure. We can add better error handling and user notification in a later patch.

> @@ +396,5 @@
> > +};
> > +
> > +XPCOMUtils.defineLazyGetter(this, "AitcStorage", function() {
> > +  return new AitcStorageImpl();
> > +});
> \ No newline at end of file
> 
> Do you need a singleton here or will an instance be attached to a
> higher-level instance (like the AITC Manager)? The fewer singletons you
> create, the better.

DOMApplicationRegistry is a singleton so there is no point to making this a class. We will change this later, when DOMApplicatoinRegistry supports personas  (bug 746204).

> @@ +11,5 @@
> > +  receipts: [],
> > +  manifestURL: "/manifest.webapp",
> > +  installOrigin: "http://localhost",
> > +  installedAt: Date.now(),
> > +  modifiedAt: Date.now()
> 
> Does it matter these may or may not be the same value?

Nope.
Attached patch AITC Storage RC3 (obsolete) — Splinter Review
Ahoy! Behold, a new version of the patch with manifest fetching goodness. We fetch the manifests in parallel so we can add each one to the local registry individually as they come along. We need tests, you say? Why yes indeed, they all pass!
Attachment #628042 - Attachment is obsolete: true
Attachment #628042 - Flags: review?(mconnor)
Attachment #628042 - Flags: review?(gps)
Attachment #628153 - Flags: review?(mconnor)
Attachment #628153 - Flags: review?(gps)
(In reply to Anant Narayanan [:anant] from comment #5)
> We no longer need to update manifests (DOMApplicationRegistry does it), this
> simplifies the logic dramatically. I added more documentation, and fixed
> most comments.

\o/
 
> I can't use CommonUtils.jsonLoad because it doesn't tell me if the write
> actually succeeded or not.

You can always file a feature request...
Attachment #628153 - Attachment is patch: true
Comment on attachment 628153 [details] [diff] [review]
AITC Storage RC3

Review of attachment 628153 [details] [diff] [review]:
-----------------------------------------------------------------

Didn't review the reconciling algorithm in storage.js because I am extremely tired.

::: services/aitc/modules/storage.js
@@ +48,5 @@
> +  this._log.info("AitcQueue instance loading");
> +
> +  let self = this;
> +  if (this._file.exists()) {
> +    this._getFile(function _gotFile(data) {

No need for leading _.

@@ +50,5 @@
> +  let self = this;
> +  if (this._file.exists()) {
> +    this._getFile(function _gotFile(data) {
> +      if (data) {
> +        self._queue = data;

You may want to Array.isArray() before assignment.

@@ +62,5 @@
> +  }
> +}
> +AitcQueue.prototype = {
> +  /**
> +   * Add an object to the queue.

Please also document that it saves the new data to disk.

@@ +93,5 @@
> +    }    
> +  },
> +
> +  /**
> +   * Remove the object at the head of the queue.

Document is saves.

@@ +130,5 @@
> +   * Return the object at the front of the queue without removing it.
> +   */
> +  peek: function peek() {
> +    if (!this._queue.length) {
> +      throw new Error("Queue is empty");

I'm not convinced raising is the right action here. I would return null instead. But, it's your API.

@@ +132,5 @@
> +  peek: function peek() {
> +    if (!this._queue.length) {
> +      throw new Error("Queue is empty");
> +    }
> +    this._log.info("Peek returning head of queue");

Of course it did. Maybe just "Peek returned data." Or even better, in the beginning of the function, "Peek called when length " + this._queue-length

@@ +139,5 @@
> +
> +  /**
> +   * Find out the length of the queue.
> +   */
> +  length: function length(cb) {

get length() { ... }

Everything (AFAIK) in JS exposes length as a property, not a function.

@@ +209,5 @@
> +    } catch (e) {
> +      this._writeLock = false;
> +      cb(msg);
> +    }
> +  },

That's like 60 lines you shouldn't have to reinvent. There's got to be a better way. For the record, the fact it takes this many lines of chrome-privileged JS to JSON I/O is a complete joke. It makes me angry. But, I won't take it out on you :)

@@ +264,5 @@
> +   *    4c. If remote app is not marked as deleted and isn't present locally,
> +   *        add to the "to be installed" set.
> +   *  5. For each app either in the "to be installed" or "to be deleted" set,
> +   *     apply the changes locally. For apps to be installed, we must also
> +   *     fetch the manifest.

\o/ Great comments!

::: services/aitc/tests/unit/head_global.js
@@ +7,1 @@
>  do_load_httpd_js();

This is deprecated after bug 755196 lands. Please Cu.import("resource://testing-common/httpd.js") instead. And, you don't need to do that in head_global.js.

@@ +70,5 @@
> +    do_print("Is there a process already listening on port " + port + "?");
> +    do_print("==========================================");
> +    do_throw(ex);
> +  }
> +}

Again, this file is 95% similar to services/common/head_global.js. The purpose of the "global" [head] file is that it is shared amongst everything. Either reference the existing head_global.js or change this to head_aitc.js or something.

::: services/aitc/tests/unit/test_storage_queue.js
@@ +3,5 @@
> +
> +Cu.import("resource://services-aitc/storage.js");
> +Cu.import("resource://services-common/async.js");
> +
> +let queue = null;

Nit: No need to assign null.

Also, having a global queue instance breaks the concept of multiple tests. But, I'll let it slide. Just beware that things like deterministic test execution order may not always work. It is best to have setUp() and tearDown() functions or multiple queue instances for each test. You can easily write a synchronous helper to aid with this.

@@ +17,5 @@
> +  try {
> +    queue.enqueue("foo");
> +  } catch (e) {
> +    do_check_eq(e.toString(), "Error: enqueue called without callback");
> +  }

This doesn't actually ensure the exception was thrown! We have a helper in services/common/tests/unit/head_helpers.js called ensureThrows() which can help. There is also do_throw() from the xpcshell test scope.

I see you don't list head_helpers() as a head file, so I guess it isn't available. I can easily move these functions into testing-only JS modules if you need them. (All [head] files should probably be modules.)

@@ +23,5 @@
> +  try {
> +    queue.dequeue();
> +  } catch (e) {
> +    do_check_eq(e.toString(), "Error: dequeue called without callback");
> +  }

Ditto.

@@ +29,5 @@
> +  run_next_test();
> +});
> +
> +add_test(function test_queue_enqueue() {
> +  // Add to queue

Nit: Periods on all sentences. Nit exists throughout file.

@@ +33,5 @@
> +  // Add to queue
> +  let testObj = {foo: "bar"};
> +  queue.enqueue(testObj, function(err, done) {
> +    do_check_eq(err, null);
> +    do_check_eq(done, true);

do_check_true(done);

@@ +65,5 @@
> +  let items = [{test:"object"}, "teststring", 42];
> +
> +  // Two random numbers: how many items to queue and how many to remove
> +  let num = Math.floor(Math.random() * 100);
> +  let rem = Math.floor(Math.random() * num);

What if Math.random() returns 0?

@@ +69,5 @@
> +  let rem = Math.floor(Math.random() * num);
> +
> +  // First insert all the items we will remove later
> +  for (let i = 0; i < rem; i++) {
> +    let ins = items[Math.round(Math.random() * 2)];

This math isn't sound. See https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Math/random for proper, normally distributed values in a range. It's arguably OK for test code. But, I thought I'd point it out so you don't make this mistake on real code.

@@ +72,5 @@
> +  for (let i = 0; i < rem; i++) {
> +    let ins = items[Math.round(Math.random() * 2)];
> +    let cb = Async.makeSpinningCallback();
> +    queue.enqueue(ins, cb);
> +    do_check_eq(cb.wait(), true);

do_check_true.

@@ +79,5 @@
> +  do_check_eq(queue.length(), rem);
> +
> +  // Now insert the items we won't remove
> +  let check = [];
> +  for (let i = 0; i < (num - rem); i++) {

Nit: It is (hopefully) optimized, but a performance zealot may want you to write as: for (let i = num - rem; i; i--)

The theory is the loop continuation expression is much simpler to evaluate. Hopefully the compiler realizes num and rem aren't touched during the loop and figures this out automagically.

@@ +80,5 @@
> +
> +  // Now insert the items we won't remove
> +  let check = [];
> +  for (let i = 0; i < (num - rem); i++) {
> +    check.push(items[Math.round(Math.random() * 2)]);

suspect math again

@@ +111,5 @@
> +});
> +
> +add_test(function test_queue_writelock() {
> +  // Queue should not enqueue or dequeue if lock is enabled
> +  queue._writeLock = true;

It would be preferred to test this through the actual mechanism that sets queue._writeLock. But, that could be difficult without monkeypatching. If you are up for it...

::: services/aitc/tests/unit/test_storage_registry.js
@@ +63,5 @@
> +
> +function create_servers() {
> +  // Setup servers to server manifests at each port
> +  let manifests = [manifest1, manifest2_bad, manifest2_good, manifest3];
> +  for (let i = 0; i < manifests.length; i++) {

Pro tip: you can now "for (let manifest of manifests)" \o/

@@ +69,5 @@
> +    httpd_setup({"/manifest.webapp": function(req, res) {
> +      res.setStatusLine(req.httpVersion, 200, "OK");
> +      res.setHeader("Content-Type", "application/x-web-app-manifest+json");
> +      res.bodyOutputStream.write(response, response.length);
> +    }}, 8080 + i);

Please make 8080 a constant. In services/common/tests/unit/head_helpers.js, there is a get_server_port(). Ideally that would keep track of in-use ports. For now, you can just call and increment the result.

@@ +119,5 @@
> +    do_check_eq(DOMApplicationRegistry.itemExists(id3), true);
> +    do_check_eq(DOMApplicationRegistry._appId(fakeApp2.origin), null);
> +    run_next_test();
> +  });
> +});
\ No newline at end of file

I think there are more scenarios that could be tested.
Attached patch AITC Storage RC4 (obsolete) — Splinter Review
(In reply to Gregory Szorc [:gps] from comment #9)
> ::: services/aitc/modules/storage.js
> @@ +48,5 @@
> > +  this._log.info("AitcQueue instance loading");
> > +
> > +  let self = this;
> > +  if (this._file.exists()) {
> > +    this._getFile(function _gotFile(data) {
> 
> No need for leading _.

Why not? It's a private function. Or do you mean for _gotFile?

> @@ +50,5 @@
> > +  let self = this;
> > +  if (this._file.exists()) {
> > +    this._getFile(function _gotFile(data) {
> > +      if (data) {
> > +        self._queue = data;
> 
> You may want to Array.isArray() before assignment.

The array is only manipulated by AitcQueue.

> @@ +62,5 @@
> > +  }
> > +}
> > +AitcQueue.prototype = {
> > +  /**
> > +   * Add an object to the queue.
> 
> Please also document that it saves the new data to disk.

I felt this was implied by the definition above, a "file-backed persistent queue" :)

> ::: services/aitc/tests/unit/head_global.js
> @@ +7,1 @@
> >  do_load_httpd_js();
> 
> This is deprecated after bug 755196 lands. Please
> Cu.import("resource://testing-common/httpd.js") instead. And, you don't need
> to do that in head_global.js.

I've imported head_global.js from services-common, so hopefully I don't have to worry about this.

> Also, having a global queue instance breaks the concept of multiple tests.
> But, I'll let it slide. Just beware that things like deterministic test
> execution order may not always work. It is best to have setUp() and
> tearDown() functions or multiple queue instances for each test. You can
> easily write a synchronous helper to aid with this.

I didn't want to add an element to the queue only to test dequeue, since we need to test adding to the queue anyway... and so on. That would make for very verbose tests. Mostly the whole file is one big test ;)

> @@ +17,5 @@
> > +  try {
> > +    queue.enqueue("foo");
> > +  } catch (e) {
> > +    do_check_eq(e.toString(), "Error: enqueue called without callback");
> > +  }
> 
> This doesn't actually ensure the exception was thrown! We have a helper in
> services/common/tests/unit/head_helpers.js called ensureThrows() which can
> help. There is also do_throw() from the xpcshell test scope.

That's not what I'm testing for. The callbacks provided to AitcQueue don't throw exceptions, because they're in the test. I'm testing for synchronous exceptions thrown when invalid arguments are provided. It's a pretty meaningless test. I don't even know why I put it there.

> @@ +65,5 @@
> > +  let items = [{test:"object"}, "teststring", 42];
> > +
> > +  // Two random numbers: how many items to queue and how many to remove
> > +  let num = Math.floor(Math.random() * 100);
> > +  let rem = Math.floor(Math.random() * num);
> 
> What if Math.random() returns 0?

Good catch, I added +1 at the end so the range is 1-100.

> @@ +69,5 @@
> > +  let rem = Math.floor(Math.random() * num);
> > +
> > +  // First insert all the items we will remove later
> > +  for (let i = 0; i < rem; i++) {
> > +    let ins = items[Math.round(Math.random() * 2)];
> 
> This math isn't sound. See
> https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Math/
> random for proper, normally distributed values in a range. It's arguably OK
> for test code. But, I thought I'd point it out so you don't make this
> mistake on real code.

Yes, I'm twice as likely to get index 1 than 0 or 2.

> ::: services/aitc/tests/unit/test_storage_registry.js
> @@ +63,5 @@
> > +
> > +function create_servers() {
> > +  // Setup servers to server manifests at each port
> > +  let manifests = [manifest1, manifest2_bad, manifest2_good, manifest3];
> > +  for (let i = 0; i < manifests.length; i++) {
> 
> Pro tip: you can now "for (let manifest of manifests)" \o/

But I need my 'i' to add to the port! Nice we can finally do that now.
Attachment #628153 - Attachment is obsolete: true
Attachment #628153 - Flags: review?(mconnor)
Attachment #628153 - Flags: review?(gps)
Attachment #628610 - Flags: review?(mconnor)
Attachment #628610 - Flags: review?(gps)
(In reply to Anant Narayanan [:anant] from comment #10)
> (In reply to Gregory Szorc [:gps] from comment #9)
> > ::: services/aitc/modules/storage.js
> > @@ +48,5 @@
> > > +  this._log.info("AitcQueue instance loading");
> > > +
> > > +  let self = this;
> > > +  if (this._file.exists()) {
> > > +    this._getFile(function _gotFile(data) {
> > 
> > No need for leading _.
> 
> Why not? It's a private function. Or do you mean for _gotFile?

Yes, _gotFile.
 
> > @@ +50,5 @@
> > > +  let self = this;
> > > +  if (this._file.exists()) {
> > > +    this._getFile(function _gotFile(data) {
> > > +      if (data) {
> > > +        self._queue = data;
> > 
> > You may want to Array.isArray() before assignment.
> 
> The array is only manipulated by AitcQueue.

Today it is an Array. I could see a future where you decide to change the on-disk format to an object, let's say. And, if the application is downgraded, this could break horribly. I guess if we ever do that, we'll have to change the filename. Or, you could make the serialized structure be an object with a version property.

> 
> > @@ +62,5 @@
> > > +  }
> > > +}
> > > +AitcQueue.prototype = {
> > > +  /**
> > > +   * Add an object to the queue.
> > 
> > Please also document that it saves the new data to disk.
> 
> I felt this was implied by the definition above, a "file-backed persistent
> queue" :)

Yes. But, the semantics of when the queue is persisted aren't called out.

> I didn't want to add an element to the queue only to test dequeue, since we
> need to test adding to the queue anyway... and so on. That would make for
> very verbose tests. Mostly the whole file is one big test ;)

I hear ya. That's why I'd create a helper that returns a pre-populated AitcQueue for tests that need to operate on it.

> 
> > @@ +17,5 @@
> > > +  try {
> > > +    queue.enqueue("foo");
> > > +  } catch (e) {
> > > +    do_check_eq(e.toString(), "Error: enqueue called without callback");
> > > +  }
> > 
> > This doesn't actually ensure the exception was thrown! We have a helper in
> > services/common/tests/unit/head_helpers.js called ensureThrows() which can
> > help. There is also do_throw() from the xpcshell test scope.
> 
> That's not what I'm testing for. The callbacks provided to AitcQueue don't
> throw exceptions, because they're in the test. I'm testing for synchronous
> exceptions thrown when invalid arguments are provided. It's a pretty
> meaningless test. I don't even know why I put it there.

No, I mean the test doesn't actually verify that the do_check_eq() in the catch block is even executed. If enqueue() doesn't throw (it currently does b/c cb is not defined), the test will still pass! Try commenting out the throw in enqueue() and you will see this.
Fixed all the things.

(In reply to Gregory Szorc [:gps] from comment #11)
> > That's not what I'm testing for. The callbacks provided to AitcQueue don't
> > throw exceptions, because they're in the test. I'm testing for synchronous
> > exceptions thrown when invalid arguments are provided. It's a pretty
> > meaningless test. I don't even know why I put it there.
> 
> No, I mean the test doesn't actually verify that the do_check_eq() in the
> catch block is even executed. If enqueue() doesn't throw (it currently does
> b/c cb is not defined), the test will still pass! Try commenting out the
> throw in enqueue() and you will see this.

D'oh! Clearly I left my brain at work last night.
Attachment #628610 - Attachment is obsolete: true
Attachment #628610 - Flags: review?(mconnor)
Attachment #628610 - Flags: review?(gps)
Attachment #629059 - Flags: review?(mconnor)
Attachment #629059 - Flags: review?(gps)
Patch requires bug 754538.
Status: NEW → ASSIGNED
Depends on: 754538
Depends on: 760448
Comment on attachment 629059 [details] [diff] [review]
AITC Storage RC4.1

Review of attachment 629059 [details] [diff] [review]:
-----------------------------------------------------------------

Partial review.

::: services/aitc/modules/storage.js
@@ +209,5 @@
> +    } catch (e) {
> +      this._writeLock = false;
> +      cb(msg);
> +    }
> +  },

I filed and landed bug 760448 for the functionality you need. Please use CommonUtils.json{Load,Save}.

::: services/aitc/services-aitc.js
@@ +1,2 @@
>  pref("services.aitc.client.log.level", "Debug");
> +pref("services.aitc.storage.log.level", "Debug");pref("services.aitc.client.timeout", 120);
\ No newline at end of file

\ No newline at end of file

Newline.

::: services/common/tests/unit/head_global.js
@@ +46,5 @@
>    Cu.import("resource://gre/modules/Services.jsm");
>    const handler = Services.io.getProtocolHandler("resource")
>                    .QueryInterface(Ci.nsIResProtocolHandler);
>  
> +  let modules = ["aitc", "common", "crypto"];

You might want to change this in bug 754538 instead.
So, this is the patch I am most squeamish about landing. But it's the next one in my local queue... I'd like to request a retro-active review for this file only.

I spent a little while moving over to CommonUtils.jsonSave/jsonLoad, but the tests seem to have run a bit slower. That doesn't make much sense since the old and new code are pretty much the same, but I'd still prefer making that change as a follow-up. I filed Bug 760896.

The other big concern with this patch was the reconciliation algorithm. I filed Bug 760895 to make sure we don't forget.

I'm going to leave the review flags on, since I realize you might not have read the entire code (Greg has, at-least once, but Mike hasn't). Please file any issues you find as followup bugs!
https://hg.mozilla.org/mozilla-central/rev/bca65d63b382
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in services]
Whiteboard: [qa+]
Verified on Nightly on Windows 7 through doing an initial test pass on AITC desktop. I can confirm that this has indeed landed. All follow-up issues should be filed in separate bugs.
Status: RESOLVED → VERIFIED
Whiteboard: [qa+] → [qa!]
Comment on attachment 629059 [details] [diff] [review]
AITC Storage RC4.1

Bug landed, clearing flags.
Attachment #629059 - Flags: review?(mconnor)
Attachment #629059 - Flags: review?(gps)
Product: Web Apps → Web Apps Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: