Closed Bug 780707 Opened 8 years ago Closed 8 years ago

Contacts API: support prompting

Categories

(Core :: DOM: Device Interfaces, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla17
blocking-basecamp +

People

(Reporter: gwagner, Assigned: gwagner)

Details

Attachments

(1 file, 5 obsolete files)

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.
blocking-basecamp: --- → ?
Attached patch WiP (obsolete) — Splinter Review
First working version with the new prompting version just for the clear method.
Attached patch patch (obsolete) — Splinter Review
Working version.
Attachment #649423 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
Attachment #649488 - Attachment is obsolete: true
Attachment #649512 - Flags: review?(doug.turner)
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
Attachment #649512 - Flags: review?(bugs)
smaug knows more about the mm than just about anyone.  can you also look over this patch?
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
Attachment #649512 - Flags: review?(bugs) → review+
(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?
(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.
Oh, for some reason the classinfo is for contentframemessagemanager.
I'll fix that at some point.
Attached patch patch (obsolete) — Splinter Review
Attachment #649512 - Attachment is obsolete: true
Attachment #649512 - Flags: review?(doug.turner)
Attachment #649798 - Flags: review?(doug.turner)
Attached patch patch (obsolete) — Splinter Review
Makefile was missing
Attachment #649798 - Attachment is obsolete: true
Attachment #649798 - Flags: review?(doug.turner)
Attachment #649807 - Flags: review?(doug.turner)
Attached patch patchSplinter Review
Argh missing request check.
Attachment #649807 - Attachment is obsolete: true
Attachment #649807 - Flags: review?(doug.turner)
Attachment #649820 - Flags: review?(doug.turner)
blocking-basecamp: ? → +
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.
Attachment #649820 - Flags: review?(doug.turner) → review+
https://hg.mozilla.org/mozilla-central/rev/a96d79dd9d2c
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.