951766 - [Contacts API] Reimplement the "mozContact.prototype.init" method for the sake of backward compatibility

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Contacts, defect)

Description

[Julien Wajsberg [:julienw]]

From this thread: https://groups.google.com/forum/#!topic/mozilla.dev.webapi/0a9bdLl8-7Q

The move to WebIDL removed the `init` method.

Asking 1.3? as there is no point in not blocking.

Comment 1

[Reuben Morais [:reuben]]

Attachment: Reintroduce mozContact.init with a deprecation warning

Comment 2

[Reuben Morais [:reuben]]

Comment on attachment 8349730 [details] [diff] [review]
Reintroduce mozContact.init with a deprecation warning

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

::: dom/contacts/ContactManager.js
@@ +49,5 @@
> +    // init is deprecated, warn once in the console if it's used
> +    if (!mozContactInitWarned) {
> +      mozContactInitWarned = true;
> +      Cu.reportError("mozContact.init is DEPRECATED. Use the mozContact constructor instead. " +
> +                     "See https://developer.mozilla.org/docs/WebAPI/Contacts for details.");

Ideally we'd report a real warning here but that's tricky to do from JS and doesn't get us much (no file/line number info).

Comment 3

[Julien Wajsberg [:julienw]]

Comment on attachment 8349730 [details] [diff] [review]
Reintroduce mozContact.init with a deprecation warning

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

Does not look like a central or 1.3 patch?

On central we already have a "init" method in Contact.prototype so you'll need to rename it, I guess it's possible with webIDL?

Because of this I'd like to wait before giving r+ :) But the general thing looks good to me.

::: dom/contacts/ContactManager.js
@@ +54,5 @@
> +    }
> +
> +    for (let prop in aProp) {
> +      this[prop] = aProp[prop];
> +    }

could you just call __init?

::: dom/webidl/Contacts.webidl
@@ +90,5 @@
>    [Cached, Pure] attribute sequence<DOMString>?       jobTitle;
>    [Cached, Pure] attribute sequence<DOMString>?       note;
>    [Cached, Pure] attribute sequence<DOMString>?       key;
>  
> +  void init(optional ContactProperties properties);

why is it optional?

I think it was not optional in the previous implementation, and that your code is not checking that it's present.

Comment 4

[Joe Cheng [:jcheng] (please needinfo)]

do we have any user / developer impact without the .init method? thanks

Comment 5

[Julien Wajsberg [:julienw]]

Yep, I think that this is highly possible that breaking this compatibility will break third-party apps. On the mailing list there was quite an agreement about this.

If we don't block for 1.3 then this bug is useless: there is point in adding it only for 1.4, because the apps would need to be changed for 1.3.

Comment 6

[Joe Cheng [:jcheng] (please needinfo)]

traige: 1.3+ agree with comment 5

Comment 7

[Joe Cheng [:jcheng] (please needinfo)]

Hi Reuben, wonder if you will be working on this starting next week? thanks

Comment 8

[Reuben Morais [:reuben]]

(In reply to Joe Cheng [:jcheng] from comment #7)
> Hi Reuben, wonder if you will be working on this starting next week? thanks

Yep, I addressed the review comments offline but never sent the response. Sorry about that.

Comment 9

[Reuben Morais [:reuben]]

(In reply to Julien Wajsberg [:julienw] from comment #3)
> On central we already have a "init" method in Contact.prototype so you'll
> need to rename it, I guess it's possible with webIDL?

Yep, after bug 951875.

> ::: dom/contacts/ContactManager.js
> @@ +54,5 @@
> > +    }
> > +
> > +    for (let prop in aProp) {
> > +      this[prop] = aProp[prop];
> > +    }
> 
> could you just call __init?

Sure.

> ::: dom/webidl/Contacts.webidl
> @@ +90,5 @@
> >    [Cached, Pure] attribute sequence<DOMString>?       jobTitle;
> >    [Cached, Pure] attribute sequence<DOMString>?       note;
> >    [Cached, Pure] attribute sequence<DOMString>?       key;
> >  
> > +  void init(optional ContactProperties properties);
> 
> why is it optional?

Because it has to be, as a dictionary parameter that is not followed by a required parameter. Or something. WebIDL has some weird rules.

> I think it was not optional in the previous implementation, and that your
> code is not checking that it's present.

IIRC |contact.init();| in the XPIDL implementation was equivalent to |contact.init({});|. I'll have to check though. And |for (… in undefined)| just no-ops, no need to check for presence.

Comment 10

[Reuben Morais [:reuben]]

Attachment: Reintroduce mozContact.init with a deprecation warning, v2

Comment 11

[Julien Wajsberg [:julienw]]

Comment on attachment 8358798 [details] [diff] [review]
Reintroduce mozContact.init with a deprecation warning, v2

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

Sorry, I still have a doubt regarding the behavior of the init method. See below.

::: dom/contacts/ContactManager.js
@@ +52,5 @@
> +      Cu.reportError("mozContact.init is DEPRECATED. Use the mozContact constructor instead. " +
> +                     "See https://developer.mozilla.org/docs/WebAPI/Contacts for details.");
> +    }
> +
> +    this.__init(aProp);

