749397 - Network Monitor should be embedded into Firefox

Status: RESOLVED INVALID

(DevTools :: General, defect, P1)

Comment 1

[Joe Walker [:jwalker]]

Attachment: Upload 1

Comment 2

[Joe Walker [:jwalker]]

Attachment: Upload 2

Working patch!

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

> +  // Enable network monitor?
> +  let devToolbarEnabled =
The var name should be rather 'netMonitorEnabled' instead of 'devToolbarEnabled'


Why NewHttpMonitor? It's just a workaround?

What about following:
//Options.initialize("extensions.httpmonitor");
//Options.registerDefaultPrefs(DefaultPrefs);

Should we use NetMonitor.jsm and httpmonitor/app/main as the place for specific intialization steps?


> this._panelWindow.foo = this._httpMonitor;
What foo is for?

> NetMonitor.prototype.handleEvent = function DT_handleEvent(aEvent)
Where this is used?

Honza

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Btw. where is the script that generates monitor.jsm?

Honza

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

I am also seeing:

JavaScript error: resource:///modules/devtools/NetMonitor.jsm, line 160: TypeError: this._panelWindow is undefined

When closing NetMonitor console.

I guess because:

this._onunload = this._onunload.bind(this);

is missing.

Honza

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Another exception:

JavaScript error: resource:///modules/devtools/NetMonitor.jsm, line 121: TypeErr
or: this._browser is undefined

STR
1) Open NetMonitor
2) Close NetMinotaur
3) Open NetMonitor --> Exception

Also there is a beast hidden in the STR (just a little prank in case you would be bored by my comments :)

Honza

Comment 7

[Joe Walker [:jwalker]]

(In reply to Jan Honza Odvarko from comment #3)
> > +  // Enable network monitor?
> > +  let devToolbarEnabled =
> The var name should be rather 'netMonitorEnabled' instead of
> 'devToolbarEnabled'
> 
> 
> Why NewHttpMonitor? It's just a workaround?

Yes, and a totally nasty one. I wanted to have access to the HttpMonitor variable at the higher scope. I'd like there to be a better option, but not put much thought into it so far. Suggestions welcome.

Perhaps the best place to discuss these types of changes is in the github branch for this?
https://github.com/joewalker/httpmonitor/commits/mozilla-build

> What about following:
> //Options.initialize("extensions.httpmonitor");
> //Options.registerDefaultPrefs(DefaultPrefs);
> 
> Should we use NetMonitor.jsm and httpmonitor/app/main as the place for
> specific intialization steps?

I think Firefox specific initialization should be done in NetMonitor.jsm and general init should be done in httpmonitor


> > this._panelWindow.foo = this._httpMonitor;
> What foo is for?

Testing. It should go.


> > NetMonitor.prototype.handleEvent = function DT_handleEvent(aEvent)
> Where this is used?

It's not. It can probably go.

Comment 8

[Joe Walker [:jwalker]]

(In reply to Jan Honza Odvarko from comment #4)
> Btw. where is the script that generates monitor.jsm?

https://github.com/joewalker/httpmonitor/blob/mozilla-build/build.js#L44

Comment 9

[Joe Walker [:jwalker]]

(In reply to Jan Honza Odvarko from comment #5)
> I am also seeing:
> 
> JavaScript error: resource:///modules/devtools/NetMonitor.jsm, line 160:
> TypeError: this._panelWindow is undefined
> 
> When closing NetMonitor console.
> 
> I guess because:
> 
> this._onunload = this._onunload.bind(this);
> 
> is missing.

type-type-type
*was* missing.

Thanks.

Comment 10

[Joe Walker [:jwalker]]

(In reply to Jan Honza Odvarko from comment #6)
> Another exception:
> 
> JavaScript error: resource:///modules/devtools/NetMonitor.jsm, line 121:
> TypeErr
> or: this._browser is undefined
> 
> STR
> 1) Open NetMonitor
> 2) Close NetMinotaur
> 3) Open NetMonitor --> Exception
> 
> Also there is a beast hidden in the STR (just a little prank in case you
> would be bored by my comments :)

:)
Added to todo-list.

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 624719 [details] [diff] [review]
Upload 2

Just updating feedback flag, excellent work!
Honza

Comment 12

[Joe Walker [:jwalker]]

Attachment: Upload 3

Comment 13

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I can't start the Net Minotaur ... my error:
log: NetMonitor._onload()
error
TypeError
  - prototype TypeError
    - fileName = resource:///modules/devtools/NetMonitor.jsm
    - lineNumber = 146
  - prototype Error
  - prototype Object

Seems like this._httpMonitor is an empty object. Not sure why ... timing issue maybe.

Comment 14

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Seems like the bug from comment 13 was fixed by bug 756887

Comment 15

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Upload 4

Rebase

Comment 16

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Tests patch

Comment 17

[Joe Walker [:jwalker]]

Network Monitor being done elsewhere.