Closed Bug 573103 Opened 10 years ago Closed 10 years ago

Implement WebConsole Network Panel

Categories

(DevTools :: General, defect, P1, blocker)

x86
All
defect

Tracking

(blocking2.0 beta5+)

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta5+

People

(Reporter: ddahl, Assigned: julian.viereck)

References

Details

(Whiteboard: [kd4b5])

Attachments

(10 files, 14 obsolete files)

77.54 KB, image/png
Details
103.44 KB, image/png
Details
96.24 KB, image/png
Details
92.35 KB, image/png
Details
162.64 KB, image/png
Details
143.40 KB, image/png
Details
409.51 KB, image/png
Details
420.08 KB, image/png
Details
336.48 KB, image/png
Details
58.42 KB, patch
Details | Diff | Splinter Review
For each logged entry in the console that is a reference to an object, DOM node, XHR, etc. -  clicking on the entry should open a details panel/widget that shows us what properties it has.
Depends on: 573102
Blocks: 529086
Severity: normal → blocker
Priority: -- → P1
Note: Bug 573102 is about implementing a PropertyPanel that can be used to inspect JS Objects andDOM nodes. Should this panel be used for XHR data as well (maybe as first iteration and change it later?)
Whiteboard: [kd4b4]
Assignee: nobody → jviereck
Just did a short chat with Patrick. His idea was to open up a new panel to display all these information in. That panel then has sections like "Timing", "Request: Header", "Request: Body", "Response: Header", "Response: Body". The look could be somewhat how Joe's last iteration of the advanced CSS Inspector looks like: https://bug582596.bugzilla.mozilla.org/attachment.cgi?id=463090. Keeping the look the same over the dev-tools might be a good idea.
Status: NEW → ASSIGNED
Attached image First iteration
This is a first/very early iteration. It's based on Joe's work with some small modifications to the CSS.

The basic idea how the data is presented is taken from WebKit's WebInspector (including the header namings etc). I don't feel good about taking that much over from WebKit, although they have copied Firebug as well.

Please tell me what you think!
Keywords: uiwanted
Julian, I think that's pretty slick. Very cool and consistent with the new style panel styling Joe's been working on, which I like.

One thing, the twisties in the headers when they're collapsed should be pointing to the right. See the "Timing" header for the example. I'm not sure if that's just because of whatever templates you were using to generate that or if it's the graphic itself.

We might want to adjust the ordering of some of the sections. Timing should probably be last in the list. Request Headers and Response headers should be first and second.

