Followup for bug 1134073: add a new "Stack Trace" tab to the HTTP log message in Console, and show the request stack trace there.

The existing console and stack trace code was quite messy, so this is a rather big patch with many related refactorings. Parts 1 - 3: Add new features and improvements to React components in devtools/client/shared/components/frame.js, fix existing usages in Memory and Performance tool. Parts 4 - 5: Add a Stack Trace tab to console netlogging. Parts 6 - 7: Use the frame.js components to display all source locations and stack traces in Console.

The Frame component now has two new options: - showAnonymousFunctionName: display empty function name as <anonymous> - showFullSourceUrl: show the long form source URL instead of the short one I also fixed the showEmptyPathAsHost option added in bug 1255908 - it shouldn't be a property of the frame object. There's also one simplification: render the line and column into one "frame-link-line" element, don't create a separate <span> for every number/colon. The tests now contain test cases for the new options, and are refactored to support specifying options in the test cases (i.e., pass the whole props object as input instead of just the frame). The code that inspects the markup is changed to accomodate the frame-link-line change. The CSS styles got cleaned up and simplified.

Fixes existing usages of the Frame component in Performance and Memory tools.

Removes duplicate styling for the Frame component from jit-optimizations.css (in Performance tool)

Now we're finally getting to the main patch: add a new "Stack Trace" tab to the HTTP console log message.

Here I propose a styling cleanup of the HTTP console log messages. Instead of defining its own font families and styles, just inherit them from the parent console. I think it looks much better now and better fits into the surrounding UI.

Here I refactor the console output to consistently use the frame.js components to display all source locations: message locations, stack traces in console.trace() and console.error(). The existing code is quite a mess and I had to do a lot of refactoring to make all the flex layouts work correctly: - all messages use a message-flex-body container, it's rendered only at one place in Messages.Simple._renderBody, duplicate code removed - the message-repeats and message-location nodes are rendered only at one place (in Messages.Simple), duplicate code removed - added support for "_attachment" property to Messages.Simple, similar to existing "_message" property. This allows for elegant rendering of attachment-like data like stack traces or a table in console.table() This patch is quite a hairball and might be difficult to review, but I've been unable to divide it into multiple parts that would still make independent sense.

Fixed the webconsole tests to respect the changed markup. Also, fixed a bug where the check for source like always succeeded, even when the line was incorrect: location.line === rule.source.line is never true, because one is string ("1"), the other one is number (1)

This image contains an overview of all important UI changes that happen in this bug.

