Closed Bug 905700 (dbg-source) Opened 11 years ago Closed 9 years ago

Debugger.Source Integration

Categories

(DevTools :: Debugger, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 36

People

(Reporter: fitzgen, Assigned: jlong)

References

(Blocks 3 open bugs)

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 8 obsolete files)

We have the basics of Debugger.Source, and we can start integrating with the debugger server.

Should just need to load source text from Debugger.Source instead of fetching again.

Might need to assign phony URLs to eval scripts when we send the newSource packet.
OS: Mac OS X → All
Priority: -- → P3
Hardware: x86 → All
Version: 25 Branch → Trunk
(In reply to Nick Fitzgerald [:fitzgen] from comment #0)
> Might need to assign phony URLs to eval scripts when we send the newSource
> packet.

We can do better than that, right? Since we're now sending a source actor form everywhere we used to be sending a URL, we can represent sources that have no URL as... sources with no URL!

What's missing is the ability to set breakpoints by source actor, which we should definitely have.
I don't remember the exact details, but after talking with Benvie the other day, we figured we probably want to keep breakpoints as a per URL thing, and give eval scripts "URLs" like "eval://1" or something, and scratchpad-in-the-debugger scripts "URLs" like "scratchpad://<name of scratchpad>".

This way, we can maintain the same the breakpoints for a given scratchpad-in-the-debugger, even though editing and re-evaling will create new Debugger.Source instances.

Perhaps there is a better way?
(In reply to Nick Fitzgerald [:fitzgen] from comment #2)
> I don't remember the exact details, but after talking with Benvie the other
> day, we figured we probably want to keep breakpoints as a per URL thing, and
> give eval scripts "URLs" like "eval://1" or something, and
> scratchpad-in-the-debugger scripts "URLs" like "scratchpad://<name of
> scratchpad>".
> 
> This way, we can maintain the same the breakpoints for a given
> scratchpad-in-the-debugger, even though editing and re-evaling will create
> new Debugger.Source instances.
> 
> Perhaps there is a better way?

Yeah, this was pretty much what we talked about. The idea is that we want to be able to evaluate the same source multiple times without destroying the context, and have the script maintain the same identity on the client side.
Assignee: nobody → nfitzgerald
Priority: P3 → P2
Depends on: 917579
Depends on: 833618
Blocks: 833618
No longer depends on: 833618
Depends on: 918053
Depends on: 917963
Depends on: 920281
No longer blocks: 833618
Depends on: 935203
Depends on: 935373
Attached image sources-diagram.jpg
This is a little diagram I drew to illustrate what the relationships between SourceActor, Debugger.Source, and URLs, the BreakpointStore.

A SourceActor has one "current" Debugger.Source that represents the most up to date D.S instance for that source, and many "old" D.S instances from past reloads or evals (in the case of a scratchpad, for example). The old evals will probably be referenced via weakmap so we don't just keep them alive forever. We need to know about old D.S instances so we can show the "(vm)" or "(old)" tag when an old version of a source is on the stack for whatever reason.

A SourceActor's breakpoints will go into the BreakpointStore only if it is considered a "persistent" source. A source is "persistent" if it has one of the following properties:

* displayURL
* sourceMappingURL
* "real" URL (aka not inherited from an eval or dynamic script tag injection)

We will use the property that makes it persistent as the "url" key in breakpoint locations in the BreakpointStore.
Assignee: nobody → bbenvie
Status: NEW → ASSIGNED
Blocks: 991179
Assignee: bbenvie → nobody
Depends on: 939636
Blocks: 1036866
Blocks: 1010150
Depends on: 1037675
Summary: use Debugger.Source in the debugger server → [meta] use Debugger.Source in the debugger server
Whiteboard: [status:inflight]
Assignee: nobody → jlong
Depends on: 1061961
I'm in the process of trying to organize all the debugger bugs. I hope you don't mind if I rename this meta bug to make it a bit easier for me to find.
Summary: [meta] use Debugger.Source in the debugger server → [meta] Debugger.Source
No longer blocks: 1005542
Depends on: 1005542
Blocks: 863078
No longer blocks: 1036866
Depends on: 1056409
I have all mochitests & xpcshell tests passing with my patch which implements this. I am now doing a little refactoring to make the server much clearer about what's going on (I learned a lot while fixing all the tests). I'll post patches here soon.
Alias: dbg-source
Attached patch backend.patch (obsolete) — Splinter Review
Attachment #8507073 - Flags: review?(nfitzgerald)
Attached patch frontend.patch (obsolete) — Splinter Review
Attachment #8507074 - Flags: review?(nfitzgerald)
Attached patch mochitests-update.patch (obsolete) — Splinter Review
Attachment #8507075 - Flags: review?(nfitzgerald)
The 3 above patches update the the client and server to use Debugger.Source's instead of urls for everything. It's a lot of changes, so let me know how I can help the review process. There are a lot of changes to the mochitests, so I included that as a separate patch. You should be able to apply these in any order.

You will need to apply the patch from bug 1074745 first.

This code moves us into a better position for handling lots of stuff. Eval debugging is the big one of course. The code that handles sourcemapping has been ruthlessly simplified as much as possible (code flow-wise), so now you can call `getOriginalLocation` from anywhere (across debuggers for example) and it will properly work (and only parse the same sourcemaps once). If a source is pretty-printed is should Just Work.

The frontend required lots of little changes to key the sources off of actor id instead of url, while the backend required quite a bit of refactoring. And of course it broke tons of mochitests so I had to fix all of them (and added some helper functions in head.js).

This is just a first step, and there are still things we need to do:

* Most importantly, I need to add a bunch of tests to test the eval debugging. I haven't added any tests yet, but now that the current code is working I can do that. I wanted to get the review going though because there's so much to review. This won't land until I get tests done.

* Some of the functions (like `getSourceURL` in scripts.js) specifically check the `introductionType` of the source act differently if it's an eval source. Right now I think they are only checking for `eval`, when they should also check for `function` and other eval types. Need to do this before landing this.

* Pretty-printing and blackboxing preferences are still keyed off URL. I'm actually not entirely sure what happens right now if you try it on an eval source; I think it will work but it won't remember the pref and automatically do it later. This can be a followup bug.

* How eval sources are displayed in the sources pane was done quickly and it might clog it up too easily. We should rethink how they are displayed. Followup bug.

* We need to send `displayURL` in the SourceActor form so that the UI can do better stuff with it. Followup bug.
Also: basic backwards compatibility is in these patches. If you look in `setBreakpoint` in the dbg-client, it checks a trait that tells us if the server supports D.S or not, and sends the request to the thread instead of the source if it doesn't. Additionally there are a few places in the frontend that handle backwards compat. I was able to connect to an old Firefox and set breakpoints, but it hasn't been heavily tested. Unfortunately I don't know of a way to heavily test it :(
Made a push to try to gauge how many failures there are: https://tbpl.mozilla.org/?tree=Try&rev=86eca41e6ce0
Crap. Looks like there's a bunch of other tests I need to fix. I doubt it's nearly as much as the debugger, but looks like other tools like console, etc test integration with debugger and I need to fix those. I hadn't run those locally.
A quick thought about these patches: I tried to normalize some styles but let know if I changed things wrongly (for example, using backticks in comments to denote code instead of pipes). Also I used `let` by default but if we are moving to using `const` I can change that too.
Comment on attachment 8507073 [details] [diff] [review]
backend.patch

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

Looks good in general. Comments below. Want to see it again once you've updated the patch.

Need more documentation on the "hard" stuff, what the difference is with and without the hard versions, when to use the hard flags, and why.

::: toolkit/devtools/client/dbg-client.jsm
@@ +2478,5 @@
> +      };
> +
> +      // Backwards compatibility: send the breakpoint request to the
> +      // thread if the server doesn't support Debugger.Source actors
> +      if (!root.traits.sourceActors) {

Nite: "sourceActors" seems like a little bit of a misnomer to me. We've had SourceActors for a while, and reading this I would assume this trait is about those. Maybe "DebuggerSourceActors"? "supportsEvals"? "breakpointsOnSourceActors"?

@@ +2493,5 @@
> +            this._client,
> +            this,
> +            aResponse.actor,
> +            location,
> +            root.traits.conditionalBreakpoints ? condition : undefined

We didn't do this before (did we even have the conditionalBreakpoints trait?), why is it necessary now? Wouldn't the condition just be ignored by the server, rather than explicitly ignored by the client? Seems we could avoid this little complication by letting old servers ignore the condition and we would get the exact same behavior we have here.

@@ +2522,5 @@
> +            ? () => this._activeThread.resume()
> +            : noop;
> +
> +      doSetBreakpoint(cleanUp);
> +

Nit: no blank line at the end of the function, please.

@@ +2540,5 @@
>   *        url and line.
>   * @param aCondition string
>   *        The conditional expression of the breakpoint
>   */
> +function BreakpointClient(aClient, aSourceClient, aActor, aLocation, aCondition) {

Please update the doc comment with the new `aSourceClient` parameter.

@@ +2621,5 @@
>            deferred.reject(aResponse);
>            return;
>          }
>  
> +        let source = gThreadClient.source({ actor: this.location.actor })

ThreadClient.prototype.source is meant to take a proper source form, and in this case if (sometime in the future) we ever try to ask the created source client about the source's black-boxed-ness, we won't get the correct result because of the mocked source form.

But the BreakpointClient is initialized with a SourceClient, but it just doesn't keep it around. It seems like it would be easier/cleaner to just keep the SourceClient around and not have to mock a phony source form just to get the SourceClient back again (which is cached anyways!).

::: toolkit/devtools/server/actors/script.js
@@ +171,5 @@
>      dbg_assert(line != null);
>  
>      var foundBreakpoint = this.hasBreakpoint(aLocation);
>      if (foundBreakpoint == null) {
> +      throw new Error("No breakpoint at = " + (url || actor)

Wait why are we still using url here, but not in any of the other BreakpointStore methods? This looks wrong...

@@ +227,3 @@
>      }
>  
> +    for (let actor of this._iterActor(aSearchParams.source && aSearchParams.source.actor)) {

Nit: > 80 chars on this line.

Also, it wasn't immediately clear to me that we are relying on a kind of implicit ternary operator behavior here. I think this would read much better as the following, and get within 80 chars.

  const actorParam = aSearchParams.source ? aSearchParams.source.actor : null;
  for (let actor of this._iterActors(actorParam)) ...

@@ +241,5 @@
>        }
>      }
>    },
>  
> +  _iterActor: function* (actor) {

Super nit: let's rename this `_iterActors` so it fits with `_iterLines` and `_iterColumns`.

@@ +321,5 @@
> +SourceActorStore.prototype = {
> +  /**
> +   * Lookup an existing actor id that represents this source, if available
> +   */
> +  getReusableId: function(aSource, aOriginalUrl) {

I think we need to use more consistent/clear terminology here (as I mention below). The SourceActorStore is mapping one kind of ID to another kind of ID and we should be explicit about which IDs we are talking about at any given time.

So that was a round about way to suggest that we rename this method to `getReusableActorId`.

@@ +342,5 @@
> +
> +  /**
> +   * Make a unique URL from a source that identifies it across reloads
> +   */
> +  getUniqueURL: function(aSource, aOriginalUrl) {

URLs are pretty concrete; not really something we can just make up (and what we are returning here isn't necessarily a valid URL, displayName can be anything (plus, I don't think we are using this as a URL anyways; just as a key, right?)).

How about renaming this `getUniqueKey` or `getPersistentKey`?

@@ +352,5 @@
> +      // For eval sources, forcefully return null if they don't have
> +      // these special URLs. Right now eval sources have a
> +      // `source.url` property when they shouldn't.
> +      let url = aSource.displayURL || aSource.sourceMapURL;
> +      if (!url && aSource.introductionType !== 'eval') {

Wouldn't we want to persist eval'd sources with the same sourceMapURL or displayURL? A module loader that uses displayURL is typically also using eval, and it would be nice to persist breakpoints for those sources.

@@ +828,5 @@
>  
> +      let loc = getFrameLocation(aFrame);
> +      this.sources.getOriginalLocation(loc).then(aOrigPosition => {
> +        packet.frame.where = {
> +          source: aOrigPosition.sourceActor && aOrigPosition.sourceActor.form(),

When will we ever not have a source actor for a frame? Self-hosted functions?

Can you avoid using && when your result isn't supposed to be a boolean?

  const sourceForm = aOrigPosition.sourceActor
    ? aOrigPosition.sourceActor.form()
    : null;

@@ +1967,2 @@
>        generatedLocation));
> +    const url = sourceActor && sourceActor.url;

? :

@@ +1999,2 @@
>        generatedLocation));
> +    const url = sourceActor && sourceActor.url;

? :

@@ +2276,5 @@
> + * sources are sourcemapped. When a source is sourcemapped, actors are
> + * created for both the "generated" and "original" sources, and the client will
> + * only see the original sources. We separate these because there isn't
> + * a 1:1 mapping of generated to original sources; one generated source
> + * may represent 10 original sources, so we need to create 11 separate

Nitty nitty nit nit: s/10/N/ and s/11/N + 1/ here and below.

@@ +2280,5 @@
> + * may represent 10 original sources, so we need to create 11 separate
> + * actors.
> + *
> + * There are 4 different scenarios for sources that you should
> + * understand:

Five bullets scenarios listed below :P

@@ +2282,5 @@
> + *
> + * There are 4 different scenarios for sources that you should
> + * understand:
> + *
> + * - A normal source

What is a "normal" source?

Either (1) a source referred to via a <script> tag with a "src" attribute or (2) a <script> tag with inlined text content?

I think we should be explicit and list these cases out here (or at least define what a "normal" script is).

@@ +2364,5 @@
> +    // treat HTML pages with inline scripts as a special SourceActor
> +    // that doesn't have either
> +    let introductionUrl = (source &&
> +                           source.introductionScript &&
> +                           source.introductionScript.source.url);

Nit: pls no &&

@@ +2434,5 @@
> +    let toResolvedContent = t => {
> +      return {
> +        content: t,
> +        contentType: this._contentType
> +      };

Frank Nitti:

  t => ({ ... })

@@ +2482,5 @@
>      let packet = {
>        from: this.actorID
>      };
>  
> +    return resolve(null).then(() => {

Whoa whoa whoa, why are we doing `resolve(null)`? Just to let a single tick of the event loop happen? Why?

This is both confusing and scaring me...

@@ +2504,2 @@
>        }
> +      else {

Nit: let's just remove this else b/c the if always returns. We can get rid of some indentation here.

@@ +5108,5 @@
> +  this._sourceMapCache = Object.create(null);
> +  // Debugger.Source -> SourceActor
> +  this._sourceActors = new Map();
> +  // url -> SourceActor
> +  this._sourcemappedSourceActors = Object.create(null);

Nit: _sourceMappedSourceActors (capitalize the M in mapped)

@@ +5138,3 @@
>     */
> +  source: function  ({ source, originalUrl, generatedSource,
> +              isInlineSource, contentType }) {

funky indentation here; line up isInlineSource with source.

@@ +5174,5 @@
> +      // Not all "original" scripts are distinctly separate from the
> +      // generated script. Pretty-printed sources have a sourcemap for
> +      // themselves, so we need to make sure there a real source
> +      // doesn't already exist with this URL.
> +      for(let [source, actor] of this._sourceActors) {

for_(

@@ +5216,5 @@
> +
> +    // Don't notify a new source if it's a generated one, as it has
> +    // sourcemapped sources. The generated one is created to set
> +    // breakpoints.
> +    if (!source || !this._sourceMaps.has(source)) {

Does this do the correct thing when we start source mapping, but then it is disabled later (so this._sourceMaps.has(source) could be true)?

Do we want a `this._useSourceMaps &&` prefix on the conditional?

@@ +5242,5 @@
> +  },
> +
> +  getSourceByURL: function(url) {
> +    if (url) {
> +      for(let [source, actor] of this._sourceActors) {

for_(

@@ +5290,5 @@
> +    // if the source is an HTML-embedded <script> tag. Since we don't
> +    // have an API implemented to detect whether this is the case, we
> +    // need to be conservative and only treat valid js files as real
> +    // sources. Otherwise, use the `originalUrl` property to treat it
> +    // as an HTML source that manages multiple inline sources.

Is this still true? Doesn't introductionType give us this info? Can we simplify this logic?

@@ +5311,5 @@
>  
>    /**
>     * Return a promise of an array of source actors representing all the
>     * sources of |aScript|.
> +

Remove this change.

@@ +5328,5 @@
> +          return [
> +            this.source({ originalUrl: s,
> +                          generatedSource: aScript.source })
> +            for (s of map.sources)
> +            ];

-2 space indent on "];"

@@ +5330,5 @@
> +                          generatedSource: aScript.source })
> +            for (s of map.sources)
> +            ];
> +        }
> +        else {

Remove else since we always return in the if-branch.

@@ +5340,5 @@
>  
>    /**
>     * Return a promise of a SourceMapConsumer for the source map for
> +   * `aSource`; if we already have such a promise extant, return that.
> +   * This should only be called if you really know what you're doing,

"unless you really know what you are doing" seems unnecessarily scary and doesn't help educate the reader as to when they would really know what they're doing, etc.

Can we say something along the lines of "Use this instead of `getSourceMap` for 'internal' source maps that we want to use even when the source map pref isn't set (pretty printing, for example) ... blah blah blah. If you're unsure, just use `getSourcemap`."

@@ +5436,5 @@
>      return Services.io.newURI(
>        ".", null, Services.io.newURI(aPath, null, null)).spec;
>    },
>  
> +  clearSourceMapCache: function(aSourceMapURL, opts = { hard: false }) {

This definitely needs a doc comment. When do I use `hard` or not?

@@ +5445,5 @@
> +    }
> +
> +    if (oldSm) {
> +      // Clear out the current cache so all sources will get the new one
> +      for(let [source, sm] of this._sourceMaps.entries()) {

for_(

@@ +5453,5 @@
> +      }
> +    }
> +  },
> +
> +  setSourceMapHard: function(aSource, aUrl, aMap) {

Needs a doc comment and should explain when/why I should use this instead of `setSourceMap`

@@ +5502,5 @@
> +          column: sourceCol,
> +          name: sourceName
> +        };
> +      }
> +      else {

Remove the else branch since the if will always return.

@@ +5549,5 @@
> +          line: genLine,
> +          column: genColumn
> +        };
> +      }
> +      else {

Remove else branch because if always returns.

@@ +5661,5 @@
> +/*
> + * Returns a new object that has all of the properties of `obj1` and
> + * `obj2` combined.
> + */
> +function merge(obj1, obj2) {

Can we make `update` support this use case, like we discussed in the other bug?

::: toolkit/devtools/server/tests/unit/head_dbg.js
@@ +15,5 @@
>  
>  const Services = devtools.require("Services");
>  // Always log packets when running tests. runxpcshelltests.py will throw
>  // the output away anyway, unless you give it the --verbose flag.
> +Services.prefs.setBoolPref("devtools.debugger.log", false);

Why this change?

::: toolkit/devtools/transport/transport.js
@@ +547,5 @@
> +              dump(k + ' ');
> +            }
> +            dump('\n');
> +            throw e;
> +          }

Why this change?
Attachment #8507073 - Flags: review?(nfitzgerald) → feedback+
Comment on attachment 8507074 [details] [diff] [review]
frontend.patch

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

Looks good. Want to see it again after all of the backwards compat is there.

::: browser/devtools/debugger/debugger-commands.js
@@ +56,5 @@
> +  let items = dbg._view.Sources.items;
> +  return items.reduce(function(acc, item) {
> +    let source = item.attachment.source;
> +    if (source.url) {
> +      acc.push({

I think this would be more clear as a filter + map:

return items
  .filter(i => !!i.attachment.source.url)
  .map(i => ({
    name: i.attachment.source.url,
    value: i.attachment.source.actor
  }));

::: browser/devtools/debugger/debugger-controller.js
@@ +652,5 @@
>    _refillFrames: function() {
>      // Make sure all the previous stackframes are removed before re-adding them.
>      DebuggerView.StackFrames.empty();
> +
> +    // TODO: need to be backwards compatible

It would be nice if we encapsulated all the compatibility munging at the client-layer (where we have traditionally done it) and the UI/frontend didn't have to muddy itself up.

Is there a reason we can't do it there in this case?

@@ +1396,5 @@
>  
>      // Can't use promise.all, because if one fetch operation is rejected, then
>      // everything is considered rejected, thus no other subsequent source will
>      // be getting fetched. We don't want that. Something like Q's allSettled
>      // would work like a charm here.

Aside/pre-existing: sigh... why didn't we just "polyfill" allSettled?

@@ +1990,3 @@
>          aBreakpointClient.requestedLocation = aLocation;
> +        aBreakpointClient.location = actualLoc;
> +        aBreakpointClient.location.actor = actualLoc.source && actualLoc.source.actor;

?:

::: browser/devtools/debugger/debugger-panes.js
@@ +153,5 @@
> +        label = "unknown";
> +        group = "unknown";
> +      }
> +      unicodeUrl = "eval";
> +    }

Can we split this out to a helper function (possibly in a follow up, if you prefer)

  let { label, group, unicodeUrl } = getLabelStuff(...);

@@ +574,5 @@
> +   *        - url: a url (might be null)
> +   */
> +  getActorForLocation: function(aLocation) {
> +    if (aLocation.url) {
> +      for(var item of this) {

for_(

let item

@@ +1337,5 @@
>  
>      const data = traceItem.attachment.trace;
>      const { location: { url, line } } = data;
> +    DebuggerView.setEditorLocation(
> +      DebuggerView.Sources.getActorForLocation({ url: url }),

{ url }
Attachment #8507074 - Flags: review?(nfitzgerald) → feedback+
Comment on attachment 8507075 [details] [diff] [review]
mochitests-update.patch

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

Looks good, but we can't just remove a ton of assertions browser_dbg_stack-05.js. r=me with that test coverage added back.

::: browser/devtools/debugger/test/browser_dbg_addon-sources.js
@@ +4,5 @@
>  // Ensure that the sources listed when debugging an addon are either from the 
>  // addon itself, or the SDK, with proper groups and labels.
>  
>  const ADDON_URL = EXAMPLE_URL + "addon3.xpi";
> +let gClient;

Is this necessary? Doesn't seem to be used elsewhere.

::: browser/devtools/debugger/test/browser_dbg_stack-05.js
@@ -80,5 @@
> -      is(gEditor.getDebugLocation(), 5,
> -        "Editor debug location is correct.");
> -
> -      deferred.resolve();
> -    });

These are a lot of assertions that don't exist anymore... Why did you remove this? It seems like this is reducing our test coverage.

@@ -123,5 @@
> -        is(gEditor.getDebugLocation(), 4,
> -          "Editor debug location is correct.");
> -
> -        deferred.resolve();
> -      });

Ditto.
Attachment #8507075 - Flags: review?(nfitzgerald) → review+
(In reply to Nick Fitzgerald [:fitzgen] from comment #17)
> Comment on attachment 8507073 [details] [diff] [review]
> backend.patch
> 
> Review of attachment 8507073 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good in general. Comments below. Want to see it again once you've
> updated the patch.
> 
> Need more documentation on the "hard" stuff, what the difference is with and
> without the hard versions, when to use the hard flags, and why.

Yeah, those sourcemapping functions were added last when I was busy trying to get this done. Will add a bunch more comments.

If I didn't reply to something below you can assume I'll fix it.

> @@ +2493,5 @@
> > +            this._client,
> > +            this,
> > +            aResponse.actor,
> > +            location,
> > +            root.traits.conditionalBreakpoints ? condition : undefined
> 
> We didn't do this before (did we even have the conditionalBreakpoints
> trait?), why is it necessary now? Wouldn't the condition just be ignored by
> the server, rather than explicitly ignored by the client? Seems we could
> avoid this little complication by letting old servers ignore the condition
> and we would get the exact same behavior we have here.

We did do it before, it was just copied over: https://github.com/mozilla/gecko-dev/blob/master/toolkit/devtools/client/dbg-client.jsm#L1773.

It's not as much about ignoring it on the server, it's just that the BreakpointClient instance shouldn't have it set. I remember that it needs to be very explicit for some reason; note that `hasCondition` carefully checks the property: https://github.com/mozilla/gecko-dev/blob/master/toolkit/devtools/client/dbg-client.jsm#L2565. You are right that we probably could relax this a little bit, but this is all just copied from how it works before and I don't want to risk introducing unrelated bugs.

> 
> @@ +2621,5 @@
> >            deferred.reject(aResponse);
> >            return;
> >          }
> >  
> > +        let source = gThreadClient.source({ actor: this.location.actor })
> 
> ThreadClient.prototype.source is meant to take a proper source form, and in
> this case if (sometime in the future) we ever try to ask the created source
> client about the source's black-boxed-ness, we won't get the correct result
> because of the mocked source form.
> 
> But the BreakpointClient is initialized with a SourceClient, but it just
> doesn't keep it around. It seems like it would be easier/cleaner to just
> keep the SourceClient around and not have to mock a phony source form just
> to get the SourceClient back again (which is cached anyways!).

Good point, done!

> 
> ::: toolkit/devtools/server/actors/script.js
> @@ +171,5 @@
> >      dbg_assert(line != null);
> >  
> >      var foundBreakpoint = this.hasBreakpoint(aLocation);
> >      if (foundBreakpoint == null) {
> > +      throw new Error("No breakpoint at = " + (url || actor)
> 
> Wait why are we still using url here, but not in any of the other
> BreakpointStore methods? This looks wrong...

Look at the variable declaration; we destructure both `actor` and `url` off the source form, and will print the url of the source in the error if it's available. This is just for better error messages. This is the only method that throws an error.

> 
> @@ +227,3 @@
> >      }
> >  
> > +    for (let actor of this._iterActor(aSearchParams.source && aSearchParams.source.actor)) {
> 
> Nit: > 80 chars on this line.
> 
> Also, it wasn't immediately clear to me that we are relying on a kind of
> implicit ternary operator behavior here. I think this would read much better
> as the following, and get within 80 chars.
> 
>   const actorParam = aSearchParams.source ? aSearchParams.source.actor :
> null;
>   for (let actor of this._iterActors(actorParam)) ...
> 

Hm, I disagree (not about the >80, I'll definitely split it up). The fact that conditions return the result of the last expression is useful and very common for things like this. It's the same in several other languages (python, scheme, etc) and is purposefully done for this reason. Most of the time you only care if something is truthy or not, but I could see when passing values across the protocol as a case where you do want to enforce `null`. But in local functions like this... I think it's a lot less noise. What do others (past, dcamp) think about this style?

It's complementary to the style of using `||`: `let x = foo || bar`. I think `let x = foo ? foo : bar` is a bit noisier.

> @@ +321,5 @@
> > +SourceActorStore.prototype = {
> > +  /**
> > +   * Lookup an existing actor id that represents this source, if available
> > +   */
> > +  getReusableId: function(aSource, aOriginalUrl) {
> 
> I think we need to use more consistent/clear terminology here (as I mention
> below). The SourceActorStore is mapping one kind of ID to another kind of ID
> and we should be explicit about which IDs we are talking about at any given
> time.
> 
> So that was a round about way to suggest that we rename this method to
> `getReusableActorId`.

+1

> 
> @@ +342,5 @@
> > +
> > +  /**
> > +   * Make a unique URL from a source that identifies it across reloads
> > +   */
> > +  getUniqueURL: function(aSource, aOriginalUrl) {
> 
> URLs are pretty concrete; not really something we can just make up (and what
> we are returning here isn't necessarily a valid URL, displayName can be
> anything (plus, I don't think we are using this as a URL anyways; just as a
> key, right?)).
> 
> How about renaming this `getUniqueKey` or `getPersistentKey`?

+1

> 
> @@ +352,5 @@
> > +      // For eval sources, forcefully return null if they don't have
> > +      // these special URLs. Right now eval sources have a
> > +      // `source.url` property when they shouldn't.
> > +      let url = aSource.displayURL || aSource.sourceMapURL;
> > +      if (!url && aSource.introductionType !== 'eval') {
> 
> Wouldn't we want to persist eval'd sources with the same sourceMapURL or
> displayURL? A module loader that uses displayURL is typically also using
> eval, and it would be nice to persist breakpoints for those sources.


That logic does that (it should). It tries to get a url (or "key") from `displayURL` or `sourceMapURL` and if that doesn't work, and it IS eval, then it returns null. This code could be restructured to make that more clear.

> @@ +828,5 @@
> >  
> > +      let loc = getFrameLocation(aFrame);
> > +      this.sources.getOriginalLocation(loc).then(aOrigPosition => {
> > +        packet.frame.where = {
> > +          source: aOrigPosition.sourceActor && aOrigPosition.sourceActor.form(),
> 
> When will we ever not have a source actor for a frame? Self-hosted functions?

A few of these quick checks exist to get tests passing. Tests showed some rare edge cases with sourcemapping and I can't remember exactly what is was. However, I think I refactored some of the sourcemapping after adding some of these so I'll remove it and see if everything still works (and tests pass). `getOriginalLocation` should always return valid info now.

> Can you avoid using && when your result isn't supposed to be a boolean?

I'd like to talk about this more... In cases where you just care if it's truthy or not I like it a lot more.

> 
> @@ +2280,5 @@
> > + * may represent 10 original sources, so we need to create 11 separate
> > + * actors.
> > + *
> > + * There are 4 different scenarios for sources that you should
> > + * understand:
> 
> Five bullets scenarios listed below :P

Haha, there were actually only 4 but I added an extra bullet on the last line.

> 
> @@ +2282,5 @@
> > + *
> > + * There are 4 different scenarios for sources that you should
> > + * understand:
> > + *
> > + * - A normal source
> 
> What is a "normal" source?
> 
> Either (1) a source referred to via a <script> tag with a "src" attribute or
> (2) a <script> tag with inlined text content?
> 
> I think we should be explicit and list these cases out here (or at least
> define what a "normal" script is).

Definitely. A normal source is a non-sourcemapped piece of JavaScript that is not inlined with the HTML page. (anything eval, new Function, external JS file falls under this). I fleshed out the comment.

> @@ +2364,5 @@
> > +    // treat HTML pages with inline scripts as a special SourceActor
> > +    // that doesn't have either
> > +    let introductionUrl = (source &&
> > +                           source.introductionScript &&
> > +                           source.introductionScript.source.url);
> 
> Nit: pls no &&

This is a great example of why `&&` is better. To use ternary here would explode the code because I need to do 2 checks.

> 
> @@ +2434,5 @@
> > +    let toResolvedContent = t => {
> > +      return {
> > +        content: t,
> > +        contentType: this._contentType
> > +      };
> 
> Frank Nitti:
> 
>   t => ({ ... })

I'm confused, is that even valid? This is using the fat arrow syntax where the braces are defining the body of the code.

> 
> @@ +2482,5 @@
> >      let packet = {
> >        from: this.actorID
> >      };
> >  
> > +    return resolve(null).then(() => {
> 
> Whoa whoa whoa, why are we doing `resolve(null)`? Just to let a single tick
> of the event loop happen? Why?
> 
> This is both confusing and scaring me...

Sorry for the scare. It's just terrible coding on my part. I think I saw that style somewhere else and it must have slightly infected me. I'll chop off my arm to save myself.

The reason is to get "inside" a promise because I have an `if` branch where one side returns a promise and the other doesn't, and I want to execute code after either one is resolved. Argh, this is where Task solves it, but I'm not sure if I can use that yet because of worker debugging. I'll ask Eddy. Either way I'll refactor this.

> @@ +5138,3 @@
> >     */
> > +  source: function  ({ source, originalUrl, generatedSource,
> > +              isInlineSource, contentType }) {
> 
> funky indentation here; line up isInlineSource with source.

Weird, I don't see that in my code, not sure how the patch came out that way. I don't see any tabs or anything. I'll watch out for it in my next patch.

> 
> @@ +5174,5 @@
> > +      // Not all "original" scripts are distinctly separate from the
> > +      // generated script. Pretty-printed sources have a sourcemap for
> > +      // themselves, so we need to make sure there a real source
> > +      // doesn't already exist with this URL.
> > +      for(let [source, actor] of this._sourceActors) {
> 
> for_(

FINE

> 
> @@ +5216,5 @@
> > +
> > +    // Don't notify a new source if it's a generated one, as it has
> > +    // sourcemapped sources. The generated one is created to set
> > +    // breakpoints.
> > +    if (!source || !this._sourceMaps.has(source)) {
> 
> Does this do the correct thing when we start source mapping, but then it is
> disabled later (so this._sourceMaps.has(source) could be true)?
> 
> Do we want a `this._useSourceMaps &&` prefix on the conditional?

Disabling source mapping send a `reconfigure` request right? If so, it should work because on `reconfigure` the existing ThreadSources is blown away. If not, yeah we can add that check here.

> 
> @@ +5290,5 @@
> > +    // if the source is an HTML-embedded <script> tag. Since we don't
> > +    // have an API implemented to detect whether this is the case, we
> > +    // need to be conservative and only treat valid js files as real
> > +    // sources. Otherwise, use the `originalUrl` property to treat it
> > +    // as an HTML source that manages multiple inline sources.
> 
> Is this still true? Doesn't introductionType give us this info? Can we
> simplify this logic?

Unfortunately, no. I thought it would. But `introductionType` is both `scriptElement` for inlined sources and external sources (since they both come from a `script` tag). I actually had removed this code until I started running the tests and saw that everything was being detected as an inline source.

Might be worth a followup bug to provide that in the API?

> 
> @@ +5311,5 @@
> >  
> >    /**
> >     * Return a promise of an array of source actors representing all the
> >     * sources of |aScript|.
> > +
> 
> Remove this change.

Whoops, accident.

> 
> @@ +5330,5 @@
> > +                          generatedSource: aScript.source })
> > +            for (s of map.sources)
> > +            ];
> > +        }
> > +        else {
> 
> Remove else since we always return in the if-branch.

+1

> 
> @@ +5340,5 @@
> >  
> >    /**
> >     * Return a promise of a SourceMapConsumer for the source map for
> > +   * `aSource`; if we already have such a promise extant, return that.
> > +   * This should only be called if you really know what you're doing,
> 
> "unless you really know what you are doing" seems unnecessarily scary and
> doesn't help educate the reader as to when they would really know what
> they're doing, etc.
> 
> Can we say something along the lines of "Use this instead of `getSourceMap`
> for 'internal' source maps that we want to use even when the source map pref
> isn't set (pretty printing, for example) ... blah blah blah. If you're
> unsure, just use `getSourcemap`."

Yeah, my comment actually is just wrong now. `fetchSourceMap` is safe to use. I changed it because I realized that before (when `fetchSourceMap` always fetched, and everything like `getOriginalLocation` used `getSourceMap`), other tools with other Debugger objects were forced to call `sourcesForScript` or one of those on each source to make sure the sourcemap was loaded, and *then* they could call `getOriginalLocation`.

That puts a ton of burden on other things like the tracer, as they don't care at all about the specific source actors or anything, so they shouldn't have to construct source actors first before doing any sourcemapping. In `_fetchSourceMap` it now checks if `_useSourceMaps` is true and only downloads then, which makes it safe to always call the higher-level `fetchSourceMap`, so `getOriginalLocation` calls it. Which means the tracer can pass a Debugger.Source straight into `getOriginalLocation` and everything should work properly.

A little confusing, maybe... But to the outside world hopefully it's simple.

> 
> @@ +5436,5 @@
> >      return Services.io.newURI(
> >        ".", null, Services.io.newURI(aPath, null, null)).spec;
> >    },
> >  
> > +  clearSourceMapCache: function(aSourceMapURL, opts = { hard: false }) {
> 
> This definitely needs a doc comment. When do I use `hard` or not?

Done.

> @@ +5453,5 @@
> > +      }
> > +    }
> > +  },
> > +
> > +  setSourceMapHard: function(aSource, aUrl, aMap) {
> 
> Needs a doc comment and should explain when/why I should use this instead of
> `setSourceMap`

Done. `setSourceMap` actually isn't even called anymore, should we leave it in anyway?

> 
> @@ +5502,5 @@
> > +          column: sourceCol,
> > +          name: sourceName
> > +        };
> > +      }
> > +      else {
> 
> Remove the else branch since the if will always return.

> 
> @@ +5661,5 @@
> > +/*
> > + * Returns a new object that has all of the properties of `obj1` and
> > + * `obj2` combined.
> > + */
> > +function merge(obj1, obj2) {
> 
> Can we make `update` support this use case, like we discussed in the other
> bug?

How could we do that? The semantics are different; this doesn't modify anything and returns a new object. We could move this into utils if you want.

> 
> ::: toolkit/devtools/server/tests/unit/head_dbg.js
> @@ +15,5 @@
> >  
> >  const Services = devtools.require("Services");
> >  // Always log packets when running tests. runxpcshelltests.py will throw
> >  // the output away anyway, unless you give it the --verbose flag.
> > +Services.prefs.setBoolPref("devtools.debugger.log", false);
> 
> Why this change?

Didn't see this in my diff; accidental. 

> 
> ::: toolkit/devtools/transport/transport.js
> @@ +547,5 @@
> > +              dump(k + ' ');
> > +            }
> > +            dump('\n');
> > +            throw e;
> > +          }
> 
> Why this change?

Accidental as well, leftover from debugging.
Blocks: 833744
I will be focusing on this again and making sure it lands in the next few weeks. Still need to fix up a few things from the review above, but I just fixed a bunch of other tests in other tools (console, profiler, etc) so let's see if we can make this green:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b62a5f3ea299
@fitzgen going through your review piece-by-piece, currently looking at:

> ::: browser/devtools/debugger/debugger-controller.js
> @@ +652,5 @@
>>    _refillFrames: function() {
>>      // Make sure all the previous stackframes are removed before re-adding them.
>>      DebuggerView.StackFrames.empty();
>> +
>> +    // TODO: need to be backwards compatible
>
>It would be nice if we encapsulated all the compatibility munging at the client-layer (where we have traditionally done it) and the UI/frontend didn't have to muddy itself up.
>
> Is there a reason we can't do it there in this case?

Yeah, we should do that, but I'm not sure how. The `where` property of a frame now has a `source` property (the source form of the actor) instead of just `url`. How can we automatically fill that in for older servers? Would it be OK to always do a `getSources` for each `fillFames` request? that's the only way we'll be able to resolve urls -> source actors
I've updated my patch will all of your suggestions except for the few comments on the tests. Will post new patches today (probably also with a few additional tests that test the eval functionality)

(In reply to Nick Fitzgerald [:fitzgen] from comment #19)
> 
> ::: browser/devtools/debugger/test/browser_dbg_stack-05.js
> @@ -80,5 @@
> > -      is(gEditor.getDebugLocation(), 5,
> > -        "Editor debug location is correct.");
> > -
> > -      deferred.resolve();
> > -    });
> 
> These are a lot of assertions that don't exist anymore... Why did you remove
> this? It seems like this is reducing our test coverage.

This was a fallout from how I changed doc_script-switching-01.html. It used to use `eval('debugger')` but that made it impossible for me to fix all the tests because now that evals show up as separate sources it destroyed all the tests. We made it just `debugger` and it was much more feasible to fix the tests.

However, now instead of there being 4 stack frames there are only 2 in that tests. The ones I removed were just repetitions of the checks that checked the first frame, so I don't think we're losing much. However, if you think it's important, we can create a new html file with more stack frames and add more assertions for each frame.
Attached patch frontend(2).patch (obsolete) — Splinter Review
This is an updated patch for all the frontend changes (included tests).
Attachment #8507074 - Attachment is obsolete: true
Attachment #8507075 - Attachment is obsolete: true
Attachment #8525465 - Flags: review?(nfitzgerald)
Attached patch backend(2).patch (obsolete) — Splinter Review
The updated backend patch.
Attachment #8507073 - Attachment is obsolete: true
Attachment #8525466 - Flags: review?(nfitzgerald)
These patches incorporate changes from fitzgen's review (hopefully I didn't miss anything). I also fixed a lot more test. Here is the latest try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a3381f576332

Everything is green except for dt1 on linux debug, and it looks to be a race condition in one of the tracer tests. Working on fixing that now. Feels like it's getting pretty close.
Attached patch frontend(2).patch (obsolete) — Splinter Review
Rebased
Attachment #8525465 - Attachment is obsolete: true
Attachment #8525465 - Flags: review?(nfitzgerald)
Attachment #8525637 - Flags: review?(nfitzgerald)
Attached patch backend(2).patch (obsolete) — Splinter Review
rebased
Attachment #8525466 - Attachment is obsolete: true
Attachment #8525466 - Flags: review?(nfitzgerald)
Attachment #8525638 - Flags: review?(nfitzgerald)
Latest try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=885ef2e565aa

Still trying to fix whatever is going on in browser_dbg_tracing-03.js
I lied. This should be the most up-to-date one with just failures on linux debug: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d22b182475e7
Comment on attachment 8525637 [details] [diff] [review]
frontend(2).patch

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

::: browser/devtools/canvasdebugger/canvasdebugger.js
@@ +1254,5 @@
>  function viewSourceInDebugger(url, line) {
>    let showSource = ({ DebuggerView }) => {
> +    let item = DebuggerView.Sources.getItemForAttachment(a => a.source.url === url);
> +    if (item) {
> +      DebuggerView.setEditorLocation(item.attachment.source.actor, line, { noDebug: true }).then(() => {

We seem to be repeating this function/logic in a few places, and your updates have made you have to touch them all, which is less than great. Can you file a follow up to make this stuff a proper, shared utility?

::: browser/devtools/canvasdebugger/test/head.js
@@ +272,5 @@
>  }
> +
> +function getSourceActor(aSources, aURL) {
> +  let item = aSources.getItemForAttachment(a => a.source.url === aURL);
> +  return item && item.value;

There you go again...
Attachment #8525637 - Flags: review?(nfitzgerald) → review+
Comment on attachment 8525638 [details] [diff] [review]
backend(2).patch

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

::: toolkit/devtools/DevToolsUtils.js
@@ +373,5 @@
> +  Object.keys(obj2).forEach(k => {
> +    res[k] = obj2[k];
> +  });
> +  return res;
> +}

(In reply to James Long (:jlongster) from comment #20)
> (In reply to Nick Fitzgerald [:fitzgen] from comment #17)
> > @@ +5661,5 @@
> > > +/*
> > > + * Returns a new object that has all of the properties of `obj1` and
> > > + * `obj2` combined.
> > > + */
> > > +function merge(obj1, obj2) {
> > 
> > Can we make `update` support this use case, like we discussed in the other
> > bug?
> 
> How could we do that? The semantics are different; this doesn't modify anything
> and returns a new object. We could move this into utils if you want.

As I said in the other bug, make `update` take any number of arguments, and then merge them all into the first argument. So you're uses would become:

  const merged = update({}, obj1, obj2);

It just sucks having such similar things all over the place, and the more fragmented we get, the more confusing it is to choose which to use when, and test coverage is probably worse, etc.

Also, this doesn't deal with getters/setters, while `update` does.

::: toolkit/devtools/client/dbg-client.jsm
@@ +1738,1 @@
>        for each (let frame in aResponse.frames) {

While you're here, could you remove this spidermonkey-ism in favor of something standardized?

@@ +1738,5 @@
>        for each (let frame in aResponse.frames) {
> +        if (!frame.where.source) {
> +          // Older servers use urls instead, so we need to resolve
> +          // them to source actors
> +          for (var grip of threadGrips) {

Why var? If it isn't escaping its block, use let, or else it's confusing and people wonder why you used var.

@@ +2364,5 @@
> +        condition: condition
> +      };
> +
> +      // Backwards compatibility: send the breakpoint request to the
> +      // thread if the server doesn't support Debugger.Source actors

Nit: missing period.

::: toolkit/devtools/server/actors/script.js
@@ +305,5 @@
>        }
>      }
>    },
>  
> +  _iterLines: function* (actor, aLine) {

We should probably be consistent and use `aActor` since all the other params do here :(

@@ +365,5 @@
> +}
> +
> +SourceActorStore.prototype = {
> +  /**
> +   * Lookup an existing actor id that represents this source, if available

Nit: missing period.

@@ +376,5 @@
> +    return null;
> +  },
> +
> +  /**
> +   * Update a source with an actorID

Nit: missing period.

@@ +386,5 @@
> +    }
> +  },
> +
> +  /**
> +   * Make a unique URL from a source that identifies it across reloads

Nit: missing period.

@@ +870,5 @@
> +        if (!aOrigPosition.sourceActor) {
> +          // The only time the source actor will be null is if there
> +          // was a sourcemap and it tried to look up the original
> +          // location but there was no original URL. This is a strange
> +          // scenario so we simply don't pause

Nit: missing period.

@@ +874,5 @@
> +          // scenario so we simply don't pause
> +          DevToolsUtils.reportException(
> +            'ThreadActor',
> +            new Error('attempted to pause in a script with a sourcemap but ' +
> +                      'could not find original location')

Nit: capitalization + period.

@@ +2336,4 @@
>   *
> + * - A single non-sourcemapped source that is not inlined in HTML
> + *   (separate JS file, eval'ed code, etc)
> + * - A single sourcemapped source which creates 10 original sources

s/10/N/
Attachment #8525638 - Flags: review?(nfitzgerald) → review+
Lovely work! Thanks, James!
Thanks for the quick review! I can definitely change everything you mentioned (most were accidents, like using `var` etc). Just need to fix this one last test failure... I'll also write a few tests that test the eval sources specifically. Hopefully I'll be able to add `checkin-needed` by next Monday.
This does not depend on the Promises work anymore
No longer depends on: 939636
No longer depends on: 917963
No longer depends on: 1005542
No longer depends on: 935373
Summary: [meta] Debugger.Source → Debugger.Source Integration
Blocks: 637572
Attached patch backend+frontend.patch (obsolete) — Splinter Review
Not entirely sure if I should have done this, but I merged the patches... I can separate them again before it's merged if that's helpful.
Attachment #8525637 - Attachment is obsolete: true
Attachment #8525638 - Attachment is obsolete: true
The above patches are just cleaned up versions of the ones that fitzgen r+. Here's the (hopefully) final try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1beae108d9bf

If all are green when I wake up tomorrow, I'll mark `checkin-needed`.
So a few xpcshell tests failed (forgot to run those locally :/) but hopefully this should be green: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=95017a362cfa
Added a few small fixes to some of the xpcshell tests
Attachment #8528215 - Attachment is obsolete: true
Try is looking pretty good. There's a few failures but those all looks completely unrelated.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ce5a390e46a9
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [status:inflight][fixed-in-fx-team]
Target Milestone: --- → Firefox 36
I've added a section to the debugger doc for this:
https://developer.mozilla.org/en-US/docs/Tools/Debugger#Debug_eval_sources

Let me know if it needs anything else.

(I think it might be good to split each of these "How do I..." bits into a separate page, and have an index page for them.)
Flags: needinfo?(nfitzgerald)
(In reply to Will Bamberg [:wbamberg] from comment #45)
> I've added a section to the debugger doc for this:
> https://developer.mozilla.org/en-US/docs/Tools/Debugger#Debug_eval_sources
> 
> Let me know if it needs anything else.
> 
> (I think it might be good to split each of these "How do I..." bits into a
> separate page, and have an index page for them.)

Oops, wrong bug, sorry.
Flags: needinfo?(nfitzgerald)
OK, another try: https://developer.mozilla.org/en-US/docs/Tools/Debugger#Debug_eval_sources. Please let me know if this is accurate.
Flags: needinfo?(jlong)
(In reply to Will Bamberg [:wbamberg] from comment #47)
> OK, another try:
> https://developer.mozilla.org/en-US/docs/Tools/Debugger#Debug_eval_sources.
> Please let me know if this is accurate.

That looks pretty solid, although I might avoid mentioning the pretty printing stuff, since that is just a TODO item that should be fixed pretty soon here.

Also, you don't need quotes in the //# sourceURL pragma (unless you want them to also show up there in the debugger's UI).

Good stuff!
Thanks Nick. It's a great feature to have landed! 

> That looks pretty solid, although I might avoid mentioning the pretty
> printing stuff, since that is just a TODO item that should be fixed pretty
> soon here.

I called this out just because it's much easier to debug pretty-printed sources, so that was the first thing I tried to do when I tried this out, and was then surprised it didn't work. So I think unless this TODO will be uplifted to Firefox 36, I'd rather keep the note in, so as to avoid confusion. But if you feel strongly, it's simple to remove.
 
> Also, you don't need quotes in the //# sourceURL pragma (unless you want
> them to also show up there in the debugger's UI).

Done.
Flags: needinfo?(jlong)
I apologise if this is an inappropriate place for this question, as it stems from my ignorance of the patch/test/release process used for Firefox.

In 1060732 I raised an issue about the Firefox debugger requesting sources from the network. That particular issue still occurs in Nightly. I guess my question is whether the fix in *this* issue is already in Nightly, and if it is then perhaps my original issue needs to be revisited?
(In reply to Scott Macpherson from comment #50)
> I apologise if this is an inappropriate place for this question, as it stems
> from my ignorance of the patch/test/release process used for Firefox.

No worries, things are complex!

> In 1060732 I raised an issue about the Firefox debugger requesting sources
> from the network. That particular issue still occurs in Nightly. I guess my
> question is whether the fix in *this* issue is already in Nightly, and if it
> is then perhaps my original issue needs to be revisited?

A bug's changes reach Nightly within 24 hours of landing in m-c, so according to comment 44, 2014-11-26 (or perhaps the day after) is the approximate time here.

It seems this bug did not have the effect on bug 1060732 that Panos expected, so we should reopen bug 1060732.  Further comments about this would make more sense on bug 1060732.
Yeah, we have to re-fetch anything that's not a normal JS file because all have have in the engine are JS sources. If they came from something else, like an HTML page, we need to re-fetch it.

We may not have to, if there's a way to get the cached HTML page from the browser somehow, but that's we'll have to explicitly code that in and this bug isn't related. Actually, if the devtools was opened when the page was loaded, we should definitely be able to grab the HTML source. If they weren't opened, I'm pretty sure we'll always have to re-fetch.
(In reply to James Long (:jlongster) from comment #52)
> We may not have to, if there's a way to get the cached HTML page from the
> browser somehow, but that's we'll have to explicitly code that in and this
> bug isn't related. Actually, if the devtools was opened when the page was
> loaded, we should definitely be able to grab the HTML source. If they
> weren't opened, I'm pretty sure we'll always have to re-fetch.

I just noticed that we are always setting the no-cache flag if we are using fetch instead of D.S, but we should only be doing that if it is a source mapped source, not an inline HTML source. This would fix the behavior for HTML source got via GET (like the majority are) but not for POSTs, since fetch only supports GET, IIRC.

The real solution is to show each inline source on its own, and stop displaying the whole HTML file, as we have discussed before. Then we won't need to use fetch for anything other than source mapped sources.
(In reply to Nick Fitzgerald [:fitzgen] from comment #53)
> I just noticed that we are always setting the no-cache flag if we are using
> fetch instead of D.S, but we should only be doing that if it is a source
> mapped source, not an inline HTML source. This would fix the behavior for
> HTML source got via GET (like the majority are) but not for POSTs, since
> fetch only supports GET, IIRC.

That is (or should be) the way it works right now: http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/script.js#2384, the `loadFromCache` flag is set if it's not sourcemapped. Actually, I just stared at that code for a while, and the `!this.source` is confusing... shouldn't it be: `loadFromCache: this.source`? We know a source is sourcemapped if it doesn't have a source attached.
Ah ok, I missed this bit of logic:

      if (isInlineSource) {
        // If it's an inline source, the fake HTML source hasn't been
        // created yet (would have returned above), so flip this source
        // into a sourcemapped state by giving it an `originalUrl` which
        // is the HTML url.
        originalUrl = source.url;
        source = null;
      }
(In reply to James Long (:jlongster) from comment #54)
> shouldn't it be: `loadFromCache: this.source`? We know a source
> is sourcemapped if it doesn't have a source attached.

So I think this *is* wrong, but not in the way you describe:

Neither inline HTML nor source mapped sources will have `this.source`, so branching on its existence will always yield the same result and is the wrong thing to do.

We need to branch on whether this is a source mapped source or an inline HTML source. The only way to do that at the moment is via `this._originalUrl`, right?
Flags: needinfo?(jlong)
(In reply to Nick Fitzgerald [:fitzgen] from comment #56)
> (In reply to James Long (:jlongster) from comment #54)
> > shouldn't it be: `loadFromCache: this.source`? We know a source
> > is sourcemapped if it doesn't have a source attached.
> 
> So I think this *is* wrong, but not in the way you describe:
> 
> Neither inline HTML nor source mapped sources will have `this.source`, so
> branching on its existence will always yield the same result and is the
> wrong thing to do.

Why is that the wrong thing to do? In a way the HTML page is a sourcemapped thing, it's a bunch of text that represents a bunch of sources combined together. For an HTML page (which all inline sources use), it won't have a source and we need to re-fetch the HTML content. For a sourcemapped script, it won't have a source and we need to fetch the content of the original file. It should be the same for both, right?

> 
> We need to branch on whether this is a source mapped source or an inline
> HTML source. The only way to do that at the moment is via
> `this._originalUrl`, right?

I believe inline sources right now have the originalURL set to the HTML url, so we can't distinguish. But that was on purpose, since I'm treating it as kind of a sourcemapped thing. We can change it as needed though if I'm wrong here.

I'll file a new bug about the fetching logic and we can continue the discussion there.
Flags: needinfo?(jlong)
As it took me quite a while to find this issue again, I want to add that it adds support for dynamically evaluated scripts.

Sebastian
Depends on: 1327986
Depends on: 1327991
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.