Each network log entry is at the outset only a record of the initial http request, and has no subsequent transaction data. Each original log entry needs to be updated with additional nodes that can be displayed via an "arrow button" or something like that.

A lot of this functionality is already in the patch on bug 534398, we mostly need tests and UI implementation.

(In reply to comment #0) > Each original log entry needs to be updated with additional nodes that can be > displayed via an "arrow button" or something like that. Maybe instead of using an arrow button, use a panel for the output?

First part: Implements the underlaying infrastructure to log more then just the beginning of a network request. Also includes unit tests. There is not to much that gets displayed of this extra data that is logged expect the total duration for the request and the response status. Patch applies cleanly on trunk.

This adds a very simple way to inspect the networking activity using a PropertyPanel. This patch requires the patches from bug 573102 (JS/DOM Object Introspector). Also, the HUD.jsterm is often null. To fix this, the patch 459449 from bug 578658 is required.

Comment on attachment 460563 [details] [diff] [review] Patch 1, part 1 ># HG changeset patch ># Parent c2fa4377a7994542e1a77910375ca3c78f4462b5 > >diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm >--- a/toolkit/components/console/hudservice/HUDService.jsm >+++ b/toolkit/components/console/hudservice/HUDService.jsm >@@ -83,16 +83,293 @@ XPCOMUtils.defineLazyGetter(this, "strin > > const ERRORS = { LOG_MESSAGE_MISSING_ARGS: > "Missing arguments: aMessage, aConsoleNode and aMessageNode are required.", > CANNOT_GET_HUD: "Cannot getHeads Up Display with provided ID", > MISSING_ARGS: "Missing arguments", > LOG_OUTPUT_FAILED: "Log Failure: Could not append messageNode to outputNode", > }; > >+/** >+ * Implements the nsIStreamListener and nsIRequestObserver interface. Used >+ * within the HS_httpObserverFactory function to get the response body of >+ * requests. >+ * >+ * The code is mostly based on code listings from: >+ * >+ * http://www.softwareishard.com/blog/firebug/ >+ * nsitraceablechannel-intercept-http-traffic/ >+ */ This is looking good. The first thing you need to do when using source from other projects is add the project's license and contributors to this source file. There may be an example in the inspector's DOMPlate code. I have to start working on a blocker, but will keep working through this patch each day. I'm thinking we should target it for beta 4

oh wait. I meant the parts pulled from firebug itself.

This bug is part of improving the logging of networking messages in the WebConsole. Requesting blocking status for this bug.

Comment on attachment 460565 [details] [diff] [review] Patch 1, part 2 >+ header: httpActivity.response.header, >+ status: httpActivity.response.status >+ }, >+ "Timing": formatedTiming >+ }; >+ >+ let link = msgObject.elementFactory("span"); I think gavin likes these as <a> tags instead of <spans>

Comment on attachment 460563 [details] [diff] [review] Patch 1, part 1 >+ * The code is mostly based on code listings from: >+ * >+ * http://www.softwareishard.com/blog/firebug/ >+ * nsitraceablechannel-intercept-http-traffic/ >+ */ anything that came out of firebug should also include their license and authors >+ CCIN: function (aCName, aIfaceName) All functions should be named: CCIN: function HS_CCIN() [or whatever...] >+ // The following try { } body always throughs an exp because hudId >+ // is undefined in the end. Just use the catch { } body. >+ // DISCUSS: What's the downside in using only the catch { } body? >+ // That is correct, perhaps for now, you leave this code in here knowing it always fails, there is no downside to the catch always executing. We need to file a followup bug on moving this code into a method that logs image requests regardless, to either all WebConsoles or after looking deeper into firebug stealing their code for matching up requests with contentWindows. I have a feeling the use of the httpActivityObserver is what is killing this connection after all, as they are not using it, IIRC. > let _hud = new HeadsUpDisplay(config); >+ _hud.gBrowser = gBrowser; I am not sure if this is needed anymore, or if it is should be a part of this patch > > let hudWeakRef = Cu.getWeakReference(_hud); > HUDService.registerHUDWeakReference(hudWeakRef, hudId); > } > else { > // only need to attach a console object to the window object > let config = { hudNode: hudNode, > consoleOnly: true, > contentWindow: aContentWindow > }; > > let _hud = new HeadsUpDisplay(config); >+ _hud.gBrowser = gBrowser; ditto + with changes

Will this include detailed timing data? Webkit shows me time spent for Connection, SSL, Receiving, etc. http://grab.by/5PGC

