Bug 1020288 (monitor)

Add a runtime monitor to WebIDE

RESOLVED FIXED in Firefox 34

Status

DevTools
WebIDE
RESOLVED FIXED
4 years ago
a month ago

People

(Reporter: janx, Assigned: janx)

Tracking

({dev-doc-needed, perf})

Trunk
Firefox 34
dev-doc-needed, perf
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 11 obsolete attachments)

57.55 KB, image/png
Details
36.29 KB, patch
janx
: review+
Details | Diff | Splinter Review
(Assignee)

Description

4 years ago
In Firefox WebIDE, add a Monitor tool to display performance metrics about a Firefox OS device or simulator.
(Assignee)

Updated

4 years ago
Summary: Add a runtime Monitor to WebIDE → Add a runtime monitor to WebIDE
(Assignee)

Comment 1

4 years ago
Created attachment 8434322 [details] [diff] [review]
Add a runtime monitor to WebIDE. r=paul
Depends on: 1025804
Depends on: 1025828
(Assignee)

Updated

4 years ago
Blocks: 1026163
(Assignee)

Comment 2

4 years ago
Created attachment 8440954 [details] [diff] [review]
Add a runtime monitor to WebIDE.
Attachment #8434322 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Blocks: 1026169
(Assignee)

Comment 3

4 years ago
Created attachment 8441396 [details] [diff] [review]
Add a runtime monitor to WebIDE.
Attachment #8440954 - Attachment is obsolete: true
(Assignee)

Comment 4

4 years ago
Created attachment 8441406 [details]
screenshot

Here is a screenshot of the current work-in-progress Monitor.
The data is forwarded from the Developer HUD to the Monitor.

The first graph is random data.
The second graph is USS of each app.
The third graph is jank in milliseconds.

