Jan Keromnes [:janx] Assignee
	Description • 10 years ago

Firefox OS developers would greatly benefit from opt-in debug information shown on-device as overlays or widgets. Good examples are visual error/warning feedback for apps, reflow counters, memory usage, and maybe even a small console in the dropdown menu.

	Paul Rouget [:paul]
	Comment 1 • 10 years ago

Jan, can you attach some screenshots?

	Jan Keromnes [:janx] Assignee
	Comment 2 • 10 years ago

Paul, here is a "manual screenshot" of my prototype in action (manual because my keon doesn't have a memory card).

	Jan Keromnes [:janx] Assignee
	Comment 3 • 10 years ago

The screenshot is from a debug app I wrote, and the colored counters are a debug overlay shown over every app (will be configurable):

- grey means total memory per app (using past's memory actor in bug#923275),
- purple means reflows,
- yellow means warning (absent in this screenshot because there are none),
- red means errors.

	Jan Keromnes [:janx] Assignee
	Comment 4 • 10 years ago

This patch makes the SystemApp listen for the `'message'` event on its window. When a message[1] is posted, the `display()` method is called, creating a devtools overlay inside an app's `<div>` and over its iframe, populated with widgets.

[1] A message is formatted like so: `{manifestURL: 'xxx', widgets: [{color:'red',value:5}]}`

	Jan Keromnes [:janx] Assignee
	Comment 5 • 10 years ago

Note: The Gaia patch also adds a "Show debug info for Apps" setting toggler in the sleep menu (the "Developer menu" needs to be enabled in the developer settings). It is similar to the "Enable APZ for Apps" toggler introduced by vingtetun, but it needs a better place to live in (maybe inside a new shiny developer menu that makes sense).

	Jan Keromnes [:janx] Assignee
	Comment 6 • 10 years ago

This patch introduces the GaiaDevtools object, which can be asked to watch specific apps and show debug information for them.

Pending ways to configure it, it automatically listens for the opening and closing of apps to show info for all of them.

This patch includes past's memory actor patch from bug#923275, and uses it to display total memory footprint of apps.

Caveats:
- Right now, it initializes only after 20 seconds. This was to prevent a race condition but will soon be removed.
- It doesn't care yet about the "app.debug_info" setting introduces in the Gaia patch.
- I think there is a bug in `watchApps`, because since I rebased last week, I only receive "appOpen" and "appClose" events for a seemingly random subset of apps (in my last try, Clock, Settings, Emails, Calendar and Messages didn't trigger "appOpen" or "appClose", but "Marketplace" and my app "Quirks" did). Thus the excessive logs.

	Jan Keromnes [:janx] Assignee
	Comment 7 • 10 years ago

TODO:
- Remove 20s init timeout.
- Figure out why "appOpen"/"appClose" are often not triggered.
- Figure out why "console.{log,error,warn}" are never counted.
- Configure which apps are watched on what types of events in a JSON string pref.
- Migrate existing debug overlays to this one (Grid, FPS, Time to Load, ...).
- Add tests.

	Jan Keromnes [:janx] Assignee
	Comment 8 • 10 years ago

- Fixed init sequence.
- Fixed console.{warn,error}.
- The lack of "appOpen"/"appClosed" for certified apps was not a bug (need to set "devtools.debugger.forbid-certified-apps" to false to see them).

	Paul Rouget [:paul]
	Comment 9 • 10 years ago

Can you remove the memory profiler patch from your patch and make this bug depends on bug 923275?

	Alexandre Poirot [:ochameau]
	Comment 10 • 10 years ago

Comment on attachment 8362493 [details] [diff] [review]
Gecko patch

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

You are beginning with quite big patch we a good overview of the whole stack, good job, keep going :)

::: b2g/chrome/content/devtools.js
@@ +70,5 @@
> +      let data = {manifestURL: app.manifest, widgets: []};
> +      if (app.memory > 0) data.widgets.push({color: 'grey', value: Math.round(app.memory/1024) + ' KB'});
> +      if (app.reflows > 0) data.widgets.push({color: 'purple', value: app.reflows});
> +      if (app.warnings > 0) data.widgets.push({color: 'orange', value: app.warnings});
> +      if (app.errors > 0) data.widgets.push({color: 'red', value: app.errors});

paul is going to confirm, but I'm not sure we accept one liner like this in gecko codebase.
We have to add braces, even for one-line statements

@@ +72,5 @@
> +      if (app.reflows > 0) data.widgets.push({color: 'purple', value: app.reflows});
> +      if (app.warnings > 0) data.widgets.push({color: 'orange', value: app.warnings});
> +      if (app.errors > 0) data.widgets.push({color: 'red', value: app.errors});
> +
> +      document.querySelector('#systemapp').contentWindow.postMessage(data, '*');

Unless someone from gaia told you to use postMessage,
the common way to send non-standard chrome messages to the system app is to use mozChromeEvent.

Something like that:
  shell.sendChromeEvent({type: 'devtools-event', widgets: [...]})
And from gaia:
  window.addEventListener('mozChromeEvent', function mozAppReady(event) {
    if (event.detail.type != 'devtools-event')
      return;
    ...
  });

@@ +88,5 @@
> +          this._client.addListener('logMessage', this.listener.bind(this));
> +          this._client.addListener('pageError', this.listener.bind(this));
> +          this._client.addListener('consoleAPICall', this.listener.bind(this));
> +          this._client.addListener('reflowActivity', this.listener.bind(this));
> +          this._client.request({to: this._webappsActor, type: 'watchApps'}, (res) => {

I'm wondering if it is really usefull to query the webapps actor to retrieve app actors.
It feels weird for me, as such script, living next to shell.js, can listen for appOpen/appClose
events being dispatched by the system app.

One option could be to factor out its _connectToApp function:
  http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/webapps.js#792
In order to create the app actor by ourself. We may have to do that to support Firefox/Fennec apps, the watchApps and getAppActor will most likely be factored out to better support multiple platforms.
Then we would listen to appWillOpen/appClose, like what webapps actors does, but without being limited by the forbid-certified-apps pref.
But at the end, it also looks like we are duplicating the actor code... so it may be better to keep current way of doing.

Does that make any sense?!

@@ +102,5 @@
> +            });
> +          });
> +          this.log('importing Loader.jsm and MemoryFront');
> +          const {devtools} = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});
> +          this._MemoryFront = devtools.require("devtools/server/actors/memory").MemoryFront;

What about exposing MemoryFront like DebuggerClient with a lazy getter?

