Closed
Bug 594261
Opened 14 years ago
Closed 14 years ago
Factor out geolocation prompt into something that can be reused.
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: dougt, Assigned: dougt)
References
Details
Attachments
(3 files, 4 obsolete files)
6.13 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
16.90 KB,
patch
|
cjones
:
review+
pavlov
:
approval2.0+
|
Details | Diff | Splinter Review |
20.57 KB,
patch
|
Gavin
:
review+
pavlov
:
approval2.0+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•14 years ago
|
||
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)
Assignee | ||
Updated•14 years ago
|
Attachment #472935 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 2•14 years ago
|
||
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.
Assignee | ||
Comment 3•14 years ago
|
||
Attachment #472938 -
Flags: review?(mark.finkle)
Comment 4•14 years ago
|
||
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?
Assignee | ||
Comment 6•14 years ago
|
||
Ben, this sounds great. It will make your future IPC work free.
Assignee | ||
Comment 7•14 years ago
|
||
Attachment #473178 -
Flags: review?(jst)
Assignee | ||
Updated•14 years ago
|
Attachment #473178 -
Flags: review?(jst) → review-
Assignee | ||
Comment 8•14 years ago
|
||
Attachment #473178 -
Attachment is obsolete: true
Attachment #473198 -
Flags: review?(jones.chris.g)
Comment 9•14 years ago
|
||
Comment on attachment 472935 [details] [diff] [review] patch v.1 Could you keep the requesting* .idl names.
Attachment #472935 -
Flags: review?(jst) → review+
Comment 10•14 years ago
|
||
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)
Assignee | ||
Comment 11•14 years ago
|
||
hmm. old patch, i suppose.
Assignee | ||
Comment 12•14 years ago
|
||
Attachment #472935 -
Attachment is obsolete: true
Attachment #473224 -
Flags: review?
Attachment #472935 -
Flags: review?(jst)
Attachment #472935 -
Flags: review?(gavin.sharp)
Assignee | ||
Updated•14 years ago
|
Attachment #473224 -
Flags: review? → review?(gavin.sharp)
Comment 13•14 years ago
|
||
Wrong patch? var components = [BrowserGlue, GeolocationPrompt]; still hasn't been updated, and the ID wasn't changed in the manifest.
Assignee | ||
Comment 14•14 years ago
|
||
Attachment #473224 -
Attachment is obsolete: true
Attachment #473224 -
Flags: review?(gavin.sharp)
Assignee | ||
Updated•14 years ago
|
Attachment #473611 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 15•14 years ago
|
||
fixing irc comments from gavin.
Attachment #473611 -
Attachment is obsolete: true
Attachment #473627 -
Flags: review?(gavin.sharp)
Attachment #473611 -
Flags: review?(gavin.sharp)
Comment 16•14 years ago
|
||
Comment on attachment 473627 [details] [diff] [review] patch v.3 r=me on the browser/ changes.
Attachment #473627 -
Flags: review?(gavin.sharp) → review+
Comment 17•14 years ago
|
||
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.
Assignee | ||
Comment 19•14 years ago
|
||
> 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+
Assignee | ||
Comment 21•14 years ago
|
||
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.
Assignee | ||
Updated•14 years ago
|
Attachment #473198 -
Flags: approval2.0?
Assignee | ||
Updated•14 years ago
|
Attachment #473627 -
Flags: approval2.0?
Updated•14 years ago
|
Attachment #473198 -
Flags: approval2.0? → approval2.0+
Updated•14 years ago
|
Attachment #473627 -
Flags: approval2.0? → approval2.0+
Assignee | ||
Comment 22•14 years ago
|
||
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
Assignee | ||
Comment 23•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/91808d29bce3 http://hg.mozilla.org/mozilla-central/rev/caa83e7f0020 fennec part coming soon.
Assignee | ||
Comment 24•14 years ago
|
||
Fennec: http://hg.mozilla.org/mobile-browser/rev/70ea8fe5a13c http://hg.mozilla.org/mobile-browser/rev/4e898b286b51 (renaming)
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
(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.
You need to log in
before you can comment on or make changes to this bug.
Description
•