Add an isHandlingUserInput attribute to nsIContentPermissionRequest

RESOLVED FIXED in Firefox 59

Status

()

enhancement
P2
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: johannh, Assigned: johannh)

Tracking

(Blocks 2 bugs)

Trunk
Firefox 59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

(Whiteboard: [fxprivacy])

Attachments

(1 attachment)

Assignee

Description

2 years ago
It would be helpful for future permission notification work if we had a way to tell if a permission request is the result of a user action or not.

The immediate use case would be bug 1334496 where we would like to turn on autofocus for permission notifications by default but might want to restrict stealing focus from content to when the user triggered the request.

This could also be helpful for implementing measures to combat permission notification spam (e.g. preventing on-load permission requests) though there are no concrete plans on how we would use it right now.
Iteration: --- → 55.1 - Mar 20
Priority: -- → P2
Assignee

Comment 2

2 years ago
Comment on attachment 8848113 [details]
Bug 1345424 - Add an isHandlingUserInput attribute to permission requests.

Mike, this has been sitting in my queue for some time and I'd like to finally take the time to finish it up properly. This is a WIP patch that implements the functionality I'm interested in, though there are no tests yet.

Maybe you can provide some initial feedback on this? I'd also appreciate suggestions for potential reviewers for the DOM parts, I'll split up the commit as needed.

I'm also not sure how to handle the FlyWeb case right now. Can that even be user-triggered?
Attachment #8848113 - Flags: feedback?(mconley)
Assignee

Comment 3

2 years ago
Oh I should also note that WebRTC doesn't seem to use nsIContentPermissionRequest and would therefore probably need to be solved separate to this bug.
Whiteboard: [fxprivacy] [triage] → [fxprivacy]
Comment on attachment 8848113 [details]
Bug 1345424 - Add an isHandlingUserInput attribute to permission requests.

This looks like the right idea to me.

Part of me wonders whether we should take the onus of passing along the IsHandlingUserInput information away from the callers.

I wonder if it'd be possible to have a partial implementation of nsIContentPermissionRequest that the others can subclass from which would take care of detecting the user input state on instantiation?
Attachment #8848113 - Flags: feedback?(mconley) → feedback+
Assignee

Comment 5

2 years ago
Yeah, that makes sense. I'll look into it, thanks!
Assignee

Updated

2 years ago
No longer blocks: 1334496
Assignee

Updated

2 years ago
Blocks: 1354551
Assignee

Updated

2 years ago
Blocks: 1375683
Comment hidden (mozreview-request)
Assignee

Comment 7

2 years ago
So I did not go for Mike's suggestion in this patch, partly because I'm not sure how to partially implement an IDL interface and partly because I don't feel that there's a lot of added complexity for implementors to justify this overhead. I hope that works for everyone :)

Comment 9

2 years ago
mozreview-review
Comment on attachment 8848113 [details]
Bug 1345424 - Add an isHandlingUserInput attribute to permission requests.

https://reviewboard.mozilla.org/r/121080/#review208106


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: dom/flyweb/FlyWebService.cpp:139
(Diff revision 2)
>    {
>      *aRequestingElement = nullptr;
>      return NS_OK;
>    }
>  
> +  NS_IMETHOD GetIsHandlingUserInput(bool *aIsHandlingUserInput) override

Warning: 'virtual' is redundant since the function is already declared 'override' [clang-tidy: modernize-use-override]
Assignee

Comment 10

2 years ago
Ah, I forgot to note that the FlyWeb implementation here is just a dummy because we'll hopefully remove it in the next days in bug 1374574.

Comment 11

2 years ago
mozreview-review
Comment on attachment 8848113 [details]
Bug 1345424 - Add an isHandlingUserInput attribute to permission requests.

https://reviewboard.mozilla.org/r/121080/#review208146

