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

RESOLVED FIXED in Firefox 49

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: schien, Assigned: chunmin)

Tracking

(Blocks 2 bugs)

unspecified
Firefox 49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed, relnote-firefox -)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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.
Summary: [Presentation WebAPI] provide a trusted UI for device selection → [Presentation WebAPI] provide a trusted UI for device selection on Fennec
Assignee: schien → cchang
Duplicate of this bug: 1262779
Attachment #8741200 - Flags: review?(mark.finkle)
Attachment #8741200 - Attachment is obsolete: true
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/
Attachment #8741202 - Flags: review?(margaret.leibovic) → review+
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 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/
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 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/
(In reply to Chun-Min Chang[:chunmin] from comment #9)
Request r? again for changing controlling logic.
Attachment #8741202 - Flags: review+ → review?(margaret.leibovic)
Attachment #8741202 - Flags: review?(margaret.leibovic) → review+
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
(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 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/
Attachment #8741202 - Attachment description: MozReview Request: Bug 1232105 - device prompt UI for presentation api; r?margaret → MozReview Request: Bug 1232105 - device prompt UI for presentation api; r=margaret
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/df6ee0f30ab2
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Passing by note: the .properties file is missing a MPL2 license header.
Blocks: 1268767
(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.
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)]:
relnote-firefox: --- → ?
Relnote - on this request - not sure this would be easily consumable in the release notes. If anyone disagrees, please feel free to renominate.
You need to log in before you can comment on or make changes to this bug.