595445 - Factor out geolocation prompt into something that can be reused - Part 2

Status: RESOLVED FIXED

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

Description

[Doug Turner (:dougt)]

In bug 594261, I move geolocation over to use the content permission prompt.  During this work, it became clear we could also move out the base classes that geolocation into dom/base and rename them something appropriate.  This will allow indexedDB and desktop notifications to share the same base classes.

Comment 1

[Doug Turner (:dougt)]

Attachment: patch v.1

Comment 2

[Doug Turner (:dougt)]

Attachment: patch v.1

missed a file

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 474336 [details] [diff] [review]
patch v.1

Some drive-by comments, obviously only applicable if Olli doesn't disagree.

>diff --git a/dom/base/Makefile.in b/dom/base/Makefile.in
>--- a/dom/base/Makefile.in
>+++ b/dom/base/Makefile.in
>@@ -73,16 +73,17 @@ EXPORTS = \
>   nsIScriptObjectOwner.h \
>   nsIScriptObjectPrincipal.h \
>   nsIScriptRuntime.h \
>   nsIScriptTimeoutHandler.h \
>   nsPIDOMWindow.h \
>   nsPIWindowRoot.h \
>   nsFocusManager.h \
>   nsWrapperCache.h \
>+  nsContentPermissionHelper.h \

mozilla/dom/ContentPermissionHelper.h?

>@@ -96,16 +97,17 @@ CPPSRCS =			\
> 	nsHistory.cpp		\
> 	nsMimeTypeArray.cpp	\
> 	nsPluginArray.cpp	\
> 	nsWindowRoot.cpp	\
> 	nsDOMClassInfo.cpp	\
> 	nsScriptNameSpaceManager.cpp \
> 	nsDOMScriptObjectFactory.cpp \
> 	nsQueryContentEventResult.cpp \
>+        nsContentPermissionHelper.cpp \

This line probably wants a tab like the others. Also, ContentPermissionHelper.cpp?

