1474236 - Remove defer usage in JsTerm.js

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P1)

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1474236 - Remove defer usage in JsTerm.js; .

This patch removes defer usage and now relies on async function.
Functions that was using callbacks are also moved to async, since
the callback only purpose was to resolve the deferred promise.

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

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8990660 [details]
Bug 1474236 - Remove defer usage in JsTerm.js; .

https://reviewboard.mozilla.org/r/255740/#review262448

I am seeing some try failures.

E.g. devtools/client/webconsole/test/mochitest/browser_jsterm_dollar.js fails for me too (Win10)

Honza

::: devtools/client/webconsole/components/JSTerm.js:457
(Diff revision 1)
> -    };
> +      };
>  
> -    const mappedString = await this.hud.owner.getMappedExpression(executeString);
> +      const mappedString = await this.hud.owner.getMappedExpression(executeString);
> -    this.requestEvaluation(mappedString, options).then(onResult, onResult);
> +      const response = await this.requestEvaluation(mappedString, options);
> +      await this._executeResultCallback(response);
> +    } catch (e) {}

Why is the try/catch needed now? No catch logic needed?

Honza

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8990660 [details]
Bug 1474236 - Remove defer usage in JsTerm.js; .

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

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8990660 [details]
Bug 1474236 - Remove defer usage in JsTerm.js; .

https://reviewboard.mozilla.org/r/255740/#review262482

Looks good to me now.

R+ 

I have just the question from comment #2

> Why is the try/catch needed now? No catch logic needed?

Honza

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

> Why is the try/catch needed now? No catch logic needed?

Before there was this `.then(onResult, onResult)` which I thought could be replace by an empty catch block.
But I realize that might not be the case (because we don't call requestEvaluation if we fail before the call to it).

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8990660 [details]
Bug 1474236 - Remove defer usage in JsTerm.js; .

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/034e4cfc6349
Remove defer usage in JsTerm.js; r=Honza.

Comment 8

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/034e4cfc6349