636169 - Web Console notifications needed for customization via Addons

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[David Dahl :ddahl]

We will need a couple of notifications for addons and other web console code to listen for to make the web console customizable.

I'm thinking web-console-created, web-console-destroyed, web-console-collapsed and web-console-opened

Comment 1

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

a couple off the top of my stack:

web-console-filters-changed
web-console-cleared
js-term-executed
js-term-helper-executed
web-console-message-created

... anything else?

what about a web-console-page-console-modified for when the window.console object gets paved over or modified?

Comment 2

[David Dahl :ddahl]

(In reply to comment #1)
> a couple off the top of my stack:
> 
> web-console-filters-changed
> web-console-cleared
> js-term-executed
> js-term-helper-executed
> web-console-message-created
> 
> ... anything else?
> 
> what about a web-console-page-console-modified for when the window.console
> object gets paved over or modified?

I like these, i wonder what the reviewer will think about so many...

Comment 3

[David Dahl :ddahl]

Attachment: v 0.1added several notifications

Added several notifications:

web-console-created
web-console-destroyed
web-console-page-console-modified
web-console-message-created

- the concept is that the DevTools SDK will provide a registry of events that can have callbacks - the callback will then be handed a simplified WebConsole object with an elegant API exposing only what needs to be exposed to Addon devs. It will be trivial to let addons know when a new console is opened so it can be modified, e.g:

DevToolsSDK.webconsole.onCreate(function(aWebConsoleWrapper){...});

or when a new message has been created:

DevToolsSDK.webconsole.onLogEvent(function(aLogNode){...});

etc...

Comment 4

[David Dahl :ddahl]

Attachment: v 0.2 removed log message

whoops. killed log() call

Comment 5

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

Comment on attachment 514551 [details] [diff] [review]
v 0.1added several notifications

per irc...

+    let id = new String(this.hudId);
+    log(this.hudId + "\n\n");

lose the log.

+                      nodeId: aNode.getAttribute("id") };
+    notifyObj.wrappedJSObject = notifyObj;

lose that line.

Comment 6

[David Dahl :ddahl]

Attachment: v 0.3 removed unneeded wrappedJSObject

Comment 7

[David Dahl :ddahl]

Attachment: v 0.4 - its only 4.6k!

Removed console modified observer notification due to trouble testing it - tests included, all pass.

Comment 8

[David Dahl :ddahl]

Comment on attachment 515170 [details] [diff] [review]
v 0.4 - its only 4.6k!

Drivers: I know there is no review yet... This is a tiny patch that will give the forthcoming DevTools SDK and the Addons SDK a way to know as soon as a console is created, destroyed or a message is logged to it. Addon authors (and the devtools team using the Addons SDK) will be able to easily extend the console and the console UI. Would love approval!

Comment 9

[David Dahl :ddahl]

Attachment: v 0.4.1 only 8.6 k, tiny!

added test to patch.

Comment 10

[Dave Townsend [:mossop]]

Comment on attachment 515176 [details] [diff] [review]
v 0.4.1 only 8.6 k, tiny!

Might be worth verifying that the node ID passed with the message creation is something useful.

Comment 11

[David Dahl :ddahl]

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

Comment 12

[David Dahl :ddahl]

After several hours of hacking on some SDK stuff for the console, I realized that this notification is sent just milliseconds before the HeadsUpDisplay object is registered with the HUDService. I do after all need to get a reference to the HeadsUpDisplay at the moment the notification comes in.

patch forthcoming

Comment 13

[David Dahl :ddahl]

Attachment: v 0.1 Moved notification after HUD registration

Comment 14

[David Dahl :ddahl]

http://hg.mozilla.org/mozilla-central/rev/3e456bd488af

Comment 15

[David Dahl :ddahl]

Attachment: v. 0.1 experimental for talos

Comment 16

[David Dahl :ddahl]

Comment on attachment 516448 [details] [diff] [review]
v. 0.1 experimental for talos

Pushed to try server, ran talos tests, all tests passed: http://tbpl.mozilla.org/?tree=MozillaTry&rev=e30fab3e4af7

I think this was a fluke. So strange.

Comment 17

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

there were a number of other talos regressions yesterday as well. Not pointing at this patch.

from dev.tree-management

Regression :( Ts, MAX Dirty Profile increase 14.5% on MacOSX 10.5.8 Firefox
---------------------------------------------------------------------------
   Previous: avg 709.790 stddev 19.505 of 30 runs up to revision b6fd106a4eb3
   New     : avg 812.432 stddev 21.975 of 5 runs since revision 4b7c9efbf269
   Change  : +102.642 (14.5% / z=5.262)
   Graph   : http://mzl.la/gBOaPc

Changeset range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b6fd106a4eb3&tochange=4b7c9efbf269

Changesets:
 * http://hg.mozilla.org/mozilla-central/rev/290403971149
   : Marco Bonardo <[e-mail]> - Bug 637957 - Make results notifier hold a strong reference to  mCallback, to prevent it being destroyed prematurely.
r=sdwilsh a=blocker
   : http://bugzilla.mozilla.org/show_bug.cgi?id=637957

 * http://hg.mozilla.org/mozilla-central/rev/7bc88a8f9095
   : Bjarne Herland <[e-mail]> - Bug 634906: Allow Allow-Origin:null in CORS. r/a=sicking
   : http://bugzilla.mozilla.org/show_bug.cgi?id=634906

 * http://hg.mozilla.org/mozilla-central/rev/4b7c9efbf269
   : Boris Zbarsky <[e-mail]> - Bug 637644.  Start layout on the new document in CreateAboutBlankContentViewer, since if we don't do it here no one ever will.  r=jst, a=blocker
   : http://bugzilla.mozilla.org/show_bug.cgi?id=637644

Bugs:
 * http://bugzilla.mozilla.org/show_bug.cgi?id=634906
 * http://bugzilla.mozilla.org/show_bug.cgi?id=637644
 * http://bugzilla.mozilla.org/show_bug.cgi?id=637957

Comment 18

[Asa Dotzler [:asa]]

Comment on attachment 516448 [details] [diff] [review]
v. 0.1 experimental for talos

too late for Firefox 4. sorry. (also, we generally don't approve unreviewed patches)

Comment 19

[David Dahl :ddahl]

(In reply to comment #18)
> Comment on attachment 516448 [details] [diff] [review]
> v. 0.1 experimental for talos
> 
> too late for Firefox 4. sorry. (also, we generally don't approve unreviewed
> patches)

Did you look at the patch? Its moving 2 lines to another method due to talos apparent flakiness.

Comment 20

[David Dahl :ddahl]

Comment on attachment 516448 [details] [diff] [review]
v. 0.1 experimental for talos

Looks like talos noise as the spike has gone away. also the try server test with this patch was not spiking either.