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.

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

Working version.

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

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

(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.

Makefile was missing

Argh missing request check.

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.

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