@@ +229,5 @@
> +  };
> +
> +  /*navigator.mozSettings.addObserver("app.debug_info", function(setting) {
> +    if (setting.settingValue) GaiaDevtools.init();
> +  });*/

I think what you want is something like this:
  http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/settings.js#634
And ideally, we would only load this devtools.js file if the setting is true,
instead of always loading it from shell.html

	Paul Rouget [:paul]
	Comment 11 • 10 years ago

Comment on attachment 8362110 [details] [diff] [review]
Gaia patch, adds a window post message API to display colored widgets over apps.

>diff --git a/apps/system/style/window.css b/apps/system/style/window.css
>+.appWindow > .devtools-overlay > .widget {
>+  font-size: 24px;

Make it much smaller.

>diff --git a/build/config/common-settings.json b/build/config/common-settings.json
>index ad1bf51..1842665 100644
>--- a/build/config/common-settings.json
>+++ b/build/config/common-settings.json
>@@ -6,6 +6,7 @@
>    "app.launch_path.blacklist": [],
>    "app.reportCrashes": "ask",
>    "app.update.interval": 86400,
>+   "app.debug_info": false,

don't use "debug", but something that includes the string "devtools".

Let's see how that evolve once you figure an API for the shell.html-level mechanism.

	Paul Rouget [:paul]
	Comment 12 • 10 years ago

>+++ b/b2g/chrome/content/devtools.js
>@@ -0,0 +1,247 @@
>+XPCOMUtils.defineLazyGetter(this, 'DebuggerClient', function() {
>+  return Cu.import('resource://gre/modules/devtools/dbg-client.jsm', {}).DebuggerClient;
>+});
>+
>+(function(window) {

I don't think you need to isolate GaiaDevtools.
And rename GaiaDevtools to something more sensible (DevtoolsWatcher maybe?)

>+
>+  let GaiaDevtools = {
>+
>+    _client: null,
>+    _webappsActor: null,
>+    _MemoryFront: null,
>+    _apps: {byActor: {}, byManifest: {}},

Use one Map(). And iterate over the map to find the manifest or actor (then you won't have to sync 2 objects).

>+    _watchAll: true,
>+
>+    log: function(message) {
>+      dump('GaiaDevtools: ' + message + '\n');
>+    },

Use a global const: `const DEVTOOLS_LOG_PREFIX=...`.

>+    listener: function(type, packet) {
>+
>+      let app = this._apps.byActor[packet.from];
>+      let output = '';
>+
>+      //this.log('listener received ' + packet.type);
>+
>+      switch (packet.type) {
>+
>+        case 'pageError':
>+          if (packet.pageError.warning || packet.pageError.strict) {
>+            app.warnings++;
>+            output = 'warning (';
>+          } else {
>+            app.errors++;
>+            output += 'error (';
>+          }
>+          let {errorMessage, category, lineNumber, columnNumber} = packet.pageError;
>+          output += category + '): ' + errorMessage + ':' + lineNumber + ':' + columnNumber;
>+          break;
>+
>+        case 'consoleAPICall':
>+          switch (packet.message.level) {
>+            case 'error':
>+              app.errors++;
>+              output = 'error (console)';
>+              break;
>+            case 'warn':
>+              app.warnings++;
>+              output = 'warning (console)';
>+              break;
>+          }
>+          break;
>+
>+        case 'reflowActivity':
>+          app.reflows++;
>+          let {start, end, sourceURL} = packet;
>+          let duration = Math.round((end - start) * 100) / 100;
>+          let functionName = packet.functionName || "";
>+          output = 'reflow: ' + duration + 'ms';
>+          if (sourceURL) {
>+            output += ' ' + formatSourceURL(packet);
>+          }
>+          break;
>+      }
>+
>+      this.log(output);
>+      this.update(app);
>+    },
>+
>+    update: function(app) {
>+      let data = {manifestURL: app.manifest, widgets: []};
>+      if (app.memory > 0) data.widgets.push({color: 'grey', value: Math.round(app.memory/1024) + ' KB'});
>+      if (app.reflows > 0) data.widgets.push({color: 'purple', value: app.reflows});
>+      if (app.warnings > 0) data.widgets.push({color: 'orange', value: app.warnings});
>+      if (app.errors > 0) data.widgets.push({color: 'red', value: app.errors});
>+
>+      document.querySelector('#systemapp').contentWindow.postMessage(data, '*');
>+    },
>+
>+    init: function(callback) {
>+      if (!DebuggerServer.initialized) RemoteDebugger.start();
>+      this._client = new DebuggerClient(DebuggerServer.connectPipe());
>+      this.log('connecting client...');
>+      this._client.connect((type, traits) => {
>+        this._client.listTabs((res) => {
>+          this.log('listTabs replied ' + JSON.stringify(res));
>+          this._webappsActor = res.webappsActor;
>+          this.log('adding listeners for logMessage, pageError, consoleAPICall, reflowActivity');
>+          this._client.addListener('logMessage', this.listener.bind(this));
>+          this._client.addListener('pageError', this.listener.bind(this));
>+          this._client.addListener('consoleAPICall', this.listener.bind(this));
>+          this._client.addListener('reflowActivity', this.listener.bind(this));
>+          this._client.request({to: this._webappsActor, type: 'watchApps'}, (res) => {
>+            this.log('watchApps replied ' + JSON.stringify(res));
>+            this.log('adding listeners for appOpen, appClose');
>+            this._client.addListener('appOpen', (type, { manifestURL }) => {
>+              this.log('appOpen ' + type + ' ' + manifestURL);
>+              if (this._watchAll) this.watch(manifestURL);
>+            });
>+            this._client.addListener('appClose', (type, { manifestURL }) => {
>+              this.log('appClose ' + type + ' ' + manifestURL);
>+              this.unwatch(manifestURL);
>+            });
>+          });
>+          this.log('importing Loader.jsm and MemoryFront');
>+          const {devtools} = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});
>+          this._MemoryFront = devtools.require("devtools/server/actors/memory").MemoryFront;
>+          if (callback) callback();
>+        });
>+      });
>+      this.log('end of init()');
>+    },
>+
>+    list: function(callback) {
>+      this._client.request({to: this._webappsActor, type: "listRunningApps"}, callback);
>+    },
>+
>+    watch: function(manifestURL) {
>+      this.log('request to watch ' + manifestURL);
>+      if (this._apps.byManifest[manifestURL]) return;
>+      this.log('getting app actor');
>+      this._client.request({to: this._webappsActor, type: 'getAppActor', manifestURL: manifestURL }, (res) => {
>+        if (res.error) {
>+          this.log('watch error ' + manifestURL + ' ' + res.error);
>+          return;
>+        }
>+        let actor = res.actor.consoleActor;
>+        let memoryFront = this._MemoryFront(this._client, res.actor);
>+        this.log('starting app listeners for LogMessage, PageError, ConsoleAPI, ReflowActivity');
>+        this._client.request({
>+          to: actor,
>+          type: 'startListeners',
>+          listeners: ['LogMessage', 'PageError', 'ConsoleAPI', 'ReflowActivity']
>+        }, (res) => {
>+          this.log('app listeners started');
>+          let app = {
>+            manifest: manifestURL,
>+            actor: actor,
>+            errors: 0,
>+            warnings: 0,
>+            reflows: 0,
>+            memory: 0,
>+            timer: null
>+          };
>+          this._apps.byManifest[manifestURL] = app;
>+          this._apps.byActor[actor] = app;
>+          (function measure() {
>+            memoryFront.measure().then((res) => {
>+              app.memory = res.total;
>+              // res.jsObjectsSize
>+              // res.jsStringsSize
>+              // res.jsOtherSize
>+              // res.domSize
>+              // res.styleSize
>+              // res.otherSize
>+              // res.totalSize
>+              // res.jsMilliseconds
>+              // res.nonJSMilliseconds
>+              this.update(app);
>+            }, (err) => {
>+              this.log('memory error ' + JSON.stringify(err));
>+              this.unwatch(app.manifestURL);
>+            });
>+            app.timer = setTimeout(measure, 1000);
>+          })();
>+        });
>+      });
>+      this.log('end of watch()');
>+    },
>+
>+    unwatch: function(manifest) {
>+      this.log('request to unwatch ' + manifest);
>+      let app = this._apps.byManifest[manifest];
>+      if (!app) return;
>+      clearTimeout(app.timer);
>+      this.log('stopping listeners');
>+      this._client.request({
>+        to: app.actor,
>+        type: 'stopListeners',
>+        listeners: ['LogMessage', 'PageError', 'ConsoleAPI', 'ReflowActivity']
>+      }, (res) => {
>+        this.log('app listeners stopped');
>+        delete this._apps.byManifest[manifest];
>+        delete this._apps.byActor[app.actor];
>+      });
>+    },
>+
>+    formatSourceURL: function(packet) {
>+      let {sourceURL, functionName, sourceLine} = packet;
>+
>+      if (sourceURL.substr(0, 5) == "data:") {
>+        let commaIndex = sourceURL.indexOf(",");
>+        if (commaIndex > -1) {
>+          sourceURL = "data:" + sourceURL.substring(commaIndex + 1);
>+        }
>+      }
>+
>+      // Remove any query parameters.
>+      let hookIndex = sourceURL.indexOf("?");
>+      if (hookIndex > -1) {
>+        sourceURL = sourceURL.substring(0, hookIndex);
>+      }
>+
>+      // Remove any hash fragments.
>+      let hashIndex = sourceURL.indexOf("#");
>+      if (hashIndex > -1) {
>+        sourceURL = sourceURL.substring(0, hashIndex);
>+      }
>+
>+      // Remove a trailing "/".
>+      if (sourceURL[sourceURL.length - 1] == "/") {
>+        sourceURL = sourceURL.replace(/\/+$/, "");
>+      }
>+
>+      // Remove all but the last path component.
>+      let slashIndex = sourceURL.lastIndexOf("/");
>+      if (slashIndex > -1) {
>+        sourceURL = sourceURL.substring(slashIndex + 1);
>+      }
>+
>+      // Add function name
>+      sourceURL = 'function ' + (functionName || '<anonymousFunction>') + ', ' + sourceURL;
>+
>+      // Add line number
>+      sourceURL += ' line ' + sourceLine;
>+
>+      return sourceURL;
>+    }
>+
>+  };

Your code is not modular. And too hardly tight to the actor and protocol mechanisms.

Would be great if you could make self-contained widgets mechanisms:

Maybe something like that:

DevtoolsWatch.init = function() {
  this.watch("reflow");
  this.watch("jserrors");
  this.unwatch("reflow");
  this.trackApp("app://clock.gaiamobile.org");
  this.trackApp("*");
  this.untrack("app://clock.gaiamobile.org");
  this.untrack("*");
}

With all the mechanic to use watch/unwatch/track/untrack bound to a setting listener.

Then you would register "watcher" like that:

function ReflowWatcherClass() {
}

ReflowWatcherClass.prototype = {
  supports: function(type) {}, // return true/false if supports "type" (param used in watch/unwatch)
  enable: function(app) {},
}

DevtoolsWatch.registerWatcher(ReflowWacher);

I'm not sure this is the best approach, just some random thoughts.
But the ideal is that at some point, someone could easily add a new widget by simply implementing
the ReflowWatchClass without having to hack the core devtools object.

Keep in mind that there are 2 type of overlays: widgets (reflows) and layers (grid).

>+  /*navigator.mozSettings.addObserver("app.debug_info", function(setting) {
>+    if (setting.settingValue) GaiaDevtools.init();
>+  });*/
>+
>+  Services.obs.addObserver(() => {
>+    GaiaDevtools.log('initializing...');
>+    GaiaDevtools.init(function() {
>+      GaiaDevtools.log('init() complete');
>+    });
>+  }, 'browser-ui-startup-complete', false);
>+
>+})(this);

