Last Comment Bug 670002 - Use source maps in the web console
: Use source maps in the web console
Status: NEW
[polish-backlog][difficulty=hard][gam...
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Console (show other bugs)
: unspecified
: All All
: P1 normal with 55 votes (vote)
: ---
Assigned To: Jaideep Bhoosreddy [:jbhoosreddy]
:
Mentors:
: 934151 934701 996063 1221005 1239681 (view as bug list)
Depends on: 669999 672829 674283 676281 679165 679181 1130214 1177279 1179823 1251033
Blocks: 952127 1111089 1179813 618650
  Show dependency treegraph
 
Reported: 2011-07-07 14:11 PDT by Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
Modified: 2016-06-27 12:21 PDT (History)
68 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Source map integration w/ the webconsole V1 (17.80 KB, patch)
2011-08-08 19:44 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
no flags Details | Diff | Review
Source map integration w/ the webconsole V2 (16.10 KB, patch)
2011-08-09 10:45 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
bugzilla: feedback+
Details | Diff | Review
Source map integration w/ the webconsole V3 (26.72 KB, patch)
2011-08-31 19:37 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
no flags Details | Diff | Review
670002.patch [WIP] (17.18 KB, patch)
2016-06-23 14:56 PDT, Jaideep Bhoosreddy [:jbhoosreddy]
no flags Details | Diff | Review

Description Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2011-07-07 14:11:12 PDT
The lines (and eventually columns) in logged messages and uncaught errors should map back to the original source if a source map is present for the script which logged the message or where the error originated.
Comment 1 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2011-08-08 19:44:56 PDT
Created attachment 551663 [details] [diff] [review]
Source map integration w/ the webconsole V1

This is not ready for official review because the patches that it depends on have not landed yet, and so some things aren't available unless you apply patches manually, or are stubbed out. That is why I am just asking for feedback.
Comment 2 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2011-08-08 19:53:04 PDT
Disregard the SourceMapConsumer.jsm stuff, that belongs in bug669999. Sorry.
Comment 3 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2011-08-09 10:40:16 PDT
I just realized that it is not very clear how to structure your patch queue so that this stuff will apply in a clean manner and actually work. Here is my .hg/patch/series:

  bug-665167-part-1.patch
  bug-665167-part-2.patch
  bug-665167-part-3.patch
  bug-665167-part-4.patch
  bug-665167-part-5.patch
  bug-676281-add-getAllScripts.patch
  bug-669999-add-core-source-map-lib.patch
  bug-670002-source-map-integration-with-webconsole.patch

I will update the patch in a minute to remove the changes which belong in bug669999.
Comment 4 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2011-08-09 10:45:21 PDT
Created attachment 551821 [details] [diff] [review]
Source map integration w/ the webconsole V2

Moved the changes to SourceMapConsumer.jsm out of this patch and in to the patch for bug669999.
Comment 5 David Dahl :ddahl 2011-08-11 13:10:35 PDT
Comment on attachment 551821 [details] [diff] [review]
Source map integration w/ the webconsole V2

># HG changeset patch
># Parent 8fb6aa825ec9a9282e04f193ce0864b8b29bb93a
># User Nick Fitzgerald <fitzgen@gmail.com>
>
>diff --git a/browser/devtools/sourcemap/Makefile.in b/browser/devtools/sourcemap/Makefile.in
>--- a/browser/devtools/sourcemap/Makefile.in
>+++ b/browser/devtools/sourcemap/Makefile.in
>@@ -45,6 +45,7 @@
> include $(DEPTH)/config/autoconf.mk
>
> EXTRA_JS_MODULES = SourceMapConsumer.jsm \
>+		SourceMapUtils.jsm \
> 		$(NULL)
>

I know it seems like a picky thing, but you should probably consolidate all sourceMap JSMs.

> ifdef ENABLE_TESTS
>diff --git a/browser/devtools/sourcemap/SourceMapUtils.jsm b/browser/devtools/sourcemap/SourceMapUtils.jsm
>new file mode 100644
...
>+function constantProperty(aValue) {
>+  return {
>+    value: aValue,
>+    writable: false,
>+    enumerable: true,
>+    configurable: false
>+  };
>+}
>+
Nice. Too much boilerplate with gecko, no?

>+const ERRORS = Object.create(null, {
>+  NO_SOURCE_MAP: constantProperty(1),
>+  BAD_SOURCE_MAP: constantProperty(2),
>+  UNKNOWN_ERROR: constantProperty(3)
>+});
>+
>+

>+function sourceMapURLForFilename(aFilename, aWindow) {
>+  // XXX: Stubbed out for now. Depends on jsdbg2 stuff.
>+  //
>+  // let dbg = new Debugger(aWindow);
>+  // let arr = dbg.getAllScripts();
>+  // let len = arr.length;
>+  // for (var i = 0; i < len; i++) {
>+  //   if (arr[i].filename == aFilename) {
>+  //     return arr[i].sourceMappingURL;
>+  //   }
>+  // }
>+  // return null;
>+  return aFilename + '.map';
>+}
Better to use "// TODO:  blah foo see bug 123456" - file a followup bug even if this is a tiny thing, and make it blocked by the landing (unless this is really imminent)

>+
>+/**
>+ * Fetch the source map associated with the given filename.
>+ *
>+ * @param aFilename The js file you want to fetch the associated source map for.
>+ * @param aWindow The currently active window.
>+ * @param aSuccessCallback The function to call when we find a source map.
>+ * @param aErrorCallback The function to call when there is no source map.
>+ */
>+function sourceMapForFilename(aFilename, aWindow, aSuccessCallback, aErrorCallback) {
>+  aErrorCallback = aErrorCallback || noop;
>+  let sourceMapURL = sourceMapURLForFilename(aFilename, aWindow);
>+  if (sourceMapURL) {
>+    try {
>+      xhrGet(sourceMapURL, 'text/json', function (xhr) {
>+        let sourceMap;
>+        try {
>+          sourceMap = new SourceMapConsumer(JSON.parse(xhr.responseText));
>+        } catch (e) {

nit: catch on newline - i know... its 'just feedback':)

>diff --git a/browser/devtools/webconsole/HUDService.jsm b/browser/devtools/webconsole/HUDService.jsm
>--- a/browser/devtools/webconsole/HUDService.jsm
>+++ b/browser/devtools/webconsole/HUDService.jsm
>@@ -1,4 +1,4 @@
>-/* -*- Mode: js2; js2-basic-offset: 2; indent-tabs-mode: nil; -*- */
>+* -*- Mode: js2; js2-basic-offset: 2; indent-tabs-mode: nil; -*- */
> /* vim: set ft=javascript ts=2 et sw=2 tw=80: */
> /* ***** BEGIN LICENSE BLOCK *****
>  * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>@@ -71,6 +71,10 @@
...
>
>+XPCOMUtils.defineLazyServiceGetter(this, "IOService",
>+                                   "@mozilla.org/network/io-service;1",
>+                                   "nsIIOService");
>+

"io" is a property of "Services": http://mxr.mozilla.org/mozilla-central/source/toolkit/content/Services.jsm#80

>   /**
>+   * The panel which holds the original and generated location info for the
>+   * currently focused console message. It is created the first time there is a
>+   * console message which has source mapping info.
>+   */
>+  locationContainerPanel: null,
>+
>+  /**
>+   * Returns the left and top offset of a node.
>+   */
Document the args here

>+  elementOffset:
>+  function ConsoleUtils_elementOffset(aNode, aLeft, aTop) {
>+    let elemLeft = typeof aLeft === "number" ? aLeft : 0;
>+    let elemTop = typeof aTop === "number" ? aTop : 0;
>+    if (!aNode) {
>+      return {
>+        left: elemLeft,
>+        top: elemTop
>+      };

Why do you return left: 0 top: 0 if this is node is undefined or null?

>+    }
>+    let { left, top } = aNode.getBoundingClientRect();
not sure if the reviewer will like this jetpack-style syntax

>+    return this.elementOffset(aNode.offsetParent,
>+                              elemLeft + left,
>+                              elemTop + top);
>+  },
>+
>+  /**
>+   * Returns true/false on whether or not the mouseout handler for the
>+   * locationContainerPanel should fire.
>+   */
proper javadoc arguments documentation please

>+  /**
>+   * Asynchronously check if there is a source map for this message's
>+   * script. If there is, update the locationNode and clipboardText.
>+   *
>+   * TODO: Use column numbers once bug 568142 is fixed.
>+   * TODO: Don't require aSourceURL once bug 676281 is fixed, just a way to
>+   *       get a script.
>+   */
Do you need new bugs filed for these TODOs?



Do you have tests for this? I hear your patch queue is insane, do you have a try build with all of this working?

Looks good so far, please conform to existing styles for bracing catch(es) and if/else
Comment 6 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2011-08-15 14:46:23 PDT
(In reply to David Dahl :ddahl from comment #5)
> I know it seems like a picky thing, but you should probably consolidate all
> sourceMap JSMs.

I would, but the reason I haven't is because SourceMapConsumer.jsm is dynamically built from the contents of https://github.com/mozilla/source-map

The reason being that third parties (CoffeeScript/Minifiers/etc) will be using this library as well, and so it stands by itself.

> >+  elementOffset:
> >+  function ConsoleUtils_elementOffset(aNode, aLeft, aTop) {
> >+    let elemLeft = typeof aLeft === "number" ? aLeft : 0;
> >+    let elemTop = typeof aTop === "number" ? aTop : 0;
> >+    if (!aNode) {
> >+      return {
> >+        left: elemLeft,
> >+        top: elemTop
> >+      };
> 
> Why do you return left: 0 top: 0 if this is node is undefined or null?

This should be more clear once I document this function better, but the idea is that you only pass the function aNode, and then when it recursively calls itself it is using aLeft and aTop as accumulators for the left and top offsets. The recursion stops when aNode is null, meaning that there are no more offsetParents and we have reached the top and accumulated all offsets along the way.

The function is basically answering the question "what is the offset of a node?" with "it is its offset from its parent plus its parent's offset". When there is no more parent, there is no more offset, hence the zeros.

> >+    let { left, top } = aNode.getBoundingClientRect();
> not sure if the reviewer will like this jetpack-style syntax

I thought one the benefits of working on JavaScript code (as opposed to ECMAScript) was that we got to use all of our cool extensions? It sure as heck looks a lot better than

  let temp = aNode.getBoundingClientRect();
  let left = temp.left;
  let top = temp.top;

:(

> >+  /**
> >+   * Asynchronously check if there is a source map for this message's
> >+   * script. If there is, update the locationNode and clipboardText.
> >+   *
> >+   * TODO: Use column numbers once bug 568142 is fixed.
> >+   * TODO: Don't require aSourceURL once bug 676281 is fixed, just a way to
> >+   *       get a script.
> >+   */
> Do you need new bugs filed for these TODOs?

Will do.

> Do you have tests for this? I hear your patch queue is insane, do you have a
> try build with all of this working?

No, I need to write tests, I have only been testing by hand myself. I have been putting it off because I don't know our testing framework(s) that well.
Comment 7 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2011-08-29 18:14:53 PDT
For posterity, and because the patch queue is intense, I created a user repo to hold my patch queue for this bug: http://hg.mozilla.org/users/nfitzgerald_mozilla.com/source-map-webconsole-patches
Comment 8 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2011-08-31 19:37:16 PDT
Created attachment 557396 [details] [diff] [review]
Source map integration w/ the webconsole V3

Updated version of the patch. Adds tests. All the logical tests pass, however I am seeing some memory leaks:

  TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 2133603 bytes during test execution
  TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1437 instances of AtomImpl with size 40 bytes each (57480 bytes total)
  TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of BackstagePass with size 48 bytes
  TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of DR_State with size 56 bytes
  TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of DocumentRule with size 72 bytes
  TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of GenericFactory with size 32 bytes

Not sure if these are from the tests or the codes.
Comment 9 Steve Roussey (:sroussey) 2011-11-20 11:14:17 PST
By the way, can we get source mappings for things besides JS? SASS -> CSS comes to mind. Even minified HTML.
Comment 10 Kevin Dangoor 2011-11-21 10:51:07 PST
Yep, makes sense. I've created a feature page for that:

https://wiki.mozilla.org/DevTools/Features/CSSSourceMap
Comment 11 Panos Astithas [:past] 2012-01-17 08:37:22 PST
Bug triage, filter on PEGASUS.
Comment 12 Paul Rouget [:paul] 2012-12-05 03:55:06 PST
triage. Filter on PINKISBEAUTIFUL
Comment 13 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2013-08-19 12:26:38 PDT
Not actively working on this anytime soon, so I'm unassigning myself so someone else can take it.
Comment 14 Panos Astithas [:past] 2013-11-05 00:30:31 PST
*** Bug 934701 has been marked as a duplicate of this bug. ***
Comment 15 Mihai Sucan [:msucan] 2013-11-07 07:58:36 PST
Nick, could you please summarize the work needed for fixing this bug? Is there some API in the debugger's actors I can call and give it an URL and line number to get the source-mapped URL and line number? I'm trying to understand the amount of work that would be needed.
Comment 16 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2013-11-07 10:07:22 PST
(In reply to Mihai Sucan [:msucan] from comment #15)
> Nick, could you please summarize the work needed for fixing this bug? Is
> there some API in the debugger's actors I can call and give it an URL and
> line number to get the source-mapped URL and line number? I'm trying to
> understand the amount of work that would be needed.

The relevant code is the ThreadSources class in the script actors.

However, a source map's URL is only ever exposed to JS through |Debugger.Script|, which means that currently you have to force debug mode to get at it. We can't do that in the web console.

We need some platform work. I see two options:

1) Add |sourceMapURL| properties to nsIScriptError(2) and friends. Basically every single thing that reports a message/warning/error and has a JS url would have to also include a source map url. This seems like a lot of work, and easy to miss a couple messages, or forget to add it to new message types, etc etc.

2) Have SpiderMonkey maintain a fixed size map of script url -> source map url. Fixed size so that memory doesn't just keep growing infinitely. We can replace the least recently seen script whenever we get a new one, and only put scripts in the map if they do have a source map url. If we can expose access to this map as some kind of method like |getSourceMapURL(scriptURL)|, we have everything we need. I talked to :ejpbruel (cc'ing) about this approach and he was optimistic.

I think we should look into option 2.

Once we have that, it is easy to fetch the source map and update the links in the webconsole once you have better source location information from the source map.
Comment 17 Jukka Jylänki 2013-11-08 10:51:53 PST
*** Bug 934151 has been marked as a duplicate of this bug. ***
Comment 18 Mihai Sucan [:msucan] 2014-04-14 10:11:09 PDT
*** Bug 996063 has been marked as a duplicate of this bug. ***
Comment 19 Florian Bender 2014-11-22 06:49:50 PST
Is this bug still valid?
Comment 20 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2014-12-04 17:02:57 PST
(In reply to Florian Bender from comment #19)
> Is this bug still valid?

Yes
Comment 21 Kumar McMillan [:kumar] (needinfo all the things) 2014-12-09 08:16:45 PST
It would be really helpful to get this feature in WebIDE (and device logcat) for B2G app development. Unlike regular websites, there is no alternative (e.g. Chrome) so debugging minified code is hard.
Comment 22 David Bruant 2015-01-21 05:15:26 PST
Is bug 905700 a dependency of this bug?
Comment 23 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2015-01-21 08:48:25 PST
I don't think so.
Comment 24 James Long (:jlongster) 2015-02-05 13:24:19 PST
I made it a Q1 goal to start work on this. It would make working with scripts with sourcemaps so much easier.
Comment 25 Jim Blandy :jimb 2015-02-05 16:25:44 PST
I've added bug 1130214 as a blocker for this, so that we can use Debugger to access source map metadata in lightweight tools like the console, without inhibiting asm.js optimizations.
Comment 26 Jeff Griffiths (:canuckistani) (:⚡︎) 2015-06-02 16:03:02 PDT
Bumping priority: https://news.ycombinator.com/item?id=9649498
Comment 27 Alexandre Poirot [:ochameau] 2015-10-29 07:21:37 PDT
Just got pinged about the status of this. I imagine this is waiting for bug 1177279?
Comment 28 James Long (:jlongster) 2015-10-29 07:43:17 PDT
(In reply to Alexandre Poirot [:ochameau] from comment #27)
> Just got pinged about the status of this. I imagine this is waiting for bug
> 1177279?

Yes, but more importantly everything is blocked on this piece of architecture: bug 1132501. We are figuring out ways to share scripts across all of our tools and I think we are almost there. We are waiting to land that bug after this week's uplift and then we can do a lot more.
Comment 29 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2015-11-05 08:17:22 PST
*** Bug 1221005 has been marked as a duplicate of this bug. ***
Comment 30 flying sheep 2015-12-18 08:33:46 PST
eh, didn’t that work at one point?

anyway: any progress?
Comment 31 James Long (:jlongster) 2015-12-18 08:47:46 PST
(In reply to flying sheep from comment #30)
> eh, didn’t that work at one point?
> 
> anyway: any progress?

This has never been implemented. And yes, we are slowly making progress on bug 1132501, just need to fix some obscure test failures for it to land. After that we can implement this without much work.
Comment 32 flying sheep 2015-12-18 09:41:55 PST
happy to hear that :D
Comment 33 James Long (:jlongster) 2016-01-14 07:16:51 PST
*** Bug 1239681 has been marked as a duplicate of this bug. ***
Comment 34 Jordan Santell [:jsantell] [@jsantell] (Away from Bugzilla for awhile) 2016-02-24 13:31:21 PST
Starting to work on this in bug 1177279
Comment 35 Jordan Santell [:jsantell] [@jsantell] (Away from Bugzilla for awhile) 2016-04-05 11:45:06 PDT
Unassigning myself from bugs I won't be able to get to due to other commitments.

Let me know if you have questions about this one though.
Comment 36 J. Ryan Stinnett [:jryans] (use ni?) 2016-05-02 10:32:33 PDT
Joe, is there someone else that could take this on?  I think most of us agree this is a pretty critical feature in today's source map heavy world, so I don't want it to get lost.
Comment 37 Joe Walker [:jwalker] (needinfo me or ping on irc) 2016-05-05 01:56:00 PDT
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #36)
> Joe, is there someone else that could take this on?  I think most of us
> agree this is a pretty critical feature in today's source map heavy world,
> so I don't want it to get lost.

Not forgotten. Thanks for the ping.
Comment 38 Jordan Santell [:jsantell] [@jsantell] (Away from Bugzilla for awhile) 2016-05-10 14:38:00 PDT
So where this was left off:

The webconsole is now using the frame component for its sources (bug 1251033) and we have a SourceLocationController from bug 1177279, which fires callbacks when a source its aware of changes or is updated. In hindsight, I don't think this API is the best, but it will work, but should be straight forward to change this to an EventEmitter-based API. Each Frame component can take the SLC singleton (should probably live on target/toolbox or something and used across all tools) and listen to changes for the source the Frame component is listening to. That way, each frame component is the only component responsible for updating the display.

Whoever works on this, feel free to ping me and we can set up a meeting or chat if it'll help!
Comment 39 James Long (:jlongster) 2016-05-10 14:52:11 PDT
FWIW, I will definitely pick this up later this year if it hasn't been by then.
Comment 40 James Long (:jlongster) 2016-06-16 10:12:23 PDT
Jaideep expressed interest during the work week in working on this, which is great! Jordan and I can mentor him on this. Let us know if you have any problems getting started (we already gave him some notes).
Comment 41 Jordan Santell [:jsantell] [@jsantell] (Away from Bugzilla for awhile) 2016-06-19 13:42:35 PDT
To summarize what we talked about in terms of what are the last steps to get a basic version of this working:

* Create a SLC attached to the toolbox
* Pass in the SLC when instantiating FrameViews
* FrameView should bind via `bindLocation` on the SLC and update its line/column/url when it gets an update event
* Possibly ensure that only the webconsole does this, not the browser toolbox console, or browser console
* Should probably put this behind a preference flag for now until we handle a few of the follow ups (I think James sent an email? caching, batching, etc)
Comment 42 Jaideep Bhoosreddy [:jbhoosreddy] 2016-06-23 14:56:10 PDT
Created attachment 8764745 [details] [diff] [review]
670002.patch [WIP]

So, with Jordan's help, I was able to some progress. It does work if the sourcemap is already loaded before, but it does not seem to update the url, line, if the sourcemap is loaded afterwards. I'm not sure why that is happening, maybe James would have some ideas about that.

But in terms of the FrameView, I've updated the url, line, column location, and modified the onClick callback to point to the "new" source.

One more thing is that I point all other sources to jsdebugger, maybe there's a better way to select which sources should be pointed to it.
Comment 43 Jordan Santell [:jsantell] [@jsantell] (Away from Bugzilla for awhile) 2016-06-23 15:34:18 PDT
Comment on attachment 8764745 [details] [diff] [review]
670002.patch [WIP]

Review of attachment 8764745 [details] [diff] [review]:
-----------------------------------------------------------------

Some initial comments!

::: devtools/client/framework/source-location.js
@@ +119,5 @@
>    });
>  }
>  
>  /**
> + * Takes a serializedisSourceRelated SourceActor form and returns a boolean indicating

looks like a find and replace typo

::: devtools/client/shared/components/frame.js
@@ +27,5 @@
>      showFunctionName: PropTypes.bool,
>      // Option to display a host name after the source link.
>      showHost: PropTypes.bool,
> +    sourceLocationController: PropTypes.object,
> +    isSourceMapped: PropTypes.bool,

isSourceMapped is a state, not a prop

@@ +34,5 @@
>    getDefaultProps() {
>      return {
>        showFunctionName: false,
>        showHost: false,
> +      isSourceMapped: false,

isSourceMapped is a state, not a prop

@@ +41,5 @@
>  
> +  componentWillMount: function () {
> +    dump("WillMountComponent");
> +    const { frame } = this.props;
> +    this.setState({frame});

maybe when we store the state, instead of using the 'frame' object, convert it into a 'location' object (url/line/column), so we don't have to convert it back and forth elsewhere.

So state would be:
{
  location: { url, line, column },
  functionDisplayName,
  isSourceMapped
}

@@ +79,5 @@
>      // Exclude all falsy values, including `0`, as even
>      // a number 0 for line doesn't make sense, and should not be displayed.
>      // If source isn't linkable, don't attempt to append line and column
>      // info, as this probably doesn't make sense.
> +    if ((isLinkable || isSourceMapped) && line) {

I'd add a comment here explaining why we also check for isSourceMapped since those names are not necessarily URLs that are linkable, but still a valid source in the debugger

@@ +103,5 @@
> +    // Since SourceMapped URLs might not be parsed properly by parseURL
> +    // check for "/" in short. If "/" in short, take everything after last "/".
> +    if (isSourceMapped) {
> +      short = short.lastIndexOf("/") < 0 ?
> +          short : short.slice(short.lastIndexOf("/") + 1);

I wonder if there's a better parsing utility (like getSourceNames in source-utils) or something that's better able to handle this case of finding a good name via "short"

@@ +135,4 @@
>        sourceEl = dom.a({
>          onClick: e => {
>            e.preventDefault();
> +          onClick({fullURL: source, line});

We shouldn't use webconsole's naming here, we should pass in the same info as the SLC uses for "location": url, line and column, and webconsole should use that naming

::: devtools/client/webconsole/webconsole.js
@@ +2627,5 @@
>          target = "jsdebugger";
> +      } else {
> +        //Temporary hack to point all other sources to jsdebugger.
> +        // TODO: Find a better way to point scripts (with other .*) to jsdebugger.
> +        target = "jsdebugger";

We should just default to jsdebugger -- if the debugger can't open it because we don't have a source for it, then it'll fall back to normal 'view source' anyway
Comment 44 Jordan Santell [:jsantell] [@jsantell] (Away from Bugzilla for awhile) 2016-06-23 15:40:58 PDT
So James, the issue is when opening the console with this patch, we immediately call resolveLocation when we print the source, and get an error that the source does not exist. It'll work on refresh however.

If we, on a fresh page, start with the debugger open first, it works however. Any ideas? I thought the debugger always had sources now
Comment 45 James Long (:jlongster) 2016-06-27 09:27:51 PDT
(In reply to Jordan Santell [:jsantell] [@jsantell] (Away from Bugzilla for awhile) from comment #44)
> So James, the issue is when opening the console with this patch, we
> immediately call resolveLocation when we print the source, and get an error
> that the source does not exist. It'll work on refresh however.
> 
> If we, on a fresh page, start with the debugger open first, it works
> however. Any ideas? I thought the debugger always had sources now

I'm not sure exactly what you mean, but it sounds like if you start with the devtools open at all it should work, no matter which tool is selected. The problem is when opening the devtools and it immediately shows locations in the console, and you try to sourcemap them. The problem is that the debugger may still be initializing, and it hasn't created those sources yet.

I don't know how to implement it yet, but you probably need to wait for the debugger to initialize. I'll investigate this further and recommend a fix.
Comment 46 James Long (:jlongster) 2016-06-27 12:21:12 PDT
Said this on IRC, but I'll log it here. I'm fixing bug 1238173 so we don't let the debugger code get too burdened by old code. We need to change how we query scripts, and I think I should be able to land that patch soon.

You don't even need my patch though. You can go and ahead use the `findScripts` API directly which is how everything should be getting sources. Change `onResolveLocation` to look like this and the above problem should go away: https://gist.github.com/jlongster/aeb9d7ba6b9f3fa92a8387a9f5155cce

Note You need to log in before you can comment on or make changes to this bug.