It is often very difficult to follow the flow of events in devtools code, especially because of our prolific use of promises. We should add devtools.dump.emit=false (true only for tests) as this allows us to log extra details to test logs. e.g. 17:42:28 INFO - EMITTING: select(webconsole) 17:42:28 INFO - EMITTING: changed([object XULElement] (tab)) 17:42:28 INFO - EMITTING: webconsole-selected([object Object]) ... 17:25:25 INFO - EMITTING: markuploaded() 17:25:25 INFO - EMITTING: before-new-node-front([Front for domnode/conn28.domnode37] (BODY), inspector-open) 17:25:25 INFO - EMITTING: new-node-front([Front for domnode/conn28.domnode37] (BODY), inspector-open) 17:25:25 INFO - EMITTING: inspector-ready([object Object]) 17:25:25 INFO - EMITTING: toolbox-ready([object Object]) 17:25:25 INFO - TEST-PASS | chrome://mochitests/content/browser/browser/devtools/inspector/test/browser_inspector_bug_699308_iframe_navigation.js | found the iframe element 17:25:25 INFO - EMITTING: before-new-node-front([Front for domnode/conn28.domnode37] (BODY), test) 17:25:25 INFO - EMITTING: new-node-front([Front for domnode/conn28.domnode37] (BODY), test) 17:25:25 INFO - EMITTING: select(inspector) 17:25:25 INFO - EMITTING: changed([object XULElement] (tab)) ... 17:42:30 INFO - EMITTING: destroyed() 17:42:30 INFO - EMITTING: toolbox-destroyed(TabTarget:null)

We should wait for Joe's feedback to see if it is okay to have this on for default when testing.

Removed a \n

Now no longer logs in tests by default

Comment on attachment 8377895 [details] [diff] [review] Patch V2 Review of attachment 8377895 [details] [diff] [review]: ----------------------------------------------------------------- "hearth" -> "harth" in commit (: Worried about the big try/catch: ::: browser/devtools/shared/event-emitter.js @@ +163,5 @@ > + dump(")"); > + } > + } > + } catch(e) { > + // Do nothing We should avoid a big try/catch if we can. What was the reason for it?

(In reply to Heather Arthur [:harth] from comment #4) > Comment on attachment 8377895 [details] [diff] [review] > Patch V2 > > Review of attachment 8377895 [details] [diff] [review]: > ----------------------------------------------------------------- > > "hearth" -> "harth" in commit (: > Sorry, no brain today. > Worried about the big try/catch: > > ::: browser/devtools/shared/event-emitter.js > @@ +163,5 @@ > > + dump(")"); > > + } > > + } > > + } catch(e) { > > + // Do nothing > > We should avoid a big try/catch if we can. What was the reason for it? We don't, removed and just generally tidied.

Now the logging looks like this: EMITTING: BC_update/<() -> emit(breadcrumbs-updated, [Front for domnode/conn0.domnode29] (DIV#output)) from resource:///modules/devtools/inspector/breadcrumbs.js:695

Bug 976679 moves event-emitter to toolkit, so you'll need to rebase if that lands before.

Mike, you told me to hold off on reviewing this. Is it ready now?

Yup, this is ready for review.

We need the try / catch in order to avoid logging info about dead objects (exception). As this is an option enabled by flipping the devtools.dump.emit pref I think we should be fine.

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #10) > Created attachment 8383038 [details] [diff] [review] > event-emitter-logging-974056.patch > > We need the try / catch in order to avoid logging info about dead objects > (exception). As this is an option enabled by flipping the devtools.dump.emit > pref I think we should be fine. It'd be a good to keep it as small as possible. Can we only wrap the statement that would throw the exception?

Any part of the for loop that goes through any passed arguments can throw so that is about the smallest we can make it.