Let's move any inline JavaScript, styles, and event handlers out of this file: https://mxr.mozilla.org/mozilla-central/source/browser/devtools/netmonitor/netmonitor.xul

This one is a little more involved - the event handlers should be added in netmonitor-view.js, but probably in different places. Victor - here are the places I would think that the event handlers should be registered, can you confirm that these make sense? * Handlers in #requests-menu-toolbar - add in ToolbarView.initialize / remove in ToolbarView.destroy * Handlers in #requests-menu-footer - add in RequestsMenu.initialize / remove in RequestsMenu.destroy * Handlers in #details-pane - add in _initializePanes / remove in _destroyPanes (I think?) * Handlers in #networkPopupSet - add in RequestsMenu.initialize / remove in RequestsMenu.destroy (I think?) Some of these are kind of going to be a pain (the repeated filterOn and sortBy calls that take a parameter). We would either need to create a new function for each one: * this.filterOnJS = this.filterOn.bind(this, 'js'); querySelector("#requests-menu-filter-js-button").add / removeEventListener("click", this.filterOnJS) or add an attribute on the markup with the variable and have a filter function that passes along the attribute into filterOn: * <button id="requests-menu-filter-js-button" data-filter="js">

(In reply to Brian Grinstead [:bgrins] from comment #1) > > * Handlers in #requests-menu-toolbar - add in ToolbarView.initialize / > remove in ToolbarView.destroy I think it makes more sense to put these in RequestsMenu.initialize/destroy. > * Handlers in #requests-menu-footer - add in RequestsMenu.initialize / > remove in RequestsMenu.destroy I like this. > * Handlers in #details-pane - add in _initializePanes / remove in > _destroyPanes (I think?) Yeah, this makes sense. > * Handlers in #networkPopupSet - add in RequestsMenu.initialize / remove in > RequestsMenu.destroy (I think?) I think this is weird, especially for <menuitem>s. Come to think about it, how about defining <commandset>s and storing these events there. We could then use "command" attributes everywhere instead. > Some of these are kind of going to be a pain (the repeated filterOn and > sortBy calls that take a parameter). We would either need to create a new > function for each one: > > * this.filterOnJS = this.filterOn.bind(this, 'js'); > querySelector("#requests-menu-filter-js-button").add / > removeEventListener("click", this.filterOnJS) > This is super noisy, but I don't feel strongly against it. > or add an attribute on the markup with the variable and have a filter > function that passes along the attribute into filterOn: > > * <button id="requests-menu-filter-js-button" data-filter="js"> However, this is superbly nice :) Let's use this instead if possible.

> > * Handlers in #networkPopupSet - add in RequestsMenu.initialize / remove in > > RequestsMenu.destroy (I think?) > > I think this is weird, especially for <menuitem>s. > > Come to think about it, how about defining <commandset>s and storing these > events there. We could then use "command" attributes everywhere instead. > If we move the logic to a <commandset>, then I think anything inline will still have to move out of the file. For instance in the debugger, even though this allows us to use "command" attributes throughout the rest of the file, the actual oncommand="DebuggerView.Sources.toggleBlackBoxing()" is going to have to move into an external file: https://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger.xul#34. There may still be some benefit to doing this if a command is reused (like in a keyset and popupset). We also need to bind onpopupshowing for network-request-popup so we will need to at least some binding happen for the <menupopup> somewhere in the JS file.

I want to work on this bug. This is going to be my first contribution to open source. I have working build of Firefox. I am starting with fixes suggested in Comment 1 .

(In reply to Aakash Bapna from comment #4) > I want to work on this bug. This is going to be my first contribution to > open source. I have working build of Firefox. > I am starting with fixes suggested in Comment 1 . Aakash, sounds great! Also take a look at Victor's responses in Comment 2 for updates on where some of the event handlers should go. Let me know if you need any help.

(In reply to Aakash Bapna from comment #4) > I want to work on this bug. This is going to be my first contribution to > open source. I have working build of Firefox. > I am starting with fixes suggested in Comment 1 . And if you haven't seen it yet, take a look at https://wiki.mozilla.org/DevTools/Hacking for more information about building and running the DevTools code. As you make changes to the netmonitor.xul and netmonitor-view.js files you will need to rebuild the browser files and restart the browser: ./mach build browser ./mach run -p development And if you want to run the tests on the netmonitor to make sure everything is working you can use: ./mach mochitest-browser browser/devtools/netmonitor/

Why do we have to remove events in destroy() ? Won't they be automatically removed when nodes are removed? (Perhaps memory leaks?) This is how I had started with fixing filter/sort events- RequestsMenuView.initialize: $("#toolbar-labels").addEventListener("click", this.toolbarLabelSortEvent.bind(this), false); //the .bind() will be pain when removing event. ... RequestsMenuView.toolbarLabelSortEvent: function(event) { if( event.target.classList.contains("requests-menu-header-button") ) { var key = event.target.getAttribute("data-key"); key && NetMonitorView.RequestsMenu.sortBy.call(this, key); } } ---- Are there any DOM event helpers available? (like for event namespaces, delegating events)

(In reply to Aakash Bapna from comment #7) > Why do we have to remove events in destroy() ? Won't they be automatically > removed when nodes are removed? (Perhaps memory leaks?) There are situations where not removing event listeners will cause memory leaks. In some cases it is not necessary, but we usually remove all added listeners as a matter of convention. > $("#toolbar-labels").addEventListener("click", > this.toolbarLabelSortEvent.bind(this), false); //the .bind() will be pain > when removing event. Before adding the event listener in the initializer, you can say this.toolbarLabelSortEvent = this.toolbarLabelSortEvent.bind(this). Then you can add and remove the event listeners with addEventListener("click", this.toolbarLabelSortEvent, false); > ... > RequestsMenuView.toolbarLabelSortEvent: function(event) { > if( event.target.classList.contains("requests-menu-header-button") ) { > var key = event.target.getAttribute("data-key"); > key && NetMonitorView.RequestsMenu.sortBy.call(this, key); > } > } This looks pretty good. I wouldn't use the `key && syntax` though - we could do something like this instead: var key = event.target.dataset("key"); if (key) { NetMonitorView.RequestsMenu.sortBy.call(this, key); } We don't really care if the element has the particular class name - just if it has the sort key. Also, a minor change, you can use the dataset property (https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement.dataset) instead of getAttribute. > Are there any DOM event helpers available? (like for event namespaces, > delegating events) I can't think of any off the top of my head, but I think you are on the right path with binding to a single element and using event.target in toolbarLabelSortEvent.

I have made suggested changes, please check the attached preliminary patch. Few notes- - Pulled out code for getting key with event in separate helper function- getKeyWithEvent(). Using this for filtering, sorting and updating(in RequestDetails) events - event.target.dataset is undefined somehow, resorting to getAttribute for now. - One test failed with timeout error. "TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/netmonitor/test/browser_net_accessibility-02.js | This test exceeded the timeout threshold. It should be rewritten or split up. If that's not possible, use requestLongerTimeout(N), but only as a last resort." - I am still getting hang of oncommand in menuitem.

(In reply to Aakash Bapna from comment #9) > - event.target.dataset is undefined somehow, resorting to getAttribute for > now. This must only be available on HTML documents, you are right to use getAttribute instead. > > - One test failed with timeout error. > "TEST-UNEXPECTED-FAIL | > chrome://mochitests/content/browser/browser/devtools/netmonitor/test/ > browser_net_accessibility-02.js | This test exceeded the timeout threshold. > It should be rewritten or split up. If that's not possible, use > requestLongerTimeout(N), but only as a last resort." > I'd try running just that test on its own, using ./mach mochitest-browser browser/devtools/netmonitor/test/browser_net_accessibility-02.js. It could be that the test just ran too long, the window lost focus during the test, or some other issues. If you don't get the error again, I wouldn't worry about it, we will push to the test server before landing any changes. If you still get the error, what does it look like it happening? Is it doing stuff the whole time, or just sitting there doing nothing until it times out? > - I am still getting hang of oncommand in menuitem. Can you elaborate on this? What steps are you taking to get this to happen, and what do you mean by 'hang'?

Comment on attachment 8362627 [details] [diff] [review] netmonitor_cleanup.patch Review of attachment 8362627 [details] [diff] [review]: ----------------------------------------------------------------- I've left some comments, mostly whitespace and code format related. ::: browser/devtools/netmonitor/netmonitor-view.js @@ +99,5 @@ > > this._detailsPane.setAttribute("width", Prefs.networkDetailsWidth); > this._detailsPane.setAttribute("height", Prefs.networkDetailsHeight); > this.toggleDetailsPane({ visible: false }); > + Whitespace stuff: * There is trailing whitespace on some empty lines. Can you remove this? * Please use 2 spaces instead of 4 for indenting for your changes. @@ +103,5 @@ > + > + this.sendCustomRequestEvent = this.RequestsMenu.sendCustomRequest.bind(this.RequestsMenu) > + this.closeCustomRequestEvent = this.RequestsMenu.closeCustomRequest.bind(this.RequestsMenu) > + this.cloneSelectedRequestEvent = this.RequestsMenu.cloneSelectedRequest.bind(this.RequestsMenu) > + this.updateCustomRequestEvent = getKeyWithEvent( this.CustomRequest.onUpdate.bind(this.CustomRequest) ); extra space in between )) @@ +287,5 @@ > this.widget.addEventListener("select", this._onSelect, false); > this._splitter.addEventListener("mousemove", this._onResize, false); > window.addEventListener("resize", this._onResize, false); > + > + this.toolbarLabelSortEvent = getKeyWithEvent( this.sortBy.bind(this) ); Please remove spaces around the inner function calls @@ +290,5 @@ > + > + this.toolbarLabelSortEvent = getKeyWithEvent( this.sortBy.bind(this) ); > + this.requestMenuFilterEvent = getKeyWithEvent( this.filterOn.bind(this) ); > + this.clearEvent = this.clear.bind(this); > + this.contextMenuShowingEvent = this._onContextShowing.bind(this); In the case of _onContextShowing, you could just do `this._onContextShowing = this._onContextShowing.bind(this);` rather than naming a new method. @@ +2269,5 @@ > } > > +function getKeyWithEvent(callback) { > + return function(event) { > + var key = event.target.getAttribute("data-key"); // event.target.dataset.key throws error that dataset is undefined getAttribute is fine, dataset must be available on HTML docs only. ::: browser/devtools/netmonitor/netmonitor.xul @@ +49,5 @@ > class="requests-menu-header requests-menu-status-and-method" > align="center"> > <button id="requests-menu-status-button" > class="requests-menu-header-button requests-menu-status" > + data-key="status" Please line data-key up with the attribute above on this and all the following elements

> > - I am still getting hang of oncommand in menuitem. > > Can you elaborate on this? What steps are you taking to get this to happen, > and what do you mean by 'hang'? Sorry, I misunderstood this - now I see what you mean. You should be able to do addEventListener("command") with these events - I would probably just do all three nodes individually, for now in RequestsMenu (we may decide this should be moved somewhere else, but that should be an easy change).

Fixed the whitespace issues, moved popup menuitem events to view file and added comments for the new function getKeyWithEvent(). The test case still fails. I see all the requests being made in net panel, all checks pass but get a fail message just after finish() is called. 0:55.77 TEST-INFO | chrome://mochitests/content/browser/browser/devtools/netmonitor/test/browser_net_accessibility-02.js | finish() was called, cleaning up... 0:55.96 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/netmonitor/test/browser_net_accessibility-02.js | This test exceeded the timeout threshold. It should be rewritten or split up. If that's not possible, use requestLongerTimeout(N), but only as a last resort."

Okay, it passes when I call requestLongerTimeout(60) in the test file. Anything to worry?

(In reply to Aakash Bapna from comment #14) > Okay, it passes when I call requestLongerTimeout(60) in the test file. > Anything to worry? We try to avoid having to add requestLongerTimeout unless if it is absolutely necessary. I've pushed your patch to the test server so we can see if this is an issue on those systems: https://tbpl.mozilla.org/?tree=Try&rev=f792ebd31edd (it usually takes a couple of hours to fully build and run the tests). What OS are you using, by the way?

(In reply to Aakash Bapna from comment #14) > Okay, it passes when I call requestLongerTimeout(60) in the test file. > Anything to worry? Also FYI, requestLongerTimeout takes an integer multiplier, so to get 60 seconds you would pass 2 into the function. See https://developer.mozilla.org/en-US/docs/Browser_chrome_tests for more info about it. // requestLongerTimeout accepts an integer factor, that is a multiplier for the the default 30 seconds timeout. // So a factor of 2 means: "Wait for at last 60s (2*30s)".

Comment on attachment 8363767 [details] [diff] [review] netmonitor_cleanup.patch Review of attachment 8363767 [details] [diff] [review]: ----------------------------------------------------------------- Overall, the changes are looking very good. There are still quite a few issues with whitespace and code formatting, which I've listed. There is a doc with some more information about the standards for this type of thing: https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style. In this case if you remove trailing whitespace, convert indentation to spaces, and make sure all statements end with a semicolon we will be good to go. Also, we will check with the results of the try push to see if this test timeout is an issue (I'm not running into it on my computer, and I'm not really sure why these changes would trigger the error, but it is something we definitely want to check). ::: browser/devtools/netmonitor/netmonitor-view.js @@ +99,5 @@ > > this._detailsPane.setAttribute("width", Prefs.networkDetailsWidth); > this._detailsPane.setAttribute("height", Prefs.networkDetailsHeight); > this.toggleDetailsPane({ visible: false }); > + There is still some trailing whitespace throughout the file. If you go to https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=961085&attachment=8363767 and click on netmonitor-view.js, you can see all the places with it, highlighted in red. There may be a way to have your text editor highlight or automatically remove the trailing whitespace as well. @@ +100,5 @@ > this._detailsPane.setAttribute("width", Prefs.networkDetailsWidth); > this._detailsPane.setAttribute("height", Prefs.networkDetailsHeight); > this.toggleDetailsPane({ visible: false }); > + > + this.sendCustomRequestEvent = this.RequestsMenu.sendCustomRequest.bind(this.RequestsMenu) Make sure all statements here end with a semicolon. @@ +2278,5 @@ > > /** > + * Helper method to get function which fetches key from event target, executes only if key is present. > + * @param function callback > + * callback to execute when key is present in event.target Suggest to update this comment to say: "Function to execute execute when data-key is present in event.target." @@ +2280,5 @@ > + * Helper method to get function which fetches key from event target, executes only if key is present. > + * @param function callback > + * callback to execute when key is present in event.target > + * @return function > + * to bind to event handler Suggest to update this comment to say "Wrapped function with the target data-key as the first argument." @@ +2288,5 @@ > + var key = event.target.getAttribute("data-key"); > + if(key) { > + callback.call(null, key); > + } > + } Trailing whitespace, and add a semicolon at the end of the return ::: browser/devtools/netmonitor/netmonitor.xul @@ +46,5 @@ > class="requests-menu-header requests-menu-status-and-method" > align="center"> > <button id="requests-menu-status-button" > class="requests-menu-header-button requests-menu-status" > + data-key="status" Although they are lining up nicely here in the diff viewer, many of these attributes have tabs in them. You can see that it isn't lining up correctly if you open view-source:https://bug961085.bugzilla.mozilla.org/attachment.cgi?id=8363767. Please convert all indentation to spaces in this file and make sure it all lines up.

If this patch ends up adding a requestLongerTimeout, please make sure it's documented (as in, add a comment explaining why it's there and what probably caused it). It's a good practice to document requestLongerTimeouts, executeSoons, thread dispatches, and the sorts, to make sure they're there for a good reason.

Thanks for the suggestions, I have made the fixes. I am on Mac, Textmate was changing spaces to tabs.

Comment on attachment 8364456 [details] [diff] [review] netmonitor_cleanup.patch Review of attachment 8364456 [details] [diff] [review]: ----------------------------------------------------------------- The try push doesn't seem to have any problems with the timeout you are seeing locally - I've triggered a few more test runs to make sure, but I don't think we will need to worry about the requestLongerTimeout here. Once you upload the next version, you can mark review?=bgrins. ::: browser/devtools/netmonitor/netmonitor-view.js @@ +108,5 @@ > + > + $("#custom-request-send-button").addEventListener("click", this.sendCustomRequestEvent, false); > + $("#custom-request-close-button").addEventListener("click", this.closeCustomRequestEvent, false); > + $("#headers-summary-resend").addEventListener("click", this.cloneSelectedRequestEvent, false); > + $("#details-pane").addEventListener("input", this.updateCustomRequestEvent, false); One last thing - please change #details-pane to #custom-pane to be a little more specific in which elements we are listening to for the oninput event.

changed #details-pane to #custom-pane.

Comment on attachment 8365176 [details] [diff] [review] netmonitor_cleanup.patch Review of attachment 8365176 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/netmonitor/netmonitor-view.js @@ +126,5 @@ > + > + $("#custom-request-send-button").removeEventListener("click", this.sendCustomRequestEvent, false); > + $("#custom-request-close-button").removeEventListener("click", this.closeCustomRequestEvent, false); > + $("#headers-summary-resend").removeEventListener("click", this.cloneSelectedRequestEvent, false); > + $("#details-pane").removeEventListener("input", this.updateCustomRequestEvent, false); the removeEventListener should also change to #custom-pane

Yikes! changed that.

Comment on attachment 8365228 [details] [diff] [review] netmonitor_cleanup.patch Review of attachment 8365228 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, and has a green try: https://tbpl.mozilla.org/?tree=Try&rev=578fb6c097da. Victor, care to have a look? And as I said earlier, we should not need to add requestLongerTimeout in the test as that seems to be a local problem.

Comment on attachment 8365228 [details] [diff] [review] netmonitor_cleanup.patch Review of attachment 8365228 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for the patch! This looks great. r+ with changes below. ::: browser/devtools/netmonitor/netmonitor-view.js @@ +102,5 @@ > this.toggleDetailsPane({ visible: false }); > + > + this.sendCustomRequestEvent = this.RequestsMenu.sendCustomRequest.bind(this.RequestsMenu); > + this.closeCustomRequestEvent = this.RequestsMenu.closeCustomRequest.bind(this.RequestsMenu); > + this.cloneSelectedRequestEvent = this.RequestsMenu.cloneSelectedRequest.bind(this.RequestsMenu); These three should be in RequestsMenuView, in initialize/destroy IMHO. @@ +103,5 @@ > + > + this.sendCustomRequestEvent = this.RequestsMenu.sendCustomRequest.bind(this.RequestsMenu); > + this.closeCustomRequestEvent = this.RequestsMenu.closeCustomRequest.bind(this.RequestsMenu); > + this.cloneSelectedRequestEvent = this.RequestsMenu.cloneSelectedRequest.bind(this.RequestsMenu); > + this.updateCustomRequestEvent = getKeyWithEvent(this.CustomRequest.onUpdate.bind(this.CustomRequest)); This should be in CustomRequestView. Please add initialize/destroy methods in there, just like NetworkDetailsView has, and have this event listener added/removed there. You'll have to call CustomRequestView.initialize/destroy in NetMonitorView.initialize/destroy. @@ +286,5 @@ > this.widget.addEventListener("select", this._onSelect, false); > this._splitter.addEventListener("mousemove", this._onResize, false); > window.addEventListener("resize", this._onResize, false); > + > + this.toolbarLabelSortEvent = getKeyWithEvent(this.sortBy.bind(this)); Nit: rename this to requestsMenuSortEvent, since it's also part of the RequestsMenu. @@ +287,5 @@ > this._splitter.addEventListener("mousemove", this._onResize, false); > window.addEventListener("resize", this._onResize, false); > + > + this.toolbarLabelSortEvent = getKeyWithEvent(this.sortBy.bind(this)); > + this.requestMenuFilterEvent = getKeyWithEvent(this.filterOn.bind(this)); requests* @@ +2276,5 @@ > return [(name + "=" + value) for ({name, value} of aParams)].join("&"); > } > > /** > + * Helper method to get function which fetches key from event target, executes only if key is present. This comment doesn't shed too much light on why it's needed. How about adding a few explanations as to its purpose. Nit: please add an extra line between the method explanation and the @param section. @@ +2285,5 @@ > + */ > +function getKeyWithEvent(callback) { > + return function(event) { > + var key = event.target.getAttribute("data-key"); > + if(key) { Nit: add a space between if and ( ::: browser/devtools/netmonitor/netmonitor.xul @@ +225,5 @@ > custom-header"/> > <hbox flex="1" pack="end"> > <button class="devtools-toolbarbutton" > label="&netmonitorUI.custom.send;" > + id="custom-request-send-button"/> Nit: can you please put the id above the class? @@ +230,3 @@ > <button class="devtools-toolbarbutton" > label="&netmonitorUI.custom.cancel;" > + id="custom-request-close-button"/> Ditto. @@ +233,3 @@ > </hbox> > </hbox> > <hbox id="custom-method-and-url" Unrelated, but whenever I see this custom-method-and-url box, I cry a little :) Can't wait for the new design. @@ +321,5 @@ > crop="end" > flex="1"/> > <button id="headers-summary-resend" > label="&netmonitorUI.summary.editAndResend;" > + class="devtools-toolbarbutton"/> Nit: please place class below id and line all the attributes horizontally.

Modifying as suggested. P.S. whats "Nit"?

> this._onContextNewTabCommand = this.openRequestInTab.bind(this); > this._onContextCopyUrlCommand = this.copyUrl.bind(this); > this._onContextResendCommand = this.cloneSelectedRequest.bind(this); > this.sendCustomRequestEvent = this.sendCustomRequest.bind(this); > this.closeCustomRequestEvent = this.closeCustomRequest.bind(this); > this.cloneSelectedRequestEvent = this.cloneSelectedRequest.bind(this); Would it make sense to bind and save to original variable itself? > this.openRequestInTab = this.openRequestInTab.bind(this);

(In reply to Aakash Bapna from comment #26) > Created attachment 8365673 [details] [diff] [review] > > P.S. whats "Nit"? "Nit" is nitpicking :)

Comment on attachment 8365673 [details] [diff] [review] netmonitor_cleanup.patch Review of attachment 8365673 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/netmonitor/netmonitor-view.js @@ +279,5 @@ > + this.clearEvent = this.clear.bind(this); > + this._onContextShowing = this._onContextShowing.bind(this); > + this._onContextNewTabCommand = this.openRequestInTab.bind(this); > + this._onContextCopyUrlCommand = this.copyUrl.bind(this); > + this._onContextResendCommand = this.cloneSelectedRequest.bind(this); (In reply to Aakash Bapna from comment #27) > > Would it make sense to bind and save to original variable itself? > > this.openRequestInTab = this.openRequestInTab.bind(this); It doesn't matter too much here. I'd say leave it as it is. @@ +1475,5 @@ > CustomRequestView.prototype = { > /** > + * Initialization function, called when the network monitor is started. > + */ > + initialize: function() { Nit: please add a dumpn("Initializing the CustomRequestView");\n call here, similar to what other views do. This is useful when troubleshooting startup/shutdown issues. @@ +1484,5 @@ > + > + /** > + * Destruction function, called when the network monitor is closed. > + */ > + destroy: function() { dumpn("Destroying the CustomRequestView");\n @@ +2291,5 @@ > return [(name + "=" + value) for ({name, value} of aParams)].join("&"); > } > > /** > + * Helper method to get wrapped function which can be binded to event directly and is executed only when data-key is present in event.target. "Helper method to get a wrapped function which can be bound to as an event listener directly and is executed only when data-key is present in event.target." fixed some typos.

Done.

OK Aakash, one last try push before we mark checkin-needed: https://tbpl.mozilla.org/?tree=Try&rev=4d40ebc92930

Aakash, everything looks good! You can add checkin-needed to the Keywords field in order to get it checked in.

Thanks Brian, Victor for all the help! can anyone invite me to mozillians.org?

(In reply to Aakash Bapna from comment #35) > Thanks Brian, Victor for all the help! can anyone invite me to > mozillians.org? Sure, I sent an invite