Closed Bug 946294 Opened 7 years ago Closed 7 years ago

Use sequences in the Contacts interfaces

Categories

(Core Graveyard :: DOM: Contacts, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla29

People

(Reporter: reuben, Assigned: reuben)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

Bug 942631 finally allows us to have real arrays \o/
Depends on: 946898
Depends on: 946909
Need deps before tests pass.
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.
Attachment #8343334 - Flags: feedback?(anygregor)
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?
(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…
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.
Duplicate of this bug: 938261
Blocks: 949415
No longer blocks: 949415
Blocks: 949197
Attachment #8343334 - Attachment is obsolete: true
Attachment #8343334 - Flags: feedback?(anygregor)
Incorporates the changes to save() needed to make sure we validate everything in the sequences before we save the contact.
Attachment #8349615 - Flags: review?(bzbarsky)
Comment on attachment 8349615 [details] [diff] [review]
Use sequences for the array attributes, v2

r=me, though I only skimmed the test changes.
Attachment #8349615 - Flags: review?(bzbarsky) → review+
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.
Attachment #8349615 - Flags: review?(anygregor)
(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.
Flags: needinfo?(jonas)
Flags: needinfo?(ehsan)
I can't think of another difference except for save (and also remove) to start throwing if handed off an invalid object.
Flags: needinfo?(ehsan)
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.
Flags: needinfo?(jonas)
Attachment #8349615 - Flags: review?(anygregor) → review+
Keywords: checkin-needed
Ugh. I forgot I added a test.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/583a039e34f9
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Whiteboard: [qa-]
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.