Closed Bug 1408945 Opened 2 years ago Closed 2 years ago

Enable browser_webconsole_optimized_out_vars.js in the new console frontend

Categories

(DevTools :: Console, enhancement, P1)

enhancement

Tracking

(firefox59 fixed)

RESOLVED FIXED
Firefox 59
Tracking Status
firefox59 --- fixed

People

(Reporter: nchevobbe, Assigned: jdescottes)

References

(Blocks 1 open bug)

Details

(Whiteboard: [newconsole-mvp])

Attachments

(1 file)

No description provided.
Blocks: 1400847
Priority: P3 → P2
Whiteboard: [newconsole-mvp]
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Priority: P2 → P1
Comment on attachment 8931435 [details]
Bug 1408945 - enable browser_webconsole_optimized_out_vars.js;

https://reviewboard.mozilla.org/r/202580/#review208112

Thanks Julian, some comments to make it even better :)

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_optimized_out_vars.js:39
(Diff revision 1)
> -    hud.jsterm.execute("upvar");
> +  hud.jsterm.execute("upvar");
> -    yield waitForMessages({
> +  await waitFor(() => findMessage(hud, "optimized out", ".message"));

can we use waitForMessage instead ? waitFor is polling, and we should only use it when we don't know when/how the message is triggered.

const onOptimizedOutMessage = waitForMessage(hud, "optimized out"); // Ommit the third param since it defaults to ".message"
hud.jsterm.execute("upvar");
await onOptimizedOutMessage;
ok(true, "Optimized out message logged");

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_optimized_out_vars.js:46
(Diff revision 1)
>  
> -function ensureThreadClientState(aPanel, aState) {
> +// Debugger helper functions adapted from devtools/client/debugger/test/head.js.
> -  let thread = aPanel.panelWin.gThreadClient;
> -  let state = thread.state;
>  
> -  info("Thread is: '" + state + "'.");
> +async function ensureThreadClientState(panel, expectedState) {

nit: rename panel to debuggerPanel to make it obvious what panel refers to

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_optimized_out_vars.js:55
(Diff revision 1)
> +    await waitForThreadEvent(panel, expectedState);
>    }
> -  return waitForThreadEvents(aPanel, aState);
>  }
>  
> -function waitForThreadEvents(aPanel, aEventName, aEventRepeat = 1) {
> +function waitForThreadEvent(panel, eventName) {

nit: rename panel to debuggerPanel to make it obvious what panel refers to

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_optimized_out_vars.js:58
(Diff revision 1)
> -  thread.addListener(aEventName, function onEvent(eventName, ...args) {
> -    info("Thread event '" + eventName + "' fired: " + (++count) + " time(s).");
> +  return new Promise(resolve => {
> +    let thread = panel.panelWin.gThreadClient;
>  
> -    if (count == aEventRepeat) {
> +    thread.addListener(eventName, function onEvent() {
> -      ok(true, "Enough '" + eventName + "' thread events have been fired.");
>        thread.removeListener(eventName, onEvent);
> -      deferred.resolve.apply(deferred, args);
> -    }
> +      resolve();
> +    });
>    });

I think we can use addOneTimeListener, which returns a promise.
Attachment #8931435 - Flags: review?(nchevobbe) → review-
Comment on attachment 8931435 [details]
Bug 1408945 - enable browser_webconsole_optimized_out_vars.js;

https://reviewboard.mozilla.org/r/202580/#review208410

One minor comment, but this is good, thanks Julian

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_optimized_out_vars.js:55
(Diff revision 2)
>  
> -  if (state == aState) {
> -    return promise.resolve(null);
> +async function ensureThreadClientState(debuggerPanel, state) {
> +  let thread = debuggerPanel.panelWin.gThreadClient;
> +  info(`Thread is: '${thread.state}'.`);
> +  if (thread.state != state) {
> +    info("Waiting for thread event: '${eventName}'.");

I think `eventName` should be `state`
Attachment #8931435 - Flags: review?(nchevobbe) → review+
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7ef48daab640
enable browser_webconsole_optimized_out_vars.js;r=nchevobbe
https://hg.mozilla.org/mozilla-central/rev/7ef48daab640
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.