Closed
Bug 1232105
Opened 9 years ago
Closed 9 years ago
[Presentation WebAPI] provide a trusted UI for device selection on Fennec
Categories
(Firefox for Android Graveyard :: General, defect)
Firefox for Android Graveyard
General
Tracking
(firefox49 fixed, relnote-firefox -)
RESOLVED
FIXED
Firefox 49
People
(Reporter: schien, Assigned: chunmin)
References
Details
Attachments
(2 files, 1 obsolete file)
9.18 KB,
patch
|
Details | Diff | Splinter Review | |
58 bytes,
text/x-review-board-request
|
Margaret
:
review+
|
Details |
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.
Reporter | ||
Updated•9 years ago
|
Reporter | ||
Updated•9 years ago
|
Summary: [Presentation WebAPI] provide a trusted UI for device selection → [Presentation WebAPI] provide a trusted UI for device selection on Fennec
Reporter | ||
Updated•9 years ago
|
Assignee: schien → cchang
Assignee | ||
Comment 2•9 years ago
|
||
Assignee | ||
Comment 3•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/46285/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46285/
Attachment #8741200 -
Flags: review?(mark.finkle)
Assignee | ||
Updated•9 years ago
|
Attachment #8741200 -
Flags: review?(mark.finkle)
Assignee | ||
Updated•9 years ago
|
Attachment #8741200 -
Attachment is obsolete: true
Assignee | ||
Comment 4•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/46289/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46289/
Attachment #8741202 -
Flags: review?(margaret.leibovic)
Assignee | ||
Comment 5•9 years ago
|
||
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/
Updated•9 years ago
|
Attachment #8741202 -
Flags: review?(margaret.leibovic) → review+
Comment 6•9 years ago
|
||
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.
Assignee | ||
Comment 7•9 years ago
|
||
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/
Assignee | ||
Comment 8•9 years ago
|
||
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.
Assignee | ||
Comment 9•9 years ago
|
||
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/
Assignee | ||
Comment 10•9 years ago
|
||
(In reply to Chun-Min Chang[:chunmin] from comment #9)
Request r? again for changing controlling logic.
Assignee | ||
Updated•9 years ago
|
Attachment #8741202 -
Flags: review+ → review?(margaret.leibovic)
Updated•9 years ago
|
Attachment #8741202 -
Flags: review?(margaret.leibovic) → review+
Comment 11•9 years ago
|
||
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•9 years ago
|
||
(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");
Assignee | ||
Comment 13•9 years ago
|
||
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
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 14•9 years ago
|
||
Keywords: checkin-needed
Comment 15•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Comment 16•9 years ago
|
||
Passing by note: the .properties file is missing a MPL2 license header.
Assignee | ||
Comment 17•9 years ago
|
||
(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•8 years ago
|
||
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:
--- → ?
Comment 19•8 years ago
|
||
Relnote - on this request - not sure this would be easily consumable in the release notes. If anyone disagrees, please feel free to renominate.
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•