980503 - Implement front end for web audio editor

Status: RESOLVED FIXED

(DevTools Graveyard :: Web Audio Editor, defect)

Comment 1

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 980503-webaudio-editor-first-pass.patch

This is the first pass with a working panel and basic functionality. Few more things will need to be added before a prototype release however, this is just to get code in there. This is dependent on the actor patch (bug 980502) to be landed, so this won't work alone.

Comment 2

[Jordan Santell [:jsantell] [@jsantell]]

Also, to CYA, D3 and Dagre-D3 are in this patch with the following licenses:

dagre-d3: https://github.com/cpettitt/dagre-d3/blob/master/LICENSE (MIT)
d3: https://github.com/mbostock/d3/blob/master/LICENSE (3 clause BSD?)

Pinging Rob and dcamp to double check if this is cool.

Comment 3

[Victor Porof [:vporof][:vp]]

O_O

Comment 4

[Rob Campbell [:rc] (:robcee)]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #2)
> Also, to CYA, D3 and Dagre-D3 are in this patch with the following licenses:
> 
> dagre-d3: https://github.com/cpettitt/dagre-d3/blob/master/LICENSE (MIT)
> d3: https://github.com/mbostock/d3/blob/master/LICENSE (3 clause BSD?)
> 
> Pinging Rob and dcamp to double check if this is cool.

BSD and MIT are generally compatible with MPL. Include the license files. We can ping legal for a review if we're at all worried.

Comment 5

[Victor Porof [:vporof][:vp]]

Comment on attachment 8387775 [details] [diff] [review]
980503-webaudio-editor-first-pass.patch

I'm concerned about shipping so much third party code, because:

- Shipping *minified* third party libraries is not something I'd feel comfortable with, because this cripples our ability to debug it and track down potential problems in the future. I don't we've ever shipped minified code, in devtools at least.

- Unminified D3 + Dagre-D3 would end up being 400K; that's a lot of code, and we should at least ask a module owner how they feel about this. They might not be happy about Firefox download sizes increasing a fair bit for a tool that's awesome, but disabled by default.

- When it comes to manipulating SVG, we have tons of libraries out there to choose from, and if we *do* make the decision that shipping so much third party code is ok, we need to be sure we picked the right library, especially by considering our future needs when it comes to building graphical widgets (like memory timelines, profiler flamecharts, network activity graphs etc.). Are we sure D3 is the best solution for our current and future implementation needs? What about http://snapsvg.io/ ?

- Lastly, how hard would it be to actually implement the UI for nodes and arcs manually, in plain old SVG directly? Since you're probably not using even 10% of D3's capabilities, would it be tedious and difficult to write a graphing library, specially tailored for our needs?

Adding Rob as an additional reviewer, and needinfoing Gavin about shipping the third party code.

Comment 6

[Jordan Santell [:jsantell] [@jsantell]]

In terms of writing something specifically for this without D3, that's doable. Dagre-d3 uses Dagre to get the positioning of nodes in a DAG (rendering agnostic), which I think we should continue to use rather than reinventing the wheel, but the rendering we could roll our own. I agree, D3 shouldn't be used just for this, but if this is something devtools would like to use for other components, then maybe something to consider.

If there is a library that would be used throughout devtools, I think you have the best judgement, Victor. I spent a lot of time looking at all the options, and the most flexible and surprisingly smallest was D3, but you are correct that this component is using less than 10% of its features.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

400K before or after omni.ja compression? I wouldn't worry too much about it either way.

You should loop in Gerv to sign off on the licensing, but BSD/MIT are easy.

Comment 8

[Rob Campbell [:rc] (:robcee)]

