552822 - Geolocation E10S work

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Doug Turner (:dougt)]

Ideally the geolocation service only lives in the chrome process.  Also, we have for investigate how best to get to the notification box from chrome so that we can ask the user if they want to share their location.

Comment 1

[Josh Matthews [:jdm]]

Taking.  I've got a brief overview of my plans at https://wiki.mozilla.org/Electrolysis/Geolocation and it doesn't look like it'll be a big deal.

Comment 2

[Josh Matthews [:jdm]]

Attachment: Part 1: split nsGeolocation into nsGeolocationChrome and nsGeolocationContent

Part 1 of 2.  This basically just does code rearranging, and shouldn't affect existing behaviour in any way.

Comment 3

[Josh Matthews [:jdm]]

Attachment: Part 2: implement IPDL remoting

Part 2 of 2.  The actual meat of the remoting, following the design laid out in the wiki.

Comment 4

[Josh Matthews [:jdm]]

I'm hoping there will be a part 3 which includes tests, but I haven't figured out how to do IPC mochitests yet.  Pointers welcome.

Comment 5

[Josh Matthews [:jdm]]

Bleah, I forgot to remove my debug changes to the geolocation providers in the uploaded patch.  That's fixed locally.

Comment 6

[Doug Turner (:dougt)]

this doesn't apply any longer either.  can you post a new patch?

Comment 7

[Doug Turner (:dougt)]

+ifdef MOZ_IPC
+CPPSRCS += 	nsGeolocationContent.cpp \
+		$(NULL)
+endif
+


No need to add the new line or $(NULL)


+
+extern PRBool sGeoEnabled;

Add a comment mentioning where you think that this static should be found.



   // This is the last geo position that we have seen.
   nsCOMPtr<nsIDOMGeoPosition> mLastPosition;
 };
 
 
