Closed Bug 1299061 Opened 3 years ago Closed 3 years ago

[Presentation WebAPI] Expose the browser that the request was originated in

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: chunmin, Assigned: chunmin)

References

Details

(Whiteboard: [ETA 9/2])

Attachments

(1 file)

To prompt the device selection UI on webpage, we need to expose the associated <xul:browser> for the request like the nsIContentPermissionPrompt.

Maybe we add a property to nsIPresentationDeviceRequest to get it(e.g., nsIPresentationDeviceRequest.chromeEventHandler or nsIPresentationDeviceRequest.browser).
Blocks: 1289974
Assignee: nobody → cchang
Comment on attachment 8786233 [details]
Bug 1299061 - Expose the browser that the request was originated in;

https://reviewboard.mozilla.org/r/75226/#review73102

r- because of that nsCOMPtr. Make it nsWeakPtr and I'll do a quick re-review.

::: dom/presentation/PresentationRequest.cpp:193
(Diff revision 1)
>    if(NS_WARN_IF(!service)) {
>      promise->MaybeReject(NS_ERROR_DOM_OPERATION_ERR);
>      return promise.forget();
>    }
>  
> +  nsCOMPtr<nsIDOMEventTarget> handler =

So this code can run in child process too, in which case chrome event handler is so called "nsWindowRoot" object. Is that what we want?
I guess, based on the other parts of the patch, yes.

::: dom/presentation/PresentationService.cpp:87
(Diff revision 1)
>  
>    nsString mRequestUrl;
>    nsString mId;
>    nsString mOrigin;
>    uint64_t mWindowId;
> +  nsCOMPtr<nsIDOMEventTarget> mChromeEventHandler;

Since PresentationDeviceRequest isn't cycle collectable, I'm a bit worried about this nsCOMPtr which is pointing to a cycle collectable object.
Could you make this nsWeakPtr. DOM Element implementation support use of nsWeakPtr
(do_GetWeakReference / do_QueryReferent)

::: dom/presentation/ipc/PresentationIPCService.cpp:61
(Diff revision 1)
>  PresentationIPCService::StartSession(const nsAString& aUrl,
>                                       const nsAString& aSessionId,
>                                       const nsAString& aOrigin,
>                                       const nsAString& aDeviceId,
>                                       uint64_t aWindowId,
> +                                     nsIDOMEventTarget *aEventTarget,

Nit, nsIDOMEventTarget* aEventTarget

::: dom/presentation/ipc/PresentationIPCService.cpp:68
(Diff revision 1)
>  {
>    if (aWindowId != 0) {
>      AddRespondingSessionId(aWindowId, aSessionId);
>    }
>  
> +  nsPIDOMWindowInner* window =

Ahaa, we don't use aEventTarget in child process. I see.
Attachment #8786233 - Flags: review?(bugs) → review-
Comment on attachment 8786233 [details]
Bug 1299061 - Expose the browser that the request was originated in;

https://reviewboard.mozilla.org/r/75226/#review73536

::: dom/presentation/interfaces/nsIPresentationService.idl:60
(Diff revision 2)
>                        for prompt device selection dialog.
>     * @param windowId: The inner window ID associated with the presentation
>     *                  session. (0 implies no window ID since no actual window
>     *                  uses 0 as its ID. Generally it's the case the window is
>     *                  located in different process from this service)
> +   * @param eventTarget: The XUL browser element that the request was originated

Well, you don't pass xul:browser element in child process, but nsWindowRoot object, so tweak the comment a bit. Maybe say
"The chrome event handler, in particular XUL browser element in parent process, that the request was originated in"
Attachment #8786233 - Flags: review?(bugs) → review+
Whiteboard: [ETA 9/2]
Keywords: checkin-needed
Whiteboard: [ETA 9/2]
Whiteboard: [ETA 9/2]
Needs rebasing. Also, please clear the pending issues in MozReview so that the patch can be autolanded.
Keywords: checkin-needed
Already rebase. Please try again. Thx!
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/85a53a4c3778
Expose the browser that the request was originated in; r=smaug
Keywords: checkin-needed
OK. I've rebase again. Hope there is no more needs to rebase.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/298ebd8ce251
Expose the browser that the request was originated in; r=smaug
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/298ebd8ce251
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Depends on: 1306210
No longer depends on: 1306210
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.