(In reply to :Gavin Sharp (email [e-mail]) from comment #7)
> 400K before or after omni.ja compression? I wouldn't worry too much about it
> either way.
> 
> You should loop in Gerv to sign off on the licensing, but BSD/MIT are easy.

yeah, that's been my experience, but will ask gerv for a license review.

If filesize is not a huge concern we can go ahead with D3 as a stopgap. I like that we DAGRE is an agnostic graph layout tool.

Jordan, can we get this rebased? I'm getting a ton of hunk failures applying this patch.

thanks!

Comment 9

[Victor Porof [:vporof][:vp]]

Comment on attachment 8387775 [details] [diff] [review]
980503-webaudio-editor-first-pass.patch

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

Ask me again for review after addressing all comments.

Replace all libraries with the un-minified versions.

::: browser/devtools/webaudioeditor/test/head.js
@@ +1,2 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */

This file seems to be in both the backend and frontend patches?

::: browser/devtools/webaudioeditor/webaudioeditor-controller.js
@@ +15,5 @@
> +const { defer, all } = Cu.import("resource://gre/modules/Promise.jsm", {}).Promise;
> +Promise.defer = defer;
> +Promise.all = all;
> +
> +const require = Cu.import("resource://gre/modules/devtools/Loader.jsm", {}).devtools.require;

You're importing Loader.jsm twice.

@@ +18,5 @@
> +
> +const require = Cu.import("resource://gre/modules/devtools/Loader.jsm", {}).devtools.require;
> +const EventEmitter = require("devtools/toolkit/event-emitter");
> +const STRINGS_URI = "chrome://browser/locale/devtools/webaudioeditor.properties"
> +console = Cu.import("resource://gre/modules/devtools/Console.jsm").console;

We already have the console object in all jsms afaik.

@@ +22,5 @@
> +console = Cu.import("resource://gre/modules/devtools/Console.jsm").console;
> +
> +// The panel's window global is an EventEmitter firing the following events:
> +const EVENTS = {
> +  // When new programs are received from the server.

"Programs"?

@@ +23,5 @@
> +
> +// The panel's window global is an EventEmitter firing the following events:
> +const EVENTS = {
> +  // When new programs are received from the server.
> +  START_CONTEXT: "WebAudioEditor:StartContext",

What exactly is "start context"? OP please explain.

@@ +35,5 @@
> +  CHANGE_PARAM: "WebAudioEditor:ChangeParam",
> +
> +  // When the UI is reset from tab navigation.
> +  UI_RESET: "WebAudioEditor:UIReset",
> +  // When a param has been changed via the UI and successfully

Nit: add a newline after UI_RESET.

@@ +41,5 @@
> +  UI_SET_PARAM: "WebAudioEditor:UISetParam",
> +
> +  // When an audio node is added to the list pane
> +  UI_ADD_NODE_LIST: "WebAudioEditor:UIAddNodeList",
> +  

Nit: trailing whitespace.

@@ +55,5 @@
> +/**
> + * Track an array of audio nodes
> + */
> +let graphNodes = [];
> +let graphEdges = [];

Would it make more sense for graphEdges to be a WeakMap?

@@ +57,5 @@
> + */
> +let graphNodes = [];
> +let graphEdges = [];
> +
> +function createGraphNode (actor) {

Nit: please document this function.

@@ +70,5 @@
> +      window.emit(EVENTS.CREATE_NODE, actor);
> +    });
> +}
> +
> +function createGraphEdge (sourceActor, destActor) {

Ditto.

@@ +73,5 @@
> +
> +function createGraphEdge (sourceActor, destActor) {
> +  let source = actorToGraphNode(sourceActor);
> +  let dest = actorToGraphNode(destActor);
> +  graphEdges.push({ source: source, target: dest });

Wouldn't it be consistent to emit an CONNECT_NODE event here?
Do you handle the case when you're adding the same edge twice? WeakMaps would probably solve this problem more easily?

@@ +76,5 @@
> +  let dest = actorToGraphNode(destActor);
> +  graphEdges.push({ source: source, target: dest });
> +}
> +
> +function removeGraphEdge (sourceActor) {

Ditto.

It'd be nicer if these 3 functions were part of an object literal, like AudioContextGraph or something.

@@ +149,5 @@
> +   * Handles a host change event on the parent toolbox.
> +   */
> +  _onHostChanged: function() {
> +    if (gToolbox.hostType == "side") {
> +      $("#shaders-pane").removeAttribute("height");

Nope.

@@ +175,5 @@
> +        }).then(() => window.emit(EVENTS.UI_RESET));
> +        break;
> +      }
> +      case "navigate": {
> +        // Case of bfcache, probably TODO

File a bug, XXX, add the bug number etc.

@@ +232,5 @@
> +      WebAudioGraphView.refresh();
> +      window.emit(EVENTS.CONNECT_NODE, sourceActor, destActor);
> +    });
> +  },
> +  

Nit: trailing whitespace.

@@ +241,5 @@
> +    removeGraphEdge(nodeActor);
> +    WebAudioGraphView.refresh();
> +    window.emit(EVENTS.DISCONNECT_NODE, nodeActor);
> +  },
> +  

Nit: trailing whitespace.