Listening to a setting sounds better: devtools.devtoolsOverlay = [{app: "*", what: ["memory", "errors", "warning"]}, {app:"app://clock.gaiamobile.org", ["reflow"]}]

	Paul Rouget [:paul]
	Comment 13 • 10 years ago

In my previous comment:

s/ReflowWatcherClass/DevtoolsWatcherClass/

	Jan Keromnes [:janx] Assignee
	Comment 14 • 10 years ago

WIP. Refactored gecko patch to make it more modular. Addressed nits, except coding style and settings.

This patch stops using the Webapps actor to listen for appOpen/appClose, but still uses it to get app actors. Filed #962577 to get this fixed.

	Jan Keromnes [:janx] Assignee
	Comment 15 • 10 years ago

Now uses mozChromeEvents instead of postMessage. CSS unchanged, vingtetun likes the bigger widgets.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 16 • 10 years ago

(In reply to Jan Keromnes [:janx] from comment #15)
> Created attachment 8363691 [details] [diff] [review]
> layers.gaia.diff
> 
> Now uses mozChromeEvents instead of postMessage. CSS unchanged, vingtetun
> likes the bigger widgets.

I won't fight too hard for the size of the widgets but I like big fonts, that's more convenient for my eyes. If it hurts others, maybe the right thing would then to fix the underlying core issue (the error, warning, reflow, lag, etc.. ;))

	Paul Rouget [:paul]
	Comment 17 • 10 years ago

