Closed Bug 594261 Opened 9 years ago Closed 9 years ago

Factor out geolocation prompt into something that can be reused.

Categories

(Core :: DOM: Core & HTML, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: dougt, Assigned: dougt)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 4 obsolete files)

We should factor out the prompting code so that other device APIs can reuse it.  I am thinking immediately about desktop notifications  (bug 573588) which needs exactly the same interface.  Also factoring out this interface will allow me to remote the prompt for desktop notifications in exactly the same way we do for geo -- saving code.

I think it is a pretty generic solution to the problem of web content asking for privileged apis.
Attached patch patch v.1 (obsolete) — Splinter Review
1) remove nsIGeolocationPrompt.idl
2) add new nsIContentPermissionPrompt.idl

Basically this could have been a rename and patch, but either way.

The difference in the interface is:

a) added a type attribute so that the implementer can known what kind of prompt is being asked for.

b) removed the "requesting" prefix to all of the attributes.  I never really liked that.

c) added some comments for each of the attributes

3) fixed up the firefox glue code to use this new interface.
Assignee: nobody → doug.turner
Attachment #472935 - Flags: review?(jst)
Attachment #472935 - Flags: review?(gavin.sharp)
gavin, can you review the firefox bits -- you were the last to touch them. :-)

jst, can you review the move.

bent, can you provide any feedback that might help with indexDB.
Attached patch fennec bitsSplinter Review
Attachment #472938 - Flags: review?(mark.finkle)
Comment on attachment 472938 [details] [diff] [review]
fennec bits

>diff --git a/components/GeolocationPrompt.js b/components/GeolocationPrompt.js

Rename the file: "ContentPermissionPrompt.js" ?