I have a doubt: should `init` reset the state ? I mean, reset the properties that are not specified in `aProp` to `null` ? I think we should (and that's what it used to do in 1.2 and before).

Should be easy to do with the PROPERTIES array.

::: dom/contacts/tests/test_contacts_basics2.html
@@ +778,5 @@
> +    c.init({name: ["Bar"]});
> +    c.init({name: ["Bar"]});
> +    ise(c.name[0], "Bar", "Same name");
> +    SimpleTest.endMonitorConsole();
> +  },

For my peace of mind, can I have a test with a init call without argument ? `c.init()` ?

And one testing the reset to "null" of a previously initialized property when we call init without that property?

Comment 12

[Reuben Morais [:reuben]]

Attachment: Reintroduce mozContact.init with a deprecation warning, v3

Sounds good. How's this?

Comment 13

[Julien Wajsberg [:julienw]]

Comment on attachment 8359285 [details] [diff] [review]
Reintroduce mozContact.init with a deprecation warning, v3

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

r=me

Great !

Please commit with a green try :)

Comment 14

[Reuben Morais [:reuben]]

As green as it gets, I guess: https://tbpl.mozilla.org/?tree=Try&rev=598e73a31e6c

Comment 15

[Julien Wajsberg [:julienw]]

Yes.

Comment 16

[Reuben Morais [:reuben]]

Comment on attachment 8359285 [details] [diff] [review]
Reintroduce mozContact.init with a deprecation warning, v3

Let's try someone who's not on PTO :)

Comment 17

[Reuben Morais [:reuben]]

Thanks!

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

Comment 18

[Jean Gong :jgong]

PM triaged this bug and it be a 1.3 blocker.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cdafc366803c

Comment 20

[Boris Zbarsky [:bzbarsky]]

> WebIDL has some weird rules.

The rule is simple.  If you have a trailing dictionary argument, it has to be optional, and the default value is null, which behaves the same for dictionaries as passing {}.  Basically, keeps the callers from having to sprinkle {} all over the place to get the default behavior.

Comment 21

[Reuben Morais [:reuben]]

(In reply to Boris Zbarsky [:bz] from comment #20)
> > WebIDL has some weird rules.
> 
> The rule is simple.  If you have a trailing dictionary argument, it has to
> be optional, and the default value is null, which behaves the same for
> dictionaries as passing {}.  Basically, keeps the callers from having to
> sprinkle {} all over the place to get the default behavior.

Right, I didn't mean weird as in complicated, but weird as in it doesn't feel like it belongs in the WebIDL spec, but rather as a recommendation for spec authors to write specs that match what JS libraries look like and JS authors expect these days. But maybe that's the best/only way to make sure people won't write/implement unidiomatic APIs.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

This is going to need a branch-specific patch for Aurora uplift.

Comment 23

[Reuben Morais [:reuben]]

Ah. We'll need bug 951875 in Aurora since we still have nsIDOMGlobalPropertyInitializer::Init being used there.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

This still doesn't apply with bug 951875 applied.

patching file dom/contacts/ContactManager.js
Hunk #1 FAILED at 28
1 out of 1 hunks FAILED -- saving rejects to file dom/contacts/ContactManager.js.rej
patching file dom/contacts/tests/test_contacts_basics2.html
Hunk #1 FAILED at 762
1 out of 1 hunks FAILED -- saving rejects to file dom/contacts/tests/test_contacts_basics2.html.rej
patching file dom/webidl/Contacts.webidl
Hunk #1 FAILED at 85
1 out of 1 hunks FAILED -- saving rejects to file dom/webidl/Contacts.webidl.rej

Comment 25

[Reuben Morais [:reuben]]

Sorry, I should have been clearer, we needed bug 951875 *and* a branch patch.

https://hg.mozilla.org/releases/mozilla-aurora/rev/52e00136eb5c