Comment on attachment 8363690 [details] [diff] [review]
layers.gecko.diff

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

This is great. Needs some cleaning (many logs), some tests and figure out how to not flood logcat.

::: b2g/chrome/content/devtools.js
@@ +10,5 @@
> +XPCOMUtils.defineLazyGetter(this, 'MemoryFront', function() {
> +  const {devtools} = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});
> +  return devtools.require("devtools/server/actors/memory").MemoryFront;
> +});
> +

You might want to move that down next to the memory section.

@@ +17,5 @@
> +  WidgetPanel.init(function() {
> +    WidgetPanel.log('init() complete');
> +  });
> +}, 'browser-ui-startup-complete', false);
> +

Are the logs needed?

@@ +22,5 @@
> +
> +// WIDGET PANEL
> +
> +let WidgetPanel = {
> +

This variable will be accessible from shell.html scripts. You might want to rename it to DevtoolsWidgetPanel

@@ +43,5 @@
> +        }
> +        let s = document.querySelector('#systemapp').contentWindow;
> +        s.addEventListener('appwillopen', this);
> +        s.addEventListener('appterminated', this);
> +        if (callback) callback();

I don't think this callback is needed. Is it?

@@ +88,5 @@
> +
> +  _defaultDisplayWidgets: function() {
> +    let data = {manifestURL: this.manifest, widgets: []};
> +    //WidgetPanel.log('displaying ' + this.manifest);
> +    for(let widget of this.widgets.values()) {

> for (

(add a space)

@@ +125,5 @@
> +    client.addListener('logMessage', this.consoleListener.bind(this));
> +    client.addListener('pageError', this.consoleListener.bind(this));
> +    client.addListener('consoleAPICall', this.consoleListener.bind(this));
> +    client.addListener('reflowActivity', this.consoleListener.bind(this));
> +  },

Bind only once:

this.consoleListener = this.consoleListener.bind(this);

@@ +190,5 @@
> +    }
> +
> +    app.display();
> +    //WidgetPanel.log(output);
> +  },

I guess we're dumping way too much data here. logcat is flooded with devtools info. Where/how can we dump these lines?

	Paul Rouget [:paul]
	Comment 18 • 10 years ago

Updated screenshot: http://i.imgur.com/vufp6YS.png

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 19 • 10 years ago

Comment on attachment 8363690 [details] [diff] [review]
layers.gecko.diff

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

One thing I'm unsure about, is the fact that most of the data processing is done in devtools.js. I would be happy if some of this logic lives in Gaia (in a file with the same name if needed). For example anything that contains a string (like kb) may be localized and we don't want to do that in b2g/.

Generally the code looks good.

::: b2g/chrome/content/devtools.js
@@ +1,4 @@
> +'use strict';
> +
> +const WIDGET_PANEL_LOG_PREFIX = 'WidgetPanel';
> +const MEMORY_WATCHER_INTERVAL = 1000;

Maybe a pref would be nice ?

Look on mxr for Services.prefs and how it works.

@@ +30,5 @@
> +  _watchers: [],
> +  _metrics: {},
> +  _trackAllApps: false,
> +
> +  init: function(callback) {

nit: as an old guy I like to name functions so a name appears in profiler instead of anonymous. I never remember if the root Gecko issue is fixed so usually I do a:

init: function wp_init(callback) {
...
},

to all my methods.

@@ +42,5 @@
> +          w.init(this._client);
> +        }
> +        let s = document.querySelector('#systemapp').contentWindow;
> +        s.addEventListener('appwillopen', this);
> +        s.addEventListener('appterminated', this);

I don't think we want to rely on Gaia internal events. Can we keep those events in Gaia but instead create a new dedicated mozChromeEvent/mozContentEvent protocol to communicate ?

@@ +45,5 @@
> +        s.addEventListener('appwillopen', this);
> +        s.addEventListener('appterminated', this);
> +        if (callback) callback();
> +      });
> +    });

I think this method miss some line breaks. Line breaks are free and cheap and leads to more readable code.

@@ +125,5 @@
> +    client.addListener('logMessage', this.consoleListener.bind(this));
> +    client.addListener('pageError', this.consoleListener.bind(this));
> +    client.addListener('consoleAPICall', this.consoleListener.bind(this));
> +    client.addListener('reflowActivity', this.consoleListener.bind(this));
> +  },

Same comment as paul. I'm also a bit surprised that the client.addListener API does not offer some sugar to avoid the need to do a bind, but I assume that could be because it is implemented in JS?

@@ +159,5 @@
> +        } else {
> +          app.widgets.get('errors').value++;
> +          output += 'error (';
> +        }
> +        let {errorMessage, category, lineNumber, columnNumber} = packet.pageError;

An intermediary variable |let pageError = packet.pageError;| will be nice instead of having to read the property 3 times in this function.

@@ +185,5 @@
> +        output = 'reflow: ' + duration + 'ms';
> +        if (sourceURL) {
> +          output += ' ' + formatSourceURL(packet);
> +        }
> +        break;

You can probably have a |let widgets = apps.widgets;| at the top of the switch.