looking good!
(In reply to comment #4) 
> One thing, the twisties in the headers when they're collapsed should be
> pointing to the right. See the "Timing" header for the example. I'm not sure if
> that's just because of whatever templates you were using to generate that or if
> it's the graphic itself.

No, the twisties are attempted to be like that. If a section is opened, the twisties go down, if a section is closed, they point to the left. But maybe this assumption by me is wrong?

> 
> We might want to adjust the ordering of some of the sections. Timing should
> probably be last in the list. Request Headers and Response headers should be
> first and second.

My idea was to have timing up there as it holds information about all the following "events" (header received etc).

As I'm writting, I'm wondering if that extra section is necessary. Maybe it would be the best to add the timing information to the header information directly, aligned to the right, next beside the twisties? Would save screen space. 

The extra section "Timing" is required if we want to display the timing information using a graph, but that might should be done using a "Networking Timing Panel" that displays all the networking and timing information like it's done in the Firebug Net panel or the Resource section of WebInspector. I expect that designing this graphical display will take more time then we have for this so the simple "put the timing in the header" way could be suitable for now?

---

I wonder how a large chunk of response body should be displayed within the panel. There will be need to add a section "Response Body" that isn't in the markup (yet). If the panel has the current width as in the markup, then a long scrollbar will appear to scroll over the response body. The user can resize the panel to inspect the response body better, but maybe there is an better idea then just resizing the panel.

Also, are there any kind of highlighter in the Firefox codebase already, that could be used to highlight the response body?
(In reply to comment #5)

> As I'm writting, I'm wondering if that extra section is necessary. Maybe it
> would be the best to add the timing information to the header information
> directly, aligned to the right, next beside the twisties? Would save screen
> space. 

simpler is better right now for sure. see how it looks, but i think we want to err on the side of simple/less code/ file followup bugs

> 
> The extra section "Timing" is required if we want to display the timing
> information using a graph, but that might should be done using a "Networking
> Timing Panel" that displays all the networking and timing information like it's
> done in the Firebug Net panel or the Resource section of WebInspector. I expect
> that designing this graphical display will take more time then we have for this
> so the simple "put the timing in the header" way could be suitable for now?
this is indeed a separate bug (or meta bug)


> I wonder how a large chunk of response body should be displayed within the
> panel. There will be need to add a section "Response Body" that isn't in the
> markup (yet). If the panel has the current width as in the markup, then a long
> scrollbar will appear to scroll over the response body. The user can resize the
> panel to inspect the response body better, but maybe there is an better idea
> then just resizing the panel.
I would justuse the scrollbars for now, perhaps a pop-out window like a view-source window can be used later to allow easier inspection of the response.

> 
> Also, are there any kind of highlighter in the Firefox codebase already, that
> could be used to highlight the response body?
not sure what you are asking for? code highlighter? for markup, yes, css and js maybe not. I'll ask around and look at the view-source code for you.
Attached image Second iteration
Compared to the first iteration, this one:

- has no arrows in the headers anymore. This goes with the question: Should the content always be expanded when the panel is opened? Should it be able to collapse the body?

- the "Timing" section is gone. Instead, some simple timing information is added to the headers directly. The number to the right of the "Request Headers" header is the absolute time when the request started + the delta based on when the page is loaded. The number on the "Response Headers" header shows the delta on how long it took from the request till the first response. The time-delta on the "Response Body" shows the time it took to receive the body after the response header was received. During one network request we store up to 12 timing information. The three numbers presented here are based around the question "what are the information a developer will need most". Some developers might care about how long it took to look up the DNS, but I think most are interested in:
  - when did the request start (after I hit "enter" to load/reload the page)
  - how long did it take to get a response from the server (latency)
  - how long did it take to get the response body from the server

Q: Should the "Response Headers|Body" have an absolute time as well?

- added a "Response Body" section with a few lines of content (more then can be displayed in the panel's view port)
(In reply to comment #7)
> During one network request we store up to 12
> timing information. The three numbers presented here are based around the
> question "what are the information a developer will need most".

DNS and SSL are interesting in the initial requests when those have the biggest impact.  I don't know what the 12 numbers are, but I'd probably want to see them all at some point.  I've filed bugs based on the SSL info from Chrome's debug panel.

It's nice to see the deltas at a glance, but if I'm interested in timing info I'd like to see it all in one place (and with as much detail as you can offer).
These are the timing infos I record currently - not saying that it might not be possible to add more by using some other Gecko observer mechanism:

      transactionCodes: {
         0x5001: "REQUEST_HEADER",
         0x5002: "REQUEST_BODY_SENT",
         0x5003: "RESPONSE_START",
         0x5004: "RESPONSE_HEADER",
         0x5005: "RESPONSE_COMPLETE",
         0x5006: "TRANSACTION_CLOSE",

         0x804b0003: "STATUS_RESOLVING",
         0x804b0007: "STATUS_CONNECTING_TO",
         0x804b0004: "STATUS_CONNECTED_TO",
         0x804b0005: "STATUS_SENDING_TO",
         0x804b000a: "STATUS_WAITING_FOR",
         0x804b0006: "STATUS_RECEIVING_FROM"
       }

The first attempt of this is to get "something" in. It's not to long until beta5. What do you think, how important is DNS and SSL timing for the majority of developers? Personally, I never was to much interested in.
I agree that connection setup isn't as generally useful as the pieces you have.  I'll use Chrome if I want to get at the other data for now. ;)
Attached image "real" 1
Attached image "real 2"
Uploaded two "real" screenshots that I've taken from the so far implemented NetworkPanel. The screenshot shows the problem when there is more content in one line then it can be displayed in it (so x-scrollbars appear). Also, a very large response body doesn't look that nice + shows up scroll bars (see "real 2").
Comment on attachment 464843 [details]
"real 2"

Please note the comments/ideas/thoughts in the bug so far as well.
Attachment #464843 - Flags: ui-review?(limi)
That's generally looking good!

Can there be multiple of these open at a time? If so, the title bar should probably reflect either the time of the request or the URL requested.
Attached image "real" 3
Added new section "Sent Cookies". This data is sent with the request header and as such could be in the same section but I have the feeling this is something independent enough to open up a separate section.

To prevent the text leaking to the right of the sections I've added overflow-x: auto in the css. This causes scrollers to appear within the sections. To prevent the Response Body section to be giant I've added a max-height=350px to the sections.

UX question: Are there now to many scrollers? How will this behave on non OSX platforms?

This version looks good enough to me so I will stick with it as long as that's okay for the UX experts. Note that I might add arrows back to expand, close a header by clicking on the section.
This is a screenshot of the current NetworkPanel made on Gnome (clearlooks theme) Ubuntu 10.04.
Attached patch Patch v1 (obsolete) — Splinter Review
Patch that implements the NetworkPanel. I sent this and another patch to try server. If there are no leakings, then I will start the feedback/review process.
Whiteboard: [kd4b4] → [kd4b5]
Attached file Patch v2 (obsolete) —
Updated patch. This patch depends on the attachments in bug 568634. It includes a working NetworkPanel that works for inspecting text + (cached) images.
Attachment #465648 - Attachment is obsolete: true
Attachment #466463 - Flags: feedback?(ddahl)
Requesting blocking status on this bug as it's an important feature for the WebConsole. It allows the user to inspect single network requests.
blocking2.0: --- → ?
Depends on: 568634
No longer depends on: 573102
Attached patch patch v3 (obsolete) — Splinter Review
Added some more comments, added more i10n support, ensure that the unit tests are running alone and not in parallel to other tests (caused orange in try build) + other smaller improvements.
Attachment #466463 - Attachment is obsolete: true
Attachment #466649 - Flags: feedback?(ddahl)
Attachment #466463 - Flags: feedback?(ddahl)
Attached patch patch v3.1 (obsolete) — Splinter Review
Rebased patch + improved l10n support + small cleanup.
Attachment #466649 - Attachment is obsolete: true
Attachment #467129 - Flags: feedback?(ddahl)
Attachment #466649 - Flags: feedback?(ddahl)
Blocks: 588540
Comment on attachment 467129 [details] [diff] [review]
patch v3.1

the only thing I don't like is the 162 line method: "update: function NP_update()"

Perhaps you can re-factor that into several methods? does that make sense? if not, why not. It will be harder to maintain.

Looking good though. Will build it tomorrow.
Attachment #467129 - Flags: feedback?(ddahl) → feedback-
Attached patch Patch v4 (obsolete) — Splinter Review
New NetworkPanel that has a much simplier update() function. Thanks for pointing this out David.
Attachment #467129 - Attachment is obsolete: true
Attachment #467433 - Flags: feedback?(ddahl)
This patch is badly bitrotted, can you rebase. I just need to run tests and one last look.
The patch bitrotted in 22 minutes?
(In reply to comment #25)
> This patch is badly bitrotted, can you rebase. I just need to run tests and one
> last look.

Sorry, David, I should have told you what you need to apply this:

- patch from bug 588345 (Fix l10n comments for JS/DOM Object Introspector)
- patch from bug 587573 (Log image requests to the WebConsole)

I shouldn't have based that patches on top of bug 588345 as this is not related to this work but it already has an r+ just waiting for blocking/approval which means it's nearly about to get checked in and I expect it to land before this patches will do.

Tell me if that's okay for testing purpose, otherwise I upload a rebased patch that is not build on top of bug 588345 (which would require some work... that's why I'm asking).
As there is some conversation going on about displaying network request duration in the WebConsole (bug 568634) I raise my hand for feedback on the l10n parts of this bug as well. The new added l10n section looks currently like this:

  NetworkPanel.label=Inspect Network Request
  NetworkPanel.requestHeaders.label=Request Headers
  NetworkPanel.requestCookie.label=Sent Cookie
  NetworkPanel.requestBody.label=Request Body
  NetworkPanel.responseHeaders.label=Response Headers
  NetworkPanel.responseBody.label=Response Body
  NetworkPanel.responseNoBody.label=There Is No Response Body
  NetworkPanel.responseImage.label=Received Image
  NetworkPanel.responseImageCached.label=Cached Image
  # LOCALIZATION NOTE (NetworkPanel.deltaDurationMS):
  #
  # This string is used to show the duration between two network events (e.g
  # request and respones header or response header and response body).
  #
  # `Δ` results in the greek capital delta character.
  # For more information see the page:
  #   http://www.fileformat.info/info/unicode/char/394/index.htm
  NetworkPanel.deltaDurationMS=Δ%Sms
  # LOCALIZATION NOTE (NetworkPanel.imageSizeDeltaDurationMS):
  # This string is used to show the duration between the response header and the
  # response body event. It also shows the size of the received or cached image.
  #
  # The first %S is replace by the width of the inspected image.
  # The second %S is replaced by the height of the inspected image.
  # The third %S is replaced by the duration between the response header and the
  # response body event.
  #
  # `Δ` results in the greek capital delta character.
  # For more information see the page:
  #   http://www.fileformat.info/info/unicode/char/394/index.htm
  NetworkPanel.imageSizeDeltaDurationMS=%Sx%Spx, Δ%Sms

I don't want to explain anything because it should be already there - if not, please let me know and I add more information as needed.
(In reply to comment #27)
> Sorry, David, I should have told you what you need to apply this:
> 
> - patch from bug 588345 (Fix l10n comments for JS/DOM Object Introspector)
> - patch from bug 587573 (Log image requests to the WebConsole)
> 
> I shouldn't have based that patches on top of bug 588345 as this is not 

Aha. well, i think we should really get bug 587573 at least fully reviewed before we spend too much time reviewing this patch as there will be a bunch of rebasing to do. I will land bug 587573 ASAP as it is IMHO int the top 2 most important patches.
Comment on attachment 467433 [details] [diff] [review]
Patch v4

Looking good, will f+ soon, maybe after 587573 is reviewed:)
Sounds like a great enhancement, but not holding the release for it, blocking-.
blocking2.0: ? → -
Attached patch Patch v5 (obsolete) — Splinter Review
This patch replaces is changes a lot: The NetworkPanel is no longer a html file but instead an XHTML file to have support for DTDs. A new DTD is added that holds the i10n strings for the basic UI (this was suggested by Axel). Also, this patch makes sure the NetworkPanel's document is loaded before it's accessed.
Attachment #467433 - Attachment is obsolete: true
Attachment #468511 - Flags: feedback?(ddahl)
Attachment #467433 - Flags: feedback?(ddahl)
Attached patch Patch v5.1 (obsolete) — Splinter Review
Sorry David for the spam... One string got lost during the merge.
Attachment #468511 - Attachment is obsolete: true
Attachment #468513 - Flags: feedback?(ddahl)
Attachment #468511 - Flags: feedback?(ddahl)
Attached patch Patch v5.2 (obsolete) — Splinter Review
Rebased patch.
Attachment #468513 - Attachment is obsolete: true
Attachment #468513 - Flags: feedback?(ddahl)
Attachment #468674 - Flags: feedback?(ddahl)
Comment on attachment 468674 [details] [diff] [review]
Patch v5.2

I like the functionality, but I think after speaking with Kevin and Limi, it might be worth our time to see what happens when we either dock this output in a vbox and insert it to the right of the console output, OR, display it inline in the console the way that firebug does it, basically appending this into an output node. You would want to collapse all of it, with an obvious way to expand the output. I'm just not sure the panels are the way to go.
Attachment #468674 - Flags: feedback?(ddahl) → feedback-
Attached image Integration1 (obsolete) —
(In reply to comment #35)
> Comment on attachment 468674 [details] [diff] [review]
> Patch v5.2
> 
> I like the functionality, but I think after speaking with Kevin and Limi, it
> might be worth our time to see what happens when we either dock this output in
> a vbox and insert it to the right of the console output

A *VERY* simple/fast hack to put the NetworkPanel to the right. Shows an collapsed "Request Headers".
Attached image Integrate2
Same idea as Integration1 (attachment 468783 [details]), showing a problem when the requested URL is very long...
Attached image Integrate1.1
Attachment #468783 - Attachment is obsolete: true
Attached image Integration3
This screenshot shows an UX problem: if there is to much content in the response body. The height of the response body div is limited to a fixed number using "min-height" in the css of the XHTML. If the height of the WebConsole is smaller then this min-height, then the section header "Response Body" is no longer visible and as such the user doesn't know what he is inspecting. Also, see the entire content of the response body scrolling in the response body div as well as in the panel itself is necessary which just feels wrong.

A solution for this might be to set the min-height value depending on the WebConsole height. If some just could tell me how to do this... Can we modify CSS rules using some internal XPCOM magic?
Right — I wouldn't even try to show the response object in that space, can we link to a dedicated window for it instead? It's (usually) going to be of equivalent size and content of an HTML page, isn't it? If it is, we should give it a real window, like with View Source.

If there are two classes of response objects, can we display the smaller ones inline and link off to a dedicated window if the size exceeds some threshold?
Looking at the amount of work to do and the resources available, I just don't think it's possible to rework the UI for these now. I think they need to stay as panels.
Comment on attachment 468674 [details] [diff] [review]
Patch v5.2

Since we want to get this landed, we should just do it as a panel to get the string in, and file and inline view followup bug
Attachment #468674 - Flags: feedback- → feedback+
Attached patch Patch v5.3 (obsolete) — Splinter Review
Small improvements + rebased.
Attachment #468674 - Attachment is obsolete: true
Attachment #469154 - Flags: review?(sdwilsh)
This patch is the basic "view of HTTP requests" for the console and, as such, it is both an important feature *and* has strings, so this should really be a beta 5 or 6 blocker.

This patch has been looked at within our group but has not yet gone through review, so landing it in beta 5 may be unrealistic. Since the patch is already here, though, I propose we land early in beta 6.
blocking2.0: - → ?
Julian just pointed out that this bug is blocking other bugs (which are already listed as beta5+ blockers). I'll see what the chances are of getting this reviewed today.
Attachment #469154 - Flags: review?(sdwilsh) → review?(dietrich)
Comment on attachment 469154 [details] [diff] [review]
Patch v5.3


>+///////////////////////////////////////////////////////////////////////////
>+//// Helper for creating the panel.

nit: s/panel/network panel/

>+/**
>+ * Creates a new DOMNode and appends it to aParent.
>+ *
>+ * @param nsIDOMDocument aDocument
>+ *        Document to create the new DOMNode.
>+ * @param nsIDOMNode aParent
>+ *        A parent node to append the created element.
>+ * @param string aTag
>+ *        Name of the tag for the DOMNode.
>+ * @param object aAttributes
>+ *        Attributes set on the created DOMNode.
>+ *
>+ * @returns nsIDOMNode
>+ */
>+function appendChild(aDocument, aParent, aTag, aAttributes)
>+{
>+  let node = createElement(aDocument, aTag, aAttributes);
>+  aParent.appendChild(node);
>+  return node;
>+}

createAndAppendElement would be more descriptive. also, why not just make parent an optional arg to createElement?

>+
>+///////////////////////////////////////////////////////////////////////////
>+//// NetworkPanel
>+
>+/**
>+ * Creates a new NetworkPanel.
>+ *
>+ * @param nsIDOMNode aParent
>+ *        Parent node to append the created panel to.
>+ * @param nsIDOMDocument aDocument
>+ *        Document to create the new nodes on.

could leave this out, using aParent.ownerDocument instead.

>+  // Create the tree.

nit: s/tree/something else/

>+  // Destroy the panel when it's closed.
>+  this.panel.addEventListener("popuphidden", function onPopypHide() {
>+    self.panel.removeEventListener("popuphidden", onPopypHide, false);

typo

>+  /**
>+   * Callback is called once the NetworkPanel is ready and updated once. Used
>+   * by unit tests.
>+   */
>+  isReadyCallback: null,

hm, can't you listen for the event sent when a popup is loaded, and check that popup's id for a match?

>+  /**
>+   *
>+   * @returns boolean
>+   *          Returns true if the server responded that the request is already
>+   *          in the browser's cached, false otherwise.

typo: cached.

>+  /**
>+   * Appends the node with id=aId by the text aValue.
>+   *
>+   * @param string aId
>+   * @param string aValue
>+   * @returns void
>+   */
>+  _appendNode: function NP_appendNode(aId, aValue)
>+  {
>+    let textNode = this.document.createTextNode(aValue);
>+    this.document.getElementById(aId).appendChild(textNode);
>+  },

nit: _appendTextNode for clarity

>+
>+  /**
>+   * Adds a list of key-values to the node with id=aPartenId.

typo

also, this comment doesn't describe what actually happens. add a note explaining that it builds actual HTML content for display?

ftr, i think the styling needs work, but ui-review will weigh in on that. biggest issue to me is that the value data is not aligned against center vertical line, making the general appearance of the panel jumbled, and making it difficult for the eye to quickly scan the contents.

>+  /**
>+   * Processes the request header.

what do you mean by process, and what's it do with the processed content? ditto for the processing methods following this.

>+      let cookies = request.header.Cookie.split(";");
>+      let cookieList = {};
>+      let cookieListSorted = {};
>+      cookies.forEach(function(cookie) {
>+        let name, value;
>+        [name, value] = cookie.trim().replace("\n", "").split("=");

can this also be \r\n or \r? can't remember for cookie content...

>   unregisterDisplay: function HS_unregisterDisplay(aId)
>   {
>+    // Remove children from the output. If the output is not cleared, there can
>+    // be leaks as some nodes has node.onclick = function; set and GC can't
>+    // remove the nodes then.
>+    HUDService.clearDisplay(aId);
>+

why are you doing that then? why not use addEventListener and remove the listener when appropriate?

>+<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en">
>+<head>
>+  <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"/>
>+  <style>

split this out into a separate file, move to the theme dirs.


>+  </div>
>+</div>
>+</body>
>+</html>
>\ No newline at end of file

fix

>+<!ENTITY networkPanel.requestHeaders              "Request Headers">
>+<!ENTITY networkPanel.requestCookie               "Sent Cookie">
>+<!ENTITY networkPanel.requestBody                 "Request Body">
>+<!ENTITY networkPanel.responseHeaders             "Response Headers">
>+<!ENTITY networkPanel.responseBody                "Response Body">
>+<!ENTITY networkPanel.responseNoBody              "There Is No Response Body">

s/There Is// IMO, but ask Limi.

r=me for what's here so far. however, i'd like to see another revision of this post-UI-review, and with the alignment issue i mentioned above fixed.

i'm also concerned about the platform agnostic look and feel. but i guess issues with that will shake out in ui-review.
Attachment #469154 - Flags: review?(dietrich) → review+
Attached patch Patch v5.4 (obsolete) — Splinter Review
Same as Patch v5.3 but replaced "Contenet" by "Content".
Attachment #469154 - Attachment is obsolete: true
Attached patch Patch v6 (obsolete) — Splinter Review
Dietrich, thanks a lot for jumping in and doing the review :)

This patch addresses most of your review comments. Some notes:


> >+  /**
> >+   * Processes the request header.
> 
> what do you mean by process, and what's it do with the processed content? ditto
> for the processing methods following this.

> >+
> >+  /**
> >+   * Adds a list of key-values to the node with id=aPartenId.
> 
> 
> also, this comment doesn't describe what actually happens. add a note
> explaining that it builds actual HTML content for display?
 
I've added some more comments that explain what happen in each function. The _process* functions are now renamed to _display*.

> ftr, i think the styling needs work, but ui-review will weigh in on that.
> biggest issue to me is that the value data is not aligned against center
> vertical line, making the general appearance of the panel jumbled, and making
> it difficult for the eye to quickly scan the contents.

Will discuss this with Limi. I can see your point but I think as the key values are bold/black and the values are slightly grayed there is enough difference to keep the keys and values apart.

> >+      let cookies = request.header.Cookie.split(";");
> >+      let cookieList = {};
> >+      let cookieListSorted = {};
> >+      cookies.forEach(function(cookie) {
> >+        let name, value;
> >+        [name, value] = cookie.trim().replace("\n", "").split("=");
> 
> can this also be \r\n or \r? can't remember for cookie content...

Question: Does it need the "\n" here at all? The cookies are sent within the header which is separated by \r|\n|\r\n so there can't be any kind of linebreak in the header cookie string.

> >   unregisterDisplay: function HS_unregisterDisplay(aId)
> >   {
> >+    // Remove children from the output. If the output is not cleared, there can
> >+    // be leaks as some nodes has node.onclick = function; set and GC can't
> >+    // remove the nodes then.
> >+    HUDService.clearDisplay(aId);
> >+
> 
> why are you doing that then? why not use addEventListener and remove the
> listener when appropriate?

Well... you're right. However this is much easier to implement then keeping track of each node with an onclick event to clear it up later. That would be much more work and if we should do it, I would be glad to put that in a followup bug so that we don't block this bug for much longer.
Attachment #469729 - Attachment is obsolete: true
Attachment #469880 - Flags: review?(dietrich)
Attached patch Patch v6.1 (obsolete) — Splinter Review
Rebasing + added missing CSS file for gnome theme.
Attachment #469880 - Attachment is obsolete: true
Attachment #469913 - Flags: review?(dietrich)
Attachment #469880 - Flags: review?(dietrich)
> Will discuss this with Limi. I can see your point but I think as the key values
> are bold/black and the values are slightly grayed there is enough difference to
> keep the keys and values apart.

i will defer to judgement of the UX review! if you haven't already, ping Limi and ask if he's going to be able to review this immediately. if not, ask him what other UX person can review it immediately :)

> Question: Does it need the "\n" here at all? The cookies are sent within the
> header which is separated by \r|\n|\r\n so there can't be any kind of linebreak
> in the header cookie string.

good question, i don't know. look it up?

> Well... you're right. However this is much easier to implement then keeping
> track of each node with an onclick event to clear it up later. That would be
> much more work and if we should do it, I would be glad to put that in a
> followup bug so that we don't block this bug for much longer.

ok, we can live w/ it.
Comment on attachment 469913 [details] [diff] [review]
Patch v6.1


>   /**
>    * Assign a function to this property to listen for finsihed httpRequests.
>    * Used by unit tests.

typo. still not happy with this test-only code in here. file a followup to fix it later?

r=me otherwise.
Attachment #469913 - Flags: review?(dietrich) → review+
Comment on attachment 464843 [details]
"real 2"

I think this works fine for now, I'm still working on enhancements to the style panel that can probably be applied to this panel to improve whitespace & overall layout during the polish phase. I believe we have all the functionality we need.
Attachment #464843 - Flags: ui-review?(limi) → ui-review+
(In reply to comment #52)
> typo. still not happy with this test-only code in here. file a followup to fix
> it later?
fwiw, I allowed it in a different bug too.  We should strive to not have it though whenever possible.
(In reply to comment #54)
> (In reply to comment #52)
> > typo. still not happy with this test-only code in here. file a followup to fix
> > it later?
> fwiw, I allowed it in a different bug too.  We should strive to not have it
> though whenever possible.

Right, it's ugly. Before I stored the last request directly on the HUDService object but that caused some leaking. That's why I've introduced this callback. Will open a followup bug.
Blocks: 591509
Attached patch Patch v6.2 (obsolete) — Splinter Review
Rebased patch.
Rebased patch + modified to work with new patch in bug 568634.
Attachment #469913 - Attachment is obsolete: true
Attachment #470086 - Attachment is obsolete: true
Keywords: uiwantedcheckin-needed
blocking2.0: ? → beta5+
Summary: Click event handler and display of log entry details → Implement WebConsole Network Panel
Comment on attachment 470109 [details] [diff] [review]
[backed-out] Patch v6.3

http://hg.mozilla.org/mozilla-central/rev/fa93ab4adc7a
Attachment #470109 - Attachment description: Patch v6.3 → [checked-in] Patch v6.3
Comment on attachment 470109 [details] [diff] [review]
[backed-out] Patch v6.3

http://hg.mozilla.org/mozilla-central/rev/289e6da90526

leaks
Attachment #470109 - Attachment description: [checked-in] Patch v6.3 → [backed-out] Patch v6.3
http://hg.mozilla.org/mozilla-central/rev/e39d03333cdf

after tryserver success, one more push
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.