>   prompt: function(aRequest) {
>+
>+    if (aRequest.type != "geolocation")

Spaces at the top of a function are the work of the Devil. Please remove.

r+ with nits fixed
Attachment #472938 - Flags: review?(mark.finkle) → review+
(In reply to comment #2)
> bent, can you provide any feedback that might help with indexDB.

Hm. Our plan is to use the same notifications that you do for indexedDB, so as long as your prompt stuff works with different strings and images then we should be just fine. Were you looking for anything more specific?
Ben, this sounds great.  It will make your future IPC work free.
Attached patch ipc bits (obsolete) — Splinter Review
Attachment #473178 - Flags: review?(jst)
Attachment #473178 - Flags: review?(jst) → review-
Attached patch ipc bitsSplinter Review
Attachment #473178 - Attachment is obsolete: true
Attachment #473198 - Flags: review?(jones.chris.g)
Blocks: 573588
Comment on attachment 472935 [details] [diff] [review]
patch v.1

Could you keep the requesting* .idl names.
Attachment #472935 - Flags: review?(jst) → review+
Blocks: 594543
Comment on attachment 472935 [details] [diff] [review]
patch v.1

>diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js

>+ContentPermissionPrompt.prototype = {
>   classID:          Components.ID("{C6E8C44D-9F39-4AF7-BCC0-76E38A8310F5}"),

Wouldn't hurt to change the ID too.

>+  prompt: function CPP_prompt(request) {

>     if (requestingURI.schemeIs("file")) {
>       message = browserBundle.formatStringFromName("geolocation.fileWantsToKnow",
>-                                                   [request.requestingURI.path], 1);
>+                                                   [request.uri.path], 1);

this should just use requestingURI.path...

You forgot a reference to GeolocationPrompt (end of the file) - this hasn't been tested?
Attachment #472935 - Flags: review+ → review?(jst)
hmm. old patch, i suppose.
Attached patch patch v.2 (obsolete) — Splinter Review
Attachment #472935 - Attachment is obsolete: true
Attachment #473224 - Flags: review?
Attachment #472935 - Flags: review?(jst)
Attachment #472935 - Flags: review?(gavin.sharp)
Attachment #473224 - Flags: review? → review?(gavin.sharp)
Wrong patch?

var components = [BrowserGlue, GeolocationPrompt];

still hasn't been updated, and the ID wasn't changed in the manifest.
Attached patch patch v.2 (obsolete) — Splinter Review
Attachment #473224 - Attachment is obsolete: true
Attachment #473224 - Flags: review?(gavin.sharp)
Attachment #473611 - Flags: review?(gavin.sharp)
Attached patch patch v.3Splinter Review
fixing irc comments from gavin.
Attachment #473611 - Attachment is obsolete: true
Attachment #473627 - Flags: review?(gavin.sharp)
Attachment #473611 - Flags: review?(gavin.sharp)
Comment on attachment 473627 [details] [diff] [review]
patch v.3

r=me on the browser/ changes.
Attachment #473627 - Flags: review?(gavin.sharp) → review+
Though since we're planning to add other consumers, we should probably have ContentPermissionPrompt::prompt() call a geo-specific helper (_handleGeoPrompt()) to keep it readable.
Can this be reused for the popup blocker UI too? One of the things we need there is the ability to register a callback which is called if a certain option is chosen (the 'display blocked popup now' option).

Also, for IndexedDB, we need some way to cancel a opened prompt. For example if the user approves the creation of IndexedDB databases in another window.
> One of the things we need there is the ability to register a callback which is called if a certain option is chosen (the 'display blocked popup now' option).

I don't follow.

> Also, for IndexedDB, we need some way to cancel a opened prompt.

jonas, i would love that.  The underlying alert service needs to be adjusted to support that.  There was some discussion about web notifications doing the same thing.

Lets file follow ups for both of these.  I can do one for the second one, if you do one for the first.
Comment on attachment 473198 [details] [diff] [review]
ipc bits

>diff --git a/dom/ipc/PBrowser.ipdl b/dom/ipc/PBrowser.ipdl
>--- a/dom/ipc/PBrowser.ipdl
>+++ b/dom/ipc/PBrowser.ipdl
>@@ -109,17 +109,17 @@ parent:
> 
>     rpc CreateWindow() returns (PBrowser window);
> 
>     sync SyncMessage(nsString aMessage, nsString aJSON)
>       returns (nsString[] retval);
> 
>     QueryContentResult(nsQueryContentEvent event);
> 
>-    PGeolocationRequest(URI uri);
>+    PContentPermissionRequest(nsCString aType, URI uri);
> 

I really dislike using strings to identify one of a set of types known
at compile time.  Instead, how about

 struct geolocation_t {};
 //...
 union PermissionRequest {
   geolocation_t;
   //...
 };

 protocol PBrowser {
   //...
   PContentPermissionRequest(PermissionRequest aType, URI uri);
   //...
 };

Then your C++ can be

 TabParent::AllocPContentPermissionRequest(const PermissioRequest& type, const IPC::URI& uri)
 {
    switch (type.type()) {
    case PermissioRequest::Tgeolocation_t:
      return new GeolocationRequestParent(mFrameElement, uri);
    }
 }

>diff --git a/dom/ipc/PCOMContentPermissionRequestChild.h b/dom/ipc/PCOMContentPermissionRequestChild.h
>new file mode 100644
>--- /dev/null
>+++ b/dom/ipc/PCOMContentPermissionRequestChild.h
>+class PCOMContentPermissionRequestChild : public mozilla::dom::PContentPermissionRequestChild {
>+public:
>+  virtual void IPDLRelease() = 0;

Need a comment explaining why this class and method exist.

>diff --git a/dom/ipc/TabChild.cpp b/dom/ipc/TabChild.cpp
>--- a/dom/ipc/TabChild.cpp
>+++ b/dom/ipc/TabChild.cpp
> bool
>+TabChild::DeallocPContentPermissionRequest(PContentPermissionRequestChild* actor)
> {
>-  static_cast<nsGeolocationRequest*>(actor)->Release();
>-  return true;
>+    // The PContentPermissionRequestChild actor is
>+    // implemented by a refcounted class, and has an
>+    // identical lifetime.

This comment is untrue.  The lifetime of the actor and request class
(AIUI) are different, which is exactly why you need the refcounting
scheme.

>diff --git a/dom/ipc/TabParent.cpp b/dom/ipc/TabParent.cpp
>--- a/dom/ipc/TabParent.cpp
>+++ b/dom/ipc/TabParent.cpp
>+PContentPermissionRequestParent*
>+TabParent::AllocPContentPermissionRequest(const nsCString& type, const IPC::URI& uri)
> {
>-  return new GeolocationRequestParent(mFrameElement, uri);
>+  if (type.Equals(NS_LITERAL_CSTRING("geolocation"))) {
>+    return new GeolocationRequestParent(mFrameElement, uri);
>+  }
>+

See above.

>diff --git a/dom/src/geolocation/ipdl.mk b/dom/src/geolocation/ipdl.mk
>deleted file mode 100644
>diff --git a/dom/src/geolocation/nsGeolocation.cpp b/dom/src/geolocation/nsGeolocation.cpp
>--- a/dom/src/geolocation/nsGeolocation.cpp
>+++ b/dom/src/geolocation/nsGeolocation.cpp
>@@ -1110,20 +1110,21 @@ nsGeolocation::RegisterRequestWithPrompt
>     nsCOMPtr<nsPIDOMWindow> window = do_QueryReferent(mOwner);
>     if (!window)
>       return;
> 
>     // because owner implements nsITabChild, we can assume that it is
>     // the one and only TabChild.
>     TabChild* child = GetTabChildFrom(window->GetDocShell());
>     
>-    child->SendPGeolocationRequestConstructor(request, IPC::URI(mURI));
>+    nsCString type = NS_LITERAL_CSTRING("geolocation");
>+    child->SendPContentPermissionRequestConstructor(request, type, IPC::URI(mURI));
>     
>     // Retain a reference so the object isn't deleted without IPDL's knowledge.
>-    // Corresponding release occurs in DeallocPGeolocationRequest.
>+    // Corresponding release occurs in DeallocPContentPermissionRequest.
>     request->AddRef();
> 

This code is wrong.  You need to |request->AddRef()| before the
|child->SendPCOntentPermissionRequestConstructor()|.  The reason is,
if the ctor fails for some reason, IPDL will call Dealloc() on your
behalf.  (In practice, if the ctor fails, the content process will
_exit(), but getting the order right here is a good habit to get
into.)

>     unused << request->Sendprompt();

This |unused <<| is unnecessary.  Child-side methods aren't
|warn_unused_result|, only parent-side ones.

r=me with the comments above addressed.  Static typing is more a style
issue here, so I'll leave to your discretion.
Attachment #473198 - Flags: review?(jones.chris.g) → review+
test:

existing geolocation tests will exercise this code path (basically moving files around, renaming things, and a bit of plumbing)

risk/reward analysis:

in order to provide content permission dialogs, this patch is going to greatly reduce the amount of code people have to write in C++ code.  Also, it will allow you to get IPC remoting for free.  Since it is just refactoring and existing tests cover the code, it is low risk.

Try run was green.
Attachment #473198 - Flags: approval2.0?
Attachment #473627 - Flags: approval2.0?
Attachment #473198 - Flags: approval2.0? → approval2.0+
Attachment #473627 - Flags: approval2.0? → approval2.0+
cjones - thanks!  i will fix up your comments.  thanks for finding that addref() issue.  about the enum, (and as we spoke about on irc) it is a good suggestion, but we end up having to use strings latter on so that we can pass them to script in the request objects.

fwi, bug 595075 tracks extending the alerts service
Fennec:
http://hg.mozilla.org/mobile-browser/rev/70ea8fe5a13c
http://hg.mozilla.org/mobile-browser/rev/4e898b286b51 (renaming)
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Blocks: 595094
(In reply to comment #19)
> Lets file follow ups for both of these.  I can do one for the second one, if
> you do one for the first.

Filed bug 595253.
Blocks: 595437
Blocks: 595442
No longer blocks: 595437
You need to log in before you can comment on or make changes to this bug.