@@ +195,5 @@
> +
> +  formatSourceURL: function(packet) {
> +    let {sourceURL, functionName, sourceLine} = packet;
> +
> +    if (sourceURL.substr(0, 5) == "data:") {

It seems like there is a mix of "" and '' in this file.

@@ +256,5 @@
> +    let memoryFront = MemoryFront(WidgetPanel._client, app.actor);
> +
> +    (function measure() {
> +      memoryFront.measure().then((res) => {
> +        app.widgets.get('totalMemory').value = Math.round(res.total / 1024) + ' KB';

nit: you probably have a method to format the memory a bit more.

@@ +261,5 @@
> +        app.display();
> +      }, (err) => {
> +        WidgetPanel.log('memory error ' + JSON.stringify(err));
> +      });
> +      MemoryWatcher._apps[app] = setTimeout(measure, MEMORY_WATCHER_INTERVAL);

Isn't the => to keep the scope and be able to use this instead of MemoryWatcher ? I'm not to used to arrow functions yet.

@@ +266,5 @@
> +    })();
> +  },
> +
> +  untrackApp: function(app) {
> +    if (!this._apps[app]) return;

nit: I'm an old guy. single line statement has always seems hard to read to me. Please expand it on 2 lines and add a line break afterward.

Same for other one line statement.

@@ +268,5 @@
> +
> +  untrackApp: function(app) {
> +    if (!this._apps[app]) return;
> +    clearTimeout(this._apps[app]);
> +    delete this._apps[app];

make a |let apps = this._apps;| and use that all across this function

::: b2g/chrome/content/shell.html
@@ +17,5 @@
>            src="chrome://browser/content/settings.js"> </script>
>    <script type="application/javascript;version=1.8"
>            src="chrome://browser/content/shell.js"> </script>
> +  <script type="application/javascript;version=1.8"
> +          src="chrome://browser/content/devtools.js"> </script>

I wonder if we can find a way to not load this file if Gaia is not using the devtools stuff.

Maybe by using:
#ifndef RELEASE_BUILD
....
#endif

Or anything smarter if we still want to enable this file for production build, but it seems bad to load it all the time for most users that will never used it.

	Paul Rouget [:paul]
	Comment 20 • 10 years ago

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #19)
> Maybe by using:
> #ifndef RELEASE_BUILD
> ....
> #endif
> 
> Or anything smarter if we still want to enable this file for production
> build, but it seems bad to load it all the time for most users that will
> never used it.

No more #ifdef!

If we host the code in a JSM, we could just load it when the setting is toggled.
Or dynamically insert a `<script>` tag.

	Jan Keromnes [:janx] Assignee
	Comment 21 • 10 years ago

Thanks for the reviews! I addressed the nits and removed the memoryWatcher and jankWatcher for now, because they depend on other bugs and will be easy to re-add in follow-ups.

> I guess we're dumping way too much data here. logcat is flooded with devtools info. Where/how can we dump these lines?

For now no data will be dumped and the output is saved in `this._log`. I'd like to find a good place to dump those logs without flooding `adb logcat`, e.g. make them optional behind a pref, or dump them in a console widget in the dropdown menu. Any thoughts?

> One thing I'm unsure about, is the fact that most of the data processing is done in devtools.js. I would be happy if some of this logic lives in Gaia (in a file with the same name if needed). For example anything that contains a string (like kb) may be localized and we don't want to do that in b2g/.

I designed the gaia API to be just a display API receiving widget color and text information. I don't think moving some of the logic to gaia makes sense, because all the knowledge about what's being tracked resides in shell's metricWatchers (e.g. consoleWatcher and memoryWatcher). Moving some of it to gaia would require watchers to be implemented across gecko and gaia and that would be a pain.

> I don't think we want to rely on Gaia internal events. Can we keep those events in Gaia but instead create a new dedicated mozChromeEvent/mozContentEvent protocol to communicate ?

That's currently exactly what the webappsActor does to watch apps:
http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/webapps.js#906

Why is this a problem?

> I wonder if we can find a way to not load this file if Gaia is not using the devtools stuff.

> If we host the code in a JSM, we could just load it when the setting is toggled. Or dynamically insert a `<script>` tag.

This still needs to be done, I'll look into making it a JSM.

Also needs to be done:
- Disabling the setting should clean up everything.
- A better developer menu should control which metrics (e.g. errors, memory...) and which apps (e.g. single app, all, certified only...) are being tracked.
- Any pointers to help me start writing tests?

	Jan Keromnes [:janx] Assignee
	Comment 22 • 10 years ago

> I won't fight too hard for the size of the widgets but I like big fonts, that's more convenient for my eyes. If it hurts others, maybe the right thing would then to fix the underlying core issue (the error, warning, reflow, lag, etc.. ;))

I completely agree :) However, to prevent the widgets from wrapping too soon when there is a lot of information (as in screenshot.jpg), I shrank them to 3/4 of their original size.

	Jan Keromnes [:janx] Assignee
	Comment 23 • 10 years ago

New screenshot with smaller, console-only widgets. Later patches will re-add memory, jank info and more.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 24 • 10 years ago

(In reply to Jan Keromnes [:janx] from comment #21)
> 
> > I guess we're dumping way too much data here. logcat is flooded with devtools info. Where/how can we dump these lines?
> 
> For now no data will be dumped and the output is saved in `this._log`. I'd
> like to find a good place to dump those logs without flooding `adb logcat`,
> e.g. make them optional behind a pref, or dump them in a console widget in
> the dropdown menu. Any thoughts?
>

One thing that you may do, is to check if Gaia has consumed the event you sent. This way is something goes wrong in Gaia (like if the system app is broken or we don't support yet one particular type of window) you can fallback to do a dump() and display the information in logcat.
 
> > One thing I'm unsure about, is the fact that most of the data processing is done in devtools.js. I would be happy if some of this logic lives in Gaia (in a file with the same name if needed). For example anything that contains a string (like kb) may be localized and we don't want to do that in b2g/.
> 
> I designed the gaia API to be just a display API receiving widget color and
> text information. I don't think moving some of the logic to gaia makes
> sense, because all the knowledge about what's being tracked resides in
> shell's metricWatchers (e.g. consoleWatcher and memoryWatcher). Moving some
> of it to gaia would require watchers to be implemented across gecko and gaia
> and that would be a pain.
>

Gecko will never really know what is displayed on the screen, and the only part that really knows what is currently on the screen is the system app. For example it can be multiple apps on the screen and especially in the future with Tablet, and possibly TV. Some of those apps may not used the events you rely on to be opened.

Also as discussed offline my main concerns here are strings localizations, and maybe colors. If Gaia is a display API as you said, it seems to me that display variables should belongs to Gaia, so let's send a bit more metadata and delegate that to Gaia.

> > I don't think we want to rely on Gaia internal events. Can we keep those events in Gaia but instead create a new dedicated mozChromeEvent/mozContentEvent protocol to communicate ?
> 
> That's currently exactly what the webappsActor does to watch apps:
> http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/
> webapps.js#906
> 
> Why is this a problem?
>

Because we can change those without any advertisements and not all apps displayed on the screen use those. Those are also Gaia internal APIs and can we expect to be able to change them without breaking anything. And lastly if someone else rewrite a system app then it may break.

I bet that if the webapps actor use that this is because nobody ask me to review it :) So I'm pretty sure that we can't debug the cost control widget on the system app for example ?

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 25 • 10 years ago

(In reply to Jan Keromnes [:janx] from comment #23)
> Created attachment 8364273 [details]
> screenshot.png
> 
> New screenshot with smaller, console-only widgets. Later patches will re-add
> memory, jank info and more.

Sounds better to not have the UI broken on multiple lines :)

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 26 • 10 years ago

(In reply to Paul Rouget [:paul] from comment #20)
> (In reply to Vivien Nicolas (:vingtetun) (:21) from comment #19)
> > Maybe by using:
> > #ifndef RELEASE_BUILD
> > ....
> > #endif
> > 
> > Or anything smarter if we still want to enable this file for production
> > build, but it seems bad to load it all the time for most users that will
> > never used it.
> 
> No more #ifdef!
> 
> If we host the code in a JSM, we could just load it when the setting is
> toggled.
> Or dynamically insert a `<script>` tag.

Dynamically inserting it is much better. If we don't do that in that patch but in a followup, let's still use an #ifdef to not regress Tarako, and let's remove it in the followup.

	Jan Keromnes [:janx] Assignee
	Comment 27 • 10 years ago

Added Gaia unit test.

	Jan Keromnes [:janx] Assignee
	Comment 28 • 10 years ago

TODO:
- Listen for opening/closing of apps as suggested in bug #963490.
- Dynamically load devtools_layers.js in the System App if the setting is checked.
- Dump watcher events to adb if they are not consumed by the System App.
- Move color and data formatting logic to Gaia, adding more metadata to the mozChromeEvents.

	Jan Keromnes [:janx] Assignee
	Comment 29 • 10 years ago

New gecko patch fixing:
- AppOpen/AppClose listener
- Delegate data formatting to Gaia

A few FIXMEs left for followups.

	Jan Keromnes [:janx] Assignee
	Comment 30 • 10 years ago

New gaia patch with data formatting. Widgets are created for every sent metric. If a metric is known (e.g. name: 'errors') it is formatted and displayed in a special way (e.g. with a red background). If not, it will be displayed as-is with an auto-generated background color.

	Paul Rouget [:paul]
	Comment 31 • 10 years ago

Comment on attachment 8366688 [details] [diff] [review]
layers.gecko.diff

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

Review as if this code was part of the devtools. I didn't review the consoleWatcher yet.
Code style might be different for B2G. Vivien will know.

Please add comments. This is not easy to review. An example of a code that is correctly commented: /toolkit/devtools/server/actors/webapps.js

::: b2g/chrome/content/devtools.js
@@ +1,1 @@
> +'use strict';

Missing license. Explain what this file is for in a comment.

@@ +11,5 @@
> +    devtoolsWidgetPanel.init();
> +  else
> +    devtoolsWidgetPanel.uninit();
> +});
> +

nit: in devtools code, we prefer curly brackets in general.

@@ +30,5 @@
> +      return;
> +
> +    if (!DebuggerServer.initialized)
> +      RemoteDebugger.start();
> +

Curly brackets.

@@ +57,5 @@
> +    for (let manifest of this._apps.keys()) {
> +      this.untrackApp(manifest);
> +    }
> +
> +    this._client.close(); // FIXME allows a callback, should we wait?

What do you mean?

@@ +59,5 @@
> +    }
> +
> +    this._client.close(); // FIXME allows a callback, should we wait?
> +    delete this._client;
> +  },