>diff --git a/dom/base/nsContentPermissionHelper.cpp b/dom/base/nsContentPermissionHelper.cpp
>new file mode 100644
>--- /dev/null
>+++ b/dom/base/nsContentPermissionHelper.cpp
>+nsresult
>+nsContentPermissionRequestProxy::Init(const nsACString & type,
>+				      mozilla::dom::ContentPermissionRequestParent* parent)
>+{
>+  NS_ASSERTION(parent, "null parent");
>+  mParent = parent;
>+  mType   = type;
>+
>+  nsCOMPtr<nsIContentPermissionPrompt> prompt = do_GetService(NS_CONTENT_PERMISSION_PROMPT_CONTRACTID);
>+  if (!prompt) {
>+    return NS_ERROR_FAILURE;
>+  }
>+
>+  (void) prompt->Prompt(this);

Not your code, but I expected `unused << prompt->Prompt(this);`.

>+namespace mozilla {
>+namespace dom {
>+
>+ContentPermissionRequestParent::ContentPermissionRequestParent(const nsACString& type,
>+							       nsIDOMElement *element,
>+							       const IPC::URI& uri)
>+{
>+  MOZ_COUNT_CTOR(ContentPermissionRequestParent);
>+  
>+  mURI       = uri;
>+  mElement   = element;
>+  mType      = type;
>+}
>+
>+ContentPermissionRequestParent::~ContentPermissionRequestParent()
>+{
>+  MOZ_COUNT_DTOR(ContentPermissionRequestParent);
>+}
>+  

Trailing whitespace on this line.

Out of scope for this bug, but we still use the aArgument style, right?

>diff --git a/dom/src/geolocation/nsGeolocationOOP.h b/dom/base/nsContentPermissionHelper.h
>rename from dom/src/geolocation/nsGeolocationOOP.h
>rename to dom/base/nsContentPermissionHelper.h
>--- a/dom/src/geolocation/nsGeolocationOOP.h
>+++ b/dom/base/nsContentPermissionHelper.h
>@@ -30,59 +30,60 @@
>  * 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
>+#ifndef nsContentPermissionHelper_h
>+#define nsContentPermissionHelper_h
> 
> #include "base/basictypes.h"
> 
>-#include "nsIGeolocationProvider.h"
> #include "nsIContentPermissionPrompt.h"
> #include "nsString.h"
> #include "nsIDOMElement.h"
> 
> #include "mozilla/dom/PContentPermissionRequestParent.h"
> 
>-class nsGeolocationRequestProxy;
>+class nsContentPermissionRequestProxy;
> 
> namespace mozilla {
> namespace dom {
> 
>-class GeolocationRequestParent : public PContentPermissionRequestParent
>+class ContentPermissionRequestParent : public PContentPermissionRequestParent
> {
>  public:
>-  GeolocationRequestParent(nsIDOMElement *element, const IPC::URI& principal);
>-  virtual ~GeolocationRequestParent();
>+  ContentPermissionRequestParent(const nsACString& type, nsIDOMElement *element, const IPC::URI& principal);
>+  virtual ~ContentPermissionRequestParent();
>   
>   nsCOMPtr<nsIURI>           mURI;
>   nsCOMPtr<nsIDOMElement>    mElement;
>-  nsCOMPtr<nsGeolocationRequestProxy> mProxy;
>+  nsCOMPtr<nsContentPermissionRequestProxy> mProxy;

This was wrong before, but this needs to be nsRefPtr or use the appropriate interface.

>+  nsCString mType;
> 
>  private:  
>   virtual bool Recvprompt();
> };
>   
> } // namespace dom
> } // namespace mozilla
> 
>-class nsGeolocationRequestProxy : public nsIContentPermissionRequest
>+class nsContentPermissionRequestProxy : public nsIContentPermissionRequest
> {
>  public:
>-  nsGeolocationRequestProxy();
>-  virtual ~nsGeolocationRequestProxy();
>+  nsContentPermissionRequestProxy();
>+  virtual ~nsContentPermissionRequestProxy();
>   
>-  nsresult Init(mozilla::dom::GeolocationRequestParent* parent);
>+  nsresult Init(const nsACString& type, mozilla::dom::ContentPermissionRequestParent* parent);
>   
>   NS_DECL_ISUPPORTS;
>   NS_DECL_NSICONTENTPERMISSIONREQUEST;
> 
>  private:
>-  // Non-owning pointer to the GeolocationRequestParent object which owns this proxy.
>-  mozilla::dom::GeolocationRequestParent* mParent;
>+  // Non-owning pointer to the ContentPermissionRequestParent object which owns this proxy.
>+  mozilla::dom::ContentPermissionRequestParent* mParent;
>+  nsCString mType;
> };
>-#endif // nsGeolocationOOP_h
>+#endif // nsContentPermissionHelper_h

Why nsContentPermissionRequestProxy rather than mozilla::dom::ContentPermissionRequestProxy?

>diff --git a/dom/src/geolocation/nsGeolocation.h b/dom/src/geolocation/nsGeolocation.h
>--- a/dom/src/geolocation/nsGeolocation.h
>+++ b/dom/src/geolocation/nsGeolocation.h
>@@ -58,17 +58,17 @@
> #include "nsIDOMGeoPosition.h"
> #include "nsIDOMGeoPositionError.h"
> #include "nsIDOMGeoPositionCallback.h"
> #include "nsIDOMGeoPositionErrorCallback.h"
> #include "nsIDOMGeoPositionOptions.h"
> #include "nsIDOMNavigatorGeolocation.h"
> 
> #include "nsPIDOMWindow.h"
>-
>+#include "nsContentPermissionHelper.h"

Do you need to add this?

Comment 4

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

Comment on attachment 474336 [details] [diff] [review]
patch v.1


>+nsContentPermissionRequestProxy::Init(const nsACString & type,
>+				      mozilla::dom::ContentPermissionRequestParent* parent)
You have tabs in the patch which is why the indentation is wrong

>+nsContentPermissionRequestProxy::GetUri(nsIURI * *aRequestingURI)
>+{
>+  NS_ENSURE_ARG_POINTER(aRequestingURI);
>+  NS_ASSERTION(mParent, "No parent for request");
Useless assertion.

Is it guaranteed that mParent isn't ever null?


>+NS_IMETHODIMP
>+nsContentPermissionRequestProxy::Cancel()
>+{
>+  NS_ASSERTION(mParent, "No parent for request");
>+ unused << mozilla::dom::ContentPermissionRequestParent::Send__delete__(mParent, false);
I don't know why the need for unused


>+  mParent = nsnull;
So after calling cancel, many other methods will just crash.
Could you please add necessary null checks.
NS_ENSURE_STATE(mParent);


>+ContentPermissionRequestParent::ContentPermissionRequestParent(const nsACString& type,
>+							       nsIDOMElement *element,
>+							       const IPC::URI& uri)
Parameters should be in form aParameterName.

Comment 5

[Doug Turner (:dougt)]

Good driveby comments!  thanks.


> This was wrong before, but this needs to be nsRefPtr or use the appropriate interface.

Good catch - can you file a follow up bug.  I am concerned about changing this right before a code freeze.


mParent can be null (if you call cancel, for example).  I am going to add:

  if (mParent == nsnull)
    return NS_ERROR_FAILURE;

in the places where we have assertions now.

Comment 6

[walter sheets]

Just pulled from the hg repo and get this error

dom/base/nsContentPermissionHelper.h:41:29: error: base/basictypes.h: No such file or directory

while building thunderbird.  Haven't tried building firefox yet.

Comment 7

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/1d98cd73f226