Proposed doc fixes for ThreadSources and friensd

gistfile1.diff

# HG changeset patch
# Parent 2614431da3808090e45d804c1e2e98d3a737a442

diff --git a/toolkit/devtools/client/dbg-client.jsm b/toolkit/devtools/client/dbg-client.jsm
--- a/toolkit/devtools/client/dbg-client.jsm
+++ b/toolkit/devtools/client/dbg-client.jsm
@@ -1064,7 +1064,8 @@ ThreadClient.prototype = {
     after: function (aResponse) {
       if (aResponse.error) {
         // There was an error resuming, back to paused state.
-        self._state = "paused";
+        // browser/devtools/debugger/test/browser_dbg_bug740825_conditional-breakpoints-01.js
+        this._state = "paused";
       }
       return aResponse;
     },
diff --git a/toolkit/devtools/server/actors/script.js b/toolkit/devtools/server/actors/script.js
--- a/toolkit/devtools/server/actors/script.js
+++ b/toolkit/devtools/server/actors/script.js
@@ -1253,7 +1253,7 @@ ThreadActor.prototype = {
    *
    * @param aScript Debugger.Script
    *        The source script that will be stored.
-   * @returns true, if the script was added, false otherwise.
+   * @returns a promise of true, if the script was added; of false otherwise.
    */
   _addScript: function TA__addScript(aScript) {
     if (!this._allowSource(aScript.url)) {
@@ -2451,7 +2451,9 @@ function ThreadSources(aThreadActor, aUs
 
 ThreadSources.prototype = {
   /**
-   * Add a source to the current set of sources.
+   * Return the source actor representing |aURL|, creating one if none
+   * exists already. If |aURL| is not one this ThreadSources' 'allow'
+   * predicate approves of, return null.
    *
    * Right now this takes a URL, but in the future it should
    * take a Debugger.Source. See bug 637572.
@@ -2459,8 +2461,8 @@ ThreadSources.prototype = {
    * @param String aURL
    *        The source URL.
    * @param optional SourceMapConsumer aSourceMap
-   *        The source map that introduced this source.
-   * @returns a SourceActor representing the source or null.
+   *        The source map that introduced this source, if any.
+   * @returns a SourceActor representing the source at aURL or null.
    */
   source: function TS_source(aURL, aSourceMap=null) {
     if (!this._allow(aURL)) {
@@ -2483,7 +2485,12 @@ ThreadSources.prototype = {
   },
 
   /**
-   * Add all of the sources associated with the given script.
+   * Return a promise of an array of source actors representing all the
+   * sources of |aScript|.
+   *
+   * If source map handling is enabled and |aScript| has a source map, then
+   * use it to find all of |aScript|'s *original* sources; return a promise
+   * of an array of source actors for those.
    */
   sourcesForScript: function TS_sourcesForScript(aScript) {
     if (!this._useSourceMaps || !aScript.sourceMapURL) {
@@ -2507,15 +2514,20 @@ ThreadSources.prototype = {
   },
 
   /**
-   * Add the source map for the given script.
+   * Return a promise of a SourceMapConsumer for the source map for
+   * |aScript|; if we already have such a promise extant, return that.
+   * |aScript| must have a non-null sourceMapURL.
+   *
+   * Once the returned promise is resolved, this.getGeneratedLocation will
+   * be able to find the generated locations for |aScript|'s original
+   * source URLs.
    */
   sourceMap: function TS_sourceMap(aScript) {
     if (aScript.url in this._sourceMapsByGeneratedSource) {
       return this._sourceMapsByGeneratedSource[aScript.url];
     }
     dbg_assert(aScript.sourceMapURL);
-    let sourceMapURL = this._normalize(aScript.sourceMapURL,
-                                       aScript.url);
+    let sourceMapURL = this._normalize(aScript.sourceMapURL, aScript.url);
     let map = this._fetchSourceMap(sourceMapURL)
       .then((aSourceMap) => {
         for (let s of aSourceMap.sources) {
@@ -2529,7 +2541,8 @@ ThreadSources.prototype = {
   },
 
   /**
-   * Fetch the source map located at the given url.
+   * Return a promise of a SourceMapConsumer for the source map located at
+   * the given url. If there is already such a promise extant, return it.
    */
   _fetchSourceMap: function TS__fetchSourceMap(aAbsSourceMapURL) {
     if (aAbsSourceMapURL in this._sourceMaps) {
@@ -2551,7 +2564,7 @@ ThreadSources.prototype = {
   },
 
   /**
-   * Returns a promise for the location in the original source if the source is
+   * Returns a promise of the location in the original source if the source is
    * source mapped, otherwise a promise of the same location.
    *
    * TODO bug 637572: take/return a column
@@ -2622,8 +2635,8 @@ ThreadSources.prototype = {
   },
 
   iter: function TS_iter() {
-    for (let url in this._sourceActors) {
-      yield this._sourceActors[url];
+    for each (let actor in this._sourceActors) {
+      yield actor;
     }
   }
 };
@@ -2662,6 +2675,7 @@ function isNotNull(aThing) {
  * @param aURL String
  *        The URL we will request.
  * @returns Promise
+ *        A promise of the document at that URL, as a string.
  *
  * XXX: It may be better to use nsITraceableChannel to get to the sources
  * without relying on caching when we can (not for eval, etc.):

Line 35, the second sentence here seems kind of round about / a mouthful. How
does "Returns null if aURL is not allowed by the predicate" sound?

Line 74, do we need to document other methods in the middle of documenting this
current method. Seems like something that will get out of date pretty quickly. I
get that this can be used as an argument against almost all documentation, but
what I'm trying to say is that I don't think this snippet carries its weight, or
is useful really, and is somewhat out of place.

Line 94, we should probably mention that the source map url passed to this
method needs to be absolute. The variable name hints at it, but I really
should have documented this properly. Aside, thanks for coming in and cleaning
up things that I should have documented right the first time.

Line 114, for each loops didn't make it into ES.next/harmony/whatever it is
called now days, do we really want to keep using them? My understanding was that
they are never going to become a proper part of the language. We can't for of
because there is no default iterator on objects. Not sure what to do here, but
this is the reason why I avoid for of personally.

Line 35: I prefer your phrasing.

Line 74: Yes, this should be a comment on getGeneratedLocation.

Line 94: Okay, I'll add that.

Line 114: I see. for-in is best here, I guess. If we use Map instead of Object.create(null), then we could use for-of.

This is not ideal process, but:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8135cded2d1c