This is blocking Bug 670002 so marking as P1

This is for using our frame component at devtools/client/shared/components/frame.js to render the source locations in the webconsole, a first step to be able to have the frame components handle updating incase of source maps, rather than handling them in the current way of assembling the XUL DOM and manually updating.

Has patch 1251084 in here as well. everything looking good locally, try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e418f00e809

First go at this. Everything's working?! r? Nick for frame component/test changes -- this affects memory/jit coach views as well. r? Lin for the console components -- some things that I'm unsure about: * Am I missing any kind of cleanup for react? Not even unmounting the component, I was expecting leaks of some kind, but tests seem to be OK. * I removed "target" as an argument of createLocationNode, as it was only used in stack traces, I believe, and we should default to opening in debugger anyway, as it falls back to normal view-source. * Accessing React from the webconsole.js is weird -- there are a few other ways to do this, but all seem weird. Any idea? Helen, ui-r?, as this changes all source styling in the console, and also is a consistent representation for sources. Right now, other tools have red column/line numbers, but the console is all blue (this patch changes that). A few other things like how should we style the active state, maybe increase the width of the file name on console, etc., could definitely use some skilled eyes. I have no preference one way or another, but however this is styled, will affect memory/jit coach views, and eventually all tools for consistent styling, so it should look good in all places (unless there's a reason it should be different in different tools?) This is a weird patch, let me know any questions! https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b6a5b7f3090

Comment on attachment 8725501 [details] [diff] [review] 1251033-ssource-component.patch Review of attachment 8725501 [details] [diff] [review]: ----------------------------------------------------------------- LGTM! I only looked at the frame component related things. ::: devtools/client/shared/components/frame.js @@ +38,5 @@ > const { short, long, host } = getSourceNames(frame.source); > > + let tooltip = long; > + // Exclude all falsy values, including `0`, as even > + // a number 0 for line doesn't make sense, and should not be displayed. If we are assuming that lines are 1-based and not 0-based (I have this creeping feeling that they aren't consistent depending on where they're from...) then we should: * Document in propTypes that the line is expected to be 1-based * DTU.assert(line > 0) ::: devtools/client/shared/source-utils.js @@ +190,5 @@ > + location.charCodeAt(++i) === CHAR_CODE_H && > + location.charCodeAt(++i) === CHAR_CODE_P && > + location.charCodeAt(++i) === CHAR_CODE_A && > + location.charCodeAt(++i) === CHAR_CODE_D && > + location.charCodeAt(++i) === CHAR_CODE_SLASH; This pleases me greatly

Comment on attachment 8725501 [details] [diff] [review] 1251033-ssource-component.patch Review of attachment 8725501 [details] [diff] [review]: ----------------------------------------------------------------- > * Am I missing any kind of cleanup for react? Not even unmounting the > component, I was expecting leaks of some kind, but tests seem to be OK. To my knowledge, there will be leakage if you don't use unmountComponentAtNode(). What kind of test are you looking at? If it clearly shows that there is no memory leaking, then I might be wrong in this context. But if the test doesn't explicitly test for memory leaks, then I'd say we should add calls to unmountComponentAtNode. > * I removed "target" as an argument of createLocationNode, as it was only > used in stack traces, I believe, and we should default to opening in > debugger anyway, as it falls back to normal view-source. I noticed that. TBH, I don't know enough about the existing code to have an opinion on that. :bgrins might when he gets back. > * Accessing React from the webconsole.js is weird -- there are a few other > ways to do this, but all seem weird. Any idea? I asked :jryans. Looking at the code quickly, he thought that the BrowserLoader should have access to the window already and that copying it from this.window should be unnecessary. He's planning to take a look. --- I commented on a few things I noticed. All minor, none of these need to be changed for an r+. ::: devtools/client/shared/browser-loader.js @@ +88,5 @@ > const isBrowserDir = BROWSER_BASED_DIRS.filter(dir => { > return uri.startsWith(dir); > }).length > 0; > > + if (!isBrowserDir && (!uri.startsWith(baseURI) || baseURI == void 0)) { Out of curiosity, is there a reason to prefer void 0 over undefined here? ::: devtools/client/shared/components/frame.js @@ +71,5 @@ > + fields.push(dom.span({ className: "frame-link-column" }, frame.column)); > + attributes["data-column"] = frame.column; > + } > + > + attributes["data-line"] = frame.line; Are these data attributes addded for testing purposes only? If so, it might be worth adding a comment about that, but I leave that call up to you. I also wonder whether .frame-link-column/line could be reused here? It seems like they're used for testing. Didn't look too closely, though. @@ +82,5 @@ > if (showHost && host) { > fields.push(dom.span({ className: "frame-link-host" }, host)); > } > > + return dom.span(Object.assign({ I don't think that we need to use Object.assign here. If it were adding properties to the component's own props object and then passing that down to the component's child, then Object.assign would be needed. But here, I don't think it is necessary. I could be wrong, though. ::: devtools/client/webconsole/test/browser_webconsole_bug_587617_output_copy.js @@ +56,5 @@ > > // Remove new lines since getSelection() includes one between message and > // line number, but the clipboard doesn't (see bug 1119503) > let selection = (HUD.iframeWindow.getSelection() + "") > + .replace(/\r?\n|\r| /g, ""); Should maybe update the comment for the change. @@ +85,5 @@ > > // Remove new lines since getSelection() includes one between message and line > // number, but the clipboard doesn't (see bug 1119503) > let selection = (HUD.iframeWindow.getSelection() + "") > + .replace(/\r?\n|\r| /g, ""); See above. ::: devtools/client/webconsole/test/browser_webconsole_bug_613280_jsterm_copy.js @@ +70,5 @@ > > // Remove new lines since getSelection() includes one between message and line > // number, but the clipboard doesn't (see bug 1119503) > let selectionText = (HUD.iframeWindow.getSelection() + "") > + .replace(/\r?\n|\r| /g, ""); See above.

Adding needinfos: - :jryans, could you look at the BrowserLoader thing? - :bgrins, you should probably scan this. Particularly the part where the "target" parameter is removed from createLocationNode's signature

Comment on attachment 8725501 [details] [diff] [review] 1251033-ssource-component.patch Review of attachment 8725501 [details] [diff] [review]: ----------------------------------------------------------------- Setting f- for the way BrowserLoader is used. If you aren't able to use one of the methods I've suggested, please let me know! ::: devtools/client/shared/browser-loader.js @@ +48,5 @@ > * An object with two properties: > * - loader: the Loader instance > * - require: a function to require modules with > */ > function BrowserLoader(baseURI, window) { Maybe better to take an options arg, instead of a strange null first param? @@ +88,5 @@ > const isBrowserDir = BROWSER_BASED_DIRS.filter(dir => { > return uri.startsWith(dir); > }).length > 0; > > + if (!isBrowserDir && (!uri.startsWith(baseURI) || baseURI == void 0)) { Seems like `!baseURI` would be fine? ::: devtools/client/webconsole/webconsole.js @@ +231,5 @@ > > this._outputTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer); > this._outputTimerInitialized = false; > > + // We load React in the document context since it needs `window`, This feels kind of hacky and strange to me. The BrowserLoader's main purpose is to give vendor libs like React and our shared components access to the window. Do you have to load React as a <script> tag for some reason? What about this instead: 1. Remove the React <script> tags 2. Use the following in the file header: const BrowserLoaderModule = {}; Cu.import("resource://devtools/client/shared/browser-loader.js", BrowserLoaderModule); // Can keep passing "null" if you really don't want console modules to have window access. // Also, should be fine to let BrowserLoader's require replace the original require in this file. // It delegates to the usual one outside of the special browser directories. const { require } = BrowserLoaderModule.BrowserLoader("resource://devtools/client/webconsole/", this); const { createFactory } = require("devtools/client/shared/vendor/react"); const FrameView = createFactory(require("devtools/client/shared/components/frame"); This makes it much more like memory and other tools using BrowserLoader. You could even go further, if you want: 1. Create an initializer.js file for console with: const BrowserLoaderModule = {}; Cu.import("resource://devtools/client/shared/browser-loader.js", BrowserLoaderModule); const { require } = BrowserLoaderModule.BrowserLoader("resource://devtools/client/webconsole/", this); require("devtools/client/webconsole/webconsole.js"); 2. Remove React and webconsole.js script tags, add initializer.js script tag 3. Then require() in webconsole.js is already prepared to load modules that need window.

Comment on attachment 8725501 [details] [diff] [review] 1251033-ssource-component.patch Review of attachment 8725501 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/shared/browser-loader.js @@ +88,5 @@ > const isBrowserDir = BROWSER_BASED_DIRS.filter(dir => { > return uri.startsWith(dir); > }).length > 0; > > + if (!isBrowserDir && (!uri.startsWith(baseURI) || baseURI == void 0)) { `!baseURI` matches `""` and is different semantically than `null` -- meaning if I passed in `""`, I may expect EVERYTHING to get loaded BrowserLoader, as all paths technically start with an empty string. Either way, I agree this is ugly. I think a { onlySharedDirectories: true } option is probably the best, and most understandable, bet here. ::: devtools/client/shared/components/frame.js @@ +38,5 @@ > const { short, long, host } = getSourceNames(frame.source); > > + let tooltip = long; > + // Exclude all falsy values, including `0`, as even > + // a number 0 for line doesn't make sense, and should not be displayed. We talked about this in IRC briefly, but: Some valid sources have 0 line and column numbers (I've seen this mostly for inline sources, like index.html:0:0), so to prevent every consumer of this module to implement their own logic, this component handles just not rendering it. Other source inconsistencies, I'm not sure of, but I suspect those will shake out.. @@ +71,5 @@ > + fields.push(dom.span({ className: "frame-link-column" }, frame.column)); > + attributes["data-column"] = frame.column; > + } > + > + attributes["data-line"] = frame.line; Currently vestigial (and testing) now that everything is handled in this component, can't think of other scenarios that'd need this. Commenting! @@ +82,5 @@ > if (showHost && host) { > fields.push(dom.span({ className: "frame-link-host" }, host)); > } > > + return dom.span(Object.assign({ You're right, not necessary -- can just add them in the `attributes` constructor ::: devtools/client/webconsole/test/browser_webconsole_bug_587617_output_copy.js @@ +85,5 @@ > > // Remove new lines since getSelection() includes one between message and line > // number, but the clipboard doesn't (see bug 1119503) > let selection = (HUD.iframeWindow.getSelection() + "") > + .replace(/\r?\n|\r| /g, ""); +1 ::: devtools/client/webconsole/webconsole.js @@ +231,5 @@ > > this._outputTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer); > this._outputTimerInitialized = false; > > + // We load React in the document context since it needs `window`, webconsole.js is loaded via panel.js and only gets a window reference upon instantiation (so can do option 1 at this point), and changing that instantiation process, or load it via BL (option 2) seems that it'd be a lot of rewriting tests for the (probable) chance of it not working in a new context without some overhaul (maybe I'm being overly cautious, but this our oldest tool, and my most unfamiliar). Maybe a good compromise will be what's here currently, except the BL loads React/ReactDOM in webconsole.js's instantiation, instead of referencing `window.React`?

(In reply to Jordan Santell [:jsantell] [@jsantell] (Please needinfo) from comment #9) > ::: devtools/client/webconsole/webconsole.js > @@ +231,5 @@ > > > > this._outputTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer); > > this._outputTimerInitialized = false; > > > > + // We load React in the document context since it needs `window`, > > webconsole.js is loaded via panel.js and only gets a window reference upon > instantiation (so can do option 1 at this point), and changing that > instantiation process, or load it via BL (option 2) seems that it'd be a lot > of rewriting tests for the (probable) chance of it not working in a new > context without some overhaul (maybe I'm being overly cautious, but this our > oldest tool, and my most unfamiliar). > > Maybe a good compromise will be what's here currently, except the BL loads > React/ReactDOM in webconsole.js's instantiation, instead of referencing > `window.React`? Ah, I had overlooked that webconsole.js is currently a module, not a script on the page like (most?) other tools. That explains the strangeness. Yes, let's go with your compromise and at least load React via BL.

Addressed all comments! https://treeherder.mozilla.org/#/jobs?repo=try&revision=b75d1820815c

Comment on attachment 8726460 [details] [diff] [review] 1251033-ssource-component.patch Review of attachment 8726460 [details] [diff] [review]: ----------------------------------------------------------------- Loader bits look good to me. ::: devtools/client/webconsole/webconsole.js @@ +231,5 @@ > > this._outputTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer); > this._outputTimerInitialized = false; > > + this._bRequire = BrowserLoaderModule.BrowserLoader({ Seems like you only need this as a local var, it's unused outside this function. Maybe do: let { require: bRequire } = BrowserLoaderModule.BrowserLoader({ window: this.window, useOnlyShared: true }); ...or just shadow the outer require depending on how strange that seems.

Comment on attachment 8726460 [details] [diff] [review] 1251033-ssource-component.patch Review of attachment 8726460 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/shared/test/unit/test_source-utils.js @@ +78,5 @@ > + "example.com"); > +}); > + > +// Test `sourceUtils.getSourceNames`. > +add_task(function* () { Added test per bug 1251084's feedback (fixed, although not in the latest try push) ::: devtools/client/webconsole/webconsole.js @@ +231,5 @@ > > this._outputTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer); > this._outputTimerInitialized = false; > > + this._bRequire = BrowserLoaderModule.BrowserLoader({ Good call!

Via quick chat with Lin on IRC: the latest patch still doesn't handle unmounting of the react components when a line is cleared. It atleast doesn't cause the window to leak, but not sure if we check when we clear the console, or messages are cleared after X lines

Comment on attachment 8726460 [details] [diff] [review] 1251033-ssource-component.patch Review of attachment 8726460 [details] [diff] [review]: ----------------------------------------------------------------- I'm going to clear out the review request on this until there's an update on the memory issue. Feel free to r? me again then.

So unmounting react components in the webconsole, only was able to test leaking by adding a mount/unmount method on the frame component. Shook up some scenarios, due to creating the location node before we render in the DOM (stored in a queue), and the many points along its lifetime that it could be removed (different ways for different points, unfortunately, and I don't want to change the main logic for this): * When messages are flushed (log limit, clear) * When a duplicate message comes in, and just add the (2) bubble, we need to clean up the message as it's just discarded and never rendered * When we shut down the console, we don't clean up the messages currently (as no clean up is necessary), so that's another case. Not sure a good way to test this, other than in DevToolsUtils.testing, we could add an observer to the mount/unmount of the component to hold on since we don't have any methods for that currently. Or if we're not using those methods currently, maybe not a big deal? Do we still have to unmount if no component clean up necessary, or do we still need to clean it up within React? https://treeherder.mozilla.org/#/jobs?repo=try&revision=54a65b951d5f

Comment on attachment 8727687 [details] [diff] [review] 1251033-ssource-component.patch Review of attachment 8727687 [details] [diff] [review]: ----------------------------------------------------------------- Just a note, the only new changes here are all in webconsole.js

https://hg.mozilla.org/integration/fx-team/rev/10ecf21c6b48 https://hg.mozilla.org/integration/fx-team/rev/eb9b84702b2c

Landed the reviewed frame component and browser loader changes -- updating the patch with remaining pieces for review

Previously reviewed components in browser loader and frame component pulled out into separate patch and landed; remaining webconsole bits in this patch

https://hg.mozilla.org/mozilla-central/rev/10ecf21c6b48 https://hg.mozilla.org/mozilla-central/rev/eb9b84702b2c https://hg.mozilla.org/mozilla-central/rev/865dec79d8c1

Reopening, still has one patch to go

(In reply to Lin Clark [:linclark] from comment #7) > - :bgrins, you should probably scan this. Particularly the part where the > "target" parameter is removed from createLocationNode's signature That part looks fine to me

Follow-up material, but is there a reason why only the filename portion of the source link is clickable? I'd expect clicking on the line/col # to also navigate me (esp since the cursor is pointer and there's some link-like styling on it).

No historical reason; just the styling from the profiler was the baseline. Personally, I like the red column/line numbers, but it looks strange being underlined blue via the file name. Since we just uplifted, and Helen's out this week, and next for work week, I think styling this frame component should be handled in a follow up where we can hash out all the styling for this, as this will get used in a lot of places (unless we have a clear idea how it should be styled now)

(In reply to Jordan Santell [:jsantell] [@jsantell] (Please needinfo) from comment #29) > No historical reason; just the styling from the profiler was the baseline. > Personally, I like the red column/line numbers, but it looks strange being > underlined blue via the file name. Since we just uplifted, and Helen's out > this week, and next for work week, I think styling this frame component > should be handled in a follow up where we can hash out all the styling for > this, as this will get used in a lot of places (unless we have a clear idea > how it should be styled now) I'm fine with the styling in general (although the underline is a bit off), but I do think that clicking on anything in that area should navigate to the proper place. Esp since the filename gets cut off, when it's a big line / column number the clickable area becomes way small.

Would you be alright with having those style changes in a following bug where a few style issues remain for it? Are there any other things that can cause concern in this patch?

(In reply to Jordan Santell [:jsantell] [@jsantell] (Please needinfo) from comment #31) > Would you be alright with having those style changes in a following bug > where a few style issues remain for it? Are there any other things that can > cause concern in this patch? To be clear, I don't care about the style changes. I care that clicking on the line number / column number navigates me to the debugger / view source / etc. This is a UX regression compared to the current source links in the console, where the entire filename:line:col area is clickable. But, it's fine to do that in a follow-up as long as it's something that can reasonably ride the train in 48.

Comment on attachment 8728293 [details] [diff] [review] 1251033-ssource-component.patch Review of attachment 8728293 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for adding the unmount handling. I didn't get a chance to manually test the latest patch, but I believe Brian did. r+ if try is green.

Thanks Lin & Brian! I filed a follow up for the click styling and I did a manuall test of the unmounting, and seems hard to do outside of that for these one-off renderings that don't actually use an unmount behavior, but Lin and I discussed some ways to test this when we have a react rendering root in the console

Backed out for browser_scratchpad_wrong_window_focus.js permafail. https://hg.mozilla.org/integration/fx-team/rev/9f8996d35a3a https://treeherder.mozilla.org/logviewer.html#?job_id=7945025&repo=fx-team TEST-UNEXPECTED-FAIL | devtools/client/scratchpad/test/browser_scratchpad_wrong_window_focus.js | location value is correct - Got Scratchpad/50, expected Scratchpad/50:1:1

And https://treeherder.mozilla.org/logviewer.html#?job_id=7947839&repo=fx-team

Got em got em https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ddc056285ef