946294 - Use sequences in the Contacts interfaces

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Contacts, defect)

Description

[Reuben Morais [:reuben]]

Bug 942631 finally allows us to have real arrays \o/

Comment 1

[Reuben Morais [:reuben]]

Attachment: Use sequences for the array attributes

Need deps before tests pass.

Comment 2

[Reuben Morais [:reuben]]

Comment on attachment 8343334 [details] [diff] [review]
Use sequences for the array attributes

I did some tests using find() and 1500 contacts and this makes us ~8% faster, probably due to the manual validation with proxies that can be removed now.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hmm.

save() might still need to do some sort of validation.  In particular, unfortunately nothing guarantees that aContact.tel will only contain vanilla JS reflections of ContactTelField.  It could have whatever the page stuck in there, right?

Comment 4

[Reuben Morais [:reuben]]

(In reply to Boris Zbarsky [:bz] from comment #3)
> Hmm.
> 
> save() might still need to do some sort of validation.  In particular,
> unfortunately nothing guarantees that aContact.tel will only contain vanilla
> JS reflections of ContactTelField.

I hope not! Right now if I try to do something like this:

  var c = new mozContact({tel: []});
  c.tel.push({aeiou: "abcde"});

I get an empty object in save(). As long as there are no cross-compartment shenanigans, I think this is fine. There's only so much hand-holding we can do with the API users…

Comment 5

[Boris Zbarsky [:bzbarsky]]

Reuben and I talked that part over.  We do in fact need to do validation, but this will do it fine:

    try {
      for (let field of PROPERTIES) {
        aContact[field] = aContact[field];
      }
    } catch (e) {
      throw this._window.DOMError(e.name, e.message);
    }

with some comments explaining why the "no-op" assignment and why the weird throwing behavior.

Comment 7

[Reuben Morais [:reuben]]

Attachment: Use sequences for the array attributes, v2

Incorporates the changes to save() needed to make sure we validate everything in the sequences before we save the contact.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8349615 [details] [diff] [review]
Use sequences for the array attributes, v2

r=me, though I only skimmed the test changes.

Comment 9

[Reuben Morais [:reuben]]

Comment on attachment 8349615 [details] [diff] [review]
Use sequences for the array attributes, v2

Thanks! Gregor, can you take a look? Specifically any unwanted changes to the behavior? The test changes cover the obvious ones. Something new that we may want to add a test for is the fact that save() can now throw if you pass a mozContact with invalid data.

Comment 10

[Gregor Wagner [:gwagner]]

(In reply to :Reuben Morais [slow to respond until Jan 10] from comment #9)
> Comment on attachment 8349615 [details] [diff] [review]
> Use sequences for the array attributes, v2
> 
> Thanks! Gregor, can you take a look? Specifically any unwanted changes to
> the behavior? The test changes cover the obvious ones. Something new that we
> may want to add a test for is the fact that save() can now throw if you pass
> a mozContact with invalid data.

Hm thats a good question. I guess it should be similar to other APIs we design.
Ehsan, Jonas, any comments here?

But if it throws we should at least add a test case for it.

Comment 11

[(no longer active)]

I can't think of another difference except for save (and also remove) to start throwing if handed off an invalid object.

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Throwing if you attempt to save something that isn't a valid contact sounds fine to me. As long as we are backwards compatible with old code.

I think we should even throw if the page attempts to use a simple string value where an array of strings is what the API normally uses. Though it's probably too late to make such a change now.

Comment 13

[Reuben Morais [:reuben]]

Ugh. I forgot I added a test.

Comment 14

[Reuben Morais [:reuben]]

https://hg.mozilla.org/integration/b2g-inbound/rev/583a039e34f9

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/583a039e34f9