+
 /**


No new line is needed



+nsresult
+nsGeolocationChrome::Init(nsIDOMWindow* aContentDom)
+{
+  nsresult rv = nsGeolocation::Init(aContentDom);
+  if (NS_FAILED(rv) || aContentDom)
+    return rv;
+


If aContentDom is null, but |Init| is successful, do you want to return NS_OK?

Comment 8

[Doug Turner (:dougt)]

oh.. just noticed the email sig for jdm - out until may 1.

Comment 9

[Doug Turner (:dougt)]

Comment on attachment 437004 [details] [diff] [review]
Part 1: split nsGeolocation into nsGeolocationChrome and nsGeolocationContent

What about:  

doc->NodePrincipal()->GetURI(getter_AddRefs(mURI));

There is no node principal available.


New patch coming

Comment 10

[Doug Turner (:dougt)]

Comment on attachment 437005 [details] [diff] [review]
Part 2: implement IPDL remoting

new patch coming up.

Comment 11

[Doug Turner (:dougt)]

Attachment: Part 1: split nsGeolocation into nsGeolocationChrome and nsGeolocationContent (v.2)

Comment 12

[Doug Turner (:dougt)]

Attachment: Part 2: implement IPDL remoting (v.2)

Both of these are still WIP/Unreviewed by me.

Comment 13

[Josh Matthews [:jdm]]

By new, do you mean the patches are unbitrotted?  Did you include the warning fixes from bug 556860?  If not, I can post my merged version, which should apply cleanly to trunk.

Comment 14

[Doug Turner (:dougt)]

yes, unbitrotted.  If you do take another look at this soon, please take a look at the Cleanup() calls I commented out.  I am not sure exactly why you had them where they were.

Comment 15

[Josh Matthews [:jdm]]

I've got 24 hours on a train tomorrow, so I've got plenty of time to address any concerns you have.

Comment 16

[Josh Matthews [:jdm]]

Doug: the Cleanup() calls are correct.  The actor doesn't have any need to retain any of the objects, and it only stores them in member variables to keep the creation code in one place.

Comment 17

[Josh Matthews [:jdm]]

Also, the fact that nsGeolocation::Init() returns an error if there's no URI present is a merge mistake.

Relatedly, what's the kosher way to run the mochitests manually using Fennec?

Comment 18

[Doug Turner (:dougt)]

so, here is my understanding of this patch:

Someone calls navigator.geolocation.

If the caller is a content process, we create a nsGeolocationContent and pass it a dom::TabChild (PIFrameEmbeddingChild).

we save the TabChild as the mTabChild member.

...later...

geolocation.GetCurrentPosition is called in content.


We create a PGeolocationRequestChild, and send that to the chrome process.


The GeolocationRequestChild/PGeolocationRequestChild owns a pointer/reference to the nsGeolocationContent.

nsGeolocationContent::CreateRequest does some magic and returns a PGeolocationRequestChild.  (this I really don't understand)



Comments/Questions:

why do we have this #ifdefs:

#ifndef WINCE_WINDOWS_MOBILE
DOMCI_DATA(GeoPositionCoords, void)
DOMCI_DATA(GeoPosition, void)
#endif


in nsGeolocation::Init(),

this is probably wrong:
  mURI = doc->GetDocumentURI() since the user can change it.

Could we use mTabChild for this?


The patches basically applied, but the |include| semantic changed a bit.

When using watchPosition, I only see one update, then everything shuts down.

I am not sure we are passing the nsIGeolocationPrompt the right thing.  I think we might have to change this for e10s.

Comment 19

[Josh Matthews [:jdm]]

> nsGeolocationContent::CreateRequest does some magic and returns a > PGeolocationRequestChild.  (this I really don't understand)

nsGeolocationContent::CreateRequest is not particularly magical.  We create a request actor, and send it to chrome, indicating that there will be an operation of some kind following.  Then we pass along any options that exist, and return the new actor.

> why do we have this #ifdefs:

The #ifdefs were there before my code, and will remain there unless I'm informed otherwise.

> this is probably wrong:
>   mURI = doc->GetDocumentURI() since the user can change it.
>
> Could we use mTabChild for this?

Probably, yep.

> When using watchPosition, I only see one update, then everything shuts down.

Sounds like something I should investigate.  I'm surprised that it occurs; I thought I tested that.

> I am not sure we are passing the nsIGeolocationPrompt the right thing.  I
> think we might have to change this for e10s.

I investigate this some more.

Comment 20

[Doug Turner (:dougt)]

i think we might want to consider a different direction here -- what if we just remoted the GeolocationPrompt interface?  In this way, the core code would stay exactly like it is now, with the only difference being how we interact with the notification service.

Comment 21

[Doug Turner (:dougt)]

Attachment: patch v.1 (prompt only) (wip)

first cut.  looking for feedback (especially around how I get access to the TabChild)

Comment 22

[Josh Matthews [:jdm]]

>+bool
>+TabChild::DeallocPGeolocationRequest(PGeolocationRequestChild* actor)
>+{
>+  return false;
>+}

That's wrong, and will cause the child process to abort with prejudice.  |return true;| is what you want here.  You're also not calling PGeolocationRequest::Send__delete__() anywhere, so you're leaking actors in the chrome process.

In fact, I recommend sending a |bool allow| parameter in the protocol's destructor, and calling |mParent->Send__delete__(true|false)| in GeolocationRequestProxy's Allow and Cancel methods.  That'll make everything clearner.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 443633 [details] [diff] [review]
patch v.1 (prompt only) (wip)


>+void
>+nsGeolocation::RegisterRequestWithPrompt(nsGeolocationRequest* request)
>+{
>+#ifdef MOZ_IPC
>+  if (XRE_GetProcessType() == GeckoProcessType_Content) {
>+
>+    mURI->GetSpec(mURIString);
>+
>+    nsCOMPtr<nsPIDOMWindow> window = do_QueryReferent(mOwner);
>+    if (!window)
>+      return;
>+
>+    nsIDocShell *docshell = window->GetDocShell();
>+
>+    nsCOMPtr<nsIDocShellTreeItem> item = do_QueryInterface(docshell);
>+    NS_ASSERTION(item, "doc shell tree item is null");
>+    
>+    nsCOMPtr<nsIDocShellTreeOwner> owner;
>+    item->GetTreeOwner(getter_AddRefs(owner));
>+    NS_ASSERTION(owner, "doc shell tree owner is null");
>+
>+    nsCOMPtr<nsIWebProgressListener2> wpl = do_GetInterface(owner);
>+    NS_ASSERTION(wpl, "web progress listener is null");
>+
>+    // I am going to hell.
>+    mozilla::dom::TabChild* child = static_cast<mozilla::dom::TabChild*>(wpl.get());
>+    NS_ASSERTION(child, "Content Protocol is NULL!");
>+
>+    mozilla::dom::PGeolocationRequestChild* a = child->SendPGeolocationRequestConstructor(request, mURIString);
>+
>+    (void) a->Sendprompt();
>+    return;
>+  }
>+#endif
Quite a few useless assertions. In some cases there will be just
a crash, and in others the code has null check (via do_XXX methods).

I'd rather use nsIWebBrowserChrome, than nsIWebProgressListener2.
Or perhaps we could add IID for TabChild, then we could just QI to it?

>+nsresult
>+nsGeolocationRequestProxy::Init(mozilla::dom::GeolocationRequestParent* parent)
>+{
>+  NS_ASSERTION(parent, "null parent");
>+  mParent = parent;
>+
>+  nsCOMPtr<nsIGeolocationPrompt> prompt = do_GetService(NS_GEOLOCATION_PROMPT_CONTRACTID);
>+  NS_ASSERTION(prompt, "null geolocation prompt.  geolocation will not work without one.");
>+  prompt->Prompt(this);
>+
>+  // NS_ADDREF(this);  // wtf?
What is this?

Comment 24

[Doug Turner (:dougt)]

Attachment: patch v.2 (prompt only)

Comment 25

[Josh Matthews [:jdm]]

>+bool
>+TabChild::DeallocPGeolocationRequest(PGeolocationRequestChild* actor)
>+{
>+  return true;
>+}

Since this is a fairly radical departure from the usual style of protocol deallocation, an explanatory comment wouldn't be amiss.

Comment 26

[Doug Turner (:dougt)]

what do you mean?  how radical is this?

Comment 27

[Josh Matthews [:jdm]]

The fact that there's no sign of deallocation taking place is fairly radical.  Most other Dealloc functions have a delete or NS_RELEASE, so it's easy to think this is a mistake.

Comment 28

[Doug Turner (:dougt)]

sure.  i will add a comment.

Comment 29

[Doug Turner (:dougt)]

Attachment: patch v.3 (prompt only)

this also passes more context (the dom element of the xul:browser) to the parent.

Comment 30

[Doug Turner (:dougt)]

Attachment: v.3 (with new files)

Comment 31

[Doug Turner (:dougt)]

Attachment: patch v.3 (raw)

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 444917 [details] [diff] [review]
patch v.3 (raw)

>diff --git a/.hgignore b/.hgignore
This file isn't part of the fix.

>+PGeolocationRequestChild*
>+TabChild::AllocPGeolocationRequest(const nsCString& unused)
>+{
>+  NS_RUNTIMEABORT("unused");
>+  return nsnull;
>+}
>+
>+bool
>+TabChild::DeallocPGeolocationRequest(PGeolocationRequestChild* actor)
>+{
>+    /* the geolocation child actor is called in nsGeolocation and does not need to be delete here */
>+  return true;
>+}
>+
So who creates PGeolocationRequestChild and who deletes and where?
This is not at all clear to me.


