Closed Bug 1177279 Opened 9 years ago Closed 8 years ago

Source Map Controller/Renderer/Orchestrator

Categories

(DevTools :: Framework, defect)

Product:
DevTools
Component:
Framework
Version:
41 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox41 affected, firefox48 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 48
Tracking Flags:
Tracking Status
firefox41 --- affected
firefox48 --- fixed

People

(Reporter: jsantell, Assigned: jsantell)

References

(Blocks 2 open bugs,
URL
)

Details

Attachments

(1 file, 4 obsolete files)

1177279-source-location.patch
34.47 KB, patch
jryans
: review+
jlong
: review+
Details | Diff | Splinter Review 
On the front end, we need to react to source location changes when sources update via source map, pretty print, etc.

Scenarios:
* console.log, source maps load one second later, the location should be updated
* console.log a minified file, pretty print in the future, location should be updated
* same as above, but then unprettyprint in the future, and location is updated back to original location
* Any of the above scenarios, except instead of console.log (which is updating text on an element), updating text on canvas (perf tools), so has to update a canvas rendering
* Tons of console.logs on a page, and when pretty printing later, where we have to update 1000 elements, a way for a specific tool to batch/render smartly for its specific scenario

console.log is an example, but covers the "update an element's text" scenario.

Pseudocode implementation: https://gist.github.com/jsantell/f5eb39c248a1d2f59f82
Jordan, are you going to continue working on the frontend? I can take the backend piece so that the "updated" event is emitted.
Yeah I'm going to keep on this one -- the backend piece I think I have working, and I'll certainly send over for review to you to make sure it's right
Attached patch 1177279-source-location.patch (obsolete) — — Splinter Review 
Handles the orchestration of location objects -- these can be generated sources, and lazily updated when source map parsed, or the possibility of pretty prints. For actual use case, these would probably receive the already source mapped locations.

These require the debugger being opened. Should handle that in a different bug
Attachment #8628504 - Flags: review?(jlong)
Attachment #8628504 - Flags: feedback?
Attachment #8628504 - Flags: feedback? → feedback?(nfitzgerald)
Comment on attachment 8628504 [details] [diff] [review]
1177279-source-location.patch

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

