1045975 - Use nsIDOMWindowUtils.askPermission in all types of permission request

Status: RESOLVED FIXED

(Core :: Permission Manager, defect)

Description

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

nsIDOMWindowUtils.askPermission already takes care of permission prompt for both oop and non-oop case. Therefore, we don't have to inherit PCOMContentPermissionRequestChild anymore while implementing nsIContentPermissionRequest.

Here is the list of class that can be simplified:
- CameraPermissionRequest
- DeviceStorageCursorRequest
- DeviceStorageRequest
- nsDOMDeviceStorageCursor
- FileSystemPermissionRequest
- MediaPermissionRequest
- nsGeolocationRequest
- DesktopNotificationRequest
- NotificationPermissionRequest

Comment 1

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: [WIP] use-nsidomwindowutils-askpermission.patch

compiled but haven't tested

Comment 2

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: use-nsidomwindowutils-askpermission.patch

summary of changes:
1. remove the inheritance of PCOMContentPermissionRequestChild from all nsIContentPermissionRequest subclasses
2. RemotePermissionRequest doesn't need to implement nsIContentPermissionRequest, nsDOMWindowUtils.askPermission() can use the passed-in request object directly for non-oop case.
3. remove PCOMContentPermissionRequestChild and make RemotePermissionRequest directly inherit PContentPermissionRequestChild

try: https://tbpl.mozilla.org/?tree=Try&rev=db7371eb7a54

Comment 3

[[:fabrice] Fabrice Desré]

Comment on attachment 8465359 [details] [diff] [review]
use-nsidomwindowutils-askpermission.patch

Review of attachment 8465359 [details] [diff] [review]:
-----------------------------------------------------------------

