Closed Bug 1345424 Opened 7 years ago Closed 7 years ago

Add an isHandlingUserInput attribute to nsIContentPermissionRequest

Categories

(Firefox :: Site Permissions, enhancement, P2)

Product:
Firefox
Component:
Site Permissions
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 59
Iteration:
55.1 - Mar 20
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: johannh, Assigned: johannh)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxprivacy])

Attachments

(1 file)

Bug 1345424 - Add an isHandlingUserInput attribute to permission requests.
59 bytes, text/x-review-board-request
baku
: review+
Details 
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
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)
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+
Yeah, that makes sense. I'll look into it, thanks!
No longer blocks: 1334496
Blocks: 1354551
Blocks: 1375683
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 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]
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 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 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 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.
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6306f81cc295
Add an isHandlingUserInput attribute to permission requests. r=baku,smaug
https://hg.mozilla.org/mozilla-central/rev/6306f81cc295
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: