Last Comment Bug 780707 - Contacts API: support prompting
: Contacts API: support prompting
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla17
Assigned To: Gregor Wagner [:gwagner]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-06 13:41 PDT by Gregor Wagner [:gwagner]
Modified: 2012-08-09 19:57 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
WiP (11.91 KB, patch)
2012-08-06 14:39 PDT, Gregor Wagner [:gwagner]
no flags Details | Diff | Review
patch (24.56 KB, patch)
2012-08-06 17:21 PDT, Gregor Wagner [:gwagner]
no flags Details | Diff | Review
patch (22.13 KB, patch)
2012-08-06 18:39 PDT, Gregor Wagner [:gwagner]
bugs: review+
Details | Diff | Review
patch (22.12 KB, patch)
2012-08-07 14:00 PDT, Gregor Wagner [:gwagner]
no flags Details | Diff | Review
patch (23.15 KB, patch)
2012-08-07 14:14 PDT, Gregor Wagner [:gwagner]
no flags Details | Diff | Review
patch (23.19 KB, patch)
2012-08-07 14:47 PDT, Gregor Wagner [:gwagner]
dougt: review+
Details | Diff | Review

Description Gregor Wagner [:gwagner] 2012-08-06 13:41:32 PDT
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] 2012-08-06 14:39:38 PDT
Created attachment 649423 [details] [diff] [review]
WiP

First working version with the new prompting version just for the clear method.
Comment 2 Gregor Wagner [:gwagner] 2012-08-06 17:21:46 PDT
Created attachment 649488 [details] [diff] [review]
patch

Working version.
Comment 3 Gregor Wagner [:gwagner] 2012-08-06 18:39:01 PDT
Created attachment 649512 [details] [diff] [review]
patch
Comment 4 Doug Turner (:dougt) 2012-08-06 20:13:49 PDT
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) 2012-08-06 20:14:18 PDT
smaug knows more about the mm than just about anyone.  can you also look over this patch?
Comment 6 Olli Pettay [:smaug] 2012-08-07 03:16:12 PDT
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] 2012-08-07 13:50:10 PDT
(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] 2012-08-07 13:52:40 PDT
(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] 2012-08-07 13:59:46 PDT
Oh, for some reason the classinfo is for contentframemessagemanager.
I'll fix that at some point.
Comment 10 Gregor Wagner [:gwagner] 2012-08-07 14:00:28 PDT
Created attachment 649798 [details] [diff] [review]
patch
Comment 11 Gregor Wagner [:gwagner] 2012-08-07 14:14:21 PDT
Created attachment 649807 [details] [diff] [review]
patch

Makefile was missing
Comment 12 Gregor Wagner [:gwagner] 2012-08-07 14:47:08 PDT
Created attachment 649820 [details] [diff] [review]
patch

Argh missing request check.
Comment 13 Doug Turner (:dougt) 2012-08-09 10:04:32 PDT
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] 2012-08-09 11:36:31 PDT
With comments fixed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a96d79dd9d2c
Comment 15 Ryan VanderMeulen [:RyanVM] 2012-08-09 19:57:50 PDT
https://hg.mozilla.org/mozilla-central/rev/a96d79dd9d2c

Note You need to log in before you can comment on or make changes to this bug.