>+PGeolocationRequestParent*
>+TabParent::AllocPGeolocationRequest(const nsCString& principal)
>+{
>+  nsCOMPtr<nsIContent> content = do_QueryInterface(mFrameElement);
>+  nsCOMPtr<nsIDOMWindowInternal> chromeWindow = do_QueryInterface(content->GetOwnerDoc()->GetWindow());
>+
>+  nsCOMPtr<nsIDOMWindow> window;
>+  chromeWindow->GetContent(getter_AddRefs(window));
>+  return new GeolocationRequestParent(chromeWindow, mFrameElement, principal);
I don't understand this. |window| isn't used at all.


>+    nsCOMPtr<nsIDocShellTreeOwner> owner;
>+    item->GetTreeOwner(getter_AddRefs(owner));
>+    NS_ASSERTION(owner, "doc shell tree owner is null");
>+    
>+    nsCOMPtr<nsITabChild> tabchild = do_GetInterface(owner);
>+    NS_ASSERTION(tabchild, "tabchild is null");
>+    if (!tabchild)
>+      return;
Useless assertions.

>+namespace mozilla {
>+  namespace dom {
>+
>+    class GeolocationRequestParent : public PGeolocationRequestParent
>+    {
Could you use same style as we use elsewhere
namespace foo {
namespace bar {
class asdf
{
 ...

Comment 33

[Josh Matthews [:jdm]]

Comment on attachment 444917 [details] [diff] [review]
patch v.3 (raw)

Smaug said I could do a more in-depth review.  Hope I don't step on any toes.

>diff --git a/dom/ipc/TabChild.cpp b/dom/ipc/TabChild.cpp
>+PGeolocationRequestChild*
>+TabChild::AllocPGeolocationRequest(const nsCString& unused)
>+{
>+  NS_RUNTIMEABORT("unused");
>+  return nsnull;
>+}
>+
>+bool
>+TabChild::DeallocPGeolocationRequest(PGeolocationRequestChild* actor)
>+{
>+    /* the geolocation child actor is called in nsGeolocation and does not need to be delete here */
>+  return true;
>+}
>+

A comment couldn't hurt about who's responsible for allocating the protocol.  The second comment would be better served by something like: /* The actor is implemented by a refcounted nsGeolocationRequest, and has an identical lifetime. */

>diff --git a/dom/src/geolocation/PGeolocationRequest.ipdl b/dom/src/geolocation/PGeolocationRequest.ipdl
>+ * The Original Code is mozilla.org code.
>+ *
>+ * The Initial Developer of the Original Code is
>+ *  The Mozilla Foundation
>+ * Portions created by the Initial Developer are Copyright (C) 2010
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *

Contributor.

>+rpc protocol PGeolocationRequest
>+{
>+ manager PIFrameEmbedding;

Get rid of the rpc, and indent the manager line properly.

>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
>@@ -29,17 +29,41 @@
>  * use your version of this file under the terms of the MPL, indicate your
>  * decision by deleting the provisions above and replace them with the notice
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
>+
>+#ifdef MOZ_IPC
>+#include "nsGeolocationOOP.h"
>+#include "nsXULAppAPI.h"

Nix the newline.

>+#include "nsIDocShellTreeOwner.h"
>+#include "nsIDocShellTreeItem.h"
>+#include "nsIWebProgressListener2.h"
>+
>+

And this one.

>@@ -225,16 +249,25 @@ nsGeolocationRequest::GetRequestingWindo
>   NS_ENSURE_ARG_POINTER(aRequestingWindow);
> 
>   nsCOMPtr<nsIDOMWindow> window = do_QueryReferent(mLocator->GetOwner());
>   window.forget(aRequestingWindow);
> 
>   return NS_OK;
> }
> 
>+

And this one.

>@@ -957,12 +991,158 @@ nsGeolocation::WindowOwnerStillExists()
>     nsPIDOMWindow* outer = window->GetOuterWindow();
>     if (!outer || outer->GetCurrentInnerWindow() != window)
>       return PR_FALSE;
>   }
> 
>   return PR_TRUE;
> }
> 
>+void
>+nsGeolocation::RegisterRequestWithPrompt(nsGeolocationRequest* request)
>+{
>+#ifdef MOZ_IPC
>+  if (XRE_GetProcessType() == GeckoProcessType_Content) {
>+
>+    mURI->GetSpec(mURIString);

You should either use the new IPC::URI type for serializing URIs (only if you know that only nsStandardURLs will be passed over the wire) or send the charset along as well.

>+    nsCOMPtr<nsIDocShellTreeItem> item = do_QueryInterface(docshell);
>+    NS_ASSERTION(item, "doc shell tree item is null");
>+    if (!window)
>+      return;

Copy/paste error.  |if (!item)|, please.

>+nsGeolocationRequestProxy::nsGeolocationRequestProxy()
>+{
>+}
>+
>+nsGeolocationRequestProxy::~nsGeolocationRequestProxy()
>+{
>+}

MOZ_COUNT_CTOR/MOZ_COUNT_DTOR

>+NS_IMETHODIMP
>+nsGeolocationRequestProxy::Allow()
>+{
>+  NS_ASSERTION(mParent, "No parent for request");
>+  mozilla::dom::GeolocationRequestParent::Send__delete__(mParent, true);
>+  return NS_OK;
>+}
>+
>+ 

Nix the extra newline.

>diff --git a/dom/src/geolocation/nsGeolocation.h b/dom/src/geolocation/nsGeolocation.h
>   // where the content was loaded from
>   nsCOMPtr<nsIURI> mURI;
>+  nsCString mURIString;

Unneeded.

>diff --git a/dom/src/geolocation/nsGeolocationOOP.h b/dom/src/geolocation/nsGeolocationOOP.h
>+ * The Initial Developer of the Original Code is
>+ *  The Mozilla Foundation
>+ * Portions created by the Initial Developer are Copyright (C) 2010
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *

Author?

>+#ifndef __nsGeolocationOOP_h__
>+#define __nsGeolocationOOP_h__

Modern style is without leading or trailing underscores.

>+namespace mozilla {
>+  namespace dom {
>+
>+    class GeolocationRequestParent : public PGeolocationRequestParent
>+    {
>+      public:
>+        GeolocationRequestParent(nsIDOMWindow *target, nsIDOMElement *element, const nsCString& principal);
>+        virtual ~GeolocationRequestParent();
>+
>+        virtual bool Recvprompt();
>+
>+        nsCString mPrincipal;
>+
>+	nsCOMPtr<nsIDOMWindow> mTarget;
>+	nsCOMPtr<nsIDOMElement> mElement;
>+
>+	nsGeolocationRequestProxy *mProxy;
>+    };
>+
>+  } // namespace dom                                                                                                                                                                                                                   
>+} // namespace mozilla

Indentation.

>+class nsGeolocationRequestProxy : public nsIGeolocationRequest {
>+ public:
>+  nsGeolocationRequestProxy();
>+  virtual ~nsGeolocationRequestProxy();
>+
>+  nsresult Init(mozilla::dom::GeolocationRequestParent* parent);
>+
>+
>+  NS_DECL_ISUPPORTS;
>+  NS_DECL_NSIGEOLOCATIONREQUEST;
>+
>+ private:
>+  // the GeolocationRequestParent owns this.
>+  mozilla::dom::GeolocationRequestParent* mParent;
>+};
>+
>+
>+#endif // __nsGeolocationOOP_h__

The comment is misleading - I assume you mean that the GeolocationRequestParent owns the proxy, but the |this| logically refers to the variable that follows.  Also, nix the paired newlines.

>diff --git a/layout/build/nsLayoutModule.cpp b/layout/build/nsLayoutModule.cpp
>--- a/layout/build/nsLayoutModule.cpp
>+++ b/layout/build/nsLayoutModule.cpp
>@@ -31,16 +31,20 @@
>  * use your version of this file under the terms of the MPL, indicate your
>  * decision by deleting the provisions above and replace them with the notice
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
>+#ifdef MOZ_IPC
>+#include "base/basictypes.h"
>+#endif
>+
> #include "xpcmodule.h"
> #include "nsLayoutStatics.h"
> #include "nsContentCID.h"
> #include "nsContentDLF.h"
> #include "nsContentPolicyUtils.h"
> #include "nsDataDocumentContentPolicy.h"
> #include "nsNoDataProtocolContentPolicy.h"
> #include "nsDOMCID.h"

I suspect you could remove a bunch of these #ifdef blocks if you followed the practice of #including any IPDL-related headers (PGeolocationRequest.h, etc.) first in any headers that want them.

Comment 34

[Doug Turner (:dougt)]

Attachment: patch v.4

Comment 35

[Josh Matthews [:jdm]]

Comment on attachment 445040 [details] [diff] [review]
patch v.4

>diff --git a/dom/ipc/PIFrameEmbedding.ipdl b/dom/ipc/PIFrameEmbedding.ipdl
>@@ -94,16 +96,19 @@ parent:
>     rpc createWindow() returns (PIFrameEmbedding window);
> 
>     PContextWrapper();
> 
>     rpc sendSyncMessageToParent(nsString aMessage, nsString aJSON, PObjectWrapper[] aObjects)
>       returns (nsString[] retval);
> 
>     sendAsyncMessageToParent(nsString aMessage, nsString aJSON);
>+
>+    PGeolocationRequest(nsCString uri);
>+

Same comment as before: we need to either pass an IPC::URI (see netwerk/protocol/http/src/PHttpChannel.ipdl and netwerk/protocol/http/src/HttpChannelParent/Child.cpp for examples) or pass the charset as well.

>diff --git a/dom/src/geolocation/PGeolocationRequest.ipdl b/dom/src/geolocation/PGeolocationRequest.ipdl
>+ * The Original Code is mozilla.org code.
>+ *
>+ * The Initial Developer of the Original Code is
>+ *  The Mozilla Foundation
>+ * Portions created by the Initial Developer are Copyright (C) 2010
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *

Nit: add yourself

>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
>@@ -29,17 +29,40 @@
>  * use your version of this file under the terms of the MPL, indicate your
>  * decision by deleting the provisions above and replace them with the notice
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
>+#ifdef MOZ_IPC
>+#include "nsGeolocationOOP.h"
>+#include "nsXULAppAPI.h"
>+
>+#include "mozilla/dom/PIFrameEmbeddingChild.h"
>+#include "mozilla/dom/PIFrameEmbeddingParent.h"
>+#include "mozilla/dom/ContentProcessChild.h"
>+#include "nsNetUtil.h"
>+
>+#include "nsFrameManager.h"
>+#include "nsFrameLoader.h"
>+#include "nsIFrameLoader.h"
>+
>+#include "nsIDocShellTreeOwner.h"
>+#include "nsIDocShellTreeItem.h"
>+#include "nsIWebProgressListener2.h"
>+
>+
>+#include "nsDOMEventTargetHelper.h"
>+#include "TabChild.h"
>+#endif
>+

Nit: remove the extraneous newline in there.

> #include "nsGeolocation.h"
>+
> #include "nsAutoPtr.h"
> #include "nsCOMPtr.h"
> #include "nsIDOMWindow.h"
> #include "nsDOMClassInfo.h"
> #include "nsComponentManagerUtils.h"
> #include "nsICategoryManager.h"
> #include "nsISupportsPrimitives.h"
> #include "nsServiceManagerUtils.h"

Nit: here also.

>+    nsCString uriString;
>+    mURI->GetSpec(uriString);
>+    mozilla::dom::PGeolocationRequestChild* a = child->SendPGeolocationRequestConstructor(request, uriString);

Nit: split this line, please?

>+  nsCOMPtr<nsIGeolocationPrompt> prompt = do_GetService(NS_GEOLOCATION_PROMPT_CONTRACTID);
>+  NS_ASSERTION(prompt, "null geolocation prompt.  geolocation will not work without one.");
>+  if (prompt == nsnull)
>+    return;
>+  
>+  prompt->Prompt(request);
>+}

Nit: either make this |if (prompt) ...| or |if (!prompt) ...|, doesn't really matter which.

>+nsresult
>+nsGeolocationRequestProxy::Init(mozilla::dom::GeolocationRequestParent* parent)
>+{
>+  NS_ASSERTION(parent, "null parent");
>+  mParent = parent;
>+
>+  nsCOMPtr<nsIGeolocationPrompt> prompt = do_GetService(NS_GEOLOCATION_PROMPT_CONTRACTID);
>+  NS_ASSERTION(prompt, "null geolocation prompt.  geolocation will not work without one.");
>+  prompt->Prompt(this);
>+  return NS_OK;
>+}

No check for null prompt?  Make this and the above agree, one way or the other.

>+bool
>+GeolocationRequestParent::Recvprompt()
>+{
>+  mProxy = new nsGeolocationRequestProxy();
>+  NS_ASSERTION(mProxy, "Alloc of request proxy failed");
>+  (void) mProxy->Init(this);
>+  return true;
>+}

If you do end up checking for a null prompt, it's probably worth checking for Init failing and calling mProxy->Cancel() in that case.

>diff --git a/dom/src/geolocation/nsGeolocationOOP.h b/dom/src/geolocation/nsGeolocationOOP.h
>+ * The Original Code is mozilla.org code.
>+ *
>+ * The Initial Developer of the Original Code is
>+ *  The Mozilla Foundation
>+ * Portions created by the Initial Developer are Copyright (C) 2010
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *

Nit: Add yourself.

>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either the GNU General Public License Version 2 or later (the "GPL"), or
>+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the MPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
>+
>+#ifndef nsGeolocationOOP_h
>+#define nsGeolocationOOP_h
>+
>+#include "base/basictypes.h"
>+
>+#include "nspr.h"
>+#include "mozilla/net/NeckoCommon.h"

Whaaa? What's NeckoCommon doing here?

>+class GeolocationRequestParent : public PGeolocationRequestParent
>+{
>+ public:
>+  GeolocationRequestParent(nsIDOMElement *element, const nsCString& principal);
>+  virtual ~GeolocationRequestParent();
>+  
>+  virtual bool Recvprompt();
>+  

Make the following members private, please.

>+  nsCString mPrincipal;
>+  
>+  nsCOMPtr<nsIDOMElement> mElement;
>+  nsGeolocationRequestProxy *mProxy;
>+};

>+ private:
>+  // Non-owning pointer to the GeolocationRequestParent object which owns this proxy.
>+  mozilla::dom::GeolocationRequestParent* mParent;
>+};
>+
>+

Nit: Extra newline.

r=me with concerns addressed.

Comment 36

[Doug Turner (:dougt)]

Attachment: patch v.5

Comment 37

[Josh Matthews [:jdm]]

Comment on attachment 445122 [details] [diff] [review]
patch v.5

>+nsresult
>+nsGeolocationRequestProxy::Init(mozilla::dom::GeolocationRequestParent* parent)
>+{
>+  NS_ASSERTION(parent, "null parent");
>+  mParent = parent;
>+
>+  nsCOMPtr<nsIGeolocationPrompt> prompt = do_GetService(NS_GEOLOCATION_PROMPT_CONTRACTID);
>+  NS_ASSERTION(prompt, "null geolocation prompt.  geolocation will not work without one.");
>+  if (prompt)
>+    prompt->Prompt(this);
>+  return NS_OK;
>+}
  
>+bool
>+GeolocationRequestParent::Recvprompt()
>+{
>+  mProxy = new nsGeolocationRequestProxy();
>+  NS_ASSERTION(mProxy, "Alloc of request proxy failed");
>+  if (NS_FAILED(mProxy->Init(this)))
>+    mProxy->Cancel();
>+  return true;
>+}

The |if (prompt)| should instead check for absence of the prompt, and return an error.  With that fixed, r=me.

Comment 38

[Doug Turner (:dougt)]

http://hg.mozilla.org/projects/electrolysis/rev/aa0a2fc9daf6
http://hg.mozilla.org/projects/electrolysis/rev/6989da8064eb (missing file)
http://hg.mozilla.org/projects/electrolysis/rev/91789d0400b9 (missing file)

Comment 39

[Jae-Seong Lee-Russo]

Attachment: fix windows build

c:\mozilla-build\e10s\dom\ipc\TabParent.h(55) : warning C4099: 'gfxMatrix' : type name first seen using 'struct' now seen using 'class'
        c:\mozilla-build\e10s\obj-i686-pc-mingw32\dist\include\gfxMatrix.h(67) : see declaration of 'gfxMatrix'
c:/mozilla-build/e10s/dom/ipc/TabParent.cpp(455) : error C4430: missing type specifier - int assumed. Note: C++ does not support default-int
c:/mozilla-build/e10s/dom/ipc/TabParent.cpp(455) : error C2751: 'IPC::URI::{ctor}' : the name of a function parameter cannot be qualified
c:/mozilla-build/e10s/dom/ipc/TabParent.cpp(455) : error C2143: syntax error : missing ',' before '&'
c:/mozilla-build/e10s/dom/ipc/TabParent.cpp(456) : error C2511: 'mozilla::dom::PGeolocationRequestParent *mozilla::dom::TabParent::AllocPGeolocationRequest(const int)' : overloaded member function not found in 'mozilla::dom::TabParent'
        c:\mozilla-build\e10s\dom\ipc\TabParent.h(91) : see declaration of 'mozilla::dom::TabParent'

Comment 40

[Doug Turner (:dougt)]

Thank you for the patch.

http://hg.mozilla.org/projects/electrolysis/rev/cc9333c5a6a9