@@ +269,5 @@
> +function equalActors (actor1, actor2) {
> +  return actor1.actorID === actor2.actorID;
> +}
> +
> +function actorToGraphNode (actor) {

Document this, maybe make it part to the aforementioned AudioContextGraph?

@@ +277,5 @@
> +  }
> +  return null;
> +}
> +
> +function getGraphNodeById (id) {

Ditto.

@@ +284,5 @@
> +
> +/**
> + * TODO: Use as of now, unlanded Task.async
> + */
> +function async (fn) {

This isn't really async though, is it? Use DevToolsUtils.waitForTick after bug 917226?

::: browser/devtools/webaudioeditor/webaudioeditor-view.js
@@ +8,5 @@
> +const { debounce } = require("sdk/lang/functional");
> +
> +// Globals for d3 stuff
> +const WIDTH = 1000;
> +const HEIGHT = 400;

What are these? Pixels? Why the fixed size? How does this cope with a side host? Add a small comment explaining this.

@@ +24,5 @@
> +
> +const NODE_PROPERTIES = {
> +  "OscillatorNode": {
> +    "type": {
> +      "type": "string"

Are all of these used to convert the data sent by the backend as strings? If so, as I said in the backend review, you should *really* use grips instead of manually typing all of this, if at all possible.

@@ +98,5 @@
> +/**
> + * Takes a `graphNode` (has `actor`, `id` and `type`) and returns
> + * a hash of
> + */
> +function getNodeParams (graphNode) {

Shouldn't this be in the controller, part of a AudioContextGraph object?

@@ +157,5 @@
> +    $("#content").hidden = false;
> +    this.refresh();
> +  },
> +
> +  refresh: function () {

Add a comment documenting why refresh is needed and when it's called.
Why do we need both refresh and draw?

@@ +161,5 @@
> +  refresh: function () {
> +    this.draw();
> +  },
> +
> +  resetGraph: function () {

Ditto.

@@ +165,5 @@
> +  resetGraph: function () {
> +    $("#graph-target").innerHTML = "";
> +  },
> +
> +  focusNode: function (actorID) {

Ditto?

@@ +172,5 @@
> +    // Add to "selected"
> +    this._getNodeByID(actorID).classList.add("selected");
> +  },
> +
> +  blurNode: function (actorID) {

Ditto.

@@ +195,5 @@
> +  _onGraphNodeClick: function (graphNode) {
> +    WebAudioParamView.focusNode(graphNode.id);
> +  },
> +
> +  draw: function () {

Document this please. What data does this assume is available?

Why is draw under Event Handlers?

@@ +241,5 @@
> +
> +    // Override Dagre-d3's post render function by passing in our own.
> +    // This way we can leave styles out of it.
> +    renderer.postRender(function (graph, root) {
> +      // TODO change arrowhead color depending on theme-dark/theme-light

File a bug or fix it here. If this is followup material, XXX it and add the bug number here.

@@ +254,5 @@
> +          .attr("refX", 8)
> +          .attr("refY", 5)
> +          .attr("markerUnits", "strokewidth")
> +          .attr("markerWidth", 8)
> +          .attr("markerHeight", 5)

Make all the magic numbers here constants, and add a comment for each describing what they are.

@@ +284,5 @@
> +
> +let WebAudioParamView = {
> +  _paramsView: null,
> +
> +  initialize: function () {

"Initialization function, called when the tool is started."

@@ +287,5 @@
> +
> +  initialize: function () {
> +    let paramsView = this._paramsView = new VariablesView($("#web-audio-inspector-content"),
> +      Heritage.extend(GENERIC_VARIABLES_VIEW_SETTINGS, {
> +        emptyText: "No audio nodes"

This needs to be localized.

@@ +294,5 @@
> +    this.addNode = this.addNode.bind(this);
> +    window.on(EVENTS.CREATE_NODE, this.addNode);
> +  },
> +
> +  destroy: function() {

"Destruction function, called when the tool is closed."

@@ +298,5 @@
> +  destroy: function() {
> +    window.off(EVENTS.CREATE_NODE, this.addNode);
> +  },
> +
> +  resetUI: function () {

Document this please.

@@ +307,5 @@
> +   * Executed when an audio param is changed in the UI.
> +   */
> +  _onEval: async(function* (variable, value) {
> +    let ownerScope = variable.ownerView;
> +    let node = getGraphNodeById(ownerScope._id);

Accessing private properties here. You should make the id public, or have a gettter.

@@ +319,5 @@
> +      window.emit(EVENTS.UI_SET_PARAM_ERROR, node.id, propName, value);
> +    }
> +  }),
> +
> +  getScopeByID: function (id) {

Please document all methods.

@@ +343,5 @@
> +
> +    // Get actorID
> +    let match = $el.parentNode.id.match(/\(([^\)]*)\)/);
> +    let id;
> +    if (match && match.length === 2)

Why "2"?

@@ +391,5 @@
> +    window.emit(EVENTS.UI_ADD_NODE_LIST, actor.actorID);
> +  }),
> +
> +  removeNode: async(function* (graphNode) {
> +

There's nothing happening here. Followup or fix.

@@ +404,5 @@
> + * @param String type
> + *        The datatype to casat `value` to.
> + * @return Mixed
> + */
> +function cast (value, type) {

This is really really ugly. Object grips are the bee's knees.

Comment 10

[Dave Camp (:dcamp)]

Looks like you don't need any info from me, reping if I'm wrong.

Comment 11

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 980503-webaudio-editor-first-pass.patch

Made a large refactoring and addressed your previous comments (if not, I reasoned below)


* `console` is not imported unless explicitly in this case
* For the giant list of NODE_PROPERTIES, I removed the type from there since casting is no longer necessary, but left the structure there for additional controls on the front end, like which parameters should be readonly, and other future constraints. The list of properties could also be moved to the actor (audionode.getParams() -> ['threshold', 'knee', 'ratio', …]) but would still need custom properties for the front end somewhere
* Lots of documentation added, and reference bugs
* Changed how the view manages created nodes with an array (and weak map for edges) using wrapper instances

https://tbpl.mozilla.org/?tree=Try&rev=64fe7c39f19a

Comment 12

[Victor Porof [:vporof][:vp]]

Comment on attachment 8404207 [details] [diff] [review]
980503-webaudio-editor-first-pass.patch

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

r+ with comments addressed.

::: browser/devtools/webaudioeditor/test/browser_wa_graph_mouseover.js
@@ +22,5 @@
> +
> +  let $items = $$(".variables-view-scope");
> +  let $graphNodes = $$(".nodes > g");
> +
> +  Array.prototype.forEach.call($items, $item => {

> for (let item of items)
is much simpler. It also works! I'm just letting you know. I won't r- a fine patch like this one because of Array.prototype.forEach.call(thingsWithDollarsInTheirNames).

(...ps: the fact that "it works" is much more important than "it's simpler"; many simple things don't work, e.g. it's easy to type $ tar -xzf mypatcheswritteninthenextfewyears.gz but that magical archive doesn't exist, yet, unfortunately; it's all good though, I enjoy doing this, sometimes recursively)

@@ +23,5 @@
> +  let $items = $$(".variables-view-scope");
> +  let $graphNodes = $$(".nodes > g");
> +
> +  Array.prototype.forEach.call($items, $item => {
> +    info($item.id + " GOOO");

GOOO!!!!

@@ +27,5 @@
> +    info($item.id + " GOOO");
> +    mouseOver(panel.panelWin, $(".devtools-toolbar", $item));
> +    // Get actorID from id of variable scope
> +    let id = $item.id.match(/\(([^\)]*)\)/)[1];
> +  

Nit: trailing whitespace.

@@ +29,5 @@
> +    // Get actorID from id of variable scope
> +    let id = $item.id.match(/\(([^\)]*)\)/)[1];
> +  
> +    // Go over all graph nodes and check only the selected one is highlighted
> +    Array.prototype.forEach.call($graphNodes, $node => {

for .. of

::: browser/devtools/webaudioeditor/test/head.js
@@ +79,5 @@
>    ok(false, "Got an error: " + aError.message + "\n" + aError.stack);
>    finish();
>  }
>  
> +function createCanvas() {

Even though I love canvases, this function is not needed.

@@ +109,5 @@
> +// Hack around `once`, as that only resolves to a single (first) argument
> +// and discards the rest. `onceSpread` is similar, except resolves to an
> +// array of all of the arguments in the handler. These should be consolidated
> +// into the same function, but many tests will need to be changed.
> +function onceSpread(aTarget, aEvent) {

Neither this, or I'm missing something.

@@ +115,5 @@
> +  aTarget.once(aEvent, (...args) => deferred.resolve(args));
> +  return deferred.promise;
> +}
> +
> +function observe(aNotificationName, aOwnsWeak = false) {

Ditto.

@@ +183,5 @@
> +    removeTab(aPanel.target.tab)
> +  ]).then(() => {
> +    let gBrowser = window.gBrowser;
> +    while (gBrowser.tabs.length > 1) {
> +      gBrowser.removeCurrentTab();

Why is this needed? I think it should be the test's responsibility to remove specific tabs not added by initWebAudioEditor, but I'm ok with this as long as there's a good reason for it. Add a comment if so.

@@ +222,5 @@
>  function getNSpread (front, eventName, count) { return getN(front, eventName, count, true); }
> +
> +/**
> + * Waits for the UI_GRAPH_RENDERED event to fire, but only
> + * resolves when the graph was rendered with the correct count of 

Nit: trailing whitespace.

@@ +227,5 @@
> + * nodes and edges.
> + */
> +function waitForGraphRendered (front, nodeCount, edgeCount) {
> +  let deferred = Promise.defer();
> +  let eventName = "WebAudioEditor:UIGraphRendered";

Should really use the EVENTS object, since the name might change (it will never happen, I know, I'm just nitpicking).

@@ +255,5 @@
> +  let scope = view.getScopeAtIndex(index);
> +  let aVar = scope.get(prop);
> +  scope.expand();
> +
> +  executeSoon(() => {

Please document the necessity for this executeSoon(). Are you waiting for properties/variables to be fetched? If so, you should NOT use executeSoon, but wait for an event to fire, a promise to resolve, or half-life 3 to be released.

@@ +311,5 @@
> +  EventUtils.sendMouseEvent({ type: "mouseover" }, element, win);
> +}
> +
> +
> +const NODE_PROPERTIES = {

FWIW, you could probably access these in the tests via panelObject.panelWin.NODE_PROPERTIES, to avoid duplication.

::: browser/devtools/webaudioeditor/webaudioeditor-controller.js
@@ +11,5 @@
> +
> +// Override DOM promises with Promise.jsm helpers
> +const { defer, all } = Cu.import("resource://gre/modules/Promise.jsm", {}).Promise;
> +Promise.defer = defer;
> +Promise.all = all;

Ah, this brings back memories of the good old "patch things onto global objects or prototypes what could go wrong this web thing is awesome!! <3".

I would advise against this, and just having `defer` and `all` as globals. But it's your call. Get it? "your call".

@@ +167,5 @@
> +  let params = Object.keys(NODE_PROPERTIES[this.type] || {});
> +
> +  // Fetch an array of objects containing `param` and `value` properties
> +  return Promise.all(
> +    params.map(param => this.actor.getParam(param).then(val => ({ param: param, value: val })))

To avoid too many needless roundTrips, you could have a getParams request that returns everything in a single packet.

::: browser/devtools/webaudioeditor/webaudioeditor-view.js
@@ +4,5 @@
> +"use strict";
> +
> +Cu.import("resource:///modules/devtools/VariablesView.jsm");
> +Cu.import("resource:///modules/devtools/VariablesViewController.jsm");
> +const { debounce } = require("sdk/lang/functional");

TIL.

::: browser/devtools/webaudioeditor/webaudioeditor.xul
@@ +41,5 @@
> +          align="center"
> +          pack="center"
> +          flex="1"
> +          hidden="true">
> +      <label id="requests-menu-waiting-notice-label"

What is a "requests-menu" in this context?

@@ +65,5 @@
> +            <g id="graph-target" transform="translate(20,20)"/>
> +          </svg>
> +          <!--<label id="graph-label"
> +                 class="plain editor-label"
> +                 value="&webAudioEditorUI.audioContextGraph;"/>-->

Why is this commented out?
Add a comment if there's some sort of deeper meaning to it.

::: browser/locales/en-US/chrome/browser/devtools/webaudioeditor.dtd
@@ +10,5 @@
> +  - You want to make that choice consistent across the developer tools.
> +  - A good criteria is the language in which you'd find the best
> +  - documentation on web development on the web. -->
> +
> +<!-- LOCALIZATION NOTE (webAudioEditorUI.audioNodeINspector): This is the label for

audioNodeINspector or audioNodeInspector?

::: browser/themes/shared/devtools/webaudioeditor.inc.css
@@ +121,5 @@
> +
> +/* Responsive sidebar */
> +
> +@media (max-width: 700px) {
> +  #shaders-pane {

Shaders pane?
It looks like you need to remove some code.

Comment 13

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 980503-webaudio-editor-first-pass.patch

Fixed the remaining nits, and moved the node properties to the actor's getParams(), which returns prop names, values, and flags (readonly, is a buffer, etc), with tests for it as well, looking pretty solid.

Comment 14

[Victor Porof [:vporof][:vp]]

<3

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/25a5c2eb571c