::: toolkit/devtools/server/actors/script.js
@@ +1870,5 @@
>        source: aSource.form()
>      });
>    },
>  
> +  onResolveLocation: function (aRequest) {

This method should be on "parent" actors: TabActor, ChromeActor, AddonActor, etc. Not on the ThreadActor. I *think* they all inherit from TabActor, so you shouldn't need to repeat yourself a bunch.

In general, actors shouldn't be talking to their siblings or cousins in the hierarchy (which having a console/profiler actor talk to a debugger actor would be doing), but only to their parents and/or children. Otherwise, lifetimes get confusing and error prone, and this tends to lead to either leaks or dangling actor id references that error out when you try to do anything with them. This will additionally make it a lot easier to use without needing the debugger to be in a specific state in the future.
Attachment #8628504 - Flags: feedback?(nfitzgerald)
> I *think* they all inherit from TabActor, so you shouldn't need to repeat yourself a bunch.

They do, or at least that's the intention. There might be one type that still doesn't inherit, I think, but a lot of work has been done recently to have a common base actor. ChildProcessActor in child-process.js I think is one place you might need to duplicate, not sure how all that works.
(I'll review this patch tomorrow, or possibly today!)
(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #6)
> This method should be on "parent" actors: TabActor, ChromeActor, AddonActor,
> etc. Not on the ThreadActor. I *think* they all inherit from TabActor, so
> you shouldn't need to repeat yourself a bunch.

Alex recently landed some docs about the actors[1], so that's a helpful reference when discussing these matters!  Soon we hope to rename them a bit for less confusion as well.

[1]: https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/docs/actor-hierarchy.md
repinging jlongster
Comment on attachment 8628504 [details] [diff] [review]
1177279-source-location.patch

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

There's a few other small things I want to look at in this patch, but I'm going to r- mainly because of the onResolveLocation request. Move that into the TabActor and the rest of this looks good. r? me again when you do that, just didn't want this review to linger any more

::: browser/devtools/shared/source-location.js
@@ +6,5 @@
> +
> +loader.lazyImporter(this, "Task", "resource://gre/modules/Task.jsm");
> +loader.lazyRequireGetter(this, "EventTarget", "sdk/event/target", true);
> +loader.lazyRequireGetter(this, "emit", "sdk/event/core", true);
> +loader.lazyRequireGetter(this, "Class", "sdk/core/heritage", true);

I don't think it's worth making any of these lazy imports. They are all used pretty much immediately.

::: toolkit/devtools/server/actors/script.js
@@ +1870,5 @@
>        source: aSource.form()
>      });
>    },
>  
> +  onResolveLocation: function (aRequest) {

(warning: this triggered a braindump of stuff I've been meaning to figure out for a while)

Yeah, we need to figure out the "canonical" place to put data that should be global to all tools (like sources). And figure out what data and APIs should be put there.

I was thinking ThreadActor was that place, but taking a step back I realize it's really just a debugger-specific thing that handles pausing/resuming the thread and other things. You appropriately put APIs on TabTarget to work with locations, which is right, and I think we should be consistent with these RDP calls too.

This even uses `this.sources`, which is just a getter for `this.parent.sources`, since it's the target that handles sources because so many things need them. Eventually I want to move SourceActor out of here because it's no longer tied to the debugger. Eddy has talked about moving breakpoint methods out into its own module too, since it's complicated and would be good to modularize.

We're going to have to initialize the debugger on toolbox open to be able to query sources (we also want to do this to catch any `debugger` statements). However, I have concerns about eagerly fetching all the sources, since some pages have hundreds, and I'm sure there's a perf hit there.

We can lazily fetch a SourceActor based on URL (or something else, I'm not sure what locations from eval scripts show up in the console as). This means all APIs needs to be async though, even things like querying if a source is blackboxed. Right now we just check a property of the `SourceClient`, but I don't think we can do that if we're going to lazily populate sources.

Anyway, just something to think about. There's a bunch of stuff to figure out, but adding APIs and RDP methods to TabActor is what we should be doing for anything that all tools need.

::: toolkit/devtools/server/actors/utils/TabSources.js
@@ +232,5 @@
>          return this._sourceMappedSourceActors[url];
>        }
>      }
>  
> +    return null;

Why do you need to change this? We were very intentional about making the getSource* methods throw an error. It's too easy to unintentionally try to get one when it doesn't exist for some reason.

I'd rather you add methods to query the state of the sources, like "hasSource*". I like that these functions imply that you don't have to check for null; if you are calling them, you *know* the source exists.

Or add an optional parameter to these methods that explicitly turn on the null-returning behavior.
Attachment #8628504 - Flags: review?(jlong) → review-
(In reply to James Long (:jlongster) from comment #11)
> Yeah, we need to figure out the "canonical" place to put data that should be
> global to all tools (like sources). And figure out what data and APIs should
> be put there.

1. They aren't really global, they are shared within the given debugging context, ie chrome debugging, tab debugging, addon debugging, etc.

2. We have this "canonical" place and it is the parent actor (ie TabActor, AddonActor, ChromeActor, etc). Its just that traditionally, we have shared almost nothing and so there isn't much there. As we share more stuff, like this bug does, we should populate those actors with more functionality.
(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #12)
> (In reply to James Long (:jlongster) from comment #11)
> > Yeah, we need to figure out the "canonical" place to put data that should be
> > global to all tools (like sources). And figure out what data and APIs should
> > be put there.
> 
> 1. They aren't really global, they are shared within the given debugging
> context, ie chrome debugging, tab debugging, addon debugging, etc.

That's what I meant, good to clarify though.

> 
> 2. We have this "canonical" place and it is the parent actor (ie TabActor,
> AddonActor, ChromeActor, etc). Its just that traditionally, we have shared
> almost nothing and so there isn't much there. As we share more stuff, like
> this bug does, we should populate those actors with more functionality.

Ok. I'm confused when you have a reference to those. These shouldn't be tool-specific, so does the target somehow have a reference to this?

Looking at target.js, looks like it might. `target.activeTab` is a TabClient, which works for normal tab and addon debugging, right? And then it has `target.chrome` which is a client for the ChromeActor?

So we need a generic way to get a reference to whatever remote actor it is, that conforms to the TabActor interface, and will have the APIs we need.

I need to read through the docs more in webbrowser.js, I know Alex has improved them a lot.
cc'ing jryans, I'm sure he has good input here. fwiw when asking him it target was the right place to put stuff he said:

<jryans> jlongster: hmm. the main "global" things are the target and the
         client (from dbg-client.jsm). i might suggest the client for this
         case. in my mental model, the target is for "what does the toolbox
         need to get started?", and the client is more focused on specific RDP
         requests.                                                      [18:50]
Flags: needinfo?(jryans)
I think I finally understand what things are "TabActor" like things. The only things that's annoying is that not everything inherits from TabActor. ChildProcessActor, for example, does not: https://github.com/mozilla/gecko-dev/blob/master/toolkit/devtools/server/actors/child-process.js but it is that parent of the debugger when debugging "child chrome scripts" in e10s (using the Browser Content Toolbox).

I don't know if there are even plans to make it inherit from TabActor, as I assume there is a reason it is not right now. It just implements the interface, and we add stuff to that interface we'll need to add it in 2 places.

What about `client`? It looks likes that is a `DebuggerClient` instance. I'm not sure if tools that use protocol.js even use that though.
I agree with both of you that on the actor side, these shared functions make sense at the "TabActor interface"[1] level, which corresponds to "server side thing you point a whole toolbox at".  (Everyone's agreed these names are terrible, join the fun in bug 1172897 to bike shed new ones!)

(In reply to James Long (:jlongster) from comment #15)
> I think I finally understand what things are "TabActor" like things. The
> only things that's annoying is that not everything inherits from TabActor.
> ChildProcessActor, for example, does not:
> https://github.com/mozilla/gecko-dev/blob/master/toolkit/devtools/server/
> actors/child-process.js but it is that parent of the debugger when debugging
> "child chrome scripts" in e10s (using the Browser Content Toolbox).
> 
> I don't know if there are even plans to make it inherit from TabActor, as I
> assume there is a reason it is not right now. It just implements the
> interface, and we add stuff to that interface we'll need to add it in 2
> places.

I think the main reason it currently does not inherit from TabActor is that the browser content toolbox that this actor supports has only a subset of the usual features: only Console / Debugger / Scratchpad are accessible.

However, we also seem to lose time being confused by this distinction.  It may be possible to always use inheritance, but then disable some features, so we at least don't have to duplicate code -- but I haven't thought about this in depth.

> What about `client`? It looks likes that is a `DebuggerClient` instance. I'm
> not sure if tools that use protocol.js even use that though.

Part of what dbg-client.jsm implements is the `TabClient`.  The `TabClient` has the client-side requests for RDP methods on the TabActor interface today.

So, adding your methods to TabClient seems like the natural place.  If TabActors used protocol.js, this thing would be a TabFront, but they do not yet, so this is what we have instead.

(In reply to James Long (:jlongster) from comment #13)
> Looking at target.js, looks like it might. `target.activeTab` is a
> TabClient, which works for normal tab and addon debugging, right? And then
> it has `target.chrome` which is a client for the ChromeActor?

Not quite.  A target only represents *one* "thing you point a toolbox at", or in other words it is only describing a single TabActor interface.  So, for a single tab, you have target A.  For a child process you have target B.  They are separate targets.  Each thing you aim a toolbox at is a new target.

`target.activeTab` will always be the target's `TabClient`, so that should be all you need.  You should be able to reach the current target from most places in front-end code, but if that's hard for some special case, let's think about how to tweak things for that scenario.

`target.chrome` is a boolean flag that is trying to say[2] "is this targeted thing content or chrome scope?", which seems unrelated to the work being done in this bug.

[1]: https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/docs/actor-hierarchy.md#69
[2]: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/framework/target.js#330-333
Flags: needinfo?(jryans)
Depends on: 1132501
Jordan, May that help if I also review this patch? Or do you just need some free cycles to get back to this?
waiting for bug 1132501 to land due to the thread actor events moving around
Attached patch 1177279-source-location.patch (obsolete) — — Splinter Review 
Wanted to f? this sooner than later; adding comments, removing logs and fixing up tests are pretty straight forward, but my main question is where should this live? Right now thread actor emits new/updated source events, and the SourceLocationController (better name?) takes a toolbox (I'd imagine the toolbox should be the owner of this then) to register a location and bind callbacks when they update.

The next step would be actually plugging this into the toolbox (or target?) and then add a wrapper around bindLocation for our scenarios as we approach different tools: vanilla element in XUL (console), react element in HTML (memory), etc.
Attachment #8628504 - Attachment is obsolete: true
Attachment #8715110 - Flags: feedback?(nfitzgerald)
Attachment #8715110 - Flags: feedback?(jryans)
Attachment #8715110 - Flags: feedback?(jlong)
(In reply to Jordan Santell [:jsantell] [@jsantell] (Please needinfo) from comment #19)
> Right now thread actor emits new/updated source events,

This should definitely be the actor representing the debugging context emitting these events, not the ThreadActor, as we talked about a bit in older comments. So that would be the parent of the ThreadActor (and all the other tool-specific actors): TabActor/AddonActor/ChromeActor/WorkerActor.
Comment on attachment 8715110 [details] [diff] [review]
1177279-source-location.patch

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

Going to leave the specifics to jlongster and jryans.
Attachment #8715110 - Flags: feedback?(nfitzgerald)
The debugger still uses newSource event on the thread actor, but can emit on both, or change the debugger to listen to the parent actor. Thoughts? Would probably move the logic added in attach-thread back to the TabTarget then. Maybe expose the API via toolbox though?
(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #20)
> (In reply to Jordan Santell [:jsantell] [@jsantell] (Please needinfo) from
> comment #19)
> > Right now thread actor emits new/updated source events,
> 
> This should definitely be the actor representing the debugging context
> emitting these events, not the ThreadActor, as we talked about a bit in
> older comments. So that would be the parent of the ThreadActor (and all the
> other tool-specific actors): TabActor/AddonActor/ChromeActor/WorkerActor.

Why? These are things specific to the thread running the content, it doesn't change at all if it's a tab/addon/chrome/etc. Technically speaking that's a lot harder because we have to duplicate a lot of code across each type (they don't inherit from the same thing). It's way simpler to use the thread as the thing that everything uses (it's *always* there now), no matter what the parent implementation is.

I guess theoretically if we're only talking about event boundaries, the thread could implement all the logic but actually emit events on its parent (no matter what it is). But there are also cases where it's not just events: you have a URL and you want a SourceActor to set a breakpoint on it from the console (can't do that right now, just a thought). Right now we have a `sources` property on the tab/addon/etc instance but it seems like we should consolidate all of this somewhere that doesn't care what the target type is (we have to duplicate the `sources` property in 3 places).
(In reply to James Long (:jlongster) from comment #23)
> (In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #20)
> > (In reply to Jordan Santell [:jsantell] [@jsantell] (Please needinfo) from
> > comment #19)
> > > Right now thread actor emits new/updated source events,
> > 
> > This should definitely be the actor representing the debugging context
> > emitting these events, not the ThreadActor, as we talked about a bit in
> > older comments. So that would be the parent of the ThreadActor (and all the
> > other tool-specific actors): TabActor/AddonActor/ChromeActor/WorkerActor.
> 
> Why? These are things specific to the thread running the content, it doesn't
> change at all if it's a tab/addon/chrome/etc. Technically speaking that's a
> lot harder because we have to duplicate a lot of code across each type (they
> don't inherit from the same thing). It's way simpler to use the thread as
> the thing that everything uses (it's *always* there now), no matter what the
> parent implementation is.
> 
> I guess theoretically if we're only talking about event boundaries, the
> thread could implement all the logic but actually emit events on its parent
> (no matter what it is). But there are also cases where it's not just events:
> you have a URL and you want a SourceActor to set a breakpoint on it from the
> console (can't do that right now, just a thought). Right now we have a
> `sources` property on the tab/addon/etc instance but it seems like we should
> consolidate all of this somewhere that doesn't care what the target type is
> (we have to duplicate the `sources` property in 3 places).

The non-inheritance of the TabActor-like objects is confusing, and not only in this case.  Either they should be changed to use inheritance, or we should use a mixin technique to implement the shared functionality in one place.  We should really do _something_ to clean this up, no matter what choices we make about source maps in this specific bug.

It seems like the mixin technique could be used for this case, assuming we reach agreement that the events belong on the TabActor-like.
I plan to give more specific feedback here, but I am in meetings all day, so it may have to wait until tomorrow.
Comment on attachment 8715110 [details] [diff] [review]
1177279-source-location.patch

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

Overall, this seems good at a high level.  The only issue I see is where the actor parts of this live.  Like :fitzgen suggested, I agree that it should live on TabActor-ish actor since that actor owns the sources.

:jlong expressed concerns about putting things on TabActor-ish actors, since there are several of them, and they do not inherit things directly.  No one is asking for or wants code to be duplicated N times on each them, so we should write it once somewhere and use a mixin approach to implement it on each TabActor-ish actor.

::: devtools/server/actors/script.js
@@ +1891,5 @@
>    },
>  
> +  /**
> +   * A function called when there's a new or updated source from a thread actor's
> +   * sources. Emits `newSource` and `updatedSource` on the tab actor.

This comment says "tab actor", but it seems you are emitting on the thread actor.

We do in fact want the events to come from the TabActor-ish actor though, since it owns the sources.

(ThreadActor emits `newSource` because of compat.  It should probably be moved carefully with a trait or whatever, so this can be less confusing.)

@@ +1905,4 @@
>      });
>    },
>  
> +  onResolveLocation: Task.async(function *(request) {

The TabActor-ish actors own the sources, so this should be moved there.  (`onSources` should really live there too, but currently doesn't because of compat.  It should probably be moved carefully with a trait or whatever, so this can be less confusing.)
Attachment #8715110 - Flags: feedback?(jryans) → feedback-
Attached patch 1177279-source-location.patch (obsolete) — — Splinter Review 
So this is getting more and more complex. Nothing is shared between the {Tab|Worker|Addon}Client, so added that, but not sure where the ChromeClient equivilient is. Also, the actors themselves will need it, just did it for TabActor, looks like some parts of ChromeActor uses these. And again the traits on the front end to optionally connect to parent actor, as well as the different *Targets implementing the hook to listen. Are these all tested solidly? Last I checked, browser toolbox debugging was a bit fragile but that may have changed.

So this probably doesn't work, but more digging I did, the less I knew about how these pieces work. I don't know enough about all the parent actors to ensure all them are covered, handle trait-specific connections in all 4 parent actors, and just differences between them (some XClient's have `events` in prototype, others in the constructor, in devtools/shared/client/main.js, is there a reason for that?) and seems too fragile. But this seems like starting to get into some bigger refactorings, which I'm not comfortable starting to do in these sections of code. If I'm misunderstanding the size and it's just a matter of a mixin, can you let me know which Actors and Clients and Targets specifically need to be similar and inherit from a base parent? It's a bit confusing as the names aren't always consistent (ChromeActor = WindowTarget = ?Client?). Or if we need to handle all 4 targets from the parent from the start, much of the refactoring should be done in a separate bug.
Attachment #8715110 - Attachment is obsolete: true
Attachment #8715110 - Flags: feedback?(jlong)
Attachment #8717289 - Flags: feedback?(jryans)
Attachment #8717289 - Flags: feedback?(jlong)
Comment on attachment 8717289 [details] [diff] [review]
1177279-source-location.patch

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

Thanks for making an attempt at this clean up, it's sorely needed!  Unfortunately, I think it was wrong for me to push for you to do so in this bug.  The main reasoning I've changed my mind is:

1. I did more research to give you an exhaustive list of actors that need changing
2. One of them is WorkerActor
3. There is no "real" parent actor for the WorkerActor[1]

So, I think doing the work to move these things up to the TabActor-ish target actor becomes too much work and noise for this bug.  It is still work I think needs to be done, but not here.

I have filed bug 1247084 to implement the clean up.  I'll cancel this review and take another look at your previous patch.

For future reference, the TabActor-ish actors are documented in actor-heirarchy.md[2].  But this file is rude and does not mention workers at all.  I filed bug 1247091 to correct the docs.

Sorry again!

[1]: https://dxr.mozilla.org/mozilla-central/source/devtools/server/worker.js#58
[2]: https://dxr.mozilla.org/mozilla-central/source/devtools/server/docs/actor-hierarchy.md
Attachment #8717289 - Flags: feedback?(jryans)
Attachment #8717289 - Flags: feedback?(jlong)
Comment on attachment 8715110 [details] [diff] [review]
1177279-source-location.patch

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

Makes sense at a high level.  Still want to do another review when ready for r?.

Resetting f? for :jlong.

::: devtools/client/framework/source-location.js
@@ +5,5 @@
> +
> +const { Task } = require("resource://gre/modules/Task.jsm");
> +const { assert } = require("devtools/shared/DevToolsUtils");
> +
> +function SourceLocationController (toolbox) {

Who will require this module outside of test code?
Attachment #8715110 - Flags: feedback?(jlong)
Attachment #8715110 - Flags: feedback-
Attachment #8715110 - Flags: feedback+
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #29)
> Comment on attachment 8715110 [details] [diff] [review]
> 1177279-source-location.patch
> 
> Who will require this module outside of test code?

I imagine the *Target will create it and retain it, so consumers (tools) can access either it, or a target method, directly for binding locations and creating elements, like in the vanilla JS case:

```
let el = this.target.sourceLocation.createSourceElement({ line: 123, url: "http://localhost/file.js" });
this.consoleView.appendChild(el);
```

Also provided would be a way to make a react component for tools that use that. These would be in follow up bugs -- how does that sound?
(In reply to Jordan Santell [:jsantell] [@jsantell] (Please needinfo) from comment #30)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #29)
> > Comment on attachment 8715110 [details] [diff] [review]
> > 1177279-source-location.patch
> > 
> > Who will require this module outside of test code?
> 
> I imagine the *Target will create it and retain it, so consumers (tools) can
> access either it, or a target method, directly for binding locations and
> creating elements, like in the vanilla JS case:
> 
> ```
> let el = this.target.sourceLocation.createSourceElement({ line: 123, url:
> "http://localhost/file.js" });
> this.consoleView.appendChild(el);
> ```
> 
> Also provided would be a way to make a react component for tools that use
> that. These would be in follow up bugs -- how does that sound?

Okay, seems reasonable.  If it lives on the target, then I think "source-updated" should be emitted on the target, not the toolbox.
Attached patch 1177279-source-location.patch (obsolete) — — Splinter Review 
Ok, finally ready for a real review.

Follow up bugs in addition to what jryans mentioned are tests for `resolveLocation` on the tab actor itself, and then a few more source map specific things, like creating elements that update themselves based on the controller, and wiring the controller into the target itself (so it's actually used outside of tests).

This requires a remote TabTarget to setup the listeners -- is this always true? Do these not work in single process? Not sure if `target.makeRemote()` is optional or not.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c7ff6dd9284
Attachment #8715110 - Attachment is obsolete: true
Attachment #8715110 - Flags: feedback?(jlong)
Attachment #8718053 - Flags: review?(jryans)
Attachment #8718053 - Flags: review?(jlong)
(In reply to Jordan Santell [:jsantell] [@jsantell] (Please needinfo) from comment #32)
> Follow up bugs in addition to what jryans mentioned are tests for
> `resolveLocation` on the tab actor itself

Do we have TabActor method tests anywhere? Or these all tested implicitly through other tests? Can't find tests for the other methods (listFrames, listWorkers)
(In reply to Jordan Santell [:jsantell] [@jsantell] (Please needinfo) from comment #33)
> (In reply to Jordan Santell [:jsantell] [@jsantell] (Please needinfo) from
> comment #32)
> > Follow up bugs in addition to what jryans mentioned are tests for
> > `resolveLocation` on the tab actor itself
> 
> Do we have TabActor method tests anywhere? Or these all tested implicitly
> through other tests? Can't find tests for the other methods (listFrames,
> listWorkers)

I believe they may only be covered by integration tests.
(In reply to Jordan Santell [:jsantell] [@jsantell] (Please needinfo) from comment #32)
> This requires a remote TabTarget to setup the listeners -- is this always
> true? Do these not work in single process? Not sure if `target.makeRemote()`
> is optional or not.

Should be fine, these days makeRemote is called on all targets by someone or another, mainly the toolbox itself, but also many other places too.

It's yet another historical API artifact to clean up.
Comment on attachment 8718053 [details] [diff] [review]
1177279-source-location.patch

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

Overall, it seems good.  However, I think emitting the parent of the script actor may break in workers, so please test it.

Would like to see the next round still.  Only reviewed connections to the rest of the tools, relying on :jlong to make sense of the source mapping logic.

::: devtools/client/framework/source-location.js
@@ +27,5 @@
> +  target.on("will-navigate", this.reset);
> +  target.on("close", this.destroy);
> +}
> +
> +SourceLocationController.prototype.reset = function () {

Nit: The space before ( goes against our ESLint style.  Many more to clean up in various places.

@@ +62,5 @@
> + *
> + * @param {String} eventName
> + * @param {Object} sourceEvent
> + */
> +SourceLocationController.prototype._onSourcesUpdated = function (_, sourceEvent) {

Nit: Let's use `_onSourceUpdated` (singular) to match the event name and target's method name

::: devtools/client/framework/target.js
@@ +495,5 @@
>      this._onFrameUpdate = (aType, aPacket) => {
>        this.emit("frame-update", aPacket);
>      };
>      this.client.addListener("frameUpdate", this._onFrameUpdate);
> +   

Nit: whitespace

@@ +497,5 @@
>      };
>      this.client.addListener("frameUpdate", this._onFrameUpdate);
> +   
> +    this._onSourceUpdated = (event, packet) => this.emit("source-updated", packet);
> +    this.client.addListener("newSource", this._onSourceUpdated);

Where is `_onSourceUpdated` defined?

::: devtools/server/actors/script.js
@@ +1900,2 @@
>      this.conn.send({
> +      from: this._parent.actorID,

This may throw errors in worker debugging, which doesn't appear to have a parent actor ID.

Test the worker debugging case to see what happens.

::: devtools/server/actors/webbrowser.js
@@ +1919,5 @@
> +
> +      return this.sources.getOriginalLocation(generatedLocation).then(loc => {
> +        // If no map found, return this packet
> +        if (loc.originalLine == null) {
> +          return { from: this.actorID, type: "resolveLocation", status: "MAP_NOT_FOUND" };

Is this an error state?  The protocol defines error replies as having an "error" property, should the "status" be "error" instead?

@@ +1928,5 @@
> +      });
> +    }
> +
> +    // Fall back to this packet when source is not found
> +    return promise.resolve({ from: this.actorID, type: "resolveLocation", status: "SOURCE_NOT_FOUND" });

Is this an error state?  The protocol defines error replies as having an "error" property, should the "status" be "error" instead?

::: devtools/shared/client/main.js
@@ +1370,5 @@
>  
>    attachWorker: function (aWorkerActor, aOnResponse) {
>      this.client.attachWorker(aWorkerActor, aOnResponse);
> +  },
> +  

Nit: whitespace
Attachment #8718053 - Flags: review?(jryans) → review-
When debugging workers, I didn't notice any issues -- it seemed that workers weren't even hitting that line of code.
Comment on attachment 8721574 [details] [diff] [review]
1177279-source-location.patch

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

I believe this version looks good as far as wiring things up.  Still needs :jlong for source maps.
Attachment #8721574 - Flags: review?(jryans) → review+
Comment on attachment 8721574 [details] [diff] [review]
1177279-source-location.patch

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

This generally looks good, but I don't see how this would work in the scenario where a source already exists with a sourcemap applied, and a new location is logged to the console. In the debugger, I believe we will essentially be sending already-sourcemapped locations, but that's not possible with the console. Or is the intention, when we integrate it, that we can check if it's already been sourcemapped and we will manually call `resolveLocation` in the console itself?

Thanks so much for working on this, and sorry for the slow review!

::: devtools/client/framework/source-location.js
@@ +52,5 @@
> + */
> +SourceLocationController.prototype.bindLocation = function(location, callback) {
> +  assert(location.url, "Location must have a url.");
> +  assert(location.line, "Location must have a line.");
> +  this.locations.add({ location, callback });

How is this going to work when integrating in the console? This will still add new entries for each location in the UI, because you will have to bind a new `callback` to update that specific element, even if it's the same location used in previous entries. Or does the frame component help with that?

We should definitely make sure, however it's implemented, that the same location is only sourcemapped once, even if it's logged 1000 times.

@@ +66,5 @@
> +SourceLocationController.prototype._onSourceUpdated = function(_, sourceEvent) {
> +  let { type, source } = sourceEvent;
> +  // If we get a new source, and it's not a source map, abort;
> +  // we can ahve no actionable updates as this is just a new normal source.
> +  // Also abort if there's no `url`, which means it's unsourcemappable anyway,

That's not entirely true. An eval script can theoretically embed a sourcemap inline using a data URI (actually, there's no reason it can't have a normal sourceMapURL either but it's far more likely to dynamically generate it and embed it). I believe the debugger supports this properly, but doesn't show sources without a URL by default (you can give an eval source a URL with the `sourceURL` pragma). However, if you have a `debugger` statement in an eval script, or have "break on exceptions" turned on, the debugger might still pause inside one of these unnamed eval scripts. In that case, the frontend gives it an arbitrary name (like "SCRIPT1", Chrome uses "VM1") and adds the source to the UI and shows it paused in the right place.

I *think* if it has a sourcemap the frontend should treat it like any other source, and it'll work.

I just tested what the console logs when I do `eval('\nconsole.log("hi");\n');` inside a file named `foo.js`. It logs the raw URL returned from the engine: "foo.js line 3 > eval" which is wrong. We need to decide a uniform way to display the "lack of a URL" scenario. What is going to happen with your path? The URL will just error when trying to resolve it?

The fundamental conflict is that this API takes URL instead of a SourceActor ID, but I think that's OK. For this to work with most tools we pretty much have to do that. The performance tool is never going to use SourceActors.

After thinking a bit more about this, I think the `canonicalID` property of sources might help with this. That property is a unique ID that represents the script at the engine level. If we can encode that in the URL that tools get from the engine, we can use it to map to a unique script in the debugger and get a sourcemap for it. The nice thing about having that everywhere is that linking to the debugger would *always* work: click on a location and it would use the ID to open the debugger, not a URL, which famously conflicts and is hard to figure out the right place to go.

Just wanted to explain everything, this is long-term thinking. No need to do this right now. Note that this will willingly receive wrong URLs like "foo.js line 3 > eval" though, which is probably fine (it'll just error).

@@ +128,5 @@
> +function isSourceRelated(location, source) {
> +         // Mapping location to subsequently loaded source map
> +  return source.generatedUrl === location.url ||
> +         // Mapping source map loc to source map
> +         source.url === location.url

Can you add a comment here as to why we can't just check `generatedUrl`? It seems like we could only check for that, and look for locations that are actually sourcemapped. But the reason we can't do that is because of pretty-printing: in that scenario, a source is dynamically converted to and from a sourcemapped source. I don't know, at least explaining the pretty-printing scenario seems like a good thing to have in comments.

::: devtools/server/actors/script.js
@@ +1895,5 @@
> +   *
> +   * @param {String} name
> +   * @param {SourceActor} source
> +   */
> +  onSourceEvent: function (name, source) {

+1 I like renaming this, `onNewSource` sounded like it was a Debugger API hook.
Comment on attachment 8721574 [details] [diff] [review]
1177279-source-location.patch

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

::: devtools/client/framework/source-location.js
@@ +52,5 @@
> + */
> +SourceLocationController.prototype.bindLocation = function(location, callback) {
> +  assert(location.url, "Location must have a url.");
> +  assert(location.line, "Location must have a line.");
> +  this.locations.add({ location, callback });

Caching will definitely have to be done -- that can be done for the integration, but as long as file:line:column's are unique (and you mentioned in some cases, this wouldn't be true, if loading a script that was already loaded, but that is more fresh), this would work

@@ +66,5 @@
> +SourceLocationController.prototype._onSourceUpdated = function(_, sourceEvent) {
> +  let { type, source } = sourceEvent;
> +  // If we get a new source, and it's not a source map, abort;
> +  // we can ahve no actionable updates as this is just a new normal source.
> +  // Also abort if there's no `url`, which means it's unsourcemappable anyway,

All great points and insight -- once we have all these possible source map scenarios, we can test them, and I don't think that'll change the architecture of this though. Is there a list of possible source map/source representation types?

@@ +128,5 @@
> +function isSourceRelated(location, source) {
> +         // Mapping location to subsequently loaded source map
> +  return source.generatedUrl === location.url ||
> +         // Mapping source map loc to source map
> +         source.url === location.url

I believe the second conditional is for pretty printed sources -- but that should be mentioned in the comments
Comment on attachment 8721574 [details] [diff] [review]
1177279-source-location.patch

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

Can we change this patch to just include the changes on the back end? actors/script.js, actors/webbrowser.js, TabSources.js? If it means faster landing, since the SLC will need to be changed API-wise for the frame component anyway?
It's up to you, I'm fine r+'ing this patch as long as there' is a follow-up bug.
Attachment #8721574 - Flags: review?(jlong) → review+
https://hg.mozilla.org/mozilla-central/rev/4adc4cef8117
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: