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

Status: RESOLVED FIXED

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

Description

[C.M.Chang[:chunmin]]

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).

Comment 1

[C.M.Chang[:chunmin]]

Attachment: Bug 1299061 - Expose the browser that the request was originated in;

Review commit: https://reviewboard.mozilla.org/r/75226/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/75226/

Comment 2

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

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.

Comment 3

[C.M.Chang[:chunmin]]

Comment on attachment 8786233 [details]
Bug 1299061 - Expose the browser that the request was originated in;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75226/diff/1-2/

Comment 4

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

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"

Comment 5

[C.M.Chang[:chunmin]]

Comment on attachment 8786233 [details]
Bug 1299061 - Expose the browser that the request was originated in;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75226/diff/2-3/

Comment 6

[Ryan VanderMeulen (PTO, back 6-April)]

Needs rebasing. Also, please clear the pending issues in MozReview so that the patch can be autolanded.

Comment 7

[C.M.Chang[:chunmin]]

Comment on attachment 8786233 [details]
Bug 1299061 - Expose the browser that the request was originated in;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75226/diff/3-4/

Comment 8

[C.M.Chang[:chunmin]]

Already rebase. Please try again. Thx!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/85a53a4c3778
Expose the browser that the request was originated in; r=smaug

Comment 10

[Phil Ringnalda (:philor)]

Sigh. And another rebase needed, backed out in https://hg.mozilla.org/integration/autoland/rev/c3ab4ea823ee80c65a4e5df09b47c2d78499db03 for conflicting with bug 1288297.

Comment 11

[C.M.Chang[:chunmin]]

Comment on attachment 8786233 [details]
Bug 1299061 - Expose the browser that the request was originated in;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75226/diff/4-5/

Comment 12

[C.M.Chang[:chunmin]]

OK. I've rebase again. Hope there is no more needs to rebase.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/298ebd8ce251
Expose the browser that the request was originated in; r=smaug

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/298ebd8ce251