The default bug view has changed. See this FAQ.

Implement Heads Up Display console, js workspaces and related tools

VERIFIED FIXED

Status

()

Firefox
Developer Tools
VERIFIED FIXED
7 years ago
5 years ago

People

(Reporter: rc, Assigned: ddahl)

Tracking

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(9 attachments, 27 obsolete attachments)

18.20 KB, patch
Details | Diff | Splinter Review
11.83 KB, text/plain
Details
9.69 KB, patch
Details | Diff | Splinter Review
5.59 KB, text/plain
Details
13.28 KB, patch
Details | Diff | Splinter Review
11.04 KB, text/plain
Details
5.98 KB, patch
Details | Diff | Splinter Review
3.27 KB, text/plain
Details
135.20 KB, patch
ddahl
: review+
Details | Diff | Splinter Review
(Reporter)

Description

7 years ago
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().
Flags: wanted-firefox3.6?
(Reporter)

Updated

7 years ago
Component: General → Developer Tools
OS: Mac OS X → All
QA Contact: general → developer.tools
Hardware: x86 → All
(Reporter)

Updated

7 years ago
Blocks: 529086
(Reporter)

Comment 1

7 years ago
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.
(Assignee)

Updated

7 years ago
Assignee: nobody → ddahl
(Assignee)

Comment 2

7 years ago
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.
(Assignee)

Comment 3

7 years ago
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

7 years ago
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.
> 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