Jeff, there is bug 573103 which is about to implement a network panel. This bug is more about implementing the necessary infrastructure to collect all the necessary data. Can you move over to that bug? I've attached some markups there and would be glad to get feedback.

That's something we definitely want to do, but not sure if it'll make it into the first version or not.

(In reply to comment #12) > That's something we definitely want to do, but not sure if it'll make it into > the first version or not. to clarify, I meant I'm not sure we'll be getting detailed timing information in the first version.

(In reply to comment #9) > Comment on attachment 460563 [details] [diff] [review] > Patch 1, part 1 > > >+ CCIN: function (aCName, aIfaceName) > All functions should be named: CCIN: function HS_CCIN() [or whatever...] Do you want them to be called HS_CCIN or NH_CCIN (NH = NetworkManager). The code is not defined within HUD_SERVICE, that's why I'm asking. > That is correct, perhaps for now, you leave this code in here knowing it always > fails, there is no downside to the catch always executing. We need to file a > followup bug on moving this code into a method that logs image requests > regardless, to either all WebConsoles or after looking deeper into firebug > stealing their code for matching up requests with contentWindows. I have a > feeling the use of the httpActivityObserver is what is killing this connection > after all, as they are not using it, IIRC. > > > let _hud = new HeadsUpDisplay(config); > >+ _hud.gBrowser = gBrowser; > I am not sure if this is needed anymore, or if it is should be a part of this > patch I need to have a reference to gBrowser for the functions readPostTextFromRequest() and readPostTextFromPage(). This gBrowser is the same object for all windows/tabs? Can we store/query it in one central place and store it on the HUDService object?

(In reply to comment #14) > (In reply to comment #9) > > Comment on attachment 460563 [details] [diff] [review] [details] > > Patch 1, part 1 > > > > >+ CCIN: function (aCName, aIfaceName) > > All functions should be named: CCIN: function HS_CCIN() [or whatever...] > > Do you want them to be called HS_CCIN or NH_CCIN (NH = NetworkManager). The > code is not defined within HUD_SERVICE, that's why I'm asking. > NH_CCIN > > I need to have a reference to gBrowser for the functions > readPostTextFromRequest() and readPostTextFromPage(). This gBrowser is the same > object for all windows/tabs? Can we store/query it in one central place and > store it on the HUDService object? you have a gBrowser for each xulWindow that is open, that is why we need a lazy way to figure out what the gBrowser is for each window. Right now, we are using HUDService.currentContext() However, gavin does not like how this method works - that is another story. You can use it for now though.

Actually, in this case, I am not sure if HS.currentContext will work for you.

<ddahl> jviereck: you may want to add to your patch a line below this one where we set this._gBrowser = Cu.getWeakReference(gBrowser) <ddahl> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/console/hudservice/HUDService.jsm#1627 <ddahl> then write a getter that returns gBrowser in each HeadsUpDisplay object. this may already exist though... <ddahl> i think we never did that <ddahl> we just let it go out of scope <ddahl> so we need a gBrowser getter that calls get() on _gBrowser in each HeadsUpDisplay object <jviereck> ddahl: is that the "right" gBrowser then? <ddahl> jviereck: yes <jviereck> currenlty I use the gBrowser in the initializerWindow function <ddahl> yeah, that may NOT be the right one in some cases since this code will run even if the tab is not focused

Improved patch based on David's feedback. The former part 2 patch is no longer included as there is a Network Panel on the way.

While looking over this patch within my queue I realized that + // Try to get a displayNode for this loadGroup. If there is none, + // then we try to load images in most cases. + displayNode = self.getDisplayByLoadGroup(loadGroup); + if (!displayNode) { return; + } + + // Get the hudId. + outputNode = displayNode.querySelectorAll(".hud-output-node")[0]; + hudId = displayNode.getAttribute("id"); `displayNode` is not initialized (no let or var!) and `outputNode` is never used. The patch attached adds a "let" in front of `displayNode =`. Future more browser = gBrowser.getBrowserForTab(tab); +let testAsync; Adding the `testAsync;` variable is not needed (was in the first draft of this patch, but not in this version anymore).

Remove the "checkin-needed" due to new patch.

This is an overall improved patch. It doesn't leak anymore (at least in my tests - got to do another try-server run). This patch depends on the patch attached to bug 568034.

Updated patch that has i10n support + some small improvements.

This is a rebased patch + change the l10n notes + cleaned some small stuff up.

This is a rebased patch. Note that while running the unit tests I get the info: TEST-INFO | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js | Console message: [JavaScript Error: "ev.target.classList is undefined" {file: "resource://gre/modules/HUDService.jsm" line: 2265}] The code in this patch adds some textNodes to an existing xul:label, which doesn't seem to make a new function by Patrick fail. I will open up a bug on this.

Comment on attachment 467368 [details] [diff] [review] Patch v5.1 rebased > var NetworkHelper = > { > /** >+ * >+ * @param object aObj >+ * @param nsIIDRef aIID >+ */ >+ QI: function NH_QI(aObj, aIface) >+ { >+ return aObj.QueryInterface(aIface); >+ }, >+ >+ /** >+ * >+ * @param string aCName >+ * @param string aIfaceName >+ */ >+ CCSV: function NH_CCSV(aCName, aIfaceName) above the params, add a summary comment for these > /** >+ * Parses aHeader string. >+ * >+ * @param string aHeader Header string to parse. >+ * @returns array First item holds the status string, the second item holds >+ * the parsed header as an object <value>: <key> >+ */ >+ parseHttpHeader: function HS_parseHttpHeader(aHeader) >+ { >+ let lines = aHeader.split("\n"); http://www.w3.org/Protocols/rfc2616/rfc2616-sec3.html#sec3.7.1 > startHTTPObservation: function HS_httpObserverFactory() > { this is a gargantuan function. i'd prefer if it was broken up somehow, but i guess that doesn't need to happen in this patch. >diff --git a/toolkit/locales/en-US/chrome/global/headsUpDisplay.properties b/toolkit/locales/en-US/chrome/global/headsUpDisplay.properties >--- a/toolkit/locales/en-US/chrome/global/headsUpDisplay.properties >+++ b/toolkit/locales/en-US/chrome/global/headsUpDisplay.properties >@@ -59,8 +59,9 @@ jsPropertyTitle=Object Inspector > # Example: The user executed `window.document` in the WebConsole. The `document` > # object is written to the output. If the user clicks on the `document` output > # in the console, a PropertyPanel will show up. The title of the PropertyPanel > # is set to `Inspect: window.document` because the clicked `document` object was > # evaluated based on the `window.document` string. > jsPropertyInspectTitle=Inspect: %S > copyCmd.label=Copy > copyCmd.accesskey=C >+durationMS=%Sms Hm, not sure this is right for RTL. Cc l10n@mozilla.com on the bug and ask about the ideal way to do this? r=me with comments fixed.

(In reply to comment #25) > Comment on attachment 467368 [details] [diff] [review] > Patch v5.1 rebased > >diff --git a/toolkit/locales/en-US/chrome/global/headsUpDisplay.properties b/toolkit/locales/en-US/chrome/global/headsUpDisplay.properties > >--- a/toolkit/locales/en-US/chrome/global/headsUpDisplay.properties > >+++ b/toolkit/locales/en-US/chrome/global/headsUpDisplay.properties > >@@ -59,8 +59,9 @@ jsPropertyTitle=Object Inspector > > # Example: The user executed `window.document` in the WebConsole. The `document` > > # object is written to the output. If the user clicks on the `document` output > > # in the console, a PropertyPanel will show up. The title of the PropertyPanel > > # is set to `Inspect: window.document` because the clicked `document` object was > > # evaluated based on the `window.document` string. > > jsPropertyInspectTitle=Inspect: %S > > copyCmd.label=Copy > > copyCmd.accesskey=C > >+durationMS=%Sms > > Hm, not sure this is right for RTL. Cc l10n@mozilla.com on the bug and ask > about the ideal way to do this? Can someone of the l10n team please comment on this?

Comment on attachment 467368 [details] [diff] [review] Patch v5.1 rebased + msgObject.messageNode.appendChild( + msgObject.textFactory(" " + + self.getFormatStr("durationMS", [requestDuration]) + "]")); looks wrong. It's hard to tell how without completely understanding the code, what's the expected result? Also, I've heard from ehsan that at least Persian doesn't have that much of a concept of milliseconds.

(In reply to comment #27) > Comment on attachment 467368 [details] [diff] [review] > Patch v5.1 rebased > > + msgObject.messageNode.appendChild( > + msgObject.textFactory(" " + > + self.getFormatStr("durationMS", [requestDuration]) + > "]")); > > looks wrong. It's hard to tell how without completely understanding the code, > what's the expected result? When the network request starts, the URL of the request is logged to the WebConsole. Once the response header is there, a string like "[HTTP/1.1 200 OK" is added. When the request is done an additional string is added that contains the complete duration for the request tha tlooks like " 200ms]". All in all the logged network request looks like this when finished (just an example): https://bugzilla.mozilla.org/show_bug.cgi?id=568634 [HTTP/1.1 200 OK 202ms] > Also, I've heard from ehsan that at least Persian doesn't have that much of a > concept of milliseconds. I'm not sure what you want to say with this. Does it need a special function to display the duration for the Persian l10n support in seconds using a calculation?

I'd like to give some time for the l10n community to comment, a bunch of them watch :l10n. This sounds scary. I wonder if going for two strings, one with the url, and another with the url, http, status, ok, time, would give best flexibility. Then you'd replace the content on load instead of appending.

(In reply to comment #29) > I'd like to give some time for the l10n community to comment, a bunch of them > watch :l10n. This sounds scary. > > I wonder if going for two strings, one with the url, and another with the url, > http, status, ok, time, would give best flexibility. Then you'd replace the > content on load instead of appending. > http, status, ok The status string is returned by the server. It's a combination of the HTTP protocol used (version 1.1 in this example) and the status code itself (there are a few possible values, see here: http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html). That's why I think there are only 3 components that are displayed: the URL, the status code (HTTP/1.1 200 OK) and the duration (202ms). This information is not available at once but appear one after the other (which might result in 3 different l10n string?).

Updating the patch based on Dietrich's review. Waiting for feedback from the l10n team and review on bug 587573 to set the needs-checking flag.

Yeah, sounds like three strings is the best fit. It might also be better for the load phase, as you'd not end up with "http://....foo.html [HTTP/1.1 200 OK" without the closing brace (which you do right now, AFAICT?)

(In reply to comment #32) > Yeah, sounds like three strings is the best fit. It might also be better for > the load phase, as you'd not end up with "http://....foo.html [HTTP/1.1 200 OK" > without the closing brace (which you do right now, AFAICT?) If there is no closing brace this indicates that the request is still going on and is not finished yet. That's a feature ;)

(In reply to comment #32) > Yeah, sounds like three strings is the best fit. It might also be better for > the load phase, as you'd not end up with "http://....foo.html [HTTP/1.1 200 OK" > without the closing brace (which you do right now, AFAICT?) @pwalton: When I update the content of a xul:label in the WebConsole, do I have to call something special to make filtering work?

This patch includes some changes as requested by Axel to integrate a better i10n support. There is now a `networkUrlWithStatus` and `networkUrlWithStatusAndDuration` i10n string + i10n notes. Axel, can give me a hint if this is okay now?

Comment on attachment 468330 [details] [diff] [review] Patch v6.1 feedback+ with comments. <...> >diff --git a/toolkit/locales/en-US/chrome/global/headsUpDisplay.properties b/toolkit/locales/en-US/chrome/global/headsUpDisplay.properties <...> >+# LOCALIZATION NOTE (networkUrlWithStatus): >+# >+# When the HTTP request is started only the URL of the request is printed to the >+# WebConsole. As the response status of the HTTP request arrives, the URL string >+# is replaced by this string (the response status can look like `HTTP/1.1 200 OK). like `HTTP/1.1 200 OK`. >+# The braket is not closed to mark that this request is not done by now. As the bracket >+# request is finished (the HTTP connection is closed) this string is replaced >+# by `networkUrlWithStatusAndDuration` which has a closing the braket. >+# # %1$S = URL of network request # %2$S = response status code from the server (e.g. `HTTP/1.1 200 OK`) networkUrlWithStatus=%1$S [%2$S >+# LOCALIZATION NOTE (networkUrlWithStatusAndDuration): >+# >+# When the HTTP request is finished (the HTTP connection is closed) this string >+# replaces the former `networkUrlWithStatus` string in the WebConsole. >+# # %1$S = URL of network request # %2$S = response status code from the server (e.g. `HTTP/1.1 200 OK`) # %3$S = duration for the complete network request in milliseconds networkUrlWithStatusAndDuration=%1$S [%2$S %3$Sms]

Improved based on Axel's feedback. Thanks for that! @Shawn: Dietrich did a review on this bug already. Maybe you can make an iterdiff and review only the change in the meantime?

Rebased patch + small improvements.

Comment on attachment 468673 [details] [diff] [review] patch v6.3 >+ /** >+ * Stores the received data. >+ */ >+ receivedData: null, The type would be useful in the comment (it's an Array in your current code, but I think you actually want it to be a String). >+ /** >+ * Helper function for XPCOM instanciation (from Firebug) >+ * >+ * @param string aCName >+ * @param string aIfaceName >+ */ >+ CCIN: function RL_CCIN(aCName, aIfaceName) >+ { >+ return Cc[aCName].createInstance(Ci[aIfaceName]); >+ }, So, this is really awkward and totally foreign to the code base. This is trying to solve the problem that Components.Constructor was designed to solve, but in a less elegant way. See https://developer.mozilla.org/en/Components.constructor for more details. >+ onDataAvailable: function RL_onDataAvailable(aRequest, aContext, aInputStream, >+ aOffset, aCount) >+ { A comment stating that this only grabs a copy of the data, and passes it on to the original listener would be handy here. >+ // Copy received data as they come. awkward sentence. >+ let data = binaryInputStream.readBytes(aCount); >+ this.receivedData.push(data); I think, given the only consumer in this patch, that you really want to treat this as a string: this.receivedData += binaryInputStream.readBytes(aCount); >+ onStartRequest: function RL_onStartRequest(aRequest, aContext) >+ { >+ try { >+ this.originalListener.onStartRequest(aRequest, aContext); >+ } catch (ex) { >+ aRequest.cancel(ex.result); >+ } This changes the behavior here - you should just let the exception propagate up the stack, right? >+ QueryInterface: function RL_QueryInterface(aIID) Use XPCOMUtils please > /** >+ * Returns the queried interface aIface from the aObj object. >+ * >+ * @param object aObj >+ * @param nsIIDRef aIID >+ */ >+ QI: function NH_QI(aObj, aIface) >+ { >+ return aObj.QueryInterface(aIface); >+ }, We should just call QueryInterface, this wrapper doesn't get us anything other than indirection. >+ /** >+ * Returns a Components.classes service. >+ * >+ * @param string aCName >+ * @param string aIfaceName >+ */ >+ CCSV: function NH_CCSV(aCName, aIfaceName) >+ { >+ try { >+ // If fail to load, the error can be Cc[cName] has no properties. >+ return Cc[aCName].getService(Ci[aIfaceName]); >+ } >+ catch(exc) { >+ Cu.reportError(aCName + "@" + aIfaceName + " FAILED " + exc); >+ if (!Cc[aCName]) { >+ Cu.reportError("No Components.classes entry for " + aCName); >+ } >+ else if (!Ci[aIfaceName]) { >+ Cu.reportError("No Components.interfaces entry for " + aIfaceName); >+ } >+ } >+ }, You only use this for one service. That doesn't constitute the need of a helper, and this makes things slower by wrapping in a try-catch needlessly. Please remove this. >+ /** >+ * Converts aText with a given aCharset to unicode. >+ * >+ * @param string aText Text to convert. >+ * @param string aCharset Charset to convert the text to. >+ * @returns string Converted text. >+ */ nit: as I've commented in other patches, the documentation format is inconsistent >+ convertToUnicode: function NH_convertToUnicode(aText, aCharset) >+ { >+ if (!aText) { >+ return ""; >+ } This feels like a premature optimization, yeah? >+ let conv; >+ try { >+ conv = this.CCSV("@mozilla.org/intl/scriptableunicodeconverter", >+ "nsIScriptableUnicodeConverter"); Need to createInstance on this, not createService >+ conv.charset = aCharset ? aCharset : "UTF-8"; conv.charset = aCharset | "UTF-8"; >+ } >+ catch (exc) { >+ // the exception is worthless, make up a new one >+ throw new Error( >+ "Failed to convert to unicode using charset: " + >+ conv.charset + " in @mozilla.org/intl/scriptableunicodeconverter"); it's possible this will just throw again of the charset is not supported. Not sure it's even worth having the try-catch here. >+ /** >+ * Reads all available bytes from aStream and converts them to aCharset. >+ * If aNoClose is true, the internal used binaryInputStream is not closed. >+ * >+ * @param nsISeekableStream aStream You are actually passing an nsIInputStream here >+ * @param boolean aNoClose not clear what this means. Also, it's generally frowned upon to have NoSometing as a boolean argument (it gets confusing). Just make it the Something, so in this case, aClose. >+ * @returns string string of what? >+ readFromStream: function NH_readFromStream(aStream, aCharset, aNoClose) This is really "readAndConcertFromStream", right? >+ { >+ let sis = this.CCSV("@mozilla.org/binaryinputstream;1", >+ "nsIBinaryInputStream"); >+ sis.setInputStream(aStream); >+ >+ let segments = []; >+ var availableBytes; >+ while (availableBytes = aStream.available()) { >+ segments.push(sis.readBytes(availableBytes)); >+ } So, you have code here, and in RL_onDataAvailable that consume the input stream and get a string out of it. You should create a helper method that both use. In both cases, you have an nsIInputStream, so you can share code: function readInputStreamToString(aInputStream, /* optional */ aCount) { let bis = Cc["@mozilla.org/binaryinputstream;1"].createInstance(Ci.nsIBinaryInputStream); bis.setInputStream(aInputStream); let size = aCount | aInputStream.available(); return bis.readBytes(size); } Note, however, that JS is limited in what it can do here. We really want to do what NS_ReadInputStreamToString (http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsNetUtil.h#1149) does, but we don't have all the APIs available to us. This means that this can trow (and the looping it code in NH_readFromStream doesn't protect against this). >+ /** >+ * Reads the posted text from aRequest. >+ * >+ * @param nsIHttpChannel aRequest >+ * @param nsIDOMNode aBrowser >+ * @returns string or null string or null in what cases? being more elaborate in java-doc comments is helpful >+ readPostTextFromRequest: function NH_readPostTextFromRequest(aRequest, aBrowser) >+ { >+ try { >+ let is = this.QI(aRequest, Ci.nsIUploadChannel).uploadStream >+ if (is) { better to do: if (aRequest instanceof Ci.nsIUploadChannel) { let is = aRequest.uploadStream; XPConnect will do the QI for you, and you don't need to try-catch. >+ let ss = this.QI(is, Ci.nsISeekableStream); >+ let prevOffset; >+ if (ss) { let ss; if (is instanceof Ci.nsISeekableStream) { with that, you don't need the try-catch anymore >+ readPostTextFromPage: function NH_readPostTextFromPage(aUrl, aBrowser) ditto on instanceof stuff instead of doing explict QI's here. I don't see tests for the various line endings possible for HTTP headers, which would be good to have.

This is an improved patch based on Shawn's review (thanks a lot for it!). I've also moved the unit test into a separate test file. Some questions: - There is now a readInputStreamToString() function as pointed out. Should every caller of this function add a try{}cath(){} around it as it might throws an error? - You said: > I don't see tests for the various line endings possible for HTTP headers, which > would be good to have. I don't exactly what to test. The HS_parseHttpHeader function?

(In reply to comment #40) > - There is now a readInputStreamToString() function as pointed out. Should > every caller of this function add a try{}cath(){} around it as it might throws > an error? You didn't have it before, so you might be lucky enough to not have any input streams that you have to worry about this for. You won't get anything useful anyway if it throws, so it's probably OK to not catch it. > I don't exactly what to test. The HS_parseHttpHeader function? Yup. Basically, want to see that we handle the different cases of line endings there.

(In reply to comment #39) > Note, however, that JS is limited in what it can do here. We really want to do > what NS_ReadInputStreamToString > (http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsNetUtil.h#1149) > does, but we don't have all the APIs available to us. This means that this can > trow (and the looping it code in NH_readFromStream doesn't protect against > this). Which will be fixed by using NetUtil.readInputStreamToString in bug 590654. You should use that.

Shawn, this patch uses the new NetUtil.readInputStreamToString function. Most of the rest should have stayed the same beside rebasing.

Comment on attachment 469707 [details] [diff] [review] Patch v7 >+ convertToUnicode: function NH_convertToUnicode(aText, aCharset) >+ { >+ let conv; nit: declare at first use >+ conv = Cc["@mozilla.org/intl/scriptableunicodeconverter"]. >+ createInstance(Ci["nsIScriptableUnicodeConverter"]); nit: createInstance should line up with Cc, and just do Ci.nsIScript.... >+ * @returns string >+ * UTF-16 encoded string based on the content of aStream and aCharset. global-nit: think you want this lined up with "string" >+ if (is instanceof Ci.nsISeekableStream) { >+ let ss = is.QueryInterface(Ci.nsISeekableStream); you can just use is here - the instanceof check will automagically do the QI for you. >+ let prevOffset; >+ if (ss) { and this would be true if |is| is an instaceof nsISeekableStream >+ prevOffset = ss.tell(); >+ ss.seek(Ci.nsISeekableStream.NS_SEEK_SET, 0); >+ } >+ >+ // Read data from the stream. >+ let charset = aBrowser.contentWindow.document.characterSet; >+ let text = this.readAndConvertFromStream(is, charset); but given this, I think you want to support cases where is isn't a seekable stream? will look at the rest in a bit

Comment on attachment 469707 [details] [diff] [review] Patch v7 >+ readPostTextFromPage: function NH_readPostTextFromPage(aUrl, aBrowser) >+ { >+ if (aUrl == aBrowser.contentWindow.location.href) { It's not clear why you have to check this. Can you please add a comment explaining why. >+ let webNav = aBrowser.webNavigation; >+ if (webNav instanceof Ci.nsIWebPageDescriptor) { >+ let descriptor = >+ webNav.QueryInterface(Ci.nsIWebPageDescriptor).currentDescriptor; let descriptor = webNav.currentDescriptor; >+ if (descriptor instanceof Ci.nsISHEntry) { >+ let entry = descriptor.QueryInterface(Ci.nsISHEntry); >+ if (entry && entry.postData) { >+ if (entry instanceof Ci.nsISeekableStream) { >+ let postStream = entry.postData.QueryInterface(Ci.nsISeekableStream); don't need to do explicit QI's here > /** >+ * Parses aHeader string. >+ * >+ * @param string aHeader Header string to parse. >+ * @returns array First item holds the status string, the second item holds >+ * the parsed header as an object <value>: <key> >+ */ nit: inconsistent comment styling here with how you do other functions >+ parseHttpHeader: function HS_parseHttpHeader(aHeader) >+ { >+ let lines = aHeader.split(/\r\n|\n|\r/); >+ let status = lines[0]; >+ let headerContent = {}; >+ >+ // Skip the first line and parse the rest. It's clear that you are skipping the first line. What would be more useful in the comment is why you skip the first line. >+ lines.slice(1).forEach(function (line) { >+ // Skip empty lines. comment isn't needed; code is obvious > var transCodes = this.httpTransactionCodes; >+ var hudId; why var for these? >+ // In some cases loggedNode can be undefined (e.g. if an image was >+ // requested). Don't continue in such a case. >+ if (loggedNode === undefined) { just |if (!loggedNode) {| right? >+++ b/toolkit/components/console/hudservice/tests/browser/browser_webconsole_netlogging.js Tests can be public domain (which we should use) per http://www.mozilla.org/MPL/license-policy.html. Please change the license block accordingly. >+XPCOMUtils.defineLazyGetter(this, "HUDService", function () { >+ Cu.import("resource://gre/modules/HUDService.jsm"); >+ try { >+ return HUDService; >+ } >+ catch (ex) { >+ dump(ex + "\n"); >+ } >+}); No reason to make this lazy in the test. Just import it. r- for the stuff in comment 44. This is super close!

This bug is improved based on Shawn's latest feedback. The self written HTTP-header-parse is gone and instead the nsIHttpChannel.visitResponseHeaders|visitRequestHeaders functions are used. Calling the nsIHttpChannel.visitResponseHeaders from inside of the HS_httpObserverFactory function caused errors. It seems like the header is not ready to get accessed even when the transaction is closed (!!!). To get around this problem I set the httpActivity object now on the responseListener and set the response information (header, status, body) once the response is done (RL_onStopRequest). I hope this way bug 588533 might be fixed now (will need to test that later).

I would expect the earliest you could safely get the headers would be at OnDataAvailable, fwiw.

Comment on attachment 470081 [details] [diff] [review] Patch v8 >+ // Seek locks the file so, seek to the beginning only if necko hasn't nit: comma before so, not after >+ readPostTextFromPage: function NH_readPostTextFromPage(aBrowser) >+ { >+ let webNav = aBrowser.webNavigation; >+ if (webNav instanceof Ci.nsIWebPageDescriptor) { >+ let descriptor = webNav.currentDescriptor; >+ >+ if (descriptor instanceof Ci.nsISHEntry) { >+ if (descriptor.postData) { >+ if (descriptor instanceof Ci.nsISeekableStream) { so, the nesting is really deep here. I think it'd be better to do something like this: if (descriptor instanceof Ci.nsISHEntry && descriptor.postData && descriptor instanceof Ci.nsISeekableStream) { ... } >+ try { >+ // Copy the request header data. >+ aChannel.visitRequestHeaders({ >+ visitHeader: function(aName, aValue) { >+ httpActivity.request.header[aName] = aValue; >+ } >+ }); >+ } catch (ex) { } please add a comment as to why this might throw and why we don't care if it does >+ case activityDistributor.ACTIVITY_SUBTYPE_RESPONSE_HEADER: >+ msgObject = httpActivity.messageObject; >+ >+ // aExtraStringData contains the response header. The first line >+ // contains the response status (e.g. HTTP/1.1 200 OK). >+ // >+ // Note: The response header is not saved here. Calling the >+ // aChannel.visitResponseHeaders at this point sometimes >+ // causes an NS_ERROR_NOT_AVAILABLE exception. Therefore, >+ // the response header and response body is stored on the >+ // httpActivity object within the RL_onStopRequest function. >+ httpActivity.response.status = >+ aExtraStringData.split(/\r\n|\n|\r/)[0]; aExtraStringData.trim() should work here and make more sense It looks like you removed the test that tested the header with different types of line endings. I think that test was useful to keep even if you don't parse it yourself anymore, so please add it back. r=sdwilsh

(In reply to comment #48) > aExtraStringData.trim() should work here and make more sense disregard this. I misunderstood the comment.

Thanks to Shawn's tip the response header is now calculated in the onDataAvailable as well. It's not enough to do it there only as if there is no response body onDataAvailable is never called for this request but the response header must be read somewhere.

Patch as it should be (missed the hg qrefresh).

Comment on attachment 470099 [details] [diff] [review] Patch v8.1 r=sdwilsh

Comment on attachment 470101 [details] [diff] [review] [backed-out] Patch v8.1.1 r=sdwilsh on this one too, while I'm here.

By the time I was ready with my patch queue, I pulled one last time and ended up with many bad hunks. I do not know how this could happen. I created a new repo even, and applied everything, still I had hg failures. Will try to look into this over the weekend, but do not bet on it.:(

So these are the changesets I pulled when this hg failure happend: anarchy ~/code/moz/mozilla-central/mozilla-central % hg incoming comparing with https://hg.mozilla.org/mozilla-central/ searching for changes changeset: 51666:be857a136ffd parent: 51659:2715808a3010 user: Oleg Romashin <romaxa@gmail.com> date: Fri Jul 16 06:57:00 2010 -0500 summary: Bug 591559 and bug 575567 - "Drop X[Parent/Child] widget embedding from remote browser" [r=cjones] [a=bustage-fix+blocking] changeset: 51667:b49737f77ad2 user: Gavin Sharp <gavin@gavinsharp.com> date: Sat Aug 28 00:48:17 2010 -0400 summary: Bug 591566: disable browser_bug562797.js because it times out a lot, a=orange changeset: 51668:8c3b57225056 user: Brad Lassey <blassey@mozilla.com> date: Fri Aug 27 16:22:57 2010 -0400 summary: bug 591153 - Show progress dialog on start up for android r=mwu a=blocking-fennec changeset: 51669:e051ccf2361a tag: tip user: Brad Lassey <blassey@mozilla.com> date: Fri Aug 27 16:24:42 2010 -0400 summary: bug 591412 - Android make android package step failing due to error in AndroidManifest.xml r=mwu a=blocking-fennec Does not make any sense

anarchy ~/code/moz/mozilla-central/push-only/mozilla % hg qpush applying update-log-entries now at: update-log-entries anarchy ~/code/moz/mozilla-central/push-only/mozilla % hg qpush applying click-handle-log-details patching file toolkit/components/console/hudservice/HUDService.jsm Hunk #1 FAILED at 235 Hunk #2 FAILED at 451 Hunk #3 FAILED at 1002 Hunk #4 FAILED at 1361 Hunk #5 FAILED at 1407 Hunk #6 FAILED at 1444 Hunk #7 FAILED at 1461 Hunk #8 FAILED at 1543 8 out of 8 hunks FAILED -- saving rejects to file toolkit/components/console/hudservice/HUDService.jsm.rej patching file toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js Hunk #1 FAILED at 64 Hunk #2 FAILED at 563 Hunk #3 FAILED at 823 3 out of 3 hunks FAILED -- saving rejects to file toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js.rej patching file toolkit/components/console/hudservice/tests/browser/browser_webconsole_netlogging.js Hunk #1 FAILED at 11 patch failed, unable to continue (try -v) patch failed, rejects left in working dir errors during apply, please fix and refresh click-handle-log-details I'll attach the .rej

(In reply to comment #57) > Created attachment 470153 [details] > HUDService.jsm.rej David, the reject you attached is the patch for bug 573103 ("Click event handler and display of log entry details") and NOT the patch for this bug. Have you mixed them up a little bit?

Comment on attachment 470101 [details] [diff] [review] [backed-out] Patch v8.1.1 http://hg.mozilla.org/mozilla-central/rev/2d1aebfcf1ef

Comment on attachment 470101 [details] [diff] [review] [backed-out] Patch v8.1.1 http://hg.mozilla.org/mozilla-central/rev/289e6da90526 leaks

(In reply to comment #61) > http://hg.mozilla.org/mozilla-central/rev/1ac4f2b24a91 I should have mentioned that after tryserver success, we are pushing again

+networkUrlWithStatus=%1$S [%2$S shouldn't this have a closing ] ?

(In reply to comment #63) > +networkUrlWithStatus=%1$S [%2$S > > shouldn't this have a closing ] ? That's considered to be a feature, see comment 33.

sorry didn't read all of the LOCALIZATION NOTE, only what the variable stands for :-( but i thinks this will create a lot of comments from the community....