Closed
Bug 780707
Opened 12 years ago
Closed 12 years ago
Contacts API: support prompting
Categories
(Core :: DOM: Device Interfaces, defect)
Tracking
()
People
(Reporter: gwagner, Assigned: gwagner)
Details
Attachments
(1 file, 5 obsolete files)
(deleted),
patch
|
dougt
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•12 years ago
|
blocking-basecamp: --- → ?
Assignee | ||
Comment 1•12 years ago
|
||
First working version with the new prompting version just for the clear method.
Assignee | ||
Comment 2•12 years ago
|
||
Working version.
Attachment #649423 -
Attachment is obsolete: true
Assignee | ||
Comment 3•12 years ago
|
||
Attachment #649488 -
Attachment is obsolete: true
Attachment #649512 -
Flags: review?(doug.turner)
Comment 4•12 years ago
|
||
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)
Comment 5•12 years ago
|
||
smaug knows more about the mm than just about anyone. can you also look over this patch?
Comment 6•12 years ago
|
||
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+
Assignee | ||
Comment 7•12 years ago
|
||
(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•12 years ago
|
||
(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•12 years ago
|
||
Oh, for some reason the classinfo is for contentframemessagemanager.
I'll fix that at some point.
Assignee | ||
Comment 10•12 years ago
|
||
Attachment #649512 -
Attachment is obsolete: true
Attachment #649512 -
Flags: review?(doug.turner)
Attachment #649798 -
Flags: review?(doug.turner)
Assignee | ||
Comment 11•12 years ago
|
||
Makefile was missing
Attachment #649798 -
Attachment is obsolete: true
Attachment #649798 -
Flags: review?(doug.turner)
Attachment #649807 -
Flags: review?(doug.turner)
Assignee | ||
Comment 12•12 years ago
|
||
Argh missing request check.
Attachment #649807 -
Attachment is obsolete: true
Attachment #649807 -
Flags: review?(doug.turner)
Attachment #649820 -
Flags: review?(doug.turner)
Updated•12 years ago
|
blocking-basecamp: ? → +
Comment 13•12 years ago
|
||
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+
Assignee | ||
Comment 14•12 years ago
|
||
With comments fixed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a96d79dd9d2c
Comment 15•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in
before you can comment on or make changes to this bug.
Description
•