1265441 - [FlyWeb] Move FlyWeb UI implementation for Desktop to system add-on

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Justin D'Arcangelo [:justindarc]]

One pre-requisite for landing the FlyWeb feature on m-c is having all user-facing UI implemented as a system add-on. This bug is for tracking the implementation of the FlyWeb UI for Desktop.

Comment 1

[Justin D'Arcangelo [:justindarc]]

Attachment: bug1265441.patch

Comment 2

[Kannan Vijayan [:djvj]]

Comment on attachment 8743654 [details] [diff] [review]
bug1265441.patch

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

::: browser/extensions/flyweb/bootstrap.js
@@ +35,5 @@
> +      viewId: "flyweb-panel",
> +      defaultArea: CustomizableUI.AREA_NAVBAR,
> +      label: gFlyWebBundle.GetStringFromName("flyweb-button.label"),
> +      tooltiptext: gFlyWebBundle.GetStringFromName("flyweb-button.tooltiptext"),
> +      

Nit: whitespace on blank line.

@@ +112,5 @@
> +        };
> +
> +        this._discoveryCallback.start();
> +      },
> +      

Nit: whitespace on blank line.

::: browser/extensions/flyweb/skin/osx/flyweb.css
@@ +10,5 @@
> +}
> +
> +/* Necessary to mimic the behavior of all other buttons, which are darker when
> +  pressed.
> +#flyweb-button:hover:active:not([disabled="true"]):not([cui-areatype="menu-panel"]) {

Do we need this if commented out?

::: browser/extensions/flyweb/skin/windows/flyweb.css
@@ +10,5 @@
> +}
> +
> +/* Necessary to mimic the behavior of all other buttons, which are darker when
> +  pressed.
> +#flyweb-button:hover:active:not([disabled="true"]):not([cui-areatype="menu-panel"]) {

Do we need this if commented out?

Comment 3

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

Comment on attachment 8743654 [details] [diff] [review]
bug1265441.patch

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

::: browser/extensions/flyweb/bootstrap.js
@@ +4,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
> +
> +Cu.import("resource:///modules/CustomizableUI.jsm");

Probably best to use XPCOMUtils.defineLazyModuleGetter on CUI, Console, Services, to avoid slowdowns when the bootstrap.js is loaded up (particularly for CUI, which initializes a bunch of stuff on first import)

@@ +61,5 @@
> +        this._sheetURI = Services.io.newURI("chrome://flyweb/skin/flyweb.css", null, null);
> +        aDocument.defaultView.QueryInterface(Ci.nsIInterfaceRequestor).
> +            getInterface(Ci.nsIDOMWindowUtils).loadSheet(this._sheetURI, 1);
> +
> +        this._discoveryManager = new aDocument.defaultView.FlyWebDiscoveryManager();

This Widget object is a singleton that's shared between all windows. What this is saying here is that for every new window, when the widget is constructed in that window, we will overwrite the _discoveryManager in that singleton. I suspect that's not what we want... maybe only set this the first time.

@@ +75,5 @@
> +
> +        let panel = doc.getElementById("flyweb-panel");
> +        let vbox  = doc.getElementById("flyweb-items");
> +
> +        this._discoveryCallback = {

Again, just be advised that the Widget object here is a singleton shared across windows. So you're overwriting this for other windows when this is called. This is probably okay here though since in onViewHiding (which would be called in other windows before this is fired), _discoveryCallback is cleared out. Still, something to be aware of.

@@ +92,5 @@
> +              let button = doc.createElement("toolbarbutton");
> +              button.setAttribute("class", "subviewbutton");
> +              button.setAttribute("label", service.displayName);
> +              button.setAttribute("data-service-id", service.serviceId);
> +              vbox.appendChild(button);

Probably best to add these to a documentFragment and add the documentFragment all at once, once the list is constructed. See https://developer.mozilla.org/en-US/docs/Web/API/Document/createDocumentFragment for details.

@@ +123,5 @@
> +        if (aEvent.type === "command") {
> +          let serviceId = aEvent.target.getAttribute("data-service-id");
> +          this._discoveryManager.connectToService(serviceId, {
> +            connectSucceeded(service) {
> +              let url = "http://" + service.hostname + "/";

Should this default to https? Or is SSL not a thing we care about currently for FlyWeb?

::: browser/extensions/flyweb/install.rdf.in
@@ +11,5 @@
> +  <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>

We should make this multiprocess compatible by default if we can. Can you please add the multiprocessCompatible flag? https://developer.mozilla.org/en-US/Add-ons/Install_Manifests#multiprocessCompatible

::: browser/extensions/flyweb/skin/linux/flyweb.css
@@ +10,5 @@
> +}
> +
> +/* Necessary to mimic the behavior of all other buttons, which are darker when
> +  pressed.
> +#flyweb-button:hover:active:not([disabled="true"]):not([cui-areatype="menu-panel"]) {

Same as djvj's comment in the other stylesheet.

::: browser/extensions/moz.build
@@ +5,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  DIRS += [
>      'e10srollout',
> +    'flyweb',

We should only add this to the list here for Nightly and Aurora, no?

Comment 4

[Justin D'Arcangelo [:justindarc]]

Attachment: bug1265441.patch

Comment 5

[Justin D'Arcangelo [:justindarc]]

Attachment: bug1265441.patch

Forgot to make the system add-on conditional depending on release channel. This version should fix that.

Comment 6

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

Comment on attachment 8751494 [details] [diff] [review]
bug1265441.patch

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

Just some little things to deal with here, and I'll want to take one more look at it, but this is almost done.

Good work!

::: browser/extensions/flyweb/bootstrap.js
@@ +34,5 @@
> +  constructor(aWindow) {
> +    this._discoveryManager = new aWindow.FlyWebDiscoveryManager();
> +  }
> +
> +  start(callback) {

If there's already some discovery underway, are we okay with us just swapping in a new callback? Or should we throw if we're still in the process of discovering? Or should it be a no-op?

@@ +69,5 @@
> +    this._callback(services);
> +  }
> +}
> +
> +let discoveryManagerInstance;

This is a global non-class object - we tend to prefix those with "g" to make them easier to notice in the code. So maybe let's make this:

gDiscoveryManagerInstance

@@ +124,5 @@
> +            getInterface(Ci.nsIDOMWindowUtils).removeSheet(this._sheetURI, 1);
> +      },
> +
> +      onViewShowing(aEvent) {
> +        let doc = aEvent.target.ownerDocument;

Should we not try to create the discoveryManagerInstance here in onViewShowing, where it's about to be used?

@@ +148,5 @@
> +
> +          items.appendChild(fragment);
> +
> +          if (services.length > 0) {
> +            empty.setAttribute("hidden", true);

Pretty sure you can take advantage of https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Attribute/hidden and just set:

empty.hidden = true;

Then you don't need the [empty] rule in the CSS either.

@@ +154,5 @@
> +            empty.removeAttribute("hidden");
> +          }
> +        });
> +      },
> +      

Trailing ws

@@ +156,5 @@
> +        });
> +      },
> +      
> +      onViewHiding(aEvent) {
> +        discoveryManagerInstance.stop();

And set it to null? Or is there a good reason to keep it around?

::: browser/extensions/flyweb/install.rdf.in
@@ +11,5 @@
> +  <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>

Don't forget to set the <em:multiprocessCompatible>true</em:multiprocessCompatible> flag as well - that way, you don't get performance-harming shims.

::: browser/extensions/flyweb/skin/shared/flyweb.css
@@ +8,5 @@
> +  padding: 10px 20px;
> +  text-align: center;
> +}
> +
> +#flyweb-items-empty[hidden] {

I don't think you need this - see my other comment about https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Attribute/hidden

Comment 7

[Justin D'Arcangelo [:justindarc]]

Attachment: bug1265441.patch

Mike,

Thanks for all your input, especially with the naming conventions and some of the XUL stuff!

I addressed all your comments from the last version of the patch except for the handling of multiple callbacks in the `DiscoveryManager` and nullifying the `gDiscoveryManagerInstance`. The reason being that since the widget is a singleton, it should be impossible for a 2nd callback to get registered while another is in progress. Unless there is a scenario I am unaware of where this would be possible, I think we should be safe here.

I'm also going to keep the `gDiscoveryManagerInstance` around when the view closes so that we retain our `FlyWebDiscoveryManager` and have instant results on subsequent opens.

Comment 8

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

Comment on attachment 8751545 [details] [diff] [review]
bug1265441.patch

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

LGTM! Thanks justindarc.

Comment 9

[Justin D'Arcangelo [:justindarc]]

https://hg.mozilla.org/projects/larch/rev/00ed525d2d34984bed2a03c63bcc7d3d1afa133d
Bug 1265441 - [FlyWeb] Move FlyWeb UI implementation for Desktop to system add-on;r=mconley