::: dom/base/nsContentPermissionHelper.h:83
(Diff revision 2)
>  
>    static PContentPermissionRequestParent*
>    CreateContentPermissionRequestParent(const nsTArray<PermissionRequest>& aRequests,
>                                         Element* element,
>                                         const IPC::Principal& principal,
> +                                       const bool isHandlingUserInput,

params should be called aSomething.

::: dom/base/nsContentPermissionHelper.cpp:134
(Diff revision 2)
>  {
>   public:
>    ContentPermissionRequestParent(const nsTArray<PermissionRequest>& aRequests,
>                                   Element* element,
> -                                 const IPC::Principal& principal);
> +                                 const IPC::Principal& principal,
> +                                 const bool isHandlingUserInput);

here as well.

::: dom/base/nsContentPermissionHelper.cpp:359
(Diff revision 2)
>  
>  /* static */ PContentPermissionRequestParent*
>  nsContentPermissionUtils::CreateContentPermissionRequestParent(const nsTArray<PermissionRequest>& aRequests,
>                                                                 Element* element,
>                                                                 const IPC::Principal& principal,
> +                                                               const bool isHandlingUserInput,

here as well.

::: dom/base/nsContentPermissionHelper.cpp:665
(Diff revision 2)
>    elem.forget(aRequestingElement);
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP
> +nsContentPermissionRequestProxy::GetIsHandlingUserInput(bool *aIsHandlingUserInput)

bool*

::: dom/geolocation/nsGeolocation.cpp:404
(Diff revision 2)
>    *aRequestingElement = nullptr;
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP
> +nsGeolocationRequest::GetIsHandlingUserInput(bool *aIsHandlingUserInput)

bool*

::: dom/geolocation/nsGeolocation.cpp:571
(Diff revision 2)
>      if (isTooOld) {
>        return;
>      }
>    }
>  
> -  RefPtr<Position> wrapped;
> +  RefPtr<mozilla::dom::Position> wrapped;

This seems unrelated.

::: dom/geolocation/nsGeolocation.cpp:1356
(Diff revision 2)
>    *aRv = mLastWatchId++;
>  
>    RefPtr<nsGeolocationRequest> request =
>      new nsGeolocationRequest(this, Move(aCallback), Move(aErrorCallback),
>                               Move(aOptions),
> -                             static_cast<uint8_t>(mProtocolType), true, *aRv);
> +                             static_cast<uint8_t>(mProtocolType), true, EventStateManager::IsHandlingUserInput(), *aRv);

80chars max. indentation.

::: dom/notification/Notification.cpp:606
(Diff revision 2)
>    *aElement = nullptr;
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP
> +NotificationPermissionRequest::GetIsHandlingUserInput(bool *aIsHandlingUserInput)

bool*

::: dom/notification/Notification.cpp:1826
(Diff revision 2)
>    if (aCallback.WasPassed()) {
>      permissionCallback = &aCallback.Value();
>    }
> +  bool isHandlingUserInput = EventStateManager::IsHandlingUserInput();
>    nsCOMPtr<nsIRunnable> request =
> -    new NotificationPermissionRequest(principal, window, promise, permissionCallback);
> +    new NotificationPermissionRequest(principal, isHandlingUserInput, window, promise, permissionCallback);

indentation

::: dom/quota/StorageManager.cpp:727
(Diff revision 2)
>  
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP
> +PersistentStoragePermissionRequest::GetIsHandlingUserInput(bool *aIsHandlingUserInput)

bool*
Attachment #8848113 - Flags: review?(amarchesini) → review+
Comment on attachment 8848113 [details]
Bug 1345424 - Add an isHandlingUserInput attribute to permission requests.

Smaug, can you take a look at the use of EventStateManager::IsHandlingUserInput() ?
Attachment #8848113 - Flags: review?(bugs)

Comment 13

2 years ago
mozreview-review
Comment on attachment 8848113 [details]
Bug 1345424 - Add an isHandlingUserInput attribute to permission requests.

https://reviewboard.mozilla.org/r/121080/#review208208
Attachment #8848113 - Flags: review?(bugs) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 16

2 years ago
mozreview-review-reply
Comment on attachment 8848113 [details]
Bug 1345424 - Add an isHandlingUserInput attribute to permission requests.

https://reviewboard.mozilla.org/r/121080/#review208146

> This seems unrelated.

Including EventStatemanager.h introduced a namespace conflict with mozilla::Position.
Comment hidden (mozreview-request)

Comment 18

2 years ago
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6306f81cc295
Add an isHandlingUserInput attribute to permission requests. r=baku,smaug

Comment 19

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6306f81cc295
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
You need to log in before you can comment on or make changes to this bug.