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

RESOLVED FIXED in Firefox 28, Firefox OS v1.3

Status

()

Core
DOM: Contacts
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: julienw, Assigned: reuben)

Tracking

({dev-doc-needed, site-compat})

unspecified
mozilla29
x86_64
Linux
dev-doc-needed, site-compat
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:1.3+, firefox27 wontfix, firefox28 fixed, firefox29 fixed, b2g-v1.3 fixed, b2g-v1.4 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 8349730 [details] [diff] [review]
Reintroduce mozContact.init with a deprecation warning
Assignee: nobody → reuben.bmo
Status: NEW → ASSIGNED
Attachment #8349730 - Flags: superreview?(jonas)
Attachment #8349730 - Flags: review?(felash)
(Assignee)

Comment 2

5 years ago
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).
(Reporter)

Comment 3

5 years ago
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.
Attachment #8349730 - Flags: review?(felash) → feedback+
do we have any user / developer impact without the .init method? thanks
Flags: needinfo?(felash)
(Reporter)

Comment 5

5 years ago
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.
Flags: needinfo?(felash)

Updated

5 years ago
Keywords: dev-doc-needed
traige: 1.3+ agree with comment 5
blocking-b2g: 1.3? → 1.3+
Hi Reuben, wonder if you will be working on this starting next week? thanks
Flags: needinfo?(reuben.bmo)
(Assignee)

Comment 8

5 years ago
(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.
Flags: needinfo?(reuben.bmo)
(Assignee)

Comment 9

5 years ago
(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.
(Assignee)

Comment 10

5 years ago
Created attachment 8358798 [details] [diff] [review]
Reintroduce mozContact.init with a deprecation warning, v2
Attachment #8349730 - Attachment is obsolete: true
Attachment #8349730 - Flags: superreview?(jonas)
Attachment #8358798 - Flags: superreview?(jonas)
Attachment #8358798 - Flags: review?(felash)
(Reporter)

Comment 11

5 years ago
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?
Attachment #8358798 - Flags: review?(felash)
(Assignee)

Comment 12

5 years ago
Created attachment 8359285 [details] [diff] [review]
Reintroduce mozContact.init with a deprecation warning, v3

Sounds good. How's this?
Attachment #8359285 - Flags: review?(felash)
(Reporter)

Comment 13

5 years ago
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 :)
Attachment #8359285 - Flags: review?(felash) → review+
(Assignee)

Comment 14

5 years ago
As green as it gets, I guess: https://tbpl.mozilla.org/?tree=Try&rev=598e73a31e6c
(Reporter)

Comment 15

5 years ago
Yes.
(Assignee)

Updated

5 years ago
Attachment #8358798 - Attachment is obsolete: true
Attachment #8358798 - Flags: superreview?(jonas)
(Assignee)

Comment 16

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

Let's try someone who's not on PTO :)
Attachment #8359285 - Flags: superreview?(bugs)
Attachment #8359285 - Flags: superreview?(bugs) → superreview+

Comment 18

5 years ago
PM triaged this bug and it be a 1.3 blocker.
https://hg.mozilla.org/mozilla-central/rev/cdafc366803c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
> 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.
(Assignee)

Comment 21

5 years ago
(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.
This is going to need a branch-specific patch for Aurora uplift.
status-b2g-v1.3: --- → affected
status-b2g-v1.4: --- → fixed
status-firefox27: --- → wontfix
status-firefox28: --- → affected
status-firefox29: --- → fixed
Flags: needinfo?(reuben.bmo)
Keywords: branch-patch-needed
(Assignee)

Comment 23

5 years ago
Ah. We'll need bug 951875 in Aurora since we still have nsIDOMGlobalPropertyInitializer::Init being used there.
Depends on: 951875
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
(Assignee)

Comment 25

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/52e00136eb5c
status-firefox28: affected → fixed
Flags: needinfo?(reuben.bmo)
status-b2g-v1.3: affected → fixed
Keywords: branch-patch-needed

Updated

5 years ago
Keywords: site-compat
You need to log in before you can comment on or make changes to this bug.