1289974 - [Presentation WebAPI] provide a trusted UI for device selection on Firefox

Status: RESOLVED FIXED

(Firefox :: Site Permissions, defect, P5)

Description

[C.M.Chang[:chunmin]]

The device selection UI is required when we trigger PresentationRequest on desktop. 

We need to implement a nsIPresentationDevicePrompt XPCOM for PresentationService.cpp[0]. The device prompt UI on Fennec[1] might be a reference.

[0] http://searchfox.org/mozilla-central/source/dom/presentation/PresentationService.cpp#513
[1] bug 1232105

Comment 1

[C.M.Chang[:chunmin]]

Attachment: WIP

Comment 2

[C.M.Chang[:chunmin]]

Attachment: [WIP] Basic Notification UI

Comment 3

[C.M.Chang[:chunmin]]

Attachment: [WIP] List Notification UI

Comment 4

[C.M.Chang[:chunmin]]

Attachment: Screenshot for basic notification UI

Comment 5

[C.M.Chang[:chunmin]]

Attachment: Screenshot for list notification UI

Hi Aaron, Tori,
I have no spec now, so I just made two possible versions of the add-ons. One provides a single drop menu to select devices. The other provides two menus for device selection and other settings. 

Could you tell me which one is more probable candidate for the future spec?

Comment 6

[Aaron Wu]

Hi Chun-Min,

Thanks for your markups, Shih-Chiang and I had quickly discussed, we think the basic notification is based on Fennec UI which good for alpha prototype as 1st phase. Regarding the list notification (WebRTC UI-like), we can base on that to discuss advance features as 2nd phase. 

We can invite Tori to join discuss accordingly, maybe we need one visual designer as well.

Thanks,
Aaron

Comment 7

[C.M.Chang[:chunmin]]

Attachment: [WIP] Panel UI composed by XUL

Comment 8

[C.M.Chang[:chunmin]]

Attachment: Screenshot for XUL panel UI

Comment 9

[C.M.Chang[:chunmin]]

Attachment: [WIP] Panel UI composed by HTML

Use iframe in XUL Panel to load a html page.
However, the size is unpredictable unless we set a strict size for it.

Comment 10

[C.M.Chang[:chunmin]]

Attachment: Screenshot for HTML panel UI

Comment 11

[C.M.Chang[:chunmin]]

(In reply to Chun-Min Chang[:chunmin] from comment #9)
> Created attachment 8779704 [details] [diff] [review]
> [WIP] Panel UI composed by HTML
> 
> Use iframe in XUL Panel to load a html page.
> However, the size is unpredictable unless we set a strict size for it.
If we choose to use this version, I ask for front-end resources for reviewing the patch before I am not familiar with css stuff.

Comment 12

[C.M.Chang[:chunmin]]

I will stop exploring the possible UIs until I have a clear spec and clear requirements from platform team.

From last discussion, the simplest basic notification UI is enough for minimum viable product. It's better to use it to demo to the platform team and get some feedback first before getting to next phase. If there is no request for the fancy UI, we should stop here.

Comment 13

[C.M.Chang[:chunmin]]

Attachment: UI Concept

Hi mconley,
Sorry to bother you.
Since I saw you're the reviewer of FlyWeb's add-on, I was wondering if you could give me suggestions for the UI of this bug. 

Is it possible to create the UI like the attachment? Should I use panel or menupopup? Is there any way to create the footer buttons?

I heard that XUL might be replaced with HTML. If it's true, is it appropriate to add new XUL UI here?

Comment 14

[Mike Conley (:mconley) (:⚙️)]

(In reply to Chun-Min Chang[:chunmin] from comment #13)
> Is it possible to create the UI like the attachment? Should I use panel or
> menupopup? Is there any way to create the footer buttons?
> 
> I heard that XUL might be replaced with HTML. If it's true, is it
> appropriate to add new XUL UI here?

Hello chunmin!

Yes, what you're suggesting in your mock-up should be possible. These "doorhanger notifications", as the front-end team calls them, are quite common (as you probably already know), and have the option for footer buttons. See, for example, the restyling that's currently underway in bug 1267604 - specifically, the Invision mock-up: https://mozilla.invisionapp.com/share/PC3Y0QSRK#/screens/106406027

This work landed on the Elm branch only a few hours ago. You might want to base your work on top of that so that you don't get conflicts with any theme work that you attempt, if any.

These permissions doorhangers are created using a module called PopupNotifications.jsm. There is some documentation for it here: https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/PopupNotifications.jsm - but it might be slightly out of date.

We can use the GeoLocation API as an example - here's where we prompt for the GeoLocation permission:

http://searchfox.org/mozilla-central/rev/d83f1528dbcb1e837d99cf5b6c36a90b03bf0e77/browser/components/nsBrowserGlue.js#2646

This being the operative bit: http://searchfox.org/mozilla-central/rev/d83f1528dbcb1e837d99cf5b6c36a90b03bf0e77/browser/components/nsBrowserGlue.js#2642

Note that it's not necessary to put the code in nsBrowserGlue (which is a singleton in the parent process). For example, here's usage of PopupNotifications that's occurring in a browser.js script for click-to-play: http://searchfox.org/mozilla-central/rev/d83f1528dbcb1e837d99cf5b6c36a90b03bf0e77/browser/base/content/browser-plugins.js#287

Note that in order to get the icon showing up in the URL bar, you'll need to put another item in this list: http://searchfox.org
/mozilla-central/rev/d83f1528dbcb1e837d99cf5b6c36a90b03bf0e77/browser/base/content/browser.xul#731

and maybe here if it can be blocked: http://searchfox.org/mozilla-central/rev/d83f1528dbcb1e837d99cf5b6c36a90b03bf0e77/browser/base/content/browser.xul#710

And you'll need to set up the icon here: http://searchfox.org/mozilla-central/rev/d83f1528dbcb1e837d99cf5b6c36a90b03bf0e77/browser/themes/shared/notification-icons.inc.css#52 and add something like this: http://searchfox.org/mozilla-central/rev/d83f1528dbcb1e837d99cf5b6c36a90b03bf0e77/browser/themes/shared/notification-icons.inc.css#136

That's how I'd suggest you approach it for now. While it's true that we're attempting to move away from XUL in the long run, there's no HTML based permissions dialog system just yet - and if we did, we'd likely make it implement the PopupNotifications API anyways, so you might as well just use that.

(Also cc'ing :past, who is managing the team that's currently hacking on the Elm branch).

Comment 15

[Panos Astithas (he/him) [:past] (please ni?)]

Also please don't use the blue "i" icon as the notification anchor as it is too confusing next to the existing "i" icon. I think the FlyWeb icon would work fine as an anchor icon in this case (icons in the notification prompts tend to match the anchor icon in the identity panel).

Comment 16

[C.M.Chang[:chunmin]]

(In reply to Mike Conley (:mconley) - (Needinfo me!) from comment #14)
Thanks for your information so much!
Since I am not familiar enough to understand all the part, I still have some questions.

> Yes, what you're suggesting in your mock-up should be possible. These
> "doorhanger notifications", as the front-end team calls them, are quite
> common (as you probably already know), and have the option for footer
> buttons. See, for example, the restyling that's currently underway in bug
> 1267604 - specifically, the Invision mock-up:
> https://mozilla.invisionapp.com/share/PC3Y0QSRK#/screens/106406027

> This work landed on the Elm branch only a few hours ago. You might want to
> base your work on top of that so that you don't get conflicts with any theme
> work that you attempt, if any.
> 
> These permissions doorhangers are created using a module called
> PopupNotifications.jsm. There is some documentation for it here:
> https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/
> PopupNotifications.jsm - but it might be slightly out of date.
So, Will the actions in PopupNotifications be transformed into the footer button itself?

And how do I add a menupopup into PopupNotifications? or should I use listbox instead?
> We can use the GeoLocation API as an example - here's where we prompt for
> the GeoLocation permission:
> 
> http://searchfox.org/mozilla-central/rev/
> d83f1528dbcb1e837d99cf5b6c36a90b03bf0e77/browser/components/nsBrowserGlue.
> js#2646
> 
> This being the operative bit:
> http://searchfox.org/mozilla-central/rev/
> d83f1528dbcb1e837d99cf5b6c36a90b03bf0e77/browser/components/nsBrowserGlue.
> js#2642
> 
> Note that it's not necessary to put the code in nsBrowserGlue (which is a
> singleton in the parent process). For example, here's usage of
> PopupNotifications that's occurring in a browser.js script for
> click-to-play:
> http://searchfox.org/mozilla-central/rev/
> d83f1528dbcb1e837d99cf5b6c36a90b03bf0e77/browser/base/content/browser-
> plugins.js#287
From add-on's perspective, if I want to add a menupopup into PopupNotifications's content, is it better to use chromeDocument.getElementById('mainPopupSet') and then inject a new popupnotification element into it? or I can directly create one popupnotification in popup-notifications.inc[0] and call it in the add-on?

> That's how I'd suggest you approach it for now. While it's true that we're
> attempting to move away from XUL in the long run, there's no HTML based
> permissions dialog system just yet - and if we did, we'd likely make it
> implement the PopupNotifications API anyways, so you might as well just use
> that.
Good to hear that!

[0] http://searchfox.org/mozilla-central/source/browser/base/content/popup-notifications.inc

Comment 17

[C.M.Chang[:chunmin]]

Attachment: [WIP] Panel UI composed by HTML v2

Imitate the UI from UX's concept picture.

Comment 18

[C.M.Chang[:chunmin]]

Attachment: Screenshot for HTML panel UI v2

Comment 19

[C.M.Chang[:chunmin]]

Attachment: [WIP] Panel UI composed by HTML v3

Hi Fischer,
I was wondering if you could give me some suggestions about the UI layout of this patch, especially menu.html/menu.css. I'd like to draw something like attachment 8779963 [details], however, my knowledge of css layout is limited. I would greatly appreciate it if you could kindly give me some advices.

Comment 20

[Fischer [:Fischer]]

Comment on attachment 8780839 [details] [diff] [review]
[WIP] Panel UI composed by HTML v3

Review of attachment 8780839 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/extensions/presentation/content/ui/menu.css
@@ +1,5 @@
> +body {
> +  margin-top: 0px;
> +  margin-bottom: 0px;
> +  margin-left: 0px;
> +  margin-right: 0px;

"margin: 0" is enough

@@ +3,5 @@
> +  margin-bottom: 0px;
> +  margin-left: 0px;
> +  margin-right: 0px;
> +  padding: 0;
> +  font-family: "Arial", sans-serif;

font-family: Arial, sans-serif;

@@ +6,5 @@
> +  padding: 0;
> +  font-family: "Arial", sans-serif;
> +}
> +
> +hr {

This may not be needed

@@ +27,5 @@
> +  padding-top: 16px;
> +  padding-left: 16px;
> +  padding-right: 16px;
> +  max-height: 100px;
> +}

Please see the attached menu-header-with-breakline-example.png

@@ +35,5 @@
> +  align-items: center;
> +  max-height: 32px;
> +}
> +
> +#menu-header .header-title img {

Please use #menu-header > .header-title > img. But there is a CSS class selector specified on the <img> (class="element"). Maybe we could just use .element for this CSS rule. And it is better to have a related CSS class name, for example, header-img.

@@ +41,5 @@
> +  width: auto;
> +  margin-right: 10px;
> +}
> +
> +#menu-header .header-title h1 {

Please use #menu-header > .header-title > h1