Comment on attachment 8764573 [details] [diff] [review] Part 1: React components for displaying stack traces and frames Review of attachment 8764573 [details] [diff] [review]: ----------------------------------------------------------------- In general, this looks good. Marking as r- because of the style changes, though. If those need to happen, they should happen in a separate patch to keep the blame records clean and also to reduce noise for review. I'm not sure that they should happen though, see below for details. ::: devtools/client/shared/components/frame.js @@ +3,5 @@ > * You can obtain one at http://mozilla.org/MPL/2.0/. */ > > "use strict"; > > +const { DOM, createClass, createElement, PropTypes } = require("devtools/client/shared/vendor/react"); Is there a reason to switch away from importing DOM as dom here? The current style is consistent with other files in devtools. See https://dxr.mozilla.org/mozilla-central/search?q=DOM%3A+dom&redirect=false @@ -85,3 @@ > displaySource = host; > } > - sourceElements.push(dom.span({ It doesn't seem like these style changes are necessary, though I might be mistaken. Is it showing up in ESLint as a warning/error? @@ +157,4 @@ > } > }); > + > +const AsyncFrame = createClass({ Is this component used later? It would be good to see an example of its use in the same patch as it is introduced.

Comment on attachment 8764574 [details] [diff] [review] Part 2: Fix imports and usages of the frame.js module Review of attachment 8764574 [details] [diff] [review]: ----------------------------------------------------------------- We generally try to only have one component per file, unless the additional components are only used in the main component. Instead of making this change, the components from the previous patch should be in their own files.

Comment on attachment 8764575 [details] [diff] [review] Part 3: Remove duplicate styles for stack frames in Performance tool Review of attachment 8764575 [details] [diff] [review]: ----------------------------------------------------------------- I'm not the best person to review this since I'm not active on that module. I would re-route, but I'm not sure if the reviews from the previous two patches will change this.

Comment on attachment 8764579 [details] [diff] [review] Part 6: Use the stack frame components in console output, cleanup markup and styles Review of attachment 8764579 [details] [diff] [review]: ----------------------------------------------------------------- I'm not the best person for CSS review... :bgrins might be, or would know better who to reroute to.

Comment on attachment 8764579 [details] [diff] [review] Part 6: Use the stack frame components in console output, cleanup markup and styles Review of attachment 8764579 [details] [diff] [review]: ----------------------------------------------------------------- webconsole.js changes look good The console-output js and css changes are hard to review - a lot of that CSS and markup was added after experimentation and struggles with combining XUL flex and CSS flex. It could be that as the console's evolved over time and moved everything into a container html div these things aren't necessary. But what would really help for reviewing is if you could break this up into 2 patches - one that does strictly the minimum needed to implement this feature, and then another that does any cleanup. ::: devtools/client/webconsole/webconsole.js @@ -2569,5 @@ > } > > let fullURL = url.split(" -> ").pop(); > - let locationNode = this.document.createElementNS(XHTML_NS, "a"); > - locationNode.draggable = false; I don't know exactly why 'draggable = false' was added, but I have to assume it's there for a reason (maybe a test will catch it but not sure). Are you sure it can be removed?

Addressed the review comments: - one file per component, lets us keep the existing import statements intact - the AsyncFrame component is private, not exported (I don't see any use case where it could be useful standalone) - respect the existing conventions to import React.DOM as dom - no unneccessary style changes

Addressed the review comments: - as we don't need to change any import statement, this patch collapsed to a simple test fix (the changed frame-link-line markup)

Removing duplicate styles in Performance tool. As jsantell and vporof are on Tofino, I don't know who is the right reviewer. And bgrins knows everything :)

Tried to divide the console-output.js patch into multiple ones. The CSS styles: - removed flex: property from element that is not a flex-item at all - specify correct overflow behavior for message-body-wrapper - remove obsolete styling for message-location that is a leftover from times when the frame.js React component was not used and the message-location element was the actual link. This is no longer true. This should have been done back in bug 1251033 - remove obsolete styling of the stack traces (the ul/li list), now replaced by StackTrace React component The Widgets.StackTrace class now uses the StackTrace React component instead of generating its own markup Also improved the createLocationNode function in webconsole.js: - the location node is a <div>, not <a> - the <a> element is now in the Frame React component - the element doesn't need to have draggable=true - I added it instead to the Frame component (see Part 1 patch) - fixed the code that retrieved "category" from the parent element - it's not always the direct parent (message-flex-body is in between) - fixed the showEmptyPathAsHost usage - this is a followup/fix to Jaideep's work in bug 1255908 I'm also attaching a screenshot that shows the state of the UI after this patch and shows the problems that are fixed in Part 7 (the markup cleanup)

Cleanup of the webconsole markup: - render the repeat and location node at one place, the same way for all message types - render attachment-like objects (stacks, tables) using an _attachment property

Fix the webconsole mochitests - correct CSS selectors etc.

Updated the patch to fix all webconsole mochitests - now, after these changes, the try run is green.

Comment on attachment 8764960 [details] [diff] [review] Part 5: Style cleanup for HTTP console log messages I haven't applied the patch, I just went over the attachment with a list of the UI changes—so if I'm missing information by doing this, please reflag me for ui-review. Based on the attachment, I had few questions. It looks like we're losing some alignment we had in the text both for sources and their line numbers. Is it possible to bring back some of that styling? I can provide some mockups of what I think would look good if you need. This might not belong in this particular bug, but I think it might look nice to align the Response header tables holistically (currently each table is based on its own tables' width, not really treated globally). (This is what I imagine that would look like: http://cl.ly/0u0Q293V0S2b) I've annotated these notes graphically in the questions-about-the-UI-changes.png attachment.

(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #32) > Based on the attachment, I had few questions. It looks like we're losing > some alignment we had in the text both for sources and their line numbers. > Is it possible to bring back some of that styling? I can provide some > mockups of what I think would look good if you need. The neat alignment in the "before" screenshot is just a random coincidence :) All the line numbers have two digits, and we're not displaying the column numbers in the "before" state. It's all going to break down once there are more diverse line numbers like "5", "42" or "256". > This might not belong in this particular bug, but I think it might look nice > to align the Response header tables holistically (currently each table is > based on its own tables' width, not really treated globally). (This is what > I imagine that would look like: http://cl.ly/0u0Q293V0S2b) Nice idea, but is achievable if we make the width of the header names column a fixed width, with possible overflow of the header names. The "Request Headers" and "Reponse Headers" tables are two independent <table> elements - it's not possible to link their layout together, i.e., to make content width in one table influence the other one.

Comment on attachment 8764959 [details] [diff] [review] Part 4: Add stack trace tab to HTTP console log message Review of attachment 8764959 [details] [diff] [review]: ----------------------------------------------------------------- Looks great to me! Just couple of things I'd change see my inline comments. Honza ::: devtools/client/locales/en-US/netmonitor.properties @@ +322,5 @@ > netRequest.params=Params > + > +# LOCALIZATION NOTE (netRequest.stacktrace): A label used for request stacktrace tab > +# This tab displays the request's JavaScript stack trace. > +netRequest.stacktrace=Stack Trace The debugger is using "Call Stack". We should use the same label so, it's consistent. ::: devtools/client/webconsole/webconsole.js @@ +1638,5 @@ > client: this.webConsoleClient, > response: networkInfo, > node: messageNode, > + update: false, > + onViewSourceInDebugger It would be better if you pass 'this' (WebConsoleFrame) into the onNetworkEvent and implement 'onViewSourceInDebugger' action as a method of NetRequest directly. this.window.NetRequest.onNetworkEvent({ consoleFrame: this, response: networkInfo, node: messageNode, update: false, onViewSourceInDebugger }); In NetRequest constructor you can: this.client = log.consoleFrame.webConsoleClient; this.owner = log.consoleFrame.owner; .. and so you have the owner available.

Comment on attachment 8764960 [details] [diff] [review] Part 5: Style cleanup for HTTP console log messages Review of attachment 8764960 [details] [diff] [review]: ----------------------------------------------------------------- Looks good and Helen agrees with the font changes so, R+ Thanks Jarda! Honza

(In reply to Jarda Snajdr [:jsnajdr] from comment #33) > The neat alignment in the "before" screenshot is just a random coincidence > :) All the line numbers have two digits, and we're not displaying the column > numbers in the "before" state. It's all going to break down once there are > more diverse line numbers like "5", "42" or "256". Oh, good to know! Never mind then :) > > This might not belong in this particular bug, but I think it might look nice > > to align the Response header tables holistically (currently each table is > > based on its own tables' width, not really treated globally). (This is what > > I imagine that would look like: http://cl.ly/0u0Q293V0S2b) > > Nice idea, but is achievable if we make the width of the header names column > a fixed width, with possible overflow of the header names. The "Request > Headers" and "Reponse Headers" tables are two independent <table> elements - > it's not possible to link their layout together, i.e., to make content width > in one table influence the other one. Phooey—guess I'm not surprised though. Anyway, ui-r+ from me on the font changes like Honza said!

Fixed review comments: - use "Call Stack" label for the tab, be consistent with debugger - improved the way how the onViewSourceDebugger action is passed down in props

Comment on attachment 8764954 [details] [diff] [review] Part 1: React components for displaying stack traces and frames Review of attachment 8764954 [details] [diff] [review]: ----------------------------------------------------------------- As far as I'm concerned this is fine now (with the following two comments). However, I didn't test manually. We should ensure that someone has done that before committing the patch series. ::: devtools/client/shared/components/frame.js @@ +108,3 @@ > displaySource = host; > } > + This line is green and looks like it has whitespace on it. I would think this would trigger an ESLint issue, but maybe this file isn't being linted? @@ +133,5 @@ > if (isLinkable) { > sourceEl = dom.a({ > onClick: e => { > e.preventDefault(); > + onClick(frame); Was this not working as expected before?

Jarda, could you post the pages that you're using to test this (either as attachments here or pushing to a publicly accessible server)? That will help with manual testing.

Comment on attachment 8764957 [details] [diff] [review] Part 3: Remove duplicate styles for stack frames in Performance tool Review of attachment 8764957 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, can't see a difference before/after the patch

Something I noticed when testing at https://bgrins.github.io/devtools-demos/console/all.html and using the 'console API' button. The trace locations end up displaying the entire URL (a little inconsistent with the rest of the panel but I guess if there's space for it that's OK). But, they end up causing even relatively short function names to collapse as the panel gets a little smaller. IMO the function names should have priority over the location links (i.e. flex-shrink 0).

Comment on attachment 8764962 [details] [diff] [review] Part 6: Use StackTrace React component in webconsole, cleanup message-location Review of attachment 8764962 [details] [diff] [review]: ----------------------------------------------------------------- The css and markup changes seem OK to me, I tested it out locally and don't see any regressions ::: devtools/client/themes/webconsole.css @@ +111,5 @@ > } > > +.stack-trace .frame-link-source, > +.message-location .frame-link-source { > + direction: rtl; What does this do? Please remove it unless if there's a reason for it. Commenting it out doesn't seem to change anything for me. ::: devtools/client/webconsole/console-output.js @@ +3637,4 @@ > result.className = "stacktrace devtools-monospace"; > > if (this.stacktrace) { > + this.output.owner.ReactDOM.render(this.output.owner.StackTraceView({ So this seems generally fine, but I'm assuming we need to destroy the component when this thing gets destroyed. I found this: https://facebook.github.io/react/docs/top-level-api.html#reactdom.unmountcomponentatnode. I'm guessing we'll need to override the destroy function here and call it but I'd like also to know if this is actually needed before we do it. Lin, any ideas?

Comment on attachment 8764963 [details] [diff] [review] Part 7: Clean up and simplify markup of webconsole messages Review of attachment 8764963 [details] [diff] [review]: ----------------------------------------------------------------- Lin has a better understanding of the hierarchy of objects in this file now, so forwarding to her. Jarda, is this patch intended to cause no visual / behavioral changes at all (i.e. is it basically a refactor)?

Comment on attachment 8764963 [details] [diff] [review] Part 7: Clean up and simplify markup of webconsole messages Actually forwarding

(In reply to Brian Grinstead [:bgrins] from comment #43) > Comment on attachment 8764963 [details] [diff] [review] > Jarda, is this patch intended to cause no visual / > behavioral changes at all (i.e. is it basically a refactor)? I guess based on https://bugzilla.mozilla.org/attachment.cgi?id=8764965 the answer is no, and that this is causing updated layout based on the markup change

(In reply to Lin Clark [:linclark] from comment #39) > Jarda, could you post the pages that you're using to test this (either as > attachments here or pushing to a publicly accessible server)? That will help > with manual testing. Attached the page I've been using for testing during development of this feature.

(In reply to Brian Grinstead [:bgrins] from comment #41) > The trace locations end up displaying the entire URL (a little inconsistent > with the rest of the panel but I guess if there's space for it that's OK). > But, they end up causing even relatively short function names to collapse as > the panel gets a little smaller. IMO the function names should have > priority over the location links (i.e. flex-shrink 0). If the function name is very long and has flex-shrink set to 0, it will occupy the whole width of the container and will squeeze the URL completely out of the visible area. That's why I keep it shrinkable and set the .frame-link-function-name's max-width to 50%. The goal is to: 1) make both function-name and URL shrink with ellipsis when there is insufficient space 2) keep both of them at least partially visible 3) start shrink the function-name only after the URL cannot be shrinked futher You can play with the styles for a while and maybe you'll find CSS rules that achieve the goal better. I'm just setting function-name's max-width to 50%, which is simple, but not optimal. (In reply to Brian Grinstead [:bgrins] from comment #42) > > +.stack-trace .frame-link-source, > > +.message-location .frame-link-source { > > + direction: rtl; > > What does this do? Please remove it unless if there's a reason for it. > Commenting it out doesn't seem to change anything for me. This makes the URL overflow to the left side and display the ellipsis on the left. Not "http://examp..." but rather "...page.js:20". I don't like this abuse of the "direction" property, but HTML/CSS doesn't provide anything better at this moment. XUL labels have the crop=start/center/end property, which behaves much better.

(In reply to Brian Grinstead [:bgrins] from comment #45) > I guess based on https://bugzilla.mozilla.org/attachment.cgi?id=8764965 the > answer is no, and that this is causing updated layout based on the markup > change Exactly, it fixes the layout issues described in the picture. One of them is bug 1282745, the wrong placement of the Learn More link.

(In reply to Lin Clark [:linclark] from comment #38) > ::: devtools/client/shared/components/frame.js > @@ +108,3 @@ > > displaySource = host; > > } > > + > > This line is green and looks like it has whitespace on it. I would think > this would trigger an ESLint issue, but maybe this file isn't being linted? This is just a newline without any space, no ESLint issue here (or anywhere else in the file, it's eslint-clean). Formatting improvement of a code that's being substantially changed anyway. > @@ +133,5 @@ > > if (isLinkable) { > > sourceEl = dom.a({ > > onClick: e => { > > e.preventDefault(); > > + onClick(frame); > > Was this not working as expected before? This is good for the StackTrace component which displays multiple Frames and the onClick handler is the same (display the frame source in debugger). The onClick gets the frame data as a parameter. For message location (a single Frame), you had to do something like this: let frameData = ... let onClick = () = { this.viewSourceInDebugger(frameData); }; let frameComponent = Frame({ frame: frameData, onClick }); This isn't feasible anymore when you want to create StackTrace with multiple Frames.

Changed the WebConsoleFrame.unmountMessage method to properly unmount the StackTrace component. It doesn't need to care about multiple message-location's now, there is only one. Not calling the unmountComponentAtNode leads to real leaks, because React keeps (in its ReactMount internal object) a map between all the root components and their container nodes. I also reported bug 1283059 to unmount the NetInfoBody component in the netlogging code. It's not happening there and is leaking, too.

Comment on attachment 8765979 [details] [diff] [review] Part 4: Add stack trace tab to HTTP console log message Review of attachment 8765979 [details] [diff] [review]: ----------------------------------------------------------------- Nice! Honza

Comment on attachment 8766256 [details] [diff] [review] Part 6: Use StackTrace React component in webconsole, cleanup message-location Review of attachment 8766256 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/webconsole/console-output.js @@ +3637,4 @@ > result.className = "stacktrace devtools-monospace"; > > if (this.stacktrace) { > + this.output.owner.ReactDOM.render(this.output.owner.StackTraceView({ Mounting a component into a DOM element affects the DOM. This shouldn't be triggered by a render, per the component spec docs. https://facebook.github.io/react/docs/component-specs.html > The render() function should be pure, meaning that it does not modify component state, it returns the same result each time it's invoked, and it does not read from or write to the DOM or otherwise interact with the browser (e.g., by using setTimeout). If you need to interact with the browser, perform your work in componentDidMount() or the other lifecycle methods instead. Keeping render() pure makes server rendering more practical and makes components easier to think about.

Comment on attachment 8764963 [details] [diff] [review] Part 7: Clean up and simplify markup of webconsole messages Review of attachment 8764963 [details] [diff] [review]: ----------------------------------------------------------------- To be honest, I feel uncomfortable with the amount that is being changed in this file. How much of this change is strictly necessary? We should aim for having the bare minimum change here. That would be true even without parallel work going on, but it's doubly true with that in effect. This may mean that we don't use React for the StackTrace in this pass. I think that's fine, and given the problem with the render function I mentioned in the last review, might even be for the best. ::: devtools/client/webconsole/console-output.js @@ +968,2 @@ > let body = this.document.createElementNS(XHTML_NS, "span"); > + body.className = "message-body devtools-monospace"; Why is this change necessary? @@ +986,5 @@ > container.textContent = this._message; > } > > + // do this before repeatNode is rendered - it has no effect afterwards > + this._repeatID.textContent += "|" + container.textContent; This seems to be unintuitive. Why is repeatID being altered here? @@ +1046,5 @@ > } > > // The ConsoleOutput owner is a WebConsoleFrame instance from webconsole.js. > // TODO: move createLocationNode() into this file when bug 778766 is fixed. > + return this.output.owner.createLocationNode({url, line, column }); Looks like there are code style changes in this one.

(In reply to Lin Clark [:linclark] from comment #53) > Mounting a component into a DOM element affects the DOM. This shouldn't be > triggered by a render, per the component spec docs. The Widgets.Stacktrace component is not a React component. Neither are other Widgets.* components in this file. The render method is expected to return a DOM element (a real one) that will be then inserted into the DOM by the caller. For example, at this place: https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/console-output.js#1677,1692

Sorry, you are right, the `render` method there is on the existing widget, not a React component. My comment does not apply in that case. I'll look at the patch again.

(In reply to Lin Clark [:linclark] from comment #54) > To be honest, I feel uncomfortable with the amount that is being changed in > this file. How much of this change is strictly necessary? We should aim for > having the bare minimum change here. The visible effect of this patch is fixing layout issues described in attachment 8764965 [details]. That is, the stack trace not using the whole available width, and the "Learn More" link being misplaced (bug 1282745). The root cause of these issues is that the current markup structure is inconsistent in the way how the message-body-wrapper, message-flex-body, message-body and message-location elements are related to each other and to the "attachment-like" block elements like stacktrace or table. In some messages, the message-body-wrapper and message-location are siblings. In other cases, the message-location is a descendant of message-body-wrapper. In some messages, the stacktrace element is a child of message-body. In other cases, they are siblings. Obviously, such things are rather difficult to style well. So, all this patch does is that it rearranges these elements into a consistent structure: message-body-wrapper - message-flex-body - message-body - message-repeats - message-location - attachment (stacktrace or consoletable) The message-flex-body takes care of the proper alignment of the message text to the repeats and location nodes. And the attachment is always its sibling. I understand that the patch might be difficult to review, especially because the diff reads like a huge blob of random changes. But I think if you look at what the code does before and what it does after the patch, and how the old and new markup structure looks like, it becomes much simpler and clearer. > That would be true even without parallel work going on, but it's doubly true with that in effect. I'm of course aware of the work that's going on in new-console-output, and I did all this work with the intent of porting the whole location/stacktrace functionality there as soon as this bug lands in mozilla-central (or at least the React components from part 1). The markup in new-console-output is very similar, if not identical. I think this change delivers a lot of value to console users, which they wouldn't otherwise get for several more months until new-console-output ships to production. And it also makes all location/stacktrace rendering consistent across all devtools and reuse the same code. If you decide to have another look at the patch, let me know if I can provide any other help with better understanding and reviewing the changes. We can have a Vidyo call if you want to. > ::: devtools/client/webconsole/console-output.js > @@ +968,2 @@ > > let body = this.document.createElementNS(XHTML_NS, "span"); > > + body.className = "message-body devtools-monospace"; > > Why is this change necessary? This is a part of the change that divides message-body-wrapper and message-body into two separate elements. > @@ +986,5 @@ > > container.textContent = this._message; > > } > > > > + // do this before repeatNode is rendered - it has no effect afterwards > > + this._repeatID.textContent += "|" + container.textContent; > > This seems to be unintuitive. Why is repeatID being altered here? Yes, this is actually quite a horrible code. This particular change just moves the existing line to another place. Without this, messages with different text would get classified as identical. Or more precisely, test browser_webconsole_bug_644419_log_limits.js would fail (happened in one of try runs). To have any effect, the changes to this._repeatID must be very carefully placed. The need to happen before a call to Messages.Simple._renderRepeatNode, because this function computes a uid string from the repeatID, stores is as the _uid property of the DOM node (at [1]), and after this the _repeatID property is never looked at again. The actual filtering happens in WebConsoleFrame._filterRepeatedMessages which checks the node._uid property (at [2]). Good code would of course store such data in some model/state and not as a property of the markup. The new-console-output uses the message packet data to detect duplicated, so this mess is completely avoided there. [1] https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/console-output.js#1009 [2] https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/webconsole.js#1155 > > @@ +1046,5 @@ > > } > > > > // The ConsoleOutput owner is a WebConsoleFrame instance from webconsole.js. > > // TODO: move createLocationNode() into this file when bug 778766 is fixed. > > + return this.output.owner.createLocationNode({url, line, column }); > > Looks like there are code style changes in this one. Yes, and it's actually a very nice style change :) But I can remove it if you really don't like it.

Comment on attachment 8766256 [details] [diff] [review] Part 6: Use StackTrace React component in webconsole, cleanup message-location Review of attachment 8766256 [details] [diff] [review]: ----------------------------------------------------------------- This is changing too many things across different patches for me to feel confident in giving an r+. Since it has been manually tested and doesn't seem to break anything in a noticeable way, I'm fine with it landing. But I do not feel like I can confidently r+ it.

Comment on attachment 8764963 [details] [diff] [review] Part 7: Clean up and simplify markup of webconsole messages Review of attachment 8764963 [details] [diff] [review]: ----------------------------------------------------------------- Changing the review of this one to reflect the previous comment.

Comment on attachment 8766256 [details] [diff] [review] Part 6: Use StackTrace React component in webconsole, cleanup message-location Review of attachment 8766256 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/webconsole/webconsole.js @@ +2347,5 @@ > this.ReactDOM.unmountComponentAtNode(locationNode); > } > + > + // Unmount the StackTrace component if present in the message > + let stacktraceNode = node.querySelector(".stacktrace"); Does the stacktrace component include .message-location elements? if so, will it be a problem that the first one in that batch has already been unmounted on line 2347 above? If so, we should probably first unmount stacktraceNode before locationNode

(In reply to Brian Grinstead [:bgrins] from comment #60) > Does the stacktrace component include .message-location elements? if so, > will it be a problem that the first one in that batch has already been > unmounted on line 2347 above? If so, we should probably first unmount > stacktraceNode before locationNode No, it doesn't. message-location is now only the one-per-message container element where the location info is rendered. The StackTrace React component doesn't use this CSS class - it uses the "frame-link-*" CSS classes instead.

Comment on attachment 8766256 [details] [diff] [review] Part 6: Use StackTrace React component in webconsole, cleanup message-location Review of attachment 8766256 [details] [diff] [review]: ----------------------------------------------------------------- I already pretty much r+ed this already, now with the call to unmountComponent I'm happy with it ::: devtools/client/webconsole/webconsole.js @@ +2347,5 @@ > this.ReactDOM.unmountComponentAtNode(locationNode); > } > + > + // Unmount the StackTrace component if present in the message > + let stacktraceNode = node.querySelector(".stacktrace"); Is there any way to have more than one .stacktrace or .message-location (pretty sure not, but just want to double check)

(In reply to Brian Grinstead [:bgrins] from comment #62) > Is there any way to have more than one .stacktrace or .message-location > (pretty sure not, but just want to double check) The message packet always has only zero or one packet.stack or packet.filename/lineNumber properties, and there's no way how there could be multiple ones.

(In reply to Jarda Snajdr [:jsnajdr] from comment #57) > I think this change delivers a lot of value to console users, which they > wouldn't otherwise get for several more months until new-console-output > ships to production. And it also makes all location/stacktrace rendering > consistent across all devtools and reuse the same code. The intent on the new frontend is to use the exact same markup so that there aren't any css changes needed when switching to it

Comment on attachment 8764963 [details] [diff] [review] Part 7: Clean up and simplify markup of webconsole messages Review of attachment 8764963 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/webconsole/console-output.js @@ +1655,5 @@ > + * @private > + * @return DOMElement > + */ > + _renderStack: function () { > + return new Widgets.Stacktrace(this, this._stacktrace).render().element; This looks duplicated between the code above at line 1002. It appears this is a Messages.Simple so it seems that code would be making the stack for us instead of needing this _renderStack function, if this._stacktrace was called this.stack

Comment on attachment 8764963 [details] [diff] [review] Part 7: Clean up and simplify markup of webconsole messages Review of attachment 8764963 [details] [diff] [review]: ----------------------------------------------------------------- So, I'll say r+ keeping in mind Comment 65. Thanks for doing this work Jarda

(In reply to Brian Grinstead [:bgrins] from comment #65) > This looks duplicated between the code above at line 1002. It appears this > is a Messages.Simple so it seems that code would be making the stack for us > instead of needing this _renderStack function, if this._stacktrace was > called this.stack Yes, at the beginning, there is always one "stack" property in the protocol packet, but then there are actually two code paths that render the stack. One is for regular errors (nsIScriptError or console.error) in Messages.Simple, and then there is Messages.ConsoleTrace, which has its own rendering code. The difference is that Messages.Simple stacks are collapsible, while ConsoleTraces are not. But it's still duplicate code and should be solved better in new-console-output.

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/fx-team/rev/8a5b7ce6d2f9 Part 1: React components for displaying stack traces and frames r=linclark https://hg.mozilla.org/integration/fx-team/rev/fae095493197 Part 2: Fix tests that look at the Frame component markup r=linclark https://hg.mozilla.org/integration/fx-team/rev/7c355d1bfb1d Part 3: Remove duplicate styles for stack frames in Performance tool r=bgrins https://hg.mozilla.org/integration/fx-team/rev/1bb8b90bd13d Part 4: Add stack trace tab to HTTP console log message r=Honza https://hg.mozilla.org/integration/fx-team/rev/833efea0be66 Part 5: Style cleanup for HTTP console log messages r=Honza https://hg.mozilla.org/integration/fx-team/rev/9ce60fab89f5 Part 6: Use StackTrace React component in webconsole, cleanup message-location r=linclark https://hg.mozilla.org/integration/fx-team/rev/1feb7224925d Part 7: Clean up and simplify markup of webconsole messages r=bgrins https://hg.mozilla.org/integration/fx-team/rev/975d97ff77ae Part 8: Fix webconsole tests that inspect the stack frames r=bgrins

https://hg.mozilla.org/mozilla-central/rev/8a5b7ce6d2f9 https://hg.mozilla.org/mozilla-central/rev/fae095493197 https://hg.mozilla.org/mozilla-central/rev/7c355d1bfb1d https://hg.mozilla.org/mozilla-central/rev/1bb8b90bd13d https://hg.mozilla.org/mozilla-central/rev/833efea0be66 https://hg.mozilla.org/mozilla-central/rev/9ce60fab89f5 https://hg.mozilla.org/mozilla-central/rev/1feb7224925d https://hg.mozilla.org/mozilla-central/rev/975d97ff77ae

Filed an issue to track this work in the new console frontend: https://github.com/devtools-html/gecko-dev/issues/107

I've added a note here: https://developer.mozilla.org/en-US/docs/Tools/Web_Console/Console_messages#Viewing_network_request_details, let me know if you need anything else.

(In reply to Will Bamberg [:wbamberg] from comment #71) > I've added a note here: > https://developer.mozilla.org/en-US/docs/Tools/Web_Console/ > Console_messages#Viewing_network_request_details, let me know if you need > anything else. Looks good, thank you!