534398 - Implement Heads Up Display console, js workspaces and related tools

Status: VERIFIED FIXED

(DevTools :: General, defect)

Description

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

We need a console object to inject the console API (e.g., from Firebug) in a browser's top-level window object. I think as a start, we should implement,

console.log() as a bare-minimum with stubs for console.warn and console.message pointing back to this method.

See linked URL for other examples of logging functions we might like to implement.

Initially, console.log() should write to either the console-service or window.dump().

Comment 1

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

full console API can be found here:

http://getfirebug.com/wiki/index.php/Console_API

again, I expect we'll cherry pick a few of the easier ones, like:

log, info, warn, and maybe error, assert, time. Others may be stubbed or we could have a ConsoleException that does something clever.

Comment 2

[David Dahl :ddahl]

I spent some time recently on String.prototype.printf:

http://daviddahl.blogspot.com/2010/01/everybody-needs-little-printf-in-their.html

I think this is a good approach, except in this case it is String.prototype.logf which produces the output for the console and sets the eventhandlers on each log message.

Comment 3

[David Dahl :ddahl]

on second thought, they may be a bit brittle of an approach. I went back to the prototype and realized we were using an HTML document for all of the logging output, which will simplify all of this stuff:)

Comment 4

[John J. Barton]

Since Web developers expect window.console to have specific semantics, as a practical matter you will need to either duplicate the Firebug window.console semantics or pick a different object name.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> either duplicate the Firebug window.console semantics

It seems like we only need to duplicate the semantics Firebug and Safari have in common, no?

Comment 6

[John J. Barton]