@@ +44,5 @@
> +
> +#menu-header .header-title h1 {
> +  font-size: 20px;
> +  font-weight: normal;
> +  color: #555555;

We could use color:#555; but I am fine with #555555

@@ +54,5 @@
> +  max-height: 56px;
> +}
> +
> +/* Menu Content */
> +.menu-content {

Since we have the menu-container id, why not use #menu-container and reduce the use of the menu-content CSS class selector.

@@ +67,5 @@
> +  list-style-type: none;
> +  line-height: 10px;
> +}
> +
> +.menu-content ul li a {

Only need
.menu-content a {

}
This would apply this rule to all <a> under the div.menu-content

@@ +73,5 @@
> +  text-decoration: none;
> +  display: block;
> +}
> +
> +.menu-content ul li a:hover {

Use .menu-content a:hover

@@ +77,5 @@
> +.menu-content ul li a:hover {
> +  background-color: #0099ff;
> +}
> +
> +.menu-content ul li a img {

Use .menu-content img

::: browser/extensions/presentation/content/ui/menu.html
@@ +14,5 @@
> +    <h1>Devices</h1>
> +  </div>
> +  <p>Pick one device:</p>
> +</div>
> +<hr>

I wouldn't say <hr> is needed here since <hr>'s use case is for https://developer.mozilla.org/en/docs/Web/HTML/Element/hr. It seems here we just want to have a breakline for UI style so maybe we could use div#menu-header's border to make one.

Comment 21

[Fischer [:Fischer]]

Attachment: menu-header-with-breakline-example.png

Comment 22

[Fischer [:Fischer]]

(In reply to Chun-Min Chang[:chunmin] from comment #19)
> Created attachment 8780839 [details] [diff] [review]
> [WIP] Panel UI composed by HTML v3
> 
> Hi Fischer,
> I was wondering if you could give me some suggestions about the UI layout of
> this patch, especially menu.html/menu.css. I'd like to draw something like
> attachment 8779963 [details], however, my knowledge of css layout is
> limited. I would greatly appreciate it if you could kindly give me some
> advices.

@chunmin,
I only looked into menu.html and menu.css. Please let me know if I missed some other files.
Thank you.

Comment 23

[C.M.Chang[:chunmin]]

(In reply to Fischer [:Fischer] from comment #22)
> @chunmin,
> I only looked into menu.html and menu.css. Please let me know if I missed
> some other files.
> Thank you.
Thanks a lot for your help :) It's really informative!
I might ask mconley's help to review the other part after this UI is agreed with UX.

Comment 24

[C.M.Chang[:chunmin]]

Attachment: Bug 1289974 part 1: Device selection for presentation API on Desktop;

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

Comment 25

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8781402 [details]
Bug 1289974 part 1: Device selection for presentation API on Desktop;

https://reviewboard.mozilla.org/r/71856/#review69508

Hey chunmin,

Thanks for the patch! Lots of good stuff in here, and I think the general approach of using an iframe in a panel is a neat idea.

Mainly, I'm concerned with the following:

1) I worry that this is all a bit over-engineered. The DeviceMenu abstraction, for example, I don't understand why this is in its own JSM, and needs to be distinct from PresentationDevicePrompt.
2) Too much usage of the process-wide nsIObserverService, when something like message passing would probably be more appropriate
3) At least one part of this (a contentWindow access) will not work with e10s (unless you use CPOWs, which you shouldn't)
4) There are some minor style inconsistencies (quoting) and some hardcoded strings
5) There are some hardcoded dimensions that I worry will not be flexible enough for various zoom levels.
7) I worry that there's no great way of knowing which <xul:browser> a presentation API request came from. Assuming that it's the most recent window's selected browser is not a safe assumption. I think that's more of an API-level concern.

For the last, generally what we do for e10s is the following:
1) Have the API fire a DOM Event when a request is made
2) Have a content script (or a process script) capture that DOM event, and send a message using sendAsyncMessage to the parent process (this will work transparently even in non-remote browsers)
3) Have the parent process receive the message. The `message.target` will be the <xul:browser> that sent the message. Have the parent then use that information to figure out whether or not to display the prompt

I hope this helps! Let me know if you have any questions.

::: browser/extensions/presentation/bootstrap.js:26
(Diff revision 1)
> +function uninstall(aData, aReason) {
> +}
> +
> +function startup(aData, aReason) {
> +  log("startup");
> +  // Initialize Presentation API

The code seems pretty self-explanatory - not sure the comment adds much. Same on line 32.

::: browser/extensions/presentation/bootstrap.js:27
(Diff revision 1)
> +}
> +
> +function startup(aData, aReason) {
> +  log("startup");
> +  // Initialize Presentation API
> +  Presentation && Presentation.init();

Is there ever a chance that Presentation is not defined? If not, we probably don't need the &&.

::: browser/extensions/presentation/content/DeviceMenu.jsm:8
(Diff revision 1)
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +'use strict';
> +
> +var EXPORTED_SYMBOLS = ['DeviceMenu'];

You're using double-quotes throughout most of this file. Might as well be consistent. Same on lines 6, 12 and 13.

::: browser/extensions/presentation/content/DeviceMenu.jsm:38
(Diff revision 1)
> +const UI_WIDTH = 320;
> +const UI_HEIGHT = 350;

What about at different zoom levels?

::: browser/extensions/presentation/content/DeviceMenu.jsm:133
(Diff revision 1)
> +          aReject("Dismiss by user");
> +        }
> +      });
> +
> +      let { selectedBrowser } = self._getBrowserData();
> +      let anchor = selectedBrowser.contentWindow.top;

With e10s, you have no access to the contentWindow anymore (the contentWindow exists in the content process).

What is the UX spec here with respect to anchoring?

::: browser/extensions/presentation/content/DeviceMenu.jsm:168
(Diff revision 1)
> +    if (!aData) {
> +      log("No available data to be loaded");
> +      return;
> +    }
> +    let data = JSON.stringify(aData);
> +    Services.obs.notifyObservers(null, LOAD_PRESENTATION_DATA, data);

I assume you're doing this to communicate with the content in the iframe. You should probably use postMessage instead sending the process-wide observer notification.

