STR: (Win7_64, Nightly 44, 32bit, ID 20151005030206, new profile, safe mode) 1. Open browser console (Ctrl+Shift+J) 2. Make sure that you enabled option "Provide search suggestions" 3. Type something in search bar 4. Click on one of XHRs in browser console Result: Nothing happens Expectations: There should be a way to view XHRs from console, because it was possible in the past. I'm posting this, because it's presented on DevEdition, and it affects me.

Flagging as a regression & developer blocker - this used to work, but I think changed when the in-page version of devtools added the network tab. I guess it broke the "browser" console when that changed. Unfortunately for developers trying to work with Firefox, loosing the ability to easily inspect connections is a big loss.

Well, I think that "hardcore" devs still can use "Browser toolbox" to view XHRs, but if the "Browser console" is still enabled, it should work as expected. Here's a link to MDN (just in case): https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox

AIUI, this is somewhat "expected" by the change in bug 861335 which removes the network info pop-up. From this discussion in bug 1204035, it sounds like we're directing people to the browser toolbox if they want to view the network details. Still though, it is confusing today, since each network activity line is linked but goes nowhere when clicked in the browser console. :bgrins / :past, any thoughts on what to do here?

The Browser Toolbox is indeed the tool to inspect network requests from add-ons, etc. We might want to do bug 696385 for the Browser Console only.

I think getting rid of the underlined text and the cursor: pointer would be the easiest thing to prevent confusion here. However, firebug.next has a feature where you can expand net requests directly in the console. Honza, is there a bug on file / any plans to integrate this into the devtools repo?