(In reply to comment #5)
> > either duplicate the Firebug window.console semantics
> 
> It seems like we only need to duplicate the semantics Firebug and Safari have
> in common, no?

What then do you tell Firebug developers when their code breaks in Firefox?

Comment 7

[Johnathan Nightingale [:johnath]]

(In reply to comment #6)
> (In reply to comment #5)
> > > either duplicate the Firebug window.console semantics
> > 
> > It seems like we only need to duplicate the semantics Firebug and Safari have
> > in common, no?
> 
> What then do you tell Firebug developers when their code breaks in Firefox?

Yeah - I think that we're talking about net-new functionality here, and not breaking the experience of the dominant web-development-on-Firefox tool is a valuable goal. I might suggest that, especially in the early development stages, where you want to explore the space without being constrained by the decisions other tools/browsers have made, you choose your own object name - so that wrangling semantic consistency doesn't eat too many cycles.

If we get an otherwise functional console using window.activitylog or window.debug and the biggest debate is around whether we should try to move back to window.console or not - I'd say we're succeeding. If development of the tool gets hung up on semantic differences in window.console, less so.

Comment 8

[David Dahl :ddahl]

I am not too worried about what we call it. There is 1 line of code where we name this object "console". We can even set a pref for what we call it. I'm calling it console for now just for the familiarity. 

I am sure we can come up with a better name, I just haven't thought about it yet.

Comment 9

[David Dahl :ddahl]

So the patch on bug 549539 works like a charm. This, of course changes all of the calculus of the existing implementation, so I have a few tweaks to make before things are really working.  

Of course the topic we observe "content-document-global-created" is a huge time-saver, as implementing that functionality seems impossible.

Also, I am going to move the HeadsUpDisplay.jsm into toolkit and use it's constructor as a Factory in the HUDService.

Patches coming tomorrow.

Comment 10

[David Dahl :ddahl]

Attachment: v 0.1 toolkit patch

moved most of the implementation into toolkit, still need to clean these patches up before any "first pass" reviews. This allows logging to a "heads up display" console from content. creating a try build right now.

Comment 11

[David Dahl :ddahl]

Attachment: 0.1 browser patch

Needs clean up - HeadsUpDisplay.jsm is being removed "browser"

Comment 12

[David Dahl :ddahl]

See user repo here: http://hg.mozilla.org/users/ddahl_mozilla.com/heads-up-display/

Comment 13

[David Dahl :ddahl]

while writing browser chrome tests, I discovered that there seems to be some latency between setting a property in the xpcom service (actually, adding a property name and value to this._headsUpDisplays - which keeps track of display widgets).

when a display is added  to the dom, the XUL elements are created, the console is intialized, adeed tot he contentWindow, and the dom ID we are using is added to a registry.

the test runs so fast that the registry is not updated before we check for it.

Comment 14

[David Dahl :ddahl]

please ignore previous comment...

Looks like I forgot to setup the "DOMContentLoaded" event listener

Comment 15

[David Dahl :ddahl]

Added many tests, many more to come, starting to work on HTTP observer/logger.

Will clean up front end code (removal) - before submitting for first pass review.

Latest changeset:

http://hg.mozilla.org/users/ddahl_mozilla.com/heads-up-display/rev/a0cccca48385

Comment 16

[David Dahl :ddahl]

Talked to blizzard today about Web Timing Spec at W3C. May be a good addition to our featureset/roadmap.

Comment 17

[David Dahl :ddahl]

I have begun to add the idea of "mixins" into the HUDService so any gecko app that wants to use HUDService can easily register a Mixin object that follows a specific interface for getting output nodes and dealing with attachment/detachment of UI widgets, etc. There is a smattering of this in thee latest changeset.

Comment 18

[David Dahl :ddahl]

(In reply to comment #16)
> Talked to blizzard today about Web Timing Spec at W3C. May be a good addition
> to our featureset/roadmap.
http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/05bb34ad4177eec4?pli=1

Comment 19

[David Dahl :ddahl]

Attachment: v 0.1 Toolkit HUDService patch

Removed all browser code. revamped tests, asking for a first pass review before I code much more. Based on some help with HttpObservers from Mossop, I will continue looking into LoadGroups. 

The HeadsUpDisplay.jsm will probably go away entirely, as I have been migrating things into the service as I figure out more how this should all work.

Comment 20

[David Dahl :ddahl]

Worked on the JS code evaluator, created a "terminal-like" input + output pane:

http://img6.yfrog.com/img6/4179/screenshot008z.png

pushed code to the user repo:

http://hg.mozilla.org/users/ddahl_mozilla.com/heads-up-display/

Comment 21

[David Dahl :ddahl]

Pushed some new functionality to the user repo:

commit message: "added http observation, tests and cleaned up dom node creation as well as started using HTML namespaced nodes instead of XUL in the console output"

http://hg.mozilla.org/users/ddahl_mozilla.com/heads-up-display/

Right now, the network observer logs several times for each network request, will have to figure out how to consolidate or ignore duplicate events.

Have tests for the network stuff, need to write more.

I need to start looking at the DOM element events now as well.

Comment 22

[David Dahl :ddahl]

Latest try build is here.

https://build.mozilla.org/tryserver-builds/[e-mail]-hudConsoleNetwork/

forgot to mention that since I changed the DOM nodes to html, I can now use node.scrollIntoView(false) to mimic "real console" scrolling behaviour.

Comment 23

[David Dahl :ddahl]

Figured out http logging redundancies and started ConsoleStorage module

http://hg.mozilla.org/users/ddahl_mozilla.com/heads-up-display/rev/c10e0b9f3dbf

Comment 24

[David Dahl :ddahl]

Today I got rid of the UUID generator in favor of a JS Generator that emits a sequence of ints for the ConsoleStorage backend

I implemented DOM Mutation event logging. 

Wrote tests for the ConsoleStorage module.

Worked on the front-end a bit as well.

http://hg.mozilla.org/users/ddahl_mozilla.com/heads-up-display/log/39798

Comment 25

[David Dahl :ddahl]

Probably need to talk with mrbkap about messages like this:

###!!! ASSERTION: What kind of wrapper is this?: 'IS_SLIM_WRAPPER(mCurrentJSObject)', file /home/ddahl/code/moz/user_repos/HUD/mozilla/js/src/xpconnect/src/xpccallcontext.cpp, line 218

Comment 26

[David Dahl :ddahl]

Noticed some new assertions today:

###!!! ASSERTION: This is unsafe: 'nsContentUtils::IsSafeToRunScript()', file /home/ddahl/code/moz/user_repos/HUD/mozilla/layout/base/nsDocumentViewer.cpp, line 1114
###!!! ASSERTION: This is unsafe! Fix the caller!: 'Error', file /home/ddahl/code/moz/user_repos/HUD/mozilla/content/events/src/nsEventDispatcher.cpp, line 490

Don't have time right now to research what is happening, so far does not seem to impact any functionality.

Comment 27

[Boris Zbarsky [:bzbarsky]]

If you're seeing those assertions, and if it's something web script can trigger as it wishes, then you have a security hole.

Comment 28

[David Dahl :ddahl]

I commented out a call to openWindow via the windowWatcher service and the asserts went away:

ww.openWindow(null, GLOBAL_CONSOLE_URI, "_blank", "chrome,dialog=no,all", null);

It is a mute point as I am removing all of this anyway. Thanks, bz.

Comment 29

[David Dahl :ddahl]

Attachment: HUD Console v0.1

This patch is 132k, not *too* bad. A "first pass" review would be most helpful at this point. There are many concepts in this code that are germinating yet, and need feedback.

Comment 30

[David Dahl :ddahl]

Attachment: HUD Console v0.1.1

Comment 31

[David Dahl :ddahl]

Latest Try build is here: https://build.mozilla.org/tryserver-builds/[e-mail]-HUDConsole0415/

Latest changesets are here: http://hg.mozilla.org/users/ddahl_mozilla.com/heads-up-display/

Comment 32

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

I think you've added to the original intent of this bug which was just to make a global window.console (or similarly-named property) available at page-creation. This looks like it contains most of the HUD. This is OK, feel free to modify the bug's subject as-needed.

A couple of pre-review review notes that might help: open braces for functions and methods should go on new lines, as in:

function ConsoleStorage()
{
  this.sequencer = null;
  this.consoleDisplays = {}
...

See https://developer.mozilla.org/En/Developer_Guide/Coding_Style for more details.

Also, I'm changing the status of this bug since you're doing the work.

Comment 33

[David Dahl :ddahl]

Attachment: HUD Console v0.1.2

Fixed coding style issues, removed dead code, added copious comments

Comment 34

[David Dahl :ddahl]

Attachment: HUD Console v0.1.3

Spoke with vlad this morning and he would like to review this patch when he has more time to really focus on it. He mentioned that bz and jonas might be good "first-pass" reviewers. This patch is still a bit of a WIP, and probably needs some direction and shaping yet, but we would like to land it asap on trunk.

Comment 35

[David Dahl :ddahl]

I also asked for feedback from from mrbkap - but I was also thinking mrbkap would make a GREAT reviewer for this patch (as well as others who sit near you at work:))

Comment 36

[David Dahl :ddahl]

Attachment: HUD Console v0.1.4

added a toolbar for console toggling of event listeners, started L10N string bundles, etc. Pushed to repo here: http://hg.mozilla.org/users/ddahl_mozilla.com/heads-up-display/rev/6b9ff3dc7cd0

Comment 37

[Axel Hecht [:Pike]]

Is headsUpDisplay.dtd used yet? I'm seeing "foo:" strings in there, which may hint at hard-coded whitespace. Same for those strings in the .properties file.

Comment 38

[David Dahl :ddahl]

(In reply to comment #37)
> Is headsUpDisplay.dtd used yet? I'm seeing "foo:" strings in there, which may
> hint at hard-coded whitespace. Same for those strings in the .properties file.

The dtd is not used yet, I am just using the properties file for now. Is there a rule on whitespace I should know? I am new to L10N work.

Comment 39

[Axel Hecht [:Pike]]

Something like
<label value="&foo.label;"/> <label id="somevalue/>
would have the " " hard coded, and should be written as
<label value="&foo.label;"/><label id="somevalue"/>
with
<!ENTITY foo.label "Some value: ">

Not sure if that's in effect here.

If you need a trailing whitespace in a .properties file, you'll need to unicode escape it, as regular whitespace is truncated.

Comment 40

[Dietrich Ayala (:dietrich)]

Comment on attachment 440923 [details] [diff] [review]
HUD Console v0.1.4

Per IRC, did a drive-by review. It was my first look, so definitely correct me if I've misunderstood something.

Some overall comments about structure and file organization:

* for consistency in toolkit, in console/hudservice the files should be in subdirs:
  * modules (for jsm)
  * src (for your xpcom components)
  * public (for your idl)

* the xpcom components should be combined into a single file

* actually, why do you need an IDL and those xpcom components? why can't they be js modules?

* the jsm files seem to be used together most of the time. is there ever a scenario where they wouldn't all be in play
when the console UI is visible? if not, they should probably all be combined into a single jsm, via the preprocessor maybe?

[UPDATE] After going over the patch again, I'm not seeing the need for XPCOM here.
It seems like you could move it all into a series of JS files that are compiled by the preprocessor into one single JS file at build-time, and pulled into host applications via a single jsm entry point.

You could initialize in the browser after the first point at which the window is up. Or maybe even just on first instantiation...

So, in this scenario (and including the host-app registration later in my comments) the host setup would look something like:

Cu.import("resource://gre/modules/HUDService.jsm");
HUDService.init(FirefoxAppHooksObject);

and that's all! No XPCOM, and just a single jsm.


+function ConsoleStorage()
+{

here and throughout, start brace should go on the same line

hm, actually, i'm exporting Places and /browser JS style. i *think* that's preferable, but maybe you were following the style in the other console code or something?

+  this.sequencer = null;
+  this.consoleDisplays = {};
+  // each display will have an index that tracks each ConsoleEntry
+  this.displayIndexes = {};
+  this.globalStorageIndex = [];
+  this.globalDisplay = {};

should all be defined first and documented in the prototype?

+  displayStore: function CS_displayStore(aId)

for all the methods in all of your classes in these modules, you should add at least a short description.

+var EXPORTED_SYMBOLS = ["NodeFactory",
+                        "LogMessage",
+                        "FirefoxApplicationHooks",
+                        "ConsoleDOMListeners",
+                        "ConsoleUtils",
+                        "LogFactory",
+                       ];
+
+/**
+ * Firefox-specific Application Hooks.
+ * Each Gecko-based application will need an object like this in
+ *  order to use the Heads Up Display
+ */

a better approach might be to have these hooks in /browser, where you initialize
this stuff, and register the hooks with the HUDService (or utils, whatever).

this allows any toolkit-app to use HUD without having to modify the HUD code.

eg, the browser code would look something like:

Cu.import("HUDUtils");

HUDUtils.applicationHook.register(FirefoxApplicationHooks);

+function FirefoxApplicationHooks()
+{ }
+
+FirefoxApplicationHooks.prototype = {

s/Firefox/Browser/

+/*
+ * HeadsUpDisplay is an interactive console initialized *per tab*  that
+ * displays console log data as well as provides an interactive terminal to
+ * manipulate the current tab's document content
+ * */

per previous comment about per-file summary comments, this'll do :)

+    if (HUDService.appName() == "FIREFOX") {
+      let outputCSSClassOverride = "hud-msg-node hud-console";
+      let mixin = new JSTermFirefoxMixin(context, aParentNode, aExistingConsole, outputCSSClassOverride);
+      let inputNode = new JSTerm(context, aParentNode, mixin);
+    }
+    else {
+      throw new Error("Unsupported Gecko Application");
+    }

shouldn't hard-code the app. should instead do something like the
hooks registration i described earlier, where the host application registers itself.

+  {
+    var loadGroup = this.contentWindow
+                    .QueryInterface(Ci.nsIInterfaceRequestor)
+                    .getInterface(Ci.nsIWebNavigation)
+                    .QueryInterface(Ci.nsIDocumentLoader).loadGroup;
+    return loadGroup;
+  },

dots at end of line

+XPCOMUtils.defineLazyServiceGetter(this, "prefService",
+                                   "@mozilla.org/preferences-service;1",
+                                   "nsIPrefService");
+
+XPCOMUtils.defineLazyServiceGetter(this, "obsService",
+                                   "@mozilla.org/observer-service;1",
+                                   "nsIObserverService");
+
+XPCOMUtils.defineLazyServiceGetter(this, "mozConsole",
+                                   "@mozilla.org/consoleservice;1",
+                                   "nsIConsoleService");

Services.jsm for these?

+Cu.import("resource://gre/modules/Services.jsm");
+
+XPCOMUtils.defineLazyServiceGetter(this, "ioSvc",
+                                   "@mozilla.org/network/io-service;1",
+                                   "nsIIOService");
+

you pull in Services.jsm, but then don't use it here for this and a bunch of services after this one

+
+// Console-related imports
+Cu.import("resource://gre/modules/HeadsUpDisplay.jsm");
+Cu.import("resource://gre/modules/ConsoleStorage.jsm");
+Cu.import("resource://gre/modules/JSTerm.jsm");
+Cu.import("resource://gre/modules/HUDUtils.jsm");

Per my earlier comment, why not pull all these in via the preprocessor, turn this into a single jsm.

My new tattoo is going to be "s/XPCOM//".

Comment 41

[Robert Kaiser]

(In reply to comment #40)
> * for consistency in toolkit, in console/hudservice the files should be in
> subdirs:
>   * modules (for jsm)
>   * src (for your xpcom components)
>   * public (for your idl)

As ted to call me wrong on that, but IIRC the direction we were going recently is to do away with that way of distinguishing by "how it's built" and going more towards logical structure and throwing all of those into the same logically consistent directory.

Comment 42

[David Dahl :ddahl]

(In reply to comment #40)
> (From update of attachment 440923 [details] [diff] [review])

> * actually, why do you need an IDL and those xpcom components? why can't they
> be js modules?

Please see what hudservice.js and hudwindowobserver.js are doing. (these are the workhorses of this patch anyway) I am not sure how it would be possible to listen globally for exceptions and network activity without a service running, nor would it be possible to listen for global-content-window-created topic without the windowobserver service.


> My new tattoo is going to be "s/XPCOM//".

you gotta have a dream I guess

Comment 43

[David Dahl :ddahl]

Also, I should mention that Robcee and Johnath's original design is preserved in this patch and called for a "proper, threadsafe service" - or something to that effect.

> +function ConsoleStorage()
> +{

> here and throughout, start brace should go on the same line

Ah! you missed the sdwilsh-style-guide-grumble-fest! please see the review for the Inspector: 
search for "nit: opening brace on new line" in bug 547453 :)

Comment 44

[David Dahl :ddahl]

(In reply to comment #41)
> As ted to call me wrong on that, but IIRC the direction we were going recently
> is to do away with that way of distinguishing by "how it's built" and going
> more towards logical structure and throwing all of those into the same
> logically consistent directory.

This is exactly what I learned from rs when I was puzzled over the dir structure and makefiles. Doing away with the extra directory recursion speeds up the build process (a little) as well.

Comment 45

[Dietrich Ayala (:dietrich)]

(In reply to comment #43)
> Ah! you missed the sdwilsh-style-guide-grumble-fest! please see the review for
> the Inspector: 
> search for "nit: opening brace on new line" in bug 547453 :)

Ok, that's fine if they already approved that :)

(In reply to comment #44)
> > As ted to call me wrong on that, but IIRC the direction we were going recently
> > is to do away with that way of distinguishing by "how it's built" and going
> > more towards logical structure and throwing all of those into the same
> > logically consistent directory.
> 
> This is exactly what I learned from rs when I was puzzled over the dir
> structure and makefiles. Doing away with the extra directory recursion speeds
> up the build process (a little) as well.

I had no idea. No problem then :)

Comment 46

[Dietrich Ayala (:dietrich)]

(In reply to comment #42)
> > * actually, why do you need an IDL and those xpcom components? why can't they
> > be js modules?
> 
> Please see what hudservice.js and hudwindowobserver.js are doing. (these are
> the workhorses of this patch anyway) I am not sure how it would be possible to
> listen globally for exceptions and network activity without a service running,
> nor would it be possible to listen for global-content-window-created topic
> without the windowobserver service.

Some clarification:

1. You don't need to be an XPCOM service, you just need to be able be QI'd to
the right listener interfaces.

2. You also don't need to register as a category observer for a startup
notification. You can utilize nsBrowserGlue (in Firefox's case) to do that for
you.

And I'm still not convinced that you need to initialize any of this before the
console is loaded for the first time.

Comment 47

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

hey fellas!

Dietrich, being thread-safe and toolkitty was indeed one of the original requirements we imposed on David when he took this over. Our prototype relied on an XPCOM component for early startup and as a mechanism to receive messages from potentially any source. That was a design decision that informed this implementation. If you think we need to change direction, we should probably have a discussion outside of this bug.

I guess the nsBrowserGlue suggestion fits into the above as well. In order to grab exceptions and errors from startup, we need to be up and running before the browser's been initialized.

anyway, we should talk!

Comment 48

[David Dahl :ddahl]

After finally digesting what dietrich is saying, all I can say is wow. that is some very slick anti-xpcom ability in jsms. re-org in process!

Comment 49

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

yeah, seconded and agree! :)

Comment 50

[David Dahl :ddahl]

Attachment: HUD Console v0.1.5

Consolidation is working, Network logging is broken due to wrapper issue. The wrappers around the weakref'd loadGroups we keep in this.loadGroups are different when we compare them. the load group we get from the network activity is xpconnect wrapped and the one we store as a weakref is XPCWrappedNative once we call .get() when comparing it. Also need to figure out the best topic to use when window observer is init'd.

Comment 51

[Dietrich Ayala (:dietrich)]

Comment on attachment 442876 [details] [diff] [review]
HUD Console v0.1.5


>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
>@@ -7586,8 +7586,22 @@ var TabContextMenu = {
>     // XXXzeniko should't we just disable this item as we disable
>     // the tabbrowser-multiple items above - for consistency?
>     document.getElementById("context_undoCloseTab").hidden =
>       Cc["@mozilla.org/browser/sessionstore;1"].
>       getService(Ci.nsISessionStore).
>       getClosedTabCount(window) == 0;
>   }
> }
>+
>+// get HUDService singleton object from toolkit
>+Cu.import("resource://gre/modules/HUDService.jsm");

instead of importing at browser window load, should make the two exported objects lazy getters.

>+ * The Original Code is DevTools code
>+ *
>+ * The Initial Developer of the Original Code is
>+ *   Mozilla Corporation

s/Corporation/Foundation/

>+ * Portions created by the Initial Developer are Copyright (C) 2010
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   David Dahl <[e-mail]>

add "(original author)"

>+
>+var EXPORTED_SYMBOLS = ["HUDService", "HeadsUpDisplayUICommands",];

remove the dangling comma.

should be consistent and use either HUD or HeadsUpDisplay for both exported objects.

>+XPCOMUtils.defineLazyServiceGetter(this, "wm",
>+                                   "@mozilla.org/appshell/window-mediator;1",
>+                                   "nsIWindowMediator");

available via services.jsm

>+XPCOMUtils.defineLazyServiceGetter(this, "obsService",
>+                                   "@mozilla.org/observer-service;1",
>+                                   "nsIObserverService");

ditto

>+function HUD_SERVICE()
>+{
>+  this.application = "FIREFOX"; // wtf!!! set to null later? by what?// this.appName();
>+  log("this.application: " + this.application);
>+  if (this.application == "FIREFOX") {
>+    var mixins = new FirefoxApplicationHooks();
>+  }
>+  else if (this.application == "FENNEC") {
>+    // XXX: provide mixins for FENNEC
>+  }
>+  log("mixins >>>>>>>>>>>>>>>>>> " + mixins);
>+  for (var prop in mixins) {
>+    log(prop + ": " + mixins[prop]);
>+  }
>+  this.registerApplicationHooks(this.application, mixins);
>+  // log("mixins: " + this.mozApps[this.application]);
>+  log("mozApps: " + this.mozApps);
>+  this.mixins = this.mozApps[this.application];
>+  log("mozapps[app]" + this.mozApps[this.application]);
>+  log("mixins: " + this.mixins);
>+  this.activatedContexts = [];
>+  this._headsUpDisplays = {};
>+  this.displayRegistry = {};
>+  this.uriRegistry = {};
>+  this.loadGroups = {};
>+  this.removeMutationListeners = null;
>+  this.sequencer = null;
>+
>+  this.storage = new ConsoleStorage();
>+  this.filterPrefs = {};
>+  this.defaultFilterPrefs = this.storage.defaultDisplayPrefs;
>+  this.defaultGlobalConsolePrefs = this.storage.defaultGlobalConsolePrefs;

why all in the ctor instead of setting these defaults in the prototype?

also, should all be documented.

>+  currentContext: function HS_currentContext() {
>+    if (this.mozApps["FIREFOX"]) {
>+      var window = this.mozApps["FIREFOX"].getCurrentContext();
>+      return window;
>+    }
>+    else {
>+      throw new Error("Gecko application mixins are null");
>+    }
>+  },

there'll never be >1 app registered at one time, so this seems unnecessary. if Firefox, set the context as a property in the ctor and let the context be set by other apps via the registration hook?

>+  clearDisplay: function HS_clearDisplay(aId)
>+  {
>+    var displayNode = this.getOutputNodeById(aId);
>+    var outputNode = displayNode.querySelectorAll(".hud-output-node")[0];
>+    var children = outputNode.childNodes;
>+    var len = children.length;
>+    for (var i = 0; i < len; i++) {
>+      try {
>+        if (children[i]) {
>+          outputNode.removeChild(children[i]);
>+        }
>+      }
>+      catch (ex) {
>+        // XXX: wtf? getting a pointer exception!
>+        log(ex);
>+      }
>+    }
>+  },

nodelists are live, so your index (i) is going to be wrong at some point through the loop.

instead:

while(outputNode.firstChild) outputNode.removeChild(outputNode.firstChild);

>+
>+  /**
>+   * get a unique ID form the sequence generator

form

>+  /**
>+   * Check to see if global loggin is enabled

loggin

up to here.

Comment 52

[Dietrich Ayala (:dietrich)]

Comment on attachment 442876 [details] [diff] [review]
HUD Console v0.1.5


>diff --git a/browser/installer/package-manifest.in b/browser/installer/package-manifest.in
>--- a/browser/installer/package-manifest.in
>+++ b/browser/installer/package-manifest.in
>@@ -172,16 +172,17 @@
> @BINPATH@/components/feeds.xpt
> #ifdef MOZ_GTK2
> @BINPATH@/components/filepicker.xpt
> #endif
> @BINPATH@/components/find.xpt
> @BINPATH@/components/fuel.xpt
> @BINPATH@/components/gfx.xpt
> @BINPATH@/components/htmlparser.xpt
>+@BINPATH@/components/hudservice.xpt

can remove this, and all references to nsIHUDService.

Comment 53

[Dietrich Ayala (:dietrich)]

Comment on attachment 442876 [details] [diff] [review]
HUD Console v0.1.5

>+<!ENTITY hudConsoleCmd.label        "Heads Up Display">
>+<!ENTITY hudConsoleCmd.accesskey    "U">
>+<!ENTITY hudConsoleCmd.commandkey   "k">
>+

heh, i like the choice of coming right after the old console's shortcut :)

should run this by DAF to see if it conflicts w/ common extensions, or Linux peculiarities.

>+  updateEntry: function CS_updateEntry(aUUID)
>+  {
>+    // update an individual entry
>+  },

mark as TODO or remove if not needed?

>+/**
>+ * Firefox-specific Application Hooks.
>+ * Each Gecko-based application will need an object like this in
>+ *  order to use the Heads Up Display
>+ */
>+function FirefoxApplicationHooks()
>+{ }
>+
>+FirefoxApplicationHooks.prototype = {

i'll re-iterate my belief that this should be in /browser, set via a public api for doing so, and that a HUDApplicationHooks interface (term used loosely!) should be well documented.

>+///////////////////////////////////////////////////////////////////////////////
>+// HUDWindowObserver
>+///////////////////////////////////////////////////////////////////////////////

>+///////////////////////////////////////////////////////////////////////////////
>+// HUDConsoleObserver
>+///////////////////////////////////////////////////////////////////////////////

these observers are both singletons, so probably don't require prototype, etc.

there's also Firefox-specific code in there that should be pushed out into the ApplicationHook.

the category observer stuff is obsolete now that these aren't components. if browser.xul load is not early enough, have nsBrowserGlue init HUDService when it receives the right notifications

>+    var exposedAPIMethods = ["logMessage", "registerDisplay",
>+                             "unregisterDisplay", "appName",
>+                             "getHeadsUpDisplay", "getDisplayByURISpec",
>+                             "getDisplayByLoadGroup", "displays",
>+                             "displaysIndex", "getDisplayRegistry",
>+                             "getURIRegistry", "getHUDIdsForURISpec",
>+                             "initializeJSTerm", "getLoadGroup",
>+                             "updateLoadGroup", "attachMutationListeners",
>+                             "updateTerminal", "sequenceId",
>+                             "registerGlobalOutputNode",
>+                             "reportConsoleServiceContentScriptError",
>+                             "reportConsoleServiceMessage",
>+                             "getDefaultFilterPrefs", "getFilterState",
>+                             "setFilterState", "currentContext",
>+                             "activateHUDForContext", "canActivateContext",
>+                             "clearDisplay",];
>+
>+    var len = exposedAPIMethods.length;
>+
>+    for (var i = 0; i < len; i++) {
>+      let _func = exposedAPIMethods[i];
>+      HUDService[_func] = function () {
>+        return hs[_func].apply(hs, arguments);
>+      };
>+    }

this seems unnecessary at this point. we don't protect our js modules anywhere else in the codebase this way.

Comment 54

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

(In reply to comment #40)
> * for consistency in toolkit, in console/hudservice the files should be in
> subdirs:
>   * modules (for jsm)
>   * src (for your xpcom components)
>   * public (for your idl)

We're actually going the other way - the public/src/modules hierarchy is more trouble than it's worth for relatively small components, so in general I prefer just putting everything in the top level.

Comment 55

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

Bah, I should read all of my bugmail before commenting. I see that's already been covered - nevermind!

Comment 56

[David Dahl :ddahl]

(In reply to comment #51)
> (From update of attachment 442876 [details] [diff] [review])
> >+// get HUDService singleton object from toolkit
> >+Cu.import("resource://gre/modules/HUDService.jsm");
> 
> instead of importing at browser window load, should make the two exported
> objects lazy getters.
> 
indeed, done.

> should be consistent and use either HUD or HeadsUpDisplay for both exported
> objects.
> 
Changed them both to HUD*

> 
> >+function HUD_SERVICE()
> >+{
> >+...
> >+  this.storage = new ConsoleStorage();
> >+  this.filterPrefs = {};
> >+  this.defaultFilterPrefs = this.storage.defaultDisplayPrefs;
> >+  this.defaultGlobalConsolePrefs = this.storage.defaultGlobalConsolePrefs;
> 
> why all in the ctor instead of setting these defaults in the prototype?

I moved most of these into the prototype, left the storage-related few in the ctor.

> also, should all be documented.
indeed. working on it.
> 
> >+  currentContext: function HS_currentContext() {
> >+    if (this.mozApps["FIREFOX"]) {
> >+      var window = this.mozApps["FIREFOX"].getCurrentContext();
> >+      return window;
> >+    }
> >+    else {
> >+      throw new Error("Gecko application mixins are null");
> >+    }
> >+  },
> 
> there'll never be >1 app registered at one time, so this seems unnecessary. if
> Firefox, set the context as a property in the ctor and let the context be set
> by other apps via the registration hook?

The context is looked up periodically and will not be the same all of the time. I think I may have changed it to this method when nothing else would work for me. This was a simplistic that needs to be streamlined for sure. I was putting all of the "gecko-independent" / "mixin" work on hold until a later milestone. I felt Like I had to play around with concepts in order to not paint myself in a corner. perhaps I add a comment to this effect until I get all of the "blocker" work out of the way?

> nodelists are live, so your index (i) is going to be wrong at some point
> through the loop.
> 
> instead:
> 
> while(outputNode.firstChild) outputNode.removeChild(outputNode.firstChild);

Works like a charm. thanks.

Comment 57

[David Dahl :ddahl]

(In reply to comment #53)
> (From update of attachment 442876 [details] [diff] [review])
> >+<!ENTITY hudConsoleCmd.label        "Heads Up Display">
> >+<!ENTITY hudConsoleCmd.accesskey    "U">
> >+<!ENTITY hudConsoleCmd.commandkey   "k">
> >+
> 
> heh, i like the choice of coming right after the old console's shortcut :)
> 
Oh yeah! snarky!

> should run this by DAF to see if it conflicts w/ common extensions, or Linux
> peculiarities.

Ok, will craft an email later today.

> 
> >+  updateEntry: function CS_updateEntry(aUUID)
> >+  {
> >+    // update an individual entry
> >+  },
> 
> mark as TODO or remove if not needed?

marked TODO

> 
> >+/**
> >+ * Firefox-specific Application Hooks.
> >+ * Each Gecko-based application will need an object like this in
> >+ *  order to use the Heads Up Display
> >+ */
> >+function FirefoxApplicationHooks()
> >+{ }
> >+
> >+FirefoxApplicationHooks.prototype = {
> 
> i'll re-iterate my belief that this should be in /browser, set via a public api
> for doing so, and that a HUDApplicationHooks interface (term used loosely!)
> should be well documented.

That sounds like a good policy. I was keeping all of the "mixin" polish for after this current milestone, at least until after all real "blockers" are put to bed.

> 
> >+///////////////////////////////////////////////////////////////////////////////
> >+// HUDWindowObserver
> >+///////////////////////////////////////////////////////////////////////////////
> 
> >+///////////////////////////////////////////////////////////////////////////////
> >+// HUDConsoleObserver
> >+///////////////////////////////////////////////////////////////////////////////
> 
> these observers are both singletons, so probably don't require prototype, etc.

Don't I still need an "observe" method in the prototype? How else would I accomplish this?

> 
> there's also Firefox-specific code in there that should be pushed out into the
> ApplicationHook.
> 
> the category observer stuff is obsolete now that these aren't components. if
> browser.xul load is not early enough, have nsBrowserGlue init HUDService when
> it receives the right notifications

again, would love some pseudo-code on how to deal with this. i'm not sure what you mean. (what else is new?:))

> 
> >+    var exposedAPIMethods = ["logMessage", "registerDisplay",
> >+                             "unregisterDisplay", "appName",
> >+                             "getHeadsUpDisplay", "getDisplayByURISpec",
> >+                             "getDisplayByLoadGroup", "displays",
> >+                             "displaysIndex", "getDisplayRegistry",
> >+                             "getURIRegistry", "getHUDIdsForURISpec",
> >+                             "initializeJSTerm", "getLoadGroup",
> >+                             "updateLoadGroup", "attachMutationListeners",
> >+                             "updateTerminal", "sequenceId",
> >+                             "registerGlobalOutputNode",
> >+                             "reportConsoleServiceContentScriptError",
> >+                             "reportConsoleServiceMessage",
> >+                             "getDefaultFilterPrefs", "getFilterState",
> >+                             "setFilterState", "currentContext",
> >+                             "activateHUDForContext", "canActivateContext",
> >+                             "clearDisplay",];
> >+
> >+    var len = exposedAPIMethods.length;
> >+
> >+    for (var i = 0; i < len; i++) {
> >+      let _func = exposedAPIMethods[i];
> >+      HUDService[_func] = function () {
> >+        return hs[_func].apply(hs, arguments);
> >+      };
> >+    }
> 
> this seems unnecessary at this point. we don't protect our js modules anywhere
> else in the codebase this way.

Mossop does indeed do this kind of thing in the EM to keep extension developers from using apis that should be totally private. I don't think this is absolutely needed either, was just being "safe".  I have removed it.

Comment 58

[David Dahl :ddahl]

Attachment: WIP HUD Console 0517

Saving my work, as I may have destroyed my local hg repo

Comment 59

[Dietrich Ayala (:dietrich)]

Comment on attachment 442876 [details] [diff] [review]
HUD Console v0.1.5

canceling feedback requests on the old patch

Comment 60

[David Dahl :ddahl]

Attachment: 0.1.6 HUDConsole patch for 1st pass review

Comment 61

[David Dahl :ddahl]

There are a few issues to be aware of when reviewing:

1. You can turn on the DOM Mutation Listeners, but not off, as I am still working on the callback function storage, retrieval and destruction

2. Not all HTTP connections can be matched up with the corresponding console display, namely, IMAGES. this is due to all images being in a "synthetic loadGroup" see bug 568034

3. Not all CSS parser errors or JS errors can be surfaced into the corresponding console as the nsIConsoleMessages do not have any reference to  the contentWindow where they were spawned. see bug 567165

4. Some tests are broken in browser_HUDServiceTests.js. Not sure what is going on, I continue to investigate. seems like something is asserting a bunch and is going into a loop. nuts! ctrl-c kills it and the tests seem to pass. i will update this asap.

5. The mutation test never runs because of some event handler weirdness in my browser test.

6. I will kick off a try build now, as my local repo is 1 week out of date with trunk. I will also merge with m-c asap.

I will add more details as I think of them or come across them.

Comment 62

[Dietrich Ayala (:dietrich)]

Comment on attachment 447642 [details] [diff] [review]
0.1.6 HUDConsole patch for 1st pass review

overall comment: i think it's time to resolve all the XXX/TODO items in the patch. either fix them, or if they don't block beta then file a bug and note the bug number in the comment.

+XPCOMUtils.defineLazyGetter(this, "HUDService", function () {
+  // get HUDService from toolkit
+  Cu.import("resource://gre/modules/HUDService.jsm");
+  try {
+    return HUDService;
+  }
+  catch (ex) {
+    dump(ex + "\n");
+  }
+});

is this used anywhere other than in the HUDConsoleUI getter?

diff --git a/toolkit/components/console/hudservice/ConsoleStorage.jsm b/toolkit/components/console/hudservice/ConsoleStorage.jsm
new file mode 100644
--- /dev/null
+++ b/toolkit/components/console/hudservice/ConsoleStorage.jsm

why is this in a separate jsm from the service?

+// ConsoleEntry tracks the details of each logged event or message
+// constructor takes an object literal that requires "type" and "message"
+// network events will have 1 entry with a "transactions" property that
+// details each http sub-transaction

hard to follow this - add proper punctuation?

+var EXPORTED_SYMBOLS = ["HUDService", "HUDConsoleUI",];
+
+XPCOMUtils.defineLazyServiceGetter(this, "ioSvc",
+                                   "@mozilla.org/network/io-service;1",
+                                   "nsIIOService");
+
...

+XPCOMUtils.defineLazyServiceGetter(this, "sss",
+                                   "@mozilla.org/content/style-sheet-service;1",
+                                   "nsIStyleSheetService");

some of these are in services.jsm.

actually, i said that in my last review. please do another pass on those review comments. outside of the comments below, i'm going to hold off reviewing further until those comments are addressed.

+ConsoleUtils.prototype = {
+
+  /**
+   * Generates a miliisecond resolution timestamp for console messages

millisecond

+   *
+   * @returns string
+   */
+  timeStamp: function Console_timeStamp()

s/Console/ConsuleUtils/

+//////////////////////////////////////////////////////////////////////////
+// HeadsUpDisplayUICommands
+//////////////////////////////////////////////////////////////////////////
+
+HeadsUpDisplayUICommands = {

let HeadsUpDisplayUICommands = {


+HUDWindowObserver = {

let HUDWindowObserver = {

+  QueryInterface: XPCOMUtils.generateQI(
+    [Ci.nsIObserver,]
+  ),
+
+  observe: function observe_func(aSubject, aTopic, aData)

s/observe_func/HWO_observe/

+// create hwo (HUDWindowObserver) variable here to reference inside the
+// HUDService and elsewhere
+var hwo;
+
+/**
+ * HUDWindowObs starts the window observer when the HUDService is initiated
+ *
+ */
+function HUDWindowObs() {
+  hwo = HUDWindowObserver;
+  Services.obs.addObserver(hwo, "content-document-global-created", false);
+  var win = HUDService.currentContext();
+  hwo.observe(win, "hud-init", null);
+}

this doesn't appear necessary. why not not just make this HWO.init() and call that from the HUDService ctor?

+
+///////////////////////////////////////////////////////////////////////////////
+// HUDConsoleObserver
+///////////////////////////////////////////////////////////////////////////////
+
+/**
+ * HUDConsoleObserver: observe nsIConsoleService for global consoleMessages
+ * route them to the HUDService if the consoleMessage originates outside of
+ * the HUDService, ignore messages created by the HUDService.
+ *
+ */
+
+HUDConsoleObserver = {

let HUDConsoleObserver = {

+    let categories = ["component javascript",
+                      "XUL javascript",
+                      "XPConnect JavaScript",];

unused?

+
+    if (aSubject instanceof Ci.nsIConsoleMessage) {
+      var err = aSubject.QueryInterface(Ci.nsIScriptError);
+      switch (err.category) {
+        case "XPConnect JavaScript":
+        case "component javascript":
+        case "chrome javascript":
+          // log("ignoring an error from xpconnect: \n" + err);
+          // XXX: ignore these two kinds of errors for now
+          return;

there are three errors there, and add a note why you're ignoring them.

is this something that needs to be fixed for beta? if not, file a bug for this.

+/**
+ * HUDConsoleObs starts the console observer when the
+ * HUDService is initiated
+ *
+ */
+function HUDConsoleObs() {
+  Services.obs.addObserver(HUDConsoleObserver,
+                           "content-document-global-created", false);
+}

ditto as for the window observer initialization comment above

+
+///////////////////////////////////////////////////////////////////////////
+// appName
+///////////////////////////////////////////////////////////////////////////
+
+/**
+ * Get the app's name so we can properly dispatch app-specific
+ * methods per API call
+ * @returns Geko application name

Gecko

+///////////////////////////////////////////////////////////////////////////
+// HUDService  and ConsoleUI (exported symbols)
+///////////////////////////////////////////////////////////////////////////

s/ConsoleUI/HUDConsoleUI/

+
+try {
+  var HUDService = new HUD_SERVICE();
+  // start window and console observers
+  HUDWindowObs();
+  HUDConsoleObs();
+}
+catch (ex) {
+  dump("*** HUDService failed initialization...\n");
+  dump("*** " + ex + "\n");
+}

s/dump/Components.utils.reportError/

What can throw from those three things? If each can throw, should put them all in separate try blocks.

(actually, see the above comments on folding those into the HUDService ctor)

Comment 63

[David Dahl :ddahl]

latest try builds: http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-8fcfa9ff98e4/

Comment 64

[David Dahl :ddahl]

Another thing that is broken: the HttpActivityObserver just falls down when you open a second tab, and we get no network traffic logging. I think this started when we switched from an XPCOM svc to a JSM.

Comment 65

[David Dahl :ddahl]

(In reply to comment #62)
> (From update of attachment 447642 [details] [diff] [review])
> overall comment: i think it's time to resolve all the XXX/TODO items in the
> patch. either fix them, or if they don't block beta then file a bug and note
> the bug number in the comment.
> 
Gotcha. Johnath asked me to get this patch submitted for review with the broken stuff, regardless. I just need to get all followup bugs filed. I will re-submit the patch later today.

Comment 66

[David Dahl :ddahl]

Comment on attachment 442876 [details] [diff] [review]
HUD Console v0.1.5

>+<!ENTITY hudConsoleCmd.label        "Heads Up Display">
>+<!ENTITY hudConsoleCmd.accesskey    "U">
>+<!ENTITY hudConsoleCmd.commandkey   "k">
>+

> should run this by DAF to see if it conflicts w/ common extensions, or Linux
> peculiarities.

still need to run this Key Command choice by DAF.

>+  updateEntry: function CS_updateEntry(aUUID)
>+  {
>+    // update an individual entry
>+  },

> mark as TODO or remove if not needed?

created a bug 568634


(In reply to comment #62)
> (From update of attachment 447642 [details] [diff] [review])
> overall comment: i think it's time to resolve all the XXX/TODO items in the
> patch. either fix them, or if they don't block beta then file a bug and note
> the bug number in the comment.
> 
I have filed the bugs and notated them in the patch. I am still weeding down which are blockers.

> +XPCOMUtils.defineLazyGetter(this, "HUDService", function () {
> +  // get HUDService from toolkit
> +  Cu.import("resource://gre/modules/HUDService.jsm");
> +  try {
> +    return HUDService;
> +  }
> +  catch (ex) {
> +    dump(ex + "\n");
> +  }
> +});
> 
> is this used anywhere other than in the HUDConsoleUI getter?

No it is not. Mossop said it might make sense to just init it when you load the resource, I tried to use the method outlined in teh previous feedback, but I could not get the getter to work.

> diff --git a/toolkit/components/console/hudservice/ConsoleStorage.jsm
> b/toolkit/components/console/hudservice/ConsoleStorage.jsm
> new file mode 100644
> --- /dev/null
> +++ b/toolkit/components/console/hudservice/ConsoleStorage.jsm
> 
> why is this in a separate jsm from the service?

The test suite is an xpcshell test suite. I have filed a bug to re-integrate it into the HUDService.jsm and re-factor the tests.

> +// ConsoleEntry tracks the details of each logged event or message
> +// constructor takes an object literal that requires "type" and "message"
> +// network events will have 1 entry with a "transactions" property that
> +// details each http sub-transaction
> 
> hard to follow this - add proper punctuation?
> 
I changed it into a proper function comment, ditching that verbiage as it was fo rme to understand what I was writing.

> +var EXPORTED_SYMBOLS = ["HUDService", "HUDConsoleUI",];
> +
> +XPCOMUtils.defineLazyServiceGetter(this, "ioSvc",
> +                                   "@mozilla.org/network/io-service;1",
> +                                   "nsIIOService");
> +
> ...
> 
> +XPCOMUtils.defineLazyServiceGetter(this, "sss",
> +                                  
> "@mozilla.org/content/style-sheet-service;1",
> +                                   "nsIStyleSheetService");
> 
> some of these are in services.jsm.

using "Services.io" now

> +// create hwo (HUDWindowObserver) variable here to reference inside the
> +// HUDService and elsewhere
> +var hwo;
> +
> +/**
> + * HUDWindowObs starts the window observer when the HUDService is initiated
> + *
> + */
> +function HUDWindowObs() {
> +  hwo = HUDWindowObserver;
> +  Services.obs.addObserver(hwo, "content-document-global-created", false);
> +  var win = HUDService.currentContext();
> +  hwo.observe(win, "hud-init", null);
> +}
> 
> this doesn't appear necessary. why not not just make this HWO.init() and call
> that from the HUDService ctor?

I tried that, and I thought I had a chicken and egg problem, where the service was very unhappy without the HWO running first.
> 
> +
> +///////////////////////////////////////////////////////////////////////////////
> +// HUDConsoleObserver
> +///////////////////////////////////////////////////////////////////////////////
> +
> +/**
> + * HUDConsoleObserver: observe nsIConsoleService for global consoleMessages
> + * route them to the HUDService if the consoleMessage originates outside of
> + * the HUDService, ignore messages created by the HUDService.
> + *
> + */
> +
> +HUDConsoleObserver = {
> 
> let HUDConsoleObserver = {
> 
> +    let categories = ["component javascript",
> +                      "XUL javascript",
> +                      "XPConnect JavaScript",];
> 
> unused?
 No, this is used it gets all of the consoleService messages.

> 
> +
> +    if (aSubject instanceof Ci.nsIConsoleMessage) {
> +      var err = aSubject.QueryInterface(Ci.nsIScriptError);
> +      switch (err.category) {
> +        case "XPConnect JavaScript":
> +        case "component javascript":
> +        case "chrome javascript":
> +          // log("ignoring an error from xpconnect: \n" + err);
> +          // XXX: ignore these two kinds of errors for now
> +          return;
> 
> there are three errors there, and add a note why you're ignoring them.

clarified the note - we ignore any chrome errors as this is a web content-focused tool.

> 
> is this something that needs to be fixed for beta? if not, file a bug for this.
no. my bad for adding the "XXX" there.

> 
> +/**
> + * HUDConsoleObs starts the console observer when the
> + * HUDService is initiated
> + *
> + */
> +function HUDConsoleObs() {
> +  Services.obs.addObserver(HUDConsoleObserver,
> +                           "content-document-global-created", false);
> +}
> 
> ditto as for the window observer initialization comment above

These observers are central to the console's error and activity logging.

> +
> +try {
> +  var HUDService = new HUD_SERVICE();
> +  // start window and console observers
> +  HUDWindowObs();
> +  HUDConsoleObs();
> +}
> +catch (ex) {
> +  dump("*** HUDService failed initialization...\n");
> +  dump("*** " + ex + "\n");
> +}
> 
> s/dump/Components.utils.reportError/
> 
> What can throw from those three things? If each can throw, should put them all
> in separate try blocks.

If anything goes wrong, I want to destroy any objects created, filed a bug and annotated the source

> 
> (actually, see the above comments on folding those into the HUDService ctor)

I tried that, but to no avail. there was a severe chicken/egg issue

Comment 67

[David Dahl :ddahl]

Attachment: 0.1.7 HUDConsole patch

Filed all followup bugs and they are all notated in this patch.

Comment 68

[Dietrich Ayala (:dietrich)]

> > let HUDConsoleObserver = {
> > 
> > +    let categories = ["component javascript",
> > +                      "XUL javascript",
> > +                      "XPConnect JavaScript",];
> > 
> > unused?
>  No, this is used it gets all of the consoleService messages.

i meant the categories bit.

Comment 69

[David Dahl :ddahl]

(In reply to comment #68)
> > > let HUDConsoleObserver = {
> > > 
> > > +    let categories = ["component javascript",
> > > +                      "XUL javascript",
> > > +                      "XPConnect JavaScript",];
> > > 
> > > unused?
> >  No, this is used it gets all of the consoleService messages.
> 
> i meant the categories bit.

It is not used. good catch.

Comment 70

[David Dahl :ddahl]

Attachment: 0.1.8 HUDConsole patch

cleaned up how the HUD is launched, added UI commands to shut down each HUD. Spent some time working through the network image requests. (image logging is still not working)

Comment 71

[David Dahl :ddahl]

Attachment: 0.1.9 HUDConsole patch

re-combined ConsoleStorage.jsm, amounng other clean ups and tweaks. New try build is in progress

Comment 72

[Robert Sayre]

Hmm, I thought we decided that using nsIConsoleService was a bad way to go here, since it isn't actually working right, and doesn't associate the messages with their originators in the way we would like.

Is there a list of probes you have working and need/want somewhere?

Comment 73

[David Dahl :ddahl]

(In reply to comment #72)
> Hmm, I thought we decided that using nsIConsoleService was a bad way to go
> here, since it isn't actually working right, and doesn't associate the messages
> with their originators in the way we would like.
> 
correct. I am only using it right now to pick up some errors that can be associated because they are happening directly on the page being observed. All of the nsIConsoleListener code will go away.

> Is there a list of probes you have working and need/want somewhere?

I implemented your idea of overriding "onerror", which works pretty good. bz says that it will not pick up on css errors though.
http://hg.mozilla.org/users/ddahl_mozilla.com/heads-up-display/file/37324bf6edf2/toolkit/components/console/hudservice/HUDService.jsm#l193

probes wanted for each contentWindow:
JS Warning
JS Error
JS Strict, etc...
CSS Parser Error

It'd be cool if the probe returns the window.ID, the error message, the stack, the file, line and column

Something like this would make a huge difference and no doubt simplify the console a bit. 

Perhaps we should convert this bug 567165 to "Create contentWindow probes for JS errors and warnings" or something like that

Comment 74

[David Dahl :ddahl]

Attachment: 0,2 HUDConsole0608.diff

Tweaked CSS for button problems on MacOS X, fixed derregister problem where the splitter is left as cruft in the DOM. Fixed a check in a test that doesn't quit properly.

Comment 75

[Dietrich Ayala (:dietrich)]

Attachment: some review comments

Comment 76

[David Dahl :ddahl]

Attachment: review comments response

new patch is coming momentarily.

Comment 77

[David Dahl :ddahl]

Attachment: 0.2.1 HUDConsole Patch

Comment 78

[Dietrich Ayala (:dietrich)]

Attachment: review comments on 0611.

Comment 79

[David Dahl :ddahl]

In reply to attached comments:
> +  this.registerApplicationHooks(this.application, mixins);
> +  this.mixins = this.mozApps[this.application];

> you get mixins above, and then register it, but then get it from mozApps and > set it as .mixins? why not just this.mixins = mixins?

> actually, why is .applicationHooks a hash? why would >1 application hooks > ever be registered simultaneously?
>
> it looks like you could just do this.mixins = mixins, and just get rid of all > the applicationHooks stuff.

The point was to have all (Fennec, Tbird and Fx) mixin objects in this file , hence the need for the "appName()" function to sniff what Gecko app we are running in before completely initializing. 

I will simplifiy this to mixins = mixins. and revisit in the followup bug 568621

Comment 80

[David Dahl :ddahl]

+  getLoadContextFromChannel: function HS_getLoadContextFromChannel(aChannel)
+  {
+    try {
+      return aChannel.QueryInterface(Ci.nsIChannel).notificationCallbacks.getInterface(Ci.nsILoadContext);
+    }
+    catch (e) {}
+    try {
+      return aChannel.QueryInterface(Ci.nsIChannel).loadGroup.notificationCallbacks.getInterface(Ci.nsILoadContext);
+    }
+    catch (e) {}
+    return null;
+  },

> unless this can throw for totally valid reasons, should at least log the 
exceptions via Cu.reportError.

This was a snippet i got directly from bz, so I left all of the try/catch alone. I will update with Cu.reportError

Comment 81

[Boris Zbarsky [:bzbarsky]]

The try above could throw if the channel's notification callbacks are null.  Which is going to be the case for pretty much all loads associated with a webpage except XHR loads, image loads, and document loads.  It could also throw if the channel's callbacks don't have an nsILoadContext (so will throw for XHR loads).

The second try above could throw if the loadgroup has no notification callbacks or if those don't have an nsILoadContext.  That doesn't come up with web page loads, but does with various things extensions do, perfectly legitimately (the loads are not attached to a window).  Oh, happens for HTML5 app cache stuff too, and maybe a few other things.

Bottom line: logging in the catch there is probably not worth it for the latter try/catch and definitely wrong for the former try/catch.

Comment 82

[David Dahl :ddahl]

Attachment: 0.2.2 HUDConsole Patch

going back through comments now, will attach asap

Comment 83

[David Dahl :ddahl]

Attachment: 0.2.3 HUDConsole Patch

disregard previous patch

Comment 84

[David Dahl :ddahl]

Attachment: 0.2.4 HUDConsole Patch

switched a bunch of log() to use Cu.reportError(), fixed mac toolbar css, added a "shutdown" function to HUDService

Comment 85

[David Dahl :ddahl]

Attachment: review comments response 2

attach review comments response

Comment 86

[David Dahl :ddahl]

Attachment: 0.2.5 HUDConsole Patch

Esentialy the same as previous patch, but I have fixed all of the tests to reflect new apis and removed methods, and all tests now run as a suite with out a timeout error. yay.

Comment 87

[David Dahl :ddahl]

(In reply to comment #86)
> Created an attachment (id=451400) [details]
> 0.2.5 HUDConsole Patch
> 
> Esentialy the same as previous patch, but I have fixed all of the tests to
> reflect new apis and removed methods, and all tests now run as a suite with out
> a timeout error. yay.


ALSO: my console user repo has been merged with M-C

Comment 88

[Dietrich Ayala (:dietrich)]

Attachment: review comments on 0615

Comment 89

[David Dahl :ddahl]

Attachment: review comment response 3

Comment 90

[David Dahl :ddahl]

Attachment: 0.2.6 HUDConsole Patch

Will be asking for security review from mrbkap as well on the jsterm Cu.evalInSandbox bits.

Comment 91

[David Dahl :ddahl]

mrbkap:

Will you review 2 methods in the JSTerm prototype:

http://hg.mozilla.org/users/ddahl_mozilla.com/heads-up-display/file/d440e5296103/toolkit/components/console/hudservice/HUDService.jsm#l2117

the createSandbox and execute methods, respectively.

I have a build for you to use if needed.

Comment 92

[David Dahl :ddahl]

In the latest patch I try to get at Cc, Cu, etc:

Components
[object nsXPCComponents @ 0xaa75a900 (native @ 0xaa75a880)]13:56:40:180: 

13:56:46:020: Error: Permission denied for <file://> to get property XPCComponents.classesComponents.interfaces[object nsXPCComponents_Interfaces @ 0xaa7b9980 (native @ 0xa8f03a00)]

13:57:00:836: Error: Permission denied for <file://> to get property XPCComponents.utils

13:57:07:699: Error: Permission denied for <file://> to get property XPCComponents.classes

It seems like it is all wrapped properly, no?

Comment 93

[David Dahl :ddahl]

(In reply to comment #92)
> In the latest patch I try to get at Cc, Cu, etc:

forgot to mention this is typing into the console's input.

Comment 94

[David Dahl :ddahl]

Attachment: 0.2.6 HUDConsole Patch - the real one

real patch now... whoops

Comment 95

[Dietrich Ayala (:dietrich)]

Attachment: review comments on 0617

Comment 96

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 452092 [details] [diff] [review]
0.2.6 HUDConsole Patch - the real one

Note: I talked to ddahl on IRC, just posting here for posterity.

>+  createSandbox: function JST_setupSandbox()
>+  {
>+    // create a JS Sandbox out of this.context
>+    this._window.wrappedJSObject.jsterm = {};
>+    this.console = this._window.wrappedJSObject.console;

I think the only reason you need to do this is that evalInSandbox is getting in your way and ignoring the .wrappedJSObject on the line setting __proto__ below.

ddahl'll file a bug on evalInSandbox to not do that.

>+      if (str != "undefined" &&
>+        result.toString() != "undefined" &&
>+        result != undefined) {

This doesn't look like what you want... I'd write this as:

if (result !== undefined) {
}

note: this will output "null", which is what the JS engine's shell does and follows javascript: behavior.

>+    catch (ex) {
>+      if (!(ex.toString() == undefined)) {

this probably wants to be:

if (ex) {

r=me with those comments addressed.

Comment 97

[David Dahl :ddahl]

Attachment: review comments 4

Comment 98

[David Dahl :ddahl]

Attachment: 0.2.7 HUDConsole patch

hmmm. tests pass when run individually but not as a group.

Comment 99

[David Dahl :ddahl]

i still need to address mrbkap's comments as well.

Comment 100

[David Dahl :ddahl]

Attachment: 0.2.7 HUDConsole patch

Updated patch with mrbkap's comments, combined all tests into a single file. all tests pass.

Comment 101

[Dietrich Ayala (:dietrich)]

Comment on attachment 452186 [details] [diff] [review]
0.2.7 HUDConsole patch

r=me with the last few changes talked about on irc.

Comment 102

[David Dahl :ddahl]

Attachment: 0.2.8 HUDConsole patch

changed as per Dietrich's irc notes

Comment 103

[Dietrich Ayala (:dietrich)]

http://hg.mozilla.org/mozilla-central/rev/b805434e7e4b

landed. will close this out if it sticks.

Comment 104

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Backed out:
http://hg.mozilla.org/mozilla-central/rev/063826c84e20
http://hg.mozilla.org/mozilla-central/rev/297b7471e5e7
because it caused leaks in the mochitest-browser-chrome test for all three debug mochitest-other runs that have completed so far.

Comment 105

[David Dahl :ddahl]

Attachment: 0.2.9 HUDConsole Patch

Added more shutdown/reference release methods. Now I keep track of all HeadsUpDisplay objects via weakRef. Locally all leaks are now gone, but try server is hosed. The last run through, the linux leaks went away but the mac and windows leak runs crashed b4 hg could pull.

Comment 106

[Dietrich Ayala (:dietrich)]

interdiff: https://bugzilla.mozilla.org/attachment.cgi?oldid=452337&action=interdiff&newid=452612&headers=1

Comment 107

[Dietrich Ayala (:dietrich)]

--- a/toolkit/components/console/hudservice/HUDService.jsm	
+++ a/toolkit/components/console/hudservice/HUDService.jsm	
@@ -298,7 +298,10 @@ 
 
     this.unregisterActiveContext(hudId);
     this.unregisterDisplay(hudId);
+    this.deleteHeadsUpDisplay(hudId);
+    this.storage.removeDisplay(hudId);
     window.wrappedJSObject.console = null;
+
   },

3 similarly-named methods in a row, with no hints as to what the difference is between them. definitely need some descriptive renaming here.

 
@@ -403,6 +406,36 @@ 
   },
 
   /**
+   * Keeps a weka refernec for each HeadsUpDisplay that is created

typos

+   *
+   */
+  hudWeakReferences: {},
+
+  /**
+   * Register a weak reference of each HeadsUpDisplay that is created
+   *
+   * @param object aHUDRef
+   * @param string aHUDId
+   * @returns void
+   */
+  registerHUDWeakReference:
+  function HS_registerHUDWeakReference(aHUDRef, aHUDId)
+  {
+    this.hudWeakReferences[aHUDId] = aHUDRef;
+  },
+
+  /**
+   * Deletes a HeadsUpDisplay object from memory
+   *
+   * @param string aHUDId
+   * @returns void
+   */
+  deleteHeadsUpDisplay: function HS_deleteHeadsUpDisplay(aHUDId)
+  {
+    delete this.hudWeakReferences[aHUDId].get();
+  },

i thought the whole point of weakrefs is that they're non-owning, so you shouldn't have to clean them up. maybe the reason this fixes the leak has nothing to do with your weakref stuff, and just that you're now tracking and deleting the huds at all?

if you properly clean up each hud when it's context goes away then you shouldn't need any of this.

@@ -463,10 +496,15 @@ 
         break;
       }
     }
+    // remove the DOM Nodes
     parent.removeChild(outputNode);
 
+    // remove our record of the DOM Nodes from the registry
     delete this._headsUpDisplays[aId];
 
+    // remove the HeadsUpDisplay object from memory
+    this.deleteHeadsUpDisplay(aId);
+

when is unregisterDisplay called? if it was called at all the right times (on every tab close event?), then you wouldn't need to build out and clean-up the list of weakrefs.

@@ -491,6 +529,18 @@ 
     for (var displayId in this._headsUpDisplays) {
       this.unregisterDisplay(displayId);
     }
+    for (var hudId in this.hudWeakReferences) {
+      delete this.deleteHeadsUpDisplay(hudId);
+    }

this is redundant, since unregisterDisplay does it.

+
+    delete this.storage;
+    delete this.sequencer;
+    delete this.activatedContexts;
+    delete this.displayRegistry;
+    delete this.loadGroups;
+    delete this.mutationEventFunctions;
+    delete this.filterPrefs;
+    delete this._headsUpDisplays;

now that you've got the patch to a point where it's not leaking, comment this out and selectively re-add so we can confirm if any of this is involved. if not, then it doesn't need to be there.

Comment 108

[David Dahl :ddahl]

Attachment: 0.3.0 HUDConsole Patch

recduced the leak-clearing code to a reasonable amount. tested on 3 platforms, no leaks.

Comment 109

[David Dahl :ddahl]

Interdiff:

https://bugzilla.mozilla.org/attachment.cgi?oldid=452337&action=interdiff&newid=452836&headers=1

Comment 110

[Dietrich Ayala (:dietrich)]

Comment on attachment 452836 [details] [diff] [review]
0.3.0 HUDConsole Patch


@@ -482,6 +526,22 @@ 
     for (var displayId in this._headsUpDisplays) {
       this.unregisterDisplay(displayId);
     }
+    for (var hudId in this.hudWeakReferences) {
+      delete this.deleteHeadsUpDisplay(hudId);
+    }

per irc, this can go away because it's done in unregisterDisplay

+    // delete the storage as it holds onto channels
+    delete this.storage;
+
+     var xulWindow = aContentWindow.QueryInterface(Ci.nsIInterfaceRequestor)
+      .getInterface(Ci.nsIWebNavigation)
+      .QueryInterface(Ci.nsIDocShellTreeItem)
+      .rootTreeItem
+      .QueryInterface(Ci.nsIInterfaceRequestor)
+      .getInterface(Ci.nsIDOMWindow);
+
+    let xulWindow = XPCNativeWrapper.unwrap(xulWindow);
+    let gBrowser = xulWindow.gBrowser;
+    gBrowser.tabContainer.removeEventListener("TabClose", this.onTabClose, false);

fix the indent

also, you redeclare xulWindow instead of just reassigning.

+   * Keeps track of whether onTabClose event Listener has been created
+   *
+   */
+  onTabCloseCreated: null,

registering same event listener more than once is a no-op, so just do that if there's not a reasonable way to detect it without keeping this around.

r=me with these changes

Comment 111

[David Dahl :ddahl]

Attachment: 0.3.1 HUDConsole Patch

r+ via dietrich on irc

Comment 112

[Dietrich Ayala (:dietrich)]

take two: http://hg.mozilla.org/mozilla-central/rev/aed5499f979d

keeping this open until it's confirmed sticking.

Comment 113

[David Dahl :ddahl]

it seems to have passed all tests, no leaks.

Comment 114

[Francesco Lodolo [:flod]]

Not sure if this is the right bug to ask: in headsUpDisplay.properties almost all button labels are in plural form (Warnings, Exceptions, Preferences). Why "Error" (btnError) and "Dom Mutation" (btnMutation) in singular form?

All those buttons are displayed on the same line, so it doesn't sound quite right.

Comment 115

[David Dahl :ddahl]

(In reply to comment #114)
> Not sure if this is the right bug to ask: in headsUpDisplay.properties almost
> all button labels are in plural form (Warnings, Exceptions, Preferences). Why
> "Error" (btnError) and "Dom Mutation" (btnMutation) in singular form?

Yeah, that is a problem, the UI is not final of course. I will file a bug on these toolbarbutton labels.

Comment 116

[Maniac Vlad Florin (:vladmaniac)]

Removing in-litmus? flag since this issue is covered by many tests in Litmus

Comment 117

[Simona Badau, Desktop QA]

Verified on: 
Build identifier: Mozilla/5.0 (Windows NT 6.1; rv:2.2a1pre) Gecko/20110404 Firefox/4.2a1pre

Comment 118

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

clearing some flags.