Closed Bug 1450145 Opened 6 years ago Closed 6 years ago

webconsole should make use of the debugger's original scope maps

Categories

(DevTools :: General, enhancement)

Product:
DevTools
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox61 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: loganfsmyth, Assigned: loganfsmyth)

Details

Attachments

(1 file, 2 obsolete files)

Bug 1450145 - Rewrite console expressions through the debugger panel when available;
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details 
Comment on attachment 8963808 [details]
Bug 1450145 - Rewrite console expressions through the debugger panel when available;

https://reviewboard.mozilla.org/r/232668/#review238220

Overall this is looking good and simple to me, thanks Logan :)
I'm only concerns about hitting getMappedExpression (and getting the debugger panel) for each evaluation as it might impact performances, but I'll profile this when the debugger bundle containing this function is available on mozilla-central

::: devtools/client/debugger/new/test/mochitest/browser_dbg-babel-breakpoint-console.js:28
(Diff revision 3)
> +
> +  const invokeResult = invokeInTab(fnName);
> +
> +  let invokeFailed = await Promise.race([
> +    waitForPaused(dbg),
> +    invokeResult.then(() => new Promise(() => {}), () => true)

if I understand correctly, we wait until:  
- waitForPaused resolves
- OR invokeResult fails

if that's the case, maybe we could use invokeResult.catch(() => true) since it's a bit more terse and better conveys what we are waiting for

::: devtools/client/debugger/new/test/mochitest/browser_dbg-babel-breakpoint-console.js:55
(Diff revision 3)
> +    await dbg.client.evaluate(`
> +      window.TEST_RESULT = false;
> +    `);
> +    await jsterm.execute(`
> +      TEST_RESULT = ${statement};
> +    `);
> +
> +    const result = await dbg.client.evaluate(`window.TEST_RESULT`);

I think we could simplify that by doing 
```js
const result = jsterm.execute(statement)
```

::: devtools/client/debugger/new/test/mochitest/browser_dbg-babel-breakpoint-console.js:63
(Diff revision 3)
> +    await jsterm.execute(`
> +      TEST_RESULT = ${statement};
> +    `);
> +
> +    const result = await dbg.client.evaluate(`window.TEST_RESULT`);
> +    is(result.result, true);

nit: Add a description message so when the test fails we know which statement is involved
 ```js
is(result, true, `'${statement}' evaluates to true`)
```

::: devtools/client/debugger/new/test/mochitest/browser_dbg-babel-breakpoint-console.js:68
(Diff revision 3)
> +    is(result.result, true);
> +  }
> +
> +}
> +
> +add_task(async function() {

nit: I like to have the add_task block at the beginning of the file so I can have a global sense of what it does. But since it's in the debugger folder, let's follow the convention we use there.

::: devtools/client/webconsole/hudservice.js:496
(Diff revision 3)
> +
> +    if (!panel) {
> +      return expression;
> +    }
> +
> +    return panel.getMappedExpression(expression);

I find a bit weird to get the whole debugger panel to get the getMappedExpression function.
could we have a reference to it in the  WebConsole object maybe ?
Also, are we going to hit this function each time we evaluate something in the console, even if the debugger isn't paused ?

::: devtools/client/webconsole/jsterm.js:439
(Diff revision 3)
>     *        Optional function to invoke when the result is displayed.
>     *        This is deprecated - please use the promise return value instead.
>     * @returns Promise
>     *          Resolves with the message once the result is displayed.
>     */
> -  execute: function(executeString, callback) {
> +  execute: async function(executeString, callback) {

I wonder if switching to an async function might have side effects on other parts of the code. We should make sure tests are green once the debugger bundle lands.
Good point on the performance implications, we can probably improve this a bit:

1. only check if we're paused
2. only check if we're paused in an original location. We can teach the source-map-worker how to do this.

Perhaps these items are follow ups once we land this.

By the way, we looked into autocomplete for original expressions and that is possible but non-trivial
Comment on attachment 8963808 [details]
Bug 1450145 - Rewrite console expressions through the debugger panel when available;

https://reviewboard.mozilla.org/r/232668/#review238220

Cool. If the debugger panel exists, the performance should essentially be tied to the parse time of the snippet that the user is evaluating, so I don't anticipate it being noticable for the user. We take the value, send it to a worker thread for parsing and rewriting, then send it back. If the panel hasn't been opened yet, we won't try to open it, so it should only be even slightly noticable if the user had just kicked off parsing of a larger file that would block the worker for a bit, which I'd guess is relatively uncommon.

> if I understand correctly, we wait until:  
> - waitForPaused resolves
> - OR invokeResult fails
> 
> if that's the case, maybe we could use invokeResult.catch(() => true) since it's a bit more terse and better conveys what we are waiting for

FYI, I don't know that it's worth you reviewing this function because it already exists in some of the other test files and I'll probably move it to HEAD soon, I just wanted to keep the changes in this PR as minimal as I could. To answer your question though:

> if I understand correctly

The intention of this check is essentially to say, if the `invokeResult` rejects immediately, catch it and fail, otherwise do nothing. The `invokeResult` promise should not have resolved at this point, if things worked properly. This is essentially to handle the case where the function that was invoked through an immediate error instead of hitting the breakpoint.

> I think we could simplify that by doing 
> ```js
> const result = jsterm.execute(statement)
> ```

The return value of `.execute` is the console message, not the evaluated value. Since I'm checking for the boolean value here, there didn't seem to be a good way to do that other than relying on side-effects.

> nit: Add a description message so when the test fails we know which statement is involved
>  ```js
> is(result, true, `'${statement}' evaluates to true`)
> ```

Will do!

> I find a bit weird to get the whole debugger panel to get the getMappedExpression function.
> could we have a reference to it in the  WebConsole object maybe ?
> Also, are we going to hit this function each time we evaluate something in the console, even if the debugger isn't paused ?

> could we have a reference to it in the  WebConsole object maybe ?

This makes use of all of the debugger's logic around sourcemap processing, based on the current paused location. It seems like there isn't anything small enough to load individually.

> are we going to hit this function each time we evaluate something in the console, even if the debugger isn't paused

If the panel hasn't been opened yet, or things aren't paused, this should be very fast. It'll essentially be dispatching an action that returns immediately, so I wouldn't expect it to be more that a couple turns of the even loop.

> I wonder if switching to an async function might have side effects on other parts of the code. We should make sure tests are green once the debugger bundle lands.

Yeah we'll definitely have to see how the tests look. `.execute` already returns a promise at least, so it should only be an issue if something was relying on synchronous side-effects from `.execute`.
Attachment #8963807 - Attachment is obsolete: true
Attachment #8963820 - Attachment is obsolete: true
Comment on attachment 8963808 [details]
Bug 1450145 - Rewrite console expressions through the debugger panel when available;

https://reviewboard.mozilla.org/r/232668/#review238220

> The return value of `.execute` is the console message, not the evaluated value. Since I'm checking for the boolean value here, there didn't seem to be a good way to do that other than relying on side-effects.

oh right (and it seems a bit weird to me, we'll probably change this soon)
Comment on attachment 8963808 [details]
Bug 1450145 - Rewrite console expressions through the debugger panel when available;

https://reviewboard.mozilla.org/r/232668/#review238692
Attachment #8963808 - Flags: review?(nchevobbe) → review+
Comment on attachment 8963808 [details]
Bug 1450145 - Rewrite console expressions through the debugger panel when available;

https://reviewboard.mozilla.org/r/232668/#review238796

::: devtools/client/webconsole/hudservice.js:496
(Diff revision 4)
> +
> +    if (!panel) {
> +      return expression;
> +    }
> +
> +    return panel.getMappedExpression(expression);

some mochitests fail because the old debugger panel does not have getMappedExpression, we can do one of two things:

1. check if the panel has getMappedExpression here
2. add a no-op getMappedExpression to the old debugger panel
Updated with option 2, added a no-op to the old panel.

Also did a pass over the tests to fix the ones I could see had other issues. Still certainly possible there are tests I missed though, so we shall see.
Updated with I think the last of the mochitest issues.

I also tweaked the `execute` logic a little more so that the history-update runs before we `await` so all of that will happen synchronously instead of partially-synchronous, partially-asynchronous.
Assignee: nobody → loganfsmyth
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 8963808 [details]
Bug 1450145 - Rewrite console expressions through the debugger panel when available;

https://reviewboard.mozilla.org/r/232668/#review239086

Looks good to me ! Thanks Logan for fixing those tests

::: devtools/client/debugger/panel.js:135
(Diff revision 6)
> +    // No-op implementation since this feature doesn't exist in the older
> +    // debugger implementation.

nit: We should say that the function was added in the new debugger panel. Here it's not crystal clear
As discussed with Jason on slack, let's not change this comment, since the old debugger panel will be deleted soonish
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/450636c02e4d
Rewrite console expressions through the debugger panel when available; r=nchevobbe
https://hg.mozilla.org/mozilla-central/rev/450636c02e4d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Thanks for reviewing!
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: