1232105 - [Presentation WebAPI] provide a trusted UI for device selection on Fennec

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

This device selection UI is required if we want to enable web page running on Fennec to trigger PresentationRequest.

Per discussion with Margaret, ContentPermissionPrompt.js is the reference we should look at for creating such UI in official release, and we can use Prompt.jsm in add-on for prototyping.

Comment 2

[C.M.Chang[:chunmin]]

Attachment: nsIPresentationDevicePrompt XPCOM

Comment 3

[C.M.Chang[:chunmin]]

Attachment: MozReview Request: Bug 1232105 - device prompt UI for presentation api; r?mfinkle

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

Comment 4

[C.M.Chang[:chunmin]]

Attachment: MozReview Request: Bug 1232105 - device prompt UI for presentation api; r=margaret

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

Comment 5

[C.M.Chang[:chunmin]]

Hi Margaret,
I was wondering if you could share your time to review this.

I've tried using 'platform/xpcom'[0] to try XPCOM implementation per your suggestion, but it seems that the sample code doesn't work. Instead of using add-on, I think implement the nsIPresentationDevicePrompt XPCOM on feenec is a straight way to do that. 

The presentation api will be enabled soon this year, and xpcom calling/registration in add-on will be deprecated at the end of this year[1]. The system add-on might still use XPCOM after the policy is implemented, but there isn't any guarantees that it will continue being available. However, it's hard to afford that the XPCOM is deprecated in system add-on once presentation api is enabled at that time.

Thus, I think it's better to implement nsIPresentationDevicePrompt on fennec itself instead of using add-on.

[0] https://developer.mozilla.org/en-US/Add-ons/SDK/Low-Level_APIs/platform_xpcom
[1] https://blog.mozilla.org/addons/2015/08/21/the-future-of-developing-firefox-add-ons/

Comment 6

[:Margaret Leibovic]

Comment on attachment 8741202 [details]
MozReview Request: Bug 1232105 - device prompt UI for presentation api; r=margaret

https://reviewboard.mozilla.org/r/46289/#review44465

Given that this is not much code, and that it's well isolated, I think it's fine to include this in the product.

::: mobile/android/components/PresentationDevicePrompt.js:20
(Diff revision 1)
> +
> +const kPRESENTATIONDEVICEPROMPT_CONTRACTID = "@mozilla.org/presentation-device/prompt;1";
> +const kPRESENTATIONDEVICEPROMPT_CID        = Components.ID("{388bd149-c919-4a43-b646-d7ec57877689}");
> +
> +function debug(aMsg) {
> +  dump("-*- PresentationDevicePrompt: " + aMsg + "\n");

You should put this behind a pref, or a local variable that you set at build time, so that these messages don't clutter the log for people who aren't looking for them.

::: mobile/android/components/PresentationDevicePrompt.js:38
(Diff revision 1)
> +
> +  _getString: function(aName) {
> +    debug("_getString");
> +
> +    if (!this.bundle) {
> +        this.bundle = Services.strings.createBundle("chrome://browser/locale/devicePrompt.properties");

Nit: Two-space indenteation for JS.

I believe we have eslint support in the tree, I'd hope that would catch this.

::: mobile/android/components/PresentationDevicePrompt.js:91
(Diff revision 1)
> +    });
> +
> +    // Spin this thread while we wait for a result
> +    let thread = Services.tm.currentThread;
> +    while (response === null)
> +      thread.processNextEvent(true);

Hm... is there really no better way to do this? I see this pattern used elsewhere in our code, so I guess not.

Comment 7

[C.M.Chang[:chunmin]]

Comment on attachment 8741202 [details]
MozReview Request: Bug 1232105 - device prompt UI for presentation api; r=margaret

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

Comment 8

[C.M.Chang[:chunmin]]

https://reviewboard.mozilla.org/r/46289/#review44465

Thanks a lot for your help :)

> You should put this behind a pref, or a local variable that you set at build time, so that these messages don't clutter the log for people who aren't looking for them.

I think I can comment it by default. If someone want to trace it, then they can just un-comment it.

> Hm... is there really no better way to do this? I see this pattern used elsewhere in our code, so I guess not.

I think I can use callback-pattern to replace it.

Comment 9

[C.M.Chang[:chunmin]]

Comment on attachment 8741202 [details]
MozReview Request: Bug 1232105 - device prompt UI for presentation api; r=margaret

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

Comment 10

[C.M.Chang[:chunmin]]

(In reply to Chun-Min Chang[:chunmin] from comment #9)
Request r? again for changing controlling logic.

Comment 11

[:Margaret Leibovic]

Comment on attachment 8741202 [details]
MozReview Request: Bug 1232105 - device prompt UI for presentation api; r=margaret

https://reviewboard.mozilla.org/r/46289/#review45163

r+ with telemetry probe added in (you'll also need to import UITelemetry.jsm).

::: mobile/android/components/PresentationDevicePrompt.js:123
(Diff revision 3)
> +    this._request = aRequest;
> +
> +    let prompt = this._getPrompt(this._getString("deviceMenu.title"),
> +                                 this._getPromptMenu(this._devices));
> +
> +    this._showPrompt(prompt, this._selectDevice.bind(this));

Let's also add a telemetry probe here to understand how often this prompt is being shown to users. You can do this with:

UITelemetry.addEvent("action.1", "dialog", null, "prompt_device_selection");

For more details about mobile telemetry, you can read this page:
https://wiki.mozilla.org/Mobile/Metrics

Comment 12

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #11)

> UITelemetry.addEvent("action.1", "dialog", null, "prompt_device_selection");

Actually, this would be better:

UITelemetry.addEvent("show.1", "dialog", null, "prompt_device_selection");

Comment 13

[C.M.Chang[:chunmin]]

Comment on attachment 8741202 [details]
MozReview Request: Bug 1232105 - device prompt UI for presentation api; r=margaret

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

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/df6ee0f30ab2

Comment 15

[Carsten Book [:Tomcat]]

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

Comment 16

[Francesco Lodolo [:flod]]

Passing by note: the .properties file is missing a MPL2 license header.

Comment 17

[C.M.Chang[:chunmin]]

(In reply to Francesco Lodolo [:flod] from comment #16)
> Passing by note: the .properties file is missing a MPL2 license header.
Thanks for your reminder! I already open a bug to fix it.

Comment 18

[Barbara Bermes [:barbara] - NI please!]

Release Note Request (optional, but appreciated)
[Why is this notable]:
[Suggested wording]: Updates to Presentation Web API: provide a trusted UI for device selection
[Links (documentation, blog post, etc)]:

Comment 19

[Marcia Knous [:marcia]]

Relnote - on this request - not sure this would be easily consumable in the release notes. If anyone disagrees, please feel free to renominate.