(In reply to Brian Grinstead [:bgrins] from comment #5) > I think getting rid of the underlined text and the cursor: pointer would be > the easiest thing to prevent confusion here. > > However, firebug.next has a feature where you can expand net requests > directly in the console. Honza, is there a bug on file / any plans to > integrate this into the devtools repo? Yes, absolutely, the plan is to integrate it into the devtools repo. (and one of the things I'll start working on soon) I am happy to fix the Browser Console as part of that effort. No bug filed so far AFAIK. Honza

(In reply to Jan Honza Odvarko [:Honza] from comment #6) > (In reply to Brian Grinstead [:bgrins] from comment #5) > > I think getting rid of the underlined text and the cursor: pointer would be > > the easiest thing to prevent confusion here. > > > > However, firebug.next has a feature where you can expand net requests > > directly in the console. Honza, is there a bug on file / any plans to > > integrate this into the devtools repo? > Yes, absolutely, the plan is to integrate it into the devtools repo. > (and one of the things I'll start working on soon) > I am happy to fix the Browser Console as part of that effort. > > No bug filed so far AFAIK. Maybe we can use this bug for that

(In reply to Brian Grinstead [:bgrins] from comment #7) > Maybe we can use this bug for that Yep, works for me. Honza

How about opening the Network panel from within the Browser Console when clicking on a request? In case the request is from another tab, it would switch to the related tab/window. And in case the DevTools were not open yet, it would open them at the Network panel. This would make it work similar to the Web Console and would allow people to know in which tab/window the request was done (which you don't have using Firebug's solution). Sebastian

(In reply to Sebastian Zartner [:sebo] from comment #9) > How about opening the Network panel from within the Browser Console when > clicking on a request? > In case the request is from another tab, it would switch to the related > tab/window. And in case the DevTools were not open yet, it would open them > at the Network panel. > This would make it work similar to the Web Console and would allow people to > know in which tab/window the request was done (which you don't have using > Firebug's solution). That could be nice. Although I think a couple of things would have to be sorted out to make it work: 1) The network panel on the page's toolbox will not know of the request yet (since the webconsole client hasn't connected until the toolbox has opened). 2) Not all requests will have originated from a tab (for instance, requests triggered by chrome code) I think this should start with a product / UX discussion about if we want to add the expandable net requests feature in the console (aka Firebug's solution) or rely only on linking over to the netpanel. If we ultimately want to add that feature, then we should do that instead of somehow special casing the Browser Console. Helen, do you have an opinion about this? I found this article which explains how the feature works for more context: http://www.zyxware.com/articles/2642/debugging-tips-how-to-debug-ajax-requests-using-mozilla-firefox-and-firebug. The idea is that you can expand a network request from the console and see the same data that you would see in the network panel on that request. We currently link over to the network panel instead of showing it directly in the console.

(In reply to Brian Grinstead [:bgrins] from comment #11) > I think this should start with a product / UX discussion about if we want to > add the expandable net requests feature in the console (aka Firebug's > solution) or rely only on linking over to the netpanel. If we ultimately > want to add that feature, then we should do that instead of somehow special > casing the Browser Console. > > Helen, do you have an opinion about this? I found this article which > explains how the feature works for more context: > http://www.zyxware.com/articles/2642/debugging-tips-how-to-debug-ajax- > requests-using-mozilla-firefox-and-firebug. The idea is that you can expand > a network request from the console and see the same data that you would see > in the network panel on that request. We currently link over to the network > panel instead of showing it directly in the console. I think if we felt there was a certain amount of information devs wanted to quickly see from the console, a preview, and expanding in the console could allow devs keep their context in the console I'd be inclined to support this. But otherwise it feels more like we are just duplicating what the network panel already has. If we wanted to take this on. We could start by measuring the current clicks that open the network panel. This should give us a graph that ends up going down and to the right as people find the expansion useful enough for most tasks.

(In reply to Bryan Clark (Firefox PM) [:clarkbw] from comment #12) > I think if we felt there was a certain amount of information devs wanted to > quickly see from the console, a preview, and expanding in the console could > allow devs keep their context in the console I'd be inclined to support > this. Sounds good. Support for 'inline' XHR inspection in the Console panel is one of the most requested Firebug gaps (missing feature in DevTools) from Firebug community I've seen. Web-dev folks dealing with XHRs don't want to usually see the entire Network panel with every possible HTTP traffic detail. It's more important to have XHR response right at the finger tips (one click away). It's also important to avoid constant switching between the Console and Network panels. > But otherwise it feels more like we are just duplicating what the > network panel already has. From the UI point of view, the only additional thing in the (Console panel) UI is a small toggle icon displayed in front of XHR - allowing to expand/collapse XHR details. I believe that this isn't obstructive. From the technical point of view, UI components (based on React) used for XHR Spy will be reused by the Net panel (as part of XUL -> HTML effort), so no code duplication. I've also put together a doc describing features related to the XHR Inspection (aka XHR Spy) https://docs.google.com/document/d/1zQniwU_dkt-VX1qY1Vp-SWxEVA4uFcDCrtH03tGoHHM/edit# Honza

(In reply to Jan Honza Odvarko [:Honza] from comment #13) > [snip] > I've also put together a doc describing features related to the XHR > Inspection (aka XHR Spy) > https://docs.google.com/document/d/1zQniwU_dkt-VX1qY1Vp- > SWxEVA4uFcDCrtH03tGoHHM/edit# Ok, call me convinced.

(In reply to Jan Honza Odvarko [:Honza] from comment #13) > (In reply to Bryan Clark (Firefox PM) [:clarkbw] from comment #12) > > I think if we felt there was a certain amount of information devs wanted to > > quickly see from the console, a preview, and expanding in the console could > > allow devs keep their context in the console I'd be inclined to support > > this. > Sounds good. Support for 'inline' XHR inspection in the Console panel is one > of the most requested Firebug gaps (missing feature in DevTools) from > Firebug community I've seen. Looking over the Google doc, it seems like we're adding a large amount of information: http://cl.ly/3N393h2j3b3j I agree with Bryan that this should really only be trying to provide what users really need when debugging XHR requests. > Web-dev folks dealing with XHRs don't want to usually see the entire Network > panel with every possible HTTP traffic detail. It's more important to have > XHR response right at the finger tips (one click away). It's also important > to avoid constant switching between the Console and Network panels. > > > But otherwise it feels more like we are just duplicating what the > > network panel already has. > From the UI point of view, the only additional thing in the (Console panel) > UI is a small toggle icon displayed in front of XHR - allowing to > expand/collapse XHR details. I believe that this isn't obstructive. The toggle isn't intrusive, but the table itself is (full table with four tabs seems like a lot for a quick-preview kind of thing). I'm not arguing against the feature itself, but can we create a less feature-rich toggle without multiple tabs, with secondary/additional information being linked to in the Network panel?

(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #15) > I agree with Bryan that this should really only be trying to provide what > users really need when debugging XHR requests. I also agree and I think we should cover following UI/UX aspects: 1) Avoid endless switching between the Console and Network panel. This has been something people complained in the past when debugging XHR. It's so handy to stay in the Console, see all dynamically created console logs, JS errors, HTTP logs, etc. simultaneously and being able to inspect them right away (i.e. by one click). 2) I wouldn't be worried about amount of HTTP details displayed. It's well known structure and it's displayed only when the user explicitly request it (by expanding the XHR log). I rather think that the user should see the same info as if provided by the Network panel, but only related to the one specific XHR. 3) The way how HTTP data are presented in the Console panel should be consistent with the Network panel. In fact I have been planning to reuse the same UI components (ReactJS) in the Network panel as part of the XUL -> HTML effort. So, the user learns the UI/UX just once. 4) Also, keeping the feature compatible with the one from Firebug will help existing users to switch and adopt our devtools. > Looking over the Google doc, it seems like we're adding a large amount of information: > http://cl.ly/3N393h2j3b3j Yes, list of headers is always rather long, but note that individual sections can be collapsed (by default or by the user). My suggestion would be to collapse 'Headers coming from the cache' by default (not even on the screenshot), but I don't have strong opinion about it. Honza

(In reply to Jan Honza Odvarko [:Honza] from comment #16) > (In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #15) > > I agree with Bryan that this should really only be trying to provide what > > users really need when debugging XHR requests. > I also agree and I think we should cover following UI/UX aspects: > > 1) Avoid endless switching between the Console and Network panel. This has > been something people complained in the past when debugging XHR. It's so > handy to stay in the Console, see all dynamically created console logs, JS > errors, HTTP logs, etc. simultaneously and being able to inspect them right > away (i.e. by one click). Note that this bug refers to the Browser Console, where the requests from all pages are shown, not the Console panel. So, both solutions allow to see the logged data and the network data at the same time. And the network data will also be shown by one click in both solutions. My suggested solution has the disadvantage, that the UI is not inline (i.e. right where you clicked the request). Though it has the advantages that you can see in which tab a request was made and that it also works for network requests other than XHRs and that you can see more info about the request like its size, IP or origin (once bug 1230922 is implemented). > 3) The way how HTTP data are presented in the Console panel should be > consistent with the Network panel. In fact I have been planning to reuse the > same UI components (ReactJS) in the Network panel as part of the XUL -> HTML > effort. So, the user learns the UI/UX just once. Regarding the point above, what would happen in the case of requests that are not XHRs? And would this revert the changes from bug 861335, i.e. unify the UI between Browser Console and Console panel to show the requests inline instead of switching to the Network panel? A compromise between both solutions would be to show data for *all* requests inline (to have a consistent UI) but allow to switch to the Network panel via the context menu or some other UI. Sebastian

(In reply to Sebastian Zartner [:sebo] from comment #17) > Note that this bug refers to the Browser Console, where the requests from > all pages are shown, not the Console panel. > So, both solutions allow to see the logged data and the network data at the > same time. And the network data will also be shown by one click in both > solutions. This confused me too, but this bug is now being used for both the console and the browser console (since Honza's changes are for the console and thus will have impacts on the browser console). I'm going to change the name of the bug, maybe Honza can change it if the new name isn't quite right. > Regarding the point above, what would happen in the case of requests that > are not XHRs? And would this revert the changes from bug 861335, i.e. unify > the UI between Browser Console and Console panel to show the requests inline > instead of switching to the Network panel? > > A compromise between both solutions would be to show data for *all* requests > inline (to have a consistent UI) but allow to switch to the Network panel > via the context menu or some other UI. What are some examples of requests that are network requests in the console that are not XHR requests? I thought that XHR was an umbrella for all of them.

(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #18) > I'm going to change the name of the bug, maybe Honza can change it > if the new name isn't quite righ Great, thanks Helen. > What are some examples of requests that are network requests in the console > that are not XHR requests? I thought that XHR was an umbrella for all of > them. XHR requests are subset of HTTP requests. https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest XHR => XMLHttpRequest is used heavily in AJAX programming. An example of non XHR is e.g. the top level document load request. The Console panel allows the user to see all HTTP requests (not only XHR) and support for inline inspection (being discussed in this bug) can apply to all of them (not only XHR). In fact, I am already having the inline-inspection active for *all* HTTP request and it works just fine and it's definitely something I support to do. In other words, the toggle button would appear for every HTTP log in the Console, not just XHR. Honza

(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #18) > (In reply to Sebastian Zartner [:sebo] from comment #17) > > Note that this bug refers to the Browser Console, where the requests from > > all pages are shown, not the Console panel. > > So, both solutions allow to see the logged data and the network data at the > > same time. And the network data will also be shown by one click in both > > solutions. > This confused me too, but this bug is now being used for both the console > and the browser console (since Honza's changes are for the console and thus > will have impacts on the browser console). I'm going to change the name of > the bug, maybe Honza can change it if the new name isn't quite right. I've clarified it further. As Honza didn't react to the first part of comment 17, here's the summary of the two solutions again: 1. Show the requests inline by clicking on them Advantages: UI in one place, same behavior for browser and web console Disadvantages: missing information, duplicates Network panel UI, behavior of web console changes again 2. Show requests in Network panel Advantages: all request information visible, user knows in which tab/window a request happened Disadvantages: UI at different places, not conform with Firebug's behavior I want to note again that the solutions don't exclude each other. One may be the default, the other one available via a different action or preference. Sebastian

(In reply to Sebastian Zartner [:sebo] from comment #20) > As Honza didn't react to the first part of comment 17, here's the summary of > the two solutions again: The best is to NI me. > 1. Show the requests inline by clicking on them > Advantages: UI in one place, same behavior for browser and web console > Disadvantages: missing information, duplicates Network panel UI, behavior of > web console changes again (as mentioned in comment #16) I prefer displaying all information related to the clicked HTTP request and so, there are 'no missing information'. The user is mainly focused on clicked request in this case and seeing info about all HTTP requests can be rather counterproductive. But, I like having a context menu action like "Inspect In Net Panel" that would navigate the user to the Network panel if needed. This would be even consistent with actions like "Inspect In DOM Panel" etc. Also, I am not seeing 'duplicates Network panel UI' as disadvantage - I rather see it as sharing the same user experience well known from the Net panel, so the user doesn't have to learn anything new and doesn't have to switch panels either. But of course the rendering code is there just once (as soon as all is React). > 2. Show requests in Network panel > Advantages: all request information visible, user knows in which tab/window > a request happened > Disadvantages: UI at different places, not conform with Firebug's behavior Firebug compatibility is important here since another goal is to make Firebug users happy (also because this feature has been available for years and proven over time). > I want to note again that the solutions don't exclude each other. One may be > the default, the other one available via a different action or preference. This can be achieved by having the "Inspect In Net Panel" action. Note that I am thinking mainly about web-developers (the Console panel) since that's the most important case. At the same time, I am not seeing any reason why the Browser Console should be different. Honza

I don't feel like I'm adding to the conversation at this point; I do have some hesitation about duplication with the Network panel, but since for the moment we're working really hard to make Firebug users feel welcome I'm going to shelve my concerns and see how things develop. Honza, the Google doc with proposed changes looks good to me, let me know if you need any additional UI/UX work to be done.

(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #22) > I don't feel like I'm adding to the conversation at this point; I do have > some hesitation about duplication with the Network panel, Ok, I see. I am happy to have an extra meeting any time later or we can wait for user feedback and see what users say - and adjust the UX so, it also works for you :-) > but since for the > moment we're working really hard to make Firebug users feel welcome I'm > going to shelve my concerns and see how things develop. Honza, the Google > doc with proposed changes looks good to me, let me know if you need any > additional UI/UX work to be done. Alright, thanks! Honza

(In reply to Jan Honza Odvarko [:Honza] from comment #23) > (In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #22) > > I don't feel like I'm adding to the conversation at this point; I do have > > some hesitation about duplication with the Network panel, > Ok, I see. I am happy to have an extra meeting any time later or we can wait > for user feedback and see what users say - and adjust the UX so, it also > works for you :-) I think we should see how it gets used and adjust the UX accordingly (if it needs to be adjusted at all). Thanks Honza!

Ok, so I've filed bug 1238953 for the Network panel switch option. I've also adjusted the summary again, as it still wasn't quite right. Sebastian

I am attaching the current work (wip) so, it's available for testing. It's stable, but code clean up is needed before review. Honza

Updated patch (fixing CSS). Honza

:Honza, is this bug summary correct? In bug 1238881 comment 10, you said this is primarily for web devs. So I do not understand "unify behavior with web console" if we are adding new behavior to web console that does exist and leaving browser console alone. This also seems like entirely the wrong bug to use if we don't do anything in the browser console, given the original reporter's comment 0...

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #28) > :Honza, is this bug summary correct? In bug 1238881 comment 10, you said > this is primarily for web devs. So I do not understand "unify behavior with > web console" if we are adding new behavior to web console that does exist > and leaving browser console alone. The Browser console will be supporting this as well (as I've written in that comment) so, it's related. But, yes, ugh, I should have created another report, but see comment #6 and #7, ... and I decided to use this one. Let me know if it still makes sense to switch to another report and not make it even more confusing (given all the existing comments). Honza

Another patch update, more CSS clean up. Honza

(In reply to Jan Honza Odvarko [:Honza] from comment #29) > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #28) > > :Honza, is this bug summary correct? In bug 1238881 comment 10, you said > > this is primarily for web devs. So I do not understand "unify behavior with > > web console" if we are adding new behavior to web console that does exist > > and leaving browser console alone. > The Browser console will be supporting this as well (as I've written in that > comment) so, it's related. > > But, yes, ugh, I should have created another report, but see comment #6 and > #7, > ... and I decided to use this one. Let me know if it still makes sense to > switch to another report and not make it even more confusing > (given all the existing comments). Since this bug is already confusing enough, let's stick with it. But please update the summary a bit to describe the solution you are implementing and remove the "unify..." since new features are being added to the console (it doesn't seem like we are "unifying" anything). If the thing you are implementing won't solve the reporter's original problem, then it would be nice to at least file a new bug for their problem so it's not lost.

Alright, to make purpose of this report clear (hopefully): * This bug is about XHR inspection in the Console panel (aka webconsole) * Bug 1239665 is about XHR inspection in the Browser console The plan is implementing the feature in the Console panel first and consequently fix also the Browser console. Bug 1239665 depends on this bug report. Honza

I very close to asking for a review (James I am thinking about you, it's all React :-). I am only yet waiting for try results. https://treeherder.mozilla.org/#/jobs?repo=try&revision=42008ac2e31c General comments: 1) There are no tests yet. I am planning to write test for overall functionality only (mochitests), but I am happy to also do tests for React components if we have a consensus in time. Otherwise we can do it later. 2) It isn't a small patch, so I wrote md doc (it's part of the patch, see 'docs' dir) explaining both the UX and internal architecture. I am happy to improve/extend it. James I can also do live-doc-examples for 'reps' components. Do you have any related docs for this? I know you started something already... 3) James, I am happy to have 1:1 meeting about this if it'd help you to for the review...? 4) See this bug 1238881 comment #15 for STR how to test the feature (includes a link to a simple test page). 5) There shouldn't be any performance penalty when having this new feature on. All additional HTTP data are requested from the back-end on demand as the user expands/inspects the logs. The only additional thing that is happening to the Console UI automatically is adding a new class "xhrSpy" to every HTTP log in the console (to display a toggle button) and registering a click listener. Honza

Very out of the loop with this bug, but sure, I can review some code. I'd highly recommend writing xpcshell tests to test your individual React components. It doesn't matter what we choose in the future, it should be trivial to port them to whatever we choose. But that's going to be easier to port than no tests at all for each component. I don't think I'll be able to review in depth because I'm not familiar with this feature, our requirements, or any design constraints. Has Helen looked at this? I can review the code itself, however, and general React code. I'm still working on some docs but trying to figure out how to write a React style guide without just throwing it at people (want to get people's input). I should put at least some things up somewhere though.

Updated patch Honza

(In reply to James Long (:jlongster) from comment #34) > Very out of the loop with this bug, but sure, I can review some code. > > I'd highly recommend writing xpcshell tests to test your individual React > components. It doesn't matter what we choose in the future, it should be > trivial to port them to whatever we choose. But that's going to be easier to > port than no tests at all for each component. > > I don't think I'll be able to review in depth because I'm not familiar with > this feature, our requirements, or any design constraints. Has Helen looked > at this? Yes There is also a feature description here: https://docs.google.com/document/d/1zQniwU_dkt-VX1qY1Vp-SWxEVA4uFcDCrtH03tGoHHM/edit#heading=h.nrck3r9arqhn > I can review the code itself, however, and general React code. I'm > still working on some docs but trying to figure out how to write a React > style guide without just throwing it at people (want to get people's input). > I should put at least some things up somewhere though. OK, reviewing the general React style will be great. Honza

@James: can you please take look at the React code style? Note that there is a md doc in 'docs' directory (includes a list of all components at the end). You don't need any other patches to run this feature. Thanks! Honza

Test suite covering this features Honza

An minor update: * Better rendering of posted data * Code clean up @James, there is one thing I'd like to fix in a follow up when integrating with Redux. It's related to the way how components fetch data from the back-end (debugger server). It currently starts from render method and I'd like to move it to async action objects as soon as Redux is in place. Let me know if this is a blocker for this patch (believe me, I don't like it either!), but I am currently focusing rather on landing this feature not appending more and more stuff to it. Honza

Also updating the tests. Honza

Small update: * Changes in locale file * xhr/main.js included directly in the webconsole.xul (using <script> tag) * Rename: 'response-size-limit' component to 'size-limit' * Reuse size-limit component in the Post tab (already used in Response tab) * Fix Post tab when the post-body is not fully fetched (support for LongString) * Small CSS tweaks * Docs updated Honza

Currently looking through these patches. Sorry I haven't finished the review yet, lots of things came up this week. Nothing stands out to me yet, but I've mostly focused on individual files and don't understand how everything hooks together yet. This will be top priority for me to finish.

(In reply to James Long (:jlongster) from comment #47) > Currently looking through these patches. Sorry I haven't finished the review > yet, lots of things came up this week. Nothing stands out to me yet, but > I've mostly focused on individual files and don't understand how everything > hooks together yet. This will be top priority for me to finish. Thanks James. I am happy to have 1:1 meeting to give you an overview of the patch it that would help. Honza

Comment on attachment 8711003 [details] [diff] [review] bug1211525.patch Looks like this one wasn't made obsolete

I'll have the first review done today. FWIW I think it would be good if Brian looked at this as well. I don't know as much about the console and there are some significant changes here. Brian, can you glance it this and make sure it's sane? I'll do an in-depth review.

Comment on attachment 8712216 [details] [diff] [review] bug1211525.patch Review of attachment 8712216 [details] [diff] [review]: ----------------------------------------------------------------- The overall structure looks great. It's pretty consistent and it seems easy to refactor in the future when we have more opinions on how to integrate React. I didn't look at every detail, so I have no idea if how your are rendering HTML elements works completely or things like that, but I'm assuming it's fine. I was looking for style nits, React problems, or more general issues with the approach. Because this is a good bit of code, and it touches the console, I think it would be wise to get a second person to look through this, at least at a higher level. I hope Brian can take a look. Just a few nits below. ::: devtools/client/locales/en-US/xhr-spy.properties @@ +1,1 @@ > +# LOCALIZATION NOTE (xhrSpy.headers): A label used for Headers tab Does all this specifically need to be in a separate properties file? Shouldn't we have most of these already from the network panel? ::: devtools/client/webconsole/xhr/components/post-tab.js @@ +179,5 @@ > + group = { > + key: "raw", > + name: Locale.$STR("xhrSpy.rawData"), > + content: DOM.div({className: "netInfoResponseContent"}, > + safeString(text) Why do you need this? This string is passed through React so it should automatically be safely escaped. What escaping do you need that React doesn't do? @@ +192,5 @@ > + let actions = this.props.actions; > + let file = this.props.data; > + > + if (file.discardRequestBody) { > + return DOM.span({className: "netInfoBodiesDiscarded"}, I noticed that syntax quite a bit throughout your components, and I don't think it's conforms to our (unwritten) style guide... I think it would be better like: return DOM.span( {className: "netInfoBodiesDiscarded"}, Locale.$STR("xhrSpy.requestBodyDiscarded") ); In other places you have more properties and broken them up across lines, but the first entry is still on the same line as the element call, which is a bit strange. Doing it this way, you can add more properties and they all line up. In the effort of keeping the style consistent across code I would prefer this way. ::: devtools/client/webconsole/xhr/components/size-limit.js @@ +8,5 @@ > +// Shortcuts > +const DOM = React.DOM; > + > +/** > + * This template represents the 'Response' panel and is responsible Comment here is wrong ::: devtools/client/webconsole/xhr/data-provider.js @@ +63,5 @@ > + actor: actor, > + args: args > + }; > + > + var event = new MessageEvent("devtools/content/message", { I would prefer to avoid using native events and dispatching on the window. In several places you have explicit message logging, but if you use `EventEmitter` it supports that already; you can just turn on event logging and you'll see everything. I suggest making `DataProvider` an EventEmitter instance and dispatching events on it, and bind listeners to it. No need for this MessageEvent here, just `this.emit({ ... })`. ::: devtools/client/webconsole/xhr/xhr-spy.js @@ +41,5 @@ > + this.initialize(log); > +} > + > +XhrSpy.prototype = > +/** @lends XhrSpy */ nit: I've never seen this style elsewhere in the codebase, I'd prefer to remove the @lends and put the bracket on the same line.

I'm going to hold on on an r+ until I get a second person to glance at this and see if I'm missing anything crucial.

(In reply to James Long (:jlongster) from comment #50) > I'll have the first review done today. FWIW I think it would be good if > Brian looked at this as well. I don't know as much about the console and > there are some significant changes here. Brian, can you glance it this and > make sure it's sane? I'll do an in-depth review. I'll plan on taking a look at this tomorrow.

(In reply to Brian Grinstead [:bgrins] from comment #53) > (In reply to James Long (:jlongster) from comment #50) > > I'll have the first review done today. FWIW I think it would be good if > > Brian looked at this as well. I don't know as much about the console and > > there are some significant changes here. Brian, can you glance it this and > > make sure it's sane? I'll do an in-depth review. > > I'll plan on taking a look at this tomorrow. Thanks Brian, I was about to ask you for review too :-) Honza

(In reply to James Long (:jlongster) from comment #51) > Comment on attachment 8712216 [details] [diff] [review] > bug1211525.patch > > Review of attachment 8712216 [details] [diff] [review]: > ----------------------------------------------------------------- > > The overall structure looks great. It's pretty consistent and it seems easy > to refactor in the future when we have more opinions on how to integrate > React. I didn't look at every detail, so I have no idea if how your are > rendering HTML elements works completely or things like that, but I'm > assuming it's fine. I was looking for style nits, React problems, or more > general issues with the approach. Thanks James! > Because this is a good bit of code, and it touches the console, I think it > would be wise to get a second person to look through this, at least at a > higher level. I hope Brian can take a look. Yep, I was already planning to ask Brian :-) > Just a few nits below. > > ::: devtools/client/locales/en-US/xhr-spy.properties > @@ +1,1 @@ > > +# LOCALIZATION NOTE (xhrSpy.headers): A label used for Headers tab > > Does all this specifically need to be in a separate properties file? > Shouldn't we have most of these already from the network panel? Fixed I merged all into netmonitor.properties file. There are still some duplicates since many strings are defined in .dtd file, which is not scriptable. I'll fix this (remove the .dtd file) as part of the network panel refactoring. > ::: devtools/client/webconsole/xhr/components/post-tab.js > @@ +179,5 @@ > > + group = { > > + key: "raw", > > + name: Locale.$STR("xhrSpy.rawData"), > > + content: DOM.div({className: "netInfoResponseContent"}, > > + safeString(text) > > Why do you need this? This string is passed through React so it should > automatically be safely escaped. What escaping do you need that React > doesn't do? The problem is when uploading binary files. There is the following error without the conversion: JavaScript Error: "not well-formed" (reported by React) I am not exactly sure why React fails. STR: upload binary data, expand related XHR log and select the Post tab. > @@ +192,5 @@ > > + let actions = this.props.actions; > > + let file = this.props.data; > > + > > + if (file.discardRequestBody) { > > + return DOM.span({className: "netInfoBodiesDiscarded"}, > > I noticed that syntax quite a bit throughout your components, and I don't > think it's conforms to our (unwritten) style guide... I think it would be > better like: > > return DOM.span( > {className: "netInfoBodiesDiscarded"}, > Locale.$STR("xhrSpy.requestBodyDiscarded") > ); > > In other places you have more properties and broken them up across lines, > but the first entry is still on the same line as the element call, which is > a bit strange. Doing it this way, you can add more properties and they all > line up. In the effort of keeping the style consistent across code I would > prefer this way. Could we yet discuss this (perhaps involve others?). I don't have a strong opinion, but... When reading community documentation the style (mostly jsx) goes usually like as follows: <div className="comment"> <h2 className="commentAuthor"> {this.props.author} </h2> </div> Which I think corresponds to: DOM.div({className: "comment"}, DOM.h2({className: "commentAuthor"}, this.props.author ) ) (in my experience there is typically just one attribute, in most cases it's the className) That's why I used, for example (className on the same line as tag name): DOM.div({className: "paramsTabBox"}, DOM.div({className: "panelContent"}, NetInfoParams({params: data.request.queryString}) ) ) My 'feeling' is that it's better than the following (suggested) syntax: DOM.div( {className: "paramsTabBox"}, DOM.div( {className: "panelContent"}, NetInfoParams( {params: data.request.queryString}) ) ) I know, picking the right code style (especially for markup) is hard :-) > > ::: devtools/client/webconsole/xhr/components/size-limit.js > @@ +8,5 @@ > > +// Shortcuts > > +const DOM = React.DOM; > > + > > +/** > > + * This template represents the 'Response' panel and is responsible > > Comment here is wrong Fixed > > ::: devtools/client/webconsole/xhr/data-provider.js > @@ +63,5 @@ > > + actor: actor, > > + args: args > > + }; > > + > > + var event = new MessageEvent("devtools/content/message", { > > I would prefer to avoid using native events and dispatching on the window. > In several places you have explicit message logging, but if you use > `EventEmitter` it supports that already; you can just turn on event logging > and you'll see everything. > > I suggest making `DataProvider` an EventEmitter instance and dispatching > events on it, and bind listeners to it. No need for this MessageEvent here, > just `this.emit({ ... })`. I'd like to use EventEmitter, but let me explain first why I didn't... (Sorry if it's long! This might be part of a discussion about how to properly design toolbox panels in the new world of HTML and React). The entire XHR Spy feature is separated into two parts: 1) Chrome: the Console Overlay + main (chrome privileges) 2) Content: entire UI, all React components and user interaction logic (content privileges) add #1) The chrome part is just a small piece of the entire feature and its only purpose is represent a connection to the backend. It can be even smaller when we put the code into main.js as you suggested. There are actually only two important methods/events: - newLog: send new network logs (plain JSON packets) to the content (#2). - onRequestData: fetch more data about the net log from the backend (as requested by the content) and forward response back to the content (#2). add #2) The content part is where most of the code lives. The entire UI rendering and user action handling. This code doesn't require extra privileges, it's pure web app. The communication with the chrome (#1) is done through DOM events. There are only two important methods/events (you can guess): - onNewLog: Listen for new net logs (yes, plain JSON packets) coming from the chrome (#1) - requestData: ask for more data about the net log/actor as the user requires it. Note that, this dom-event-based-channel can be very simply replaced by standard HTTP requests (sent to in-product server) --- I've move one step further when working on the DOM panel. It uses the same technique, but for the entire panel (i.e. the entire panel UI is running in content). And also requesting more data (as the user is expanding DOM panel nodes) through a DOM event sent to the chrome scope. When building the DOM panel UI, I've actually made a simple webpack configuration (directly in our tree), bundled all required modules and developed using `webpack --watch`. Next, I want to spin up a web server (just like I did in bug 1240494 for the JSON Viewer) and use HTTP instead of DOM events. This would allow easily streaming the entire DOM panel UI through HTTP and access backend through RDP (I should make a demo for our meeting :) We can make the chrome part (#1) even more simple by using 'ports', see an example coming from Add-on SDK. https://github.com/mozilla/addon-sdk/blob/master/examples/actor-repl/data/index.html#L116 It allows sending/receiving RDP packets directly from the content. (so, we can avoid HTTP server atm) --- Also, having the entire UI running in a scope with no extra privileges is safe. (btw. I'm planning to reuse all the components in the Net panel and we could go towards the same approach). > > ::: devtools/client/webconsole/xhr/xhr-spy.js > @@ +41,5 @@ > > + this.initialize(log); > > +} > > + > > +XhrSpy.prototype = > > +/** @lends XhrSpy */ > > nit: I've never seen this style elsewhere in the codebase, I'd prefer to > remove the @lends and put the bracket on the same line. Fixed I am attaching updated patch. Honza

One little follow up related to the EventEmitter. The point why I didn't use it is that it requires chrome privileges and so, can't be used in content scope part of the XHR spy. Honza

Comment on attachment 8716357 [details] [diff] [review] bug1211525.patch Helen, flagging for ui-review because most of the review comments on this patch are going to be implementation-specific and not affect the UI. Can you please try out this patch and give some feedback? For the UI: the styling doesn't seem to match up with the rest of the toolbox in a few ways (gradients, fonts, tab rounding, etc) and it'd be great if you could have a pass at a mockup or at least pointing out ways we can make it blend in with the console. Also, any general UX feedback on the feature would be much appreciated!

Comment on attachment 8716357 [details] [diff] [review] bug1211525.patch (In reply to Brian Grinstead [:bgrins] from comment #57) > Comment on attachment 8716357 [details] [diff] [review] > bug1211525.patch > > Helen, flagging for ui-review because most of the review comments on this > patch are going to be implementation-specific and not affect the UI. Can > you please try out this patch and give some feedback? For the UI: the > styling doesn't seem to match up with the rest of the toolbox in a few ways > (gradients, fonts, tab rounding, etc) and it'd be great if you could have a > pass at a mockup or at least pointing out ways we can make it blend in with > the console. Also, any general UX feedback on the feature would be much > appreciated! Np! I have a few notes. UX: - The double scroll is a bit awkward. I think we might be better served by only allowing scroll in the console, instead of a double scroll within the XHR foldout and the console itself. - There's a pointer cursor for entire line, but only portions of the line are clickable and do anything (the '+' and the link). I would leave the pointer cursor as it is but have clicking that line anywhere but the link toggle the additional XHR information. (This gives a much bigger click space than the '+' button alone.) - Additional click-space notes: I took a small video of this which I'll attach, but the space you can click to open/collapse the accordion feels a little arbitrary. I'd contain it only to the line I mentioned above. - This is also a UI issue, but the hover state no longer means anything when you have the XHR expanded—it's just a blue background making the XHR not blend in with the console as well. I would remove the background even though it's technically :hover'd. UI (some of which probably aren't in the scope of this patch): - I notice the black bar on the left side for XHRs isn't the same color as the background color on the tag for <XHR>. It'd be nice if they were. - It'd be nice if we gave more right/left padding on those little tags in general, they feel a little cramped. - I notice we switch between our sans-serif and our monospace in these tables a lot, and I think it looks a little weird. I'd just use the monospace. - Also, looks like the entire table is bolded. Let's make left side labels bold, right side regular. - The table should probably have a max-width so things like Access-Control-Expose-Headers wraps a little sooner for readability. - I agree with bgrins that the '+' and '-' buttons are very... default-Firefox-y, not super great looking. We have some other '+' we're starting to use in other places, I'll attach that to the bug. - I'm not super opposed to the tabs, but I think we in general don't need to box things in as much—I'll attach a mockup, but I think we should try just removing the borders around most things. They don't seem super necessary.

(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #59) > Created attachment 8717311 [details] > side-by-side-mockup.png I generally like that mockup, though I have some notes myself: - The white background for the tab contents makes it hard to distinguish between the request lines. The tabs should be visibly split from the line by a background and/or a border. - The gray text color suggests "deactivatedness". It should be darker. - There may a visual difference be made between the tabs and the headers, e.g. by making the border line lighter for the headers. Sebastian

(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #58) Thanks Helen! > UX: > - The double scroll is a bit awkward. I think we might be better served by > only allowing scroll in the console, instead of a double scroll within the > XHR foldout and the console itself. Fixed Note that console.trace() is doing the same (double scrollbars). We should make the stack trace info hidden by default and append a toggle button to open it. > - There's a pointer cursor for entire line, but only portions of the line > are clickable and do anything (the '+' and the link). I would leave the > pointer cursor as it is but have clicking that line anywhere but the link > toggle the additional XHR information. (This gives a much bigger click space > than the '+' button alone.) Good point, fixed. You can now click anywhere between the + toggle button (button included) and the far right side except of the URL and Status. > - Additional click-space notes: I took a small video of this which I'll > attach, but the space you can click to open/collapse the accordion feels a > little arbitrary. I'd contain it only to the line I mentioned above. > - This is also a UI issue, but the hover state no longer means anything when > you have the XHR expanded—it's just a blue background making the XHR not > blend in with the console as well. I would remove the background even though > it's technically :hover'd. Done > > UI (some of which probably aren't in the scope of this patch): > - I notice the black bar on the left side for XHRs isn't the same color as > the background color on the tag for <XHR>. It'd be nice if they were. Fixed > - It'd be nice if we gave more right/left padding on those little tags in > general, they feel a little cramped. Added couple of pixels left and right. > - I notice we switch between our sans-serif and our monospace in these > tables a lot, and I think it looks a little weird. I'd just use the > monospace. Before I change it, let me explain the the concept behind the fonts: I am using sans-serif for labels and messages (such as tab name or section name) and monospace for actual values (e.g. response body, header value, url param value, etc.) consistently. And this is especially nice in case of the Response body (or post body). When the response isn't available (e.g. not recorded on the backend) there is a message saying 'Response body is not not available'. The message is using sans-serif as opposed to the actual response body data that is using monospace. So, it's visually clear that the message isn't the actual response body. Similarly there is a message + link for fetching the rest of the response body data from the backend (if limit has been reached). Again, the message is using sans-serif to clarify that it's a helper message not data. You might want to see the difference in the doc (there is a screenshot in Response tab chapter) https://docs.google.com/document/d/1zQniwU_dkt-VX1qY1Vp-SWxEVA4uFcDCrtH03tGoHHM/edit# We might want to pick different fonts, but the distinction is good. > - Also, looks like the entire table is bolded. Let's make left side labels > bold, right side regular. Ah, yes, this is a bug, fixed! > - The table should probably have a max-width so things like > Access-Control-Expose-Headers wraps a little sooner for readability. Yes, agree. The issue is even more visible in the response body (response body can be one giant line of minified js source). The question is what the max-width should be. We don't wan to wrap the lines after 80 chars if folks are using 30" monitors. Also in case of a raw response body it isn't visible where are real end of lines after wrapping, also copy to clipboard should do the right thing (so no extra chars should be used). It would be great if we can discuss this more and come up with proper solution. A follow up? > - I agree with bgrins that the '+' and '-' buttons are very... > default-Firefox-y, not super great looking. We have some other '+' we're > starting to use in other places, I'll attach that to the bug. Done. Could I have versions for the Dark theme as well? (the buttons are background images so, inverting would apply on labels too) > - I'm not super opposed to the tabs, but I think we in general don't need to > box things in as much—I'll attach a mockup, but I think we should try just > removing the borders around most things. They don't seem super necessary. Done Note that the visual things like colors are fixed for the light/dark themes and kept for the Firebug theme. Patch updated Honza

Update patch (forgot to include the icons) Honza

(In reply to Jan Honza Odvarko [:Honza] from comment #63) > Created attachment 8717436 [details] [diff] [review] > bug1211525.patch > > - I notice we switch between our sans-serif and our monospace in these > > tables a lot, and I think it looks a little weird. I'd just use the > > monospace. > Before I change it, let me explain the the concept behind the fonts: > > I am using sans-serif for labels and messages (such as tab name or section > name) and monospace for actual values (e.g. response body, header value, url > param value, etc.) consistently. > > And this is especially nice in case of the Response body (or post body). > When the response isn't available (e.g. not recorded on the backend) there > is a message saying 'Response body is not not available'. The message is > using sans-serif as opposed to the actual response body data that is using > monospace. So, it's visually clear that the message isn't the actual > response body. > Similarly there is a message + link for fetching the rest of the response > body data from the backend (if limit has been reached). Again, the message > is using sans-serif to clarify that it's a helper message not data. > > You might want to see the difference in the doc (there is a screenshot in > Response tab chapter) > https://docs.google.com/document/d/1zQniwU_dkt-VX1qY1Vp- > SWxEVA4uFcDCrtH03tGoHHM/edit# > > We might want to pick different fonts, but the distinction is good. I still think we can get away with using the same font with different weights—still denotes that they're different without having a weird visual discrepancy. Could you try it out and mark me for review on a patch and we'll make a definitive decision then? > > - The table should probably have a max-width so things like > > Access-Control-Expose-Headers wraps a little sooner for readability. > Yes, agree. > The issue is even more visible in the response body (response body can be > one giant line of minified js source). The question is what the max-width > should be. We don't wan to wrap the lines after 80 chars > if folks are using 30" monitors. Also in case of a raw response body it > isn't visible where are real end of lines after wrapping, also copy to > clipboard should do the right thing (so no extra chars should be used). It > would be great if we can discuss this more and come up with proper solution. > A follow up? I actually think even on 30" monitors we should probably wrap ~70 or so characters. There's an idea (even with lots of whitespace) of an optimal reading length. (http://baymard.com/blog/line-length-readability) It does sound like there are some edge cases here though, like you said. Can you log a bug and we'll discuss copy-to-clipboard and large response bodies? > > - I agree with bgrins that the '+' and '-' buttons are very... > > default-Firefox-y, not super great looking. We have some other '+' we're > > starting to use in other places, I'll attach that to the bug. > Done. Could I have versions for the Dark theme as well? > (the buttons are background images so, inverting would apply on labels too) Yeah, of course. I'll mark myself needinfo? until I get you those.

Comment on attachment 8717438 [details] [diff] [review] bug1211525.patch Removing myself from review for the moment since I have some additional comments + added the dark icons above.

(In reply to Jan Honza Odvarko [:Honza] from comment #63) > > - I agree with bgrins that the '+' and '-' buttons are very... > > default-Firefox-y, not super great looking. We have some other '+' we're > > starting to use in other places, I'll attach that to the bug. > Done. Could I have versions for the Dark theme as well? > (the buttons are background images so, inverting would apply on labels too) With the dark / light icons I've generally tried to apply icons to a separate element and then use the dark theme variation as the main icon, so that we can do inversion and not keep the duplicate icons in tree. Would it be possible to make a new '.devtools-expander' kind of element (could be a button) so that we can re-use it later on and not need the duplicate icons? Or possibly make the label have that class and then apply the image to ::before similar to how .devtools-button does it.

Comment on attachment 8717438 [details] [diff] [review] bug1211525.patch Review of attachment 8717438 [details] [diff] [review]: ----------------------------------------------------------------- I did a pretty detailed code review. Since this is such a big patch, I think a third, higher level architecture review would be good to have. There are a few overarching thoughts: 1. Wherever we are using this.props, we should have a corresponding PropTypes. This defines what properties a component expects. This helps both in testing and also as documentation for consumers of the component. In the case where a component uses a nested value of a property, this can be defined using the shape() method. However, we should limit our use of nesting. This will need to be added to every component, but in the comments below, I only called it where I had another comment about the property. 2. We are very close to having all of the webconsole files passing ESLint, thanks to the work of a contributor. Before we land this, let's make sure that it passes ESLint too. ::: devtools/client/jsonview/components/reps/element.js @@ +18,5 @@ > + > +/** > + * @rep Generic XML/HTML element template. > + */ > +var Element = React.createClass( See xml-view.js. Do we want to maintain this? Or is this something a library could do for us? @@ +19,5 @@ > +/** > + * @rep Generic XML/HTML element template. > + */ > +var Element = React.createClass( > +/** @lends Element */ If I read the comments correctly, you were planning to remove the @lends statements. I think this one was missed. @@ +193,5 @@ > +/** > + * A template for element attribute > + */ > +var Attr = React.createClass( > +/** @lends Attr */ See comment above about @lends @@ +241,5 @@ > +}; > + > +// Registration > + > +function supportsObject(object, type) { is supportsObject used anywhere? @@ +253,5 @@ > + > + return /^HTML.*Element$/.test(type); > +} > + > +const ElementFactory = React.createFactory(Element); is ElementFactory used anywhere? ::: devtools/client/jsonview/components/reps/str.js @@ +1,1 @@ > +/* See license.txt for terms of usage */ Is there a reason this has a different license block? ::: devtools/client/jsonview/components/reps/url.js @@ +1,1 @@ > +/* See license.txt for terms of usage */ See comment above about license. ::: devtools/client/locales/en-US/netmonitor.properties @@ +276,5 @@ > +# LOCALIZATION NOTE (xhrSpy.response): A label used for Response tab > +# This tab displays HTTP response body > +xhrSpy.response=Response > + > +# LOCALIZATION NOTE (xhrSpy.response): A label used for a section Comment doesn't match @@ +293,5 @@ > +# LOCALIZATION NOTE (xhrSpy.sizeLimitMessage): A label used > +# in Response and Post tabs in case the body is bigger than given limit. > +# It allows the user to click and fetch more from the backend. > +# The <a> markup in this message must be retained. > +xhrSpy.sizeLimitMessage=Size limit has been reached. Click <a>here</a> to load more. I could be wrong on this, but the a tag is a little confusing. I don't see a precedent for having an empty a tag in a property. Is there a more standard way of handling this case? ::: devtools/client/locales/en-US/xhr-spy.properties @@ +6,5 @@ > +# section in the Header tab. The section displays HTTP response headers. > +xhrSpy.responseHeaders=Response Headers > + > +# LOCALIZATION NOTE (xhrSpy.requestHeaders): A label used as a title for > +# section in the Header tab. The section displays HTTP response headers. s/response/request @@ +22,5 @@ > +# LOCALIZATION NOTE (xhrSpy.response): A label used for Response tab > +# This tab displays HTTP response body > +xhrSpy.response=Response > + > +# LOCALIZATION NOTE (xhrSpy.response): A label used for a section Comment doesn't match @@ +43,5 @@ > +# LOCALIZATION NOTE (xhrSpy.sizeLimitMessage): A label used > +# in Response and Post tabs in case the body is bigger than given limit. > +# It allows the user to click and fetch more from the backend. > +# The <a> markup in this message must be retained. > +xhrSpy.sizeLimitMessage=Size limit has been reached. Click <a>here</a> to load more. See comment about <a> in netmonitor.properties above. ::: devtools/client/webconsole/xhr/components/headers-tab.js @@ +21,5 @@ > + render: function() { > + let data = this.props.data; > + let actions = this.props.actions; > + let responseHeaders = data.response.headers; > + let requestHeaders = data.request.headers; See comment about destructuring ::: devtools/client/webconsole/xhr/components/net-info-body.js @@ +19,5 @@ > + * This template renders the basic Network log info body. It's not > + * visible by default, the user needs to expand the network log > + * to see it. > + * > + * There is set of tabs displaying details about network events: Nit - s/There is/This is the @@ +31,5 @@ > + displayName: "NetInfoBody", > + > + getDefaultProps: function () { > + return { > + tabActive: 1 1. I don't think this needs to be a property on this component. It doesn't look like XhrSpy passes in a value for this. It seems like maybe the Tabs component could manage this. If it does need to be on this component, it should be documented with a PropType. 2. This prop name made me think it was a boolean—whether the tab was active or not. But it seems to be the index of the active tab. I would suggest "activeTab" instead of "tabActive". I know that the existing Tabs component uses tabActive, so if we want to change it, it could be addressed in a follow up. @@ +52,5 @@ > + > + hasCookies: function() { > + var data = this.state.data; > + var request = data.request; > + var response = data.response; I think this would be clearer with destructuring. > {request, response} = this.state.data; @@ +62,5 @@ > + getTabPanels: function() { > + var actions = this.props.actions; > + var data = this.state.data; > + var request = data.request; > + var response = data.response; See comment above about destructuring. @@ +73,5 @@ > + var panels = []; > + > + // Headers tab > + panels.push( > + TabPanel({className: "headers", key: "headers", You and James had a discussion above about the code style for the properties param. I agree with you that the opening brace of the props object should be on the first line. I've seen that used in the docs (e.g. at the bottom of React's Displaying Data doc). So if you have a single property, it should go on that same line. However, if you have multiple properties in the object, I think it should be split up to 1 property per line, as shown in the Mozilla Code Style. e.g. > TabPanel({ > className: "headers", > key: "headers" > ... ::: devtools/client/webconsole/xhr/components/net-info-groups.js @@ +15,5 @@ > +var NetInfoGroupList = React.createClass({ > + displayName: "NetInfoGroupList", > + > + render: function() { > + var groups = this.props.groups; This is a case where we should use PropTypes.shape() to define what properties group needs. @@ +46,5 @@ > + displayName: "NetInfoGroup", > + > + getDefaultProps: function() { > + return { > + open: true, Does open get passed in from a parent anywhere? If not, this can probably just be state. If it does need to be a prop, it should have PropTypes definition. ::: devtools/client/webconsole/xhr/components/params-tab.js @@ +6,5 @@ > +const React = require("devtools/client/shared/vendor/react"); > +const { createFactories } = require("devtools/client/jsonview/components/reps/rep-utils"); > +const { NetInfoParams } = createFactories(require("./net-info-groups")); > + > +// Constants This comment says "Canstants" in some cases, and "Shortcuts" in others. I'd recommend either sticking with Shortcuts or removing the comment altogether. @@ +18,5 @@ > +var ParamsTab = React.createClass({ > + displayName: "ParamsTab", > + > + render: function() { > + var actions = this.props.actions; This isn't used, so it should be removed. If it was used, it should have a PropTypes definition. @@ +19,5 @@ > + displayName: "ParamsTab", > + > + render: function() { > + var actions = this.props.actions; > + var data = this.props.data; This should have a PropType definition. Since the component only uses the request, maybe the prop that is passed in should be request instead of data. If you do keep it as a nested object under data, then it should use the .shape() method to describe data. ::: devtools/client/webconsole/xhr/components/post-tab.js @@ +29,5 @@ > + */ > +var PostTab = React.createClass({ > + displayName: "PostTab", > + > + isJson: function(file) { There are a lot of methods on the class here that feel like they could be helpers outside of the class. @@ +91,5 @@ > + var value = XhrUtils.getHeaderValue(file.request.headers, "content-type"); > + return XhrUtils.isHTML(value); > + }, > + > + renderXml: function(file) { Would these render helpers make sense as different components? @@ +210,5 @@ > + // Render post body data. The right representation of the data > + // is picked according to the content type. > + let groups = []; > + groups.push(this.renderUrlEncoded(file)); > + //groups.push(this.renderMultiPart(file)); Commented out code. Should this be a TODO? ::: devtools/client/webconsole/xhr/components/response-tab.js @@ +19,5 @@ > + > +// Shortcuts > +const DOM = React.DOM; > + > +const trace = { See other comment about trace. @@ +46,5 @@ > + > + return Json.isJSON(content.mimeType, content.text); > + }, > + > + parseJson: function(file) { Can any of these helpers be shared with post-tab.js? @@ +199,5 @@ > + * 2) Raw response data (always displayed if not discarded) > + */ > + render: function() { > + var actions = this.props.actions; > + var file = this.props.data; THe PropTypes definition for this should include all properties handled in helper renderers. ::: devtools/client/webconsole/xhr/components/xml-view.js @@ +12,5 @@ > + > +// Shortcuts > +const DOM = React.DOM; > + > +var XmlView = React.createClass({ 1. Do we need to handle XML serialization manually? Could we possibly delegate to a library that handles it for us? 2. If we do keep it, is there a plan to have XmlView do anything besides going down one level in the prop nesting? If not, should the two be combined? @@ +29,5 @@ > + * This template is responsible for rendering entire XML element > + * including optional attributes and children. > + */ > +var CompleteElement = > +/** @lends CompleteElement */ See comment above about lends @@ +161,5 @@ > +} > + > +// Registration > + > +const CompleteElementClass = React.createClass(CompleteElement); In most of the other files you use a different pattern. Instead of declaring a variable with the specification, the spec object is defined when it's passed in. ::: devtools/client/webconsole/xhr/console-overlay.js @@ +93,5 @@ > + this.panelFrame = panelFrame; > + }, > + > + onBuild: function(eventId, panel) { > + trace.log("ConsoleOverlay.onReady;"); This logs onReady, but the function is onBuild. Should this switch? @@ +113,5 @@ > + }); > + }, > + > + onReady: function(eventId, panel) { > + trace.log("ConsoleOverlay.onBuild;"); See comment above. ::: devtools/client/webconsole/xhr/data-provider.js @@ +19,5 @@ > + * Communication with the chrome scope is based on message > + * exchange. > + */ > +var DataProvider = > +/** @lends DataProvider */ See comment above about @lends ::: devtools/client/webconsole/xhr/docs/xhr-spy.md @@ +1,1 @@ > +# XHR Inspect (aka XHR Spy) This document should probably go in the (new-ish) devtools/docs directory @@ +10,5 @@ > +_XHR Spy feature is available in the Console panel (for web developers) > +as well as in the Browser Console (for devtools and extension developers)._ > + > +The current implementation is based on React (no XUL) and some of the existing > +components should also be used when porting the Network panel to HTML. The part talking about how components should also be used when porting Network panel to HTML sounds like something that should be captured in an issue rather than docs. @@ +79,5 @@ > +The response is parsed using DOM parser and displayed as an XML markup. > + > +[1] List of JSON mime-types: `devtools/client/webconsole/xhr/utils/json` > +[2] List of Image mime-types: `devtools/client/webconsole/xhr/utils/json` > +[3] List of XML mime-types: `devtools/client/webconsole/xhr/utils/json` I didn't see the list of image or xml mime-types in this file. @@ +159,5 @@ > +`devtools/client/webconsole/xhr/xhr-spy` > + > +This module represents `XhrSpy` object. It's the internal representation > +of HTTP request and it keeps its state. All HTTP details fetched dynamically > +from the backed are stored in this object. s/backed/backend @@ +192,5 @@ > +UI of XHR Spy is rendering data coming from the backend (the debugger server). > +The fetching process is asynchronous and needs to cross content/chrome boundary > +as well as use RDP protocol. > + > +This is how to flow looks like: s/how to/what the @@ +196,5 @@ > +This is how to flow looks like: > +1) The user expands XHR log and the Headers tab is selected by default. > +2) The UI component calls `requestData` action. > +3) The action checks a response-cache. If the response has been already > +fetched it's returned; otherwise `DataProvider` is used to fetch the data. s/fetched it's/fetched, it's @@ +197,5 @@ > +1) The user expands XHR log and the Headers tab is selected by default. > +2) The UI component calls `requestData` action. > +3) The action checks a response-cache. If the response has been already > +fetched it's returned; otherwise `DataProvider` is used to fetch the data. > +4) `DataProvider` checks if there isn't an existing pending request, Using "isn't" here and then saying "if yes" and "if no" is a little confusing. Does "if yes" mean that there isn't an existing pending request? Or that there is? ::: devtools/client/webconsole/xhr/main.js @@ +2,5 @@ > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > +"use strict"; > + > +var { classes: Cc, interfaces: Ci, utils: Cu } = Components; Do you need Cc and Ci here? ::: devtools/client/webconsole/xhr/xhr-spy.js @@ +3,5 @@ > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > +"use strict"; > + > +// React > +const ReactDOM = require("devtools/client/shared/vendor/react-dom"); s/ReactDOM/DOM/ This makes it consistent with all of the others. @@ +7,5 @@ > +const ReactDOM = require("devtools/client/shared/vendor/react-dom"); > + > +// Reps > +const { createFactories } = require("devtools/client/jsonview/components/reps/rep-utils"); > +const { Url } = require("devtools/client/jsonview/components/reps/url"); Discussed with Brian. We would prefer not to couple the webconsole and jsonview. Let's move all the reps to shared. @@ +16,5 @@ > +const { DataProvider } = require("./data-provider"); > + > +// Helper tracer. Should be generic sharable by other modules (bug 1171927) > +const trace = { > + log: function() { There are calls to trace.log() throughout. Does that currently do anything? We should land trace before this patch, or remove all of the trace calls. @@ +235,5 @@ > + > + this.body.setState(newState); > + }, > + > + // Actions Since we will be using Redux, we should limit the use of the word actions. In Redux, an action is an object. That object has a type and a payload.

(In reply to Lin Clark [:linclark] from comment #72) > ::: devtools/client/jsonview/components/reps/element.js > @@ +18,5 @@ > > + > > +/** > > + * @rep Generic XML/HTML element template. > > + */ > > +var Element = React.createClass( > > See xml-view.js. Do we want to maintain this? Or is this something a library > could do for us? I believe the netmonitor is using CodeMirror for this (which does syntax highlighting / code collapsing / etc). Honza, could the xml preview be handled by this mechanism instead? And/or could we push special XML previewing into another bug?

Comment on attachment 8717438 [details] [diff] [review] bug1211525.patch Review of attachment 8717438 [details] [diff] [review]: ----------------------------------------------------------------- A couple general notes: 1) I think this should move into client/shared/components and out of webconsole. This is because we will want to use the same component for the netmonitor in the future, and the concept of 'display a network request' is a useful shared component within the toolbox. 2) This isn't just for xhr requests, so I think the component should be called something else. Maybe net-request? ::: devtools/client/webconsole/xhr/console-overlay.js @@ +29,5 @@ > + "webconsole/xhr/components/net-info-groups", > + "webconsole/xhr/components/response-tab" > +]; > + > +// TODO: The localization should be updated as soon as the team agrees If you want to leave this as a TODO, then let's file another bug. But if we are going to do a simple access on a properties file for now, I think it's OK to ship without a TODO (since all of our code will have the same migration path) @@ +98,5 @@ > + > + this.panel = panel; > + > + let win = this.panelFrame.contentWindow; > + win.addEventListener("devtools/content/message", The distinction between chrome and content here is a little confusing given our widely used 'content' term to mean the content process or page's global. I understand this component is content privileged, but it's still running in the parent process, right? This message passing has been the trickiest part for me to figure out so far. ::: devtools/client/webconsole/xhr/xhr-spy.js @@ +106,5 @@ > + // about the XHR. > + this.parentNode.classList.add("xhrSpy"); > + > + // Register a click listener. > + this.addClickListener(); This sort of injecting behavior / DOM onto an existing message seems like something that might break in the future (especially if the messages themselves become Components). I don't have a better suggestion right now, but we should think about how this would attach if the message was a component and not a plain DOM element.

Here's a weird overflow on cnn.com. I think this isn't happening without the patch applied

Screenshot of an image not loading on cnn.com, just an empty data URI is rendered

(In reply to Brian Grinstead [:bgrins] from comment #75) > Created attachment 8717727 [details] > Here's a weird overflow on cnn.com. I think this isn't happening without the patch applied That's bug 903676

Oh, sorry. If you meant status "[HTTP/1.1 200 OK 0ms]" displayed on the second line of some messages - then it clearly doesn't happen in the latest Nightly.

(In reply to Lin Clark [:linclark] from comment #72) > Comment on attachment 8717438 [details] [diff] [review] > bug1211525.patch > > Review of attachment 8717438 [details] [diff] [review]: > ----------------------------------------------------------------- Thanks for detailed review Lin! > I did a pretty detailed code review. Since this is such a big patch, I think > a third, higher level architecture review would be good to have. > > There are a few overarching thoughts: > > 1. Wherever we are using this.props, we should have a corresponding > PropTypes. This defines what properties a component expects. This helps both > in testing and also as documentation for consumers of the component. Done > In the case where a component uses a nested value of a property, this can be > defined using the shape() method. However, we should limit our use of > nesting. There are some such cases now, but agree that we should limit it. > This will need to be added to every component, but in the comments below, I > only called it where I had another comment about the property. > > 2. We are very close to having all of the webconsole files passing ESLint, > thanks to the work of a contributor. Before we land this, let's make sure > that it passes ESLint too. Done > ::: devtools/client/jsonview/components/reps/element.js > @@ +18,5 @@ > > + > > +/** > > + * @rep Generic XML/HTML element template. > > + */ > > +var Element = React.createClass( > > See xml-view.js. Do we want to maintain this? Or is this something a library > could do for us? I filled bug 1247392 > > @@ +19,5 @@ > > +/** > > + * @rep Generic XML/HTML element template. > > + */ > > +var Element = React.createClass( > > +/** @lends Element */ > > If I read the comments correctly, you were planning to remove the @lends > statements. I think this one was missed. You are right, this was missing (this file is actually gone now since the xml preview is moved into another bug). > > @@ +193,5 @@ > > +/** > > + * A template for element attribute > > + */ > > +var Attr = React.createClass( > > +/** @lends Attr */ > > See comment above about @lends All @lends removed > > @@ +241,5 @@ > > +}; > > + > > +// Registration > > + > > +function supportsObject(object, type) { > > is supportsObject used anywhere? Yes, it's used in jsonview/components/reps/rep.js The method is utilized by an algorithm that picks the right component that should render given data object. The component that 'supports' it. The component that returns true from the method is used. > > @@ +253,5 @@ > > + > > + return /^HTML.*Element$/.test(type); > > +} > > + > > +const ElementFactory = React.createFactory(Element); > > is ElementFactory used anywhere? Not now (since xml preview will be done in a follow up) > ::: devtools/client/jsonview/components/reps/str.js > @@ +1,1 @@ > > +/* See license.txt for terms of usage */ > > Is there a reason this has a different license block? Fixed > > ::: devtools/client/jsonview/components/reps/url.js > @@ +1,1 @@ > > +/* See license.txt for terms of usage */ > > See comment above about license. Fixed > > ::: devtools/client/locales/en-US/netmonitor.properties > @@ +276,5 @@ > > +# LOCALIZATION NOTE (xhrSpy.response): A label used for Response tab > > +# This tab displays HTTP response body > > +xhrSpy.response=Response > > + > > +# LOCALIZATION NOTE (xhrSpy.response): A label used for a section > > Comment doesn't match Fixed > > @@ +293,5 @@ > > +# LOCALIZATION NOTE (xhrSpy.sizeLimitMessage): A label used > > +# in Response and Post tabs in case the body is bigger than given limit. > > +# It allows the user to click and fetch more from the backend. > > +# The <a> markup in this message must be retained. > > +xhrSpy.sizeLimitMessage=Size limit has been reached. Click <a>here</a> to load more. > > I could be wrong on this, but the a tag is a little confusing. I don't see a > precedent for having an empty a tag in a property. Is there a more standard > way of handling this case? Good point, fixed. I've used approach recommended here: https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Avoid_unnecessary_complexity_in_strings > > ::: devtools/client/locales/en-US/xhr-spy.properties > @@ +6,5 @@ > > +# section in the Header tab. The section displays HTTP response headers. > > +xhrSpy.responseHeaders=Response Headers > > + > > +# LOCALIZATION NOTE (xhrSpy.requestHeaders): A label used as a title for > > +# section in the Header tab. The section displays HTTP response headers. > > s/response/request > > @@ +22,5 @@ > > +# LOCALIZATION NOTE (xhrSpy.response): A label used for Response tab > > +# This tab displays HTTP response body > > +xhrSpy.response=Response > > + > > +# LOCALIZATION NOTE (xhrSpy.response): A label used for a section > > Comment doesn't match xhr-spy.properties file removed, all strings are now in netmonitor.properties file Comments fixed > > @@ +43,5 @@ > > +# LOCALIZATION NOTE (xhrSpy.sizeLimitMessage): A label used > > +# in Response and Post tabs in case the body is bigger than given limit. > > +# It allows the user to click and fetch more from the backend. > > +# The <a> markup in this message must be retained. > > +xhrSpy.sizeLimitMessage=Size limit has been reached. Click <a>here</a> to load more. > > See comment about <a> in netmonitor.properties above. Fixed > > ::: devtools/client/webconsole/xhr/components/headers-tab.js > @@ +21,5 @@ > > + render: function() { > > + let data = this.props.data; > > + let actions = this.props.actions; > > + let responseHeaders = data.response.headers; > > + let requestHeaders = data.request.headers; > > See comment about destructuring Destructuring used. > > ::: devtools/client/webconsole/xhr/components/net-info-body.js > @@ +19,5 @@ > > + * This template renders the basic Network log info body. It's not > > + * visible by default, the user needs to expand the network log > > + * to see it. > > + * > > + * There is set of tabs displaying details about network events: > > Nit - s/There is/This is the Fixed > > @@ +31,5 @@ > > + displayName: "NetInfoBody", > > + > > + getDefaultProps: function () { > > + return { > > + tabActive: 1 > > 1. I don't think this needs to be a property on this component. It doesn't > look like XhrSpy passes in a value for this. It seems like maybe the Tabs > component could manage this. If it does need to be on this component, it > should be documented with a PropType. Documented with a PropType now. > 2. This prop name made me think it was a booleanówhether the tab was active > or not. But it seems to be the index of the active tab. I would suggest > "activeTab" instead of "tabActive". I know that the existing Tabs component > uses tabActive, so if we want to change it, it could be addressed in a > follow up. I filled bug 1247403 > > @@ +52,5 @@ > > + > > + hasCookies: function() { > > + var data = this.state.data; > > + var request = data.request; > > + var response = data.response; > > I think this would be clearer with destructuring. Done > > > {request, response} = this.state.data; > > @@ +62,5 @@ > > + getTabPanels: function() { > > + var actions = this.props.actions; > > + var data = this.state.data; > > + var request = data.request; > > + var response = data.response; > > See comment above about destructuring. Done > > @@ +73,5 @@ > > + var panels = []; > > + > > + // Headers tab > > + panels.push( > > + TabPanel({className: "headers", key: "headers", > > You and James had a discussion above about the code style for the properties > param. I agree with you that the opening brace of the props object should be > on the first line. I've seen that used in the docs (e.g. at the bottom of > React's Displaying Data doc). > > So if you have a single property, it should go on that same line. However, > if you have multiple properties in the object, I think it should be split up > to 1 property per line, as shown in the Mozilla Code Style. e.g. > > > TabPanel({ > > className: "headers", > > key: "headers" > > ... Yes > > ::: devtools/client/webconsole/xhr/components/net-info-groups.js > @@ +15,5 @@ > > +var NetInfoGroupList = React.createClass({ > > + displayName: "NetInfoGroupList", > > + > > + render: function() { > > + var groups = this.props.groups; > > This is a case where we should use PropTypes.shape() to define what > properties group needs. Done > > @@ +46,5 @@ > > + displayName: "NetInfoGroup", > > + > > + getDefaultProps: function() { > > + return { > > + open: true, > > Does open get passed in from a parent anywhere? If not, this can probably > just be state. Yes, it's passed from e.g. the Post or Response tabs. > If it does need to be a prop, it should have PropTypes definition. Done > > ::: devtools/client/webconsole/xhr/components/params-tab.js > @@ +6,5 @@ > > +const React = require("devtools/client/shared/vendor/react"); > > +const { createFactories } = require("devtools/client/jsonview/components/reps/rep-utils"); > > +const { NetInfoParams } = createFactories(require("./net-info-groups")); > > + > > +// Constants > > This comment says "Canstants" in some cases, and "Shortcuts" in others. I'd > recommend either sticking with Shortcuts or removing the comment altogether. Ah, good already renamed all to Shortcuts couple hours ago :) > > @@ +18,5 @@ > > +var ParamsTab = React.createClass({ > > + displayName: "ParamsTab", > > + > > + render: function() { > > + var actions = this.props.actions; > > This isn't used, so it should be removed. If it was used, it should have a > PropTypes definition. Removed > > @@ +19,5 @@ > > + displayName: "ParamsTab", > > + > > + render: function() { > > + var actions = this.props.actions; > > + var data = this.props.data; > > This should have a PropType definition. Done > > Since the component only uses the request, maybe the prop that is passed in > should be request instead of data. > > If you do keep it as a nested object under data, then it should use the > .shape() method to describe data. Done > > ::: devtools/client/webconsole/xhr/components/post-tab.js > @@ +29,5 @@ > > + */ > > +var PostTab = React.createClass({ > > + displayName: "PostTab", > > + > > + isJson: function(file) { > > There are a lot of methods on the class here that feel like they could be > helpers outside of the class. > > @@ +91,5 @@ > > + var value = XhrUtils.getHeaderValue(file.request.headers, "content-type"); > > + return XhrUtils.isHTML(value); > > + }, > > + > > + renderXml: function(file) { > > Would these render helpers make sense as different components? Yes, there is more we can do as soon as we also have the XML preview (bug 1247392). Filled a follow up: Bug 1247418 > > @@ +210,5 @@ > > + // Render post body data. The right representation of the data > > + // is picked according to the content type. > > + let groups = []; > > + groups.push(this.renderUrlEncoded(file)); > > + //groups.push(this.renderMultiPart(file)); > > Commented out code. Should this be a TODO? Yes, Bug 1247423 reported > > ::: devtools/client/webconsole/xhr/components/response-tab.js > @@ +19,5 @@ > > + > > +// Shortcuts > > +const DOM = React.DOM; > > + > > +const trace = { > > See other comment about trace. > > @@ +46,5 @@ > > + > > + return Json.isJSON(content.mimeType, content.text); > > + }, > > + > > + parseJson: function(file) { > > Can any of these helpers be shared with post-tab.js? Yes, will be done as part of bug 1247418 > > @@ +199,5 @@ > > + * 2) Raw response data (always displayed if not discarded) > > + */ > > + render: function() { > > + var actions = this.props.actions; > > + var file = this.props.data; > > THe PropTypes definition for this should include all properties handled in > helper renderers. Done > > ::: devtools/client/webconsole/xhr/components/xml-view.js > @@ +12,5 @@ > > + > > +// Shortcuts > > +const DOM = React.DOM; > > + > > +var XmlView = React.createClass({ > > 1. Do we need to handle XML serialization manually? Could we possibly > delegate to a library that handles it for us? > > 2. If we do keep it, is there a plan to have XmlView do anything besides > going down one level in the prop nesting? If not, should the two be combined? Yes, I'd very much like to keep it. It's fast and lightweight (it also fits the other concepts related to object rendering) It'll be all handled by bug 1247392 so, we can continue discussing there. > > @@ +29,5 @@ > > + * This template is responsible for rendering entire XML element > > + * including optional attributes and children. > > + */ > > +var CompleteElement = > > +/** @lends CompleteElement */ > > See comment above about lends > > @@ +161,5 @@ > > +} > > + > > +// Registration > > + > > +const CompleteElementClass = React.createClass(CompleteElement); > > In most of the other files you use a different pattern. Instead of declaring > a variable with the specification, the spec object is defined when it's > passed in. > > ::: devtools/client/webconsole/xhr/console-overlay.js > @@ +93,5 @@ > > + this.panelFrame = panelFrame; > > + }, > > + > > + onBuild: function(eventId, panel) { > > + trace.log("ConsoleOverlay.onReady;"); > > This logs onReady, but the function is onBuild. Should this switch? Fixed > > @@ +113,5 @@ > > + }); > > + }, > > + > > + onReady: function(eventId, panel) { > > + trace.log("ConsoleOverlay.onBuild;"); > > See comment above. Fixed > > ::: devtools/client/webconsole/xhr/data-provider.js > @@ +19,5 @@ > > + * Communication with the chrome scope is based on message > > + * exchange. > > + */ > > +var DataProvider = > > +/** @lends DataProvider */ > > See comment above about @lends Fixed > > ::: devtools/client/webconsole/xhr/docs/xhr-spy.md > @@ +1,1 @@ > > +# XHR Inspect (aka XHR Spy) > > This document should probably go in the (new-ish) devtools/docs directory Done > > @@ +10,5 @@ > > +_XHR Spy feature is available in the Console panel (for web developers) > > +as well as in the Browser Console (for devtools and extension developers)._ > > + > > +The current implementation is based on React (no XUL) and some of the existing > > +components should also be used when porting the Network panel to HTML. > > The part talking about how components should also be used when porting > Network panel to HTML sounds like something that should be captured in an > issue rather than docs. > > @@ +79,5 @@ > > +The response is parsed using DOM parser and displayed as an XML markup. > > + > > +[1] List of JSON mime-types: `devtools/client/webconsole/xhr/utils/json` > > +[2] List of Image mime-types: `devtools/client/webconsole/xhr/utils/json` > > +[3] List of XML mime-types: `devtools/client/webconsole/xhr/utils/json` > > I didn't see the list of image or xml mime-types in this file. It's in utils/xhr, fixed > > @@ +159,5 @@ > > +`devtools/client/webconsole/xhr/xhr-spy` > > + > > +This module represents `XhrSpy` object. It's the internal representation > > +of HTTP request and it keeps its state. All HTTP details fetched dynamically > > +from the backed are stored in this object. > > s/backed/backend Fixed > > @@ +192,5 @@ > > +UI of XHR Spy is rendering data coming from the backend (the debugger server). > > +The fetching process is asynchronous and needs to cross content/chrome boundary > > +as well as use RDP protocol. > > + > > +This is how to flow looks like: > > s/how to/what the Fixed > > @@ +196,5 @@ > > +This is how to flow looks like: > > +1) The user expands XHR log and the Headers tab is selected by default. > > +2) The UI component calls `requestData` action. > > +3) The action checks a response-cache. If the response has been already > > +fetched it's returned; otherwise `DataProvider` is used to fetch the data. > > s/fetched it's/fetched, it's Fixed > > @@ +197,5 @@ > > +1) The user expands XHR log and the Headers tab is selected by default. > > +2) The UI component calls `requestData` action. > > +3) The action checks a response-cache. If the response has been already > > +fetched it's returned; otherwise `DataProvider` is used to fetch the data. > > +4) `DataProvider` checks if there isn't an existing pending request, > > Using "isn't" here and then saying "if yes" and "if no" is a little > confusing. Does "if yes" mean that there isn't an existing pending request? > Or that there is? The description is fixed. > > ::: devtools/client/webconsole/xhr/main.js > @@ +2,5 @@ > > + * License, v. 2.0. If a copy of the MPL was not distributed with this > > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > +"use strict"; > > + > > +var { classes: Cc, interfaces: Ci, utils: Cu } = Components; > > Do you need Cc and Ci here? No, fixed > > ::: devtools/client/webconsole/xhr/xhr-spy.js > @@ +3,5 @@ > > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > +"use strict"; > > + > > +// React > > +const ReactDOM = require("devtools/client/shared/vendor/react-dom"); > > s/ReactDOM/DOM/ This makes it consistent with all of the others. > > @@ +7,5 @@ > > +const ReactDOM = require("devtools/client/shared/vendor/react-dom"); > > + > > +// Reps > > +const { createFactories } = require("devtools/client/jsonview/components/reps/rep-utils"); > > +const { Url } = require("devtools/client/jsonview/components/reps/url"); > > Discussed with Brian. We would prefer not to couple the webconsole and > jsonview. Let's move all the reps to shared. Definitely, it's currently blocked by bug 1240494 as soon as this is fixed I'll move it in bug 1247434 > > @@ +16,5 @@ > > +const { DataProvider } = require("./data-provider"); > > + > > +// Helper tracer. Should be generic sharable by other modules (bug 1171927) > > +const trace = { > > + log: function() { > > There are calls to trace.log() throughout. Does that currently do anything? > We should land trace before this patch, or remove all of the trace calls. Removed > > @@ +235,5 @@ > > + > > + this.body.setState(newState); > > + }, > > + > > + // Actions > > Since we will be using Redux, we should limit the use of the word actions. > In Redux, an action is an object. That object has a type and a payload. Good point, agree Updated patch will follow. Honza

Comment on attachment 8717445 [details] plus.svg Can we use our devtools arrow expandos instead of introducing the new plus/minus signs, simply for consistency ?

(In reply to Tim Nguyen [:ntim] (mostly busy until Feb. 12th) from comment #80) > Comment on attachment 8717445 [details] > plus.svg > > Can we use our devtools arrow expandos instead of introducing the new > plus/minus signs, simply for consistency ? Seems like a good idea

(In reply to Tim Nguyen [:ntim] (mostly busy until Feb. 12th) from comment #80) > Comment on attachment 8717445 [details] > plus.svg > > Can we use our devtools arrow expandos instead of introducing the new > plus/minus signs, simply for consistency ? Tim's right—only noticed yesterday after my meeting with Honza, of course—sigh.

(In reply to Brian Grinstead [:bgrins] from comment #75) > Created attachment 8717727 [details] > message-overflow-cnn.png > > Here's a weird overflow on cnn.com. I think this isn't happening without the > patch applied The problem is in the way I am inserting the net-info-body when the user toggles a network log. Heres is simplfied example how the network log markup looks like: <div class="message netRequest"> <span class="timestamp" /> <span class="indent" /> <span class="icon" /> <span class="message-body-wrapper message-body"> <span class="method">POST </span> <a class="url" /> <a class="status" /> <div class="netInfoBody"> </span> </div> The <div> element with netInfoBody in appended dynamically when the user opens net request. It's appended into 'message-body' and without any styling it's displayed at the same line as method, url and status. This is why I am setting 'display: block' to 'message-body' element and 'float: left' to the 'netInfoBody' element. This effectively breaks 'flex: 1 1 100%;' set on the 'message-body-wrapper' in webconsole.xul causing the url (also using flex) *not* wrap if there is not enough horizontal space. Do you have any tips how to fix that? Also, if we want to invert the toggle icon we need to append an extra element for it (so, it can be inverted for the dark theme). Updated patch will follow so, you can try it out on your machine. Honza

(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #65) > (In reply to Jan Honza Odvarko [:Honza] from comment #63) > I still think we can get away with using the same font with different > weights—still denotes that they're different without having a weird visual > discrepancy. Could you try it out and mark me for review on a patch and > we'll make a definitive decision then? ok, two screenshots will follow (showing all monospace and labels with sans-serif). > I actually think even on 30" monitors we should probably wrap ~70 or so > characters. There's an idea (even with lots of whitespace) of an optimal > reading length. (http://baymard.com/blog/line-length-readability) I totaly agree with the articel. It's easier to read shorter lines (that's why we have columns in newspapers, right). But formatting of the response body isn't something we should do for good. The section where the response body is labeled 'Raw' so, the user can see data as they came over the wire, including invisible chars like spaes and end-of-lines. That's why I think they should be preserved. > It does sound like there are some edge cases here though, like you said. Can > you log a bug and we'll discuss copy-to-clipboard and large response bodies? Bug 1247963 Honza

Using only monospace font.

Using sans-serif for labels Helen, these are the screenshots I promised. I like the sans-serif version more since distinction is more visible than just using different font-weight. That's why, for example, dev bloggers often use different font for 'text' and 'code' examples. Anyway, I am not pushing on this and it's your call to decide, you are the Lord Of The Designs :-) Honza

(In reply to jodvarko from comment #85) > Created attachment 8718874 [details] > monospace.png > > Using only monospace font. You can use the `devtools-monospace` class to use the proper devtools-monospace font. (In reply to jodvarko from comment #86) > Created attachment 8718875 [details] > sans-serif.png > > Using sans-serif for labels > > Helen, these are the screenshots I promised. I like the sans-serif version > more since distinction is more visible than just using different > font-weight. That's why, for example, dev bloggers often use different font > for 'text' and 'code' examples. Anyway, I am not pushing on this and it's > your call to decide, you are the Lord Of The Designs :-) > > Honza If you're using sans-serif, I think it's better to use font: message-box, since it's more consistent.

(In reply to Brian Grinstead [:bgrins] from comment #73) > (In reply to Lin Clark [:linclark] from comment #72) > > ::: devtools/client/jsonview/components/reps/element.js > > @@ +18,5 @@ > > > + > > > +/** > > > + * @rep Generic XML/HTML element template. > > > + */ > > > +var Element = React.createClass( > > > > See xml-view.js. Do we want to maintain this? Or is this something a library > > could do for us? > > I believe the netmonitor is using CodeMirror for this (which does syntax > highlighting / code collapsing / etc). Honza, could the xml preview be > handled by this mechanism instead? And/or could we push special XML > previewing into another bug? Yes, another bug filled, bug 1247392 (In reply to Brian Grinstead [:bgrins] from comment #74) > Comment on attachment 8717438 [details] [diff] [review] > bug1211525.patch > > Review of attachment 8717438 [details] [diff] [review]: > ----------------------------------------------------------------- > > A couple general notes: > > 1) I think this should move into client/shared/components and out of > webconsole. This is because we will want to use the same component for the > netmonitor in the future, and the concept of 'display a network request' is > a useful shared component within the toolbox. I'd like to wait with this yet. Please don't get me wrong, I am all for sharing components and there is already good set of those I'd like to move to the shared dir [1] asap. In this particular bug, even if the 'net info body' component will *appear* in the Console and Net panels it doesn't mean that they are actually used by various parts in our code base (it'll be still only in the Net panel). I can imagine we could share e.g. the Spinner() or InfoGroup(), but e.g. PostTab() is specific for the Net panel. I'd like to rather wait for specific use case to appear and then reevaluate. [1] Tree, Toolbar, Tabs, Reps as the first round > 2) This isn't just for xhr requests, so I think the component should be > called something else. Maybe net-request? Yes agree and I like your suggestion, it's now NetRequest. > > ::: devtools/client/webconsole/xhr/console-overlay.js > @@ +29,5 @@ > > + "webconsole/xhr/components/net-info-groups", > > + "webconsole/xhr/components/response-tab" > > +]; > > + > > +// TODO: The localization should be updated as soon as the team agrees > > If you want to leave this as a TODO, then let's file another bug. But if we > are going to do a simple access on a properties file for now, I think it's > OK to ship without a TODO (since all of our code will have the same > migration path) Done > > @@ +98,5 @@ > > + > > + this.panel = panel; > > + > > + let win = this.panelFrame.contentWindow; > > + win.addEventListener("devtools/content/message", > > The distinction between chrome and content here is a little confusing given > our widely used 'content' term to mean the content process or page's global. > I understand this component is content privileged, but it's still running in > the parent process, right? This message passing has been the trickiest part > for me to figure out so far. Yes, the stuff is running in webconsole.xul (chrome scope) and so, in the parent process. The 'content' term in overloaded these days. I am talking about 'content scope' i.e. no chrome privileges. Better case where I am using this approach (implementing panel content as HTML with no chrome privileges) is the DOM panel. The entire code I've done for the DOM panel content is pure HTML running with no chrome. Please see my comment here: https://bugzilla.mozilla.org/show_bug.cgi?id=1201475#c7 I'd love to have an extra meeting for this, discuss how to properly implement panels in the new HTML/React world. I belive that what I used in the DOM panel could be the first step towards devtools.html > > ::: devtools/client/webconsole/xhr/xhr-spy.js > @@ +106,5 @@ > > + // about the XHR. > > + this.parentNode.classList.add("xhrSpy"); > > + > > + // Register a click listener. > > + this.addClickListener(); > > This sort of injecting behavior / DOM onto an existing message seems like > something that might break in the future (especially if the messages > themselves become Components). I don't have a better suggestion right now, > but we should think about how this would attach if the message was a > component and not a plain DOM element. Agree, we might revisit this wnen working on the inline object inspecing. (In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #82) > (In reply to Tim Nguyen [:ntim] (mostly busy until Feb. 12th) from comment > #80) > > Comment on attachment 8717445 [details] > > plus.svg > > > > Can we use our devtools arrow expandos instead of introducing the new > > plus/minus signs, simply for consistency ? > > Tim's right—only noticed yesterday after my meeting with Honza, of > course—sigh. Alright, updated patch will follow. Honza

Patch updated. Missing things: * Decide whether to use differnt font for labels * Fix oveflow of net logs * Image preview doesn't work sometimes * Update tests (refactor to adopt renamed files) * Use proper toggle-icon (with invert for dark-them) for net log (related to the overflow issue) There are several follow-ups reported and I believe we need meta-bug for them. Honza

(In reply to jodvarko from comment #89) > * Use proper toggle-icon (with invert for dark-them) for net log (related to > the overflow issue) Can you also use the normal toggle icon for the top level expansion next to the request name?

(In reply to jodvarko from comment #88) > > @@ +98,5 @@ > > > + > > > + this.panel = panel; > > > + > > > + let win = this.panelFrame.contentWindow; > > > + win.addEventListener("devtools/content/message", > > > > The distinction between chrome and content here is a little confusing given > > our widely used 'content' term to mean the content process or page's global. > > I understand this component is content privileged, but it's still running in > > the parent process, right? This message passing has been the trickiest part > > for me to figure out so far. > Yes, the stuff is running in webconsole.xul (chrome scope) and so, > in the parent process. The 'content' term in overloaded these days. > I am talking about 'content scope' i.e. no chrome privileges. > > Better case where I am using this approach (implementing panel content as > HTML > with no chrome privileges) is the DOM panel. The entire code I've done for > the DOM panel content is pure HTML running with no chrome. > Please see my comment here: > https://bugzilla.mozilla.org/show_bug.cgi?id=1201475#c7 > > I'd love to have an extra meeting for this, discuss how to properly > implement panels in the new HTML/React world. I belive that what I used in > the DOM panel could be the first step towards devtools.html So does it *need* to run in content scope to get the react stuff working? This part seems to add some complexity to the patch. Also when we turn the messages themselves into Components then injecting extra DOM in this manner won't work - it should be managed by the message component and not through an overlay system. This makes me think it'd be easier to construct and inject the net request component directly from logNetEvent for now if that's possible. What do you think?

(In reply to jodvarko from comment #89) > * Fix oveflow of net logs So did this fix the issue from Comment 83 then?

(In reply to Brian Grinstead [:bgrins] from comment #91) > (In reply to jodvarko from comment #88) > > > @@ +98,5 @@ > > > > + > > > > + this.panel = panel; > > > > + > > > > + let win = this.panelFrame.contentWindow; > > > > + win.addEventListener("devtools/content/message", > > > > > > The distinction between chrome and content here is a little confusing given > > > our widely used 'content' term to mean the content process or page's global. > > > I understand this component is content privileged, but it's still running in > > > the parent process, right? This message passing has been the trickiest part > > > for me to figure out so far. > > Yes, the stuff is running in webconsole.xul (chrome scope) and so, > > in the parent process. The 'content' term in overloaded these days. > > I am talking about 'content scope' i.e. no chrome privileges. > > > > Better case where I am using this approach (implementing panel content as > > HTML > > with no chrome privileges) is the DOM panel. The entire code I've done for > > the DOM panel content is pure HTML running with no chrome. > > Please see my comment here: > > https://bugzilla.mozilla.org/show_bug.cgi?id=1201475#c7 > > > > I'd love to have an extra meeting for this, discuss how to properly > > implement panels in the new HTML/React world. I belive that what I used in > > the DOM panel could be the first step towards devtools.html > > So does it *need* to run in content scope to get the react stuff working? > This part seems to add some complexity to the patch. Also when we turn the > messages themselves into Components then injecting extra DOM in this manner > won't work - it should be managed by the message component and not through > an overlay system. This makes me think it'd be easier to construct and > inject the net request component directly from logNetEvent for now if that's > possible. What do you think? True, could be easier. I am looking into it now. Honza

Summary of changes: 1) The overlay is removed 2) It's the logNetEvent function which sets the log's CSS and markup 3) All net events/udpates are directly passed to the NetRequest object, which adds/removes the details body. It'll be fairly straightforward to turn this into a component. 4) Broken overflow fixed. --- add #4) I had to change the markup a bit in order to fix the layout. Here is the old version: <div class="message netRequest"> <span class="timestamp" /> <span class="indent" /> <span class="icon" /> <span class="message-body-wrapper message-body"> <span class="method">POST </span> <a class="url" /> <a class="status" /> <div class="netInfoBody"> </span> </div> And new version (notice the new 'box'): <div class="message netRequest"> <span class="timestamp" /> <span class="indent" /> <span class="icon" /> <span class="message-body-wrapper message-body"> <div class="box"> <span class="method">POST </span> <a class="url" /> <a class="status" /> </div> <div class="netInfoBody"> <!-- HTTP details--> </div> </span> </div> Brian, it would be safer to use the original structure and change only CSS, but I could find any solution. Tips? What impact will this change have on webconsole tests? Honza

I assume we can just use the same icon and rotate it when needed with CSS transforms.

Helen, I don't understant, wny the new icon? Shouldn't I just use those we are using everywhere else? Honza

(In reply to Jan Honza Odvarko [:Honza] from comment #96) > Helen, I don't understant, wny the new icon? Shouldn't I just > use those we are using everywhere else? > > Honza That _is_ the new icon we're using everywhere else. You may have seen the icon in a few other places like the tree view: http://cl.ly/3O2Z3f1B3I2h It's the same icon, just small and with a "whitesmoke" fill (both of which can be affected with CSS).

(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #97) > That _is_ the new icon we're using everywhere else. You may have seen the > icon in a few other places like the tree view: http://cl.ly/3O2Z3f1B3I2h > > It's the same icon, just small and with a "whitesmoke" fill (both of which > can be affected with CSS). I see, ok. I filled a follow up: bug 1250407. This report is already getting huge and I'd like to avoid any additional requirements that would block landing. Honza

All review comments resolved and patch updated. Also tests are updated and passing. Since Brian is unavailable atm, are there any volunteers for the review? Honza

Patrick here is the patch with tests I mentioned on IRC The 'import-globals-from' doesn't seem to work /* import-globals-from ../../../test/head.js */ Honza

Yet, fixing one CSS path. Honza

Comment on attachment 8722962 [details] [diff] [review] bug1211525-tests.patch Review of attachment 8722962 [details] [diff] [review]: ----------------------------------------------------------------- I applied both patches locally and couldn't reproduce the file-not-found error we discussed about on IRC. I did find a few eslint errors though that can be fixed easily. I'll upload a patch in a minute.

Apply this on top of your test patch. It should fix the eslint errors you've been seeing.

Minor last minute change.

Patch updated. Lin, can you please review the patch? Several people already reviewed it (James, Helen, BrianG) and I resolved all comments. So, it should be close. Honza

Patrick, I am assigning also you for the eslint stuff. Honza

Comment on attachment 8723052 [details] [diff] [review] bug1211525.patch Review of attachment 8723052 [details] [diff] [review]: ----------------------------------------------------------------- The integration points with the webconsole look much simpler now. I'm still getting a horizontal overflow on cnn.com when expanding the http://www.cnn.com/. Also the twisty for net requests aren't lining up with the twisty for errors (i'll attach a screenshot showing both of these things). ::: devtools/client/jsonview/components/reps/url.js @@ +10,5 @@ > +var Url = {}; > + > +// Implementation > + > +Url.getFileName = function(url) { Looks like there are some duplicate files in this patch in between jsonview/components and shared/components ::: devtools/client/webconsole/net/net-request.js @@ +58,5 @@ > + // Add an event listener to toggle the expanded state when clicked. > + // The event bubbling is canceled if the user clicks on the log > + // itself (not on the expanded body), so opening of the default > + // modal dialog is avoided. > + this.parentNode.addEventListener("click", (event) => { Could we check specifically for a click on the toggle icon? Seems like that's what all these conditions are doing effectively. Also it seems that the NetRequest should inject the toggle icon since it wouldn't do anything without this component. ::: devtools/client/webconsole/webconsole.js @@ +1536,5 @@ > if (mixedRequest) { > severity = SEVERITY_WARNING; > } > > + let messageBox = this.document.createElementNS(XHTML_NS, "div"); This DOM reorganization seems a little risky, especially since I'm still seeing overflow issues (see review summary comment). @@ +1605,5 @@ > networkInfo.node = messageNode; > > this._updateNetMessage(actorId); > > + if (this.window.NetRequest) { Technically we should be even appending a toggle icon if window.NetRequest doesn't exist, see related comment in the click handler for NetRequest ::: devtools/docs/http-inspector.md @@ +111,5 @@ > +* Content scope: this is where the entire XHR feature lives (most of the code). > +* Chrome scope: this is a small piece of code representing the integration > + with the Console panel. > + > +The communication between chrome and content is done through asynchronous This documentation is now out of date, at least regarding the overlay and message passing. Is there code actually running in the content scope anymore? Can you please do a pass through this documentation to make sure it's up to date with the latest changes? @@ +155,5 @@ > +TODO: as soon as Redux is in place the Store object will contains a list > +of spies (currently represented by `spies` map). > + > +### XHR Spy > +`devtools/client/webconsole/xhr/xhr-spy` xhr spy name is out of date

Shows the two issues I mentioned in the review comments

Comment on attachment 8723053 [details] [diff] [review] bug1211525-tests.patch Review of attachment 8723053 [details] [diff] [review]: ----------------------------------------------------------------- r=pbro for the eslint changes

(In reply to (Unavailable until 3-7) Brian Grinstead [:bgrins] from comment #111) > Comment on attachment 8723052 [details] [diff] [review] > bug1211525.patch > > Review of attachment 8723052 [details] [diff] [review]: > ----------------------------------------------------------------- Thanks Brian! > The integration points with the webconsole look much simpler now. > > I'm still getting a horizontal overflow on cnn.com when expanding the > http://www.cnn.com/. Also the twisty for net requests aren't lining up with > the twisty for errors (i'll attach a screenshot showing both of these > things). > > ::: devtools/client/jsonview/components/reps/url.js > @@ +10,5 @@ > > +var Url = {}; > > + > > +// Implementation > > + > > +Url.getFileName = function(url) { > > Looks like there are some duplicate files in this patch in between > jsonview/components and shared/components Fixed > > ::: devtools/client/webconsole/net/net-request.js > @@ +58,5 @@ > > + // Add an event listener to toggle the expanded state when clicked. > > + // The event bubbling is canceled if the user clicks on the log > > + // itself (not on the expanded body), so opening of the default > > + // modal dialog is avoided. > > + this.parentNode.addEventListener("click", (event) => { > > Could we check specifically for a click on the toggle icon? Seems like > that's what all these conditions are doing effectively. Also it seems that > the NetRequest should inject the toggle icon since it wouldn't do anything > without this component. Helen suggested to handle click anywhere on the line #58 to open the details (but not break the link to the Net panel). I like it and this is also why there is couple more conditions. > > ::: devtools/client/webconsole/webconsole.js > @@ +1536,5 @@ > > if (mixedRequest) { > > severity = SEVERITY_WARNING; > > } > > > > + let messageBox = this.document.createElementNS(XHTML_NS, "div"); > > This DOM reorganization seems a little risky, especially since I'm still > seeing overflow issues (see review summary comment). Yeah, it's a bit risky agree, removed. I have used flex-wrap to solve that issue (the one I described in comment #94)! > > @@ +1605,5 @@ > > networkInfo.node = messageNode; > > > > this._updateNetMessage(actorId); > > > > + if (this.window.NetRequest) { > > Technically we should be even appending a toggle icon if window.NetRequest > doesn't exist, see related comment in the click handler for NetRequest Done > > ::: devtools/docs/http-inspector.md > @@ +111,5 @@ > > +* Content scope: this is where the entire XHR feature lives (most of the code). > > +* Chrome scope: this is a small piece of code representing the integration > > + with the Console panel. > > + > > +The communication between chrome and content is done through asynchronous > > This documentation is now out of date, at least regarding the overlay and > message passing. Is there code actually running in the content scope > anymore? Can you please do a pass through this documentation to make sure > it's up to date with the latest changes? Updated > > @@ +155,5 @@ > > +TODO: as soon as Redux is in place the Store object will contains a list > > +of spies (currently represented by `spies` map). > > + > > +### XHR Spy > > +`devtools/client/webconsole/xhr/xhr-spy` > > xhr spy name is out of date Fixed Honza

Yet fix for webconsole tests. Eventually I simplified the click handler so, clicking on the toggle button and the method label (displayed just next to it) opens/collapses the body. The rest of the line is mostly occupied by the URL so, it's not that big UX change I guess. New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=de2a46488aeb Honza

Fixing eslint warnings (using es6 class instead of createClass) Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e3f43437078 Honza

Comment on attachment 8725165 [details] [diff] [review] bug1211525.patch Review of attachment 8725165 [details] [diff] [review]: ----------------------------------------------------------------- The patch is starting to get hard to review because it's so large. Some of the newest additions aren't directly related to the issue... for example, adding propTypes to existing components. Before the next round of review, would it be possible break out any things that could be reviewed independently of each other? I don't have a big enough chunk of time to review the whole patch at the moment. I'll be coming back to it tomorrow, but here's what I saw so far. Overall, I think it would be good to document all of the propTypes with a simple one line comment above. You can see an example of this in the memory tool's model file: https://dxr.mozilla.org/mozilla-central/source/devtools/client/memory/models.js#57 ::: devtools/client/jsonview/components/reps/toolbar.js @@ +37,2 @@ > render: function() { > var props = Object.assign({className: "btn"}, this.props); Looking at all of the places where ToolbarButton is used, it seems like they all add their own classnames, which include btn. Do we get anything from having a ToolbarButton component? @@ +42,5 @@ > }, > }); > > // Exports from this module > exports.Toolbar = Toolbar; It's a little confusing that this isn't indented. Is this because of AMD? @@ +44,5 @@ > > // Exports from this module > exports.Toolbar = Toolbar; > exports.ToolbarButton = ToolbarButton; > + Getting a "Block must not be padded by blank lines" error here. ::: devtools/client/jsonview/components/reps/tree-view.js @@ +26,3 @@ > > + propTypes: { > + mode: PropTypes.string.isRequired, Could you add a comment describing mode here? It's not clear how mode is used. Also, it should probably be treated as an enum, which can be indicated with React.PropTypes.oneOf. @@ +68,5 @@ > + // Data > + > + componentDidMount: function() { > + let members = initMembers(this.props.data, 0); > + this.setState({data: members, searchFilter: this.props.searchFilter}); Calling this.setState here feels a little weird to me. I realize that you're doing this here so that you catch the initial render (which componentWillReceiveProps does not). Is there a reason why initMembers can't just be called before you iterate over the array in render? ::: devtools/client/locales/en-US/netmonitor.properties @@ +296,5 @@ > +# The {{link}} will be replace at run-time by an active link. > +# String with ID 'netRequest.sizeLimitMessageLink' will be used as text > +# for this link. > +netRequest.sizeLimitMessage=Size limit has been reached. Click {{link}} to load more. > +netRequest.sizeLimitMessageLink=here I haven't seen multiple properties grouped like this before. What I've seen has usually been a comment above each property. Have you seen properties grouped like this in another file? ::: devtools/client/shared/components/reps/array.js @@ +26,5 @@ > let ArrayRep = React.createClass({ > displayName: "ArrayRep", > > + propTypes: { > + object: PropTypes.array.isRequired why is it called object but expects an array? I see that this is the same for caption.js too... maybe instead of object it should be called item or something like that.

(In reply to Lin Clark [:linclark] from comment #117) > Comment on attachment 8725165 [details] [diff] [review] > bug1211525.patch > > Review of attachment 8725165 [details] [diff] [review]: > ----------------------------------------------------------------- > > The patch is starting to get hard to review because it's so large. Some of > the newest additions aren't directly related to the issue...for example, > adding propTypes to existing components. Before the next round of review, > would it be possible break out any things that could be reviewed > independently of each other? Yeah, the patch went already through set of reviews (jlongster, helenvholmes, bgrins) and bunch of comments has been made and resolved. That's why I am filling follow ups for almost all new suggestions now (bug 906239, bug 1238953, bug 1239665, bug 1247065, bug 1247392, bug 1247403, bug 1247418, bug 1247423, bug 1253237). I didn't for propTypes, my mistake. I don't know how to split the patch, so it's easier to review. At this point, I'll will only fix real bugs in the patch. > I don't have a big enough chunk of time to review the whole patch at the > moment. I'll be coming back to it tomorrow, but here's what I saw so far. I know and thanks for the review! Note that Brian already did quite in-depth review so, I guess we should be mostly fine here. > Overall, I think it would be good to document all of the propTypes with a > simple one line comment above. You can see an example of this in the memory > tool's model file: > https://dxr.mozilla.org/mozilla-central/source/devtools/client/memory/models. > js#57 > > ::: devtools/client/jsonview/components/reps/toolbar.js > @@ +37,2 @@ > > render: function() { > > var props = Object.assign({className: "btn"}, this.props); Agreed, filled Bug 1253237 > Looking at all of the places where ToolbarButton is used, it seems like they > all add their own classnames, which include btn. Do we get anything from > having a ToolbarButton component? Note that the ToolbarButton has been introduced in JSONViewer so, not part of this patch. In any case I think we should have Toolbar and ToolbarButton components in our shared directory and they should be reused for Toolbox panels (we might want to have bugs for it if that make sense to you). > > @@ +42,5 @@ > > }, > > }); > > > > // Exports from this module > > exports.Toolbar = Toolbar; > > It's a little confusing that this isn't indented. Is this because of AMD? Yes, the code is wrapped in define() function and should be indented just like I did for e.g. shared/components/reps/array component. Filled bug 1253243 > @@ +44,5 @@ > > > > // Exports from this module > > exports.Toolbar = Toolbar; > > exports.ToolbarButton = ToolbarButton; > > + > > Getting a "Block must not be padded by blank lines" error here. Covered by bug 1253243 mentioned above. > > ::: devtools/client/jsonview/components/reps/tree-view.js > @@ +26,3 @@ > > > > + propTypes: { > > + mode: PropTypes.string.isRequired, > > Could you add a comment describing mode here? It's not clear how mode is > used. Also, it should probably be treated as an enum, which can be indicated > with React.PropTypes.oneOf. Good point. The JSON Viewer is going to use the devtools/client/shared/tree (or any alternatives we end up having) and so, the comment should be done there. So, this is blocked by bug 1247065 > > @@ +68,5 @@ > > + // Data > > + > > + componentDidMount: function() { > > + let members = initMembers(this.props.data, 0); > > + this.setState({data: members, searchFilter: this.props.searchFilter}); > > Calling this.setState here feels a little weird to me. I realize that you're > doing this here so that you catch the initial render (which > componentWillReceiveProps does not). Is there a reason why initMembers can't > just be called before you iterate over the array in render? Just like the commented above (the JSON View will use shared tree and this will be removed) > > ::: devtools/client/locales/en-US/netmonitor.properties > @@ +296,5 @@ > > +# The {{link}} will be replace at run-time by an active link. > > +# String with ID 'netRequest.sizeLimitMessageLink' will be used as text > > +# for this link. > > +netRequest.sizeLimitMessage=Size limit has been reached. Click {{link}} to load more. > > +netRequest.sizeLimitMessageLink=here > > I haven't seen multiple properties grouped like this before. What I've seen > has usually been a comment above each property. Have you seen properties > grouped like this in another file? I took a quick look at our files and I see the same in app-manager.properties and appcacheutils.properties. I am sure there are other places in our code. I don't see any recommendation on https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices though. But, I know this works for localizes (from my many years of experience when localizing Firebug). > ::: devtools/client/shared/components/reps/array.js > @@ +26,5 @@ > > let ArrayRep = React.createClass({ > > displayName: "ArrayRep", > > > > + propTypes: { > > + object: PropTypes.array.isRequired > > why is it called object but expects an array? I see that this is the same > for caption.js too... maybe instead of object it should be called item or > something like that. This is part of the 'reps' concept. There are many templates representing JS/DOM types and each renders an 'object' passed it. The right template is picked dynamically and the object being rendered is passed into it always as the same property. The code that passes the |object| in doesn't even know what is the actual template that renders it (you might see the polymorphic behavior here). Honza

Comment on attachment 8725165 [details] [diff] [review] bug1211525.patch Review of attachment 8725165 [details] [diff] [review]: ----------------------------------------------------------------- I noticed another thing (which should hopefully be resolved when switching to the new tree, or the switch to Redux)... expansion settings aren't maintained when you switch tabs. I'll post a video demonstrating this. One code standard problem: Components don't fully define their propTypes. There are multiple places where children properties that aren't defined are directly accessed in a component. I understand that this patch was mostly created before we had that rule. Plus, we want to land this sooner rather than later. Because of those two factors, I suggest that we add this cleanup to one of the followups. I'd like :bgrins to review the test patch before this is committed. Leaving this as feedback+, he can r+ it when he has had a chance to review the tests. ::: devtools/client/webconsole/net/.eslintrc @@ +13,5 @@ > + "setTimeout": true > + }, > + "rules": { > + "no-unused-vars": [2, {"args": "none"}], > + "max-len": [1, 80, 2, {"ignoreUrls": true, "ignorePattern": "\\s*require\\s*\\(|^\\s*loader\\.lazy"}], Why is max-len switched to a warning here? ::: devtools/docs/http-inspector.md @@ +1,1 @@ > +# HTTP Inspectot (aka XHR Spy) s/Inspectot/Inspector

Comment on attachment 8723053 [details] [diff] [review] bug1211525-tests.patch Review of attachment 8723053 [details] [diff] [review]: ----------------------------------------------------------------- As mentioned in comment above, I'd like :bgrins to review this before we commit. One nit... the test filenames still use the old name (xhrspy). ::: devtools/client/shared/frame-script-utils.js @@ +49,5 @@ > */ > function promiseXHR(data) { > let xhr = new content.XMLHttpRequest(); > > + content.console.log("promise XHR", data); is having this console.log statement here intentional? ::: devtools/client/webconsole/net/test/mochitest/.eslintrc @@ +1,2 @@ > +{ > + // Extend from the shared list of defined globals for mochitests. do we need this file if there's nothing in it?

(In reply to Lin Clark [:linclark] from comment #119) > Comment on attachment 8725165 [details] [diff] [review] > bug1211525.patch > > Review of attachment 8725165 [details] [diff] [review]: > ----------------------------------------------------------------- > > I noticed another thing (which should hopefully be resolved when switching > to the new tree, or the switch to Redux)... expansion settings aren't > maintained when you switch tabs. I'll post a video demonstrating this. Ah yes, this is because trees are generated every time the tab is rendered. I think that the problem is not with the tree, but with how to tree is used. I Reported bug 1253640 for this and it'll be good topic for discussion. > > One code standard problem: Components don't fully define their propTypes. > There are multiple places where children properties that aren't defined are > directly accessed in a component. I understand that this patch was mostly > created before we had that rule. Plus, we want to land this sooner rather > than later. Because of those two factors, I suggest that we add this cleanup > to one of the followups. Yes, I made a comment here: https://bugzilla.mozilla.org/show_bug.cgi?id=1253237#c1 > > I'd like :bgrins to review the test patch before this is committed. Leaving > this as feedback+, he can r+ it when he has had a chance to review the tests. OK I'll assign Brian for the review as soon as he's back (it's not possible to do it now) > > ::: devtools/client/webconsole/net/.eslintrc > @@ +13,5 @@ > > + "setTimeout": true > > + }, > > + "rules": { > > + "no-unused-vars": [2, {"args": "none"}], > > + "max-len": [1, 80, 2, {"ignoreUrls": true, "ignorePattern": "\\s*require\\s*\\(|^\\s*loader\\.lazy"}], > > Why is max-len switched to a warning here? Removed. This was left over (the rule has been fixed as part of bug 1247186 > > ::: devtools/docs/http-inspector.md > @@ +1,1 @@ > > +# HTTP Inspectot (aka XHR Spy) > > s/Inspectot/Inspector Fixed Thanks Lin! Updated patch attached. Honza

Patch updated (max-len rule removed and doc typo fixed) Honza

Patch updated: - Using createClass (not es6 classes) - Export components directly - 1 component per file (+ css related split) Honza

Tests updated (making sure xhrspy keyword is not used) Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e14b8efc395 Honza

Comment on attachment 8728431 [details] [diff] [review] bug1211525.patch Review of attachment 8728431 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/webconsole/net/main.js @@ +11,5 @@ > +// Initialize module loader and load all modules of the new inline > +// preview feature. The entire code-base doesn't need any extra > +// privileges and runs entirely in content scope. > +const rootUrl = "resource://devtools/client/webconsole/net/"; > +const require = BrowserLoader(rootUrl, this).require; This doesn't work for me locally - I get this error: "DevToolsUtils.assert threw an exception: Error: Assertion failure: Cannot use both `baseURI` and `useOnlyShared`. Stack: reallyAssert@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/DevToolsUtils.js:297:17 BrowserLoaderBuilder@resource://devtools/client/shared/browser-loader.js:76:3 BrowserLoader@resource://devtools/client/shared/browser-loader.js:56:32 @resource://devtools/client/webconsole/net/main.js:15:17 Line: 297, column: 17" Apparently there was a BrowserLoader API change so it takes in only a single param - `BrowserLoader(rootUrl, this)` -> `BrowserLoader({ baseURI: rootUrl, window: this })`

(In reply to Brian Grinstead [:bgrins] from comment #126) > Apparently there was a BrowserLoader API change so it takes in only a single > param - `BrowserLoader(rootUrl, this)` -> `BrowserLoader({ baseURI: rootUrl, > window: this })` Fixed Honza

Comment on attachment 8728431 [details] [diff] [review] bug1211525.patch Review of attachment 8728431 [details] [diff] [review]: ----------------------------------------------------------------- Just submitting a couple of comments I had stored in a draft on this obsolete patch ::: devtools/client/locales/en-US/netmonitor.properties @@ +300,5 @@ > +netRequest.sizeLimitMessageLink=here > + > +# LOCALIZATION NOTE (netRequest.responseBodyDiscarded): A label used > +# in Response tab if the response body is not available. > +netRequest.responseBodyDiscarded=Response body is not not available. Something like "Response body was not stored" will be more clear that it was something on our end and not a network error. @@ +304,5 @@ > +netRequest.responseBodyDiscarded=Response body is not not available. > + > +# LOCALIZATION NOTE (netRequest.requestBodyDiscarded): A label used > +# in Post tab if the post body is not available. > +netRequest.requestBodyDiscarded=Request post body is not available. Something like "Request post body was not stored" will be more clear that it was something on our end and not a network error.

Comment on attachment 8728988 [details] [diff] [review] bug1211525.patch Review of attachment 8728988 [details] [diff] [review]: ----------------------------------------------------------------- Still have to go through more of the NetRequest components, but going to leave the notes here ::: devtools/client/shared/components/reps/string.js @@ +106,3 @@ > }; > + > + exports.isCropped = isCropped; If all these functions are being exported they are presumably going to be used elsewhere and should have unit tests, no? ::: devtools/client/shared/components/reps/url.js @@ +71,5 @@ > + let lastDot = url.lastIndexOf("."); > + return url.substr(lastDot + 1); > + }; > + > + Url.parseURLParams = function(url) { A few things about this file: 1) Many of the methods exported aren't used AFAICT. 2) There is a lot of code here handling corner cases. Was this ported from Firebug? What about using the URL constructor along with things like new URL(url).searchParams instead of the string manipulation? 3) I don't see any tests for this in the next patch. This needs at least a basic test in place so that it can be expanded. ::: devtools/client/themes/webconsole.css @@ +261,5 @@ > border-radius: 3px; > font-weight: bold; > font-size: 10px; > padding: 2px; > + margin: 0 2px 0 3px; Two issues: 1) This isn't friendly for RTL 2) -moz-margin-end: 1ex; is set further in this block so the 2px right margin isn't doing anything in an LTR build. Equivalent would be to remove this line and replace -moz-margin-end: 1ex with: margin-inline-start: 3px; margin-inline-end: 1ex; Although looking at the UI the start margin at 0 seems OK - what's the purpose of this change? ::: devtools/client/webconsole/net/components/spinner.css @@ +5,5 @@ > +/******************************************************************************/ > +/* Spinner */ > + > +.netInfoBody .spinner { > + background-image: url("../images/loading.png"); We have an existing devtools-throbber class. Would we be able to use that instead of adding these new files so it'll be consistent across the tools? Then, if Firebug needs a diff. throbber UI we can override it at that level. ::: devtools/client/webconsole/net/net-request.js @@ +92,5 @@ > + }, > + > + onToggleBody: function(event) { > + let target = event.currentTarget; > + let logRow = getAncestorByClass(target, "netRequest"); Instead of the helper function, what about target.closest(".netRequest") ? ::: devtools/client/webconsole/net/utils/json.js @@ +30,5 @@ > + > + let regex, matches; > + > + let first = firstNonWs(jsonString); > + if (first !== "[" && first !== "{") { We have some kind of JSON handling already for places like the net monitor. I was going to suggest re-using what is there but it seems that's inline in the view and isn't handling as many cases as this is: https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/netmonitor-view.js#2987. So if we want to go with this implementation I think we should re-use this one inside of the netmonitor view instead. In which case this file should move to devtools/shared/json.js or similar. I'd be OK with filing a follow-up to do that. Regardless, this needs at least a stub unit test that has at least one assertion for each function so we can expand it as we go. ::: devtools/client/webconsole/net/utils/net.js @@ +57,5 @@ > +}; > + > +var NetUtils = {}; > + > +NetUtils.isImage = function(contentType) { Can you add a unit test for this file that has at least one test for each method in the API? ::: devtools/client/webconsole/webconsole.js @@ +1811,5 @@ > node: networkInfo.node, > response: packet, > }])); > + > + if (this.window.NetRequest) { I'm thinking this call should happen before "new-messages" is emitted to more closely match the event flow for tests as when a new message is added

(In reply to Brian Grinstead [:bgrins] from comment #128) > Comment on attachment 8728431 [details] [diff] [review] > bug1211525.patch > > Review of attachment 8728431 [details] [diff] [review]: > ----------------------------------------------------------------- > > Just submitting a couple of comments I had stored in a draft on this > obsolete patch > > ::: devtools/client/locales/en-US/netmonitor.properties Both strings updated. (In reply to Brian Grinstead [:bgrins] from comment #129) > Comment on attachment 8728988 [details] [diff] [review] > bug1211525.patch > > Review of attachment 8728988 [details] [diff] [review]: > ----------------------------------------------------------------- > > Still have to go through more of the NetRequest components, but going to > leave the notes here > > ::: devtools/client/shared/components/reps/string.js > @@ +106,3 @@ > > }; > > + > > + exports.isCropped = isCropped; > > If all these functions are being exported they are presumably going to be > used elsewhere and should have unit tests, no? Unit test for isCropped and cropString added into the *tests.patch (will be updated) (also the module is not exporting escapeNewLines and cropMultipleLines since it isn't necessary) > ::: devtools/client/shared/components/reps/url.js > @@ +71,5 @@ > > + let lastDot = url.lastIndexOf("."); > > + return url.substr(lastDot + 1); > > + }; > > + > > + Url.parseURLParams = function(url) { > > A few things about this file: > 1) Many of the methods exported aren't used AFAICT. > 2) There is a lot of code here handling corner cases. Was this ported from > Firebug? What about using the URL constructor along with things like new > URL(url).searchParams instead of the string manipulation? > 3) I don't see any tests for this in the next patch. This needs at least a > basic test in place so that it can be expanded. I've reduced the API to parseURLParams and parseURLEncodedText Unit test are included in the *tests patch) > > ::: devtools/client/themes/webconsole.css > @@ +261,5 @@ > > border-radius: 3px; > > font-weight: bold; > > font-size: 10px; > > padding: 2px; > > + margin: 0 2px 0 3px; > > Two issues: > > 1) This isn't friendly for RTL > 2) -moz-margin-end: 1ex; is set further in this block so the 2px right > margin isn't doing anything in an LTR build. > > Equivalent would be to remove this line and replace -moz-margin-end: 1ex > with: > > margin-inline-start: 3px; > margin-inline-end: 1ex; Done > > Although looking at the UI the start margin at 0 seems OK - what's the > purpose of this change? Helen requested a bit more padding around the XHR tag(comment #58 and my reply #63) > > ::: devtools/client/webconsole/net/components/spinner.css > @@ +5,5 @@ > > +/******************************************************************************/ > > +/* Spinner */ > > + > > +.netInfoBody .spinner { > > + background-image: url("../images/loading.png"); > > We have an existing devtools-throbber class. Would we be able to use that > instead of adding these new files so it'll be consistent across the tools? > Then, if Firebug needs a diff. throbber UI we can override it at that level. Done > > ::: devtools/client/webconsole/net/net-request.js > @@ +92,5 @@ > > + }, > > + > > + onToggleBody: function(event) { > > + let target = event.currentTarget; > > + let logRow = getAncestorByClass(target, "netRequest"); > > Instead of the helper function, what about target.closest(".netRequest") ? Nice catch, done! > > ::: devtools/client/webconsole/net/utils/json.js > @@ +30,5 @@ > > + > > + let regex, matches; > > + > > + let first = firstNonWs(jsonString); > > + if (first !== "[" && first !== "{") { > > We have some kind of JSON handling already for places like the net monitor. > I was going to suggest re-using what is there but it seems that's inline in > the view and isn't handling as many cases as this is: > https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/ > netmonitor-view.js#2987. > > So if we want to go with this implementation I think we should re-use this > one inside of the netmonitor view instead. In which case this file should > move to devtools/shared/json.js or similar. I'd be OK with filing a > follow-up to do that. Yes, that was my plan, filled bug 1255782 > > Regardless, this needs at least a stub unit test that has at least one > assertion for each function so we can expand it as we go. Done, a test is in the *tests.patch > > ::: devtools/client/webconsole/net/utils/net.js > @@ +57,5 @@ > > +}; > > + > > +var NetUtils = {}; > > + > > +NetUtils.isImage = function(contentType) { > > Can you add a unit test for this file that has at least one test for each > method in the API? Done, a test is in the *tests.patch > > ::: devtools/client/webconsole/webconsole.js > @@ +1811,5 @@ > > node: networkInfo.node, > > response: packet, > > }])); > > + > > + if (this.window.NetRequest) { > > I'm thinking this call should happen before "new-messages" is emitted to > more closely match the event flow for tests as when a new message is added Done Further, I spitted the patch into more, hope it'll be easier for you. * bug1211525.patch - related to the HTTPi * bug1211525-proptypes.patch - adding proptypes to reps. This patch also includes two tiny changes, json-panel.js is changing tree 'mode' from tiny -> short and reps/object avoids recursion when rendering object preview. * bug1211525-eslint.patch - making eslint happy * bug1211525-tests.patch - tests Thanks for the review Brian! Honza

Patrick, these are just eslint changes. When applying the patch you need to do it in this order: 1) bug1211525.patch 2) bug1211525-proptypes.patch 3) bug1211525-eslint.patch Honza

Yet, fix the commit message

Comment on attachment 8728432 [details] [diff] [review] bug1211525-tests.patch Review of attachment 8728432 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/shared/frame-script-utils.js @@ +49,5 @@ > */ > function promiseXHR(data) { > let xhr = new content.XMLHttpRequest(); > > + content.console.log("promise XHR", data); The question from Comment 121 wasn't addressed: "is having this console.log statement here intentional?"

(In reply to Brian Grinstead [:bgrins] from comment #136) > The question from Comment 121 wasn't addressed: "is having this console.log > statement here intentional?" Ah, I missed that comment, fixed (log removed) Honza

Comment on attachment 8729556 [details] [diff] [review] bug1211525.patch Review of attachment 8729556 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/shared/components/reps/url.js @@ +7,5 @@ > +"use strict"; > + > +// Make this available to both AMD and CJS environments > +define(function(require, exports, module) { > + function parseURLParams(url) { What does this give us over using the built in URL object and it's searchParams property? I'm still fine with exporting helper functions for it

Comment on attachment 8729556 [details] [diff] [review] bug1211525.patch Review of attachment 8729556 [details] [diff] [review]: ----------------------------------------------------------------- Another round of notes ::: devtools/client/webconsole/net/components/net-info-body.css @@ +106,5 @@ > +.theme-dark .netInfoBody > .tabs .tabs-menu-item a { > + color: var(--theme-body-color); > +} > + > +.theme-light .netInfoBody > .tabs .tab-panel, We've been working to remove hardcoded instances of .theme-light and .theme-dark throughout our codebase. It makes the CSS inflexible and harder to adapt it for theming when we rely on those class names. Also, I don't get this pattern where .theme-light and .theme-dark are repeated with the same selector. I take it this is a way to set the Firebug theming as default and then override light and dark? Ideally any variations between themes will be handled with variables so changes can be contained to where the variables are defined. So in this particular case, there's another `.netInfoBody > .tabs .tab-panel` above that does `border: 1px solid var(--net-border)`. So can we set that net-border variable to be transparent in light and dark theme? If not, then we should answer either: 1) Would the UI be OK to use the default net border color? 2) Should a new variable be made for this case? ::: devtools/client/webconsole/net/components/net-info-group.css @@ +6,5 @@ > +/* Net Info Group */ > + > +.netInfoBody .netInfoGroup { > + line-height: 13px; > + color: #565656; Please extract hardcoded colors into variables @@ +43,5 @@ > + background-image: url("chrome://devtools/skin/images/firebug/twisty-open-firebug.svg"); > + cursor: pointer; > +} > + > +/* Group content is expandable/collapsible by clicking on the title*/ Nit: whitespace before */ ::: devtools/client/webconsole/net/components/post-tab.js @@ +248,5 @@ > +/** > + * Make sure the string doesn't contains any dangerous characters > + * (e.g. multipart post body can contain uploaded files) > + */ > +function safeString(text) { Is this actually meant as a security feature? If not, it should be renamed. If so, I don't get it - I thought React was already sanitizing anything we output in the template. ::: devtools/client/webconsole/net/components/response-tab.js @@ +56,5 @@ > + return null; > + } > + > + let jsonString = new String(content.text); > + return Json.parseJSONString(jsonString, "http://" + file.request.url); There is an extra parameter here that is unused by parseJSONString ::: devtools/client/webconsole/net/utils/json.js @@ +77,5 @@ > + > + try { > + return JSON.parse(jsonString); > + } catch (err) { > + console.error(err); Do we really want to console.error here? This is user-generated data that could very well not be JSON. Debatable but I don't think it's worth notifying in the console about this @@ +94,5 @@ > + > + try { > + return JSON.parse(jsonString); > + } catch (err) { > + console.error(err); Ditto

Comment on attachment 8729563 [details] [diff] [review] bug1211525-proptypes.patch Review of attachment 8729563 [details] [diff] [review]: ----------------------------------------------------------------- r=me. Rubber stamp on tree-view.js changes since I've been informed it's reorganizing / whitespace changes and it's hard to follow in splinter

Comment on attachment 8729593 [details] [diff] [review] bug1211525-tests.patch Review of attachment 8729593 [details] [diff] [review]: ----------------------------------------------------------------- Just some notes, haven't reviewed everything ::: devtools/client/shared/components/reps/test/unit/test_string.js @@ +22,5 @@ > + equal(isCropped(shortString, limit), false); > + equal(isCropped(longString, limit), true); > + > + // cropString > + equal(cropString(longString, limit), "This...ing"); Something I noticed. It's using "..." to crop, but really it should a value from a properties file or `Services.prefs.getComplexValue("intl.ellipsis", Ci.nsIPrefLocalizedString).data`. I know the reps code also has to run in content priv - but do we have some way to load localization files? If not, it could use the '…' character at least instead of ... This could definitely be done in a follow up, or if you want to update right now while the test is being added that works too ::: devtools/client/webconsole/net/test/mochitest/browser_net_basic.js @@ +22,5 @@ > + method: "GET", > + url: JSON_XHR_URL > + }); > + > + ok(netInfoBody, "The network details must be available"); Are there other properties on netInfoBody that would be useful to assert beyond truthiness? The call to executeAndInspectXhr is doing an identical assertion before resolving ::: devtools/client/webconsole/net/test/unit/test_json-utils.js @@ +27,5 @@ > + * Testing API provided by webconsole/net/utils/json.js > + */ > +function run_test() { > + // parseJSONString > + ok(parseJSONString(simpleJson) != null); Could check is(parseJSONString(simpleJson).name, "John") @@ +28,5 @@ > + */ > +function run_test() { > + // parseJSONString > + ok(parseJSONString(simpleJson) != null); > + ok(parseJSONString(jsonInFunc) != null); Could check is(parseJSONString(jsonInFunc).name, "John") @@ +31,5 @@ > + ok(parseJSONString(simpleJson) != null); > + ok(parseJSONString(jsonInFunc) != null); > + > + // isJSON > + equal(isJSON(textMimeType, json1), true); So should this be true or false with jsonMimeType? What about with unknownMimeType?

(In reply to Brian Grinstead [:bgrins] from comment #138) > Comment on attachment 8729556 [details] [diff] [review] > bug1211525.patch > > Review of attachment 8729556 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: devtools/client/shared/components/reps/url.js > @@ +7,5 @@ > > +"use strict"; > > + > > +// Make this available to both AMD and CJS environments > > +define(function(require, exports, module) { > > + function parseURLParams(url) { > > What does this give us over using the built in URL object and it's > searchParams property? I'm still fine with exporting helper functions for it Agreed, using URL is better, Fixed. (In reply to Brian Grinstead [:bgrins] from comment #139) > Comment on attachment 8729556 [details] [diff] [review] > bug1211525.patch > > Review of attachment 8729556 [details] [diff] [review]: > ----------------------------------------------------------------- > > Another round of notes > > ::: devtools/client/webconsole/net/components/net-info-body.css > @@ +106,5 @@ > > +.theme-dark .netInfoBody > .tabs .tabs-menu-item a { > > + color: var(--theme-body-color); > > +} > > + > > +.theme-light .netInfoBody > .tabs .tab-panel, > > We've been working to remove hardcoded instances of .theme-light and > .theme-dark throughout our codebase. It makes the CSS inflexible and harder > to adapt it for theming when we rely on those class names. Also, I don't > get this pattern where .theme-light and .theme-dark are repeated with the > same selector. I take it this is a way to set the Firebug theming as > default and then override light and dark? > > Ideally any variations between themes will be handled with variables so > changes can be contained to where the variables are defined. So in this > particular case, there's another `.netInfoBody > .tabs .tab-panel` above > that does `border: 1px solid var(--net-border)`. So can we set that > net-border variable to be transparent in light and dark theme? If not, then > we should answer either: > > 1) Would the UI be OK to use the default net border color? > 2) Should a new variable be made for this case? TODO > > ::: devtools/client/webconsole/net/components/net-info-group.css > @@ +6,5 @@ > > +/* Net Info Group */ > > + > > +.netInfoBody .netInfoGroup { > > + line-height: 13px; > > + color: #565656; > > Please extract hardcoded colors into variables Done > > @@ +43,5 @@ > > + background-image: url("chrome://devtools/skin/images/firebug/twisty-open-firebug.svg"); > > + cursor: pointer; > > +} > > + > > +/* Group content is expandable/collapsible by clicking on the title*/ > > Nit: whitespace before */ Fixed > > ::: devtools/client/webconsole/net/components/post-tab.js > @@ +248,5 @@ > > +/** > > + * Make sure the string doesn't contains any dangerous characters > > + * (e.g. multipart post body can contain uploaded files) > > + */ > > +function safeString(text) { > > Is this actually meant as a security feature? If not, it should be renamed. > If so, I don't get it - I thought React was already sanitizing anything we > output in the template. The problem is when uploading binary files. There is the following error without the conversion: JavaScript Error: "not well-formed" (reported by React) I am not exactly sure why React fails. STR: upload binary data, expand related XHR log and select the Post tab. See also comment #55 > > ::: devtools/client/webconsole/net/components/response-tab.js > @@ +56,5 @@ > > + return null; > > + } > > + > > + let jsonString = new String(content.text); > > + return Json.parseJSONString(jsonString, "http://" + file.request.url); > > There is an extra parameter here that is unused by parseJSONString Fixed > > ::: devtools/client/webconsole/net/utils/json.js > @@ +77,5 @@ > > + > > + try { > > + return JSON.parse(jsonString); > > + } catch (err) { > > + console.error(err); > > Do we really want to console.error here? This is user-generated data that > could very well not be JSON. Debatable but I don't think it's worth > notifying in the console about this OK, done. > > @@ +94,5 @@ > > + > > + try { > > + return JSON.parse(jsonString); > > + } catch (err) { > > + console.error(err); > > Ditto Done (In reply to Brian Grinstead [:bgrins] from comment #141) > Comment on attachment 8729593 [details] [diff] [review] > bug1211525-tests.patch > > Review of attachment 8729593 [details] [diff] [review]: > ----------------------------------------------------------------- > > Just some notes, haven't reviewed everything > > ::: devtools/client/shared/components/reps/test/unit/test_string.js > @@ +22,5 @@ > > + equal(isCropped(shortString, limit), false); > > + equal(isCropped(longString, limit), true); > > + > > + // cropString > > + equal(cropString(longString, limit), "This...ing"); > > Something I noticed. It's using "..." to crop, but really it should a value > from a properties file or `Services.prefs.getComplexValue("intl.ellipsis", > Ci.nsIPrefLocalizedString).data`. I know the reps code also has to run in > content priv - but do we have some way to load localization files? If not, > it could use the '…' character at least instead of ... Localization is one of the topics we need discuss yet. We need a solution that plays nice with React and works in the content priv scope. I'll put this on agenda for the Components meeting. I replace '...' with '…' for now. > > This could definitely be done in a follow up, or if you want to update right > now while the test is being added that works too > > ::: devtools/client/webconsole/net/test/mochitest/browser_net_basic.js > @@ +22,5 @@ > > + method: "GET", > > + url: JSON_XHR_URL > > + }); > > + > > + ok(netInfoBody, "The network details must be available"); > > Are there other properties on netInfoBody that would be useful to assert > beyond truthiness? The call to executeAndInspectXhr is doing an identical > assertion before resolving Yes, added check for Headers and Response tabs (these two should always be available). > > ::: devtools/client/webconsole/net/test/unit/test_json-utils.js > @@ +27,5 @@ > > + * Testing API provided by webconsole/net/utils/json.js > > + */ > > +function run_test() { > > + // parseJSONString > > + ok(parseJSONString(simpleJson) != null); > > Could check is(parseJSONString(simpleJson).name, "John") Done > > @@ +28,5 @@ > > + */ > > +function run_test() { > > + // parseJSONString > > + ok(parseJSONString(simpleJson) != null); > > + ok(parseJSONString(jsonInFunc) != null); > > Could check is(parseJSONString(jsonInFunc).name, "John") Done > > @@ +31,5 @@ > > + ok(parseJSONString(simpleJson) != null); > > + ok(parseJSONString(jsonInFunc) != null); > > + > > + // isJSON > > + equal(isJSON(textMimeType, json1), true); > > So should this be true or false with jsonMimeType? What about with > unknownMimeType? Couple more checks added. Thanks! Honza

(In reply to Jan Honza Odvarko [:Honza] from comment #142) > > We've been working to remove hardcoded instances of .theme-light and > > .theme-dark throughout our codebase. It makes the CSS inflexible and harder > > to adapt it for theming when we rely on those class names. Also, I don't > > get this pattern where .theme-light and .theme-dark are repeated with the > > same selector. I take it this is a way to set the Firebug theming as > > default and then override light and dark? > > > > Ideally any variations between themes will be handled with variables so > > changes can be contained to where the variables are defined. So in this > > particular case, there's another `.netInfoBody > .tabs .tab-panel` above > > that does `border: 1px solid var(--net-border)`. So can we set that > > net-border variable to be transparent in light and dark theme? If not, then > > we should answer either: > > > > 1) Would the UI be OK to use the default net border color? > > 2) Should a new variable be made for this case? > TODO Ah, forgot to reply here.. This comment touches the bug 1244054. The thing is that Firebug theme is quite different that light and dark. Light and Dark themes are inherently very similar, the difference is mostly in light and dark colors and so, it's simple to extract these colors into variables and provide two sets of values. This approach doesn't require different CSS rules since the differentiation is done through variables. I don't know yet what is the best for the Firebug theme, but if we use the same approach (differentiation through variables) we might end-up with big set of variables touching plenty of various details and being a mess. It might be better to just allow CSS rules (again?) as the way how to implement the difference. Why exactly this makes CSS inflexible? Can we make the CSS structure better? We might want to move this discussion to bug 1244054 (hope we can find a quick and good solution here). Honza

Comment on attachment 8729559 [details] [diff] [review] bug1211525-eslint.patch Review of attachment 8729559 [details] [diff] [review]: ----------------------------------------------------------------- There's a lot of react-specific changes here that I don't really feel comfortable reviewing as I don't have enough experience with the framework. They seem simple enough, but in some cases propTypes have been added, displayNames changed, methods moved around, etc... and I can't tell if that's eslint related only, or has more important implications in how these react components are going to work. So I'm transferring this to Lin. Feel free to re-assign to someone else if needed.

@Brian: I know last week was crazy, but do you have time now to finish the review, please? Honza

Comment on attachment 8730298 [details] [diff] [review] bug1211525.patch Review of attachment 8730298 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for delays - here's another round... I think it's pretty close ::: devtools/client/locales/en-US/netmonitor.properties @@ +304,5 @@ > +netRequest.responseBodyDiscarded=Response body was not stored. > + > +# LOCALIZATION NOTE (netRequest.requestBodyDiscarded): A label used > +# in Post tab if the post body is not available. > +netRequest.requestBodyDiscarded=Request post body was not stored. should this be POST? @@ +308,5 @@ > +netRequest.requestBodyDiscarded=Request post body was not stored. > + > +# LOCALIZATION NOTE (netRequest.post): A label used for Post tab > +# This tab displays HTTP post body > +netRequest.post=Post ditto ::: devtools/client/webconsole/net/components/net-info-body.css @@ +14,5 @@ > + display: block; > +} > + > +.netInfoBody *:focus { > + outline: 0 !important; curious - why no outline on focus? @@ +75,5 @@ > + overflow: auto; > + height: calc(100% - 31px); /* minus the height of the tab bar */ > +} > + > +.netInfoBody > .tabs .tab-panel > DIV, DIV should be lower case I guess, or if there's a class to use instead let's use that @@ +107,5 @@ > + color: var(--theme-body-color); > +} > + > +.theme-light .netInfoBody > .tabs .tab-panel, > +.theme-dark .netInfoBody > .tabs .tab-panel { It looked like this was still a TODO in the last comment, but why all these special cases for both light and dark theme. Seems like if it's applied on both themes it should be folded into the rule above, right? See related comment in net-info-group.css about the firebug images ::: devtools/client/webconsole/net/components/net-info-body.js @@ +139,5 @@ > + > + return panels; > + }, > + > + render() { There's been some discussion in Bug 1258353 about moving render() just below react boilerplate but above component specific methods. See: https://bugzilla.mozilla.org/show_bug.cgi?id=1258353#c7 ::: devtools/client/webconsole/net/components/net-info-group.css @@ +28,5 @@ > + padding-left: 3px; > +} > + > +.netInfoBody .netInfoGroupTwisty { > + background-image: url("chrome://devtools/skin/images/firebug/twisty-closed-firebug.svg"); I'd flip this and make the light/dark theme image the default and then override properties for the firebug theme. That's more consistent with the rest of the tools ::: devtools/client/webconsole/net/components/post-tab.js @@ +248,5 @@ > +/** > + * Make sure the string doesn't contains any dangerous characters > + * (e.g. multipart post body can contain uploaded files) > + */ > +function safeString(text) { I'm not a fan of the word 'safe' and 'dangerous' here. It makes it sound like this is a security feature when really from the comments in the bug it sounds like it's a workaround for a "not well-formed" error that react reports when there's multipart data passed to render. Is that accurate? If so, could you rename the function and comment to explain @@ +250,5 @@ > + * (e.g. multipart post body can contain uploaded files) > + */ > +function safeString(text) { > + text = JSON.stringify(text); > + text = text.replace(/\\r\\n/g, "\r\n").replace(/\\"/g, "\""); I don't understand the purpose of this replace - it looks like it's replacing characters with themselves. What's a situation where this would change the value of 'text'? ::: devtools/client/webconsole/net/main.js @@ +25,5 @@ > + "chrome://devtools/locale/netmonitor.properties"); > + > +// Stylesheets > +var styleSheets = [ > + "jsonview/css/dom-tree", Nit but I'd actually prefer to see the full path here to make grepping easier const styleSheets = [ "resource://devtools/client/jsonview/css/dom-tree.css", "resource://devtools/client/jsonview/css/toolbar.css", "resource://devtools/client/shared/components/reps/reps.css", etc ] ::: devtools/client/webconsole/net/net-request.css @@ +13,5 @@ > + > +/* Dark and Light theme use monospace font even for labels */ > +:root.theme-light, > +:root.theme-dark { > + --net-font-family: monospace; Again, I'd flip this and make monospace default then do an override for the Firebug theme. It just better fits in with how the rest of the theme works @@ +33,5 @@ > +/* Themes */ > + > +.theme-dark .netRequest .message-body, > +.theme-light .netRequest .message-body { > + background-position: 0 2px; What's up with this change?

(In reply to Brian Grinstead [:bgrins] from comment #147) > Comment on attachment 8730298 [details] [diff] [review] > bug1211525.patch > > Review of attachment 8730298 [details] [diff] [review]: > ----------------------------------------------------------------- > > Sorry for delays - here's another round... I think it's pretty close > > ::: devtools/client/locales/en-US/netmonitor.properties > @@ +304,5 @@ > > +netRequest.responseBodyDiscarded=Response body was not stored. > > + > > +# LOCALIZATION NOTE (netRequest.requestBodyDiscarded): A label used > > +# in Post tab if the post body is not available. > > +netRequest.requestBodyDiscarded=Request post body was not stored. > > should this be POST? Sounds better, done. > > @@ +308,5 @@ > > +netRequest.requestBodyDiscarded=Request post body was not stored. > > + > > +# LOCALIZATION NOTE (netRequest.post): A label used for Post tab > > +# This tab displays HTTP post body > > +netRequest.post=Post > > ditto Done > > ::: devtools/client/webconsole/net/components/net-info-body.css > @@ +14,5 @@ > > + display: block; > > +} > > + > > +.netInfoBody *:focus { > > + outline: 0 !important; > > curious - why no outline on focus? It's just that every click on a tab (with HTTP details) focuses it and the border appears. It doesn't look nice. I don't have a strong opinion and I can remove it if this is an issue. > > @@ +75,5 @@ > > + overflow: auto; > > + height: calc(100% - 31px); /* minus the height of the tab bar */ > > +} > > + > > +.netInfoBody > .tabs .tab-panel > DIV, > > DIV should be lower case I guess, or if there's a class to use instead let's > use that Fixed > > @@ +107,5 @@ > > + color: var(--theme-body-color); > > +} > > + > > +.theme-light .netInfoBody > .tabs .tab-panel, > > +.theme-dark .netInfoBody > .tabs .tab-panel { > > It looked like this was still a TODO in the last comment, but why all these > special cases for both light and dark theme. Seems like if it's applied on > both themes it should be folded into the rule above, right? See related > comment in net-info-group.css about the firebug images I simplified the rules (joined some of them) and Firebug is not default. > > ::: devtools/client/webconsole/net/components/net-info-body.js > @@ +139,5 @@ > > + > > + return panels; > > + }, > > + > > + render() { > > There's been some discussion in Bug 1258353 about moving render() just below > react boilerplate but above component specific methods. See: > https://bugzilla.mozilla.org/show_bug.cgi?id=1258353#c7 Yes, I agree with comment #7 (render near the top) and I can fix this later (this touches a lot of places). React eslint rules has changed several times already during this patch life-time and I'd like to stop moving things around again and again for now and land this first. > > ::: devtools/client/webconsole/net/components/net-info-group.css > @@ +28,5 @@ > > + padding-left: 3px; > > +} > > + > > +.netInfoBody .netInfoGroupTwisty { > > + background-image: url("chrome://devtools/skin/images/firebug/twisty-closed-firebug.svg"); > > I'd flip this and make the light/dark theme image the default and then > override properties for the firebug theme. That's more consistent with the > rest of the tools Agree, done > > ::: devtools/client/webconsole/net/components/post-tab.js > @@ +248,5 @@ > > +/** > > + * Make sure the string doesn't contains any dangerous characters > > + * (e.g. multipart post body can contain uploaded files) > > + */ > > +function safeString(text) { > > I'm not a fan of the word 'safe' and 'dangerous' here. It makes it sound > like this is a security feature when really from the comments in the bug it > sounds like it's a workaround for a "not well-formed" error that react > reports when there's multipart data passed to render. Is that accurate? If > so, could you rename the function and comment to explain Done (renamed to 'sanitize' and commented). > > @@ +250,5 @@ > > + * (e.g. multipart post body can contain uploaded files) > > + */ > > +function safeString(text) { > > + text = JSON.stringify(text); > > + text = text.replace(/\\r\\n/g, "\r\n").replace(/\\"/g, "\""); > > I don't understand the purpose of this replace - it looks like it's > replacing characters with themselves. What's a situation where this would > change the value of 'text'? It's for stringifying `multipart/form-data` containing binary data. The second line is fixing escaped chars (newlines) and the third line is removing enclosing quotes. > > ::: devtools/client/webconsole/net/main.js > @@ +25,5 @@ > > + "chrome://devtools/locale/netmonitor.properties"); > > + > > +// Stylesheets > > +var styleSheets = [ > > + "jsonview/css/dom-tree", > > Nit but I'd actually prefer to see the full path here to make grepping easier Agree, done. > > const styleSheets = [ > "resource://devtools/client/jsonview/css/dom-tree.css", > "resource://devtools/client/jsonview/css/toolbar.css", > "resource://devtools/client/shared/components/reps/reps.css", > etc > ] > > ::: devtools/client/webconsole/net/net-request.css > @@ +13,5 @@ > > + > > +/* Dark and Light theme use monospace font even for labels */ > > +:root.theme-light, > > +:root.theme-dark { > > + --net-font-family: monospace; > > Again, I'd flip this and make monospace default then do an override for the > Firebug theme. It just better fits in with how the rest of the theme works Done > > @@ +33,5 @@ > > +/* Themes */ > > + > > +.theme-dark .netRequest .message-body, > > +.theme-light .netRequest .message-body { > > + background-position: 0 2px; > > What's up with this change? Good call, obsolete, removed. Also: - The shared/components/tree is adopted - Patches rebased Thanks! Honza

Comment on attachment 8733880 [details] [diff] [review] bug1211525.patch Review of attachment 8733880 [details] [diff] [review]: ----------------------------------------------------------------- OK, thanks for sticking with it through the reviews. I think this is good to go and we can handle additional issues in follow ups. * There's a weird hover state on the light theme with expanded requests on 404's where it's red when the mouse isn't over it but becomes white when hovered ::: devtools/client/webconsole/net/components/headers-tab.js @@ +61,5 @@ > + > + return ( > + DOM.div({className: "headersTabBox"}, > + DOM.div({className: "panelContent"}, > + NetInfoGroupList({groups: groups}) I don't see the status code in the response headers section of the tab (testing on http://www.cnn.com/ and including some 404s that I get)