Last Comment Bug 670002 - Use source maps in the web console
: Use source maps in the web console
Status: RESOLVED FIXED
[polish-backlog][difficulty=hard][gam...
: dev-doc-complete
Product: Firefox
Classification: Client Software
Component: Developer Tools: Console (show other bugs)
: unspecified
: All All
: P1 normal with 52 votes (vote)
: Firefox 50
Assigned To: Jaideep Bhoosreddy [:jbhoosreddy]
:
: Brian Grinstead [:bgrins]
Mentors:
: 934151 934701 996063 1221005 1239681 (view as bug list)
Depends on: 669999 672829 674283 676281 679165 679181 1130214 1177279 1179823 1251033 1294355 1296589
Blocks: 952127 1111089 1179813 1289570 1294578 618650
  Show dependency treegraph
 
Reported: 2011-07-07 14:11 PDT by Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
Modified: 2016-09-29 04:50 PDT (History)
75 users (show)
bgrinstead: needinfo? (jlaster)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


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 | Splinter 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 | Splinter 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 | Splinter Review
670002.patch [WIP] (17.18 KB, patch)
2016-06-23 14:56 PDT, Jaideep Bhoosreddy [:jbhoosreddy]
no flags Details | Diff | Splinter Review
670002.patch (17.21 KB, patch)
2016-07-01 00:56 PDT, Jaideep Bhoosreddy [:jbhoosreddy]
no flags Details | Diff | Splinter Review
670002.patch [1.0] (17.62 KB, patch)
2016-07-01 15:26 PDT, Jaideep Bhoosreddy [:jbhoosreddy]
no flags Details | Diff | Splinter Review
670002.1.patch [WIP] (11.70 KB, patch)
2016-07-05 08:20 PDT, Jaideep Bhoosreddy [:jbhoosreddy]
no flags Details | Diff | Splinter Review
670002.1.patch [WIP] (11.77 KB, patch)
2016-07-05 08:26 PDT, Jaideep Bhoosreddy [:jbhoosreddy]
no flags Details | Diff | Splinter Review
670002.patch [2.0] (24.22 KB, patch)
2016-07-08 14:13 PDT, Jaideep Bhoosreddy [:jbhoosreddy]
no flags Details | Diff | Splinter Review
670002.patch [3.0] (23.71 KB, patch)
2016-07-11 11:50 PDT, Jaideep Bhoosreddy [:jbhoosreddy]
no flags Details | Diff | Splinter Review
670002.patch [4.0] (28.92 KB, patch)
2016-07-12 18:14 PDT, Jaideep Bhoosreddy [:jbhoosreddy]
jsantell: feedback+
Details | Diff | Splinter Review
670002.patch [5.0] (30.84 KB, patch)
2016-07-17 01:08 PDT, Jaideep Bhoosreddy [:jbhoosreddy]
no flags Details | Diff | Splinter Review
670002.tests.patch (18.43 KB, patch)
2016-07-17 01:11 PDT, Jaideep Bhoosreddy [:jbhoosreddy]
jsantell: review+
Details | Diff | Splinter Review
670002.patch [5.0] (30.83 KB, patch)
2016-07-17 01:16 PDT, Jaideep Bhoosreddy [:jbhoosreddy]
jsantell: review+
Details | Diff | Splinter Review
670002.patch [6.0] (28.75 KB, patch)
2016-07-19 14:29 PDT, Jaideep Bhoosreddy [:jbhoosreddy]
jaideepb: review+
Details | Diff | Splinter Review
670002.tests.patch [1.0] (11.03 KB, patch)
2016-07-19 14:42 PDT, Jaideep Bhoosreddy [:jbhoosreddy]
jaideepb: review+
Details | Diff | Splinter Review
670002.patch [6.0] (28.85 KB, patch)
2016-07-20 00:40 PDT, Jaideep Bhoosreddy [:jbhoosreddy]
jaideepb: review+
Details | Diff | Splinter Review
670002.patch [6.0] (28.87 KB, patch)
2016-07-20 13:25 PDT, Jaideep Bhoosreddy [:jbhoosreddy]
jaideepb: review+
Details | Diff | Splinter Review
670002.patch [6.0] (28.91 KB, patch)
2016-07-20 19:09 PDT, Jaideep Bhoosreddy [:jbhoosreddy]
jaideepb: review+
Details | Diff | Splinter 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
Comment 47 Jaideep Bhoosreddy [:jbhoosreddy] 2016-07-01 00:56:01 PDT
Created attachment 8767076 [details] [diff] [review]
670002.patch

In this patch, the source mapping works. It is hidden behind a preference so that it doesn't slow down webconsole output for heavy sites. I'm hoping to land this implementation of sourcemapping. Jordan already gave me many pointers of how to proceed. I will create follow-up bugs for those. And there are weird issues which come up due to various corner cases, so follow-up bugs for them too. Let me know if there's any suggestions for this patch, or any bug you faced with it.
Comment 48 Jordan Santell [:jsantell] [@jsantell] (Away from Bugzilla for awhile) 2016-07-01 11:01:26 PDT
Comment on attachment 8767076 [details] [diff] [review]
670002.patch

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

Some more changes needed, but looking good!

::: devtools/client/framework/toolbox.js
@@ +122,5 @@
>    this._target = target;
>    this._toolPanels = new Map();
>    this._telemetry = new Telemetry();
>  
> +  this._sourceLocationController = new SourceLocationController(this._target);

We should put the pref here -- otherwise, the SLC is still tracking and storing sources for everything, which will add overhead. This means toggling the pref would require a toolbox restart, but that's fine.

::: devtools/client/preferences/devtools.js
@@ +249,5 @@
>  pref("devtools.webconsole.filter.servererror", false);
>  pref("devtools.webconsole.filter.serverwarn", false);
>  pref("devtools.webconsole.filter.serverinfo", false);
>  pref("devtools.webconsole.filter.serverlog", false);
> +pref("devtools.webconsole.sourcemap.enabled", true);

Two things: this isn't specific to webconsole; maybe 'devtools.sourcemaplocations.enabled'? And the other is, this definitely should be false by default

::: devtools/client/shared/components/frame.js
@@ +46,5 @@
> +    if (Services.prefs.getBoolPref("devtools.webconsole.sourcemap.enabled")) {
> +      const sourceLocationController = this.props.sourceLocationController;
> +      if (sourceLocationController) {
> +        sourceLocationController.bindLocation(source, (prevLocation, nextLocation) => {
> +          const newFrame = { source: nextLocation.url, line: nextLocation.line,

Nit: each property should be on a new line here

@@ +80,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) {

Do we use `isLinkable` without also checking `isSourceMapped`? If so, we should roll these into the same boolean, like if we can parse URL OR if it's a source map then => isLinkable

@@ +100,5 @@
>                   frame.functionDisplayName)
>        );
>      }
> +    let displaySource = short;
> +    // Since SourceMapped URLs might not be parsed properly by parseURL

Explain what 'not parsed properly' means; that source mapped URLs could look like '/files/file.coffee', and not URLs and could look like file paths

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

formatting: the last line of the tertiary should be on a new line (I think this is a linting rule)

@@ +118,5 @@
>  
>      // If source is linkable, and we have a line number > 0
> +    // Source mapped sources might not necessary linkable, but they
> +    // are still valid in the debugger.
> +    if ((isLinkable || isSourceMapped) && line) {

Maybe store this conditional up top like shouldDisplayLine (same with column) since it's more complex now and we use it twice
Comment 49 Jaideep Bhoosreddy [:jbhoosreddy] 2016-07-01 15:26:52 PDT
Created attachment 8767346 [details] [diff] [review]
670002.patch [1.0]

As per Jordan's feedback on the previous patch.
Comment 50 Jaideep Bhoosreddy [:jbhoosreddy] 2016-07-05 08:20:35 PDT
Created attachment 8768022 [details] [diff] [review]
670002.1.patch [WIP]

I'm rewriting the API that handles source mapping following the suggestions from Jordan. It's still a work in progress, but I wanted to get feedback on this approach. Do let me know whatever you make of this.
This approach is aimed to solve performance issues by using event emitters and caching.
Comment 51 Jaideep Bhoosreddy [:jbhoosreddy] 2016-07-05 08:26:16 PDT
Created attachment 8768023 [details] [diff] [review]
670002.1.patch [WIP]

Small fix from previous patch.
Comment 52 Jordan Santell [:jsantell] [@jsantell] (Away from Bugzilla for awhile) 2016-07-06 09:46:27 PDT
Comment on attachment 8768023 [details] [diff] [review]
670002.1.patch [WIP]

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

::: devtools/client/framework/sourcemap.js
@@ +17,5 @@
> + */
> +
> +function SourceMapService(target) {
> +  this._target = target;
> +  this._sourceMap = new Map();

this._sourceMap could have a more specific name describing what it actually is; I have to read through the code to know what this does

@@ +54,5 @@
> +    this._sourceMap.set(location.url, new Map());
> +  } else {
> +    const cachedLocation = this._sourceMap.get(location.url).get(serialize(location));
> +    if (cachedLocation) {
> +      result = cachedLocation;

This result isn't actually used, as we'll always call `resolveLocation` below, whether or not the value has been cached

@@ +75,5 @@
> +  if (!source.url || type === "newSource" && !source.isSourceMapped) {
> +    return;
> +  }
> +
> +  if (this._sourceMap.has(source.generatedUrl)) {

So the data structure behind this is a map of maps -- I think the common source map usage is a giant build.js file, which maps to several files, meaning we still have to iterate over every observed location -- I think just keying it from the serialized value makes sense, like, `this._sourceMap['http://file.js:123:456']` should be sufficient

@@ +112,5 @@
> +}
> +
> +function serialize(source) {
> +  const { url, line, column } = source;
> +  return `${url}:${line}${column}`;

Needs a : between line and column, and handle undefined line/column values (a source looking like "http://mozilla.org:undefined:undefined" will look like an error)

::: devtools/client/shared/components/frame.js
@@ +63,5 @@
> +    if (sourceMapService) {
> +      let source = this.getSource(this.state.frame);
> +      sourceMapService.on(this.serialize(source), this.onSourceUpdated);
> +      source = this.getSource(prevState.frame);
> +      sourceMapService.off(this.serialize(source), this.onSourceUpdated);

Why do you bind on then off here? I'm not sure what's the reason for this or what's happening

@@ +69,5 @@
> +  },
> +
> +  onSourceUpdated: function (location) {
> +    const sourceMapService = this.props.sourceMapService;
> +    sourceMapService._resolveLocation(location).then((resolvedLocation) => {

If `_resolveLocation` is to be a public API, we should remove the underscore, a convention used for private methods

@@ +80,5 @@
> +      }
> +    });
> +  },
> +
> +  getSource: function (frame) {

getSource/getFrame functions are a bit confusing -- it's unclear what they're referring to (we have a lot of very similarly named things in all of this code which doesn't help)

@@ +100,5 @@
> +      functionDisplayName: this.props.frame.functionDisplayName,
> +    };
> +  },
> +
> +  serialize: function (source) {

Should expose this utility in the source map file and just use it rather than making it a method
Comment 53 Jaideep Bhoosreddy [:jbhoosreddy] 2016-07-08 14:13:03 PDT
Created attachment 8769344 [details] [diff] [review]
670002.patch [2.0]
Comment 54 Jaideep Bhoosreddy [:jbhoosreddy] 2016-07-11 11:50:09 PDT
Created attachment 8769822 [details] [diff] [review]
670002.patch [3.0]
Comment 55 Jordan Santell [:jsantell] [@jsantell] (Away from Bugzilla for awhile) 2016-07-11 16:05:34 PDT
Comment on attachment 8769822 [details] [diff] [review]
670002.patch [3.0]

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

This is looking great! Mostly nits, the only part that may need some logic change is the _locationStore/_promiseCache redundancy.

::: devtools/client/framework/moz.build
@@ +20,5 @@
>      'menu.js',
>      'selection.js',
>      'sidebar.js',
>      'source-location.js',
> +    'sourcemap.js',

I'd call this `source-map-service`, and remove `source-location`

::: devtools/client/framework/sourcemap.js
@@ +84,5 @@
> + */
> +SourceMapService.prototype._resolveAndUpdate = function (location) {
> +  this._resolveLocation(location).then(resolvedLocation => {
> +    if (resolvedLocation) {
> +      if (resolvedLocation.url !== location.url) {

Is there a reason for this conditional? Wouldn't pretty printed sources still have the same URL, but updated location?

@@ +102,5 @@
> + * @param location
> + * @return Promise<Object>
> + * @private
> + */
> +SourceMapService.prototype._resolveLocation = Task.async(function* (location) {

If I'm understanding this correctly, locationStore and promiseCache are used similarly here -- just the promiseCache stores the promise to the RDP request, and the locationStore stores the primitive result of said promise. Maybe we can get away with one store, as the initial resolution will be a failure (if we do not yet have the source-updated event), but when we do get a source update event, that'll invalidate our cache for us so we can requery the correct location at that point.

@@ +134,5 @@
> +  return resolvedLocation;
> +});
> +
> +/**
> + * Checks if the source updated event is relevant to source mapping

Mention this is fired via 'source-updated' event from target

@@ +163,5 @@
> +    if (this._locationStore.has(sourceUrl)) {
> +      const unresolvedLocations = [...this._locationStore.get(sourceUrl).keys()];
> +      this._invalidateCache(sourceUrl);
> +      for (let unresolvedLocation of unresolvedLocations) {
> +        dump("\r\nresolve and update: "+unresolvedLocation);

nit: debugging message

@@ +212,5 @@
> +function serialize(source) {
> +  let { url, line, column } = source;
> +  line = line || 0;
> +  column = column || 0;
> +  return `${url}<:>${line}<:>${column}`;

As a nice-to-have, we should make "<:>" be a constant at the top of the file, like SOURCE_TOKEN or something for serialize/deserialize

::: devtools/client/shared/components/frame.js
@@ +187,5 @@
> +      displaySource = displaySource.lastIndexOf("/") < 0 ?
> +        displaySource :
> +        displaySource.slice(displaySource.lastIndexOf("/") + 1);
> +    } else {
> +      if (showEmptyPathAsHost && (displaySource === "" || displaySource === "/")) {

Roll this into one `else if` statement

@@ +214,5 @@
>      }
>  
>      // If source is not a URL (self-hosted, eval, etc.), don't make
>      // it an anchor link, as we can't link to it.
> +    const that = this;

We don't need to redefine `this` because the arrow function below perpetuates the `this` context, so you can continue using `this.getSource(frame)`
Comment 56 Jaideep Bhoosreddy [:jbhoosreddy] 2016-07-12 18:14:06 PDT
Created attachment 8770345 [details] [diff] [review]
670002.patch [4.0]

So, I'm not sure if I've properly solved the _locationStore/_promiseCache redundancy. But right now, there's just one store called _locationStore which simply caches the promised location directly.
I believe I implemented all other feedback I got from Jordan in this patch also.
Comment 57 Jordan Santell [:jsantell] [@jsantell] (Away from Bugzilla for awhile) 2016-07-12 20:23:50 PDT
Comment on attachment 8770345 [details] [diff] [review]
670002.patch [4.0]

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

Some minor comments, but this looks great, I'd say tests is the next step

::: devtools/client/framework/source-map-service.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +"use strict";
> +
> +const { Task } = require("devtools/shared/task");
> +var EventEmitter = require("devtools/shared/event-emitter");

nit: const

@@ +41,5 @@
> +/**
> + * Clears the store containing the cached resolved locations and promises
> + */
> +SourceMapService.prototype.reset = function () {
> +  if (this._locationStore) {

What's the scenario where we reset but don't have the store? Should only happen after `destroy`, is there a straggling call to reset somewhere after it's destroyed?

@@ +71,5 @@
> + * @param location
> + * @param callback
> + */
> +SourceMapService.prototype.unsubscribe = function (location, callback) {
> +  this.off(serialize(location), callback);

Do we need to do any cache clearing here?

@@ +82,5 @@
> + * @private
> + */
> +SourceMapService.prototype._resolveAndUpdate = function (location) {
> +  this._resolveLocation(location).then(resolvedLocation => {
> +    if (resolvedLocation && !isSameLocation(location, resolvedLocation)) {

Can you comment why we need to check to ensure that these locations are different?

@@ +104,5 @@
> +  if (!location.url || !location.line) {
> +    return null;
> +  }
> +  let resolvedLocation = null;
> +  const cachedLocation = this._getLocationFromStore(location);

This is looking much cleaner, nice! For a future refactoring, maybe can have a separate singleton handle all the actions on the store, proxying to essentially the cache's helper methods.
Comment 58 Jaideep Bhoosreddy [:jbhoosreddy] 2016-07-15 13:44:37 PDT
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ef7223a092c
Comment 59 Jaideep Bhoosreddy [:jbhoosreddy] 2016-07-17 01:08:11 PDT
Created attachment 8771741 [details] [diff] [review]
670002.patch [5.0]
Comment 60 Jaideep Bhoosreddy [:jbhoosreddy] 2016-07-17 01:11:37 PDT
Created attachment 8771743 [details] [diff] [review]
670002.tests.patch

Few things to note for this test. Unpretty printing test still doesn't work. And is therefore left commented out. Secondly, Source Map Service does not emit the resolved location to the callback if the resolved location is the same as the original location. And therefore, the third testcase has been removed, as it is not valid anymore.
Comment 61 Jaideep Bhoosreddy [:jbhoosreddy] 2016-07-17 01:16:23 PDT
Created attachment 8771744 [details] [diff] [review]
670002.patch [5.0]
Comment 62 Jaideep Bhoosreddy [:jbhoosreddy] 2016-07-17 01:17:36 PDT
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8c838e19d3d
Comment 63 Jordan Santell [:jsantell] [@jsantell] (Away from Bugzilla for awhile) 2016-07-19 11:01:45 PDT
Comment on attachment 8771743 [details] [diff] [review]
670002.tests.patch

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

I would see about `hg mv` these test files as they're rather similar rather than deleting/adding. And the auxiliary test files that are being copied into devtools/client/framework/test also need to be added. Looks good otherwise!

::: devtools/client/framework/test/browser.ini
@@ +8,5 @@
>    browser_toolbox_sidebar_tool.xul
>    browser_toolbox_window_title_changes_page.html
>    browser_toolbox_window_title_frame_select_page.html
> +  code_binary_search.coffee
> +  code_binary_search.js

If these files are being copied/moved over into framework, they should also be in the patch as well

::: devtools/client/framework/test/browser_source_map-01.js
@@ +12,5 @@
> + * are subsequently found. Also checks when no column is provided, and
> + * when tagging an already source mapped location initially.
> + */
> +
> +const DEBUGGER_ROOT = "http://example.com/browser/devtools/client/debugger/test/mochitest/";

Since the files are moved to framework (according to the browser.ini), we should use the normal root for these tests rather than the debugger root.

::: devtools/client/framework/test/browser_source_map-02.js
@@ +6,5 @@
> + * Tests the SourceMapService updates generated sources when pretty printing
> + * and un pretty printing.
> + */
> +
> +const DEBUGGER_ROOT = "http://example.com/browser/devtools/client/debugger/test/mochitest/";

Same with these fixture files, should reference framework root if we moved them into this directory
Comment 64 Jordan Santell [:jsantell] [@jsantell] (Away from Bugzilla for awhile) 2016-07-19 11:36:43 PDT
Comment on attachment 8771744 [details] [diff] [review]
670002.patch [5.0]

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

Some last comments, but with these changes, and tests passing, let's land it, looks great!

::: devtools/client/framework/moz.build
@@ +19,5 @@
>      'menu-item.js',
>      'menu.js',
>      'selection.js',
>      'sidebar.js',
> +    'source-map-service.js',

Be sure we're using `hg mv` to track history between source-location and source-map-service

::: devtools/client/framework/source-map-service.js
@@ +86,5 @@
> + * @private
> + */
> +SourceMapService.prototype._resolveAndUpdate = function (location) {
> +  this._resolveLocation(location).then(resolvedLocation => {
> +    // Check to make sure that the resolved location is not same as the original location

Comment should indicate why this matters (performance?), or why this would happen in the first place (this I'm unclear on)

@@ +108,5 @@
> +  // Location must have a url and a line
> +  if (!location.url || !location.line) {
> +    return null;
> +  }
> +  let resolvedLocation = null;

`resolvedLocation` isn't true here -- it's still a promise. I think it'd be easier to read and reason about if we just returned in the conditionals.

@@ +111,5 @@
> +  }
> +  let resolvedLocation = null;
> +  const cachedLocation = this._locationStore.get(location);
> +  if (cachedLocation) {
> +    resolvedLocation = cachedLocation;

return cachedLocation

@@ +116,5 @@
> +  } else {
> +    const promisedLocation = resolveLocation(this._target, location);
> +    if (promisedLocation) {
> +      this._locationStore.set(location, promisedLocation);
> +      resolvedLocation = promisedLocation;

return promisedLocation

@@ +145,5 @@
> +    sourceUrl = source.generatedUrl;
> +  } else if (source.url && source.isPrettyPrinted) {
> +    sourceUrl = source.url;
> +  }
> +  if (sourceUrl) {

Only need one conditional here, and with store.js changes, can just be:

const storedLocations = this._locationStore.getByUrl(sourceUrl);
if (storedLocations.length) {
  this._locationStore.clearByUrl(sourceUrl);
  for (let storedLocation of storedLocations) {
    this._resolveAndUpdate(deserialze(storedLocation));
  }
}

@@ +146,5 @@
> +  } else if (source.url && source.isPrettyPrinted) {
> +    sourceUrl = source.url;
> +  }
> +  if (sourceUrl) {
> +    if (this._locationStore.has(sourceUrl)) {

see store.js for more comments on the locationStore method names

::: devtools/client/framework/store.js
@@ +12,5 @@
> +  this.set = this.set.bind(this);
> +  this.clear = this.clear.bind(this);
> +  this.has = this.has.bind(this);
> +  this.getAll = this.getAll.bind(this);
> +  this.invalidateCache = this.invalidateCache.bind(this);

I don't think any of these methods need context bound

@@ +59,5 @@
> + * Utility proxy method to Map.has() method
> + * @param url
> + * @returns {boolean}
> + */
> +Store.prototype.has = function (url) {

I would expect `has` to also take a location and return a boolean version of the `get` method -- I think combining the `has` method with `getAll` would serve the same purpose, and also indicate which methods take a location and which take a URL. I think getting rid of `has` (unless it was a boolean version of `get`, which we don't need) and renaming `getAll` to `getByURL` that returns an array which we can check the length of in SMS.

@@ +67,5 @@
> +/**
> + * Retrieves an object containing all locations to be resolved when `source-updated`
> + * event is triggered.
> + * @param url
> + * @returns {*[]}

Return type here would be `{Array<String>}` as it's returning an array of serialized locations

@@ +78,5 @@
> + * Invalidates the stale location promises from the store when `source-updated`
> + * event is triggered, and when FrameView unsubscribes from a location.
> + * @param url
> + */
> +Store.prototype.invalidateCache = function (url) {

A better name to indicate this is a url not location based API could be `invalidateByURL` or `clearByURL`

@@ +81,5 @@
> + */
> +Store.prototype.invalidateCache = function (url) {
> +  this._safeAccessInit(url);
> +  this._store.get(url).clear();
> +  this._store.set(url, new Map());

Don't need to clear and set again, just setting will overwrite the previous value stored by the key

@@ +84,5 @@
> +  this._store.get(url).clear();
> +  this._store.set(url, new Map());
> +};
> +
> +exports.Store = Store;

This file and global name, 'Store', is in all of frameworks, but really only specific to locations for source maps, so I don't think it has much use outside of that. That being said, the generic name indicates otherwise. I think a name like LocationStore/location-store would be more appropriate here
Comment 65 Jaideep Bhoosreddy [:jbhoosreddy] 2016-07-19 14:29:32 PDT
Created attachment 8772583 [details] [diff] [review]
670002.patch [6.0]
Comment 66 Jaideep Bhoosreddy [:jbhoosreddy] 2016-07-19 14:42:12 PDT
Created attachment 8772589 [details] [diff] [review]
670002.tests.patch [1.0]
Comment 67 Brian Grinstead [:bgrins] 2016-07-19 14:49:50 PDT
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b23e098d3893
Comment 68 Jaideep Bhoosreddy [:jbhoosreddy] 2016-07-19 22:42:32 PDT
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b558154c7a8
Comment 69 Jaideep Bhoosreddy [:jbhoosreddy] 2016-07-20 00:40:00 PDT
Created attachment 8772720 [details] [diff] [review]
670002.patch [6.0]
Comment 70 Pulsebot 2016-07-20 07:42:01 PDT
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/aab8baf2c5f5
Use source maps in the web console w/ performance issues. r=jsantell
https://hg.mozilla.org/integration/fx-team/rev/c6a1177a17e4
Tests for source maps in the web console w/ performance issues. r=jsantell
Comment 71 Wes Kocher (:KWierso) 2016-07-20 13:00:41 PDT
I had to back this out for eslint failures: https://hg.mozilla.org/integration/fx-team/rev/c6e4b6a744697056dcebe1cf298ed3b7de85311c

TEST-UNEXPECTED-ERROR | devtools/client/shared/components/frame.js:141:1 | Line 141 exceeds the maximum line length of 90. (max-len)
TEST-UNEXPECTED-ERROR | devtools/client/shared/components/frame.js:186:1 | Line 186 exceeds the maximum line length of 90. (max-len)
TEST-UNEXPECTED-ERROR | devtools/client/shared/components/frame.js:194:5 | Block must not be padded by blank lines. (padded-blocks)
Comment 72 Jaideep Bhoosreddy [:jbhoosreddy] 2016-07-20 13:25:09 PDT
Created attachment 8773006 [details] [diff] [review]
670002.patch [6.0]
Comment 73 Wes Kocher (:KWierso) 2016-07-20 15:16:14 PDT
I forgot to back out the other part of this: https://hg.mozilla.org/integration/fx-team/rev/0b9ec2488c89
Comment 74 Jaideep Bhoosreddy [:jbhoosreddy] 2016-07-20 19:09:22 PDT
Created attachment 8773116 [details] [diff] [review]
670002.patch [6.0]

Apparently my eslint stopped working which unfortunately caused this issue. should work properly now.
Comment 75 Pulsebot 2016-07-20 21:02:55 PDT
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/486691c44ed6
Tests for source maps in the web console w/ performance issues; r=jsantell
https://hg.mozilla.org/integration/fx-team/rev/4c3f8c8dbefa
Use source maps in the web console w/ performance issues; r=jsantell
Comment 77 David Bruant 2016-07-26 01:53:25 PDT
IIUC this is a long-awaited feature.

<3 to everyone involved!
Comment 78 Dan Mosedale (:dmose) 2016-07-26 12:50:36 PDT
Indeed, thanks so much!
Comment 79 Will Bamberg [:wbamberg] 2016-09-07 20:22:31 PDT
I've added a note on this: https://developer.mozilla.org/en-US/docs/Tools/Web_Console/Console_messages#Source_maps.

I also wrote a minimal demo, do you think it's worth linking?

https://wbamberg.github.io/devtools-examples/sourcemaps-in-console/index.html
Comment 80 Jordan Santell [:jsantell] [@jsantell] (Away from Bugzilla for awhile) 2016-09-09 17:56:00 PDT
@Will: Nice! I think it's worth linking -- FWIW, I think maybe a babel/coffeescript source map might be more obvious than unminified code -- linking to the debugger and seeing non-JS/ES67, as well as something like a .coffee file in the console
Comment 81 Dan Mosedale (:dmose) 2016-09-12 08:20:47 PDT
@wbamberg: very cool!  I'd suggest also adding a line to the MDN note that explains what disadvantages/problems a user should expect to see if they set the pref to its non-default value, as well as linking to whatever bug is tracking turning them on by default.
Comment 82 Will Bamberg [:wbamberg] 2016-09-12 16:15:49 PDT
https://bugzilla.mozilla.org/show_bug.cgi?id=670002#c47(In reply to Jordan Santell [:jsantell] [@jsantell] (Away from Bugzilla for awhile) from comment #80)
> @Will: Nice! I think it's worth linking -- FWIW, I think maybe a
> babel/coffeescript source map might be more obvious than unminified code --
> linking to the debugger and seeing non-JS/ES67, as well as something like a
> .coffee file in the console

That's a good suggestion. I know nothing practical about CoffeeScript, but have had a go here: https://wbamberg.github.io/devtools-examples/sourcemaps-in-console/index.html Does that look all right? If so I'll move this repo under the MDN org on GitHub.

(In reply to Dan Mosedale (:dmose) from comment #81)
> @wbamberg: very cool!  I'd suggest also adding a line to the MDN note that
> explains what disadvantages/problems a user should expect to see if they set
> the pref to its non-default value, as well as linking to whatever bug is
> tracking turning them on by default.

This also sounds like a good suggestion. I think the bug is bug 1289570, yes? In terms of potential disadvantages: apart from general comments about it being experimental (hence potentially buggy) and possibly making console output slower, I haven't seen anything in that bug or this one. Is there anything more specific I should say here?
Comment 83 Dan Mosedale (:dmose) 2016-09-13 08:04:04 PDT
:wbamberg that seems pretty clearly the correct bug, and I'd guess that your list of disadvantages is correct, but :bgrins is a better one to confirm that, I think.

Another thing that's relevant is that if it slows down nightly, I'd also suggest some text about being sure this pref is off until the linked-to bug gets fixed in <https://developer.mozilla.org/en-US/docs/Mozilla/Benchmarking>, as teams around here sometimes do profiling work against Nightly, since it's the "most current" build.
Comment 84 Brian Grinstead [:bgrins] 2016-09-15 15:14:49 PDT
(In reply to Will Bamberg [:wbamberg] from comment #82)
> This also sounds like a good suggestion. I think the bug is bug 1289570,
> yes? In terms of potential disadvantages: apart from general comments about
> it being experimental (hence potentially buggy) and possibly making console
> output slower, I haven't seen anything in that bug or this one. Is there
> anything more specific I should say here?

Just as you mentioned - that's the correct bug.  And it's off by default because of perf and that it's still experimental / potentially buggy.

Also, before we turn it on by default I believe we will need to make some technical changes to switch to a client-side implementation of source maps that the new debugger frontend is going to use.  I had a chat with Jaideep, Jason, and James about this - maybe one of them can outline the steps we need to go through before turning it on (along with any issues in gh/bugzilla).
Comment 85 Will Bamberg [:wbamberg] 2016-09-19 16:40:10 PDT
I've updated:
https://developer.mozilla.org/en-US/docs/Mozilla/Benchmarking
https://developer.mozilla.org/en-US/docs/Tools/Web_Console/Console_messages#Source_maps

...and marking this dev-doc-complete, but let me know if you see anything else.

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