780707 - Contacts API: support prompting

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Gregor Wagner [:gwagner]]

The idea is to use something similar to the askPermission approach in https://mxr.mozilla.org/mozilla-central/source/mobile/xul/chrome/content/WebappsUI.js#66

For contacts we have to deal with 2 parent roundtrips. First we do the permission checking with prompting if needed and the 2nd roundtrip does the actual DB request.

Comment 1

[Gregor Wagner [:gwagner]]

Attachment: WiP

First working version with the new prompting version just for the clear method.

Comment 2

[Gregor Wagner [:gwagner]]

Attachment: patch

Working version.

Comment 3

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 4

[Doug Turner (:dougt)]

Comment on attachment 649512 [details] [diff] [review]
patch

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

::: dom/contacts/ContactManager.js
@@ +420,5 @@
> +    this._setMetaData(newContact, aContact);
> +    debug("send: " + JSON.stringify(newContact));
> +    request = this.createRequest();
> +    let options = { contact: newContact };
> +    let allowCallback = function() {

s/allowCallback/permissionCallback ?  It does get called back when deny is invoked, right?

::: dom/contacts/fallback/ContactDB.jsm
@@ +301,5 @@
>     *        - count
>     */
>    find: function find(aSuccessCb, aFailureCb, aOptions) {
> +    
> +    let options = aOptions;

I don't understand why we are making a new local var here.

::: dom/contacts/fallback/ContactService.jsm
@@ +127,4 @@
>          break;
>        case "Contact:Save":
>          this._db.saveContact(
> +          msg.options.contact, 

trailing ws

@@ +133,5 @@
>          );
>          break;
>        case "Contact:Remove":
>          this._db.removeContact(
> +          msg.options.id, 

same

::: dom/contacts/tests/test_contacts_basics.html
@@ +407,5 @@
>    function () {
>      ok(true, "Adding a new contact with properties1");
>      createResult1 = new mozContact();
>      createResult1.init(properties1);
> +    mozContacts.oncontactchange = null;

I don't understand this change.

::: dom/permission/PermissionPromptHelper.jsm
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

This looks really useful.  Even more if there was some small example of how to use it in this file?

@@ +3,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +let DEBUG = 1;

No.  No one likes your dump messages.

@@ +22,5 @@
> +XPCOMUtils.defineLazyGetter(this, "ppmm", function() {
> +  return Cc["@mozilla.org/parentprocessmessagemanager;1"].getService(Ci.nsIFrameMessageManager);
> +});
> +
> +var permissionManager = Components.classes["@mozilla.org/permissionmanager;1"].getService(Ci.nsIPermissionManager);

Use Cc.

@@ +23,5 @@
> +  return Cc["@mozilla.org/parentprocessmessagemanager;1"].getService(Ci.nsIFrameMessageManager);
> +});
> +
> +var permissionManager = Components.classes["@mozilla.org/permissionmanager;1"].getService(Ci.nsIPermissionManager);
> +var secMan = Components.classes["@mozilla.org/scriptsecuritymanager;1"].getService(Components.interfaces.nsIScriptSecurityManager);

Use Ci too

@@ +49,5 @@
> +        aCallbacks.cancel();
> +        return;
> +    }
> +
> +  // FIXXMEE PERMISSION MAGIC! Bug 773114.

What is this about?

@@ +64,5 @@
> +    let mm = aMessage.target.QueryInterface(Ci.nsIFrameMessageManager);
> +    let msg = aMessage.json;
> +
> +    let result;
> +    switch (aMessage.name) {

you could just use an  if() stmt

Comment 5

[Doug Turner (:dougt)]

smaug knows more about the mm than just about anyone.  can you also look over this patch?

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 649512 [details] [diff] [review]
patch

>+  receiveMessage: function(aMessage) {
>+    debug("PermissionPromptHelper::receiveMessage " + aMessage.name);
>+    let mm = aMessage.target.QueryInterface(Ci.nsIFrameMessageManager);
Is this QI actually needed?


>+    let msg = aMessage.json;
New code could start using .data. We use structured clones now, and .json is for backwards compatibility only.


r+ to mm usage

Comment 7

[Gregor Wagner [:gwagner]]

(In reply to Doug Turner (:dougt) from comment #4)
> Comment on attachment 649512 [details] [diff] [review]
> patch
> 
> Review of attachment 649512 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/contacts/ContactManager.js
> @@ +420,5 @@
> > +    this._setMetaData(newContact, aContact);
> > +    debug("send: " + JSON.stringify(newContact));
> > +    request = this.createRequest();
> > +    let options = { contact: newContact };
> > +    let allowCallback = function() {
> 
> s/allowCallback/permissionCallback ?  It does get called back when deny is
> invoked, right?

No we have an optional cancelCallback. If not set, the .onerror callback for the request gets called.

> 
> ::: dom/contacts/fallback/ContactDB.jsm
> @@ +301,5 @@
> >     *        - count
> >     */
> >    find: function find(aSuccessCb, aFailureCb, aOptions) {
> > +    
> > +    let options = aOptions;
> 
> I don't understand why we are making a new local var here.

fixed

> 
> ::: dom/contacts/tests/test_contacts_basics.html
> @@ +407,5 @@
> >    function () {
> >      ok(true, "Adding a new contact with properties1");
> >      createResult1 = new mozContact();
> >      createResult1.init(properties1);
> > +    mozContacts.oncontactchange = null;
> 
> I don't understand this change.

The test was wrong :)

> @@ +49,5 @@
> > +        aCallbacks.cancel();
> > +        return;
> > +    }
> > +
> > +  // FIXXMEE PERMISSION MAGIC! Bug 773114.
> 
> What is this about?

That should hook into the prompting code but my understanding is that this is not in place yet right?

Comment 8

[Philipp von Weitershausen [:philikon]]

(In reply to Olli Pettay [:smaug] from comment #6)
> >+  receiveMessage: function(aMessage) {
> >+    debug("PermissionPromptHelper::receiveMessage " + aMessage.name);
> >+    let mm = aMessage.target.QueryInterface(Ci.nsIFrameMessageManager);
> Is this QI actually needed?

Sadly yes. You just get an nsISupports there.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Oh, for some reason the classinfo is for contentframemessagemanager.
I'll fix that at some point.

Comment 10

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 11

[Gregor Wagner [:gwagner]]

Attachment: patch

Makefile was missing

Comment 12

[Gregor Wagner [:gwagner]]

Attachment: patch

Argh missing request check.

Comment 13

[Doug Turner (:dougt)]

Comment on attachment 649820 [details] [diff] [review]
patch

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

with those fixes, r+

::: dom/contacts/ContactManager.js
@@ +271,3 @@
>        throw Components.results.NS_ERROR_FAILURE;
> +    }
> +    this.askPermission("contacts", "listen", null, allowCallback, cancelCallback);

why are you passing "contacts" here?  Instead, just hard code this in the askPermission function?

@@ +336,5 @@
>          break;
> +      case "PermissionPromptHelper:AskPermission:OK":
> +        debug("id: " + msg.requestID);
> +        req = this.getRequest(msg.requestID);
> +        if (!req)

if (!req) {
  break;
}

@@ +405,5 @@
> +      anniversary:     null,
> +      sex:             null,
> +      genderIdentity:  null
> +    };
> +    for (let field in newContact.properties)

add {}

::: dom/permission/Makefile.in
@@ +6,5 @@
> +topsrcdir	= @top_srcdir@
> +srcdir		= @srcdir@
> +VPATH            = \
> +  $(srcdir)        \
> +  $(NULL)

DEPTH = @DEPTH@
topsrcdir = @top_srcdir@
srcdir = @srcdir@
VPATH = @srcdir@

See bug 774032.  Yes, it is okay to rejoice.

@@ +12,5 @@
> +include $(DEPTH)/config/autoconf.mk
> +
> +MODULE         = dom
> +LIBRARY_NAME   = jsdompermission_s
> +LIBXUL_LIBRARY = 1

Don't need LIBXUL_LIBRARY, i think.

@@ +23,5 @@
> +  $(NULL)
> +
> +# Add VPATH to LOCAL_INCLUDES so we are going to include the correct backend
> +# subdirectory (and the ipc one).
> +LOCAL_INCLUDES += $(VPATH:%=-I%)

don't need.

@@ +29,5 @@
> +include $(topsrcdir)/config/config.mk
> +include $(topsrcdir)/ipc/chromium/chromium-config.mk
> +include $(topsrcdir)/config/rules.mk
> +
> +DEFINES += -D_IMPL_NS_LAYOUT

Do you need this define?

::: dom/permission/PermissionPromptHelper.jsm
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

There is some trailing whitespace in this file.

Comment 14

[Gregor Wagner [:gwagner]]

With comments fixed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a96d79dd9d2c

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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