Aren't you supposed to remove the observers here?

@@ +63,5 @@
> +  },
> +
> +  registerWatcher: function dwp_registerWatcher(watcher) {
> +    this._watchers.unshift(watcher);
> +  },

unshift :) Not saying it is bad, but why not the traditional push() ?

@@ +65,5 @@
> +  registerWatcher: function dwp_registerWatcher(watcher) {
> +    this._watchers.unshift(watcher);
> +  },
> +
> +  trackApp: function dwp_trackApp(manifestURL, displayMetrics) {

Explain in a comment what displayMetrics is (apparently, it's a function).

@@ +76,5 @@
> +      type: 'getAppActor',
> +      manifestURL: manifestURL
> +    }, (res) => {
> +      if (res.error) {
> +        return;

Don't you want to dump the error?

@@ +100,5 @@
> +    if (app) {
> +      for (let w of this._watchers) {
> +        w.untrackApp(app);
> +      }
> +      delete app.metrics;

Is this delete useful?

@@ +106,5 @@
> +      this._apps.delete(manifestURL);
> +    }
> +  },
> +
> +  _defaultDisplayMetrics: function dwp_defaultDisplayMetrics() {

So "this" here is the app. This is confusing as this function is part of dwp. Can you find a better way to do that?

@@ +139,5 @@
> +          return;
> +        this.trackApp(manifestURL);
> +        this._urls.set(frameLoader.messageManager, manifestURL);
> +        break;
> +

Why do you use var suddenly?

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 32 • 10 years ago

Comment on attachment 8366695 [details] [diff] [review]
layers.gaia.diff

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

Also what about creating a apps/system/devtools/ directory ? The rule in Gaia is that if you have one file then it should stays in the root directory, if you have multiples then it should be inside its own. I know that you have one file for now but we may expect a few more files (for touch event for example). Let me know if that makes sense or if you plan to reuse the same file.

::: apps/system/js/devtools_layers.js
@@ +1,2 @@
> +(function(window) {
> +

'use strict';

@@ +1,3 @@
> +(function(window) {
> +
> +  window.addEventListener('mozChromeEvent', function(e) {

nit: give a name to your function

@@ +1,5 @@
> +(function(window) {
> +
> +  window.addEventListener('mozChromeEvent', function(e) {
> +    if (e.detail.type == 'widget-panel-update') {
> +      display(e.detail.data);

I was going to say do: e.preventDefault(); but now that the system app is just a display API and does not dump message itself, you probably still want to dump on logcat from the chrome side anyway.

@@ +31,5 @@
> +    data.metrics.forEach(function(metric) {
> +      html += widget(metric);
> +    });
> +
> +    overlay.innerHTML = html;

It seems a bit agressive to have to reconstruct the whole structure all the time. But I guess this can be fixed in a followup.

@@ +35,5 @@
> +    overlay.innerHTML = html;
> +  }
> +
> +  function widget(metric) {
> +    var color;

No need to declare |color| here as you may return early without having used it. As a rule I always try to declare my variables as close as possible to where they are used.

@@ +38,5 @@
> +  function widget(metric) {
> +    var color;
> +    var value = metric.value;
> +
> +    if (!value) return '';

nit:
if (!value) {
  return '';
}

@@ +66,5 @@
> +    var hue = 0;
> +    for (var i = 0; i < name.length; i++) {
> +      hue += name.charCodeAt(i);
> +    }
> +    return 'hsl(' + (hue % 360) + ', 75%, 50%)';

Sounds fun.

::: apps/system/style/window.css
@@ +360,5 @@
> +  justify-content: flex-end;
> +}
> +
> +.appWindow > .devtools-layer > .widget {
> +  font-size: 18px;

18px -> 1.8rem

@@ +366,5 @@
> +  line-height: 30px;
> +  height: 30px;
> +  min-width: 30px;
> +  text-align: center;
> +  flex: 0l;

0l -> 0 ?

@@ +368,5 @@
> +  min-width: 30px;
> +  text-align: center;
> +  flex: 0l;
> +  opacity: 0.8;
> +}

What about creating your own css file?

::: build/settings.js
@@ +111,4 @@
>    settings['debugger.remote-mode'] = config.REMOTE_DEBUGGER ? 'adb-only'
>                                                              : 'disabled';
>  
> +  settings['devtools.overlay'] = false;

Let's move that to build/config/common-settings.js for now.

	Jan Keromnes [:janx] Assignee
	Comment 33 • 10 years ago

Thanks for the feedback!

> Please add comments. This is not easy to review. An example of a code that is correctly commented:
> /toolkit/devtools/server/actors/webapps.js

Done.

> Missing license. Explain what this file is for in a comment.

Added license and explanatory comments for devtoolsWidgetPanel and consoleWatcher.

> Aren't you supposed to remove the observers here?

They are removed.

> Don't you want to dump the error?

This will be refactored away in Bug #962577.

	Jan Keromnes [:janx] Assignee
	Comment 34 • 10 years ago

> Also what about creating a apps/system/devtools/ directory ? The rule in Gaia is that if you have one
> file then it should stays in the root directory, if you have multiples then it should be inside its own.
> I know that you have one file for now but we may expect a few more files (for touch event for example).
> Let me know if that makes sense or if you plan to reuse the same file.

I will respect the rule for now, but I might move the files into a `apps/system/devtools/` directory in follow-up patches if it makes sense.

> I was going to say do: e.preventDefault(); but now that the system app is just a display API and does
> not dump message itself, you probably still want to dump on logcat from the chrome side anyway.

I added `e.preventDefault()`. For now, the gecko side considers the event always consumed and thus never logs information, but this will be fixed later to depend on `e.isDefaultPrevented()`. Are you suggesting that before fixing this we should always log?

> It seems a bit agressive to have to reconstruct the whole structure all the time. But I guess this can
> be fixed in a followup.

It makes the API much simpler (no need to calculate display deltas or keep track of what's displayed / what's not).

> What about creating your own css file?

Done.

> Let's move that to build/config/common-settings.js for now.

Done.

	Jan Keromnes [:janx] Assignee
	Comment 35 • 10 years ago

Comment on attachment 8367216 [details] [diff] [review]
layers.gecko.diff

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

::: b2g/chrome/content/devtools.js
@@ +65,5 @@
> +
> +    for (let manifest of this._apps.keys()) {
> +      this.untrackApp(manifest);
> +    }
> +

Services.obs.removeObserver() x 3

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 36 • 10 years ago

(In reply to Jan Keromnes [:janx] from comment #34)
> Created attachment 8367220 [details] [diff] [review]
> layers.gaia.diff
> > I was going to say do: e.preventDefault(); but now that the system app is just a display API and does
> > not dump message itself, you probably still want to dump on logcat from the chrome side anyway.
> 
> I added `e.preventDefault()`. For now, the gecko side considers the event
> always consumed and thus never logs information, but this will be fixed
> later to depend on `e.isDefaultPrevented()`. Are you suggesting that before
> fixing this we should always log?
>

What you said at the end.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 37 • 10 years ago

Comment on attachment 8367220 [details] [diff] [review]
layers.gaia.diff

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

r+ with nits.

::: apps/system/js/devtools_layers.js
@@ +9,5 @@
> +    }
> +  });
> +
> +  function display(data) {
> +    var iframe = document.querySelector('iframe[mozapp="' + data.manifestURL + '"]');

I'm pretty sure this line is bigger than 80 chars, but I may be wrong and I don't want to count by hand :)

@@ +39,5 @@
> +  }
> +
> +  function widget(metric) {
> +    var value = metric.value;
> +

nit: no need for the line break here.

	Jan Keromnes [:janx] Assignee
	Comment 51 • 10 years ago

Opened a pull request, carrying over vingtetun's r+, and addressed nits:

> What you said at the end.

We'll now log everything.

> I'm pretty sure this line is bigger than 80 chars, but I may be wrong and I don't want to count
> by hand :)

Fixed two overflowing lines.

> nit: no need for the line break here.

Removed.

	Alexandre Poirot [:ochameau]
	Comment 52 • 10 years ago

Comment on attachment 8367268 [details] [diff] [review]
Firefox OS should be able to show on-device debug information for apps

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

Looks good, would be worth using sendCustomEvent and load devtools.js lazily in this patch rather than waiting for followup.

::: b2g/chrome/content/devtools.js
@@ +15,5 @@
> +    devtoolsWidgetPanel.init();
> +  } else {
> +    devtoolsWidgetPanel.uninit();
> +  }
> +});

Can you move this to settings_listener.js, and load devtools.js only on demand, with something like this:
let devtoolsWidgetPanel;
SettingsListener.observe('devtools.overlay', false, (value) => {
  if (!devtoolsWidgetPanel) {
    let scope = {};
    Services.scriptloader.loadSubScript('chrome://browser/content/devtools.js', scope);
    devtoolsWidgetPanel = scope.devtoolsWidgetPanel;
  }
  ...
});

@@ +103,5 @@
> +        manifest: manifestURL,
> +        actor: res.actor,
> +        metrics: new Map(),
> +        display: displayMetrics || this._defaultDisplayMetrics
> +      };

nit: it might have been easier to understand/read by using prototype and instanciating an object.
  function App(...) {...}
  App.prototype.display = function defaultDisplayMetrics(...) {...}
  
  let app = new App(manifestURL, res.actor, displayMetrics);

Also custom display method isn't used yet and complexify the comprehension. It looks like a premature feature code.

@@ +145,5 @@
> +        data.metrics.push({name: name, value: metrics.get(name)});
> +      }
> +    }
> +
> +    shell.sendChromeEvent({type: 'widget-panel-update', data: data});

So if I understood vivien and fabrice, for performance reasons, it is better to use a custom event.
  shell.sendCustomEvent('widget-panel-update', data);
To prevent dispatching all mozChromeEvent listener on such event.

@@ +253,5 @@
> +        } else {
> +          this.bump(app, 'errors');
> +          output += 'error (';
> +        }
> +        let {errorMessage, category, lineNumber, columnNumber} = pageError;

`errorMessage` can be a long string actor and then you will print something like [object Object].
You can try to do similar thing to what is done here:
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/webconsole/webconsole.js#1329

Also, I'm not sure you show the file name but only line/column numbers.
pageError.sourceName should contain the file url/path or something.

@@ +259,5 @@
> +        break;
> +
> +      case 'consoleAPICall':
> +        switch (packet.message.level) {
> +          case 'error':

nit: to cover all cases, there is also `console.exception` which can be considered as error,
but it looks like none of gaia apps are using it!

@@ +289,5 @@
> +      devtoolsWidgetPanel.log(output);
> +    }
> +  },
> +
> +  formatSourceURL: function cw_formatSourceURL(packet) {

Better share'n reuse than copy'n paste
  let {devtools} = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});
  let {WebConsoleUtils} = devtools.require("devtools/toolkit/webconsole/utils");
  WebConsoleUtils.abbreviateSourceURL(packet.sourceURL);

Good candidate for followup if that needs some tweaks in utils.js.

	Jan Keromnes [:janx] Assignee
	Comment 55 • 10 years ago

Comment on attachment 8367930 [details] [diff] [review]
Implement a DevTools Widget Panel for Firefox OS.

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

Thanks for your review. The new patch addresses most of the nits:

- communication uses sendCustomEvent
- devtools.js is now loaded lazily
- better output formatting for pageErrors, also handles errorMessages of type longString
- App is now a separate object with prototype, and I removed the optional parameter `displayMetric` for now
- fixed accidental copy/pasta, no need to follow it up

::: b2g/chrome/content/devtools.js
@@ +262,5 @@
> +        output += category + '): "' + (errorMessage.initial || errorMessage) +
> +          '" in ' + sourceName + ':' + lineNumber + ':' + columnNumber;
> +        break;
> +
> +      case 'consoleAPICall':

Still misses console.exception().

::: b2g/chrome/jar.mn
@@ +12,5 @@
>  * content/dbg-browser-actors.js         (content/dbg-browser-actors.js)
>  * content/settings.js                   (content/settings.js)
>  * content/shell.html                    (content/shell.html)
>  * content/shell.js                      (content/shell.js)
> +  content/devtools.js                   (content/devtools.js)

Shouldn't there be a star?

	Alexandre Poirot [:ochameau]
	Comment 56 • 10 years ago

Comment on attachment 8367930 [details] [diff] [review]
Implement a DevTools Widget Panel for Firefox OS.

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

Works great. Thanks for this substantial feature!

::: b2g/chrome/content/shell.html
@@ +17,5 @@
>            src="chrome://browser/content/settings.js"> </script>
>    <script type="application/javascript;version=1.8"
>            src="chrome://browser/content/shell.js"> </script>
> +  <!-- <script type="application/javascript;version=1.8"
> +          src="chrome://browser/content/devtools.js"> </script> -->

I think we can just remove that now.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 58 • 10 years ago

I wonder if it worth fixing the wrong display of the tool for the keyboard/cost control widget in this PR ? I won't mind about a followup though.

	Alexandre Poirot [:ochameau]
	Comment 59 • 10 years ago

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

	Jan Keromnes [:janx] Assignee
	Comment 60 • 10 years ago

Fixing for keyboard/cost control will have to wait for the follow-up, we'll take apart the webappsActor and fix both the way it tracks running apps and creates app-specific actors (Bugs #963490 and #962577) so that those can be reused by the devtools layers.

	Jan Keromnes [:janx] Assignee
	Comment 61 • 10 years ago

The try run seems ok apart from a few marionette timeouts and a random webaudio test fail. Is that good enough for a checkin-needed?

	Alexandre Poirot [:ochameau]
	Comment 62 • 10 years ago

I relaunched marionette and gaia tests to see if that's intermittent test slaves failure.
Mochitests failure looks all unrelated.

	Jan Keromnes [:janx] Assignee
	Comment 63 • 10 years ago

Spamming made Mochitest 1 and Marionette WebAPI tests work.

I'm going to take the bet that my patch has nothing to do with the sample number of test_audioBufferSourceNodeOffset.html nor with the marionette InvalidResponseException.

	Jan Keromnes [:janx] Assignee
	Comment 65 • 10 years ago

Comment on attachment 8368584 [details] [diff] [review]
Implement a DevTools Widget Panel for Firefox OS. r=ochameau

Amended commit message, carrying over ochameau's r+.

	:gerard-majax
	Comment 66 • 10 years ago

https://github.com/mozilla-b2g/gaia/commit/c815574b478b53f4464ea29797813bc9f754924a

	Jan Keromnes [:janx] Assignee
	Comment 67 • 10 years ago

Comment on attachment 8367281 [details] [review]
Show app debug information in a devtools overlay.

The gaia patch has landed.

	Jan Keromnes [:janx] Assignee
	Comment 68 • 10 years ago

Not resolved yet, the gecko patch still needs checkin.

	[:fabrice] Fabrice Desré
	Comment 69 • 10 years ago

That's not something that should be tackled here, but the really meaningful number for OOP apps is the USS number we get from b2g-info since this is what the os would free once the app is killed. I'd be curious to see how it compares to the memory reporting actor.
And actually it's a bit more complex since running an app can (and will) trigger allocations in the parent process also dependig on which apis are used...

	Ryan VanderMeulen [:RyanVM]
	Comment 70 • 10 years ago

https://hg.mozilla.org/integration/fx-team/rev/29a1843b5986

	Ryan VanderMeulen [:RyanVM]
	Comment 71 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/29a1843b5986

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 72 • 10 years ago

(In reply to Fabrice Desré [:fabrice] from comment #69)
> That's not something that should be tackled here, but the really meaningful
> number for OOP apps is the USS number we get from b2g-info since this is
> what the os would free once the app is killed. I'd be curious to see how it
> compares to the memory reporting actor.

The uss info is very easy to get from this API. Probably a few lines to get it.

> And actually it's a bit more complex since running an app can (and will)
> trigger allocations in the parent process also dependig on which apis are
> used...

Agreed. But we should start with something :)