1347894 - convert uses of "defer" to "new Promise" in client/scratchpad directory

Status: RESOLVED FIXED

(DevTools Graveyard :: Scratchpad, defect, P3)

Description

[Subhdeep Saha]

Attachment: scratchpad.js

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/56.0.2924.87 Safari/537.36

Comment 1

[Tom Tromey :tromey]

Hi.  You might want some information on how to get started.
Here's a document that has an intro to work on devtools.
It also has some handy links to other sources of information.

https://wiki.mozilla.org/DevTools/Hacking

Comment 2

[Subhdeep Saha]

Attachment: Bug 1347894 convert uses of "defer" to "new Promise" in client/scratchpad directory

Review commit: https://reviewboard.mozilla.org/r/122348/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/122348/

Comment 3

[Subhdeep Saha]

Hi Tom,
I had problems while converting the function ST_connect(aSubject). This is what i had tried doing https://pastebin.mozilla.org/8982752

Could please help me.

Comment 4

[Tom Tromey :tromey]

You don't say what problem you are experiencing, but I'm going to guess it is this:

    this._connector = new Promise((resolve, reject) => {

This line runs the passed-in lambda synchronously.  However that means that _connector
isn't set:

    this._connector.then(() => clearTimeout(connectTimer));

... here.

One way around this would be to lower the clearTimeout code to later in the enclosing function.

Comment 5

[Tom Tromey :tromey]

Should I review the patch or are you going to update it for the ST_connect change?

Comment 6

[Subhdeep Saha]

It will be helpful if you can see whether i have done the rest correctly or not

Comment 7

[Tom Tromey :tromey]

Comment on attachment 8849569 [details]
Bug 1347894 convert uses of "defer" to "new Promise" in client/scratchpad directory

https://reviewboard.mozilla.org/r/122348/#review124994

Thank you for this patch.  I've made various notes about what I think might be problems with it.

::: devtools/client/scratchpad/scratchpad.js:553
(Diff revision 1)
>     *         The promise for the script evaluation result.
>     */
>    run: function SP_run()
>    {
> -    let deferred = promise.defer();
> -    let reject = aReason => deferred.reject(aReason);
> +    return new Promise((resolve, reject) => {
> +    let _reject = aReason => reject(aReason);

I don't think `_reject` is needed here, you can just substitute `reject` everywhere and it will do the right thing.

::: devtools/client/scratchpad/scratchpad.js:580
(Diff revision 1)
>     *         The promise for the script evaluation result.
>     */
>    inspect: function SP_inspect()
>    {
> -    let deferred = promise.defer();
> -    let reject = aReason => deferred.reject(aReason);
> +      return new Promise((resolve, reject) => {
> +    let _reject = aReason => reject(aReason);

Here too.

::: devtools/client/scratchpad/scratchpad.js:607
(Diff revision 1)
>     *         The promise for the script evaluation result.
>     */
>    reloadAndRun: function SP_reloadAndRun()
>    {
> -    let deferred = promise.defer();
>  

No need for this blank line now.  Assuming I'm reading this patch correctly :)

::: devtools/client/scratchpad/scratchpad.js:609
(Diff revision 1)
>    reloadAndRun: function SP_reloadAndRun()
>    {
> -    let deferred = promise.defer();
>  
> +      return new Promise((resolve, reject) => {
>      if (this.executionContext !== SCRATCHPAD_CONTEXT_CONTENT) {

I would have expected the body here to be indented more now.

::: devtools/client/scratchpad/scratchpad.js:612
(Diff revision 1)
>  
> +      return new Promise((resolve, reject) => {
>      if (this.executionContext !== SCRATCHPAD_CONTEXT_CONTENT) {
>        console.error(this.strings.
>                      GetStringFromName("scratchpadContext.invalid"));
>        return;

I think this refactoring changes the function to always return a promise, whereas before in the error case it did not.  This is most likely fine but it's good to check the callers in a case like this.

::: devtools/client/scratchpad/scratchpad.js:636
(Diff revision 1)
>     *         The promise for the script evaluation result.
>     */
>    display: function SP_display()
>    {
> -    let deferred = promise.defer();
> -    let reject = aReason => deferred.reject(aReason);
> +      return new Promise((resolve, reject) => {
> +    let _reject = aReason => reject(aReason);

I don't think `_reject` is needed, as above.

::: devtools/client/scratchpad/scratchpad.js:660
(Diff revision 1)
>              this.writeAsComment(aResponse.displayString);
> -            resolve();
> +            _resolve();
>            }
>          });
>        }
> -    }, reject);
> +    }, _reject);

Seems like the indentation is now off here.  I found this a few spots in the patch.  Not sure if I'm just missing some kind of reindent marker in the review UI though.

::: devtools/client/scratchpad/scratchpad.js:2287
(Diff revision 1)
>            releaseActor: aActor => {
>              this._scratchpad.debuggerClient.release(aActor);
>            }
>          });
>        }
> -      this._update(aObject).then(() => deferred.resolve());
> +      this._update(aObject).then(() => resolve());

I think this can be written more simply as `this._update(aObject).then(resolve)`.

Comment 8

[Subhdeep Saha]

Attachment: Error logs when i changed _reject -> reject

As you suggested to change the _reject -> reject i get the following error logs
Could you please help me

Comment 9

[Tom Tromey :tromey]

(In reply to Subhdeep Saha from comment #8)
> Created attachment 8851497 [details]
> Error logs when i changed _reject -> reject
> 
> As you suggested to change the _reject -> reject i get the following error
> logs
> Could you please help me

From the log it looks like you wrote:

let reject = aReason => reject(aReason);

... but what I was really saying is that this whole binding isn't needed at all.
That is, a function like "x => reject(x)" is identical to just "reject", so you might
as well drop the extra function and "let".

Comment 10

[Matt R [:stylizit] (slow, away until Sep)]

Attachment: Bug 1347894 - convert uses of 'defer' to 'new Promise' in client/scratchpad;

Review commit: https://reviewboard.mozilla.org/r/141354/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/141354/

Comment 11

[Matt R [:stylizit] (slow, away until Sep)]

Comment on attachment 8869875 [details]
Bug 1347894 - convert uses of 'defer' to 'new Promise' in client/scratchpad;

https://reviewboard.mozilla.org/r/141354/#review144942

::: devtools/client/scratchpad/scratchpad.js:2095
(Diff revision 1)
>    connect: function ST_connect(aSubject)
>    {
>      if (this._connector) {
>        return this._connector;
>      }
>  

This seemed to me the safest approach to keep the original async order, but there might be a more clever and neater way to achieve the same?

Comment 12

[Matt R [:stylizit] (slow, away until Sep)]

Comment on attachment 8869875 [details]
Bug 1347894 - convert uses of 'defer' to 'new Promise' in client/scratchpad;

https://reviewboard.mozilla.org/r/141354/#review144946

::: devtools/client/scratchpad/scratchpad.js:2098
(Diff revision 1)
>        return this._connector;
>      }
>  
> -    let deferred = promise.defer();
> -    this._connector = deferred.promise;
> +    let resolver;
> +    let rejecter;
> +    let deferred = new Promise((resolve, reject) => {

This seemed like the safest way to keep the original async order, but there might be a more clever and neater way to achieve the same?

Comment 13

[Matt R [:stylizit] (slow, away until Sep)]

I've taken the initiative to add some missing conversions and polish slightly Subhdeep Saha's solid start in another patch. I also addressed tromey's review issues in it.

Comment 14

[Tim Nguyen :ntim]

Comment on attachment 8869875 [details]
Bug 1347894 - convert uses of 'defer' to 'new Promise' in client/scratchpad;

https://reviewboard.mozilla.org/r/141354/#review145964

A lot of this could be simplified to async functions

::: devtools/client/scratchpad/scratchpad.js:492
(Diff revision 1)
>    evaluate: function SP_evaluate(aString)
>    {
>      let connection;
>      if (this.target) {
>        connection = ScratchpadTarget.consoleFor(this.target);
>      }
>      else if (this.executionContext == SCRATCHPAD_CONTEXT_CONTENT) {
>        connection = ScratchpadTab.consoleFor(this.gBrowser.selectedTab);
>      }
>      else {
>        connection = ScratchpadWindow.consoleFor(this.browserWindow);
>      }
>  
>      let evalOptions = { url: this.uniqueName };
>  
>      return connection.then(({ debuggerClient, webConsoleClient }) => {
> -      let deferred = promise.defer();
> +      return new Promise((resolve, reject) => {
> -
> -      webConsoleClient.evaluateJSAsync(aString, aResponse => {
> +        webConsoleClient.evaluateJSAsync(aString, aResponse => {
> -        this.debuggerClient = debuggerClient;
> +          this.debuggerClient = debuggerClient;
> -        this.webConsoleClient = webConsoleClient;
> +          this.webConsoleClient = webConsoleClient;
> -        if (aResponse.error) {
> +          if (aResponse.error) {
> -          deferred.reject(aResponse);
> +            reject(aResponse);
> -        }
> +          }
> -        else if (aResponse.exception !== null) {
> +          else if (aResponse.exception !== null) {
> -          deferred.resolve([aString, aResponse]);
> +            resolve([aString, aResponse]);
> -        }
> +          }
> -        else {
> +          else {
> -          deferred.resolve([aString, undefined, aResponse.result]);
> +            resolve([aString, undefined, aResponse.result]);
> -        }
> +          }
> -      }, evalOptions);
> -
> +        },  evalOptions);
> +      });
> -      return deferred.promise;

You could use an async function here:

async evaluate(string) {
  ...
  let { debuggerClient, webConsoleClient } = await connection;
  let response = await webConsoleClient.evaluateJSAsync(string);
  ...
  if (response.error) {
    throw new Error(response);
  } else if (response.exception !== null) {
    return [...];
  } else {
    return [...];
  }
}

::: devtools/client/scratchpad/scratchpad.js:547
(Diff revision 1)
>    run: function SP_run()
>    {
> -    let deferred = promise.defer();
> +    return new Promise((resolve, reject) => {
> -    let reject = aReason => deferred.reject(aReason);
> -
> -    this.execute().then(([aString, aError, aResult]) => {
> +      this.execute().then(([aString, aError, aResult]) => {
> -      let resolve = () => deferred.resolve([aString, aError, aResult]);
> +        let _resolve = () => resolve([aString, aError, aResult]);
>  
> -      if (aError) {
> +        if (aError) {
> -        this.writeAsErrorComment(aError).then(resolve, reject);
> +          this.writeAsErrorComment(aError).then(_resolve, reject);
> -      }
> +        }
> -      else {
> +        else {
> -        this.editor.dropSelection();
> +          this.editor.dropSelection();
> -        resolve();
> +          _resolve();
> -      }
> +        }
> -    }, reject);
> +      }, reject);
> -
> +    });
> -    return deferred.promise;

Same here:

async run() {
  let [string, error, result] = await this.execute();
  if (error) {
    await this.writeAsErrorComment(error);
  } else {
    this.editor.dropSelection();
  }
}

::: devtools/client/scratchpad/scratchpad.js:571
(Diff revision 1)
>    inspect: function SP_inspect()
>    {
> -    let deferred = promise.defer();
> +    return new Promise((resolve, reject) => {
> -    let reject = aReason => deferred.reject(aReason);
> -
> -    this.execute().then(([aString, aError, aResult]) => {
> +      this.execute().then(([aString, aError, aResult]) => {
> -      let resolve = () => deferred.resolve([aString, aError, aResult]);
> +        let _resolve = () => resolve([aString, aError, aResult]);
>  
> -      if (aError) {
> +        if (aError) {
> -        this.writeAsErrorComment(aError).then(resolve, reject);
> +          this.writeAsErrorComment(aError).then(_resolve, reject);
> -      }
> +        }
> -      else {
> +        else {
> -        this.editor.dropSelection();
> +          this.editor.dropSelection();
> -        this.sidebar.open(aString, aResult).then(resolve, reject);
> +          this.sidebar.open(aString, aResult).then(_resolve, reject);
> -      }
> +        }
> -    }, reject);
> +      }, reject);
> -
> +    });
> -    return deferred.promise;

Same here, this could be an async function

::: devtools/client/scratchpad/scratchpad.js:624
(Diff revision 1)
> -    let deferred = promise.defer();
> +    return new Promise((resolve, reject) => {
> -    let reject = aReason => deferred.reject(aReason);
> -
> -    this.execute().then(([aString, aError, aResult]) => {
> +      this.execute().then(([aString, aError, aResult]) => {
> -      let resolve = () => deferred.resolve([aString, aError, aResult]);
> +        let _resolve = () => resolve([aString, aError, aResult]);
>  
> -      if (aError) {
> +        if (aError) {
> -        this.writeAsErrorComment(aError).then(resolve, reject);
> +          this.writeAsErrorComment(aError).then(_resolve, reject);
> -      }
> +        }
> -      else if (VariablesView.isPrimitive({ value: aResult })) {
> +        else if (VariablesView.isPrimitive({ value: aResult })) {
> -        this._writePrimitiveAsComment(aResult).then(resolve, reject);
> +          this._writePrimitiveAsComment(aResult).then(_resolve, reject);
> -      }
> +        }
> -      else {
> +        else {
> -        let objectClient = new ObjectClient(this.debuggerClient, aResult);
> +          let objectClient = new ObjectClient(this.debuggerClient, aResult);
> -        objectClient.getDisplayString(aResponse => {
> +          objectClient.getDisplayString(aResponse => {
> -          if (aResponse.error) {
> +            if (aResponse.error) {
> -            reportError("display", aResponse);
> +              reportError("display", aResponse);
> -            reject(aResponse);
> +              reject(aResponse);
> -          }
> +            }
> -          else {
> +            else {
> -            this.writeAsComment(aResponse.displayString);
> +              this.writeAsComment(aResponse.displayString);
> -            resolve();
> +              _resolve();
> -          }
> +            }
> -        });
> +          });
> -      }
> +        }
> -    }, reject);
> +      }, reject);

Same here, this could be an async function

Comment 15

[Tom Tromey :tromey]

Comment on attachment 8869875 [details]
Bug 1347894 - convert uses of 'defer' to 'new Promise' in client/scratchpad;

https://reviewboard.mozilla.org/r/141354/#review146490

Thanks for the patch   A couple of nits from me, but nothing serious.

I agree with ntim that some of this code would be improved by the use of async.  However, it's up to you whether to do that or not; it's more work and there's nothing wrong with the patch in hand, especially considering that it's a patch to an under-maintained component.

::: devtools/client/scratchpad/scratchpad-panel.js:21
(Diff revision 1)
>    this.scratchpad = Scratchpad;
>  
>    Scratchpad.target = this.target;
>    Scratchpad.hideMenu();
>  
> -  let deferred = promise.defer();
> +  let deferred = new Promise(resolve => {

This can just assign directly to `this._readyObserver`, no need to introduce a new binding.

::: devtools/client/scratchpad/scratchpad.js:607
(Diff revision 1)
>      }
>  
> +    return new Promise(resolve => {
> -    let target = TargetFactory.forTab(this.gBrowser.selectedTab);
> +      let target = TargetFactory.forTab(this.gBrowser.selectedTab);
> -    target.once("navigate", () => {
> +      target.once("navigate", () => {
> -      this.run().then(results => deferred.resolve(results));
> +        this.run().then(results => resolve(results));

Seems like this could be `.then(resolve)`.

::: devtools/client/scratchpad/scratchpad.js:2098
(Diff revision 1)
>        return this._connector;
>      }
>  
> -    let deferred = promise.defer();
> -    this._connector = deferred.promise;
> +    let resolver;
> +    let rejecter;
> +    let deferred = new Promise((resolve, reject) => {

It seems reasonable enough to me.

Comment 16

[Matt R [:stylizit] (slow, away until Sep)]

Attachment: Bug 1347894 - Converted Promise-heavy functions to async/await;

Review commit: https://reviewboard.mozilla.org/r/142962/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/142962/

Comment 17

[Matt R [:stylizit] (slow, away until Sep)]

Comment on attachment 8869875 [details]
Bug 1347894 - convert uses of 'defer' to 'new Promise' in client/scratchpad;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/141354/diff/1-2/

Comment 18

[Matt R [:stylizit] (slow, away until Sep)]

Comment on attachment 8869875 [details]
Bug 1347894 - convert uses of 'defer' to 'new Promise' in client/scratchpad;

https://reviewboard.mozilla.org/r/141354/#review146490

Thanks tromey and ntim for the reviews! I haven't actually played much with async/await, so I guess it's good practice :)
I addressed the nits and converted the functions ntim pointed out to async/await in the updated patch - I hope I did it correctly.

Comment 19

[Tim Nguyen :ntim]

Comment on attachment 8869875 [details]
Bug 1347894 - convert uses of 'defer' to 'new Promise' in client/scratchpad;

https://reviewboard.mozilla.org/r/141354/#review146702

::: devtools/client/scratchpad/test/browser_scratchpad_reload_and_run.js:56
(Diff revision 2)
> -  let deferred = promise.defer();
> +  let deferred = new Promise(resolve => {
> -  browser.addEventListener("DOMWindowCreated", function () {
> +    browser.addEventListener("DOMWindowCreated", function () {
> -    browser.contentWindow.addEventListener("foo", function () {
> +      browser.contentWindow.addEventListener("foo", function () {
> -      is(browser.contentWindow.document.body.innerHTML, "Modified text",
> +        is(browser.contentWindow.document.body.innerHTML, "Modified text",
> -        "After reloading, HTML is different.");
> +          "After reloading, HTML is different.");
>  
> -      Services.prefs.clearUserPref(DEVTOOLS_CHROME_ENABLED);
> +        Services.prefs.clearUserPref(DEVTOOLS_CHROME_ENABLED);
> -      deferred.resolve();
> +        resolve();
> -    }, {capture: true, once: true});
> +      }, {capture: true, once: true});
> -  }, {capture: true, once: true});
> +    }, {capture: true, once: true});
> +  });
>  
>    ok(browser.contentWindow.document.body.innerHTML !== "Modified text",
>        "Before reloading, HTML is intact.");
> -  sp.reloadAndRun().then(deferred.promise).then(finish);
> +  sp.reloadAndRun().then(deferred).then(finish);

I think this isn't quite right, in then(deferred), you have a promise, not sure that's expected. Tom, what do you think ?

Comment 20

[Tim Nguyen :ntim]

Comment on attachment 8871504 [details]
Bug 1347894 - Converted Promise-heavy functions to async/await;

https://reviewboard.mozilla.org/r/142962/#review146698

There are a few more functions from the original patch that can be converted to async functions if you'd like to give a try (it's fine if not :)

::: devtools/client/scratchpad/scratchpad.js:509
(Diff revision 1)
> -    return connection.then(({ debuggerClient, webConsoleClient }) => {
> -      return new Promise((resolve, reject) => {
> -        webConsoleClient.evaluateJSAsync(aString, aResponse => {
> -          this.debuggerClient = debuggerClient;
> +    this.debuggerClient = debuggerClient;
> -          this.webConsoleClient = webConsoleClient;
> +    this.webConsoleClient = webConsoleClient;
> +    let aResponse = await webConsoleClient.evaluateJSAsync(aString, () => {}, evalOptions);

You can just use null or undefined instead of the no-op.

This line takes care of it: https://dxr.mozilla.org/mozilla-central/source/devtools/shared/webconsole/client.js#296

Btw, can you rename aResponse to response ? aXxx is typically used for function arguments (but we're trying to get away from it anyway).

::: devtools/client/scratchpad/scratchpad.js:543
(Diff revision 1)
>     * @return Promise
>     *         The promise for the script evaluation result.
>     */
> -  run: function SP_run()
> +  run: async function SP_run()
>    {
> -    return new Promise((resolve, reject) => {
> +    let [aString, aError, aResult] = await this.execute();

Can you name those string, error and result ?

::: devtools/client/scratchpad/scratchpad.js:562
(Diff revision 1)
>     * @return Promise
>     *         The promise for the script evaluation result.
>     */
> -  inspect: function SP_inspect()
> +  inspect: async function SP_inspect()
>    {
> -    return new Promise((resolve, reject) => {
> +    let [aString, aError, aResult] = await this.execute();

Same here

::: devtools/client/scratchpad/scratchpad.js:609
(Diff revision 1)
>     * @return Promise
>     *         The promise for the script evaluation result.
>     */
> -  display: function SP_display()
> +  display: async function SP_display()
>    {
> -    return new Promise((resolve, reject) => {
> +    let [aString, aError, aResult] = await this.execute();

same here

::: devtools/client/scratchpad/scratchpad.js:618
(Diff revision 1)
> -          objectClient.getDisplayString(aResponse => {
> +      objectClient.getDisplayString(aResponse => {
> -            if (aResponse.error) {
> +        if (aResponse.error) {
> -              reportError("display", aResponse);
> +          reportError("display", aResponse);
> -              reject(aResponse);
> -            }
> +          throw new Error(aResponse);
> +        } else {
> -            else {
> -              this.writeAsComment(aResponse.displayString);
> +          this.writeAsComment(aResponse.displayString);
> -              _resolve();
> +          return [aString, aError, aResult];

This isn't quite right because you're returning inside the callback.

In this case, you can use the promise form of getDisplayString:

let response = await objectClient.getDisplayString();

...

Comment 21

[Tom Tromey :tromey]

Comment on attachment 8871504 [details]
Bug 1347894 - Converted Promise-heavy functions to async/await;

https://reviewboard.mozilla.org/r/142962/#review147028

Thank you for doing this.  It does simplify the code nicely.
I have a few nits, but TBH if you want to just drop these, I won't mind, since I feel they are "adding on" a bit.
Also I wanted to note that I agree with ntim's review.

::: devtools/client/scratchpad/scratchpad.js:492
(Diff revision 1)
>     * @param string aString
>     *        The script you want to evaluate.
>     * @return Promise
>     *         The promise for the script evaluation result.
>     */
> -  evaluate: function SP_evaluate(aString)
> +  evaluate: async function SP_evaluate(aString)

If you're already modifying the line, you might as well remove the "SP_evaluate" - we are slowly dropping these tags as they aren't needed for decent stack traces any more.

::: devtools/client/scratchpad/scratchpad.js:541
(Diff revision 1)
>     * current context.
>     *
>     * @return Promise
>     *         The promise for the script evaluation result.
>     */
> -  run: function SP_run()
> +  run: async function SP_run()

Likewise.

::: devtools/client/scratchpad/scratchpad.js:560
(Diff revision 1)
>     * current context. The resulting object is inspected up in the sidebar.
>     *
>     * @return Promise
>     *         The promise for the script evaluation result.
>     */
> -  inspect: function SP_inspect()
> +  inspect: async function SP_inspect()

Likewise.

::: devtools/client/scratchpad/scratchpad.js:607
(Diff revision 1)
>     * selected text.
>     *
>     * @return Promise
>     *         The promise for the script evaluation result.
>     */
> -  display: function SP_display()
> +  display: async function SP_display()

Last one!

Comment 22

[Tom Tromey :tromey]

Comment on attachment 8869875 [details]
Bug 1347894 - convert uses of 'defer' to 'new Promise' in client/scratchpad;

https://reviewboard.mozilla.org/r/141354/#review147044

Thanks for the update.  This looks good to me -- there's the one oddity that ntim pointed out, but that's optional as the weirdness is pre-existing.

::: devtools/client/scratchpad/test/browser_scratchpad_reload_and_run.js:56
(Diff revision 2)
> -  let deferred = promise.defer();
> +  let deferred = new Promise(resolve => {
> -  browser.addEventListener("DOMWindowCreated", function () {
> +    browser.addEventListener("DOMWindowCreated", function () {
> -    browser.contentWindow.addEventListener("foo", function () {
> +      browser.contentWindow.addEventListener("foo", function () {
> -      is(browser.contentWindow.document.body.innerHTML, "Modified text",
> +        is(browser.contentWindow.document.body.innerHTML, "Modified text",
> -        "After reloading, HTML is different.");
> +          "After reloading, HTML is different.");
>  
> -      Services.prefs.clearUserPref(DEVTOOLS_CHROME_ENABLED);
> +        Services.prefs.clearUserPref(DEVTOOLS_CHROME_ENABLED);
> -      deferred.resolve();
> +        resolve();
> -    }, {capture: true, once: true});
> +      }, {capture: true, once: true});
> -  }, {capture: true, once: true});
> +    }, {capture: true, once: true});
> +  });
>  
>    ok(browser.contentWindow.document.body.innerHTML !== "Modified text",
>        "Before reloading, HTML is intact.");
> -  sp.reloadAndRun().then(deferred.promise).then(finish);
> +  sp.reloadAndRun().then(deferred).then(finish);

This is definitely weird, but AFAICS it is faithfully doing what the previous code did.

Probably that `.then` could be dropped without ill effects.

Comment 23

[Matt R [:stylizit] (slow, away until Sep)]

Comment on attachment 8871504 [details]
Bug 1347894 - Converted Promise-heavy functions to async/await;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142962/diff/1-2/

Comment 24

[Matt R [:stylizit] (slow, away until Sep)]

Comment on attachment 8871504 [details]
Bug 1347894 - Converted Promise-heavy functions to async/await;

https://reviewboard.mozilla.org/r/142962/#review146698

Thanks for the reviews! And no problem at all regarding the nits.

I renamed the aX vars where apppropriate and got rid of the extra function names.

I think I converted all Promise-heavy functions if I didn't miss any. There are some other functions always returning Promises but they are quite short and I don't know if they're worth converting to async?

Comment 25

[Tim Nguyen :ntim]

Comment on attachment 8871504 [details]
Bug 1347894 - Converted Promise-heavy functions to async/await;

https://reviewboard.mozilla.org/r/142962/#review147116

::: devtools/client/scratchpad/scratchpad.js:657
(Diff revision 2)
> -    return this.prettyPrintWorker.performTask("pretty-print", {
> +    let data;
> +    try {
> +      data = await this.prettyPrintWorker.performTask("pretty-print", {
> -      url: "(scratchpad)",
> +        url: "(scratchpad)",
> -      indent: tabsize,
> +        indent: tabsize,
> -      source: uglyText
> +        source: uglyText
> -    }).then(data => {
> +      });
>        this.editor.setText(data.code);
> -    }).then(null, error => {
> +      return data;

You can just move the let inside the try block, since it's not used outside of it:

try {
  let data = ...;

::: devtools/client/scratchpad/scratchpad.js:831
(Diff revision 2)
> -    return this.evaluate(functionText);
> +    const result = await this.evaluate(functionText);
> +    return result;

return this.evaluate(functionText);
 
 is equivalent since it returns a promise (async functions also return promises).
 
 So no need to use await here.

::: devtools/client/scratchpad/scratchpad.js:849
(Diff revision 2)
> -  _writePrimitiveAsComment: function SP__writePrimitiveAsComment(aValue)
> +  _writePrimitiveAsComment: async function(aValue)
>    {
> -    return new Promise((resolve, reject) => {
> -      if (aValue.type == "longString") {
> +    if (aValue.type == "longString") {
> -        let client = this.webConsoleClient;
> +      let client = this.webConsoleClient;
> -        client.longString(aValue).substring(0, aValue.length, aResponse => {
> +      let response = await client.longString(aValue).substring(0, aValue.length, null);

You can ommit the last null parameter here.

::: devtools/client/scratchpad/scratchpad.js:2156
(Diff revision 2)
> -    return client.connect()
> -      .then(() => client.getProcess())
> +    let response = await client.connect();
> +    response = await client.getProcess();

The first response variable is not needed:

await client.connect();

let response = await client.getProcess();

::: devtools/client/scratchpad/scratchpad.js:2160
(Diff revision 2)
>      let client = new DebuggerClient(DebuggerServer.connectPipe());
> -    return client.connect()
> -      .then(() => client.getProcess())
> -      .then(aResponse => {
> -        return { form: aResponse.form, client: client };
> -      });
> +    let response = await client.connect();
> +    response = await client.getProcess();
> +    return {
> +      form: response.form,
> +      client: client

Could be shortened to just client

return { form: response.form, client };

::: devtools/client/scratchpad/scratchpad.js:2179
(Diff revision 2)
> -    return this._target.makeRemote().then(() => this._target);
> +    let response = await this._target.makeRemote();
> +    return response;

The old code returns this._target

I think you'd want to do the same in the new code:


await this._target.makeRemote();
return this._target;

Comment 26

[Matt R [:stylizit] (slow, away until Sep)]

Comment on attachment 8871504 [details]
Bug 1347894 - Converted Promise-heavy functions to async/await;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142962/diff/2-3/

Comment 27

[Tom Tromey :tromey]

Comment on attachment 8871504 [details]
Bug 1347894 - Converted Promise-heavy functions to async/await;

https://reviewboard.mozilla.org/r/142962/#review148744

Thank you.  This looks good to me.
I have a feeling you'll need to re-attach your patches, this time as a series of two patches, in order to push them through try successfully.

Comment 28

[Nicolas Chevobbe [:nchevobbe][PTO - back October 26]]

Matt, I'm so sorry your patch was not being pushed and it took me so long to answer you.
I tried to apply the patch on the latest mozilla-central [1], but it does not apply.
Do you want/have time to see what's going on, and if not, are you okay with assigning this bug to someone else ?
Thanks !


[1] hg import https://hg.mozilla.org/mozreview/gecko/raw-rev/780ac668696673a6d5e6933b17c9ea8b8d477b88

Comment 29

[Matt R [:stylizit] (slow, away until Sep)]

Hi Nicolas, It's been a while! I plan to get back to it, but unfortunately I don't quite have the time yet. Happy for you to assign it to someone else if you need it to be merged quickly.

Comment 30

[Sean Reeise [:sreeise]]

I can work on this if Matt's not back yet. I don't mind converting to async await as well.

Comment 31

[Sean Reeise [:sreeise]]

Just now seeing the PTO for Nicolas. Matt, since you ok with assigning it to someone else, I assume it is safe to take it... If not, no worries.

Comment 32

[Tim Nguyen :ntim]

Hi Sean, thanks for taking over this, here are Matt's patches in case you want to work on top of them:

https://reviewboard.mozilla.org/r/141354/diff/#index_header
https://reviewboard.mozilla.org/r/142962/diff/#index_header

Comment 33

[Sean Reeise [:sreeise]]

(In reply to Tim Nguyen :ntim (please use needinfo?) from comment #32)


Hey Tim. Thanks, that fore sure helps, as well as the previous comments. I am just wondering whether this should be done in two separate commits? One for defer and the other for async await. I have replaced all the defer statements so far and changed some of the methods to async await that previously used defer syntax, as well as ran a successful try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=56e5c9e88db02f6fa5f7b44deda7f18a001335ab 

It slipped my mind about doing commits together for try, or doing one whole commit for this, so ill have to run through again before sending to phabricator. Either way works for me I am just wondering whether you guys prefer one single commit or two separate?

Comment 34

[Tim Nguyen :ntim]

(In reply to Sean Reeise [:sreeise] from comment #33)
> (In reply to Tim Nguyen :ntim (please use needinfo?) from comment #32)
> It slipped my mind about doing commits together for try, or doing one whole
> commit for this, so ill have to run through again before sending to
> phabricator. Either way works for me I am just wondering whether you guys
> prefer one single commit or two separate?

Two separate patches is definitively preferable, as it is easier to review, but we don't mind too much one single patch.

Comment 35

[Sean Reeise [:sreeise]]

Yeah I can see how that helps with reviews. I will do two patches then, it makes it easier on me as well.

Comment 36

[Sean Reeise [:sreeise]]

Attachment: Bug 1347894 - Remove uses of defer in devtools/client/scratchpad. r?jdescottes

Promises using defer are being replaced by new Promise and async await syntax. This is for the devtools/client/scratchpad directory.
Also includes refactoring of scratchpad for async await.

Comment 37

[Sean Reeise [:sreeise]]

Try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=68502d5d66e0089ad064bf45aa5a040194f4cf36

Comment 38

[Sean Reeise [:sreeise]]

Hey Tim, I have a question about what you wanted me to change in writeAsErrorComment. If I use return when it checks for a primitive or for 'preview' in the exception, it will cause a timeout. After that return works when doing it for the response from ObjectClient. Am I missing something or not seeing something here? The returns are all inside a promise constructor, doing this:

        if (["undefined", "null", "Infinity", "-Infinity", "NaN", "-0"].includes(type)) {
          resolve(type);
        } else if (type == "longString") {  

will work fine, but this:

        if (["undefined", "null", "Infinity", "-Infinity", "NaN", "-0"].includes(type)) {
          return type;
        } else if (type == "longString") {

causes a timeout on tests.

Also about deconstructing error with:

const { exception, type } = error;

causes an issue with some tests. For example, on "-0" the failure is:

 FAIL Correct exception message for '-0' is shown - Got throw -0;
/*
Exception: [object Object]
*/, expected throw -0;
/*
Exception: -0
*/

This is when the value is resolved however not returned as you suggested, so that might be the issue.

Thanks for you help.

Comment 39

[Sean Reeise [:sreeise]]

I am not sure if it might be something with what the test is excepting as out put either. The test that fails is:

browser_scratchpad_throw_output.js

Comment 40

[Tim Nguyen :ntim]

(In reply to Sean Reeise [:sreeise] from comment #38)
> Hey Tim, I have a question about what you wanted me to change in
> writeAsErrorComment. If I use return when it checks for a primitive or for
> 'preview' in the exception, it will cause a timeout. After that return works
> when doing it for the response from ObjectClient. Am I missing something or
> not seeing something here? The returns are all inside a promise constructor,
> doing this:
> 
>         if (["undefined", "null", "Infinity", "-Infinity", "NaN",
> "-0"].includes(type)) {
>           resolve(type);
>         } else if (type == "longString") {  
> 
> will work fine, but this:
> 
>         if (["undefined", "null", "Infinity", "-Infinity", "NaN",
> "-0"].includes(type)) {
>           return type;
>         } else if (type == "longString") {
> 
> causes a timeout on tests.


Ah, ok I guess I missed something when reading the code. Feel free to use resolve() in this case.

> Also about deconstructing error with:
> 
> const { exception, type } = error;
> 
> causes an issue with some tests. For example, on "-0" the failure is:
> 
>  FAIL Correct exception message for '-0' is shown - Got throw -0;
> /*
> Exception: [object Object]
> */, expected throw -0;
> /*
> Exception: -0
> */
> 
> This is when the value is resolved however not returned as you suggested, so
> that might be the issue.
> 
> Thanks for you help.

Ah, the equality check used by includes is different from the `==` operator.

You can probably use Array.prototype.find(), but I'd rather go back to what the code was before (a single if condition with a couple of `||`)

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/48715242d46a
Remove uses of defer in devtools/client/scratchpad. r=jdescottes

Comment 42

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/48715242d46a

Comment 43

[Sean Reeise [:sreeise]]

Clearing need info now that bug is fixed.