TODO:
- Y scale has unit issues (needs units, e.g. "ms", and formatting, e.g. "KB" or "MB").
- Navigation/zoom in graphs.
- There is currently no way to display markers (vertical lines in the graph).
- Aesthetics need some love (e.g. it's hard to see the title of a graph).
- We might want to disable graphs to "ignore" their data.
- Hovering over a curve should show a label with curve name and value at cursor.
(Assignee)

Comment 5

4 years ago
Comment on attachment 8441396 [details] [diff] [review]
Add a runtime monitor to WebIDE.

Paul, this is the current state of the monitor. Do you have feedback about it? Things I missed in the TODO?

At some point I'd appreciate some help on the UI.

Additional info, the current API to display data in the Monitor is to use either observer notifications on the device, or serve JSON objects with a local WebSockets server on port 9000.

Update objects can look like:

> { graph: 'memory', curve: 'homescreen', value: 155, time: 1234 }

> { curve: 'homescreen', values: [ {value: 155, time: 1234} ] }

`graph` will default to undefined.

> { time: 1234, voltage: 42, current: 58 }

`graph` defaults to undefined as well.
Unknown keys like `voltage` and `current` will be considered as separate curves.

> [ { graph: 'test', curve: '1', values: [] }, { graph: 'test', curve: '2', values: [] } ]

The format is fairly flexible and should work intuitively for any data.

> { graph: 'test', marker: 'error', time: 1234 }

This is my current idea for markers (not implemented yet) that would display a vertical bar labelled "error" in graph "test" at time 1234.
Attachment #8441396 - Flags: feedback?(paul)

Comment 6

4 years ago
(wow! I'll look at that asap!)

Comment 7

4 years ago
Comment on attachment 8441396 [details] [diff] [review]
Add a runtime monitor to WebIDE.

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

LGTM.

~~

What's the license of d3.js? It should live in /browser/devtools/shared/.

::: browser/devtools/webide/content/monitor/monitor.js
@@ +41,5 @@
> +    setInterval(function() {
> +      Monitor.update({graph: 'test', curve: 'homescreen', values: [{time: Date.now(), value: Math.ceil(Math.random()*100)}]});
> +    }, 250);
> +
> +    AppManager.on('list-tabs-response', Monitor.connectToRuntime);

Don't forget to call AppManager.off.

::: browser/devtools/webide/content/monitor/monitor.xhtml
@@ +7,5 @@
> +<!DOCTYPE html [
> +  <!ENTITY % webideDTD SYSTEM "chrome://webide/content/webide.dtd" >
> +  %webideDTD;
> +]>
> +

Not sure you need the DTD here.

@@ +13,5 @@
> +  <head>
> +    <meta charset="utf8"/>
> +    <style>
> +      body {
> +        background-color: white;

No inline CSS.
Attachment #8441396 - Flags: feedback?(paul) → feedback+

Comment 8

4 years ago
(In reply to Jan Keromnes [:janx] from comment #5)
> At some point I'd appreciate some help on the UI.

Yes. I'll see what we can do here.
If it blocks you, let me know.

> Additional info, the current API to display data in the Monitor is to use
> either observer notifications on the device, or serve JSON objects with a
> local WebSockets server on port 9000.

This is excellent! Make sure we can configure the address of the websocket server.

> Update objects can look like:
> 
> > { graph: 'memory', curve: 'homescreen', value: 155, time: 1234 }
> 
> > { curve: 'homescreen', values: [ {value: 155, time: 1234} ] }
> 
> `graph` will default to undefined.
> 
> > { time: 1234, voltage: 42, current: 58 }
> 
> `graph` defaults to undefined as well.
> Unknown keys like `voltage` and `current` will be considered as separate
> curves.
> 
> > [ { graph: 'test', curve: '1', values: [] }, { graph: 'test', curve: '2', values: [] } ]
> 
> The format is fairly flexible and should work intuitively for any data.
> 
> > { graph: 'test', marker: 'error', time: 1234 }
> 
> This is my current idea for markers (not implemented yet) that would display
> a vertical bar labelled "error" in graph "test" at time 1234.

All of that sounds good to me.

Comment 10

4 years ago
Victor has been working on graphs widgets too (attachment 8441664 [details]).
Flags: needinfo?(vporof)
We already have line and bar graphs implemented and shipped. They look like this: https://bug879008.bugzilla.mozilla.org/attachment.cgi?id=8441664 and will be used for the new profiler frontend.

The code for both graph types is at http://dxr.mozilla.org/mozilla-central/source/browser/devtools/shared/widgets/Graphs.jsm

Both graphs can easily be extended. It seems that the line graph specifically will need to be able to draw multiple curves of different colors. This isn't hard to fix.
Flags: needinfo?(vporof)
(Assignee)

Comment 12

4 years ago
Created attachment 8444138 [details] [diff] [review]
Add a Runtime Monitor to WebIDE.
Attachment #8441396 - Attachment is obsolete: true
(Assignee)

Comment 13

4 years ago
Thanks for the info Victor! The line and bar graphs from Graphs.jsm look awesome, we might want to use them in the Monitor.

For the prototype, I started using d3.js with SVG, but we should seriously consider switching to Graphs.jsm given that SVG graphs might have performance issues. I'm definitely interested in comparing both approaches.
(Assignee)

Comment 14

4 years ago
Created attachment 8448244 [details] [diff] [review]
Add a Runtime Monitor to WebIDE.
Attachment #8444138 - Attachment is obsolete: true
(Assignee)

Comment 15

4 years ago
Created attachment 8448766 [details] [diff] [review]
Add a Runtime Monitor to WebIDE.
Attachment #8448244 - Attachment is obsolete: true
(Assignee)

Comment 16

4 years ago
Comment on attachment 8448766 [details] [diff] [review]
Add a Runtime Monitor to WebIDE.

Paul, care to take a look?

Known issues:
- Punctual events are implemented, but not events with durations.
- USS measurements make the device really slow, whereas b2g-info supposedly doesn't, need to investigate.
- When you reconnect to a device, you receive one old message, making all curves start with a long straight line and causing the time scale to be uncomfortably big.
- Vertical ruler has no label on the X axis.
- Horizontal ruler label doesn't update instantly when the scale changes.
- Horizontal ruler doesn't resize when you resize the WebIDE.
- When no data is being sent, the Monitor is plain white (confusing).
Attachment #8448766 - Flags: review?(paul)
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1026169
(Assignee)

Updated

4 years ago
No longer blocks: 1026169
(Assignee)

Comment 18

4 years ago
Comment on attachment 8448766 [details] [diff] [review]
Add a Runtime Monitor to WebIDE.

(new version coming up soon)
Attachment #8448766 - Flags: review?(paul)
(Assignee)

Comment 19

4 years ago
Created attachment 8456915 [details] [diff] [review]
Add a Runtime Monitor to WebIDE.
Attachment #8448766 - Attachment is obsolete: true
(Assignee)

Comment 20

4 years ago
Comment on attachment 8456915 [details] [diff] [review]
Add a Runtime Monitor to WebIDE.

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

::: browser/devtools/webide/webide-prefs.js
@@ +12,5 @@
>  pref("devtools.webide.simulatorAddonsURL", "https://ftp.mozilla.org/pub/mozilla.org/labs/fxos-simulator/#VERSION#/#OS#/fxos_#SLASHED_VERSION#_simulator-#OS#-latest.xpi");
>  pref("devtools.webide.simulatorAddonID", "fxos_#SLASHED_VERSION#_simulator@mozilla.org");
>  pref("devtools.webide.adbAddonURL", "https://ftp.mozilla.org/pub/mozilla.org/labs/fxos-simulator/adb-helper/#OS#/adbhelper-#OS#-latest.xpi");
>  pref("devtools.webide.adbAddonID", "adbhelper@mozilla.org");
> +pref("devtools.webide.enableMonitor", true);

Self-nit: Let's not forget to change this to `false` before checking in.
(Assignee)

Comment 21

4 years ago
Comment on attachment 8456915 [details] [diff] [review]
Add a Runtime Monitor to WebIDE.

Should be easier to review now, I added a lot of comments. Please have a look.
Attachment #8456915 - Flags: review?(paul)
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
Keywords: perf

Comment 22

4 years ago
Can you rebase your patch on top of fx-team?
(Assignee)

Comment 23

4 years ago
Comment on attachment 8456915 [details] [diff] [review]
Add a Runtime Monitor to WebIDE.

(rebasing on top of fx-team...)
Attachment #8456915 - Flags: review?(paul)
(Assignee)

Comment 24

4 years ago
Created attachment 8457948 [details] [diff] [review]
Add a Runtime Monitor to WebIDE.

Rebased on top of fx-team.
Attachment #8456915 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8457948 - Flags: review?(paul)

Comment 25

4 years ago
Comment on attachment 8457948 [details] [diff] [review]
Add a Runtime Monitor to WebIDE.

- if the runtime gets disconnected, after a reconnect, the monitor shows previous data, but doesn't update the graph
- for some reason, I had to restart phone and/or webide a couple of times before getting it work
- Monitor.load should be called only once the monitor has been displayed once. Because you can't use the visibility API to do so, maybe we can use postMessage from within webide.js to tell a deck that it's now displayed/hidden
- AppManager.off('list-tabs-response', Monitor.connectToRuntime). Did you mean AppManager.off('app-manager-update', Monitor.onAppManagerUpdate) ?
- Monitor.front should be destroyed when the connection is lost, not on unload
Attachment #8457948 - Flags: review?(paul) → review-
(Assignee)

Comment 26

4 years ago
Created attachment 8458663 [details] [diff] [review]
Add a Runtime Monitor to WebIDE.

Addressed most (see below) of your nits.

However, I couldn't reproduce the disconnect/reconnect issue, nor did I have to restart several times.

Note that you need the certified apps pref to see their USS, or else you won't see any data before launching a non-certified app.
Attachment #8457948 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8458663 - Flags: review?(paul)

Comment 27

4 years ago
After closing and opening an app:

ReferenceError: client is not defined
Stack trace:
MonitorClient.prototype.destroy@chrome://webide/content/monitor.js:220:3
Monitor.onRuntimeAppEvent/</<@chrome://webide/content/monitor.js:182:40
emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/event/core.js:96:9
Request.prototype.emit@resource://gre/modules/devtools/dbg-client.jsm:1087:29
DebuggerClient.prototype.onPacket/<@resource://gre/modules/devtools/dbg-client.jsm:922:9
resolve@resource://gre/modules/devtools/deprecated-sync-thenables.js:40:40
then@resource://gre/modules/devtools/deprecated-sync-thenables.js:20:43
then@resource://gre/modules/devtools/deprecated-sync-thenables.js:58:9
DebuggerClient.prototype.onPacket@resource://gre/modules/devtools/dbg-client.jsm:861:1
DebuggerTransport.prototype._onJSONObjectReady/<@resource://gre/modules/devtools/dbg-client.jsm -> resource://gre/modules/devtools/transport/transport.js:461:9
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:83:14
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:83:14

Comment 28

4 years ago
Comment on attachment 8458663 [details] [diff] [review]
Add a Runtime Monitor to WebIDE.

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

Looking good. Still a bit of work.

Also, add a "close" button.

::: browser/devtools/webide/content/jar.mn
@@ +19,5 @@
>      content/prefs.js                  (prefs.js)
>      content/prefs.xhtml               (prefs.xhtml)
> +    content/monitor.xhtml             (monitor.xhtml)
> +    content/monitor.js                (monitor.js)
> +    content/monitor.css               (monitor.css)

Move the css file to /theme/ (I know it's not really ideal, but I'd like to keep all the CSS files in one place)

::: browser/devtools/webide/content/monitor.css
@@ +1,1 @@
> +body {

Missing license block.

::: browser/devtools/webide/content/monitor.js
@@ +8,5 @@
> +const {AppManager} = require('devtools/webide/app-manager');
> +const {AppActorFront} = require('devtools/app-actor-front');
> +const {Services} = Cu.import("resource://gre/modules/Services.jsm");
> +const EventEmitter = require("devtools/toolkit/event-emitter");
> +

Single or double quote? Take a decision :)

@@ +66,5 @@
> +  /**
> +   * Initialize the Monitor.
> +   */
> +  load: function() {
> +    Monitor.connectToWebSocket();

This should happen only once the monitor has been selected.

@@ +96,5 @@
> +    switch (what) {
> +      case 'list-tabs-response':
> +        Monitor.connectToRuntime();
> +        break;
> +      case 'runtime-disconnected':

Check "connection", and then check `if (AppMgr.selectedRuntime) {...}`
`

@@ +99,5 @@
> +        break;
> +      case 'runtime-disconnected':
> +        Monitor.disconnectFromRuntime();
> +        break;
> +      case 'selected-deck-panel':

Don't use AppMgr for UI logic.

@@ +274,5 @@
> +    .call(this.xaxis);
> +  this.yelement = this.svg.append('g').attr('class', 'y axis')
> +    .call(this.yaxis);
> +
> +  // TODO bisectDate http://bl.ocks.org/mbostock/3902569

what's that?

::: browser/devtools/webide/content/monitor.xhtml
@@ +13,5 @@
> +    <script src="chrome://browser/content/devtools/d3.js"></script>
> +    <script type="application/javascript;version=1.8" src="monitor.js"></script>
> +  </head>
> +  <body>
> +  </body>

Can you add a title? (<title> & <h1>)

::: browser/devtools/webide/content/webide.js
@@ +496,5 @@
>      let deck = document.querySelector("#deck");
>      let panel = deck.querySelector("#deck-panel-" + id);
>      deck.selectedPanel = panel;
>      this.updateProjectEditorMenusVisibility();
> +    AppManager.update("selected-deck-panel", {id: id});

Let's not use AppManager for the UI logic. Reach the iframe in the deck, and use "postMessage" to send a "selected" and "unselected" event.

::: browser/devtools/webide/content/webide.xul
@@ +163,5 @@
>          <vbox flex="1" id="runtime-actions">
>            <toolbarbutton class="panel-item" id="runtime-details" command="cmd_showRuntimeDetails"/>
>            <toolbarbutton class="panel-item" id="runtime-permissions" command="cmd_showPermissionsTable"/>
>            <toolbarbutton class="panel-item" id="runtime-screenshot"  command="cmd_takeScreenshot"/>
> +          <toolbarbutton class="panel-item" id="runtime-monitor"  command="cmd_showMonitor"/>

Do we want to expose it here? What about keeping it in the top menu?

::: browser/devtools/webide/modules/app-manager.js
@@ +118,5 @@
>  
>    onConnectionChanged: function() {
>      if (this.connection.status == Connection.Status.DISCONNECTED) {
>        this.selectedRuntime = null;
> +      this.update("runtime-disconnected");

Can you do that instead:

Can't you just check for "connection" ?

::: browser/devtools/webide/webide-prefs.js
@@ +13,5 @@
>  pref("devtools.webide.simulatorAddonsURL", "https://ftp.mozilla.org/pub/mozilla.org/labs/fxos-simulator/#VERSION#/#OS#/fxos_#SLASHED_VERSION#_simulator-#OS#-latest.xpi");
>  pref("devtools.webide.simulatorAddonID", "fxos_#SLASHED_VERSION#_simulator@mozilla.org");
>  pref("devtools.webide.adbAddonURL", "https://ftp.mozilla.org/pub/mozilla.org/labs/fxos-simulator/adb-helper/#OS#/adbhelper-#OS#-latest.xpi");
>  pref("devtools.webide.adbAddonID", "adbhelper@mozilla.org");
> +pref("devtools.webide.enableMonitor", true);

Should we keep this preffed on? I think we should.
Attachment #8458663 - Flags: review?(paul) → review-

Comment 29

4 years ago
2 other things (Can be done in a follow-up):
- we need tests
- We should have a way to stop monitoring everything, or just stop monitoring USS (you might now want to track USS if you're tracking something else)
(Assignee)

Comment 30

4 years ago
Thanks for the review! I'll work on your nits right away.

> 2 other things (Can be done in a follow-up):
> - we need tests

We definitely need tests. I was holding off until things stabilize, but I'd love to see the different data formats and the websocket connection tested.

> - We should have a way to stop monitoring everything, or just stop
> monitoring USS (you might now want to track USS if you're tracking something
> else)

That's why I made graphs collapsible, if you're not interested in USS just collapse the graph. I don't see the benefits of adding complexity to the client, protocol and data providers.

On the other hand, I did think of something more long-term: Maybe collapsed graphs should notify data providers that they can optionally stop sending data.

A corollary feature to this is that data providers that typically send massive amounts of data or have a big perf impact could just notify the Monitor front-end that they exist, without sending any actual data. The Monitor would then show a collapsed graph, and only when the user expands the graph, the corresponding provider could start its massive data collection and forwarding.
(Assignee)

Updated

4 years ago
Depends on: 1042857
(Assignee)

Comment 31

4 years ago
Created attachment 8462105 [details] [diff] [review]
Add a Runtime Monitor to WebIDE.

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #28)
> Looking good. Still a bit of work.
> 
> Also, add a "close" button.

Done.

> Move the css file to /theme/ (I know it's not really ideal, but I'd like to
> keep all the CSS files in one place)

Done (also did bug 1042857).

> Missing license block.

Fixed.

> Single or double quote? Take a decision :)

Single.

> > +  load: function() {
> > +    Monitor.connectToWebSocket();
> 
> This should happen only once the monitor has been selected.

It was already the case.

> > +      case 'runtime-disconnected':
> 
> Check "connection", and then check `if (AppMgr.selectedRuntime) {...}`

I removed the `runtime-disconnected` event, used `connection` instead and checked for `Connection.Status.DISCONNECTED`.

> Don't use AppMgr for UI logic.
> 
> Let's not use AppManager for the UI logic. Reach the iframe in the deck, and
> use "postMessage" to send a "selected" and "unselected" event.

I decided to use a lazy src attribute on the monitor instead, that is only set when the user first opens the Monitor.

Since we want to keep collected data, and continue to receive updates, even if the Monitor is closed again, we don't destroy it on close. The overhead will be minimal, because the bulk of the Monitor perf hit is painting, which doesn't happen if not visible.

> > +  // TODO bisectDate http://bl.ocks.org/mbostock/3902569
> 
> what's that?

Already done in Graph.prototype.bisectTime and Graph.prototype.valueAt, UI will follow at some point, comment removed.

> Can you add a title? (<title> & <h1>)

Added only <h1> (no other page has <title>).

> Do we want to expose it here? What about keeping it in the top menu?

Removed.

> > +      this.update("runtime-disconnected");
> 
> Can you do that instead:
> 
> Can't you just check for "connection" ?

Done (see above).

> > +pref("devtools.webide.enableMonitor", true);
> 
> Should we keep this preffed on? I think we should.

Removed the pref.
Attachment #8458663 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8462105 - Flags: review?(paul)
(Assignee)

Comment 32

4 years ago
Created attachment 8462108 [details]
screenshot.png

(new screenshot)
Attachment #8441406 - Attachment is obsolete: true
(Assignee)

Comment 33

4 years ago
FYI, we're currently evaluating different ways of getting USS data for every Firefox OS process:

1. Polling on `nsIMemoryReporterManager.residentUnique` inside a Gecko process, e.g. with a devtools actor. This only gives the USS of one Gecko process, thus needs to be done in all B2G child processes. Bug 1037465.

Drawbacks:
- Devtools actors add significant perf overhead (memory and cpu) to every child process.
- We can't access memory of non-gecko processes like Nuwa.
- Doesn't work on older FxOS versions unless we uplift the actor, nor on devices like Tarako which can't handle the devtools overhead.
- Polling independently in each process is messy, data comes back irregularly.

2. Polling on `b2g-info` from ADB shell. This works out-of-the box with every FxOS device that has `b2g-info`, doesn't suffer from the devtools overhead, and data comes back regularly.

Drawbacks:
- Parsing text output is messy (that's what this bug tried to address).
- Won't work over WiFi when the devtools support that.

From comments in bug 1043324 I see another solution:

3. Write a devtools actor only for B2G's main process (where devtools overhead is small because most required dependencies are already loaded, which is not the case in child processes). The actor's job would be to read system information about each B2G process (even non-Gecko ones).

Drawbacks:
- Requires more work than parsing `b2g-info`.
- Requires uplift to work with older versions of FxOS.

Conclusions:
1 is not the best solution, but has already been done and will land pending a better solution.
2 is easy and works effortlessly with every device that has `b2g-info`, but it's a hack.
3 might arguably be better in the long term, especially when we'll want to add more metrics like CPU usage (we could also parse `b2g-ps`), network upload/download (I don't see an easy hack for this one), etc.

Comment 34

4 years ago
Comment on attachment 8462105 [details] [diff] [review]
Add a Runtime Monitor to WebIDE.

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

::: browser/devtools/webide/content/monitor.js
@@ +164,5 @@
> +
> +  /**
> +   * Used when cleaning up.
> +   */
> +  disconnectFromWebSocket: function() {

Save the timeout in connectToWebSocket, and clear it here.
Attachment #8462105 - Flags: review?(paul) → review+

Updated

4 years ago
Blocks: 1043934
(Assignee)

Comment 35

4 years ago
Created attachment 8462570 [details] [diff] [review]
Add a Runtime Monitor to WebIDE.

Addressed nits.

Known issue:
- Seeing USS of certified apps requires the certified apps debugging pref.
Attachment #8462105 - Attachment is obsolete: true
(Assignee)

Comment 36

4 years ago
Comment on attachment 8462570 [details] [diff] [review]
Add a Runtime Monitor to WebIDE.

Carrying over Paul's r+.
Attachment #8462570 - Flags: review+

Comment 38

4 years ago
(In reply to Jan Keromnes [:janx] from comment #33)
> 3 might arguably be better in the long term, especially when we'll want to
> add more metrics like CPU usage (we could also parse `b2g-ps`), network
> upload/download (I don't see an easy hack for this one), etc.

FWIW, parsing b2g-ps has the same issues as parsing b2g-info. It's better to directly obtain the info from /proc than to scrape the information from b2g-ps.
(Assignee)

Comment 39

4 years ago
I'm curious, when you say that "parsing b2g-ps has the same issues", do you only mean the drawbacks I listed in comment 33 (scraping text is hacky, and won't work over wifi) or do you see bigger problems with the `b2g-info` / `b2g-ps` approach?
Flags: needinfo?(mwu)

Comment 41

4 years ago
Parsing b2g-ps output is actually even worse than parsing b2g-info. We have full control over b2g-info, but b2g-ps is a wrapper over the system ps, so it's even more likely to change unexpectedly.

If your code looks at files in /proc, it's unlikely to break due to changes on the gonk/kernel side. Kernel interfaces have compatibility requirements that userspace apps don't have.
Flags: needinfo?(mwu)
(Assignee)

Comment 42

4 years ago
(In reply to Michael Wu [:mwu] from comment #41)
> Parsing b2g-ps output is actually even worse than parsing b2g-info. We have
> full control over b2g-info, but b2g-ps is a wrapper over the system ps, so
> it's even more likely to change unexpectedly.

Thanks for the insight! I might still write a temporary `b2g-info` parser, before Dave can write a better /proc-reading devtools actor, but I'll definitely refrain from parsing `b2g-ps`!

Dave, do you have a bug number for the /proc-reading actor you talk about in bug 1043324 comment 5?
Flags: needinfo?(dhuseby)
https://hg.mozilla.org/mozilla-central/rev/c8ccccd52a0a
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 34
(Assignee)

Comment 44

4 years ago
dev-doc-needed because I'll need some help with the stub page https://developer.mozilla.org/en-US/docs/Tools/WebIDE/Monitor
Keywords: dev-doc-needed
(Assignee)

Updated

4 years ago
Blocks: 1043324
(Assignee)

Updated

4 years ago
Blocks: 1047355
(Assignee)

Comment 45

4 years ago
I updated the MDN page[1], but it could probably be improved (e.g. it's hard to make out the code snippets from the text, maybe some things are hard to understand and need better explaining, etc.).

[1] https://developer.mozilla.org/en-US/docs/Tools/WebIDE/Monitor
Bug 1043324 is the bug for the new b2g info reporter.
Flags: needinfo?(dhuseby)
(Assignee)

Updated

4 years ago
Blocks: 1049562
(Assignee)

Updated

4 years ago
Blocks: 1045127
(Assignee)

Updated

4 years ago
Blocks: 1043378
(Assignee)

Updated

4 years ago
Blocks: 1053062

Updated

a month ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.