7 years ago
(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?
(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.
(Assignee)

Comment 8

7 years ago
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.
(Assignee)

Comment 9

7 years ago
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.
(Assignee)

Comment 10

7 years ago
Created attachment 433221 [details] [diff] [review]
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.
(Assignee)

Comment 11

7 years ago
Created attachment 433223 [details] [diff] [review]
0.1 browser patch

Needs clean up - HeadsUpDisplay.jsm is being removed "browser"
Version: unspecified → Trunk
Blocks: 467530
(Assignee)

Comment 12

7 years ago
See user repo here: http://hg.mozilla.org/users/ddahl_mozilla.com/heads-up-display/
(Assignee)

Comment 13

7 years ago
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.
(Assignee)

Comment 14

7 years ago
please ignore previous comment...

Looks like I forgot to setup the "DOMContentLoaded" event listener
(Assignee)

Comment 15

7 years ago
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
(Assignee)

Comment 16

7 years ago
Talked to blizzard today about Web Timing Spec at W3C. May be a good addition to our featureset/roadmap.
(Assignee)

Comment 17

7 years ago
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.
(Assignee)

Comment 18

7 years ago
(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
(Assignee)

Comment 19

7 years ago
Created attachment 434652 [details] [diff] [review]
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.
Attachment #433221 - Attachment is obsolete: true
Attachment #433223 - Attachment is obsolete: true
Attachment #434652 - Flags: feedback?(vladimir)
(Assignee)

Comment 20

7 years ago
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/
(Assignee)

Comment 21

7 years ago
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.
(Assignee)

Comment 22

7 years ago
Latest try build is here.

https://build.mozilla.org/tryserver-builds/ddahl@mozilla.com-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.
(Assignee)

Comment 23

7 years ago
Figured out http logging redundancies and started ConsoleStorage module

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

Comment 24

7 years ago
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
(Assignee)

Comment 25

7 years ago
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
(Assignee)

Comment 26

7 years ago
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.
If you're seeing those assertions, and if it's something web script can trigger as it wishes, then you have a security hole.
(Assignee)

Comment 28

7 years ago
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.
(Assignee)

Comment 29

7 years ago
Created attachment 437771 [details] [diff] [review]
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.
Attachment #437771 - Flags: review?
(Assignee)

Updated

7 years ago
Attachment #434652 - Attachment is obsolete: true
Attachment #434652 - Flags: feedback?(vladimir)
(Assignee)

Updated

7 years ago
Attachment #437771 - Flags: review? → review?(vladimir)
(Assignee)

Comment 30

7 years ago
Created attachment 439412 [details] [diff] [review]
HUD Console v0.1.1
Attachment #437771 - Attachment is obsolete: true
Attachment #439412 - Flags: review?
Attachment #437771 - Flags: review?(vladimir)
(Assignee)

Updated

7 years ago
Attachment #439412 - Flags: review? → review?(vladimir)
(Assignee)

Comment 31

7 years ago
Latest Try build is here: https://build.mozilla.org/tryserver-builds/ddahl@mozilla.com-HUDConsole0415/

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

Comment 32

7 years ago
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.
Status: NEW → ASSIGNED
(Assignee)

Updated

7 years ago
Summary: create top-level console object and associated APIs in content window → Implement Heads Up Display console, js workspaces and related tools
(Assignee)

Comment 33

7 years ago
Created attachment 439794 [details] [diff] [review]
HUD Console v0.1.2

Fixed coding style issues, removed dead code, added copious comments
Attachment #439412 - Attachment is obsolete: true
Attachment #439794 - Flags: review?(vladimir)
Attachment #439412 - Flags: review?(vladimir)
(Assignee)

Updated

7 years ago
Depends on: 559482
(Assignee)

Updated

7 years ago
Depends on: 559481
(Assignee)

Comment 34

7 years ago
Created attachment 440060 [details] [diff] [review]
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.
Attachment #439794 - Attachment is obsolete: true
Attachment #440060 - Flags: review?(bzbarsky)
Attachment #439794 - Flags: review?(vladimir)
(Assignee)

Updated

7 years ago
Attachment #440060 - Flags: feedback?(mrbkap)
(Assignee)

Comment 35

7 years ago
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:))
(Assignee)

Comment 36

7 years ago
Created attachment 440923 [details] [diff] [review]
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
Attachment #440060 - Attachment is obsolete: true
Attachment #440923 - Flags: review?(mrbkap)
Attachment #440060 - Flags: review?(bzbarsky)
Attachment #440060 - Flags: feedback?(mrbkap)

Comment 37

7 years ago
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.
(Assignee)

Comment 38

7 years ago
(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

7 years ago
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 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

7 years ago
(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.
(Assignee)

Comment 42

7 years ago
(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
(Assignee)

Comment 43

7 years ago
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 :)
(Assignee)

Comment 44

7 years ago
(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.
(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 :)
(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.
(Reporter)

Comment 47

7 years ago
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!
(Assignee)

Comment 48

7 years ago
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!
(Reporter)

Comment 49

7 years ago
yeah, seconded and agree! :)
(Assignee)

Comment 50

7 years ago
Created attachment 442876 [details] [diff] [review]
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.
Attachment #440923 - Attachment is obsolete: true
Attachment #442876 - Flags: review?(dietrich)
Attachment #442876 - Flags: feedback?(mrbkap)
Attachment #440923 - Flags: review?(mrbkap)
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 <ddahl@mozilla.com>

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 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 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.
(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.
Bah, I should read all of my bugmail before commenting. I see that's already been covered - nevermind!
(Assignee)

Comment 56

7 years ago
(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.
(Assignee)

Comment 57

7 years ago
(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.
(Assignee)

Comment 58

7 years ago
Created attachment 445880 [details] [diff] [review]
WIP HUD Console 0517

Saving my work, as I may have destroyed my local hg repo
Attachment #442876 - Flags: review?(dietrich)
Attachment #442876 - Flags: feedback?(mrbkap)
Comment on attachment 442876 [details] [diff] [review]
HUD Console v0.1.5

canceling feedback requests on the old patch
(Assignee)

Comment 60

7 years ago
Created attachment 447642 [details] [diff] [review]
0.1.6 HUDConsole patch for 1st pass review
Attachment #447642 - Flags: review?(dietrich)
(Assignee)

Comment 61

7 years ago
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 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)
Attachment #447642 - Flags: review?(dietrich) → review-
(Assignee)

Comment 63

7 years ago
latest try builds: http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/ddahl@mozilla.com-8fcfa9ff98e4/
(Assignee)

Comment 64

7 years ago
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.
(Assignee)

Comment 65

7 years ago
(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.
(Assignee)

Updated

7 years ago
Blocks: 568621
(Assignee)

Updated

7 years ago
Depends on: 568622
(Assignee)

Updated

7 years ago
Depends on: 568623
(Assignee)

Updated

7 years ago
Depends on: 568626
(Assignee)

Updated

7 years ago
Depends on: 568634
(Assignee)

Updated

7 years ago
Depends on: 568643
(Assignee)

Updated

7 years ago
Depends on: 568657
(Assignee)

Updated

7 years ago
Depends on: 568658
(Assignee)

Updated

7 years ago
Depends on: 568661
(Assignee)

Comment 66

7 years ago
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
(Assignee)

Comment 67

7 years ago
Created attachment 447896 [details] [diff] [review]
0.1.7 HUDConsole patch

Filed all followup bugs and they are all notated in this patch.
Attachment #442876 - Attachment is obsolete: true
Attachment #445880 - Attachment is obsolete: true
Attachment #447642 - Attachment is obsolete: true
Attachment #447896 - Flags: review?(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.
(Assignee)

Comment 69

7 years ago
(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.
(Assignee)

Comment 70

7 years ago
Created attachment 448921 [details] [diff] [review]
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)
Attachment #447896 - Attachment is obsolete: true
Attachment #448921 - Flags: review?(dietrich)
Attachment #447896 - Flags: review?(dietrich)
(Assignee)

Updated

7 years ago
No longer depends on: 559481, 559482, 568626, 568634, 568643, 568657, 568658, 568661, 568622, 568623
(Assignee)

Comment 71

7 years ago
Created attachment 449770 [details] [diff] [review]
0.1.9 HUDConsole patch

re-combined ConsoleStorage.jsm, amounng other clean ups and tweaks. New try build is in progress
Attachment #448921 - Attachment is obsolete: true
Attachment #449770 - Flags: review?(dietrich)
Attachment #448921 - Flags: review?(dietrich)

Comment 72

7 years ago
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?
(Assignee)

Comment 73

7 years ago
(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
(Assignee)

Comment 74

7 years ago
Created attachment 450004 [details] [diff] [review]
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.
Attachment #449770 - Attachment is obsolete: true
Attachment #450004 - Flags: review?(dietrich)
Attachment #449770 - Flags: review?(dietrich)
Created attachment 450462 [details] [diff] [review]
some review comments
(Assignee)

Comment 76

7 years ago
Created attachment 450705 [details]
review comments response

new patch is coming momentarily.
(Assignee)

Comment 77

7 years ago
Created attachment 450724 [details] [diff] [review]
0.2.1 HUDConsole Patch
Attachment #450004 - Attachment is obsolete: true
Attachment #450724 - Flags: review?(dietrich)
Attachment #450004 - Flags: review?(dietrich)
(Assignee)

Updated

7 years ago
Depends on: 571634
Created attachment 451001 [details] [diff] [review]
review comments on 0611.
(Assignee)

Comment 79

7 years ago
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
(Assignee)

Comment 80

7 years ago
+  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
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.
(Assignee)

Comment 82

7 years ago
Created attachment 451104 [details] [diff] [review]
0.2.2 HUDConsole Patch

going back through comments now, will attach asap
Attachment #450724 - Attachment is obsolete: true
Attachment #451104 - Flags: review?(dietrich)
Attachment #450724 - Flags: review?(dietrich)
(Assignee)

Comment 83

7 years ago
Created attachment 451111 [details] [diff] [review]
0.2.3 HUDConsole Patch

disregard previous patch
Attachment #451104 - Attachment is obsolete: true
Attachment #451111 - Flags: review?(dietrich)
Attachment #451104 - Flags: review?(dietrich)
(Assignee)

Comment 84

7 years ago
Created attachment 451167 [details] [diff] [review]
0.2.4 HUDConsole Patch

switched a bunch of log() to use Cu.reportError(), fixed mac toolbar css, added a "shutdown" function to HUDService
Attachment #451111 - Attachment is obsolete: true
Attachment #451167 - Flags: review?(dietrich)
Attachment #451111 - Flags: review?(dietrich)
(Assignee)

Comment 85

7 years ago
Created attachment 451169 [details]
review comments response 2

attach review comments response
(Assignee)

Comment 86

7 years ago
Created attachment 451400 [details] [diff] [review]
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.
Attachment #451167 - Attachment is obsolete: true
Attachment #451400 - Flags: review?(dietrich)
Attachment #451167 - Flags: review?(dietrich)
(Assignee)

Comment 87

7 years ago
(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
Created attachment 451764 [details] [diff] [review]
review comments on 0615
(Assignee)

Comment 89

7 years ago
Created attachment 452043 [details]
review comment response 3
(Assignee)

Comment 90

7 years ago
Created attachment 452044 [details] [diff] [review]
0.2.6 HUDConsole Patch

Will be asking for security review from mrbkap as well on the jsterm Cu.evalInSandbox bits.
Attachment #451400 - Attachment is obsolete: true
Attachment #452044 - Flags: review?(dietrich)
Attachment #451400 - Flags: review?(dietrich)
(Assignee)

Comment 91

7 years ago
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.
(Assignee)

Comment 92

7 years ago
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?
(Assignee)

Comment 93

7 years ago
(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.
(Assignee)

Comment 94

7 years ago
Created attachment 452092 [details] [diff] [review]
0.2.6 HUDConsole Patch - the real one

real patch now... whoops
Attachment #452044 - Attachment is obsolete: true
Attachment #452092 - Flags: review?(dietrich)
Attachment #452044 - Flags: review?(dietrich)
Created attachment 452134 [details] [diff] [review]
review comments on 0617
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.
Attachment #452092 - Flags: review+
(Assignee)

Comment 97

7 years ago
Created attachment 452161 [details]
review comments 4
(Assignee)

Comment 98

7 years ago
Created attachment 452162 [details] [diff] [review]
0.2.7 HUDConsole patch

hmmm. tests pass when run individually but not as a group.
Attachment #452092 - Attachment is obsolete: true
Attachment #452162 - Flags: review?(dietrich)
Attachment #452092 - Flags: review?(dietrich)
(Assignee)

Comment 99

7 years ago
i still need to address mrbkap's comments as well.
(Assignee)

Comment 100

7 years ago
Created attachment 452186 [details] [diff] [review]
0.2.7 HUDConsole patch

Updated patch with mrbkap's comments, combined all tests into a single file. all tests pass.
Attachment #452162 - Attachment is obsolete: true
Attachment #452186 - Flags: review?(dietrich)
Attachment #452162 - Flags: review?(dietrich)
Comment on attachment 452186 [details] [diff] [review]
0.2.7 HUDConsole patch

r=me with the last few changes talked about on irc.
Attachment #452186 - Flags: review?(dietrich) → review+
(Assignee)

Comment 102

7 years ago
Created attachment 452337 [details] [diff] [review]
0.2.8 HUDConsole patch

changed as per Dietrich's irc notes
Attachment #452186 - Attachment is obsolete: true
Attachment #452337 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/b805434e7e4b

landed. will close this out if it sticks.
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.
(Assignee)

Comment 105

7 years ago
Created attachment 452612 [details] [diff] [review]
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.
Attachment #452337 - Attachment is obsolete: true
Attachment #452612 - Flags: review?(dietrich)
interdiff: https://bugzilla.mozilla.org/attachment.cgi?oldid=452337&action=interdiff&newid=452612&headers=1
--- 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.
Attachment #452612 - Flags: review?(dietrich) → review-
(Assignee)

Comment 108

7 years ago
Created attachment 452836 [details] [diff] [review]
0.3.0 HUDConsole Patch

recduced the leak-clearing code to a reasonable amount. tested on 3 platforms, no leaks.
Attachment #452612 - Attachment is obsolete: true
Attachment #452836 - Flags: review?(dietrich)
(Assignee)

Comment 109

7 years ago
Interdiff:

https://bugzilla.mozilla.org/attachment.cgi?oldid=452337&action=interdiff&newid=452836&headers=1
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
Attachment #452836 - Flags: review?(dietrich) → review+
(Assignee)

Comment 111

7 years ago
Created attachment 452928 [details] [diff] [review]
0.3.1 HUDConsole Patch

r+ via dietrich on irc
Attachment #452836 - Attachment is obsolete: true
Attachment #452928 - Flags: review+
take two: http://hg.mozilla.org/mozilla-central/rev/aed5499f979d

keeping this open until it's confirmed sticking.
(Assignee)

Comment 113

7 years ago
it seems to have passed all tests, no leaks.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
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.
(Assignee)

Comment 115

7 years ago
(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.
Flags: in-litmus?

Updated

7 years ago
Blocks: 574955

Updated

7 years ago
No longer blocks: 467530
Removing in-litmus? flag since this issue is covered by many tests in Litmus
Flags: in-litmus? → in-litmus+
Verified on: 
Build identifier: Mozilla/5.0 (Windows NT 6.1; rv:2.2a1pre) Gecko/20110404 Firefox/4.2a1pre
Status: RESOLVED → VERIFIED
(Reporter)

Comment 118

5 years ago
clearing some flags.
Flags: wanted-firefox3.6?

Updated

5 years ago
No longer blocks: 574955
Blocks: 574955
You need to log in before you can comment on or make changes to this bug.