::: browser/extensions/presentation/content/DeviceMenu.jsm:171
(Diff revision 1)
> +  _getBrowserData: function() {
> +    let browserWindow = Services.wm.getMostRecentWindow("navigator:browser");
> +    let selectedBrowser = browserWindow.gBrowser.selectedBrowser;
> +    let currentURL = selectedBrowser.currentURI.spec;
> +    let chromeWin = selectedBrowser.ownerGlobal;
> +    let chromeDoc = selectedBrowser.ownerDocument;

This is assuming that the selected browser is the one that sent us the Presentation API request. What do we do if a background tab requests this?

What'd probably be best is if we could somehow know which <xul:browser> the request came from - perhaps making that part of the nsIPresentationDevicePrompt interface.

::: browser/extensions/presentation/content/Presentation.jsm:32
(Diff revision 1)
> +}
> +
> +// Register/unregister a constructor as a factory.
> +function Factory() {}
> +Factory.prototype = {
> +  register: function f_register(targetConstructor) {

You don't need to name the functions anymore - in fact, this could just be:

```JavaScript
register(targetConstructor) {
  // ...
},

```

::: browser/extensions/presentation/content/Presentation.jsm:65
(Diff revision 1)
> +  _register: function p_register() {
> +    log('_register');
> +    this._devicePromptFactory = new Factory();
> +    Cu.import(PRESENTATIONDEVICEPROMPT_PATH);
> +    // Register PresentationDevicePrompt into a XPCOM component.
> +    this._devicePromptFactory.register(PresentationDevicePrompt);
> +  },
> +
> +  _unregister: function p_unregister() {
> +    log('_unregister');
> +    this._devicePromptFactory.unregister();
> +    Cu.unload(PRESENTATIONDEVICEPROMPT_PATH);
> +    // Unregister PresentationDevicePrompt XPCOM component.
> +    delete this._devicePromptFactory;

Is the indirection necessary, or can we put this stuff inside init / uninit?

::: browser/extensions/presentation/content/PresentationDevicePrompt.jsm:8
(Diff revision 1)
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/*
> + * This is the implementation of nsIPresentationDevicePrompt XPCOM.
> + * It will be registered into a XPCOM component by Presntation.jsm.

Typo: "Presntation.jsm"

::: browser/extensions/presentation/content/PresentationDevicePrompt.jsm:30
(Diff revision 1)
> +XPCOMUtils.defineLazyGetter(this, "DeviceMenu", function() {
> +  let sandbox = {};
> +  Services.scriptloader.loadSubScript("chrome://presentation.api/content/DeviceMenu.jsm", sandbox);
> +  return sandbox["DeviceMenu"];
> +});

What is the advantage of loading a JSM like this over just using Cu.import?

::: browser/extensions/presentation/content/PresentationDevicePrompt.jsm:100
(Diff revision 1)
> +
> +  // Get the deviceMenu instance. Create a new one if it doesn't exist.
> +  // Otherwise, return the existing one.
> +  get deviceMenu() {
> +    if (!this._deviceMenu) {
> +      this._deviceMenu = new DeviceMenu();

It looks like we expect there to be a 1-1 relationship between PresentationDevicePrompt and DeviceMenu.

Why then does DeviceMenu need its own JSM? Why do we need to be able to instantiate it? Can we not just have a singleton object for DeviceMenu here in PresentationDevicePrompt?

::: browser/extensions/presentation/content/ui/menu.html:8
(Diff revision 1)
> +<head>
> +  <meta charset="utf-8">
> +  <meta name="viewport" content="width=device-width, initial-scale=1.0, user-scalable=no">
> +  <title>Device Menu</title>
> +  <link rel="stylesheet" type="text/css" href="menu.css">
> +  <script src=menu.js></script>

No quotes around menu.js

::: browser/extensions/presentation/content/ui/menu.html:15
(Diff revision 1)
> +      <h1>Devices</h1>
> +    </div>
> +    <p>Pick one device:</p>

Hardcoded strings

::: browser/extensions/presentation/content/ui/menu.js:22
(Diff revision 1)
> +  // PUBLIC APIs
> +  init: function() {
> +    // Register observers
> +    Services.obs.addObserver(this, LOAD_PRESENTATION_DATA, false);
> +    // Require devices' information from DeviceMenu.jsm
> +    Services.obs.notifyObservers(null, REQUIRE_PRESENTATION_DATA, null);

Another good time to use postMessage instead of the nsIObserverService. Also, that way, you can take advantage of structured clone, and you don't have to do JSON parsing yourself.

::: browser/extensions/presentation/install.rdf.in:15
(Diff revision 1)
> +
> +  <Description about="urn:mozilla:install-manifest">
> +    <em:id>[e-mail]</em:id>
> +    <em:version>1.0.0</em:version>
> +    <em:type>2</em:type>
> +    <em:bootstrap>true</em:bootstrap>

Please mark this multiprocessCompatible, via https://developer.mozilla.org/en-US/Add-ons/Install_Manifests#multiprocessCompatible

Comment 26

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8781402 [details]
Bug 1289974 part 1: Device selection for presentation API on Desktop;

https://reviewboard.mozilla.org/r/71856/#review69520

Whoops, forgot to set flag.

Comment 27

[C.M.Chang[:chunmin]]

Comment on attachment 8781402 [details]
Bug 1289974 part 1: Device selection for presentation API on Desktop;

https://reviewboard.mozilla.org/r/71856/#review69508

Thanks a lot for the review. There are many valuable suggestions ;)

The reason I divide ```DeviceMenu``` into another JSM is that it won't be loaded until some website starts a PresentationRequest. The Presentation API now is not a W3C official standard, so it may not be used in most of the time. I am not sure if it's ok to load the javascript code with low frequency by default. If it's ok, then I can move it into ```PresentationDevicePrompt```.

After brief discussion with UX, we don't need to prompt the device selection UI in the window that fires the request. We could prompt this selection UI to the window we're using. The description in the prompt will show where the request comes from.

Thanks for the reminder for the e10s compatibility. I'll change it in next version.

> The code seems pretty self-explanatory - not sure the comment adds much. Same on line 32.

you're right. I'll remove it in next version.

> Is there ever a chance that Presentation is not defined? If not, we probably don't need the &&.

No. I'll remove it in next version.

> You're using double-quotes throughout most of this file. Might as well be consistent. Same on lines 6, 12 and 13.

I'll use double-quotes in all of the files.

> What about at different zoom levels?

Does the prompt UI need to zoom in with the page? .... I think I might not get your point here. Could you give me more hints?

> With e10s, you have no access to the contentWindow anymore (the contentWindow exists in the content process).
> 
> What is the UX spec here with respect to anchoring?

The UX expect we could prompt a device selection UI at the center here. ```anchor``` here is only to get the window's ```screenX, screenY``` and ```outerWidth, outerHeight```. Maybe we can get them in other ways.

> I assume you're doing this to communicate with the content in the iframe. You should probably use postMessage instead sending the process-wide observer notification.

Great idea! I'll change it in next version.

> You don't need to name the functions anymore - in fact, this could just be:
> 
> ```JavaScript
> register(targetConstructor) {
>   // ...
> },
> 
> ```

You're right. It will be removed in the next version.

> Is the indirection necessary, or can we put this stuff inside init / uninit?

No. I just want to label the functions ```_register``` and ```_unregister``` are exclusive for ```PresentationDevicePrompt``` now. I can move them into ```init``` and ```uninit```.

> Typo: "Presntation.jsm"

Thanks for the reminder!

> What is the advantage of loading a JSM like this over just using Cu.import?

The module defined in LazyGetter will not be imported until first use. Thus, if ```promptDeviceSelection``` is never called, then ```DeviceMenu``` won't be imported. The presentation api is not a W3C standard currently, so only few websites will use it. I think it would be better to import it when there is a specific request.

> It looks like we expect there to be a 1-1 relationship between PresentationDevicePrompt and DeviceMenu.
> 
> Why then does DeviceMenu need its own JSM? Why do we need to be able to instantiate it? Can we not just have a singleton object for DeviceMenu here in PresentationDevicePrompt?

Is it ok to load some javascript module which is used with low frequency? If it's ok, I can move ```DeviceMenu``` into ```PresentationDevicePrompt```.

> No quotes around menu.js

Thanks for the reminder!

> Hardcoded strings

This will be replaced when the devices information is loaded into this page. Would it be better if we have no initial string here?

> Please mark this multiprocessCompatible, via https://developer.mozilla.org/en-US/Add-ons/Install_Manifests#multiprocessCompatible

OK, I'll add it.

Comment 28

[C.M.Chang[:chunmin]]

Comment on attachment 8781402 [details]
Bug 1289974 part 1: Device selection for presentation API on Desktop;

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

Comment 29

[C.M.Chang[:chunmin]]

Comment on attachment 8781402 [details]
Bug 1289974 part 1: Device selection for presentation API on Desktop;

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

Comment 30

[C.M.Chang[:chunmin]]

Comment on attachment 8781402 [details]
Bug 1289974 part 1: Device selection for presentation API on Desktop;

https://reviewboard.mozilla.org/r/71856/#review69508

Hi mconley,

Sorry for ```r?``` again. I thought I could upload a WIP patch without ```r?```. It turned out that the mozreview is too smart so it requests the review automatically. I will use ```needinfo?``` on bugzilla next time.

Any way, I have another idea about the message passing bewteen the UI page(menu.html) and the DeviceMenu.jsm(it might be merged into PresentationDevicePrompt.jsm). It seems we could use ```iframe.contentDocument``` to manipulate the DOM content inside. It means that we could update the UI directly in the .jsm file without any postMessage or sendAsyncMessage. What do you think?

Comment 31

[Mike Conley (:mconley) (:⚙️)]

(In reply to Chun-Min Chang[:chunmin] from comment #27)
> Comment on attachment 8781402 [details]
> Bug 1289974 - Device selection UI for presentation API;
> 
> https://reviewboard.mozilla.org/r/71856/#review69508
> 
> Thanks a lot for the review. There are many valuable suggestions ;)
> 
> The reason I divide ```DeviceMenu``` into another JSM is that it won't be
> loaded until some website starts a PresentationRequest. The Presentation API
> now is not a W3C official standard, so it may not be used in most of the
> time. I am not sure if it's ok to load the javascript code with low
> frequency by default. If it's ok, then I can move it into
> ```PresentationDevicePrompt```.
> 

I think it's probably safe to put them in the same place. The more we can simplify this, the better. Also, the memory overhead of the different JSM compartments is non-0. I suspect we can load the JavaScript from DeviceMenu without much in the way of a performance penalty.

> After brief discussion with UX, we don't need to prompt the device selection
> UI in the window that fires the request. We could prompt this selection UI
> to the window we're using. The description in the prompt will show where the
> request comes from.
> 

I don't understand how that'll be clear to the user, to be honest. Especially if they're at the same website in several tabs. This is also very inconsistent with how DOM APIs normally ask for permission from the user. See WebRTC, for example. If a web page in a background tab requests the camera or microphone, we don't alert the most recently used window - we create a permission notification for the tab that requested it.

Who from UX did you consult on this?

Comment 32

[C.M.Chang[:chunmin]]

(In reply to Mike Conley (:mconley) - (Needinfo me!) from comment #31)
> I think it's probably safe to put them in the same place. The more we can
> simplify this, the better. Also, the memory overhead of the different JSM
> compartments is non-0. I suspect we can load the JavaScript from DeviceMenu
> without much in the way of a performance penalty.
OK, I'll merge the two js files into one.

> I don't understand how that'll be clear to the user, to be honest.
> Especially if they're at the same website in several tabs. This is also very
> inconsistent with how DOM APIs normally ask for permission from the user.
> See WebRTC, for example. If a web page in a background tab requests the
> camera or microphone, we don't alert the most recently used window - we
> create a permission notification for the tab that requested it.
> 
> Who from UX did you consult on this?
The consultant is Tori. However, I think we don't have official resource for UI/UX now. This design is used to promote the Presentation API to platform team. This API is now in W3C Editor's Draft phase. Through it, we can display web content to several multimedia devices like Chromecast and AirPlay[0].

Yes, you're right. We should have consistent prompt behavior like WebRTC. To know where the request came from, we might need to pass some window information like nsIContentPermissionPrompt[1] in DOM API level.

[0] https://developer.mozilla.org/en-US/docs/Web/API/Presentation_API#Presentation_concepts_and_usage
[1] http://searchfox.org/mozilla-central/rev/ae78ab94fadabc89fc6258d03c4a1a70f763f43a/dom/interfaces/base/nsIContentPermissionPrompt.idl#87

Comment 33

[Mike Conley (:mconley) (:⚙️)]

Note that I just filed bug 1297475, since I'm starting to see some convergence here with what is going on in bug 1292639.

I'm about to start a discussion on firefox-dev which I'll post a link to regarding making it possible to contain permission handlers within system add-ons to make this kind of interaction simpler to engineer.

Comment 34

[C.M.Chang[:chunmin]]

Attachment: part 1 - Expose requesting browser

Hi SC,
Could you give me some suggestions about exposing the requesting browser?

Comment 35

[C.M.Chang[:chunmin]]

Attachment: part 2 - Device selection UI

Hi mconley,
I have few questions about using panel with the requesting browser.

Is it possible to attach panel only to some specific browser?

In my WIP patch, I've exposed the requesting browser, but I don't know how to show the panel only in the requesting page. I know WebRTC could show only on the requesting page by using anchor element. Is it possible to do it without the anchor? If I want to popup the panel at the center of the requesting page, how to do it?

On the other hand, what should be careful in various zoom levels?

Comment 36

[C.M.Chang[:chunmin]]

(In reply to Chun-Min Chang[:chunmin] from comment #35)
> Created attachment 8784245 [details] [diff] [review]
> part 2 - Device selection UI
> 
> Hi mconley,
> I have few questions about using panel with the requesting browser.
> 
> Is it possible to attach panel only to some specific browser?
> 
> In my WIP patch, I've exposed the requesting browser, but I don't know how
> to show the panel only in the requesting page. I know WebRTC could show only
> on the requesting page by using anchor element. Is it possible to do it
> without the anchor? If I want to popup the panel at the center of the
> requesting page, how to do it?
I guess we don't need to attach the UI only to the requesting browser. We just need to control when to show the UI like[0]. If the page is inactive, we have two options:

1) Cancel the request(This is current behavior)
2) Pend the request until the page is active again.

The second one can be achieved by something like:
if (!this._isActive) {
  this._browser.onfocus = function() {
    log("Page is active again!");
    // Show the UI now...
   }
}

[0] http://searchfox.org/mozilla-central/rev/b38dbd1378cea4ae83bbc8a834cdccd02bbc5347/toolkit/modules/PopupNotifications.jsm#407

Comment 37

[C.M.Chang[:chunmin]]

Hi mconley,
I was wondering if you could give me some hints:
1. The browser that starts the request is used to check the tab is active or not[0] and decide whether the prompt is displayed like [0]. Is it right?
2. What should panel be adjusted with the zoom level? size?

[0] http://searchfox.org/mozilla-central/rev/b38dbd1378cea4ae83bbc8a834cdccd02bbc5347/toolkit/modules/PopupNotifications.jsm#407

Comment 38

[C.M.Chang[:chunmin]]

Attachment: part 2 - Device selection UI

If the page is inactive, then the selection UI will be prompted until the page is active again.

Comment 39

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

Comment on attachment 8784242 [details] [diff] [review]
part 1 - Expose requesting browser

Review of attachment 8784242 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/presentation/PresentationRequest.h
@@ +65,5 @@
>    bool IsPrioriAuthenticatedURL(const nsAString& aUrl);
>  
>    nsString mUrl;
>    RefPtr<PresentationAvailability> mAvailability;
> +  nsCOMPtr<nsPIDOMWindowInner> mWindow;

|GetOwner()| is enough.

::: dom/presentation/interfaces/nsIPresentationDevicePrompt.idl
@@ +27,5 @@
> +  // The window or element that the permission request was originated in.
> +  // Typically the element will be non-null when it's in content process.
> +  // window and element will never be null at the same time.
> +  readonly attribute mozIDOMWindow window;
> +  readonly attribute nsIDOMElement element;

|nsIDOMEventTarget| is the type you need for handling both in-process and e10s cases.

You can move the logic of |_getBrowserForRequest| to caller, which is PresentationService. Furthermore, you can make |nsIPresentationService.startService| take |nsIDOMEventTarget| as parameter, to replace the window/element pair.

BTW, you can obtain chrome event handler via nsPIDOMWindowInner::GetChromeEventHandler() in C++ directly. (see https://dxr.mozilla.org/mozilla-central/source/dom/base/nsPIDOMWindow.h#146)

Comment 40

[Mike Conley (:mconley) (:⚙️)]

Hey chunmin,

I realize I'm the bottleneck here for finding the right way of exposing the permission dialog to the user. I hope to have a solution to propose to you today.

Comment 41

[Mike Conley (:mconley) (:⚙️)]

Hey chunmin,

So I've had some conversations with the Firefox front-end team, and I've also been working with djvj for a similar issue in bug 1292639, and I think I'm ready to make some recommendations.

First of all, I'd like to better understand the product path here. Is this Presentation API UI work hoping to ride the trains up to release? Or is this only supposed to be enabled for Nightly in order to experiment?

I suspect it's the latter, but please let me know. Assuming I'm right about that, then your System Add-on should probably not be included in the build list in the browser/extensions/moz.build unless it's a NIGHTLY_BUILD - see the FlyWeb add-on for an example.

Secondly, I'm interested in hearing about your choice to supply this UI as a System Add-on. FlyWeb originally did this because of how experimental it was, and likely to change. Is the Presentation API implementation in Firefox similarly experimental and likely to change?

I recommend that this bug be blocked by bug 1297475, unless there's an urgent need to land this UI.

Comment 42

[C.M.Chang[:chunmin]]

(In reply to Mike Conley (:mconley) - (Needinfo me!) from comment #41)
Hi mconley,
Thanks for your helps and advices again.

> First of all, I'd like to better understand the product path here. Is this
> Presentation API UI work hoping to ride the trains up to release? Or is this
> only supposed to be enabled for Nightly in order to experiment?
AFAIK, the core DOM Presentation API is already landed on central[0] and can be enabled by preference on Fennec (bug 1278205). My understanding is that this is an experiment for proposing the feature to Firefox. 

Hi Aaron,
Could you provide more information here? 
> I suspect it's the latter, but please let me know. Assuming I'm right about
> that, then your System Add-on should probably not be included in the build
> list in the browser/extensions/moz.build unless it's a NIGHTLY_BUILD - see
> the FlyWeb add-on for an example.
So, should I link some .xpi to firefox like bug 1280378?

> Secondly, I'm interested in hearing about your choice to supply this UI as a
> System Add-on. FlyWeb originally did this because of how experimental it
> was, and likely to change. Is the Presentation API implementation in Firefox
> similarly experimental and likely to change?
It's definitely better to use some common modules to create something like https://mozilla.invisionapp.com/share/PC3Y0QSRK#/screens/106406027. However, the new style UI won't be updated until Bug 1267604 is land. The ETA now is 9/2, so it's easier to achieve what attachment 8779963 [details] expects by creating our own prompt UI instead of using PopupNotifications.jsm.
If the new prompt module meets the following two requirements
- Can prompt UI in the center of the browser
- Can be customized to attachment 8779963 [details]
, then it is absolutely worth to use it.

To get things done, an alternative way is to use the current prompt module to propose this feature first. Aaron, what did you think? What is the expectation from Firefox team?

> I recommend that this bug be blocked by bug 1297475, unless there's an
> urgent need to land this UI.
Aaron, should we need to change the ETA?

Comment 43

[Aaron Wu]

(In reply to Chun-Min Chang[:chunmin] from comment #42)
> (In reply to Mike Conley (:mconley) - (Needinfo me!) from comment #41)
> Hi mconley,
> Thanks for your helps and advices again.
> 
> > First of all, I'd like to better understand the product path here. Is this
> > Presentation API UI work hoping to ride the trains up to release? Or is this
> > only supposed to be enabled for Nightly in order to experiment?
> AFAIK, the core DOM Presentation API is already landed on central[0] and can
> be enabled by preference on Fennec (bug 1278205). My understanding is that
> this is an experiment for proposing the feature to Firefox. 
> 
> Hi Aaron,
> Could you provide more information here? 

Yes, it's the latter one. The main purpose is to do an experiment and have a prototype for Firefox team. That would be good to involve Firefox product team to have more comprehensive features running on Firefox product.

> > I suspect it's the latter, but please let me know. Assuming I'm right about
> > that, then your System Add-on should probably not be included in the build
> > list in the browser/extensions/moz.build unless it's a NIGHTLY_BUILD - see
> > the FlyWeb add-on for an example.
> So, should I link some .xpi to firefox like bug 1280378?
> 
> > Secondly, I'm interested in hearing about your choice to supply this UI as a
> > System Add-on. FlyWeb originally did this because of how experimental it
> > was, and likely to change. Is the Presentation API implementation in Firefox
> > similarly experimental and likely to change?
> It's definitely better to use some common modules to create something like
> https://mozilla.invisionapp.com/share/PC3Y0QSRK#/screens/106406027. However,
> the new style UI won't be updated until Bug 1267604 is land. The ETA now is
> 9/2, so it's easier to achieve what attachment 8779963 [details] expects by
> creating our own prompt UI instead of using PopupNotifications.jsm.
> If the new prompt module meets the following two requirements
> - Can prompt UI in the center of the browser
> - Can be customized to attachment 8779963 [details]
> , then it is absolutely worth to use it.
> 
> To get things done, an alternative way is to use the current prompt module
> to propose this feature first. Aaron, what did you think? What is the
> expectation from Firefox team?
> 
> > I recommend that this bug be blocked by bug 1297475, unless there's an
> > urgent need to land this UI.
> Aaron, should we need to change the ETA?

That is a good proposal, we can discuss in our group meeting on Thu. CST this week and make a final call.

Comment 44

[Aaron Wu]

(In reply to Chun-Min Chang[:chunmin] from comment #42)
> (In reply to Mike Conley (:mconley) - (Needinfo me!) from comment #41)
> Hi mconley,
> Thanks for your helps and advices again.
> 
> > First of all, I'd like to better understand the product path here. Is this
> > Presentation API UI work hoping to ride the trains up to release? Or is this
> > only supposed to be enabled for Nightly in order to experiment?
> AFAIK, the core DOM Presentation API is already landed on central[0] and can
> be enabled by preference on Fennec (bug 1278205). My understanding is that
> this is an experiment for proposing the feature to Firefox. 
> 
> Hi Aaron,
> Could you provide more information here? 

Yes, it's the latter one. The main purpose is to do an experiment and have a prototype for Firefox team. That would be good to involve Firefox product team to have more comprehensive features running on Firefox product.

> > I suspect it's the latter, but please let me know. Assuming I'm right about
> > that, then your System Add-on should probably not be included in the build
> > list in the browser/extensions/moz.build unless it's a NIGHTLY_BUILD - see
> > the FlyWeb add-on for an example.
> So, should I link some .xpi to firefox like bug 1280378?
> 
> > Secondly, I'm interested in hearing about your choice to supply this UI as a
> > System Add-on. FlyWeb originally did this because of how experimental it
> > was, and likely to change. Is the Presentation API implementation in Firefox
> > similarly experimental and likely to change?
> It's definitely better to use some common modules to create something like
> https://mozilla.invisionapp.com/share/PC3Y0QSRK#/screens/106406027. However,
> the new style UI won't be updated until Bug 1267604 is land. The ETA now is
> 9/2, so it's easier to achieve what attachment 8779963 [details] expects by
> creating our own prompt UI instead of using PopupNotifications.jsm.
> If the new prompt module meets the following two requirements
> - Can prompt UI in the center of the browser
> - Can be customized to attachment 8779963 [details]
> , then it is absolutely worth to use it.
> 
> To get things done, an alternative way is to use the current prompt module
> to propose this feature first. Aaron, what did you think? What is the
> expectation from Firefox team?
> 
> > I recommend that this bug be blocked by bug 1297475, unless there's an
> > urgent need to land this UI.
> Aaron, should we need to change the ETA?

That is a good proposal, we can discuss in our group meeting on Thu. CST this week and make a final call.

Comment 45

[C.M.Chang[:chunmin]]

Attachment: [WIP] part 1 - Expose requesting browser

Comment 46

[C.M.Chang[:chunmin]]

Attachment: [WIP] part 2 - Device selection UI

Comment 47

[C.M.Chang[:chunmin]]

Hi Mike,
There are few things to sync with you:
1. After discussion with Tori and Aaron, we agree this bug could be blocked by bug 1297475 and move the ETA backward.
2. The prototype UI could use chromeWin.PopupNotifications for functional demo(comment 6) and be leveraged with the ContentPermissionIntegration.jsm. In this experiment, the feature itself is more important than the UI design, so the UI could be improved in future version.
3. We understand it's more maintainable for using common modules to generate a prompt UI. However, the UI here is slightly different from permission request. The UI is to select an available device to connect rather than asking a permission. We could use mainAction and secondaryActions[0] as choices in dropdown menu like attachment 8776832 [details], but it's better to pop up a spreaded menulist at the center of the window so user can select devices directly. Again, the design is not urgent, so we could change it later.

[0] http://searchfox.org/mozilla-central/rev/0dd403224a5acb0702bdbf7ff405067f5d29c239/toolkit/modules/PopupNotifications.jsm#271

Comment 48

[Tori Chen [:tori]]

Thanks for chunmin's update. By the way, the reason we'd like to show all devices directly, is because the pop up is triggered by user intentionally in some cases, and it would be less intuitive to ask user to click the drop-down menu again to select one device.

Comment 49

[Mike Conley (:mconley) (:⚙️)]

(In reply to Chun-Min Chang[:chunmin] from comment #47)
> Hi Mike,

Hey chunmin - sorry I've been so hard to get a hold of lately. :/ I've had multiple things vying for my attention - I apologize.

> 1. After discussion with Tori and Aaron, we agree this bug could be blocked
> by bug 1297475 and move the ETA backward.

That makes sense. Thanks!

> 3. We understand it's more maintainable for using common modules to generate
> a prompt UI. However, the UI here is slightly different from permission
> request. The UI is to select an available device to connect rather than
> asking a permission. We could use mainAction and secondaryActions[0] as
> choices in dropdown menu like attachment 8776832 [details], but it's better
> to pop up a spreaded menulist at the center of the window so user can select
> devices directly. Again, the design is not urgent, so we could change it
> later.

The way PopupNotifications works is that it will create a <xul:popupnotification> if one does not already exist for the notificationID that you pass in. _However_, if there _is_ a pre-existing <xul:popupnotificaiton> with that ID, it will use it.

What I suggest is that your system add-on, when the permission is requested, creates the <xul:popupnotification> in the appropriate browser window dynamically, and adds in either a <xul:listbox> or <xul:menulist> for device selection. It can then populate that using the device list API that you have, showing the labels for each device.

Since the nsIPresentationDeviceRequest is slightly different from nsIContentPermissionRequest, I suggest that we just import PermissionPromptPrototype from ContentPermissionIntegration.jsm, and have it wrap the request.

Take a look at the documentation I've put into https://reviewboard.mozilla.org/r/74790/diff/3#1

Your PresentationPermissionPrompt object, since its not setting permissions, need not supply the "request" property, or a permissionKey. Instead, you construct your special <xul:popupnotification> with the device list, and set callbacks on the two actions for promptActions. Note that the promptActions need not have their "action" properties set if you're not using nsIPermissionManager.

Your callback for the "cancel" button can call nsIPresentationDeviceRequest.cancel(), and your callback for "allow" can read the selected index off of the <xul:menulist> / <xul:listbox>, map that to the device in its internal array that it used for populating the list, and then call nsIPresentationDeviceRequest.select with that device.

Does that work?

Comment 50

[Aaron Wu]

(In reply to Mike Conley (:mconley) - (Needinfo me!) from comment #49)
> (In reply to Chun-Min Chang[:chunmin] from comment #47)
> > Hi Mike,
> 
> Hey chunmin - sorry I've been so hard to get a hold of lately. :/ I've had
> multiple things vying for my attention - I apologize.
> 
> > 1. After discussion with Tori and Aaron, we agree this bug could be blocked
> > by bug 1297475 and move the ETA backward.
> 
> That makes sense. Thanks!

I will move this ETA to 9/16, leave some buffer for review and QA testing before E/Sep.

> 
> > 3. We understand it's more maintainable for using common modules to generate
> > a prompt UI. However, the UI here is slightly different from permission
> > request. The UI is to select an available device to connect rather than
> > asking a permission. We could use mainAction and secondaryActions[0] as
> > choices in dropdown menu like attachment 8776832 [details], but it's better
> > to pop up a spreaded menulist at the center of the window so user can select
> > devices directly. Again, the design is not urgent, so we could change it
> > later.
> 
> The way PopupNotifications works is that it will create a
> <xul:popupnotification> if one does not already exist for the notificationID
> that you pass in. _However_, if there _is_ a pre-existing
> <xul:popupnotificaiton> with that ID, it will use it.
> 
> What I suggest is that your system add-on, when the permission is requested,
> creates the <xul:popupnotification> in the appropriate browser window
> dynamically, and adds in either a <xul:listbox> or <xul:menulist> for device
> selection. It can then populate that using the device list API that you
> have, showing the labels for each device.
> 
> Since the nsIPresentationDeviceRequest is slightly different from
> nsIContentPermissionRequest, I suggest that we just import
> PermissionPromptPrototype from ContentPermissionIntegration.jsm, and have it
> wrap the request.
> 
> Take a look at the documentation I've put into
> https://reviewboard.mozilla.org/r/74790/diff/3#1
> 
> Your PresentationPermissionPrompt object, since its not setting permissions,
> need not supply the "request" property, or a permissionKey. Instead, you
> construct your special <xul:popupnotification> with the device list, and set
> callbacks on the two actions for promptActions. Note that the promptActions
> need not have their "action" properties set if you're not using
> nsIPermissionManager.
> 
> Your callback for the "cancel" button can call
> nsIPresentationDeviceRequest.cancel(), and your callback for "allow" can
> read the selected index off of the <xul:menulist> / <xul:listbox>, map that
> to the device in its internal array that it used for populating the list,
> and then call nsIPresentationDeviceRequest.select with that device.
> 
> Does that work?

Comment 51

[C.M.Chang[:chunmin]]

(In reply to Mike Conley (:mconley) - (Needinfo me!) from comment #49)
Sounds good! We could use listbox to show all the devices for selections. It would be closer to our expectation.

Comment 52

[C.M.Chang[:chunmin]]

Comment on attachment 8785918 [details] [diff] [review]
[WIP] part 1 - Expose requesting browser

Resolved in bug 1299061

Comment 53

[Mike Conley (:mconley) (:⚙️)]

Just a note that the API in bug 1297475 has stabilized, and I expect the patches to land this week. It might now be worth while pulling down the patches (hg pull -r bead5d6d754d00503eedad959ab00ba449ea18fd https://reviewboard-hg.mozilla.org/gecko) and attempting to build your permission dialog on top of them.

Please see the FlyWeb change I made in that series, as well as the documentation in PermissionUI.jsm for examples. Happy to answer questions if you have any!

Comment 54

[C.M.Chang[:chunmin]]

(In reply to Mike Conley (:mconley) - (needinfo me!) from comment #53)
Thanks for your helps! I will try to re-implement it today.

Comment 55

[C.M.Chang[:chunmin]]

Attachment: [WIP] Device selection UI

Comment 56

[C.M.Chang[:chunmin]]

Hi mconley,

Is there any way to prevent request.principal.URI to be shown[0]?

The problem is that we want to show the message like "Pick one device to send content from <URL_HERE>", which already contains the URL's information, so we will have duplicate information here. If there is no option to disable it, what is the original reason?

[0] http://searchfox.org/mozilla-central/rev/5bdd2876aeb9dc97dc619ab3e067e86083dad023/browser/modules/PermissionUI.jsm#332

Comment 57

[Mike Conley (:mconley) (:⚙️)]

(In reply to Chun-Min Chang[:chunmin] from comment #56)
> Hi mconley,
> 
> Is there any way to prevent request.principal.URI to be shown[0]?
> 
> The problem is that we want to show the message like "Pick one device to
> send content from <URL_HERE>", which already contains the URL's information,
> so we will have duplicate information here. If there is no option to disable
> it, what is the original reason?

Hey chunmin,

I think this is to provide consistency between our different permission dialogs, so that they have the same flow:

Generally:

Origin
Message
Options

However, I'm no opposed to making the displayURI popupOption opt-out - for example, if popupOptions returns "displayURI: false", PermissionPromptPrototype.prompt will respect that, but if it's omitted, we'll default it to true. Will that work for you?

If so, are you willing to write a patch to add that capability?

Comment 58

[C.M.Chang[:chunmin]]

(In reply to Mike Conley (:mconley) - (needinfo me!) from comment #57)
> Hey chunmin,
> 
> I think this is to provide consistency between our different permission
> dialogs, so that they have the same flow:
> 
> Generally:
> 
> Origin
> Message
> Options
> 
> However, I'm no opposed to making the displayURI popupOption opt-out - for
> example, if popupOptions returns "displayURI: false",
> PermissionPromptPrototype.prompt will respect that, but if it's omitted,
> we'll default it to true. Will that work for you?
> 
> If so, are you willing to write a patch to add that capability?
Thanks for your suggestions. I'd love to do that. I will open another bug to track it. Thanks!

Comment 59

[C.M.Chang[:chunmin]]

Comment on attachment 8781402 [details]
Bug 1289974 part 1: Device selection for presentation API on Desktop;

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

Comment 60

[C.M.Chang[:chunmin]]

Comment on attachment 8781402 [details]
Bug 1289974 part 1: Device selection for presentation API on Desktop;

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

Comment 61

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8781402 [details]
Bug 1289974 part 1: Device selection for presentation API on Desktop;

https://reviewboard.mozilla.org/r/71856/#review81516

Hey, this is starting to look pretty good. I have some suggestions for simplifications below. Let me know if you have any questions.

::: browser/extensions/moz.build:12
(Diff revision 5)
>  DIRS += [
>      'e10srollout',
>      'pdfjs',
>      'pocket',
>      'webcompat',
> +    'presentation',

As I recall from earlier in the bug, this is an experimental UI for an experimental API. We should add this to the list down in line 17 so it's only put into Nightly.

Or is this looking to ride the trains out to release? (I suspect not)

::: browser/extensions/presentation/bootstrap.js:11
(Diff revision 5)
> +XPCOMUtils.defineLazyModuleGetter(this, "Presentation",
> +                                  "chrome://presentation.api/content/Presentation.jsm");

Out of curiosity, why is it necessary for Presentation.jsm to exist? Can we not move its functionality into this file, or does having the JSM provide some special advantage that I'm not seeing?

If not, putting the functionality of Presentation.jsm in here instead allows us to lose a layer of indirection.

::: browser/extensions/presentation/content/Presentation.jsm:6
(Diff revision 5)
> +/*
> + * Presentation
> + */

Probably could be a bit more descriptive here.

::: browser/extensions/presentation/content/Presentation.jsm:20
(Diff revision 5)
> +const PRESENTATION_DEVICE_PROMPT_PATH =
> +  "chrome://presentation.api/content/PresentationDevicePrompt.jsm";

I'm not even certain that having a separate JSM for the PresentationDevicePrompt is necessary.

Earlier, you had mentioned that you didn't want to load all of the JS at start-up, so the JSM's were a way of avoiding this... but if I understand correctly, all of these things will be loaded as soon as the system add-on runs its startup method, via these steps:

1. startup runs Presentation.init, which imports Presentation.jsm
2. Presentation.init runs Presentation.\_register
3. Presentation.\_register imports PresentationDevicePrompt.jsm

So I really don't see much advantage to splitting things out like this. JSM's have non-zero memory overhead as well (since they exist in separate compartments). If there's not a good reason to split them apart like this, we should probably put them into the same file.

::: browser/extensions/presentation/content/PresentationDevicePrompt.jsm:60
(Diff revision 5)
> +
> +/*
> + * Device Selection UI
> + */
> +const kNotificationId = "presentation-device-selection";
> +const kNotificationPopupIncon = "chrome://presentation-shared/skin/link.png";

"Incon" -> "Icon"

::: browser/extensions/presentation/content/PresentationDevicePrompt.jsm:62
(Diff revision 5)
> + * Device Selection UI
> + */
> +const kNotificationId = "presentation-device-selection";
> +const kNotificationPopupIncon = "chrome://presentation-shared/skin/link.png";
> +
> +// There are no dependency between kNotificationId and kNotificationAnchorId,

Nit: "are" -> "is", "dependency" -> "dependancy"

::: browser/extensions/presentation/content/PresentationDevicePrompt.jsm:65
(Diff revision 5)
> +const kNotificationPopupIncon = "chrome://presentation-shared/skin/link.png";
> +
> +// There are no dependency between kNotificationId and kNotificationAnchorId,
> +// so it's NOT necessary to name them by same prefix
> +// (e.g., presentation-device-selection).
> +const kNotificationAnchorId = "presentation-device-selection-notification-icon";

While the above comment is true, you're still using the same prefix. Why not use kNotificationId + "-notification-icon" then?

::: browser/extensions/presentation/content/PresentationDevicePrompt.jsm:81
(Diff revision 5)
> +function PresentationPermissionPrompt(aRequest) {
> +  this.request = aRequest;
> +
> +  // Remove the notification if there is no available device.
> +  let self = this;
> +  this._listener = {

Instead of having this new object that implements nsIPresentationAvailabilityListener with a 1:1 relationship with the PresentationPermissionPrompt, can we not just have the PresentationPermissionPrompt implement nsIPresentationAvailabilityListener?

That way, you also don't need to do the `self` stuff.

::: browser/extensions/presentation/content/PresentationDevicePrompt.jsm:114
(Diff revision 5)
> +  },
> +  get popupOptions() {
> +    let self = this;
> +    return {
> +      displayURI: false,
> +      learnMoreURL: "https://developer.mozilla.org/en-US/docs/Web/API/Presentation_API",

I assume this will be updated to point to the right locale for the user before this rides the trains?

::: browser/extensions/presentation/content/PresentationDevicePrompt.jsm:128
(Diff revision 5)
> +            log("Prompt is removed.");
> +          },
> +          showing: () => {
> +            log("Prompt is showing.");
> +            // Refresh devices.
> +            this._devices = GetDevices();

Would it not be better to create the popup content here so that the user doesn't see a flash of an empty list?

::: browser/extensions/presentation/content/PresentationDevicePrompt.jsm:133
(Diff revision 5)
> +            this._devices = GetDevices();
> +          },
> +          shown: () => {
> +            log("Prompt is shown.");
> +            // Insert device selection list into popupnotification element.
> +            (self._createPopupContent.bind(self))();

Correct me if I'm wrong, but since `shown()` is an arrow function, it gets `this` bound to the outer scope's `this`... and the outer scope's `this` is actually (I think) the PresentationPermissionPrompt instance, because the eventCallback function is also using an arrow function.

If so, then I _think_ you can just use this.\_createPopupContent here. Or am I wrong because PopupNotification does some special re-binding for the event callbacks?

::: browser/extensions/presentation/content/PresentationDevicePrompt.jsm:158
(Diff revision 5)
> +      notificationIcon.setAttribute("src",
> +                                    "chrome://presentation-shared/skin/link.png"); // Icon shown on URL bar
> +      notificationIcon.classList.add("notification-anchor-icon");
> +      notificationIcon.setAttribute("role", "button");
> +      notificationIcon.setAttribute("aria-label",
> +                                    "View the device-selection request");

Needs l10n? Or are you going to wait until we're ready to ride the trains?

::: browser/extensions/presentation/content/PresentationDevicePrompt.jsm:173
(Diff revision 5)
> +  get message() {
> +    return GetString("presentation.message", this._domainName);
> +  },
> +  get promptActions() {
> +    return [{
> +      label: "Select",

This and below needs l10n? Or are you going to wait until we're ready to ride the trains?

::: browser/extensions/presentation/content/PresentationDevicePrompt.jsm:286
(Diff revision 5)
> +      aRequest.cancel(Cr.NS_ERROR_NOT_AVAILABLE);
> +      return;
> +    }
> +
> +    // Show the prompt to users.
> +    let promptUI = new PresentationPermissionPrompt(aRequest);

Since we got the devices once above, maybe we should pass them into the constructor here and have the PresentationPermissionPrompt continue to hold onto them instead of re-querying for them later on.

Comment 62

[C.M.Chang[:chunmin]]

Comment on attachment 8781402 [details]
Bug 1289974 part 1: Device selection for presentation API on Desktop;

https://reviewboard.mozilla.org/r/71856/#review81516

Thanks very much for sharing your time to help and guide.

> As I recall from earlier in the bug, this is an experimental UI for an experimental API. We should add this to the list down in line 17 so it's only put into Nightly.
> 
> Or is this looking to ride the trains out to release? (I suspect not)

I think it will only be released in Nightly, but let me check it again.

> Out of curiosity, why is it necessary for Presentation.jsm to exist? Can we not move its functionality into this file, or does having the JSM provide some special advantage that I'm not seeing?
> 
> If not, putting the functionality of Presentation.jsm in here instead allows us to lose a layer of indirection.

I will intergrate them into one file in next version.

> Probably could be a bit more descriptive here.

OK.

> I'm not even certain that having a separate JSM for the PresentationDevicePrompt is necessary.
> 
> Earlier, you had mentioned that you didn't want to load all of the JS at start-up, so the JSM's were a way of avoiding this... but if I understand correctly, all of these things will be loaded as soon as the system add-on runs its startup method, via these steps:
> 
> 1. startup runs Presentation.init, which imports Presentation.jsm
> 2. Presentation.init runs Presentation.\_register
> 3. Presentation.\_register imports PresentationDevicePrompt.jsm
> 
> So I really don't see much advantage to splitting things out like this. JSM's have non-zero memory overhead as well (since they exist in separate compartments). If there's not a good reason to split them apart like this, we should probably put them into the same file.

Hmm...I am afraid it's another issue. The separation is used to reserve the scalability for the future(There might be other modules need to be implemented and included in this add-on). However, I may overengineer again. I can do it when it need.

> "Incon" -> "Icon"

Thanks!

> Nit: "are" -> "is", "dependency" -> "dependancy"

Thanks for the reminder.

> Instead of having this new object that implements nsIPresentationAvailabilityListener with a 1:1 relationship with the PresentationPermissionPrompt, can we not just have the PresentationPermissionPrompt implement nsIPresentationAvailabilityListener?
> 
> That way, you also don't need to do the `self` stuff.

Use the object self as listener...Great idea!

> Would it not be better to create the popup content here so that the user doesn't see a flash of an empty list?

When the notification is "showing"[0], the popupnotification is not created yet[1], so there is nothing can be inserted.
However, if it about readibility, I can put ```this._devices = GetDevices()``` inside ```_createPopupContent``` so that reader will know this two is related.

[0] http://searchfox.org/mozilla-central/rev/3411332d29d377bf86405fc3ea72461ea0cb0295/toolkit/modules/PopupNotifications.jsm#800
[1] http://searchfox.org/mozilla-central/rev/3411332d29d377bf86405fc3ea72461ea0cb0295/toolkit/modules/PopupNotifications.jsm#664

> Correct me if I'm wrong, but since `shown()` is an arrow function, it gets `this` bound to the outer scope's `this`... and the outer scope's `this` is actually (I think) the PresentationPermissionPrompt instance, because the eventCallback function is also using an arrow function.
> 
> If so, then I _think_ you can just use this.\_createPopupContent here. Or am I wrong because PopupNotification does some special re-binding for the event callbacks?

Yes, you're right. I misunderstand ```this``` is the ```handler``` object.

> Needs l10n? Or are you going to wait until we're ready to ride the trains?

I will use l10n string instead.

> This and below needs l10n? Or are you going to wait until we're ready to ride the trains?

We will update the l10n string soon before release it.

> Since we got the devices once above, maybe we should pass them into the constructor here and have the PresentationPermissionPrompt continue to hold onto them instead of re-querying for them later on.

The avaliable devices may change, so that's why we need to refresh it. If the devices here is passed into the constructor but dismission is allowed, then we can't guarantee the device chosen is still available when user try to connect to it. The device might be disabled after user dismiss the request.

Comment 63

[C.M.Chang[:chunmin]]

Comment on attachment 8781402 [details]
Bug 1289974 part 1: Device selection for presentation API on Desktop;

https://reviewboard.mozilla.org/r/71856/#review81516

> I think it will only be released in Nightly, but let me check it again.

The presentation API will be enabled in Nightly first and then release in Firefox 52.

Comment 64

[C.M.Chang[:chunmin]]

(In reply to Chun-Min Chang[:chunmin] from comment #62)
> > Since we got the devices once above, maybe we should pass them into the constructor here and have the PresentationPermissionPrompt continue to hold onto them instead of re-querying for them later on.
> 
> The avaliable devices may change, so that's why we need to refresh it. If
> the devices here is passed into the constructor but dismission is allowed,
> then we can't guarantee the device chosen is still available when user try
> to connect to it. The device might be disabled after user dismiss the
> request.
If we cancel the request directly when the notification is dismissed, then we can use "devices" as a parameter and passed into the PresentationPermissionPrompt without such issue. What do you think?

Comment 65

[C.M.Chang[:chunmin]]

Attachment: WIP: Device selection

Comment 66

[C.M.Chang[:chunmin]]

Attachment: WIP: Device selection(dismiss as cancel)

Comment 67

[C.M.Chang[:chunmin]]

Comment on attachment 8781402 [details]
Bug 1289974 part 1: Device selection for presentation API on Desktop;

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

Comment 68

[C.M.Chang[:chunmin]]

(In reply to Chun-Min Chang[:chunmin] from comment #64)
> (In reply to Chun-Min Chang[:chunmin] from comment #62)
> > > Since we got the devices once above, maybe we should pass them into the constructor here and have the PresentationPermissionPrompt continue to hold onto them instead of re-querying for them later on.
> > 
> > The avaliable devices may change, so that's why we need to refresh it. If
> > the devices here is passed into the constructor but dismission is allowed,
> > then we can't guarantee the device chosen is still available when user try
> > to connect to it. The device might be disabled after user dismiss the
> > request.
> If we cancel the request directly when the notification is dismissed, then
> we can use "devices" as a parameter and passed into the
> PresentationPermissionPrompt without such issue. What do you think?
A bit more information here:
The presentation API currently used by chrome and chromecast doesn't allow "dismiss". The user has only two choice: select or cancel.

Comment 69

[Mike Conley (:mconley) (:⚙️)]

(In reply to Chun-Min Chang[:chunmin] from comment #63)
> Comment on attachment 8781402 [details]
> Bug 1289974 - Device selection for presentation API on Firefox;
> 
> https://reviewboard.mozilla.org/r/71856/#review81516
> 
> > I think it will only be released in Nightly, but let me check it again.
> 
> The presentation API will be enabled in Nightly first and then release in
> Firefox 52.

This means that localization and strings need to be landed before this uplifts to Aurora, since 52 is currently on mozilla-central. I'll continue to review with this schedule in mind.

(In reply to Chun-Min Chang[:chunmin] from comment #64)
> (In reply to Chun-Min Chang[:chunmin] from comment #62)
> > The avaliable devices may change, so that's why we need to refresh it. If
> > the devices here is passed into the constructor but dismission is allowed,
> > then we can't guarantee the device chosen is still available when user try
> > to connect to it. The device might be disabled after user dismiss the
> > request.
> If we cancel the request directly when the notification is dismissed, then
> we can use "devices" as a parameter and passed into the
> PresentationPermissionPrompt without such issue. What do you think?

I think the danger that the list of devices is out of date might always be true. I don't think we add much in terms of safety by doing the query again once the popup is shown, since the user might disconnect a device as soon as the popup appears.

What currently happens if you provide a disconnected device to the request?

Comment 70

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8781402 [details]
Bug 1289974 part 1: Device selection for presentation API on Desktop;

https://reviewboard.mozilla.org/r/71856/#review82928

Thanks so much for simplifying the patch! This is much easier to read and reason about.

I think I made a mistake in my last review. I forgot that devices could go out of date, as you pointed out, and I realize that the popup can be re-opened after being dismissed by a switch in user focus. I think going back to re-populating the device list on shown is the best way forward. Sorry about that!

I've found some small things that need to be fixed still, but it's all quite minor. This looks great. Thanks so much for this - one more interation should do it.

::: browser/extensions/presentation/content/PresentationDevicePrompt.jsm:1
(Diff revision 6)
> +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */

Thanks for combining these files - this is much simpler to read and reason about.

::: browser/extensions/presentation/content/PresentationDevicePrompt.jsm:85
(Diff revision 6)
> +            log("Dismissed by user. Cancel the request.");
> +            // Cancel the request and remove the notification.
> +            this.request.cancel(Cr.NS_ERROR_NOT_AVAILABLE);
> +            let chromeWin = this.browser.ownerGlobal;
> +            let notification =
> +              chromeWin.PopupNotifications.getNotification(this.notificationID,
> +                                                           this.browser);
> +            chromeWin.PopupNotifications.remove(notification);

Are you certain this is the desired behaviour? A user can "accidentally" dismiss the dialog by just focusing outside of the popup. Normally for permissions, if the user dismisses the popup, we keep the icon in the URL bar so that the user can try to answer again.

This bolsters your argument for querying for devices when the popup is shown.

::: browser/extensions/presentation/content/PresentationDevicePrompt.jsm:145
(Diff revision 6)
> +    return GetString("presentation.message", this._domainName);
> +  },
> +  get promptActions() {
> +    return [{
> +      label: GetString("presentation.deviceprompt.select"),
> +      accessKey: "S",

This also needs to be localized.

::: browser/extensions/presentation/content/PresentationDevicePrompt.jsm:159
(Diff revision 6)
> +        this.request.select(device);
> +        log("device: " + device.name + "(" + device.id + ") is selected!");
> +      },
> +    }, {
> +      label: GetString("presentation.deviceprompt.cancel"),
> +      accessKey: "C",

Needs localization.

::: browser/extensions/presentation/content/PresentationDevicePrompt.jsm:192
(Diff revision 6)
> +    }
> +
> +    let popupnotificationcontent =
> +      chromeDoc.createElement("popupnotificationcontent");
> +
> +    this._listbox = chromeDoc.createElement("richlistbox");

If the popup is re-shown, this list is going to be re-created.

We probably should see if there's a pre-existing list, empty it, and refresh it with new list items from the devices list.

::: browser/extensions/presentation/content/PresentationDevicePrompt.jsm:194
(Diff revision 6)
> +    let popupnotificationcontent =
> +      chromeDoc.createElement("popupnotificationcontent");
> +
> +    this._listbox = chromeDoc.createElement("richlistbox");
> +    this._listbox.setAttribute("flex", "1");
> +    this._devices.forEach((device) => {

I'm going to revert my original suggestion of querying for the devices once. We should do as you had originally done, and query for them upon construction of the list. It's not a silver bullet against being out of date, but I think we have a higher probability of having an up-to-date list in that case.

Sorry about the flip-flopping.

::: browser/extensions/presentation/content/PresentationDevicePrompt.jsm:220
(Diff revision 6)
> +function PresentationDevicePrompt() {}
> +
> +PresentationDevicePrompt.prototype = {
> +  // properties required for XPCOM registration:
> +  classID: PRESENTATIONDEVICEPROMPT_CID,
> +  classDescription: "Firefox Desktop Presentation Device Prompt",

We tend not to put the product name anywhere except our branding directories. Let's just make this "Presentation API Device Prompt"

::: browser/extensions/presentation/locale/en-US/presentation.properties:1
(Diff revision 6)
> +presentation.message=Pick one device to send content from %S
> +presentation.urlbar.tooltiptext=View the device-selection request
> +presentation.deviceprompt.select=Select
> +presentation.deviceprompt.cancel=Cancel

I assume these strings have been approved by UX?

Comment 71

[C.M.Chang[:chunmin]]

(In reply to Mike Conley (:mconley) - (needinfo me!) from comment #69)
> (In reply to Chun-Min Chang[:chunmin] from comment #64)
> > (In reply to Chun-Min Chang[:chunmin] from comment #62)
> > > The avaliable devices may change, so that's why we need to refresh it. If
> > > the devices here is passed into the constructor but dismission is allowed,
> > > then we can't guarantee the device chosen is still available when user try
> > > to connect to it. The device might be disabled after user dismiss the
> > > request.
> > If we cancel the request directly when the notification is dismissed, then
> > we can use "devices" as a parameter and passed into the
> > PresentationPermissionPrompt without such issue. What do you think?
> 
> I think the danger that the list of devices is out of date might always be
> true. I don't think we add much in terms of safety by doing the query again
> once the popup is shown, since the user might disconnect a device as soon as
> the popup appears.
> 
> What currently happens if you provide a disconnected device to the request?
The connection will fail but it won't be deleted from device-list until we receive the "device-remove" from mDNS.

Comment 72

[C.M.Chang[:chunmin]]

Comment on attachment 8781402 [details]
Bug 1289974 part 1: Device selection for presentation API on Desktop;

https://reviewboard.mozilla.org/r/71856/#review82928

Thanks for sharing your time for this. We get many useful feedbacks here and it makes us to refine the whole user experience ;)

> Are you certain this is the desired behaviour? A user can "accidentally" dismiss the dialog by just focusing outside of the popup. Normally for permissions, if the user dismisses the popup, we keep the icon in the URL bar so that the user can try to answer again.
> 
> This bolsters your argument for querying for devices when the popup is shown.

It's currently expected bahaviour. I'll explain more below.

> If the popup is re-shown, this list is going to be re-created.
> 
> We probably should see if there's a pre-existing list, empty it, and refresh it with new list items from the devices list.

I tried that before, but I found that the previously created popupnotification is always deleted in ```_clearPanel```[0]. The ```_clearPanel``` will be called in ```_onPopupHidden```[1] and ```_refreshPanel```[2]. Thus, I assume the previously created popupnotification will be always removed. However, if it's wrong, I can add code to check the existence of the previous content before creating a new one.

[0] http://searchfox.org/mozilla-central/rev/3e03a4064eb585d96f28023785a5c242969878a6/toolkit/modules/PopupNotifications.jsm#615
[1] http://searchfox.org/mozilla-central/rev/3e03a4064eb585d96f28023785a5c242969878a6/toolkit/modules/PopupNotifications.jsm#1176
[2] http://searchfox.org/mozilla-central/rev/3e03a4064eb585d96f28023785a5c242969878a6/toolkit/modules/PopupNotifications.jsm#647

> I'm going to revert my original suggestion of querying for the devices once. We should do as you had originally done, and query for them upon construction of the list. It's not a silver bullet against being out of date, but I think we have a higher probability of having an up-to-date list in that case.
> 
> Sorry about the flip-flopping.

The presentation device-list is not real-time updated, so the disconnected devices will be shown on the selection list even we use the up-to-date list. The available devices will be obtained from nsIPresentationDeviceManager, however, its update depends on mDNS and mDNS is not real-time responsive. Consider the following situation if we don't remove the notification when user dismiss it:

1. Enable one presentation device, denoted _D_ here
2. Generate a presentation request
    - The _D_ will be listed on the selection list of the prompt dialog
3. Dismiss the request
4. Disable the presentation device
5. Open the dismissed request
    - The unavailable _D_ still will be listed

That's the reason why I remove the code. Discussed with UX, we thought we should cancel the request when user dismiss it for the following reason:

1. In normal usecase, the page should have buttons to prompt the presentation requests. If the request is dismissed unintentionally, user can re-click the button to get the request again. (or maybe they can refresh the page to get it if the request is automatically generated when the page is loaded.)
2. If we don't cancel the request when user dismiss it, then it may contain the disconnected devices like the above case. We don't want to show them to users(In above case, the expected results is to remove the request). To avoid such case, we may need to decide how long the request should be pended. The user story will be more complicated. However, we desire a simpler user story in the first demo.
3. The behaviour is same as chrome.

Comment 73

[C.M.Chang[:chunmin]]

Attachment: [WIP] Device selection UI (dismiss as cancel)

changes:
- new icon

Comment 74

[C.M.Chang[:chunmin]]

Attachment: Device selection without NotNow

Comment 75

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

A web-platform test case should be affected after landing this patch: https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/presentation-api/controlling-ua/startNewPresentation_error.html#23

This test case is testing following statement in spec:

  If the algorithm isn't allowed to show a popup, return a Promise rejected with an InvalidAccessError exception and abort these steps.

This statement is implemented in Gecko by detecting if nsIPresentationDevicePrompt is implemented or not. This test case is passed because no device prompt is in m-c. We should add some pref in order to control the test environment, see https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/meta/presentation-api/controlling-ua/startNewPresentation_error.html.ini.

Comment 76

[C.M.Chang[:chunmin]]

Comment on attachment 8781402 [details]
Bug 1289974 part 1: Device selection for presentation API on Desktop;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71856/diff/6-7/

Comment 77

[C.M.Chang[:chunmin]]

Attachment: web platform test

Hi SC,
Could you take a look the modification of the web platform test for presentation api?

Comment 78

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

Comment on attachment 8807053 [details] [diff] [review]
web platform test

Review of attachment 8807053 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/presentation/PresentationRequest.cpp
@@ +148,5 @@
> +  // Drop the request if it's a recei
> +  if (Preferences::GetBool("dom.presentation.testing.simulate-receiver")) {
> +    aRv.Throw(NS_ERROR_DOM_INVALID_ACCESS_ERR);
> +    return nullptr;
> +  }

You should use |nsContentUtils::GetPresentationURL| to determine if the PresentationRequest is initiated at a receiving browsing context, see reference code at https://dxr.mozilla.org/mozilla-central/source/dom/presentation/Presentation.cpp#177

And, remember to complete the comment to explain why receiver browsing context cannot allow user gesture.

Comment 79

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8781402 [details]
Bug 1289974 part 1: Device selection for presentation API on Desktop;

https://reviewboard.mozilla.org/r/71856/#review90560

This looks okay to me, assuming that UX has signed off on the design and the strings. What is the test plan? Is there a bug to write automated tests for this feature?

::: browser/extensions/presentation/content/PresentationDevicePrompt.jsm:54
(Diff revision 7)
> +// To insert our own popupnotification content with the device list into the
> +// displayed popupnotification element.

This feels a bit like an improper sentence / sentence fragment. There's no verb, for example.

Maybe this should be "This will insert our own popupnotification content with the device list into the displayed popupnotification element." Is that an accurate reformulation?

::: browser/extensions/presentation/content/PresentationDevicePrompt.jsm:204
(Diff revision 7)
> +    popupnotificationcontent.appendChild(this._listbox);
> +    popupnotification.appendChild(popupnotificationcontent);

Just to double-check, the old `popupnotificationcontent` is removed when the popup is removed, right?

::: browser/extensions/presentation/locale/en-US/presentation.properties:1
(Diff revision 7)
> +presentation.message=Select one device to send the content.
> +presentation.urlbar.tooltiptext=View the device-selection request
> +presentation.deviceprompt.select.label=Send
> +presentation.deviceprompt.select.accessKey=S
> +presentation.deviceprompt.cancel.label=Cancel
> +presentation.deviceprompt.cancel.accessKey=C

I assume these strings have been approved by UX?

::: browser/extensions/presentation/skin/shared/link.svg:5
(Diff revision 7)
> +<g id="圖層_2">
> +</g>
> +<g id="圖層_1">

I'm pretty sure by convention (sorry about this), we need to use English ID names.

Comment 80

[C.M.Chang[:chunmin]]

Comment on attachment 8781402 [details]
Bug 1289974 part 1: Device selection for presentation API on Desktop;

https://reviewboard.mozilla.org/r/71856/#review90560

Thanks for your review! The used terms are already checked by UX. The web-platform test for presentation api is the only one test for the device prompt now. However, we've temporarily hire a software outsourcing company to test the presentation api manually.

> This feels a bit like an improper sentence / sentence fragment. There's no verb, for example.
> 
> Maybe this should be "This will insert our own popupnotification content with the device list into the displayed popupnotification element." Is that an accurate reformulation?

Thanks for the correction!

> Just to double-check, the old `popupnotificationcontent` is removed when the popup is removed, right?

The notification content is removed everytime when [```_refreshPanel```](http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/toolkit/modules/PopupNotifications.jsm#647) is called, and it will be fired in [```_showPanel```](http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/toolkit/modules/PopupNotifications.jsm#809). Thus, the old notificationcontent should be removed before the new one is created.

> I assume these strings have been approved by UX?

Yes.

> I'm pretty sure by convention (sorry about this), we need to use English ID names.

Thanks for the reminder. I'll change it.

Comment 81

[C.M.Chang[:chunmin]]

Attachment: Bug 1289974 - part 2: web-platform test;

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

Comment 82

[C.M.Chang[:chunmin]]

Comment on attachment 8781402 [details]
Bug 1289974 part 1: Device selection for presentation API on Desktop;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71856/diff/7-8/

Comment 83

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

Comment on attachment 8808051 [details]
Bug 1289974 - part 2: web-platform test;

https://reviewboard.mozilla.org/r/90984/#review90854

::: dom/presentation/PresentationRequest.cpp:154
(Diff revision 1)
>    if (NS_WARN_IF(!global)) {
>      aRv.Throw(NS_ERROR_UNEXPECTED);
>      return nullptr;
>    }
>  
> +  // Drop the request if it's a receiver.

Hmm, I don't see anything like this in the spec.
But, there is comment "The PresentationRequest object MUST be implemented in a controlling browsing context". So why do we even expose it on receiver side? There is Pref="dom.presentation.controller.enabled" in the interface, but should it be some Func="..." which then checks whether we're actually the controller or receiver - in order to hide it properly also in 1-UA case.

Comment 84

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

(In reply to Olli Pettay [:smaug] from comment #83)
> Comment on attachment 8808051 [details]
> Bug 1289974 - part 2: web-platform test;
> 
> https://reviewboard.mozilla.org/r/90984/#review90854
> 
> ::: dom/presentation/PresentationRequest.cpp:154
> (Diff revision 1)
> >    if (NS_WARN_IF(!global)) {
> >      aRv.Throw(NS_ERROR_UNEXPECTED);
> >      return nullptr;
> >    }
> >  
> > +  // Drop the request if it's a receiver.
> 
> Hmm, I don't see anything like this in the spec.
> But, there is comment "The PresentationRequest object MUST be implemented in
> a controlling browsing context". So why do we even expose it on receiver
> side? There is Pref="dom.presentation.controller.enabled" in the interface,
> but should it be some Func="..." which then checks whether we're actually
> the controller or receiver - in order to hide it properly also in 1-UA case.

The spec I'm referring to is in section 6.3.2 step 1:

If the algorithm isn't allowed to show a popup, return a Promise rejected with an InvalidAccessError exception and abort these steps.

startNewPresentation_error.html [1] is the corresponding automated web-platform test for this statement.

I'm trying to find out a reasonable scenario/configuration in Gecko without creating another test-only pref. and 1-UA receiving context can be used here since the receiver is displayed in a chromeless environment. Is there any condition we can use for implementing this statement? Or do we simply skip this test case on treeherder?

[1] https://github.com/w3c/web-platform-tests/blob/master/presentation-api/controlling-ua/startNewPresentation_error.html

Comment 85

[C.M.Chang[:chunmin]]

Comment on attachment 8808051 [details]
Bug 1289974 - part 2: web-platform test;

https://reviewboard.mozilla.org/r/90984/#review90854

> Hmm, I don't see anything like this in the spec.
> But, there is comment "The PresentationRequest object MUST be implemented in a controlling browsing context". So why do we even expose it on receiver side? There is Pref="dom.presentation.controller.enabled" in the interface, but should it be some Func="..." which then checks whether we're actually the controller or receiver - in order to hide it properly also in 1-UA case.

It intends to produce a promise rejected with an InvalidAccessError exception to pass the [web platform test: startNewPresentation_error.html](https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/presentation-api/controlling-ua/startNewPresentation_error.html#22). It is mentioned in [here](https://www.w3.org/TR/2016/CR-presentation-api-20160714/#starting-a-presentation). Is there other reasonable way to pass the test instead of this approach?

Comment 86

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

My question is that why is PresentationRequest interface visible at all in browsing contexts which are for receiver (event in 1-UA case). Or is the idea that the same *browsing context* can be both controller and receiver? (what would be the use case for that?)

Comment 87

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

https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/presentation-api/controlling-ua/startNewPresentation_error.html#22 looks like something quite different.
That talks about 'allowed to show a popup', not about something being receiver.

allowed to show a popup is defined in HTML spec, and for example HTMLInputElement::IsPopupBlocked() does a check like that.

Comment 88

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

(In reply to Olli Pettay [:smaug] from comment #87)
> https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/
> presentation-api/controlling-ua/startNewPresentation_error.html#22 looks
> like something quite different.
> That talks about 'allowed to show a popup', not about something being
> receiver.
> 
> allowed to show a popup is defined in HTML spec, and for example
> HTMLInputElement::IsPopupBlocked() does a check like that.
@smaug thanks for the information!
I don't quite understand how this implementation matched with the definition in HTML spec, but anyway we'll follow the same rule as in |HTMLInputElement::IsPopupBlocked|.

Comment 89

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

Well, isn't the idea that presentation.start() can succeed only when handling some trusted user initiated event. The code in HTMLInputElement::IsPopupBlocked may not be quite the right, but it basically relies on whatever we do elsewhere for popups.

You may want to look at these two lines http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/dom/events/Event.cpp#657,696
and the callers of those methods.

Comment 90

[C.M.Chang[:chunmin]]

Comment on attachment 8781402 [details]
Bug 1289974 part 1: Device selection for presentation API on Desktop;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71856/diff/8-9/

Comment 91

[C.M.Chang[:chunmin]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca4d6f52a9a9

Comment 92

[C.M.Chang[:chunmin]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff8518bf65ae

Comment 93

[C.M.Chang[:chunmin]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=335bc86392c7

Comment 94

[C.M.Chang[:chunmin]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=30af7ee01fdf

Comment 95

[C.M.Chang[:chunmin]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=785014b423f6

Comment 96

[C.M.Chang[:chunmin]]

Attachment: Bug 1289974 part 2: Disable web-platform-test for allowing popup temporarily;

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

Comment 97

[C.M.Chang[:chunmin]]

Comment on attachment 8781402 [details]
Bug 1289974 part 1: Device selection for presentation API on Desktop;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71856/diff/9-10/

Comment 98

[C.M.Chang[:chunmin]]

Hi smaug,
Thanks for your information and feedback. I'd like to move the discussion of web platform test to another bug, and temporarily use a pref to disallow the popup in the test. Is that okay to you?

Comment 99

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

What does adding such pref help? Just to pass test but effectively having buggy implementation?
That doesn't sound useful.
Doesn't the popup blocking check work here? If not, why?

Comment 100

[C.M.Chang[:chunmin]]

(In reply to Olli Pettay [:smaug] from comment #99)
> What does adding such pref help? Just to pass test but effectively having
> buggy implementation?
> That doesn't sound useful.
> Doesn't the popup blocking check work here? If not, why?
I was wondering if you could explain more about how the bug will happen. It's not allow to change the startNewPresentation_error.html[0], and startNewPresentation_error.html.ini[1] is only thing that can be changed. Thus, we can only add a pref to pass the test. What is the "trusted user initiated event" mentioned above that will be fired before popping nsIPresentationDevicePrompt, and it also can be triggered by a pref setting?

[0] https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/presentation-api/controlling-ua/startNewPresentation_error.html#22
[1] https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/meta/presentation-api/controlling-ua/startNewPresentation_error.html.ini

Comment 101

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

When popup blocking isn't active, we're in a state where it is 'allowed to show a popup'. In practice that means cases like we're handling a trusted 'click' event.
(Events have .isTrusted field to indicate they are coming from UA and not dispatched by some random script)

Comment 102

[C.M.Chang[:chunmin]]

(In reply to Olli Pettay [:smaug] from comment #101)
Thanks for the detailed explaination. I try to figure out two things:
1. Since we currently don't have the "allow popup" policy, what is the bug caused by lacking this implementation? What are the conditions to trigger the bug? I think "the bug" is the answer to define the "allow popup" policy.
2. How does "PopupAllowedForEvent" match to presentation API? Is there a "(un)trusted event" fired before nsIPresentationDevicePromot? Maybe we only need to find one thing untrusted, even it's not an event, then we can use it to disallow the popup.

I am not familiar about the internal work of Presentation API because I focus on front-end stuff in this project. S.C, could you provide some suggestions?

Comment 103

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

What you mean with (1)? Gecko does have "allow popup" policy.  Presentation API implementation shouldn't need to think about various events, but just the generic popup blocking.
That is what HTMLInputElement is doing for example, nsGlobalWindow has similar stuff too
http://searchfox.org/mozilla-central/rev/4b6cab91f93c73ae591dafaea40fd5704b41810e/dom/base/nsGlobalWindow.cpp#12366-12389

Comment 104

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

(In reply to Chun-Min Chang[:chunmin] from comment #102)
> (In reply to Olli Pettay [:smaug] from comment #101)
> Thanks for the detailed explaination. I try to figure out two things:
> 1. Since we currently don't have the "allow popup" policy, what is the bug
> caused by lacking this implementation? What are the conditions to trigger
> the bug? I think "the bug" is the answer to define the "allow popup" policy.
> 2. How does "PopupAllowedForEvent" match to presentation API? Is there a
> "(un)trusted event" fired before nsIPresentationDevicePromot? Maybe we only
> need to find one thing untrusted, even it's not an event, then we can use it
> to disallow the popup.
> 
> I am not familiar about the internal work of Presentation API because I
> focus on front-end stuff in this project. S.C, could you provide some
> suggestions?

From the recent comments on this bug, it's more like a incorrect implementation of the 'don't allow popup' error handling in our WebAPI implementation. Please file a new bug and move following discussion to that one.
startNewPresentation_error.html.ini is something you can change to ignore the test failure for now.

Comment 105

[C.M.Chang[:chunmin]]

Comment on attachment 8781402 [details]
Bug 1289974 part 1: Device selection for presentation API on Desktop;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71856/diff/10-11/

Comment 106

[C.M.Chang[:chunmin]]

Comment on attachment 8809291 [details]
Bug 1289974 part 2: Disable web-platform-test for allowing popup temporarily;

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

Comment 107

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

Comment on attachment 8809291 [details]
Bug 1289974 part 2: Disable web-platform-test for allowing popup temporarily;

https://reviewboard.mozilla.org/r/91870/#review92668

Comment 108

[C.M.Chang[:chunmin]]

Comment on attachment 8781402 [details]
Bug 1289974 part 1: Device selection for presentation API on Desktop;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71856/diff/11-12/

Comment 109

[C.M.Chang[:chunmin]]

Comment on attachment 8809291 [details]
Bug 1289974 part 2: Disable web-platform-test for allowing popup temporarily;

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

Comment 110

[C.M.Chang[:chunmin]]

Comment on attachment 8809291 [details]
Bug 1289974 part 2: Disable web-platform-test for allowing popup temporarily;

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

Comment 111

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9509a063368d
part 1: Device selection for presentation API on Firefox; r=mconley
https://hg.mozilla.org/integration/autoland/rev/32fcb004c724
part 2: Disable web-platform-test for allowing popup temporarily; r=smaug

Comment 112

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back these out for pgo xperf failures like https://treeherder.mozilla.org/logviewer.html#?job_id=6612756&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/03c35ed9b3ea

Comment 113

[C.M.Chang[:chunmin]]

Add this add-on to xperf_whitelist[0] may solve the problem. I'll build it on try server again. Thanks for the help.

[0] http://searchfox.org/mozilla-central/rev/886d5186f0598ab2f8a9953eb5a4dab9750ef834/testing/talos/talos/xtalos/xperf_whitelist.json#11

Comment 114

[C.M.Chang[:chunmin]]

Comment on attachment 8781402 [details]
Bug 1289974 part 1: Device selection for presentation API on Desktop;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71856/diff/12-13/

Comment 115

[C.M.Chang[:chunmin]]

Comment on attachment 8809291 [details]
Bug 1289974 part 2: Disable web-platform-test for allowing popup temporarily;

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

Comment 116

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/369e16cb8e67
part 1: Device selection for presentation API on Desktop; r=mconley
https://hg.mozilla.org/integration/autoland/rev/d17322d95e87
part 2: Disable web-platform-test for allowing popup temporarily; r=smaug

Comment 117

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/369e16cb8e67
https://hg.mozilla.org/mozilla-central/rev/d17322d95e87