Overall lgtm, less code is a win! You have some build failures on try (yeah for windows #define !). Also I'd like someone like smaug to do the review, since that touches code I'm not a peer of.

::: dom/base/nsContentPermissionHelper.h
@@ +123,4 @@
>  
>    nsCOMPtr<nsIContentPermissionRequest> mRequest;
>    nsCOMPtr<nsPIDOMWindow>               mWindow;
> +  bool mIPCOpen;

nit: alignment

Comment 4

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: use-nsidomwindowutils-askpermission.patch

fix windows build error (old #undef trick) and migrate nsPointerLockPermissionRequest to use nsIDOMWindowUtils.askPermission().

try: https://tbpl.mozilla.org/?tree=Try&rev=8925a29f3fdc

Comment 5

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

Comment on attachment 8466036 [details] [diff] [review]
use-nsidomwindowutils-askpermission.patch

I like the basic approach, but makes no sense to create DOMWindowUtils object
just for this.  do_GetInterface(mWindow) creates a DOMWindowUtils object, if needed. (DOMWindowUtils is mainly for JS stuff).

Could you move whatever nsDOMWindowUtils::AskPermission does to be
a static method in nsContentUtils, and then
make nsDOMWindowUtils::AskPermission to call that.
All the C++ code should use nsContentUtils, and JS code can access the
same stuff via nsIDOMWindowUtils

Comment 6

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

Or other, perhaps better approach, is to put the static method to nsContentPermissionHelper.h.

Comment 7

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: use-nsidomwindowutils-askpermission.patch, v2

update according to review comment, introduce |nsContentPermissionUtils| for aggregating all the static functions in nsContentPermissionHelper.cpp including |AskPermission|.

try: https://tbpl.mozilla.org/?tree=Try&rev=e0379e840593

Comment 8

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: use-nsidomwindowutils-askpermission.patch, v2.1

minor fix for do_GetService compile error.

https://tbpl.mozilla.org/?tree=Try&rev=3bad9a2b69c8

Comment 9

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

Comment on attachment 8466665 [details] [diff] [review]
use-nsidomwindowutils-askpermission.patch, v2.1

># HG changeset patch
># User Shih-Chiang Chien <[e-mail]>
># Date 1406877740 -28800
>#      Fri Aug 01 15:22:20 2014 +0800
># Node ID d383d3ee9f213bebee921edcb06c4dad0490ef81
># Parent >-    return NS_OK;
>+    nsCOMPtr<nsPIDOMWindow> window = doc->GetWindow();
>+    return nsContentPermissionUtils::AskPermission(this, window);
The old code never returns non-NS_OK, and I think we want that behavior here.
So, 
nsContentPermissionUtils::AskPermission(this, GetInnerWindow());
return NS_OK;

>+nsContentPermissionUtils::AskPermission(nsIContentPermissionRequest* aRequest, nsPIDOMWindow* aWindow)
You should probably do
MOZ_ASSERT(!aWindow || aWindow->IsInnerWindow());
NS_ENSURE_STATE(aWindow && aWindow->IsCurrentInnerWindow());



>+  void IPDLAddRef() {
{ goes to its own line.
Same also elsewhere.

> class RequestPromptEvent : public nsRunnable
> {
> public:
>-  RequestPromptEvent(nsGeolocationRequest* request)
>-    : mRequest(request)
>+  RequestPromptEvent(nsGeolocationRequest* aRequest, nsWeakPtr aWindow)
>+    : mRequest(aRequest)
>+    , mWindow(aWindow)
>   {
>   }
> 
>   NS_IMETHOD Run() {
>-    nsCOMPtr<nsIContentPermissionPrompt> prompt = do_CreateInstance(NS_CONTENT_PERMISSION_PROMPT_CONTRACTID);
>-    if (prompt) {
>-      prompt->Prompt(mRequest);
>+    nsCOMPtr<nsPIDOMWindow> window = do_QueryReferent(mWindow);
>+    if (!window) {
>+      return NS_OK;
>     }
Is AskPermission was null-safe, you wouldn't need this if check.



> public:
>   NS_DECL_ISUPPORTS_INHERITED
>   NS_DECL_NSICONTENTPERMISSIONREQUEST
> 
>   DesktopNotificationRequest(DesktopNotification* notification)
>     : mDesktopNotification(notification) {}
> 
>   NS_IMETHOD Run() MOZ_OVERRIDE
>   {
>-    nsCOMPtr<nsIContentPermissionPrompt> prompt =
>-      do_CreateInstance(NS_CONTENT_PERMISSION_PROMPT_CONTRACTID);
>-    if (prompt) {
>-      prompt->Prompt(this);
>+    nsCOMPtr<nsPIDOMWindow> window = mDesktopNotification->GetOwner();
>+
>+    if (!window) {
>+      return NS_OK;
>     }
Is AskPermission was null-safe, you wouldn't need this if check.


I could take still a quick look on an updated patch.

Comment 10

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Olli Pettay [:smaug] from comment #9)
> Comment on attachment 8466665 [details] [diff] [review]
> use-nsidomwindowutils-askpermission.patch, v2.1
> 
> ># HG changeset patch
> ># User Shih-Chiang Chien <[e-mail]>
> ># Date 1406877740 -28800
> >#      Fri Aug 01 15:22:20 2014 +0800
> ># Node ID d383d3ee9f213bebee921edcb06c4dad0490ef81
> ># Parent >-    return NS_OK;
> >+    nsCOMPtr<nsPIDOMWindow> window = doc->GetWindow();
> >+    return nsContentPermissionUtils::AskPermission(this, window);
> The old code never returns non-NS_OK, and I think we want that behavior here.
> So, 
> nsContentPermissionUtils::AskPermission(this, GetInnerWindow());
> return NS_OK;
> 
The old code will return non-NS_OK via those NS_ENSURE_SUCCESS statement, do you think we should change it to always return NS_OK?

Comment 11

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: use-nsidomwindowutils-askpermission.patch, v3

update according to review comments, except for making nsDOMWindowUtils::AskPermission() always return NS_OK.

Comment 12

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

(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #10)
> The old code will return non-NS_OK via those NS_ENSURE_SUCCESS statement, do
> you think we should change it to always return NS_OK?
The old code does
-    nsCOMPtr<nsIContentPermissionPrompt> prompt =
-      do_CreateInstance(NS_CONTENT_PERMISSION_PROMPT_CONTRACTID);
-    if (prompt) {
-      prompt->Prompt(this);
-    }
-
-    return NS_OK;
so it doesn't return non-NS_OK.
I was talking about the code in nsDocument.cpp, not WindowUtils.

Comment 13

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: use-nsidomwindowutils-askpermission.patch, v3.1

Sorry I misread your comment, nsDocument.cpp and FileSystemPermissionRequest.cpp is updated to always return NS_OK for the compliance of old code.

try: https://tbpl.mozilla.org/?tree=Try&rev=e9345ce80340

Comment 14

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

Comment on attachment 8467479 [details] [diff] [review]
use-nsidomwindowutils-askpermission.patch, v3.1

>+    nsCOMPtr<nsPIDOMWindow> window = doc->GetWindow();
You wan GetInnerWindow() here

Comment 15

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: use-nsidomwindowutils-askpermission.patch, v3.2

update according to review comment, carry r+.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b44017765ac

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Backed out for Windows non-unified bustage. You can disable unified builds locally by setting --disable-unified-compilation in your .mozconfig and on Try by adding it to build/mozconfig.common.override. Sorry for the hassle :(
https://hg.mozilla.org/integration/mozilla-inbound/rev/fcedb0c795f7

https://tbpl.mozilla.org/php/getParsedLog.php?id=45439732&tree=Mozilla-Inbound

Comment 18

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

windows.h is causing the trouble, again. GetDiskFreeSpace is been redefined to GetDisFreeSpaceW. Adjusting header file including sequence will solve our problem.

https://tbpl.mozilla.org/?tree=Try&rev=580d603cd045

Comment 19

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: use-nsidomwindowutils-askpermission.patch, v3.3

fix windows build error on non-unified build setting, carry r+.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/be762cafe347

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/be762cafe347